mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-08-23 11:28:14 +00:00
ChuckBuildsandClaude Opus 5
094090d138
Addresses the second review finding: a lost update on _pending_plugin_reconcile. The flag was cleared after a successful reconcile. Reconcile has already read its config by that point, so a config change arriving mid-flight set a flag that the trailing clear then erased -- a request that was never served, and the newest config never reconciled. That is the same "my save did nothing" symptom this PR exists to remove, so leaving it would have undercut the fix. Consume the request before running it instead, and re-arm only on a retryable failure. A change that lands during reconcile now stays set and is picked up on the next pass. The per-frame read stays lock-free. It is a fast path that can only produce a false negative -- the watcher setting the flag just after it is read is seen on the next iteration -- never a false positive that loses a request. The lock is taken only when a reconcile is actually pending or a config change arrives. Extracted _service_pending_reconcile() so the sequence is testable rather than buried in run()'s loop; the review asked for a regression test that invokes the subscriber during reconciliation, which is not reachable otherwise. Tests: 4 new, covering a request racing in mid-reconcile, the quiet success, the retryable-failure re-arm, and not reconciling when nothing is pending. Two of them fail against the previous clear-after-success semantics. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW
436 lines
20 KiB
Python
436 lines
20 KiB
Python
"""Tests for live plugin enable/disable hot-reload in DisplayController.
|
|
|
|
Enabling or disabling a plugin in config used to require a full display
|
|
restart because the plugin list and available_modes were built once at init.
|
|
These tests cover the reconcile path that loads/unloads plugins and rebuilds
|
|
the dispatch maps on the main thread when the enabled set changes.
|
|
"""
|
|
|
|
import copy
|
|
from unittest.mock import MagicMock
|
|
|
|
|
|
def _make_plugin(modes):
|
|
plugin = MagicMock()
|
|
plugin.modes = list(modes)
|
|
return plugin
|
|
|
|
|
|
def _wire_plugin_manager(controller, plugins, discovered=None):
|
|
"""Point the controller's mock plugin_manager at a set of fake plugins.
|
|
|
|
`plugins` maps plugin_id -> mock instance (with a .modes list).
|
|
"""
|
|
pm = controller.plugin_manager
|
|
pm.discover_plugins.return_value = list(discovered if discovered is not None else plugins.keys())
|
|
pm.load_plugin.return_value = True
|
|
pm.unload_plugin.return_value = True
|
|
pm.plugin_manifests = {}
|
|
pm.get_plugin.side_effect = lambda pid: plugins.get(pid)
|
|
return pm
|
|
|
|
|
|
def _set_config(controller, cfg):
|
|
controller.config_service.get_config = lambda: cfg
|
|
|
|
|
|
class TestPluginEnableDisableHotReload:
|
|
def test_enable_plugin_live(self, test_display_controller):
|
|
controller = test_display_controller
|
|
assert controller.available_modes == []
|
|
|
|
plugin = _make_plugin(["foo"])
|
|
_wire_plugin_manager(controller, {"foo": plugin})
|
|
_set_config(controller, {"foo": {"enabled": True}})
|
|
|
|
controller._reconcile_enabled_plugins()
|
|
|
|
assert "foo" in controller.plugin_display_modes
|
|
assert "foo" in controller.available_modes
|
|
assert controller.plugin_modes["foo"] is plugin
|
|
assert controller.mode_to_plugin_id["foo"] == "foo"
|
|
controller.plugin_manager.load_plugin.assert_any_call("foo")
|
|
|
|
def test_disable_plugin_live(self, test_display_controller):
|
|
controller = test_display_controller
|
|
plugin = _make_plugin(["live", "recent"])
|
|
_wire_plugin_manager(controller, {"sports": plugin}, discovered=["sports"])
|
|
|
|
# Enable, then disable.
|
|
_set_config(controller, {"sports": {"enabled": True}})
|
|
controller._reconcile_enabled_plugins()
|
|
assert "sports" in controller.plugin_display_modes
|
|
assert "live" in controller.available_modes and "recent" in controller.available_modes
|
|
assert "sports" in controller._plugin_config_callbacks
|
|
|
|
_set_config(controller, {"sports": {"enabled": False}})
|
|
controller._reconcile_enabled_plugins()
|
|
|
|
assert "sports" not in controller.plugin_display_modes
|
|
assert "live" not in controller.available_modes
|
|
assert "recent" not in controller.available_modes
|
|
assert "live" not in controller.plugin_modes
|
|
assert "recent" not in controller.mode_to_plugin_id
|
|
controller.plugin_manager.unload_plugin.assert_any_call("sports")
|
|
assert "sports" not in controller._plugin_config_callbacks
|
|
|
|
def test_disable_clamps_current_mode_index(self, test_display_controller):
|
|
controller = test_display_controller
|
|
p1 = _make_plugin(["a"])
|
|
p2 = _make_plugin(["b"])
|
|
_wire_plugin_manager(controller, {"p1": p1, "p2": p2}, discovered=["p1", "p2"])
|
|
|
|
_set_config(controller, {"p1": {"enabled": True}, "p2": {"enabled": True}})
|
|
controller._reconcile_enabled_plugins()
|
|
# Add order across multiple plugins is set-driven (as at init), so
|
|
# compare membership, not order.
|
|
assert set(controller.available_modes) == {"a", "b"}
|
|
|
|
# Pretend we're currently showing p2's mode.
|
|
controller.current_mode_index = controller.available_modes.index("b")
|
|
controller.current_display_mode = "b"
|
|
|
|
_set_config(controller, {"p1": {"enabled": True}, "p2": {"enabled": False}})
|
|
controller._reconcile_enabled_plugins()
|
|
|
|
assert controller.available_modes == ["a"]
|
|
# Index must be back in range and the display mode no longer the removed one.
|
|
assert 0 <= controller.current_mode_index < len(controller.available_modes)
|
|
assert controller.current_display_mode == "a"
|
|
|
|
def test_enable_keeps_current_mode(self, test_display_controller):
|
|
controller = test_display_controller
|
|
p1 = _make_plugin(["a"])
|
|
p2 = _make_plugin(["b"])
|
|
_wire_plugin_manager(controller, {"p1": p1, "p2": p2}, discovered=["p1", "p2"])
|
|
|
|
_set_config(controller, {"p1": {"enabled": True}})
|
|
controller._reconcile_enabled_plugins()
|
|
controller.current_mode_index = 0
|
|
controller.current_display_mode = "a"
|
|
|
|
# Enabling p2 should not disturb the currently-showing mode.
|
|
_set_config(controller, {"p1": {"enabled": True}, "p2": {"enabled": True}})
|
|
controller._reconcile_enabled_plugins()
|
|
|
|
assert "b" in controller.available_modes
|
|
assert controller.current_display_mode == "a"
|
|
assert controller.available_modes[controller.current_mode_index] == "a"
|
|
|
|
def test_noop_when_enabled_set_unchanged(self, test_display_controller):
|
|
controller = test_display_controller
|
|
plugin = _make_plugin(["foo"])
|
|
_wire_plugin_manager(controller, {"foo": plugin}, discovered=["foo"])
|
|
_set_config(controller, {"foo": {"enabled": True}})
|
|
controller._reconcile_enabled_plugins()
|
|
|
|
load_calls = controller.plugin_manager.load_plugin.call_count
|
|
unload_calls = controller.plugin_manager.unload_plugin.call_count
|
|
|
|
# Reconcile again with no change — must not load/unload anything.
|
|
controller._reconcile_enabled_plugins()
|
|
assert controller.plugin_manager.load_plugin.call_count == load_calls
|
|
assert controller.plugin_manager.unload_plugin.call_count == unload_calls
|
|
|
|
def test_reconcile_ignores_non_dict_config_value(self, test_display_controller, caplog):
|
|
"""A malformed config value (e.g. a stray string where a plugin's
|
|
section should be a dict) must be treated as disabled, not crash
|
|
the reconcile with AttributeError, and should be logged so it's
|
|
visible to whoever has to debug the malformed config."""
|
|
controller = test_display_controller
|
|
plugin = _make_plugin(["foo"])
|
|
_wire_plugin_manager(controller, {"foo": plugin}, discovered=["foo"])
|
|
_set_config(controller, {"foo": "not-a-dict"})
|
|
|
|
with caplog.at_level("WARNING"):
|
|
controller._reconcile_enabled_plugins() # must not raise
|
|
|
|
assert "foo" not in controller.plugin_display_modes
|
|
assert "foo" not in controller.available_modes
|
|
assert any("foo" in r.message and "not a dict" in r.message for r in caplog.records)
|
|
|
|
def test_disable_keeps_callback_when_unsubscribe_fails(self, test_display_controller):
|
|
"""If config_service.unsubscribe() raises, _unregister_plugin must
|
|
keep the callback in _plugin_config_callbacks rather than losing the
|
|
only reference to it (it still tears down the plugin itself)."""
|
|
controller = test_display_controller
|
|
plugin = _make_plugin(["live"])
|
|
_wire_plugin_manager(controller, {"sports": plugin}, discovered=["sports"])
|
|
|
|
_set_config(controller, {"sports": {"enabled": True}})
|
|
controller._reconcile_enabled_plugins()
|
|
assert "sports" in controller._plugin_config_callbacks
|
|
|
|
controller.config_service.unsubscribe = MagicMock(side_effect=RuntimeError("boom"))
|
|
|
|
_set_config(controller, {"sports": {"enabled": False}})
|
|
controller._reconcile_enabled_plugins()
|
|
|
|
assert "sports" not in controller.plugin_display_modes
|
|
assert "sports" in controller._plugin_config_callbacks
|
|
|
|
|
|
class TestReconcileReturnValue:
|
|
"""_reconcile_enabled_plugins() returns True/False so the caller (run()'s
|
|
loop) only clears _pending_plugin_reconcile on success, keeping a
|
|
retryable failure's request alive instead of silently dropping it."""
|
|
|
|
def test_returns_true_on_success(self, test_display_controller):
|
|
controller = test_display_controller
|
|
plugin = _make_plugin(["foo"])
|
|
_wire_plugin_manager(controller, {"foo": plugin}, discovered=["foo"])
|
|
_set_config(controller, {"foo": {"enabled": True}})
|
|
assert controller._reconcile_enabled_plugins() is True
|
|
|
|
def test_returns_true_for_noop(self, test_display_controller):
|
|
controller = test_display_controller
|
|
_wire_plugin_manager(controller, {}, discovered=[])
|
|
_set_config(controller, {})
|
|
assert controller._reconcile_enabled_plugins() is True
|
|
|
|
def test_returns_false_on_discovery_failure(self, test_display_controller):
|
|
controller = test_display_controller
|
|
controller.plugin_manager.discover_plugins.side_effect = RuntimeError("boom")
|
|
_set_config(controller, {})
|
|
assert controller._reconcile_enabled_plugins() is False
|
|
|
|
def test_returns_true_when_no_plugin_manager(self, test_display_controller):
|
|
controller = test_display_controller
|
|
controller.plugin_manager = None
|
|
assert controller._reconcile_enabled_plugins() is True
|
|
|
|
|
|
class TestRunWithNoModesEnabled:
|
|
"""Before hot-reload, an empty available_modes at startup was permanent
|
|
-- the display never came back without a restart. Now that a plugin can
|
|
be enabled live from the web UI, run() must idle rather than exit."""
|
|
|
|
def test_idles_instead_of_exiting(self, test_display_controller):
|
|
controller = test_display_controller
|
|
assert controller.available_modes == []
|
|
|
|
sleep_calls = []
|
|
|
|
def fake_sleep(duration, tick_interval=1.0):
|
|
sleep_calls.append(duration)
|
|
if len(sleep_calls) >= 3:
|
|
# Stand in for the process being torn down; run() catches
|
|
# this via its broad except + finally, same as any other
|
|
# unexpected error during the loop.
|
|
raise RuntimeError("stop-test-loop")
|
|
|
|
controller._sleep_with_plugin_updates = fake_sleep
|
|
|
|
controller.run()
|
|
|
|
# Old behavior returned before ever reaching the loop body, so
|
|
# _sleep_with_plugin_updates would never have been called. The idle
|
|
# tick is short (not a long sleep) so a plugin enabled via the web
|
|
# UI while idle is picked up about as promptly as it would be once
|
|
# modes exist and the loop is iterating per-frame.
|
|
assert sleep_calls == [1, 1, 1]
|
|
|
|
|
|
class TestEnabledSetChanged:
|
|
def test_detects_toggle(self, test_display_controller):
|
|
c = test_display_controller
|
|
assert c._enabled_set_changed({"a": {"enabled": True}}, {"a": {"enabled": False}}) is True
|
|
|
|
def test_no_change(self, test_display_controller):
|
|
c = test_display_controller
|
|
cfg = {"a": {"enabled": True}, "b": {"enabled": False}}
|
|
assert c._enabled_set_changed(cfg, dict(cfg)) is False
|
|
|
|
def test_new_enabled_section(self, test_display_controller):
|
|
c = test_display_controller
|
|
assert c._enabled_set_changed(
|
|
{"a": {"enabled": True}},
|
|
{"a": {"enabled": True}, "b": {"enabled": True}},
|
|
) is True
|
|
|
|
def test_ignores_non_enabled_value_edits(self, test_display_controller):
|
|
c = test_display_controller
|
|
assert c._enabled_set_changed(
|
|
{"a": {"enabled": True, "duration": 30}},
|
|
{"a": {"enabled": True, "duration": 45}},
|
|
) is False
|
|
|
|
|
|
class TestEnabledPluginNotRunning:
|
|
"""A plugin that fails validate_config() is enabled but absent, and the
|
|
config edit that fixes it is nested inside the plugin's own section -- so
|
|
the top-level ``enabled`` comparison never sees it. These cover the second
|
|
gate that queues a reconcile in that case.
|
|
"""
|
|
|
|
def test_nested_edit_is_invisible_to_the_enabled_set_check(self, test_display_controller):
|
|
"""The original gate: proves why a second one is needed."""
|
|
controller = test_display_controller
|
|
old = {"hockey-scoreboard": {"enabled": True, "nhl": {"enabled": False}}}
|
|
new = {"hockey-scoreboard": {"enabled": True, "nhl": {"enabled": True}}}
|
|
# Enabling a league changes no top-level flag.
|
|
assert controller._enabled_set_changed(old, new) is False
|
|
|
|
def test_queues_reconcile_when_enabled_plugin_is_absent(self, test_display_controller):
|
|
controller = test_display_controller
|
|
controller.plugin_manager.plugin_manifests = {"hockey-scoreboard": {}}
|
|
controller.plugin_manager.discovered_plugin_ids.return_value = {"hockey-scoreboard"}
|
|
controller.plugin_display_modes = {} # failed to load
|
|
cfg = {"hockey-scoreboard": {"enabled": True, "nhl": {"enabled": True}}}
|
|
assert controller._enabled_plugin_not_running(cfg) is True
|
|
|
|
def test_quiet_when_every_enabled_plugin_is_running(self, test_display_controller):
|
|
controller = test_display_controller
|
|
controller.plugin_manager.plugin_manifests = {"hockey-scoreboard": {}}
|
|
controller.plugin_manager.discovered_plugin_ids.return_value = {"hockey-scoreboard"}
|
|
controller.plugin_display_modes = {"hockey-scoreboard": ["nhl"]}
|
|
cfg = {"hockey-scoreboard": {"enabled": True}}
|
|
assert controller._enabled_plugin_not_running(cfg) is False
|
|
|
|
def test_disabled_plugin_does_not_queue(self, test_display_controller):
|
|
controller = test_display_controller
|
|
controller.plugin_manager.plugin_manifests = {"hockey-scoreboard": {}}
|
|
controller.plugin_manager.discovered_plugin_ids.return_value = {"hockey-scoreboard"}
|
|
controller.plugin_display_modes = {}
|
|
cfg = {"hockey-scoreboard": {"enabled": False}}
|
|
assert controller._enabled_plugin_not_running(cfg) is False
|
|
|
|
def test_non_plugin_sections_do_not_queue(self, test_display_controller):
|
|
"""``schedule``/``display`` carry their own ``enabled`` and are never
|
|
in plugin_display_modes -- without the manifest check they would queue
|
|
a reconcile, and therefore a filesystem scan, on every config save."""
|
|
controller = test_display_controller
|
|
controller.plugin_manager.plugin_manifests = {"hockey-scoreboard": {}}
|
|
controller.plugin_manager.discovered_plugin_ids.return_value = {"hockey-scoreboard"}
|
|
controller.plugin_display_modes = {"hockey-scoreboard": ["nhl"]}
|
|
cfg = {
|
|
"hockey-scoreboard": {"enabled": True},
|
|
"schedule": {"enabled": True},
|
|
"display": {"enabled": True},
|
|
}
|
|
assert controller._enabled_plugin_not_running(cfg) is False
|
|
|
|
def test_non_dict_section_is_ignored(self, test_display_controller):
|
|
controller = test_display_controller
|
|
controller.plugin_manager.plugin_manifests = {"hockey-scoreboard": {}}
|
|
controller.plugin_manager.discovered_plugin_ids.return_value = {"hockey-scoreboard"}
|
|
controller.plugin_display_modes = {}
|
|
assert controller._enabled_plugin_not_running({"hockey-scoreboard": "nonsense"}) is False
|
|
|
|
def test_no_plugin_manager_is_quiet(self, test_display_controller):
|
|
controller = test_display_controller
|
|
controller.plugin_manager = None
|
|
assert controller._enabled_plugin_not_running({"x": {"enabled": True}}) is False
|
|
|
|
|
|
class TestReconcileQueuedThroughSubscriber:
|
|
"""End-to-end through the real config-change subscriber, not the helper.
|
|
|
|
Without the second gate this is the four-day-outage path: the plugin is
|
|
enabled, absent, and the save that enables its league sets no flag.
|
|
"""
|
|
|
|
@staticmethod
|
|
def _subscriber(controller):
|
|
subs = controller.config_service._subscribers['*']
|
|
for cb in subs:
|
|
if getattr(cb, '__name__', '') == '_controller_config_change':
|
|
return cb
|
|
raise AssertionError(f"controller subscriber not found among {subs}")
|
|
|
|
@staticmethod
|
|
def _configs(controller, plugin_section_old, plugin_section_new):
|
|
"""Build two full configs differing only inside the plugin section --
|
|
the subscriber refreshes its cache from these, so they must be real."""
|
|
base = copy.deepcopy(controller.config)
|
|
old = copy.deepcopy(base)
|
|
new = copy.deepcopy(base)
|
|
old["hockey-scoreboard"] = plugin_section_old
|
|
new["hockey-scoreboard"] = plugin_section_new
|
|
return old, new
|
|
|
|
def test_nested_edit_queues_reconcile_for_absent_plugin(self, test_display_controller):
|
|
controller = test_display_controller
|
|
controller.plugin_manager.plugin_manifests = {"hockey-scoreboard": {}}
|
|
controller.plugin_manager.discovered_plugin_ids.return_value = {"hockey-scoreboard"}
|
|
controller.plugin_display_modes = {} # validate_config() said False
|
|
controller._pending_plugin_reconcile = False
|
|
|
|
old, new = self._configs(
|
|
controller,
|
|
{"enabled": True, "nhl": {"enabled": False}},
|
|
{"enabled": True, "nhl": {"enabled": True}},
|
|
)
|
|
# The original gate is blind to this edit ...
|
|
assert controller._enabled_set_changed(old, new) is False
|
|
self._subscriber(controller)(old, new)
|
|
# ... but the reconcile is queued anyway.
|
|
assert controller._pending_plugin_reconcile is True
|
|
|
|
def test_steady_state_does_not_queue_reconcile(self, test_display_controller):
|
|
"""Everything enabled is running: an unrelated edit must not queue a
|
|
reconcile, or every config save drags a filesystem scan onto the
|
|
render thread."""
|
|
controller = test_display_controller
|
|
controller.plugin_manager.plugin_manifests = {"hockey-scoreboard": {}}
|
|
controller.plugin_manager.discovered_plugin_ids.return_value = {"hockey-scoreboard"}
|
|
controller.plugin_display_modes = {"hockey-scoreboard": ["nhl"]}
|
|
controller._pending_plugin_reconcile = False
|
|
|
|
old, new = self._configs(
|
|
controller,
|
|
{"enabled": True, "scroll_speed": 1},
|
|
{"enabled": True, "scroll_speed": 2},
|
|
)
|
|
self._subscriber(controller)(old, new)
|
|
|
|
assert controller._pending_plugin_reconcile is False
|
|
|
|
|
|
class TestPendingReconcileNotLost:
|
|
"""A config change arriving *during* reconcile must not be discarded.
|
|
|
|
The flag used to be cleared after a successful reconcile. Reconcile has
|
|
already read its config by then, so that clear erased a request it never
|
|
served and the newest config never reconciled -- the same "my save did
|
|
nothing" symptom this path exists to prevent.
|
|
"""
|
|
|
|
def test_request_arriving_during_reconcile_survives(self, test_display_controller):
|
|
controller = test_display_controller
|
|
controller._pending_plugin_reconcile = True
|
|
|
|
def reconcile_and_race():
|
|
# The watcher thread queues another change while we are mid-flight.
|
|
with controller._reconcile_flag_lock:
|
|
controller._pending_plugin_reconcile = True
|
|
return True
|
|
|
|
controller._reconcile_enabled_plugins = reconcile_and_race
|
|
controller._service_pending_reconcile()
|
|
|
|
assert controller._pending_plugin_reconcile is True, \
|
|
"a config change landing during reconcile was discarded"
|
|
|
|
def test_flag_cleared_on_a_quiet_success(self, test_display_controller):
|
|
controller = test_display_controller
|
|
controller._pending_plugin_reconcile = True
|
|
controller._reconcile_enabled_plugins = lambda: True
|
|
controller._service_pending_reconcile()
|
|
assert controller._pending_plugin_reconcile is False
|
|
|
|
def test_retryable_failure_rearms(self, test_display_controller):
|
|
controller = test_display_controller
|
|
controller._pending_plugin_reconcile = True
|
|
controller._reconcile_enabled_plugins = lambda: False
|
|
controller._service_pending_reconcile()
|
|
assert controller._pending_plugin_reconcile is True
|
|
|
|
def test_no_reconcile_when_nothing_pending(self, test_display_controller):
|
|
controller = test_display_controller
|
|
controller._pending_plugin_reconcile = False
|
|
calls = []
|
|
controller._reconcile_enabled_plugins = lambda: calls.append(1) or True
|
|
controller._service_pending_reconcile()
|
|
assert calls == []
|