Code Review for Tests
Bad test code is more expensive than bad production code. It gives false confidence while hiding real bugs. Learn how to review tests like a senior engineer.
1 The Hook — Why This Matters
A mid-level developer at a Christchurch SaaS company opened a pull request with 90% line coverage. The team approved it quickly. Three weeks later, a production bug allowed users to access admin functions. The test that should have caught it had one assertion: assertTrue(true). It ran in CI, it reported coverage, and it was completely worthless. No one had reviewed the assertion quality because "it's just test code."
Tests are production code. They require the same review rigour, the same standards, and the same accountability. A senior automation engineer's job is to enforce that standard.
2 The Rule — The One-Sentence Version
Test code must pass the same quality bar as production code: naming, structure, coverage, and review. "It's just a test" is never an excuse.
Flaky tests, vague assertions, and missing teardowns cost more than production bugs because they train teams to ignore red builds. When CI is red 20% of the time for non-bug reasons, developers stop trusting it.
3 The Analogy — Think Of It Like...
A security guard who sleeps on the job but always signs the patrol log.
The log says everything is fine. The building is empty. A test with high coverage but weak assertions is the same: it produces green metrics while real vulnerabilities walk right past. Reviewing tests is checking whether the guard is actually awake, not just whether the log is complete.
Senior engineer insight
The most dangerous test code I ever reviewed had 87% branch coverage and passed every linting rule — and it tested nothing. Every assertion was assertNotNull(response): the test proved the method returned something, not that it returned the right thing. After that I stopped treating coverage numbers as a quality signal and started asking: "If a developer intentionally introduced the most obvious bug in this function, would this test catch it?" That single question has caught more real defects than any coverage gate I've ever configured.
The most common mistake: approving test PRs based on coverage percentages alone, without reading a single assertion to check it actually verifies the expected business behaviour.
From the field
A Wellington government agency was migrating a benefits calculation system and brought in an automation team to build a regression suite before go-live. The team delivered 400 tests in six weeks, CI was green, and the migration was signed off. Two weeks after go-live, Revenue NZ-integrated income assessments started returning incorrect weekly entitlements for clients who had changed their income mid-year — a calculation path the tests never touched because the team had assumed the existing unit tests covered it. They didn't. The lesson: during code review, always map test scope explicitly to business rules, not to code paths. "We have unit tests for that" is not evidence unless you can trace which unit test covers which rule.
4 Watch Me Do It — Step by Step
Here is a comprehensive test code review checklist used by top engineering teams.
- Structure and readability
# Good: describes behaviour and outcome def test_order_total_includes_shipping_when_order_exceeds_threshold(): ... # Bad: describes implementation def test_calc_total(): ...Heuristic Good Sign Bad Sign Single Responsibility One concept per test Multiple unrelated assertions Deterministic Same input -> same result Random data, time-dependent logic Isolated No test order dependency Shared state, globals Explicit Waits waitForSelector()Thread.sleep(5000)Minimal Logic Straightforward setup/assert Loops, conditionals in test body - Coverage targets by type
Coverage Type Target Caution Statement 70-85% Easy to game Branch 70-80% Better proxy for real coverage Function 90%+ Low bar; all functions should be tested Mutation 70-80% Gold standard but slow; run nightly - Automated quality gates
# GitHub Actions quality gate - name: Lint Test Code run: flake8 tests/ --max-complexity=10 - name: Coverage Gate run: | python -c " import xml.etree.ElementTree as ET root = ET.parse('coverage.xml').getroot() line_rate = float(root.get('line-rate')) branch_rate = float(root.get('branch-rate')) assert line_rate >= 0.80 assert branch_rate >= 0.70 "
5 When to Use It / When NOT to Use It
✅ Rigorous review when...
- Tests guard critical business paths
- Team has >3 automation engineers
- Flaky tests are a known problem
- Coverage targets are mandated
❌ Light review OK when...
- Proof-of-concept or spike tests
- Single engineer on a greenfield project
- Tests will be rewritten before production
6 Common Mistakes — Don't Do This
🚫 Coverage obsession
I used to think: 100% coverage means perfect testing.
Actually: Coverage measures which lines were executed, not whether they were verified. A test that calls a method without asserting the result increases coverage but catches zero bugs. Target meaningful coverage with specific assertions.
🚫 "It's just test code" mentality
I used to think: Tests don't need the same standards as production code.
Actually: Flaky, poorly-written tests cost more than production bugs. They break CI, waste debugging time, and train teams to ignore red builds. Apply the same linting, formatting, and review standards to tests as production code.
🚫 Giant setup methods
I used to think: Putting all setup in @BeforeAll is efficient.
Actually: 200-line setup methods are unmaintainable. Use builders, factories, and Testcontainers to create minimal setup per test. Each test should be readable top-to-bottom without jumping to setup code.
Why teams fail here
- Treating test review as optional admin: reviewers skim test PRs and approve based on CI green + coverage number, never reading assertion quality — so weak tests ship with the same weight as strong ones.
- No shared review checklist: every reviewer uses different mental criteria, so the same anti-pattern (e.g.
Thread.sleep(), catch-all assertions, missing teardowns) gets caught by one reviewer and missed by another. - Coverage gates without branch coverage: teams set an 80% line-coverage gate that's trivially satisfied by calling methods without asserting return values — giving false confidence in a suite that can't catch real regressions.
- Ignoring test maintainability debt: test code grows without refactoring — duplicated setup blocks, magic strings hardcoded across 200 tests, no page object model — until a single UI change breaks 150 tests and the team spends a sprint on test maintenance instead of feature work.
Key takeaway
A test that cannot fail is not a test — it is a liability wearing a green badge, and approving it is the same as deleting the coverage it claims to provide.
7 Now You Try — Interview Warm-Up
Scenario: A developer submits a PR with 95% line coverage. You review the tests and find one assertion per test, all checking happy paths. No error cases, no boundary values, no null checks. The developer says "The coverage is high; what's the problem?"
How do you respond?
Your response:
"Coverage tells us which lines ran, not whether they were verified. Your tests call the methods but don't assert edge cases, errors, or boundaries. A bug in the error handling path would execute the line but the test wouldn't catch it. Please add: 1) at least one negative test per method, 2) boundary value cases, 3) assertions that would fail if the specific bug existed. Target branch coverage >= 70%, not just line coverage."
8 Self-Check — Can You Actually Do This?
Click each question to reveal the answer. If you got all three, you're ready to practice.
Q1. What is the difference between statement coverage and branch coverage?
Statement coverage measures which lines were executed. Branch coverage measures whether both true and false paths of each condition were taken. Branch coverage is a better proxy for real test quality.
Q2. Why is Thread.sleep() an anti-pattern in tests?
It makes tests slow and flaky. If the sleep is too short, the test fails intermittently. If too long, tests waste time. Use explicit/smart waits with timeout policies instead.
Q3. What automated checks should gate a test PR?
Linting, formatting, coverage gates (line and branch), static analysis, and execution of the new tests. Manual review should assess assertion quality, test independence, selector strategy, and business logic alignment.