Reviewing Tests in the Same Diff as the Code

A test and the code it tests, arriving in one diff, are read as a unit: the test is taken as evidence that the code does what the test asserts. That reading is wrong often enough to be worth a rule. The test was written by the same author, in the same session, against an implementation that already existed or was being written concurrently. Its assertions describe observed behaviour. Whether that behaviour is the intended behaviour is a separate question, and the diff is the only place where the separation is visible, because afterwards the test reads as a specification and nobody can tell which came first.

The assertion was written after the code

The order of work matters and the artifact does not record it. When a test is written first, its assertions are derived from the requirement: the author had to decide what the answer should be before there was any answer. When a test is written after, its assertions are frequently derived from the output: the author runs the code, looks at the result, and encodes it. The second process produces tests that pass and cannot fail for the reason they exist.

The observable symptom in a diff is an assertion whose expected value is oddly specific. A rounding function that should return 2.5 is asserted to return 2.5000000000000004, which is what the implementation happened to produce. A serialiser is asserted to emit keys in a particular order that no requirement specifies. A parser is asserted to report an error at column 47, a number that comes from the fixture’s spacing rather than from a documented rule. Each of these tests will break on a refactor that preserves the contract, and none of them will break on a bug that the contract forbids.

The reviewer’s move is to ask what requirement the expected value encodes. If the answer is “the output of the current implementation”, the test is a snapshot and should either be labelled as one or replaced by an assertion on the property that matters.

Assertions that cannot fail

A weaker class of the same problem is a test whose assertion is true regardless of the code under test. Three shapes recur.

Assertion on a mock’s interaction rather than on the result. The test stubs a collaborator, calls the unit, and asserts that the stub was called. That asserts the unit invokes the collaborator, which the diff already shows, and says nothing about whether the invocation was correct. If the unit passes the wrong argument and the stub accepts anything, the test passes. Interaction assertions are legitimate when the interaction is the contract — sending a message, writing to a log — and are noise otherwise.

Assertion that swallows the exception. A test that calls the code inside a try block and asserts nothing after it passes when the code throws nothing and also when the catch block is empty. The failure mode is a test that reports success on a path it never checked.

Assertion on a derived value computed by the test itself. The expected result is computed by a second copy of the same logic in the test. If both copies share the bug — because the test’s copy was written by copying the implementation — the test agrees with the code and neither is checked. This is the classic oracle problem: the test needs an independent source of truth, and a reimplementation by the same author is not independent.

The reviewer sees all three in a diff, and the check is mechanical: for each assertion, ask which mutation of the implementation would make it fail. If no plausible mutation does, the assertion is decorative.

Coverage claims are not what they appear

A diff that adds a hundred lines of code and forty lines of test invites the inference that the code is covered. The inference is wrong in a way that matters for review, because line coverage counts executed lines and says nothing about whether any assertion distinguishes the outcomes.

A test that calls a function and asserts only that it did not raise covers every line the call touches. A test with a single case in a switch covers one arm. A test that exercises an error path by stubbing the error covers the handler’s entry but not its body if the handler returns immediately. In each case the coverage report is high and the defect-detecting power is low, and the two numbers are not substitutes.

The review-relevant question is not “is this covered” but “which behaviours does this test distinguish”. A useful heuristic: count the branches in the new code and the assertions in the new test. If branches exceed assertions substantially, the test is a smoke test for a function with real logic, and the diff should say so. Coverage also says nothing about the classes of defect that reading a diff cannot reach at all, so a high number is not evidence that the test suite compensates for them.

Test doubles hide the integration

The third structural problem in a combined diff is the substitution of doubles for the real collaborators. A unit test that replaces the database, the clock, and the HTTP client tests the unit’s own logic and nothing about the boundary. That is a reasonable design for a unit test. It becomes a problem when the diff contains only such tests and the change is a boundary change — a new query, a new request format, a new timeout.

The failure mode is specific: the double was configured according to the author’s belief about the collaborator’s behaviour. If the belief is wrong, the double encodes the error and the test confirms it. A mocked client that returns the shape the author expected passes against code that sends a shape the real client rejects. Nothing in the diff shows the mismatch, because both the code and the mock were written from the same belief.

The reviewer’s lever is to ask, for each mocked collaborator, whether the real one has been observed. If the answer is no, the test is evidence about the author’s model rather than about the system, and the change needs an integration test or an explicit statement that the boundary is unverified. This is the reason a diff containing a new mock of a real service deserves scrutiny out of proportion to its line count.

What test changes in a diff actually signal

Reading test modifications rather than additions yields different information, and it is often the most reliable signal in the diff.

Deleted assertions. An assertion removed alongside a code change is a statement that the old behaviour no longer holds. That may be the intent. It may also be a test adjusted until it passed. The reviewer should require the change description to say which behaviours intentionally changed, and then check that the deleted assertions correspond to exactly those behaviours and no others.

Weakened comparisons. An equality assertion replaced by a containment check, or an exact error message replaced by a check that some error occurred. Each relaxation loses discrimination. Sometimes the relaxation is correct — the exact message was never part of the contract — and sometimes it is the minimum edit that makes a failing test green.

Changed fixtures. A fixture edited so that the expected output matches the new output. If the fixture represents a real input, changing it changes what the test covers; if it exists only to produce the expected output, it is a snapshot input and the test is a snapshot test. The distinction is visible in the diff and is routinely skipped.

Disabled tests. A skip or an exclusion added in the same change as the code it would have tested is the clearest possible signal, and it is also the one most often accepted without discussion because the change is small and the reason is stated in a comment. The comment is worth reading; a skip with a stated reason is a known gap, and a skip without one is a deleted test.

The order in which to read the two halves

The practical consequence for review sequence is that the test should be read first when it exists, and read as a specification rather than as a verification. Reading the test first establishes the intended contract before the implementation supplies an alternative one. Reading the implementation first anchors the reader on what the code does, and the test then reads as confirmation.

When the test is added after the implementation, this ordering is not available — there is no specification to read first, only a description of observed behaviour. That is not a reason to reject the test; it is a reason to check it against the requirement rather than against the code, and to mark the assertions that encode incidental behaviour so that a future refactor knows which breaks are expected.

The wider point is that a diff containing both halves invites a comparison the reviewer cannot perform: whether the test would have failed before the change. That question has a definite answer and the answer is not in the diff. Checking it requires running the test against the pre-change tree, which is cheap for a single test and is the only way to distinguish a regression test from a test that was always going to pass. Where that check is impractical, the change description can state which test fails without the fix, and that statement is verifiable by anyone who cares to try.

Tests are also the artifact most affected by the boundary between what a diff shows and what a tree contains, since a test’s value depends on behaviour under inputs the diff does not enumerate.