Test suite overhaul + fixes for the three bugs it uncovered

.github/workflows/test.yml

@@ -4,6 +4,9 @@ on:
  pull_request:
  push:
  branches: [main]
+ # Manual runs against any branch — useful when a PR's automatic run
+ # needs a re-run or didn't get created.
+ workflow_dispatch:
 
 # Both jobs only check out the repo and run pytest.
 permissions:
@@ -13,6 +16,12 @@ jobs:
  plugin-safety:
  name: Plugin safety harness + unit tests
  runs-on: ubuntu-latest
+ env:
+ # The bundled fixture plugin gives the harness at least one real plugin
+ # to render, and REQUIRE_PLUGINS turns "discovered zero plugins" into a
+ # hard failure instead of a silent all-skip green run.
+ LEDMATRIX_PLUGINS_DIR: test/fixtures/plugins
+ LEDMATRIX_REQUIRE_PLUGINS: "1"
  steps:
  - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
  with:
@@ -29,12 +38,9 @@ jobs:
  pip install -r requirements.txt -r requirements-test.txt
  pip install RGBMatrixEmulator
 
- - name: Run harness + visual rendering tests
+ - name: Run plugin safety harness
  run: |
- pytest --no-cov \
- test/plugins/test_harness.py \
- test/plugins/test_visual_rendering.py \
- test/plugins/test_plugin_matrix.py
+ pytest --no-cov test/plugins/
 
  unit-tests:
  name: Core unit tests
@@ -55,35 +61,15 @@ jobs:
  pip install -r requirements.txt -r requirements-test.txt
  pip install RGBMatrixEmulator
 
- # Safety net for the shared sports/scroll/style infrastructure. These
- # suites existed but were not enrolled in CI, so a refactor of
- # src/base_classes or src/common could regress them silently. Enrolled
- # explicitly (not `pytest test/`) so known hardware-only suites don't
- # break CI; grow this list as more suites are made headless.
+ # Run the ENTIRE test tree (except test/plugins, which the
+ # plugin-safety job owns). New test files are enrolled automatically;
+ # excluding anything requires a visible, commented --ignore here.
+ # Coverage is measured and enforced only in this step — pytest.ini
+ # deliberately carries no coverage flags so local runs stay fast.
  - name: Run core unit suites
  run: |
- pytest --no-cov \
- test/test_skin_system.py \
- test/test_font_manager.py \
- test/test_data_sources.py \
- test/test_api_extractors.py \
- test/test_scroll_helper.py \
- test/test_scroll_helper_continuous.py \
- test/test_adaptive_layout.py \
- test/test_loader_compat_warning.py \
- test/test_sports_base_characterization.py \
- test/test_element_style.py \
- test/test_sports_core_promotions.py \
- test/test_sports_modes_promotions.py \
- test/test_sports_capabilities.py \
- test/test_sports_scroll.py \
- test/test_version_consistency.py \
- test/test_plugin_compatibility_gate.py \
- test/test_install_preserves_existing.py \
- test/test_core_owned_config_keys.py \
- test/test_async_plugin_updates.py \
- test/test_plugin_update_reservation.py \
- test/test_template_targets.py \
- test/test_widget_scripts.py \
- test/test_doc_links.py \
- test/web_interface/test_cache.py
+ pytest -m "not hardware" test/ \
+ --ignore=test/plugins \
+ --cov=src --cov=web_interface \
+ --cov-report=term \
+ --cov-fail-under=45

pytest.ini

@@ -10,16 +10,13 @@ python_functions = test_*
 testpaths = test
 
 # Output options
-# Note: Coverage options require pytest-cov to be installed
-# Run: pip install pytest-cov
-addopts = 
+# Coverage is deliberately NOT configured here: a bare local `pytest` should
+# be fast and dependency-light. Coverage is measured and enforced in exactly
+# one place — the unit-tests job in .github/workflows/test.yml.
+addopts =
  -v
  --strict-markers
  --tb=short
- --cov=src
- --cov-report=term-missing
- --cov-report=html
- --cov-fail-under=30
 
 # Markers
 markers =

requirements-test.txt

@@ -4,4 +4,6 @@ pytest>=9.0.3,<10.0.0
 pytest-cov>=4.1.0,<5.0.0
 pytest-mock>=3.11.0,<4.0.0
 freezegun>=1.2,<2 # deterministic time for golden-image tests
+psutil>=6.0.0,<8.0.0 # optional at runtime; installed for tests so the
+ # /system/status endpoint's real path is exercised
 mypy>=1.5.0,<2.0.0 # static type checking (also pinned in .pre-commit-config.yaml)

src/base_classes/sports/core.py

@@ -383,10 +383,15 @@ def render_skin_card(self, game: Dict, size: tuple) -> Optional[Image.Image]:
  ctx = skin_runtime.build_context(self, game, size=size)
  card = skin.render_vegas_card(ctx, dict(game))
  if card is not None:
+ # A successful render clears accumulated strikes, mirroring
+ # _render_game — transient failures must not add up across
+ # the session and disable a working skin.
+ self._skin_failures = 0
  return card
  ctx = skin_runtime.build_context(self, game, size=size)
  render = getattr(skin, f"render_{self.SKIN_MODE}")
  if render(ctx, dict(game)):
+ self._skin_failures = 0
  return ctx.canvas
  except Exception:
  # Card failures count toward the same 3-strike session disable

src/config_manager.py

@@ -106,18 +106,13 @@ def save_config_atomic(
  Returns:
  SaveResult with status and details
  """
- # Load current secrets to preserve them
- secrets_content = {}
- if os.path.exists(self.secrets_path):
- try:
- with open(self.secrets_path, 'r') as f_secrets:
- secrets_content = json.load(f_secrets)
- except Exception as e:
- self.logger.warning(f"Could not load secrets file {self.secrets_path} during save: {e}")
-
+ # Load current secrets to preserve them (raises if unreadable — see
+ # _load_secrets_for_save)
+ secrets_content = self._load_secrets_for_save()
+
  # Strip secrets from main config before saving
  config_to_write = self._strip_secrets_recursive(new_config_data, secrets_content)
-
+
  # Use atomic manager to save
  atomic_mgr = self._get_atomic_manager()
  result = atomic_mgr.save_config_atomic(
@@ -290,19 +285,43 @@ def _strip_secrets_recursive(self, data_to_filter: Dict[str, Any], secrets: Dict
  result[key] = value
  return result
 
+ def _load_secrets_for_save(self) -> Dict[str, Any]:
+ """Load config_secrets.json for stripping before a save.
+
+ A missing secrets file is fine (nothing to strip). But a file that
+ EXISTS and cannot be read or parsed means stripping is impossible —
+ and the in-memory config being saved has secrets deep-merged into it,
+ so proceeding would write them into config.json in plaintext. That
+ was the historical behavior; it is now a hard refusal. The save
+ raises so the caller (and user) fixes the secrets file instead of
+ silently leaking its contents into the world-readable main config.
+ """
+ if not os.path.exists(self.secrets_path):
+ return {}
+ try:
+ with open(self.secrets_path, 'r') as f_secrets:
+ return json.load(f_secrets)
+ # Only the expected read/parse failures — an unexpected implementation
+ # error should propagate as itself, not masquerade as a secrets-file
+ # problem. (JSONDecodeError and UnicodeDecodeError are ValueErrors.)
+ except (OSError, ValueError, RecursionError) as e:
+ error_msg = (
+ f"Refusing to save config: secrets file {self.secrets_path} exists "
+ f"but could not be loaded ({e}). Saving without it would write "
+ f"merged secret values into config.json in plaintext. Fix or "
+ f"remove the secrets file, then retry."
+ )
+ self.logger.error("[Config] %s", error_msg, exc_info=True)
+ raise ConfigError(error_msg, config_path=self.secrets_path) from e
+
  def save_config(self, new_config_data: Dict[str, Any]) -> None:
- """Save configuration to the main JSON file, stripping out secrets."""
- secrets_content = {}
- if os.path.exists(self.secrets_path):
- try:
- with open(self.secrets_path, 'r') as f_secrets:
- secrets_content = json.load(f_secrets)
- except Exception as e:
- self.logger.warning(f"Could not load secrets file {self.secrets_path} during save: {e}")
- # Continue without stripping if secrets can't be loaded, or handle as critical error
- # For now, we'll proceed cautiously and save the full new_config_data if secrets are unreadable
- # to prevent accidental data loss if the secrets file is temporarily corrupt.
- # A more robust approach might be to fail the save or use a cached version of secrets.
+ """Save configuration to the main JSON file, stripping out secrets.
+
+ Raises ConfigError when the secrets file exists but cannot be loaded,
+ because stripping would be impossible and secrets would leak into
+ config.json.
+ """
+ secrets_content = self._load_secrets_for_save()
 
  config_to_write = self._strip_secrets_recursive(new_config_data, secrets_content)
 

src/plugin_system/compatibility.py

@@ -180,6 +180,45 @@ def declared_min_version(manifest: Dict[str, Any]) -> Optional[str]:
  return None
 
 
+def is_update_available(installed_version: str, latest_version: str) -> bool:
+ """Return True when the registry's ``latest_version`` is strictly newer
+ than the installed version.
+
+ THE shared comparator for "should this plugin be updated?" — used by both
+ the web UI's update badge (`api_v3._is_plugin_update_available`) and the
+ store's `update_plugin` reinstall decision, so the two can never disagree.
+
+ Uses PEP 440-aware comparison (``packaging``), which also normalizes
+ equivalent spellings: ``v1.2.0`` == ``1.2.0`` and ``1.2`` == ``1.2.0``, so
+ cosmetic differences never trigger a reinstall — and a locally modified
+ plugin whose version is *ahead* of the registry is never "updated"
+ (downgraded). If either version string can't be parsed the mismatch is
+ surfaced (True) so the user can reconcile, rather than silently hiding a
+ potential update.
+ """
+ if not installed_version or not latest_version:
+ return False
+ if not isinstance(installed_version, str) or not isinstance(latest_version, str):
+ # A malformed manifest/registry can carry a number (1.2) or worse;
+ # packaging would raise TypeError. Surface the mismatch instead.
+ return True
+ if installed_version == latest_version:
+ return False
+ try:
+ from packaging.version import parse as _parse_version, InvalidVersion
+ except ImportError:
+ # packaging is a core dependency, but if it's somehow unavailable we
+ # can't compare semantically — surface the mismatch we already know
+ # exists (the two strings differ).
+ return True
+ try:
+ return _parse_version(latest_version) > _parse_version(installed_version)
+ except InvalidVersion:
+ # Unparseable version string: we can't tell direction, so surface the
+ # mismatch rather than silently hiding a potential update.
+ return True
+
+
 def check(manifest: Dict[str, Any], core_version: str) -> Tuple[bool, Optional[str]]:
  """Return ``(compatible, reason)``.
 

src/plugin_system/store_manager.py

@@ -2969,16 +2969,27 @@ def update_plugin(self, plugin_id: str) -> bool:
  remote_branch = plugin_info_remote.get('branch') or plugin_info_remote.get('default_branch')
 
  # Compare local manifest version against registry latest_version
- # to avoid unnecessary reinstalls for monorepo plugins
+ # to avoid unnecessary reinstalls for monorepo plugins. Uses the
+ # same semantic comparator as the web UI's update badge, so
+ # equivalent spellings ("v1.2.0" vs "1.2.0") never trigger a
+ # reinstall and a locally-ahead version is never downgraded.
  try:
  local_manifest_path = plugin_path / "manifest.json"
  if local_manifest_path.exists():
  with open(local_manifest_path, 'r', encoding='utf-8') as f:
  local_manifest = json.load(f)
  local_version = local_manifest.get('version', '')
  remote_version = plugin_info_remote.get('latest_version', '')
- if local_version and remote_version and local_version == remote_version:
- self.logger.info(f"Plugin {plugin_id} already at latest version {local_version}")
+ from src.plugin_system.compatibility import is_update_available
+ # No truthiness gate: the shared comparator already treats
+ # a missing version on either side as "no update", and the
+ # store must agree with the UI badge in that case too. A
+ # missing manifest (not just a missing version field)
+ # still falls through to the reinstall recovery path.
+ if not is_update_available(local_version, remote_version):
+ self.logger.info(
+ f"Plugin {plugin_id} already at latest version "
+ f"(installed {local_version}, registry {remote_version})")
  return True
  except Exception as e:
  self.logger.debug(f"Could not compare versions for {plugin_id}: {e}")

test/fixtures/plugins/ci-fixture-plugin/config_schema.json

@@ -0,0 +1,31 @@
+{
+ "$schema": "http://json-schema.org/draft-07/schema#",
+ "title": "CI Fixture Plugin",
+ "type": "object",
+ "properties": {
+ "enabled": {
+ "type": "boolean",
+ "default": true
+ },
+ "display_duration": {
+ "type": "number",
+ "default": 5
+ },
+ "border_color": {
+ "type": "array",
+ "items": {"type": "integer", "minimum": 0, "maximum": 255},
+ "minItems": 3,
+ "maxItems": 3,
+ "default": [0, 255, 0],
+ "description": "RGB color of the border rectangle."
+ },
+ "diagonal_color": {
+ "type": "array",
+ "items": {"type": "integer", "minimum": 0, "maximum": 255},
+ "minItems": 3,
+ "maxItems": 3,
+ "default": [255, 0, 0],
+ "description": "RGB color of the diagonals."
+ }
+ }
+}

test/fixtures/plugins/ci-fixture-plugin/manager.py

@@ -0,0 +1,44 @@
+"""
+CI fixture plugin.
+
+Exists so the plugin safety harness (test/plugins/test_plugin_matrix.py and
+the plugin-safety CI job) always has at least one real plugin to load and
+render — without it, an empty plugins/ directory turns the whole job into a
+green no-op. The render is deliberately trivial and fully deterministic:
+a border rectangle plus both diagonals, sized from the display manager's
+declared dimensions. No fonts, no network, no time dependence, so golden
+images are stable across platforms.
+"""
+
+from PIL import ImageDraw
+
+from src.plugin_system.base_plugin import BasePlugin
+
+
+class CIFixturePlugin(BasePlugin):
+ """Deterministic CI-only fixture plugin: renders a border + diagonals
+ pattern sized from the display's declared dimensions. Never shipped to
+ devices; exists solely so the plugin safety harness has a real plugin
+ to exercise in CI."""
+
+ def update(self) -> None:
+ """Nothing to fetch — the render is self-contained."""
+
+ def display(self, force_clear: bool = False) -> None:
+ self.display_manager.clear()
+ width = self.display_manager.matrix.width
+ height = self.display_manager.matrix.height
+ border = tuple(self.config.get("border_color", [0, 255, 0]))
+ diagonal = tuple(self.config.get("diagonal_color", [255, 0, 0]))
+
+ image = self.display_manager.image
+ draw = ImageDraw.Draw(image)
+ # Blank only the declared panel area, then draw edge-to-edge content:
+ # the border proves the plugin reads dynamic dimensions (any overflow
+ # or underfill at any size is a harness bug or a dimensions bug), the
+ # diagonals make golden comparisons sensitive to size/offset drift.
+ draw.rectangle([0, 0, width - 1, height - 1], fill=(0, 0, 0))
+ draw.rectangle([0, 0, width - 1, height - 1], outline=border)
+ draw.line([0, 0, width - 1, height - 1], fill=diagonal)
+ draw.line([0, height - 1, width - 1, 0], fill=diagonal)
+ self.display_manager.update_display()

test/fixtures/plugins/ci-fixture-plugin/manifest.json

@@ -0,0 +1,13 @@
+{
+ "id": "ci-fixture-plugin",
+ "name": "CI Fixture Plugin",
+ "version": "1.0.0",
+ "description": "Bundled test fixture so the plugin safety harness always has at least one real plugin to render in CI. Draws a deterministic border + diagonals pattern at any panel size. Not installable from the store and never shipped to devices.",
+ "author": "LEDMatrix",
+ "entry_point": "manager.py",
+ "class_name": "CIFixturePlugin",
+ "display_modes": ["ci-fixture"],
+ "update_interval": 3600,
+ "min_ledmatrix_version": "2.0.0",
+ "compatible_versions": [">=2.0.0"]
+}

test/fixtures/plugins/ci-fixture-plugin/requirements.txt

@@ -0,0 +1,7 @@
+# No dependencies — the fixture must load in any environment.
+#
+# Pillow is deliberately NOT pinned here even though manager.py imports
+# PIL: it is a core LEDMatrix dependency (see the repo-root
+# requirements.txt), so it is always present wherever the harness runs,
+# and the harness loads plugins with install_deps=False anyway. Pinning
+# it here would only invite a needless pip install during test runs.