ci: run the plugins' own tests, and repair the ones that could not pass - #265
Merged
Conversation
This workflow was the safety harness plus manifest checks, so a plugin's test_*.py guarded nothing unless someone ran it by hand. That is how a display() returning None on every path survived from February to August (#263): the harness renders screens and cannot see a return value, and its fixtures seed data so an empty-state path never executes. scripts/run_plugin_tests.py already existed, with the exit-code convention that makes this gateable -- 2 for "prerequisites absent", 1 for a real failure. It was simply never wired up. Baseline across the fleet was 75 passed, 2 skipped, 9 failed, which is why: nine reds nobody could act on are not a gate, they are noise. All nine were stale tests or environment handling, not plugin bugs: baseball asserted plugin.initialized and iterated plugin.leagues, neither of which exists; its config was flat (mlb_enabled) where the plugin reads nested (config["mlb"]["enabled"]), so it ran with every league disabled and passed vacuously baseball the antialiasing test only found the core fonts when the plugin sat inside a core checkout, so it always skipped basketball imported BasketballPluginManager, a name the plugin had stopped using, from a hardcoded /home/chuck path; it now reads class_name from the manifest, which is what the loader uses, so a rename cannot pass unnoticed basketball MockLogger lacked setLevel and %-formatting, so real code under test raised inside the stub basketball its mock competitors carried a score but no team block or competitor id, which the extractor needs before it ever looks at a score -- so all five score formats reported "could not extract" for an unrelated reason hockey constructed CacheManager(config_manager=...), removed from the core's signature, and resolved config/ relative to the plugin dir; it now mirrors the pre-flight in the baseball suite and runs 29 real render cycles hockey test_recent_games.py deleted: a print-only script with zero assertions, a hardcoded /home/chuck path and a hardcoded debug date, broken since the monorepo migration lacrosse asked ESPN for the upcoming season, which is unpublished in August; it now falls back to the last completed season and skips, rather than fails, if neither has fixtures soccer asserted a bare "MCI" cache key after logo caching became size-scoped ("MCI@32x32") to stop panel sizes colliding weather almanac fonts now resolve via LEDMATRIX_CORE (so it runs instead of failing), and astral's absence is a skip, since the plugin declares it and CI installs it Now 83 passed, 2 skipped, 0 failed. The two skips are honest: one script prompts on stdin, and astral is absent locally but present in CI. The changed-plugin detection now yields two lists. Shipped-code changes still drive the version gate and harness; test-only edits no longer do, which the existing test/ exclusion had always intended but missed for the root-level test_*.py where most of these live. A second list including tests drives the unit-test run, so a PR that only edits a test still has to prove it passes. Also gitignores emulator_config.json, which RGBMatrixEmulator drops into whatever directory the suites are invoked from. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
Contributor
|
Warning Review limit reached
Next review available in: 51 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe pull request updates the plugin test workflow and aligns plugin tests with current registry, manifest, emulator, fixture, font, dependency, and cache behavior. It also adds an emulator configuration ignore rule and removes an obsolete hockey diagnostic script. ChangesPlugin test reliability
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 23 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/baseball-scoreboard/test_baseball_plugin.py`:
- Around line 177-180: Update the test’s plugin initialization flow around
plugin._league_registry to assert that enabled equals ["mlb"] before returning
the plugin, while retaining the existing diagnostic print.
In `@plugins/basketball-scoreboard/test_plugin_syntax.py`:
- Around line 33-38: Update the import handlers in
plugins/basketball-scoreboard/test_plugin_syntax.py lines 33-38 and
plugins/ledmatrix-weather/test_almanac_moon_data.py lines 29-37 to catch
ModuleNotFoundError, skip only when exc.name is the expected root package
(src.plugin_system.base_plugin in the scoreboard test and astral in the weather
test), and re-raise all other import failures.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 01d13376-46f0-4e1c-b63f-38fe43cd405d
📒 Files selected for processing (12)
.github/workflows/test-plugins.yml.gitignoreplugins/baseball-scoreboard/test_baseball_plugin.pyplugins/baseball-scoreboard/test_score_antialiasing.pyplugins/basketball-scoreboard/test_plugin_syntax.pyplugins/basketball-scoreboard/test_score_fix_verification.pyplugins/hockey-scoreboard/test_hockey_emulator.pyplugins/hockey-scoreboard/test_recent_games.pyplugins/lacrosse-scoreboard/test_lacrosse_plugin.pyplugins/ledmatrix-weather/test_almanac_layout.pyplugins/ledmatrix-weather/test_almanac_moon_data.pyplugins/soccer-scoreboard/test_live_screens.py
💤 Files with no reviewable changes (1)
- plugins/hockey-scoreboard/test_recent_games.py
Two review findings, both about a check that could pass without meaning anything -- which is the failure this PR exists to remove. The baseball suite printed which leagues were enabled but never asserted it. That is exactly how it came to run green over a plugin with every league disabled, so a silent return to the flat-config state would have gone unnoticed again. Assert MLB is the only one enabled. The two skip guards caught ImportError broadly, so a ModuleNotFoundError raised from *inside* the core, or from inside astral, would have been filed as "not installed" and skipped. Match on exc.name and re-raise anything else: a genuine breakage should be a failure, not a skip nobody reads. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the last gap from the hockey blank-panel post-mortem (#263, merged): CI never ran a plugin's own
test_*.py.Why this matters
The Plugin Safety workflow was the harness plus manifest checks. A plugin unit test guarded nothing unless somebody ran it by hand — which is how a
display()returningNoneon 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.scripts/run_plugin_tests.pyalready existed, with the exit-code convention that makes this gateable (2= prerequisites absent,1= real failure). It was simply never wired up. Its own docstring explains why the convention matters:Baseline across the fleet was 75 passed, 2 skipped, 9 failed — which is exactly that problem, and why it couldn't just be switched on.
Triage: all nine were stale tests, not plugin bugs
I checked each one for a real defect before touching it.
AttributeError: no attribute 'initialized'plugin.initializedand iteratedplugin.leagues— neither has ever existed. Its config was flat (mlb_enabled) where the plugin reads nested (config["mlb"]["enabled"]), so it ran with every league disabled and passed vacuously ("Enabled leagues: []" on a green run)LEDMATRIX_COREand runscannot import BasketballPluginManager/home/chuckpath. Now readsclass_namefrom the manifest — what the loader uses — so a rename can't pass unnoticedMockLogger has no setLevelLoggerteamblock or competitorid, which the extractor needs before it looks at a score. All five score formats failed for an unrelated reasonCacheManager() got config_managerconfig/relative to the plugin dir. Now mirrors baseball's pre-flight and runs 29 real render cyclesNo module named cache_manager/home/chuckpath and a hardcoded debug date (2025-10-18). Broken since the migration; never gated anything even when it worked"MCI"key after logo caching became size-scoped ("MCI@32x32") so panel sizes can't collide. Logos were loading fineastralLEDMATRIX_CORE(runs instead of failing);astralis declared in the plugin'srequirements.txt, which CI installs, so its local absence is a skipNow 83 passed, 2 skipped, 0 failed. Both skips are honest: one script prompts on stdin, and
astralis absent locally but installed in CI.The workflow change
Changed-plugin detection now produces two lists:
ids— shipped code only. Drives the version gate and harness. Test-only edits are excluded, which the existingtest/exclusion always intended but missed for the root-leveltest_*.pywhere most of these actually live. Without this, this very PR would have demanded version bumps on six plugins for touching nothing a user receives.all_ids— any change including tests. Drives the unit-test run, so a PR that only edits a test still has to prove it passes.The new step runs with
if: always()so a harness failure doesn't hide these and vice versa, and installs each plugin'srequirements.txtitself rather than relying on the harness step having got that far.Also gitignores
emulator_config.json, which RGBMatrixEmulator drops into whatever directory the suites are invoked from (ledmatrix-music's committed copy stays tracked — gitignore doesn't apply to files already in the index).Note for future test authors
Adding a plugin test that can't pass in CI will now fail the build. Make it skip deliberately (print
SKIP: <reason>, exit 2) rather than leaving it red.🤖 Generated with Claude Code
https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
Summary by CodeRabbit
Bug Fixes
Tests
Chores