test-quality-review

Test quality review lens

"Tests pass" means something only if the tests could have failed. For each new or changed test ask: which behaviour breaks it, and is that the behaviour the acceptance criterion names? Live runs showed the common miss: tests existed, the suite was green, and the AC's own behaviour (concurrent writers, a paused lock holder) was never run — or one AC had no test at all and the ticket was approved anyway.

The bar is one falsifiable test per AC that exercises that AC's own behaviour — not more coverage, not a preferred style. Do not raise it.

Steps

1. Build the AC → test map. For each AC write one line: AC <id> → <test file>::<test name> — asserts <observable outcome>. Find tests by grepping the diff's test files for the AC's nouns, endpoints, error codes and function names. An AC with no line is MISSING, whatever the evidence summary claims. An AC that is purely documentation or configuration text is evidenced by the diff itself — write AC <id> → diff: <file> instead. Call search_lore for the repo's testing conventions.
2. Check each mapped test exercises the AC's own behaviour. Read the test body, not its name. It must drive the code path the AC describes and assert the outcome the AC promises. Mark it INDIRECT (no evidence) when:
   • it asserts nothing: no assertion; assertions only inside a callback, loop or branch that never runs; an async assertion not awaited or returned (the test ends first); a try/catch that swallows the assertion error; expect(x).toBeDefined(), assert True, "no exception thrown" as the only check for an AC about a value; a large snapshot generated from the current output with no AC-specific assertion;
   • it asserts only on mocks: the sole checks are toHaveBeenCalled/assert_called on a fake, or the mock returns the value that is then asserted — the real result the AC names is never observed;
   • it mocks the unit under test, or fakes the very component whose behaviour the AC names (the filesystem mocked for an "atomic write" AC, the clock frozen for a "lease expiry" AC without advancing it, the lock stubbed for a locking AC);
   • it never reaches the path the AC names: a concurrency AC tested with one sequential caller; a "cross-process" AC tested with threads or Promise.all in one process against a process-local mutex; a crash/pause/restart AC with no kill, pause or restart; an error-handling AC tested only on the happy path; a retry AC where the dependency never fails;
   • it re-implements the production logic in the expected value, so a shared bug passes both.
3. Apply the mutation test in your head. Pick the line that implements the AC (the lock, the conditional, the status code, the rename) and imagine deleting it or flipping it. Would a mapped test fail? If not, that AC is not evidenced. Also ask: would this test have passed on the base branch? A test that passes without the change proves nothing about the change. Do not edit the code to check — reason it.
4. Check the test actually ran. The recorded test output (or one run of the repo's test command) shows the new test names or a count that includes them; the file matches the runner's test pattern; no .only/fit/-k filter excludes it.
5. Check for weakening. Tests deleted, skipped (it.skip, xit, @pytest.mark.skip, t.Skip, @Disabled), loosened (exact → approximate, an error now swallowed), retried, given longer timeouts to mask a race, or thresholds lowered — each is a finding unless the ticket asked for it with a stated reason.
6. Check determinism of the tests that carry an AC. No dependence on wall-clock time, timezone, locale, order, network or shared mutable fixtures; no fixed sleeps as synchronisation (use barriers, injected hooks or polling with a deadline — the fix-flaky-test skill); seeded randomness. A concurrency test that loops enough times to expose the race, or forces the interleaving deterministically, is fine.
7. Rate each finding. Blocking: an AC that is MISSING or only INDIRECTLY tested; a test that cannot fail; a weakened or skipped test without a stated reason. Should-fix: a missing negative case when the AC names an error (that is the AC's own behaviour); non-determinism that actually made an AC's test fail or flake in the recorded runs. Note: other non-determinism risks, naming, structure, extra coverage beyond the ACs, wrong-level tests that still prove the AC. Only blocking and should-fix justify RECOMMEND CHANGES.
8. Hand the map to review-ticket. Each AC's map line and PASS / MISSING / INDIRECT classification go into that AC's single record_ac_evidence row (its step 8) — do not record a second row per AC. Record a separate manual_note only for a weakening or determinism finding, naming the test and the fix.