How to Review the Tests, Not Just the Feature Diff

A pull request lands in the review queue with a title like "Add retry logic to webhook delivery" and forty lines of application code. The diff is tidy. The new helper has a sensible name, the error handling is legible, and the branch coverage number on the CI badge ticks up by a fraction of a point. Most reviewers will read those forty lines twice, leave a comment about variable naming, approve, and move on. The tests that came with the change, maybe sixty lines across two files, get a glance at most.

That glance is where a lot of long-term trouble hides. A test suite is not a receipt for a feature. It is a set of claims about behavior, written in a language that happens to compile, and those claims can be wrong, hollow, or quietly contradictory in ways the passing green checkmark will never reveal. Reviewing the tests is a separate skill from reviewing the code, and it is one most teams never actually practice.

The assertion that proves nothing

The most common problem in a new test is an assertion so weak it would survive almost any regression. A function returns a list; the test asserts the list is not null. A handler writes to a queue; the test asserts the handler did not throw. A parser processes a malformed string; the test asserts the result "is a string." Every one of these passes, and every one of these would keep passing if the function returned an empty list, dropped the message on the floor, or echoed the input back unchanged.

When reading a test, the useful question is what change would make this assertion fail. If the answer is "almost nothing short of a crash," the assertion is decoration. A retry test that only checks the function ran without raising will not catch the bug where retries fire immediately with no backoff, or where the retry count is off by one, or where the last attempt silently swallows the final error. The test is green in all those cases.

Strong tests assert on specific values, specific call counts, specific ordering, specific error types. A reviewer scanning the new test file should be able to point at a line and say: this would break if the code did X. If that sentence cannot be formed for any line, the test is along for the ride.

Mocks that agree with the bug

Mocking is necessary and also a small act of fiction. The test replaces a real dependency with a stand-in that behaves the way the author believes the real thing behaves. When the author's belief is wrong, the mock is wrong, and the test confirms a behavior the production system will not reproduce.

Reviewers should look at the setup block of any test that uses fakes and ask whether the fake matches the real contract. If the code under test calls a payment API and the mock returns a success object with three fields, does the real API return those three fields, in that shape, on that path? If the mock raises a timeout after zero calls, does the real client actually raise or does it return an error value? A mock that returns the wrong shape lets a serializer bug sail through. A mock that never times out hides the code path that handles timeouts.

There is a subtler version of this in the assertion layer. A test can mock a dependency, then assert that the dependency was called with certain arguments, and never verify that the result flows back correctly. The test is checking the request, not the response. Half a contract, green checkmark, bug in production.

The test that tests the framework

Some new tests do not test the feature at all. They test that the language's standard library works, that the HTTP router routes, that the ORM saves a row to an in-memory database. A test that creates a record and then fetches it back proves the ORM and the database driver are functioning. It says nothing about the business rule the pull request was supposed to enforce.

Those tests are not worthless; they can catch configuration mistakes. But when a reviewer sees four new tests and three of them exercise only framework plumbing, the real question is whether the one remaining test actually covers the thing the changelog describes. Often it does not. The feature is "reject duplicate email signups," and the test checks that a signup with a fresh email succeeds. The second signup, the one that should fail, is never written.

The fix in review is straightforward: name the behavior in the PR description, then find the test that proves it. If the test is missing, say so. The author will write it in ten minutes, and the reviewer has just prevented a regression that would otherwise have been discovered by support.

Where the edge cases went

Application code review and test review see different edges. In the code, the reviewer looks at branches and asks whether each is reachable and correct. In the tests, the reviewer looks at cases and asks whether each branch has a case that hits it. The two views should line up. Often they do not.

A diff might add three conditional branches: the happy path, the retry-exhausted path, and the invalid-signature path. The new tests cover the happy path twice and the invalid-signature path once. The retry-exhausted branch, which is the whole reason the PR exists, has no test at all. The code reviewer, reading the code, will note that the branch looks correct. Only the test reviewer notices that nothing exercises it.

Boundary cases deserve the same scrutiny. An off-by-one in a retry count, a pagination limit, a batch size, a timeout threshold: these are exactly the values that break in production and exactly the values the test should pin down. A test that uses a retry count of 3 and asserts the function called the dependency at least once has not tested the boundary. It has tested that 3 is greater than zero. The test should assert exactly 3 calls, or exactly 4 including the initial attempt, depending on the contract, and it should exercise the count at 1 and at 0 if those are legal.

Tests that pass for the wrong reason

A test that passes in CI is not automatically a test that is doing its job. It can pass because of a race, because of shared state from another test, because of a stale fixture, or because of an assertion that happens to be true regardless of the code path. The reviewer's job is to spot the ones that pass for reasons unrelated to the feature.

Flaky-looking patterns are a signal. A test that sleeps for a fixed duration, or retries itself internally, or depends on wall-clock time, or reads from a shared file, is likely to pass on the author's machine and fail intermittently on CI. Worse than failing is the flaky test that passes for months and then reveals, during an incident, that it never actually asserted the invariant. The reviewer should look at any wait, any sleep, any "eventually" helper, and ask what specifically is being awaited and whether a deterministic signal exists.

A second pattern is the test that depends on test ordering. If test B only passes because test A ran first and left the database in a particular state, the suite is fragile. New tests that assume prior state should be flagged. Each test should be able to run in isolation, and the reviewer can verify this cheaply by looking for setup and teardown, by checking whether the test hits a shared fixture, and by asking the author to run it alone.

Test names that describe the code, not the behavior

A test named testRetryHelper or testHandleWebhook is a label, not a specification. It tells the reader which function is under test and nothing about what the test expects. Six months later, when the function has been renamed twice and the behavior has drifted, the name is an obstacle.

Behavioral names read like sentences: "exhausted retries surface the final error to the caller," "invalid signature is rejected before any retry is attempted," "duplicate signup returns a conflict and does not create a second record." These names do work in the review. A reviewer reading the test names alone should be able to reconstruct the intended behavior of the feature and spot the missing case. If the names read like a list of internal functions, the reviewer has to reverse-engineer intent from the assertions, and intent is exactly the thing most likely to be wrong.

Renaming a test in review is a small, cheap request. It forces the author to state what the test proves, and sometimes the act of naming reveals that the test proves less than intended.

Deleting a test is a claim about behavior

Pull requests that remove or modify existing tests deserve more scrutiny than the ones that add them. A deleted test is a deleted assertion, and the reviewer should ask what invariant is no longer protected. Sometimes the deletion is correct: the behavior genuinely changed, the test was for a removed feature. Sometimes the deletion is an accident of refactoring: a function was renamed, the test was updated incorrectly, and the assertion quietly stopped checking anything.

A modified assertion is the same problem in miniature. A test that used to assert an exact error message now asserts that some error occurred. A test that used to assert a call count of 3 now asserts a call count greater than 0. Both changes look small in the diff. Both remove the specific claim the test was making. The reviewer should be able to say why the assertion was loosened, and if the answer is "the test was failing," the next question is whether the code changed or the test was adjusted to match a bug.

What a test-focused review actually looks like

The mechanics are unglamorous. Read the PR description first and write down the behavior in one sentence. Open the test file before the application file, so the tests frame the feature instead of the diff framing the tests. For each new test, find the one line that would fail if the feature were removed, and if there is no such line, leave a comment. For each new branch in the code, find the test that hits it, and if there is no such test, leave a comment.

Run the new tests locally with the application code reverted, if that is cheap. A test that still passes against the old code is not testing the new behavior. This single check catches the hollow assertion and the framework-only test faster than any amount of reading.

The cost is real: a careful test review can double the time spent on a pull request. The return is a suite that actually fails when the feature breaks, which is the only property that matters in the incident channel at 2 a.m. when the green checkmark from six months ago is suddenly, obviously, insufficient.