Should I test that?

Should code review come before testing?

Answer

No, automated tests come before code review: write them in the same pull request, let CI pass before a reviewer starts, have the reviewer read the tests with the code, and run any manual check last, on the approved commit.

Verdict on the code under testYes

Why

Automated tests come before code review and manual testing comes after it; for the typical case, a pull request to a web application that customers use, rule R11 gives Test. Blast radius is users, and Change frequency is regularly, about once a month. Detectability is eventually, because a reviewer reads code without running it, and a wrong comparison that returns a plausible value passes the read. Reversibility is with-effort, since saved records need a repair script, and Test cost is moderate, about an hour per behaviour. A green build before review frees the reviewer to check what no test can: whether the tests assert the right results.

When the decision changes
WhenDecisionWhy
The pull request changes how refunds or invoice totals are calculatedTest mandatory: tests for each amount and boundary pass in CI before review, and the reviewer reads them firstBlast radius rises to money and Reversibility to costly, because wrong charges end in refunds
The pull request changes which account may read which recordsTest mandatory: a test that denies another account's request passes in CI before review, and the reviewer checks that the test fails without the access checkBlast radius rises to safety-or-legal and Detectability to never, because a leaked record raises no error
The pull request changes button text, and CI deploys a preview that the author and the reviewer openDo not write a test for the text change; the reviewer reads the page in the preview before approvingDetectability moves to immediately and Reversibility to trivial, because the text is on the preview screen and stores nothing
The pull request holds a data migration that runs once on productionTest the migration differently: after review, run it on a copy of production data and check the result with a queryChange frequency falls to once, so a test in the suite would never run again
The pull request changes a staff-only admin tool with no customer dataTest minimally: one test of the main path passes in CI before reviewBlast radius falls to internal, because only colleagues see a failure

What breaks if you don't test

A pull request that merges on review alone lets a mistake that reads correctly reach customers: < where the rule says <=, or a SQL filter that drops rows with a NULL status. Nothing crashes, and users report the wrong result weeks later. The wrong order breaks things too: when a manual tester signs off before review, review comments change the code afterwards, and the commit that ships is one nobody tested.

What you lose if you over-test

A full manual pass before each review round repeats with every round of comments, and only the last pass covers the commit that ships. A reviewer who checks out every pull request and clicks through it repeats what CI already ran. Tests that assert every CSS class of the change give the reviewer more lines to read and fail on the next harmless edit.

How to test

Keep the tests in the same pull request as the code, and order the steps so each one checks the commit that ships:

  1. Open a draft pull request; GitHub cannot merge it and requests no code owner review until you mark it ready.
  2. Mark it ready once CI is green. The reviewer reads the tests first and asks whether each one fails when the code is broken, as Google's review guide recommends.
  3. Require status checks and dismiss stale approvals, so a commit pushed after approval needs a new green build and a new approval.
  4. Run any manual check last, on a preview of the approved commit.

When the answer changes

  • The change charges money or decides who may read which records.
  • The change is a data migration that runs once.
  • The change shows only on screen, such as text in a preview deploy.

Real incident + Code example

The filter that passed QA on the previous commit

On a recruiting web app I worked on, a QA engineer tested a new candidate search filter on staging and signed it off on a Monday. In review on Tuesday, a colleague asked to move the filter into the SQL query, and the author rewrote it as WHERE status <> 'archived'. In PostgreSQL a comparison with NULL yields unknown, so the query dropped the 1,900 candidates imported from the old system without a status. Nobody repeated the manual pass, because the sign-off was already on the ticket. Recruiters searched an incomplete list for two weeks, until one asked why a candidate she had just called did not show up. This test now runs on every push and fails on Tuesday's query:

test("search keeps candidates imported without a status", async () => {
  await db.candidate.createMany({
    data: [
      { name: "Ana", status: "active" },
      { name: "Ben", status: null }, // imported from the old system
      { name: "Cai", status: "archived" },
    ],
  });
  const names = (await searchCandidates("")).map((c) => c.name);
  expect(names.sort()).toEqual(["Ana", "Ben"]);
});

Related questions

FAQ

What should come first: testing or code review?

Automated tests come first: CI runs them on the pull request, and code review starts after a green build. Manual testing comes after review, on the approved commit, because a manual sign-off covers only the commit it tested.

Should code reviewers test as part of the review?

No, code reviewers should not repeat the test run that CI made; they read the tests and check that each one would fail if the code were broken. A reviewer tries a change by hand only when its effect shows on screen, such as a layout change, in a preview deploy or a demo from the author.

Should QA test before or after code review?

QA should test after code review, on the commit that the reviewer approved. A manual pass before review must be repeated whenever review comments change the code.