Repository navigation
docs(review): a new gate makes existing tests fail like a stale expectation (#17054) - #17396
Conversation
…tation (#17054) Three fixtures in one PR hit the same defect and twice the suggested repair would have deleted the coverage it appeared to fix. When a change adds an authorization gate upstream, every test whose fixture predates it short-circuits before reaching its own subject. The failure reads as `assert <new> == <old>` with a plausible new value, which is indistinguishable in a CI log from an expectation that has simply drifted. The obvious repair -- update the assertion -- goes green and removes the test's real subject. Observed on #17054's desktop-authz gate: - two VNC handshake tests whose fake user carried no `role` began reporting the gate's `close(1008)` where the test asserted the handshake's `1011`; 1011 was never a changed code, it was a code no longer REACHED - a third, `role: "user"`, stopped reaching `accept()` at all Re-pinning those expectations would have removed the only coverage of the handshake-failure path and of the accept path, and left the gate's own headline case -- a signed-in account without the permission -- covered by nothing. The tell recorded with it is the cheap one: if the "corrected" expectation duplicates what the new gate's own suite already asserts, the test has stopped testing its own subject. Docs only; no code or test behaviour changes. Refs #17054
|
Warning Review limit reachedNext included review available in 44 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced 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 |
ApproveReviewed at The sharpest line is the tell, and it is the part I would not have thought to state:
That is checkable by a reviewer in seconds, unlike "think about whether the value came from the Two notes, neither blocking: No size ceiling applies. I checked for a The pattern has a mechanizable half, for later rather than here. "An expectation moved to match Ledger: |
1 participant
Thinking Path
Three test fixtures in one PR (#17054) failed for the same reason, and twice the repair I suggested would have deleted the coverage it appeared to fix. That is worth a line in the self-review list, because the failure is indistinguishable in a CI log from ordinary expectation drift.
A change adds an authorization gate upstream. Every test whose fixture predates it now short-circuits before reaching its own subject. The log says
assert <new> == <old>with a plausible new value, so the obvious move is to update the assertion — which goes green and removes the test's real subject.I made this mistake twice in one afternoon from the reviewing seat:
assert 1008 == 1011as a corrected close code (1008 Policy Violation being right for an authz refusal where 1011 Internal Error was wrong) and advised updating it with a comment. Wrong: the tests patchedauthenticate_websocketto return a user with norole, so the gate refused before the handshake ran. 1011 was never a changed code, it was a code no longer reached. Re-pinning would have made both tests assert authz — already covered by the gate's own suite — and removed the only coverage of the handshake-failure path.role: "user", stopped reachingaccept()at all. Fixing only its fixture would have left the gate's headline case — a signed-in account withoutmcp.desktop.control— covered by nothing, because the broken fixture was exercising it by accident.The cheap tell, which is what makes this checkable rather than a matter of judgement: if the "corrected" expectation duplicates what the new gate's own suite already asserts, the test has stopped testing its own subject.
What Changed
One entry appended to
docs/developer/CLAUDE_REVIEW.md's "Self-review before pushing: known self-inflicted patterns" list, as item 9, in the existing numbered format with its citation.It records the question to ask (did the observed value come from the subject, or from something that now precedes it?), the remedy (fix the fixture, not the expectation), the worked instances, and the duplicate-assertion tell.
Nothing else. No code, no test, no behaviour.
Verification
Docs-only:
git diff --name-only origin/main...HEADreturns exactly that one path. No.py,.tsor.vuefile is touched, so no test behaviour can change.The three instances cited are real and were each verified against the tree at the time:
vnc_proxy_handshake_wiring_test.py(x2){"username": "alice"}, norolevnc_proxy_websocket_auth_test.py{"username": "alice", "role": "user"}accept()never awaiteduserholds nomcp.desktop.control; on mergedmainonlyRole.ADMINandRole.OPERATORdo.The entry is advisory prose in a checklist that already carries eight such entries, so there is nothing here a guard could assert — it is read by a human before pushing, which is what the section is for.
Refs #17054
Model Used
Claude Opus 5 (1M context).