AI Code Review for Tests: Catching Tests That Pass Without Testing Anything

Pull request review has a blind spot the size of a directory. Reviewers read the production change with care, glance at the test file to confirm it exists, see that CI is green, and approve. The test file is the one part of the diff that gets a free pass precisely because it looks like diligence. That is how a codebase ends up with hundreds of tests, high coverage numbers, and regressions that each “had a test”.

This post is a checklist for what an AI code reviewer should verify about the tests in a pull request, written so you can turn it into review instructions for whichever tool you run. It is a companion to our checklists for migrations and infrastructure code, which each take a change type where the default review fails for a predictable reason and give it a fixed set of questions that closes the gap. Where PURA does something specific, we say so; most of it applies to any reviewer.

Why tests get a free pass

Three things conspire. First, test code is longer and more repetitive than production code, so human attention runs out before the file does. Second, a green check mark is a strong signal and reviewers reasonably lean on it; but CI only proves the tests passed, not that they could have failed. Third, test files rarely break anything visibly when they are wrong. A bad migration takes down production; a bad test sits quietly until the day it should have caught something.

An AI reviewer has none of the first problem and should be configured to ignore the second. It reads the test file with the same attention as line one of the production diff, and it can be told, explicitly, that a passing suite is not evidence. The questions below are what to tell it to ask.

The checklist

1. Can this test fail?

The first question for every new test is whether any plausible bug in the production change would make it go red. Tests that cannot fail come in recognisable shapes: a test that calls a function and asserts nothing; a test whose only assertion is that no exception was thrown; a test that asserts a value equals itself after a round trip through the code under test; a test that constructs the expected value using the same helper the production code uses. The reviewer should name the shape and ask the author what failure the test is meant to catch.

2. Is the thing under test real?

Over-mocking is the most common way a test stops testing. The tell is a mock of the module the PR actually changed, or a test that stubs the function being tested and then asserts the stub was called. Mocking a payment gateway or an email provider is sensible; mocking the pricing calculator in a PR that changes the pricing calculator means the test exercises the mock. A reviewer with repository context, which is the difference we described in why diff-only review misses real bugs, can check whether the mocked symbol is the one the diff touched.

3. Do the assertions check behaviour or bookkeeping?

expect(spy).toHaveBeenCalledTimes(1) is a statement about how the code is wired, not what it does. Some of that is unavoidable, but a test made only of call-count assertions will pass through any refactor that keeps the same calls and fail on any refactor that improves them, which is the opposite of what a test is for. The reviewer should flag tests with no assertion on an output, a returned value, a persisted state or an observable side effect.

4. Does the test depend on time, order or the network?

Flakiness has a short list of causes and most are visible in the diff: a real Date.now() or sleep, a hard-coded timeout, a test that reads shared mutable state set by a previous test, an unawaited promise, a live HTTP call, a dependency on locale or timezone. None of these fail on the author’s laptop on the day of the PR; all of them fail at 3 a.m. in CI three weeks later. This is the category where a reviewer that reads every test file earns its keep, because no human does.

5. Were any existing tests deleted, skipped or loosened?

The most dangerous line in a test diff is a removed one. A deleted test, a new .skip or @pytest.mark.skip, an assertion changed from an exact match to a looser one, a tolerance widened, a snapshot regenerated wholesale: each can be legitimate, and each is also exactly what a developer under deadline pressure does to make a red build green. The reviewer should list every such change in the PR and ask for the reason to be stated in the PR, not inferred. We argued in blocking versus advisory review that narrow, high-confidence finding classes are the ones worth gating; “a test was skipped without a linked issue” is a good candidate.

6. Does the test cover the branch the change introduced?

A PR that adds an if and a test that exercises only the else is a coverage number going up and a risk going unchanged. The reviewer should map new conditional branches, new error paths and new early returns in the production diff to the tests that reach them, and say plainly which ones are not reached. This is mechanical work that a model does well and a tired human skips.

7. Is the test readable as a specification?

A test name like test_handles_edge_case_2 and a body of forty setup lines is a maintenance cost dressed as safety. Ask for a name that states the behaviour and an arrange-act-assert shape that a future reader can follow. This is the softest item on the list and the one to keep advisory; it is still worth a comment, because the next person to touch the test will otherwise delete it rather than understand it.

8. Was the test written by the same agent that wrote the code?

With coding agents producing a growing share of PRs, tests and production code are increasingly written in the same breath by the same model, which means the test encodes the same misunderstanding as the code. A test that confirms the implementation does what the implementation does is tautological. For agent-authored PRs the reviewer should check the tests against the ticket or the stated intent, not against the diff. Our post on reviewing PRs whose author is not human covers the wider pattern.

Turning the checklist into review rules

The checklist is only useful if it runs on every PR without anyone remembering it. In PURA, that is what custom review instructions are for: project-level rules the reviewer applies alongside its own analysis. A compact version of the eight questions above fits in a few lines, scoped to test paths:

For files under tests/, __tests__/, or matching *.test.* / *_test.*:
- For each new test, state what production bug would make it fail.
  Flag tests with no assertions, assert-no-throw only, or round-trip
  assertions that use the code under test to build the expected value.
- Flag mocks or stubs of any symbol modified in this PR.
- Flag tests whose only assertions are call counts or call arguments.
- Flag real time, sleeps, network calls, shared mutable state, or
  unawaited promises.
- List every deleted, skipped, or loosened test and require a reason.
- Map new branches and error paths in the production diff to the tests
  that reach them; list the unreached ones.
Treat a green CI run as no evidence for any of the above.

Scoping by path matters: these rules are noise on a documentation PR and signal on a test PR. Because PURA reviews run on your own API keys with budgets you set per repository, you can afford to make the test rules thorough on the repositories where a missed regression is expensive and lighter elsewhere.

What this will and will not catch

A reviewer that applies the checklist will reliably catch the structural failures: the test with no assertion, the mocked unit under test, the quietly skipped case, the branch nobody reached. Those are the majority of “had a test” regressions, and they are findable from the diff plus the repository.

It will not tell you a test is flaky from history, because it does not run the suite and does not see CI logs; that remains the job of your CI’s retry statistics. It will not know your coverage targets unless you state them. And it will sometimes over-flag mocking that is perfectly reasonable, such as a stubbed clock in a scheduler test, which is why rule two should be worded as “flag and ask” rather than “block”. We wrote about keeping that noise tolerable in handling AI review false positives.

What it changes is the baseline. Every test file in every PR gets read, in full, with the same eight questions, before a human decides whether the green check mark means anything. For the one part of the diff that reviewers trust most and read least, that is a large improvement for very little configuration.