Repository navigation
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: vitali87/code-graph-rag/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| assert report.applied, report.message | ||
| text = (root / "pkg/shapes.py").read_text() | ||
| # Both definitions are rewritten, so no caller may still say `factor`. | ||
| assert "factor=2" not in text, text |
There was a problem hiding this comment.
This assertion only verifies that factor=2 disappeared. It also passes if the call is removed or changed to an invalid keyword, so the regression test can succeed while the rewritten program is no longer callable. Assert the rewritten signature and Square().area(size=2), and preferably execute the rewritten fixture to check its result. This is a non-blocking test-coverage concern; the practical cost is that future regressions can silently ship.
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!
Artifacts
- Executed the prepared uv pytest command against the changed regression module; all six tests passed, showing the current weak assertion is accepted.
- Authored and executed the focused Python harness that invokes the real rewrite flow and tests two broken rewritten-call mutants, providing the reproducible validation method.
- Captured the command and complete contents of the exact mutation harness that was executed, making the oracle test reproducible.
- Executed the mutation harness: line 146 accepted the invalid-keyword and removed-call results while their runtime behavior was broken, confirming the test-oracle weakness.
| "def call():\n return target(1, 2, 3)\n", | ||
| ) | ||
| store, updater = _index(root) | ||
| with pytest.raises(SignatureRefused): |
There was a problem hiding this comment.
The bare pytest.raises(SignatureRefused) accepts any refusal, including one caused by an unrelated malformed parameter specification. Capture the exception and assert the positional-only-boundary diagnostic so this test specifically proves that moving the boundary is rejected. This is a non-blocking test-coverage concern; the practical cost is that the intended guard could regress without this test detecting it.
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!
Artifacts
- Authored and executed script constructs the same temporary indexed repository as the changed test and runs the intended and malformed-spec requests, demonstrating what the bare assertion accepts.
- Executed repro with the exact changed-test request; it raises SignatureRefused with the positional-only boundary message, establishing the intended behavior.
- Executed repro adds malformed `invalid@`; it still raises SignatureRefused but with a parameter-spec message, proving the bare assertion lacks diagnostic specificity.
- Executed the changed test node under pytest; it passed while asserting only the broad exception class, confirming the weak assertion is active.
Comments Outside DiffThese findings sit on lines the diff does not cover, so they could not be posted inline. Each one leaves this list once its file changes.
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Strengthen assertions for keyword rewriting and positional-only marker placement, and correct the misleading fixture docstring.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (3)
What changed in this PR
Adds regression tests for change_signature covering duplicate sources, override keyword rewriting, and positional-only parameters.
Changes:
- Tests duplicate-source rejection and distinct mappings.
- Covers override-specific keyword names.
- Tests positional-only marker preservation and boundary validation.
| File | Description |
|---|---|
codebase_rag/tests/test_change_signature_regressions.py |
Adds regression coverage for signature rewriting failures. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| assert report.applied, report.message | ||
| text = (root / "pkg/shapes.py").read_text() | ||
| # Both definitions are rewritten, so no caller may still say `factor`. | ||
| assert "factor=2" not in text, text |
| reingest=updater.reingest, | ||
| ) | ||
| assert report.applied, report.message | ||
| assert "/" in (root / "pkg/only.py").read_text().split("\n")[0] |
| Its own fixture, with no defaulted parameter: the shared one has | ||
| `b: str = 'x'` and a `helper(2)` site relying on it, which a rename leaves | ||
| possibly-missing and the postcondition rightly rolls back. That rollback is | ||
| the operation working correctly, so the shared fixture would be testing |
603be17 to
8a3e861
Compare
07334a1 to
8760ea7
Compare
Regression coverage for duplicate sources, override keywords, positional-only markers, and related failures.
Files changed
codebase_rag/tests/test_change_signature_regressions.pyDiff stats
+206 additions, -0 deletions (206 LOC)
Dependencies
This PR depends on:
pr-7Dependency graph
Merge in this order: