test(mcp): add property tests for compare_results scoring (task 6.2) - #239
tonythethompson wants to merge 1 commit into
Conversation
Property 6: Preference-Weighted Scoring (validates Req 7.3, 7.8) - 5 hypothesis property tests verifying _normalize_and_score: - winner always has highest score - all scores in [0.0, 1.0] - balanced preference gives equal weights - specific preferences still produce valid scores - scoring preserves job IDs - 3 integration property tests via full compare_results (mocked studio): - winner has highest score - scores in valid range - JSON round-trip - 4 direct weight verification example tests: - latency preference doubles latency weight - size preference doubles size weight - accuracy preference doubles accuracy weight - balanced weights symmetric Uses @settings(max_examples=100) per design spec.
|
Deployment failed for project olive-studio with the following error: Learn More: https://vercel.com/trackdub?upgradeToPro=build-rate-limit |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @tonythethompson, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
PR Summary by Qodotest(mcp): property tests for compare_results preference-weighted scoring
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo
1. Preference properties do not verify weights
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| # Re-compute manually with all weights = 1.0 | ||
| # Since balanced already uses 1.0 for all, scores should match. | ||
| for entry in scored_balanced: | ||
| assert isinstance(entry["score"], float) | ||
| assert 0.0 <= entry["score"] <= 1.0 |
There was a problem hiding this comment.
1. Preference properties do not verify weights 🐞 Bug ⚙ Maintainability
test_balanced_gives_equal_weights and test_specific_preference_increases_metric_contribution only check score type/range, never comparing results with an independently calculated weighted score or preference-specific ordering. An implementation that ignored preference could pass these property tests, leaving the claimed property coverage incomplete despite the separate deterministic examples.
Agent Prompt
## Issue description
The balanced and specific-preference property tests do not verify weighting; they only assert that output scores are in range.
## Issue Context
The direct examples provide partial coverage, but the generated properties should validate the scoring formula across arbitrary metric sets. Compute min/max normalization independently in the test, apply the expected 1x/2x weights while skipping missing metrics, and compare expected scores with the function output using an appropriate floating-point tolerance.
## Fix Focus Areas
- olive-mcp-server/tests/test_agent_compare_props.py[115-155]
- olive-mcp-server/olive_mcp_server/tools/agent_compare.py[96-146]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| # Find the max score | ||
| max_score = max(entry["score"] for entry in scored) | ||
| winner = max(scored, key=lambda x: x["score"]) | ||
|
|
||
| assert winner["score"] == max_score |
There was a problem hiding this comment.
2. Winner property is tautological 🐞 Bug ⚙ Maintainability
The direct property test computes both max_score and winner from the same returned list, so its assertion passes for every nonempty output and does not validate scoring or winner selection. The integration test separately checks that compare_results reports an entry with the highest returned score, but neither test independently derives the expected winner from the input metrics.
Agent Prompt
## Issue description
The direct winner property selects the maximum result and then asserts that it is the maximum, making the property vacuous.
## Issue Context
Retain the integration invariant if useful, but independently calculate expected normalized weighted scores from `metrics_list`, identify the expected job ID, and compare it with the scored output. Define the expected tie behavior if ties are possible.
## Fix Focus Areas
- olive-mcp-server/tests/test_agent_compare_props.py[80-97]
- olive-mcp-server/tests/test_agent_compare_props.py[176-207]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Qodo FixerNo findings are within the configured fix scope. To change which findings are fixed, adjust the setting on your Qodo configuration page. |
|
Warning Review limit reached
Next review available in: 50 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: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
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 |
Greptile SummaryThe PR adds property-based and example tests for preference-weighted comparison scoring, including direct scoring checks and mocked
Confidence Score: 4/5The PR should not merge until Hypothesis is added to the MCP server's development dependencies so CI can collect the new tests. The new test file imports Hypothesis, while CI installs only the declared Files Needing Attention: olive-mcp-server/tests/test_agent_compare_props.py; olive-mcp-server/pyproject.toml
|
| Filename | Overview |
|---|---|
| olive-mcp-server/tests/test_agent_compare_props.py | Adds comprehensive Hypothesis scoring tests, but the unconditional Hypothesis imports cannot be collected in the currently declared CI development environment. |
Prompt To Fix All With AI
### Issue 1
olive-mcp-server/tests/test_agent_compare_props.py:18-19
**Undeclared Hypothesis test dependency**
When CI installs the declared `dev` extra and collects this module, the unconditional Hypothesis imports fail with `ModuleNotFoundError`, preventing the MCP test suite from running. Add Hypothesis to the package's development dependencies.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "test(mcp): add property tests for compar..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| from hypothesis import given, settings | ||
| from hypothesis import strategies as st |
There was a problem hiding this comment.
When CI installs the declared dev extra and collects this module, the unconditional Hypothesis imports fail with ModuleNotFoundError, preventing the MCP test suite from running. Add Hypothesis to the package's development dependencies.
Prompt To Fix With AI
This is a comment left during a code review.
Path: olive-mcp-server/tests/test_agent_compare_props.py
Line: 18-19
Comment:
**Undeclared Hypothesis test dependency**
When CI installs the declared `dev` extra and collects this module, the unconditional Hypothesis imports fail with `ModuleNotFoundError`, preventing the MCP test suite from running. Add Hypothesis to the package's development dependencies.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4299d1a4f4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| from hypothesis import given, settings | ||
| from hypothesis import strategies as st |
There was a problem hiding this comment.
The clean python-tests job installs only .[dev] (.github/workflows/ci.yml:116-120), while that extra currently contains only pytest (olive-mcp-server/pyproject.toml:18-21). Consequently this unconditional import raises ModuleNotFoundError: No module named 'hypothesis' during collection and prevents the entire MCP test suite from running; add Hypothesis to the dev extra.
AGENTS.md reference: AGENTS.md:L89-L89
Useful? React with 👍 / 👎.
| # Each preference-weighted scoring must still produce valid scores | ||
| for scored in [scored_balanced, scored_latency, scored_size, scored_accuracy]: | ||
| for entry in scored: | ||
| assert 0.0 <= entry["score"] <= 1.0 |
There was a problem hiding this comment.
For generated inputs, this test only checks that the resulting scores remain in range and never compares them with the expected weighted-average formula. For example, changing every preferred weight from 2x to 3x would still pass this property and all three fixed winner examples, despite violating the requirement this test claims to validate; compute the expected normalized score for each preference and assert equality.
Useful? React with 👍 / 👎.
|
Superseded by consolidated PR #245 |
This branch was successfully deployed
1 participant
Property 6: Preference-Weighted Scoring (validates Req 7.3, 7.8)
Uses @settings(max_examples=100) per design spec.