mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-08-02 09:18:06 +00:00
fix(sports): harden the B2/B3 capabilities against bad config and subclasses
Review pass on the phase B2-B4 changes. Every fix here is the same shape
as the crashes this PR already fixed in the hockey and baseball
extractors: a config or feed value that is present-but-wrong reaching
arithmetic or a comparison on a path with no guard.
celebrations:
- celebration_duration is coerced and floored at init. It is compared
numerically in display() *outside* any try block, so a string from a
hand-edited config propagated a TypeError straight out; zero or
negative armed a celebration that could never render.
- A render failure now disarms instead of staying armed. It previously
retried the same broken render on every frame for the rest of the
window -- a traceback per frame, and no scorebug either.
- prune_score_baselines() for the live set. Only _check_for_win removed
entries, so a game that left the live list any other way leaked its
baseline and the dict grew all season.
- display() reuses has_active_celebration() rather than repeating its
window comparison, and log lines carry a [Celebrations] prefix.
rotation:
- MAX_WEIGHT ceiling. A cycle is sum(weights) long and each step scans
every game, so an unbounded weight from a misread config spins the
display thread -- on a Pi that stalls rendering outright.
- register_rotation_strategy rejects a non-subclass factory at
registration instead of failing frames later inside schedule().
- schedule() previews through type(self), so a subclass overriding
next_game is previewed with its own ordering -- which is what the
method promises.
sports_scroll:
- scroll_speed / scroll_delay coerced. dict.get(key, default) only helps
when the key is absent; present-but-null reached the multiplication
inside __init__ and the display failed to construct at all.
- update_scroll_position and get_visible_portion moved inside the try.
They ran outside it, so a raise there reached the plugin's frame loop
despite the comment promising none can.
- prepare_and_display guards the subclass call, so one sport's bad
payload cannot take down the shared orchestration for the others.
- _current_game_type spells "nothing active" as "" in both classes; the
manager said None while the display said "".
Not taken: the report that baseball's favorite-team debug path still
reads event-level status. Verified against current code -- there are no
remaining game_event["status"] reads in that file; it was fixed in
2486bdb and the finding is stale.
Gates: 747 core unit, 66 plugin safety.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FgbA8SMutQQpXkMG8LMmC4
This commit is contained in:
@@ -86,6 +86,17 @@ class TestRegistry:
|
||||
with pytest.raises(ValueError):
|
||||
register_rotation_strategy("", SimpleRotation)
|
||||
|
||||
@pytest.mark.parametrize("bad", [
|
||||
SimpleRotation(), # an instance, not the class
|
||||
str, # unrelated class
|
||||
lambda **kw: None, # a factory function
|
||||
])
|
||||
def test_a_non_strategy_factory_is_rejected(self, bad):
|
||||
"""Fail at registration, not several frames later inside schedule(),
|
||||
where the cause is no longer on the stack."""
|
||||
with pytest.raises(TypeError):
|
||||
register_rotation_strategy("bad-factory", bad)
|
||||
|
||||
def test_weight_for_is_optional(self):
|
||||
"""Default weights are equal, so every strategy degenerates to a plain
|
||||
round robin — the pre-boost behavior."""
|
||||
@@ -111,6 +122,17 @@ class TestWeights:
|
||||
strategy = get_rotation_strategy("weighted", weight_for=lambda g: bad)
|
||||
assert strategy.weights([game("a")]) == {"a": 1}
|
||||
|
||||
def test_huge_weights_are_clamped(self):
|
||||
"""A cycle is sum(weights) long and each step scans every game, so an
|
||||
unbounded weight from a misread config spins the display thread."""
|
||||
strategy = get_rotation_strategy("weighted", weight_for=lambda g: 10_000)
|
||||
assert strategy.weights([game("a")]) == {"a": RotationStrategy.MAX_WEIGHT}
|
||||
|
||||
def test_a_clamped_cycle_stays_bounded(self):
|
||||
strategy = get_rotation_strategy("weighted", weight_for=lambda g: 10_000)
|
||||
order = strategy.schedule([game("a"), game("b")])
|
||||
assert len(order) == 2 * RotationStrategy.MAX_WEIGHT
|
||||
|
||||
|
||||
class TestSimpleRotation:
|
||||
def test_one_pass_in_feed_order(self):
|
||||
@@ -218,6 +240,20 @@ class TestSmoothWeightedRotation:
|
||||
actual = [strategy.next_game(games)["id"] for _ in range(len(preview))]
|
||||
assert preview == actual
|
||||
|
||||
def test_preview_uses_the_subclass_ordering(self):
|
||||
"""schedule() promises the order repeated next_game calls produce. Built
|
||||
from the base class, a subclass that overrides next_game gets a preview
|
||||
of the wrong algorithm."""
|
||||
class Reversed(SmoothWeightedRotation):
|
||||
def next_game(self, games):
|
||||
return super().next_game(list(reversed(games)))
|
||||
|
||||
games = [game("a"), game("b"), game("c")]
|
||||
strategy = Reversed()
|
||||
preview = strategy.schedule(games)
|
||||
actual = [strategy.next_game(games)["id"] for _ in range(len(preview))]
|
||||
assert preview == actual
|
||||
|
||||
def test_empty(self):
|
||||
assert SmoothWeightedRotation().next_game([]) is None
|
||||
assert SmoothWeightedRotation().schedule([]) == []
|
||||
@@ -462,6 +498,24 @@ class TestCelebrationConfig:
|
||||
"celebrate_opponent_goals": True})
|
||||
assert manager.celebrate_opponent_scores is False
|
||||
|
||||
@pytest.mark.parametrize("bad", ["eight", None, {}, []])
|
||||
def test_unusable_duration_falls_back(self, celebrating, bad):
|
||||
"""The duration is compared numerically on the display path, outside
|
||||
any try block — a string from a hand-edited config would propagate a
|
||||
TypeError straight out of display()."""
|
||||
manager = celebrating(mode_config={"celebration_duration": bad})
|
||||
assert manager.celebration_duration == 8.0
|
||||
|
||||
@pytest.mark.parametrize("bad", [0, -5])
|
||||
def test_non_positive_duration_is_floored(self, celebrating, bad):
|
||||
"""Zero or negative would arm a celebration that can never render."""
|
||||
manager = celebrating(mode_config={"celebration_duration": bad})
|
||||
assert manager.celebration_duration == 1.0
|
||||
|
||||
def test_numeric_string_duration_is_accepted(self, celebrating):
|
||||
assert celebrating(
|
||||
mode_config={"celebration_duration": "12"}).celebration_duration == 12.0
|
||||
|
||||
|
||||
class TestScoreDetection:
|
||||
def test_first_sighting_never_celebrates(self, celebrating):
|
||||
@@ -725,6 +779,52 @@ class TestDisplayTakeover:
|
||||
assert manager.display() is True
|
||||
assert manager.display_calls == [False]
|
||||
|
||||
def test_a_render_failure_disarms_rather_than_retrying(self, celebrating):
|
||||
"""Left armed, the same render fails on every frame for the rest of the
|
||||
window — a traceback per frame, and no scorebug."""
|
||||
manager = celebrating()
|
||||
manager._check_for_score(game("g1", home_score=0))
|
||||
manager._check_for_score(game("g1", home_score=1))
|
||||
manager._draw_celebration_layout = MagicMock(side_effect=RuntimeError("boom"))
|
||||
manager.display()
|
||||
assert manager.active_celebration is None
|
||||
manager.display()
|
||||
assert manager._draw_celebration_layout.call_count == 1
|
||||
|
||||
|
||||
class TestBaselinePruning:
|
||||
"""`_score_baselines` gains an entry per game and only _check_for_win ever
|
||||
removed one, so a board running all season grows the dict without bound."""
|
||||
|
||||
def test_prunes_games_no_longer_live(self, celebrating):
|
||||
manager = celebrating()
|
||||
for gid in ("g1", "g2", "g3"):
|
||||
manager._check_for_score(game(gid, home_score=1))
|
||||
manager.prune_score_baselines([game("g2")])
|
||||
assert set(manager._score_baselines) == {"g2"}
|
||||
|
||||
def test_keeps_every_still_live_game(self, celebrating):
|
||||
manager = celebrating()
|
||||
for gid in ("g1", "g2"):
|
||||
manager._check_for_score(game(gid, home_score=1))
|
||||
manager.prune_score_baselines([game("g1"), game("g2")])
|
||||
assert set(manager._score_baselines) == {"g1", "g2"}
|
||||
|
||||
def test_empty_live_set_clears_everything(self, celebrating):
|
||||
manager = celebrating()
|
||||
manager._check_for_score(game("g1", home_score=1))
|
||||
manager.prune_score_baselines([])
|
||||
assert manager._score_baselines == {}
|
||||
|
||||
def test_pruning_does_not_disturb_a_surviving_baseline(self, celebrating):
|
||||
manager = celebrating()
|
||||
manager._check_for_score(game("g1", home_score=2))
|
||||
manager.prune_score_baselines([game("g1")])
|
||||
manager._check_for_score(game("g1", home_score=3))
|
||||
assert manager.active_celebration is not None, (
|
||||
"pruning must not drop a live game's baseline and re-trigger the "
|
||||
"first-sighting suppression")
|
||||
|
||||
def test_disabled_manager_renders_nothing(self, celebrating):
|
||||
manager = celebrating()
|
||||
manager.is_enabled = False
|
||||
|
||||
@@ -178,7 +178,8 @@ class TestScrollSettings:
|
||||
def test_a_null_league_block_is_tolerated(self, build):
|
||||
"""`config['nhl'] = None` appears in hand-edited configs."""
|
||||
display = build({"nhl": None})
|
||||
assert display._get_scroll_settings()["scroll_speed"] == 50.0
|
||||
assert (display._get_scroll_settings()["scroll_speed"]
|
||||
== DEFAULT_SCROLL_SETTINGS["scroll_speed"])
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
@@ -245,6 +246,25 @@ class TestConfigureScrollHelper:
|
||||
display = build(global_config={"target_fps": "120"})
|
||||
display.scroll_helper.set_target_fps.assert_called_once_with(120.0)
|
||||
|
||||
@pytest.mark.parametrize("bad", [None, "fast", {}, []])
|
||||
def test_unusable_scroll_speed_degrades_instead_of_crashing(self, build, bad):
|
||||
"""`.get(key, default)` only helps when the key is *absent*. A key
|
||||
present with null reaches the arithmetic and raises inside __init__,
|
||||
taking the whole display down before it renders anything."""
|
||||
display = build({"nhl": {"scroll_settings": {"scroll_speed": bad}}})
|
||||
# 50.0 px/s * 0.01 s/frame == 0.5 px/frame, i.e. the default speed.
|
||||
display.scroll_helper.set_scroll_speed.assert_called_with(0.5)
|
||||
|
||||
@pytest.mark.parametrize("bad", [None, "slow", {}])
|
||||
def test_unusable_scroll_delay_degrades_instead_of_crashing(self, build, bad):
|
||||
display = build({"nhl": {"scroll_settings": {"scroll_delay": bad}}})
|
||||
display.scroll_helper.set_scroll_delay.assert_called_with(0.01)
|
||||
|
||||
def test_numeric_strings_are_accepted(self, build):
|
||||
display = build({"nhl": {"scroll_settings": {
|
||||
"scroll_speed": "100", "scroll_delay": "0.02"}}})
|
||||
display.scroll_helper.set_scroll_speed.assert_called_with(2.0)
|
||||
|
||||
def test_fps_clamping_is_left_to_the_helper(self, build):
|
||||
"""Deliberately not clamped here — a second copy of the range would
|
||||
drift from ScrollHelper.set_target_fps."""
|
||||
@@ -308,6 +328,16 @@ class TestFramePumping:
|
||||
display.display_manager.update_display.side_effect = RuntimeError("boom")
|
||||
assert display.display_scroll_frame() is False
|
||||
|
||||
@pytest.mark.parametrize("failing", ["update_scroll_position",
|
||||
"get_visible_portion"])
|
||||
def test_a_scroll_helper_failure_is_contained_too(self, build, failing):
|
||||
"""These ran outside the try, so a raise there reached the caller's
|
||||
frame loop despite the stated promise that none can."""
|
||||
display = build()
|
||||
display.prepare_scroll_content([{"id": "g1"}], "live", ["nhl"])
|
||||
getattr(display.scroll_helper, failing).side_effect = RuntimeError("boom")
|
||||
assert display.display_scroll_frame() is False
|
||||
|
||||
def test_frames_are_counted(self, build):
|
||||
display = build()
|
||||
display.prepare_scroll_content([{"id": "g1"}], "live", ["nhl"])
|
||||
@@ -407,7 +437,23 @@ class TestManager:
|
||||
|
||||
def test_failed_prepare_does_not_become_active(self, manager):
|
||||
assert manager.prepare_and_display([], "live", ["nhl"]) is False
|
||||
assert manager._current_game_type is None
|
||||
assert not manager._current_game_type
|
||||
|
||||
def test_a_raising_subclass_does_not_escape_the_orchestration(self, manager):
|
||||
"""prepare_scroll_content is subclass code building cards from feed
|
||||
data. One sport's bad payload must not take down the others."""
|
||||
display = manager.get_scroll_display("live")
|
||||
display.prepare_scroll_content = MagicMock(side_effect=KeyError("status"))
|
||||
assert manager.prepare_and_display([{"id": "g1"}], "live", ["nhl"]) is False
|
||||
assert not manager._current_game_type
|
||||
|
||||
def test_empty_game_type_sentinel_matches_the_display(self, manager):
|
||||
"""Both classes must spell 'nothing active' the same way; two spellings
|
||||
across two classes is a trap for anyone comparing their state."""
|
||||
manager.prepare_and_display([{"id": "g1"}], "live", ["nhl"])
|
||||
manager.clear_all()
|
||||
assert (manager._current_game_type
|
||||
== manager.get_scroll_display("live")._current_game_type == "")
|
||||
|
||||
def test_display_frame_uses_the_active_type(self, manager):
|
||||
manager.prepare_and_display([{"id": "g1"}], "live", ["nhl"])
|
||||
@@ -437,7 +483,7 @@ class TestManager:
|
||||
manager.prepare_and_display([{"id": "g1"}], "live", ["nhl"])
|
||||
manager.prepare_and_display([{"id": "g2"}], "recent", ["nhl"])
|
||||
manager.clear_all()
|
||||
assert manager._current_game_type is None
|
||||
assert not manager._current_game_type
|
||||
for game_type in ("live", "recent"):
|
||||
assert manager.get_scroll_display(game_type)._current_games == []
|
||||
|
||||
@@ -526,6 +572,11 @@ class TestAgainstTheRealScrollHelper:
|
||||
real.display_frame()
|
||||
time.sleep(0.0012)
|
||||
|
||||
# Asserted separately so a host too slow to sustain the frame rate
|
||||
# reports a timeout rather than looking like a scrolling defect.
|
||||
assert time.time() < deadline, (
|
||||
"scroll did not finish within 20s — the host may be too slow to "
|
||||
"sustain the configured frame rate")
|
||||
assert real.is_complete() is True
|
||||
assert display.scroll_helper.scroll_position > 128
|
||||
|
||||
|
||||
Reference in New Issue
Block a user