ci: run the plugins' own tests, and repair the ones that could not pass

.github/workflows/test-plugins.yml

@@ -59,16 +59,34 @@ jobs:
         run: |
           if [ "$ALL_INPUT" = "true" ]; then
             ids=$(ls plugins)
+            all_ids="$ids"
           else
-            # Plugins with a changed file outside their test/ dir.
+            # Two lists, because the consumers want different things.
+            #
+            # ids -- plugins whose *shipped* code changed. Drives the version
+            # bump gate and the safety harness. Test-only edits are excluded:
+            # they change nothing a user receives, so demanding a version bump
+            # for them is noise. The root-level test_*.py exclusion matters as
+            # much as the test/ one -- most plugins keep their tests there, so
+            # without it "test-only change" was never actually true.
             ids=$(git diff --name-only "$BASE_SHA"...HEAD -- 'plugins/*' \
               | grep -vE '^plugins/[^/]+/test/' \
+              | grep -vE '^plugins/[^/]+/test_[^/]+\.py$' \
+              | sed -E 's#^plugins/([^/]+)/.*#\1#' | sort -u)
+            # all_ids -- any change at all, tests included. Drives the unit-test
+            # run: a PR that only edits a test still has to prove the test
+            # passes, which is exactly when it most needs checking.
+            all_ids=$(git diff --name-only "$BASE_SHA"...HEAD -- 'plugins/*' \
               | sed -E 's#^plugins/([^/]+)/.*#\1#' | sort -u)
           fi
           echo "ids<<EOF" >> "$GITHUB_OUTPUT"
           echo "$ids" >> "$GITHUB_OUTPUT"
           echo "EOF" >> "$GITHUB_OUTPUT"
-          echo "Changed plugins:"; echo "$ids"
+          echo "all_ids<<EOF" >> "$GITHUB_OUTPUT"
+          echo "$all_ids" >> "$GITHUB_OUTPUT"
+          echo "EOF" >> "$GITHUB_OUTPUT"
+          echo "Changed plugins (shipped code):"; echo "$ids"
+          echo "Changed plugins (including tests):"; echo "$all_ids"
 
       - name: Enforce version bump on changed plugins
         if: steps.changed.outputs.ids != '' && github.event.inputs.all != 'true'
@@ -175,6 +193,49 @@ jobs:
           fi
           exit $fail
 
+      # The plugins' own test_*.py. Nothing ran these until now: this workflow
+      # was the harness plus manifest checks, so a plugin unit test guarded
+      # nothing unless somebody ran it by hand. That is how a display() that
+      # returned None on every path survived from February to August -- the
+      # harness renders screens and cannot see a return value, and its fixtures
+      # seed data so an empty-state path never executes.
+      #
+      # Runs with if: always() so a harness failure does not hide these, and
+      # vice versa -- one PR should surface every problem it has.
+      #
+      # run_plugin_tests.py separates "prerequisites absent" (exit 2, reported
+      # as a skip) from a real failure, which is what makes this gateable: a
+      # script wanting a tty or an out-of-season feed no longer looks like a
+      # regression. Adding a plugin test that cannot pass here will now fail
+      # CI, so make it skip deliberately rather than leaving it red.
+      - name: Run plugin unit tests on changed plugins
+        if: always() && steps.changed.outputs.all_ids != ''
+        working-directory: plugins-repo
+        env:
+          IDS: ${{ steps.changed.outputs.all_ids }}
+        run: |
+          set -e
+          present=""
+          for pid in $IDS; do
+            case "$pid" in
+              '' | *[!a-z0-9._-]*) echo "::error::invalid plugin id (redacted)"; exit 1 ;;
+            esac
+            # A plugin deleted in this PR is still a "changed" id; the runner
+            # treats an unknown id as an error, so drop it here.
+            [ -d "plugins/$pid" ] || { echo "::notice::$pid removed, skipping"; continue; }
+            # The harness step installs these too, but this step runs even when
+            # that one failed, so it cannot rely on having got that far.
+            if [ -f "plugins/$pid/requirements.txt" ]; then
+              pip install -r "plugins/$pid/requirements.txt"
+            fi
+            present="$present $pid"
+          done
+          if [ -z "$present" ]; then
+            echo "No changed plugin still present; nothing to test."
+            exit 0
+          fi
+          python scripts/run_plugin_tests.py $present --core "$GITHUB_WORKSPACE/core"
+
       - name: Nothing to check
         if: steps.changed.outputs.ids == ''
-        run: echo "No plugin code changed (test-only or docs change) — safety harness skipped."
+        run: echo "No shipped plugin code changed (test-only or docs change) — version gate and safety harness skipped; plugin unit tests still ran if any test changed."

.gitignore

@@ -41,3 +41,9 @@ plugins/**/config_secrets.json
 
 # Asset backups (local working copies)
 plugins/*/assets/**/*_backup/
+
+# RGBMatrixEmulator writes this into the working directory on first run,
+# so the plugin test suites drop one wherever they are invoked from.
+# ledmatrix-music/emulator_config.json is committed on purpose and stays
+# tracked -- .gitignore does not apply to files already in the index.
+emulator_config.json

plugins/baseball-scoreboard/test_baseball_plugin.py

@@ -80,28 +80,38 @@ def create_mock_cache_manager():
 
 
 def create_test_config() -> Dict[str, Any]:
-    """Create test configuration for baseball scoreboard plugin."""
+    """Test configuration for the baseball scoreboard plugin.
+
+    Nested per league, which is the shape the plugin reads
+    (``config["mlb"]["enabled"]``, see manager.py). It used to be flat
+    (``mlb_enabled``), which the plugin never looked at, so every league
+    resolved to disabled and the suite reported success while exercising
+    almost nothing -- "Enabled leagues: []" on a passing run.
+
+    MLB on, the other two off: one enabled league is enough to drive the
+    update / display / info path for real, and the whole suite still finishes
+    in about 25s.
+    """
     return {
-        'mlb_enabled': True,
-        'mlb_favorite_teams': ['TEX', 'NYM'],
-        'mlb_display_modes_live': True,
-        'mlb_display_modes_recent': True,
-        'mlb_display_modes_upcoming': True,
-        'mlb_live_priority': True,
-        'mlb_live_update_interval': 15,
-        'mlb_recent_update_interval': 3600,
-        'mlb_upcoming_update_interval': 3600,
-        'mlb_recent_games_to_show': 5,
-        'mlb_upcoming_games_to_show': 10,
-        'mlb_show_records': False,
-        'mlb_show_ranking': False,
-        'mlb_show_odds': False,
-        'mlb_test_mode': False,  # Set to True for test mode
-
-        'milb_enabled': False,
-        'ncaa_baseball_enabled': False,
-
+        'enabled': True,
         'display_duration': 15,
+        'mlb': {
+            'enabled': True,
+            'favorite_teams': ['TEX', 'NYM'],
+            'display_modes': {
+                'show_live': True,
+                'show_recent': True,
+                'show_upcoming': True,
+            },
+            'live_priority': True,
+            'recent_games_to_show': 5,
+            'upcoming_games_to_show': 10,
+            'show_records': False,
+            'show_ranking': False,
+            'show_odds': False,
+        },
+        'milb': {'enabled': False},
+        'ncaa_baseball': {'enabled': False},
     }
 
 
@@ -153,13 +163,31 @@ def __init__(self):
             plugin_manager=plugin_manager
         )
 
-        if plugin.initialized:
-            print("[OK] Plugin initialized successfully")
-            print(f"     Enabled leagues: {[k for k, v in plugin.leagues.items() if v.get('enabled', False)]}")
-            return plugin
-        else:
-            print("[FAIL] Plugin initialization failed")
+        # Construction not raising is the real signal. This used to gate on
+        # `plugin.initialized`, an attribute neither this plugin nor BasePlugin
+        # has ever defined, so the check raised AttributeError and reported
+        # "initialization error" on a plugin that had in fact just initialised
+        # fine. Assert the surface the rest of this suite goes on to use.
+        missing = [name for name in ("display", "update", "_league_registry")
+                   if not hasattr(plugin, name)]
+        if missing:
+            print(f"[FAIL] Plugin is missing expected attributes: {missing}")
+            return None
+
+        enabled = sorted(k for k, v in plugin._league_registry.items()
+                         if v.get("enabled", False))
+        # Assert, don't just report. The config used to be flat, so nothing was
+        # enabled and the whole suite ran green over a plugin doing nothing --
+        # printing "Enabled leagues: []" as it went. A silent return to that
+        # state is the regression most worth catching here.
+        if enabled != ["mlb"]:
+            print(f"[FAIL] expected MLB to be the only enabled league, got "
+                  f"{enabled}; the test config is not reaching the plugin")
             return None
+
+        print("[OK] Plugin initialized successfully")
+        print(f"     Enabled leagues: {enabled}")
+        return plugin
 
     except Exception as e:
         print(f"[FAIL] Plugin initialization error: {e}")
@@ -179,17 +207,22 @@ def test_plugin_update(plugin):
         plugin.update()
         print("[OK] Plugin update completed")
 
-        # Check league states
-        for league_key, league_config in plugin.leagues.items():
-            if league_config.get('enabled', False):
-                live_state = plugin.league_state[league_key]['live']
-                recent_state = plugin.league_state[league_key]['recent']
-                upcoming_state = plugin.league_state[league_key]['upcoming']
-
-                print(f"   {league_key}:")
-                print(f"     Live games: {len(live_state['games_list'])}")
-                print(f"     Recent games: {len(recent_state['games_list'])}")
-                print(f"     Upcoming games: {len(upcoming_state['games_list'])}")
+        # Report what each enabled league loaded. This used to read
+        # plugin.leagues / plugin.league_state, neither of which the plugin has
+        # -- the games are reachable through the per-mode managers held in
+        # _league_registry, so the whole block raised AttributeError and
+        # reported an update failure on an update that had succeeded.
+        for league_key, entry in plugin._league_registry.items():
+            if not entry.get('enabled', False):
+                continue
+            print(f"   {league_key}:")
+            for mode in ('live', 'recent', 'upcoming'):
+                mgr = entry.get('managers', {}).get(mode)
+                if mgr is None:
+                    print(f"     {mode.capitalize()} games: (no manager)")
+                    continue
+                games = getattr(mgr, 'games_list', None) or []
+                print(f"     {mode.capitalize()} games: {len(games)}")
 
         return True
     except Exception as e:
@@ -269,11 +302,11 @@ def run_emulator_test(plugin, duration=30):
                 if cycle_count % 5 == 0:
                     print(f"   Cycle {cycle_count}: {elapsed:.1f}s elapsed")
                     # Print current game info
-                    for league_key, league_config in plugin.leagues.items():
-                        if league_config.get('enabled', False):
-                            live_state = plugin.league_state[league_key]['live']
-                            if live_state['current_game']:
-                                game = live_state['current_game']
+                    for league_key, entry in plugin._league_registry.items():
+                        if entry.get('enabled', False):
+                            live_mgr = entry.get('managers', {}).get('live')
+                            game = getattr(live_mgr, 'current_game', None)
+                            if game:
                                 print(f"     {league_key} live: {game.get('away_abbr', '?')} @ {game.get('home_abbr', '?')}")
 
                 # Short delay between cycles

plugins/baseball-scoreboard/test_score_antialiasing.py

@@ -35,6 +35,14 @@ def _find_pixel_font():
     """
     name = "PressStart2P-Regular.ttf"
     candidates = []
+    # LEDMATRIX_CORE is the runner's contract for "the core is here", and it is
+    # absolute. Walking up from the plugin only finds the fonts when the plugin
+    # happens to sit inside a core checkout (LEDMatrix/plugin-repos/<id>/); from
+    # a standalone plugins repo it never does, so this skipped every run even
+    # when a core was supplied.
+    core = os.environ.get("LEDMATRIX_CORE")
+    if core:
+        candidates.append(os.path.join(core, "assets", "fonts", name))
     d = PLUGIN_DIR
     for _ in range(6):
         candidates.append(os.path.join(d, "assets", "fonts", name))

plugins/basketball-scoreboard/test_plugin_syntax.py

@@ -1,35 +1,71 @@
 #!/usr/bin/env python3
-"""
-Simple syntax checker for the basketball plugin.
-Tests that the plugin can be imported without errors.
+"""Smoke test: the entry point imports and declares the class the manifest names.
+
+Cheap, but it catches the two failures that stop a plugin loading at all --
+a syntax error, and a manifest whose ``class_name`` no longer matches the code.
+The second is not hypothetical: this file used to import a hardcoded
+``BasketballPluginManager``, a name the plugin had long since stopped using, so
+it failed on every run for a reason that had nothing to do with the plugin. It
+now reads the name from the manifest, which is the thing the loader actually
+uses, so a rename is either reflected in both places or caught here.
+
+Needs the core on the path for ``src.plugin_system.base_plugin``; run via
+scripts/run_plugin_tests.py, which supplies it.
+
+Exit codes: 0 pass, 2 skip (no core), 1 fail.
 """
 
-import sys
+import json
 import os
+import sys
+from pathlib import Path
 
-# Set emulator mode before any imports
-os.environ['EMULATOR'] = 'true'
+PLUGIN_DIR = Path(__file__).resolve().parent
+sys.path.insert(0, str(PLUGIN_DIR))
 
-# Add LEDMatrix to path
-sys.path.insert(0, '/home/chuck/Github/LEDMatrix')
+# Emulator mode must be set before the plugin imports anything from the core.
+os.environ.setdefault("EMULATOR", "true")
+
+core = os.environ.get("LEDMATRIX_CORE")
+if core:
+    sys.path.insert(0, core)
+
+try:
+    import src.plugin_system.base_plugin  # noqa: F401
+except ModuleNotFoundError as exc:
+    # Skip only when the core itself is absent. A ModuleNotFoundError raised
+    # from *inside* the core -- a missing dependency, a broken import in
+    # base_plugin -- is a real failure, and reporting it as "no core here"
+    # would hide it behind a skip nobody reads.
+    if exc.name not in ("src", "src.plugin_system", "src.plugin_system.base_plugin"):
+        raise
+    print(f"SKIP: needs a LEDMatrix core on the path ({exc}); "
+          f"set LEDMATRIX_CORE or run via scripts/run_plugin_tests.py --core")
+    sys.exit(2)
+
+manifest = json.loads((PLUGIN_DIR / "manifest.json").read_text(encoding="utf-8"))
+class_name = manifest["class_name"]
+entry_point = manifest.get("entry_point", "manager.py")
+module_name = entry_point[:-3] if entry_point.endswith(".py") else entry_point
 
 try:
-    # Try to import the plugin
-    from manager import BasketballPluginManager
-    print("✓ Plugin imported successfully")
-    print(f"✓ Class name: {BasketballPluginManager.__name__}")
-    print(f"✓ Base classes: {BasketballPluginManager.__bases__}")
-    print("\nPlugin structure is valid!")
-    sys.exit(0)
-
+    module = __import__(module_name)
 except SyntaxError as e:
-    print(f"✗ Syntax error: {e}")
+    print(f"FAIL: syntax error in {entry_point}: {e}")
     sys.exit(1)
 except ImportError as e:
-    print(f"✗ Import error: {e}")
+    print(f"FAIL: {entry_point} could not be imported: {e}")
     sys.exit(1)
-except Exception as e:
-    print(f"✗ Error: {e}")
-    import traceback
-    traceback.print_exc()
+
+cls = getattr(module, class_name, None)
+if cls is None:
+    available = sorted(
+        n for n, v in vars(module).items() if isinstance(v, type)
+        and v.__module__ == module.__name__)
+    print(f"FAIL: manifest class_name is {class_name!r}, but {entry_point} "
+          f"defines no such class. It defines: {', '.join(available) or '(none)'}")
     sys.exit(1)
+
+print(f"PASS: {entry_point} imports and defines {class_name}")
+print(f"      base classes: {', '.join(b.__name__ for b in cls.__bases__)}")
+sys.exit(0)