test(mcp):add-unit-tests-for-execute_and_observe - #234
tonythethompson wants to merge 1 commit into
Conversation
Add hypothesis property-based tests covering: - Property 1: Timeout Clamping Invariant - verifies _clamp_timeout produces values in [10, 1800] for any integer input and defaults to 600 for None (Requirements 1.10, 1.11, 1.12) - Property 2: Side-Effect Field Correctness - verifies side_effect: True is present on successful submissions and absent on pre-submission errors (Requirement 2.3) All tests use mocked studio_request with @settings(max_examples=100). Validates: Requirements 1.10, 1.11, 1.12, 2.3
|
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
|
Warning Review limit reached
Next review available in: 32 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 |
tonythethompson
changed the title
Code Review by Qodo
1. Test codifies lost zero exit code
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| # Note: exit_code=0 is treated as falsy by the `or` fallback in the impl, | ||
| # so it becomes None. Non-zero exit codes are captured correctly. | ||
| assert result["exit_code"] is None |
There was a problem hiding this comment.
1. Test codifies lost zero exit code 🐞 Bug ≡ Correctness
The success test explicitly expects exitCode: 0 to become None, so it locks in the implementation's falsy-value bug instead of verifying the documented exit-code result. A successful job with exit code zero is a normal outcome and should remain observable; this regression would pass the new test suite unnoticed.
Agent Prompt
## Issue description
The successful completion test supplies `exitCode: 0` but asserts that the returned `exit_code` is `None`, thereby accepting incorrect result metadata.
## Issue Context
`execute_and_observe` currently selects exit-code fields with a truthiness-based `or` expression, which discards a legitimate zero value. Update the implementation to distinguish a missing key/value from a value of zero, and make the test assert `result["exit_code"] == 0`.
## Fix Focus Areas
- olive-mcp-server/tests/test_agent_execute.py[65-67]
- olive-mcp-server/olive_mcp_server/tools/agent_execute.py[155-158]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
PR Summary by QodoAdd unit tests for execute_and_observe
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Qodo FixerNo findings are within the configured fix scope. To change which findings are fixed, adjust the setting on your Qodo configuration page. |
![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: d3e5b48e38
ℹ️ 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".
| assert result["logs"] == ["step 1", "done"] | ||
| # Note: exit_code=0 is treated as falsy by the `or` fallback in the impl, | ||
| # so it becomes None. Non-zero exit codes are captured correctly. | ||
| assert result["exit_code"] is None |
There was a problem hiding this comment.
When Studio reports exitCode: 0 for a completed job, that value is meaningful evidence of successful execution, but this assertion deliberately codifies its conversion to None. It will therefore block a correction to the or-based extraction in execute_and_observe and leaves callers unable to distinguish a clean exit from a missing exit code; assert 0 here and fix the extraction to fall back only when the camel-case key is absent or None.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR adds unit coverage for
Confidence Score: 4/5The PR appears safe to merge, with non-blocking test-quality issues around exit-code expectations and incomplete property-based coverage. The change affects tests only and introduces no production runtime behavior, but one assertion protects a known lossy result mapping and the timeout invariant is checked against only a small fixed sample. Files Needing Attention: olive-mcp-server/tests/test_agent_execute.py
|
| Filename | Overview |
|---|---|
| olive-mcp-server/tests/test_agent_execute.py | Adds broad mocked coverage, but codifies loss of exit code zero and does not implement the advertised property-generated timeout testing. |
Prompt To Fix All With AI
### Issue 1
olive-mcp-server/tests/test_agent_execute.py:65-67
**Zero exit code is discarded**
This assertion codifies the existing falsy fallback that converts Studio's valid `exitCode: 0` into `None`. It protects a lossy mapping that makes successful completion indistinguishable from a response where no exit code was received, while the sibling Studio job mapping correctly preserves zero.
### Issue 2
olive-mcp-server/tests/test_agent_execute.py:295-306
**Timeout property uses fixed examples**
The timeout invariant is tested against only eight hand-picked values rather than generated integers. A clamping regression affecting any unlisted range can therefore pass this suite, so it does not provide the property-based coverage described by the PR.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "test: add property tests for execute_and..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # Note: exit_code=0 is treated as falsy by the `or` fallback in the impl, | ||
| # so it becomes None. Non-zero exit codes are captured correctly. | ||
| assert result["exit_code"] is None |
There was a problem hiding this comment.
This assertion codifies the existing falsy fallback that converts Studio's valid exitCode: 0 into None. It protects a lossy mapping that makes successful completion indistinguishable from a response where no exit code was received, while the sibling Studio job mapping correctly preserves zero.
Prompt To Fix With AI
This is a comment left during a code review.
Path: olive-mcp-server/tests/test_agent_execute.py
Line: 65-67
Comment:
**Zero exit code is discarded**
This assertion codifies the existing falsy fallback that converts Studio's valid `exitCode: 0` into `None`. It protects a lossy mapping that makes successful completion indistinguishable from a response where no exit code was received, while the sibling Studio job mapping correctly preserves zero.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
|
|
||
| assert result["error"] == "internal_error" | ||
| assert "RuntimeError" in result["message"] | ||
| assert "side_effect" not in result | ||
| _json_round_trip(result) | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Test: Timeout clamping | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
There was a problem hiding this comment.
The timeout invariant is tested against only eight hand-picked values rather than generated integers. A clamping regression affecting any unlisted range can therefore pass this suite, so it does not provide the property-based coverage described by the PR.
Prompt To Fix With AI
This is a comment left during a code review.
Path: olive-mcp-server/tests/test_agent_execute.py
Line: 295-306
Comment:
**Timeout property uses fixed examples**
The timeout invariant is tested against only eight hand-picked values rather than generated integers. A clamping regression affecting any unlisted range can therefore pass this suite, so it does not provide the property-based coverage described by the PR.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Superseded by consolidated PR #245 |
1 participant
Summary
Adds hypothesis property-based tests for
execute_and_observe(task 3.2 from v0.3-agent-mcp-tools spec).Property 1: Timeout Clamping Invariant
_clamp_timeout(T)is in [10, 1800]Property 2: Side-Effect Field Correctness
side_effect: Trueside_effectkeyside_effect: Trueside_effectkeyTest Details
@settings(max_examples=100)per hypothesis propertyunittest.mock.patchHow to run
cd olive-mcp-server python -m pytest tests/test_agent_execute_props.py -v