From 3d347a368ab521030a720751be0e52a505963946 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Tue, 14 Jul 2026 08:21:35 -0400 Subject: [PATCH] fix(testing): stop plugin enabled:false schema defaults from silently disabling harness tests (#408) check_plugin.py, render_plugin.py, and the pytest plugin matrix each built config as {"enabled": True} then merged in config_schema.json's defaults on top, letting a plugin's own enabled:false default (a reasonable choice for a seasonal/opt-in plugin -- 15 of 23 real plugins ship one) silently win. Every harness/CI render of those plugins was testing "disabled, do nothing" rather than real behavior. Extract build_full_config() into testing/loading.py (already the shared home for plugin-discovery/config-default logic) and use it from all three call sites: schema defaults, then a forced enabled=True, then harness.json's config, then the caller's explicit config -- so a test can still deliberately disable a plugin on purpose, it just can't happen by accident via the plugin's own shipped schema default anymore. --- scripts/check_plugin.py | 13 +++---- scripts/render_plugin.py | 7 +--- src/plugin_system/testing/loading.py | 24 ++++++++++++ test/plugins/test_harness.py | 58 ++++++++++++++++++++++++++++ test/plugins/test_plugin_matrix.py | 6 +-- 5 files changed, 92 insertions(+), 16 deletions(-) diff --git a/scripts/check_plugin.py b/scripts/check_plugin.py index 0960e65a..997bf6d8 100644 --- a/scripts/check_plugin.py +++ b/scripts/check_plugin.py @@ -37,7 +37,7 @@ os.environ['EMULATOR'] = 'true' from src.logging_config import get_logger # noqa: E402 from src.plugin_system.testing.loading import ( # noqa: E402 - find_plugin_dir, load_config_defaults, load_harness_spec, load_manifest, + build_full_config, find_plugin_dir, load_harness_spec, load_manifest, ) from src.plugin_system.testing.harness import ( # noqa: E402 RenderResult, render_plugin_matrix, compare_to_goldens, write_goldens, @@ -97,12 +97,11 @@ def check_one(plugin_id: str, search_dirs: List[str], sizes, mock_data: Dict, # matrix path does; explicit CLI flags still override the file. spec = load_harness_spec(plugin_dir) - # config_schema defaults (real-install behavior), then harness.json config, - # then CLI --config — most specific wins. - full_config = {"enabled": True} - full_config.update(load_config_defaults(plugin_dir)) - full_config.update(spec.get("config", {})) - full_config.update(config) + # config_schema defaults (real-install behavior, with enabled forced True + # so a plugin's own enabled:false default can't accidentally disable + # testing), then harness.json config, then CLI --config — most specific + # wins. + full_config = build_full_config(plugin_dir, spec, config) # Precedence: CLI flag > LEDMATRIX_TEST_SIZES env > harness.json > default. effective_sizes = sizes if sizes else resolve_test_sizes(spec.get("sizes")) diff --git a/scripts/render_plugin.py b/scripts/render_plugin.py index bcacc4b8..39dee61f 100644 --- a/scripts/render_plugin.py +++ b/scripts/render_plugin.py @@ -28,7 +28,7 @@ os.environ['EMULATOR'] = 'true' # Import logger after path setup so src.logging_config is importable from src.logging_config import get_logger # noqa: E402 from src.plugin_system.testing.loading import ( # noqa: E402 - find_plugin_dir, load_manifest, load_config_defaults, + build_full_config, find_plugin_dir, load_manifest, ) logger = get_logger("[Render Plugin]") @@ -83,16 +83,13 @@ def main() -> int: manifest = load_manifest(Path(plugin_dir)) # Parse config: start with schema defaults, then apply overrides - config_defaults = load_config_defaults(Path(plugin_dir)) try: user_config = json.loads(args.config) except json.JSONDecodeError as e: logger.error("Invalid JSON config: %s", e) return 1 - config = {'enabled': True} - config.update(config_defaults) - config.update(user_config) + config = build_full_config(Path(plugin_dir), cli_config=user_config) # Load mock data if provided mock_data = {} diff --git a/src/plugin_system/testing/loading.py b/src/plugin_system/testing/loading.py index f4971ec3..061e95a0 100644 --- a/src/plugin_system/testing/loading.py +++ b/src/plugin_system/testing/loading.py @@ -88,3 +88,27 @@ def load_harness_spec(plugin_dir: Union[str, Path]) -> Dict[str, Any]: with open(mock_path, 'r') as mf: spec['mock_data_contents'] = json.load(mf) return spec + + +def build_full_config( + plugin_dir: Union[str, Path], + spec: Optional[Dict[str, Any]] = None, + cli_config: Optional[Dict[str, Any]] = None, +) -> Dict[str, Any]: + """Build the config a plugin sees under test. + + Merge order: config_schema.json defaults, then a forced ``enabled: True``, + then harness.json's config overlay, then the caller's explicit config -- + most specific wins. `enabled` is re-asserted *after* the schema defaults + so a plugin that reasonably ships `enabled: false` (e.g. a seasonal or + opt-in plugin) can't silently make every harness run test "disabled, do + nothing" by accident -- callers that genuinely want to test the disabled + path can still do so via `cli_config={"enabled": False}`. + """ + spec = spec or {} + config: Dict[str, Any] = {} + config.update(load_config_defaults(plugin_dir)) + config["enabled"] = True + config.update(spec.get("config", {})) + config.update(cli_config or {}) + return config diff --git a/test/plugins/test_harness.py b/test/plugins/test_harness.py index e92f16ca..4e2d1c88 100644 --- a/test/plugins/test_harness.py +++ b/test/plugins/test_harness.py @@ -253,3 +253,61 @@ class TestCheckPluginHonorsHarnessJson: ) assert captured["freeze_time"] == "2030-01-01 00:00:00" assert captured["config"]["timezone"] == "America/New_York" + + +class TestBuildFullConfigForcesEnabled: + """Regression: a plugin's own config_schema.json may reasonably default + enabled to False (e.g. a seasonal or opt-in plugin) -- march-madness and + 14 other real plugins do. The harness must still test it as enabled + unless a caller explicitly asks otherwise, or every render silently + becomes a same-shaped "disabled, do nothing" no-op.""" + + def _make_plugin_with_disabled_default(self, tmp_path): + pdir = tmp_path / "plugins" / "demo-seasonal" + pdir.mkdir(parents=True) + (pdir / "manifest.json").write_text(json.dumps({ + "id": "demo-seasonal", "name": "Demo Seasonal", "version": "1.0.0", + "author": "test", "entry_point": "manager.py", + "class_name": "DemoSeasonal", "display_modes": ["demo-seasonal"], + "compatible_versions": ["*"], + })) + (pdir / "config_schema.json").write_text(json.dumps({ + "type": "object", + "properties": {"enabled": {"type": "boolean", "default": False}}, + })) + return pdir + + def test_schema_disabled_default_does_not_win(self, tmp_path): + from src.plugin_system.testing.loading import build_full_config + plugin_dir = self._make_plugin_with_disabled_default(tmp_path) + config = build_full_config(plugin_dir) + assert config["enabled"] is True + + def test_harness_json_config_can_still_disable(self, tmp_path): + from src.plugin_system.testing.loading import build_full_config + plugin_dir = self._make_plugin_with_disabled_default(tmp_path) + config = build_full_config(plugin_dir, spec={"config": {"enabled": False}}) + assert config["enabled"] is False + + def test_explicit_cli_config_can_still_disable(self, tmp_path): + from src.plugin_system.testing.loading import build_full_config + plugin_dir = self._make_plugin_with_disabled_default(tmp_path) + config = build_full_config(plugin_dir, cli_config={"enabled": False}) + assert config["enabled"] is False + + def test_check_one_renders_a_schema_disabled_plugin_as_enabled(self, tmp_path, monkeypatch): + """End-to-end: check_plugin.py's check_one() must not blank-render a + plugin just because its own schema defaults enabled to False.""" + mod = _load_check_plugin_cli() + plugin_dir = self._make_plugin_with_disabled_default(tmp_path) + captured = {} + monkeypatch.setattr(mod, "render_plugin_matrix", + lambda **kw: captured.update(kw) or []) + monkeypatch.setattr(mod, "compare_to_goldens", lambda *a, **k: []) + mod.check_one( + plugin_id="demo-seasonal", search_dirs=[str(tmp_path / "plugins")], + sizes=None, mock_data={}, config={}, run_update=True, + out_dir=None, update_golden=False, golden_dir_override=None, + freeze_time=None, + ) + assert captured["config"]["enabled"] is True diff --git a/test/plugins/test_plugin_matrix.py b/test/plugins/test_plugin_matrix.py index 788754fa..350b8385 100644 --- a/test/plugins/test_plugin_matrix.py +++ b/test/plugins/test_plugin_matrix.py @@ -22,7 +22,7 @@ import pytest from src.plugin_system.testing.harness import ( render_plugin_matrix, compare_to_goldens, ) -from src.plugin_system.testing.loading import load_config_defaults, load_harness_spec +from src.plugin_system.testing.loading import build_full_config, load_harness_spec from src.plugin_system.testing.sizes import resolve_test_sizes PROJECT_ROOT = Path(__file__).resolve().parents[2] @@ -81,9 +81,7 @@ def test_plugin_renders_across_sizes_and_screens(plugin_id: str) -> None: plugin_dir = _PLUGINS[plugin_id] spec = load_harness_spec(plugin_dir) - config = {"enabled": True} - config.update(load_config_defaults(plugin_dir)) - config.update(spec.get("config", {})) + config = build_full_config(plugin_dir, spec) # Sizes: LEDMATRIX_TEST_SIZES env (test on real hardware) wins, then the # plugin's own harness.json "sizes", else the default representative sample.