test: add 306 new tests covering previously untested modules - #347
Merged
Conversation
Adds test coverage for six major untested areas: - src/base_classes/api_extractors.py — ESPN football, baseball, hockey, soccer extractors - src/base_classes/data_sources.py — ESPN, MLB, and soccer API data sources (HTTP mocked) - src/common/game_helper.py — game extraction, filtering, sorting, and summaries - src/common/utils.py — all utility functions (normalise, format, validate, parse) - src/common/scroll_helper.py — ScrollHelper init, create, update, visible portion, duration - src/background_data_service.py — cache hit/miss paths, retry, cancel, cleanup, singleton - src/vegas_mode/config.py — VegasModeConfig from_config, validate, update, ordering - src/logo_downloader.py — normalize_abbreviation, filename variations, directory helpers - src/plugin_system/health_monitor.py — HealthStatus determination, metrics, suggestions, lifecycle https://claude.ai/code/session_015792DiGo27JbgH5mk3KBjk
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR introduces comprehensive test coverage across nine test modules, validating API extractors, data sources, background service, game processing utilities, display helpers, configuration, and health monitoring without modifying any implementation code. ChangesComprehensive Test Coverage for Core Modules
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| ErrorProne | 13 high |
🟢 Metrics 380 complexity · 10 duplication
Metric Results Complexity 380 Duplication 10
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}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (2)
test/test_health_monitor.py (2)
24-32: 💤 Low valueConsider adding type hints to helper function.
Adding type hints to
_make_health_trackerwould improve clarity and align with the guideline to use type hints for function parameters and return values.📝 Proposed enhancement
def _make_health_tracker( - summary: dict | None = None, - all_summaries: dict | None = None, -): + summary: dict | None = None, + all_summaries: dict | None = None, +) -> MagicMock: """Return a mock PluginHealthTracker."""🤖 Prompt for 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. In `@test/test_health_monitor.py` around lines 24 - 32, Add explicit type hints to the helper _make_health_tracker: annotate parameters (e.g., summary: Optional[Dict[str, Any]] and all_summaries: Optional[Dict[str, Any]] or a more specific nested Dict if applicable) and add a return type of MagicMock. Import the necessary types (Optional, Dict, Any) from typing and ensure MagicMock is imported from unittest.mock so get_health_summary and get_all_health_summaries calls are reflected by the typed MagicMock return.
284-284: 💤 Low valueConsider adding a small grace period for thread startup validation.
Checking
is_alive()immediately afterstart_monitoring()could be racy on a Raspberry Pi with limited CPU resources, where thread startup may not be instantaneous. Consider adding a small retry loop or brief sleep to make the test more robust.🔄 Proposed enhancement
+import time + def test_start_monitoring(self, monitor_with_cleanup): monitor = monitor_with_cleanup monitor.start_monitoring() assert monitor._monitor_thread is not None - assert monitor._monitor_thread.is_alive() + # Give thread time to start on resource-constrained Raspberry Pi + time.sleep(0.1) + assert monitor._monitor_thread.is_alive()Or use a retry loop:
def test_start_monitoring(self, monitor_with_cleanup): monitor = monitor_with_cleanup monitor.start_monitoring() assert monitor._monitor_thread is not None # Retry for up to 1 second for _ in range(10): if monitor._monitor_thread.is_alive(): break time.sleep(0.1) else: pytest.fail("Monitor thread did not start within 1 second")As per coding guidelines: "Optimize code for Raspberry Pi's limited RAM and CPU capabilities."
🤖 Prompt for 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. In `@test/test_health_monitor.py` at line 284, The test asserts monitor._monitor_thread.is_alive() immediately after calling monitor.start_monitoring(), which is racy on low-CPU devices; modify the test (around monitor.start_monitoring and inspect monitor._monitor_thread) to wait briefly or retry until the thread becomes alive (e.g., loop with small sleep intervals for up to ~1s and fail if still not alive) so the assertion only runs once startup is observed; ensure you still assert monitor._monitor_thread is not None before the retry loop and use pytest.fail with a clear message if the thread never starts.
🤖 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 `@test/test_api_extractors.py`:
- Line 9: Remove the unused import of pytest from the top of the test module:
delete the "import pytest" statement so the module no longer imports the unused
symbol (pytest) and lint checks complaining about unused imports are resolved.
- Around line 22-28: Remove the unused pytest import from the test file: open
test/test_api_extractors.py and delete the top-level "import pytest" since it is
not referenced anywhere (the helper function _make_espn_event uses the Python
3.10 union type `dict | None` and should not be changed); ensure no other
references to pytest remain in the file and run the test suite to confirm no
import-related errors.
In `@test/test_background_data_service.py`:
- Line 12: Remove the unused imports in test/test_background_data_service.py:
delete the "Future" import from concurrent.futures and the unused "bds_module"
import so the module only imports symbols actually referenced by the tests (this
will satisfy static analysis and linters).
In `@test/test_data_sources.py`:
- Around line 9-11: The module header imports include unused symbols—remove the
unused imports `date`, `MagicMock`, and `pytest` from the top of
test/test_data_sources.py so only required imports remain (e.g., keep
`datetime`, `patch`, and `Mock` if they are used elsewhere); update the import
line(s) accordingly to eliminate lint noise.
In `@test/test_game_helper.py`:
- Line 9: Remove the unused import of pytest from the top of the module: delete
the line importing pytest in test/test_game_helper.py so there are no unused
imports; ensure no other code in the file (functions or tests) relies on pytest
being imported directly (they should rely on the pytest runner instead).
In `@test/test_health_monitor.py`:
- Line 10: The test file imports both MagicMock and patch from unittest.mock but
patch is unused; remove the unused import by deleting "patch" from the import
statement (the symbol to change is the import line that currently reads "from
unittest.mock import MagicMock, patch") so that only MagicMock remains imported.
- Around line 281-298: The tests (test_start_monitoring, test_stop_monitoring,
test_double_start_no_duplicate_threads) can leak the monitor thread if an
assertion fails; ensure the monitor's stop_monitoring() is always called by
converting the plain fixture to a pytest fixture with a teardown/finalizer or by
wrapping the body of each test in a try/finally that calls
monitor.stop_monitoring(); locate references to monitor.start_monitoring(),
monitor.stop_monitoring(), and monitor._monitor_thread in these tests and
guarantee cleanup in a finally block or fixture finalizer so threads are cleaned
even on assertion failures.
In `@test/test_logo_downloader.py`:
- Around line 10-11: Remove the unused imports in test/test_logo_downloader.py:
delete the top-level imports of os and pytest and remove any unused mock imports
(Mock and MagicMock from unittest.mock) so only actually used imports remain;
search for symbols os, pytest, Mock, and MagicMock in that file (and lines
around the import block) and remove the import statements if there are no
references.
In `@test/test_scroll_helper.py`:
- Line 11: The import "patch" is unused in the test module—remove the unused
symbol by deleting "patch" from the import statement (i.e., change "from
unittest.mock import patch" to remove or replace it with only the mocks actually
used), or if a mock is intended, replace the import with the correct symbol and
use it in the tests; ensure no references to "patch" remain in the file (look
for the import at the top of the test module).
- Line 227: The test is allocating a huge 50,000px image via
helper.create_scrolling_image([_make_image(width=50000)]); change the test to
use a much smaller width (e.g., _make_image(width=500) or another small value)
that still exercises the max-duration clamping logic, and keep the same
assertions so the behavior is validated without large memory usage; update any
helper/test names referenced (helper.create_scrolling_image and _make_image)
accordingly.
In `@test/test_utils.py`:
- Line 10: Remove the unused import statement `import pytest` from
test/test_utils.py; locate the top-level import of `pytest` and delete it so the
file no longer contains an unused import (no other code changes required).
In `@test/test_vegas_config.py`:
- Line 8: Remove the unused import of pytest from the top of
test/test_vegas_config.py; locate the line that reads "import pytest" and delete
it so the module no longer contains an unused import (no other changes
required).
---
Nitpick comments:
In `@test/test_health_monitor.py`:
- Around line 24-32: Add explicit type hints to the helper _make_health_tracker:
annotate parameters (e.g., summary: Optional[Dict[str, Any]] and all_summaries:
Optional[Dict[str, Any]] or a more specific nested Dict if applicable) and add a
return type of MagicMock. Import the necessary types (Optional, Dict, Any) from
typing and ensure MagicMock is imported from unittest.mock so get_health_summary
and get_all_health_summaries calls are reflected by the typed MagicMock return.
- Line 284: The test asserts monitor._monitor_thread.is_alive() immediately
after calling monitor.start_monitoring(), which is racy on low-CPU devices;
modify the test (around monitor.start_monitoring and inspect
monitor._monitor_thread) to wait briefly or retry until the thread becomes alive
(e.g., loop with small sleep intervals for up to ~1s and fail if still not
alive) so the assertion only runs once startup is observed; ensure you still
assert monitor._monitor_thread is not None before the retry loop and use
pytest.fail with a clear message if the thread never starts.
🪄 Autofix (Beta)
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
Run ID: bda36848-b1fe-4ce5-ab50-1a54196526c7
📒 Files selected for processing (9)
test/test_api_extractors.pytest/test_background_data_service.pytest/test_data_sources.pytest/test_game_helper.pytest/test_health_monitor.pytest/test_logo_downloader.pytest/test_scroll_helper.pytest/test_utils.pytest/test_vegas_config.py
- test_health_monitor.py: wrap start_monitoring calls in try/finally so the background thread is always stopped even when an assertion fails - test_scroll_helper.py: reduce 50,000px test image to 5,000px to avoid unnecessary memory pressure on Raspberry Pi Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This was referenced Aug 1, 2026
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
3 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.
Adds test coverage for six major untested areas:
https://claude.ai/code/session_015792DiGo27JbgH5mk3KBjk
Summary by CodeRabbit