| name | reviewing-test-quality |
| description | Reviews tests for quality: behavior vs implementation coupling, over-mocking, meaningful branch/edge coverage on the diff, regression tests for bug fixes, isolation and determinism (no shared state, real clocks, or unseeded randomness), right level per the pyramid/trophy, and disabled/focused tests sneaking in. Use when reviewing test files, test coverage, mocks, fixtures, flaky tests, or a bug fix's tests. |
| provenance | {"taxonomy_version":"v0.9","built_from":[{"category":17,"source":"docs/research/cluster-5-verification.md#17","hash":"f0616b31db4d8d43239b0d9bf58badf4d25568a4f6d45542055ea74b168ad174"}]} |
reviewing-test-quality
Do the tests prove anything? Behavior coupling, over-mocking, edge coverage, determinism.
When to use
Reviews tests for quality: behavior vs implementation coupling, over-mocking, meaningful branch/edge coverage on the diff, regression tests for bug fixes, isolation and determinism (no shared state, real clocks, or unseeded randomness), right level per the pyramid/trophy, and disabled/focused tests sneaking in. Use when reviewing test files, test coverage, mocks, fixtures, flaky tests, or a bug fix's tests.
Shape: diff. Written for concrete code; not meant for design docs or plans.
Reviewer discipline
Report only real problems. If the code correctly handles the case, reply "No findings" and stop — do not invent issues. This guards against false positives on correct code; still report every genuine issue you do find, with its full detail.
Defects are the default; improvements are opt-in. By default this lens is defect-only: do not suggest changes to code that is already correct. When the team has opted up into improvement suggestions, a finding on already-correct code is admissible only as nit-severity, route: implementer (the author applies, defers, or ignores), and must clear the non-configurable anti-churn floor: it must genuinely improve — never offer a merely equivalent alternative — and must converge (once a dimension is as good as you can confidently make it, stop; never oscillate A→B then B→A, never re-order to an equivalent state). Defects keep the strict bar above regardless of this setting.
Team preferences. If the reviewed repo has .code-quality-atlas/preferences.md, apply it before reporting: a repo's .code-quality-atlas/preferences.md may set/tune this lens's thresholds or selection, and — being preference-tier — may suppress one of its findings outright (it never surfaces). Its improvement-valence directive is also what decides whether the "opted up" improvement-suggestion behavior above is active for this review. Absent the file, apply this lens's defaults exactly as written above.
Pre-existing defects in touched code are surfaceable, not yours to fix. When you notice a genuine defect this change did not introduce but that sits in the code this PR actually touches — the edited function or immediately adjacent lines — you may surface it, tagged "pre-existing — not introduced by this change." Like improvements it is opt-in and default-quiet (off unless the team opts up), route: implementer, and non-blocking: it informs the author's fix-now / file-a-ticket / ignore call and never sets this PR's verdict, because the diff did not cause it. Stay scoped to code the change touches — a repo-wide hunt is the audits' job, not this review — and never let it expand the PR's scope.
Top checks
The head of the full checklist — enough for a first pass without opening any reference file:
- Does each test have a single reason to fail — assertions that localize the cause when it breaks? Flag Assertion Roulette (a pile of asserts with no messages, so a failure doesn't say which expectation broke) and tests that bundle several unrelated behaviors (Beck specific; Farley atomic).
- Do new/changed tests assert observable behavior (inputs→outputs, side effects), not internal calls/private state (refactor-resistant)?
- Is coverage meaningful on the new code — branches and edge cases, not just lines executed? Don't chase a % with assertion-free or tautological tests (asserting the mock, restating the framework, or a Sensitive Equality check on a whole serialized blob that breaks on unrelated change), and don't keep tests that pin no real requirement (Farley necessary).
- Bug fix → is there a regression test that fails before the fix and passes after?
- Are tests isolated and deterministic — no shared mutable state, order dependence, or real clock/network/unseeded random (flaky risk)?
- Is the test at the right level (pyramid/trophy) — logic in fast unit/integration, e2e reserved for critical journeys?
- Over-mocking smell: do mocks assert on implementation calls so a refactor breaks tests without behavior changing? Reach for the least-powerful test double — prefer a real collaborator, fake, or stub, and reserve a behavior-verifying mock for true outgoing commands (don't mock queries or value objects) (cross #11).
- Are edge/boundary cases covered (empty, null, max, error paths) — where the bugs live? Walk the CORRECT dimensions (Conformance, Ordering, Range, Reference, Existence, Cardinality, Time) to surface the missing edge (cross #1).
- Would the suite catch a real bug, not just execute lines? Apply mutation intuition — for a pure, deterministic, fast-to-test unit, prefer actually running a mutation tool (cheap, high-signal) over eyeballing it; otherwise high coverage masks weak assertions.
Mechanizing these checks
Where a finding here is one a tool can catch deterministically, surface that as an advisory route: implementer note next to the finding: the hand review caught it this time, and wiring the matching tool from reference/tool-rules.md into CI gates it going forward. This is a suggestion to mechanize, not a defect — it never blocks a verdict, and it falls away on a repo that already runs the tool.
Process notes. If this lens misfired on this change — flagged correct code, missed an obvious issue squarely in its own scope, or its checklist didn't fit the change shape — say so in one line under synthesizing-review-findings's Process notes appendix; that is not a defect finding. Say nothing if the lens worked as intended — never invent a process note to fill the section.
Going deeper
- reference/heuristics.md — the full checklist; open it when the change sits squarely in this lens's domain.
- examples.md — concrete good/bad findings, and the output format to match.
- reference/tool-rules.md — static-analysis rules covering the mechanical subset; for wiring up linters, not needed for the judgment review itself.
- reference/sources.md — the research behind each check; for provenance, not needed during a review.
