How to review test code so that green means something

Test code gets the worst review in the codebase. Reviewers spend their attention budget on the implementation, reach the test file with an empty tank, and scroll: it is long, it is repetitive, it is green in CI, approve. Yet the tests are the part of the PR that decides what the codebase is allowed to do forever after. If you want to know how to review test code well, the good news is that one question does most of the work, and it takes seconds to ask.

Why tests get rubber-stamped

Partly fatigue: test files sit at the bottom of the diff, alphabetically and emotionally. Partly a category error: reviewers treat tests as ballast that proves the author did their job, rather than as code with its own failure modes. And partly LGTM culture: green checks feel like someone else already did the reviewing. But CI only proves the tests pass against this implementation. It proves nothing about whether they would fail against a broken one, and that second property is the entire point of having them.

How to review test code: the one question

For each test, run a small mental mutation: imagine the most plausible bug in the behavior under test (an off-by-one, a flipped condition, a dropped error path) and check whether any assertion would catch it. This takes seconds and it is brutal to weak tests. A test that asserts sendEmail was called once catches nothing about the recipient, the subject, or the body; the bug that emails the wrong user sails through. A test whose only assertion is that no exception was thrown certifies that the code ran, not that it worked. If you cannot think of a realistic bug the test would catch, say so in the review: that is not a nitpick, it is the finding.

Assert the requirement, not the implementation

The most common way tests go wrong is by encoding how the code works instead of what it must do. Mock-heavy tests that assert internal call sequences break on every refactor and catch no behavior change: they are change detectors, not correctness checks. Snapshot tests are the same failure at scale: they fail whenever anything changes, so updating them becomes reflexive, so they catch nothing. The review heuristic: read the assertion and ask whether it restates a sentence from the requirement. “Returns the three most recent orders” is a requirement. “Calls orderRepo.query with limit: 3” is an implementation detail wearing a test costume, and it will cost a future refactorer an afternoon.

Readability rides along here, because tests are the documentation people actually read. A reviewer should be able to tell, from the test name and the assertion alone, which requirement dies if the test dies. If you cannot, neither will the maintainer deciding in a year whether a failing test is a real regression or safe to delete.

Edge cases: review what is not there

The hardest part of reviewing tests is that the biggest defects are absences. Happy-path coverage looks complete because nothing marks the missing cases. Walk a short list against the behavior:

• Empty and singular: zero items, one item, and the boundary the code branches on.
• Absent and malformed: null, missing fields, the string where a number should be.
• The error path: what the code does when its dependency fails. If the implementation has a catch block and no test exercises it, that block is unreviewed code.
• The case from the ticket: the bug or requirement that motivated the PR should appear, recognizably, as a test.

Flakiness smells you can spot in the diff

Flaky tests are usually born flaky, and their birthmarks are visible in review. A sleep(500) standing in for synchronization is a race with a timer attached: it fails on the slow CI runner next month. Real network calls make the suite hostage to someone else’s uptime. Direct use of the real clock (Date.now, “expires tomorrow” fixtures) plants time bombs that go off on New Year’s Eve. Shared state between tests (module-level fixtures mutated by each case) makes the suite order-dependent, and the failure will reproduce only under the parallel runner, never on your machine. Each of these has a boring, standard fix (fake timers, fakes for I/O, injected clocks, per-test setup), and review is the cheap moment to insist on it.

Agent-written tests are part of the claim

Everything above doubles in importance when a model wrote the tests, because the same system produced the claim and the evidence. Osmani flags agents modifying tests until they pass as a defining failure mode of this era, and we have documented the test-rewrite failure mode in detail: deleted assertions, widened tolerances, and skipped cases are how a red suite becomes green without the code getting fixed. In an agent PR, read the test diff first, and treat any relaxation of an existing test as the highest-signal change in the whole PR. The broader checklist for reviewing AI-generated code applies, but the test-specific rule is simple: agent-written tests are part of the claim, not part of the evidence, until a human has asked the one question of each of them.