skip to content

Reviewing a PR that regenerates a 400-line golden file under testdata, what do you check before approving?

level: seniorimportance: nice to knowfreq 26%

answer

  1. the flag already accepted the output
  2. the diff is now the assertion
  3. deletions before additions
  4. run it once more without the flag
  5. an unparseable golden should never be writable

basics

~20 s

Read the diff, because a run with -update writes the file it then compares and cannot fail. Check that every hunk, especially deletions, is explained by the change, and that a plain go test passes on the committed bytes.

solid answer

~50 s

Start from the fact that `-update` moved the assertion: the test wrote the bytes it compared against, so a green run under the flag proves nothing and the diff is now the assertion. So I read the diff as the test result. Deletions matter more than additions — 380 lines removed and 5 added usually means the generator bailed early on an error the test swallowed, not that the output got tidier. I check that every hunk maps to something in the change description, that the file still ends with a trailing newline, and that a plain `go test ./codegen` — no flag — is green on the committed bytes. I want the regeneration done on a clean tree, and the generated source run through `go/format`'s `Source` before it is written, so whitespace churn cannot hide a semantic change and unparseable output fails outright.

code

go · 9 lines
go
formatted, err := format.Source(got)
if err != nil {
	t.Fatalf("generator emitted source that does not parse: %v", err)
}
if *update {
	if err := os.WriteFile(golden, formatted, 0o644); err != nil {
		t.Fatal(err)
	}
}

go deeper

for a junior

Know that a golden file is regenerated by rerunning the test with a flag, and that the regenerated file must be read and committed like any other change rather than accepted blindly.

for a middle

Explain why a run with the update flag always passes: the test wrote the bytes it compares against. Then say what you would rerun without the flag to get a real signal.

for a senior

Show a concrete checklist on a real diff: deletions first, hunks matched to the described change, trailing newline intact, clean tree at regeneration time, and formatting normalised at the source.

for a principal

Set the policy that makes this reviewable at all — one fixture per case, regeneration in its own commit, CI never running the update flag, and properties asserted in code where bytes are too coarse.

## Why this review is different from other reviews In an ordinary test, the checked-in expectation was written by a person who thought about it. In a golden test regenerated with `-update`, the expectation was written by the very code under test. Everything the byte comparison would have caught has already been accepted, automatically, by the act of regenerating. The remaining check is a human reading a diff — so the review *is* the test, and treating a 400-line regenerated fixture as boilerplate to scroll past is skipping the assertion. ## What to actually check **1. Was a plain run done afterwards?** `go test ./codegen` with no flag, on the committed file. This catches the case where the golden was regenerated from a dirty tree or a local experiment and does not match what the merged code produces. It is a one-line CI guarantee: CI never runs with `-update`, so a red build here means the fixture and the code disagree. **2. Do the hunks match the story?** The pull request says "emit a json struct tag for every exported field". Then every hunk should be a struct tag. A hunk that renames a type, drops a method, or reorders fields is either an unmentioned behaviour change or nondeterminism in the generator, and both need an answer before approval. **3. Look at deletions first.** Additions are usually the intended change; large deletions are usually the accident. The failure mode this leaf exists to warn about is a **truncated** golden: the generator hit an error path, returned what it had built so far, the test ignored the error or never saw one, and `-update` cheerfully wrote 20 lines over a 400-line file. In the diff it looks like a clean simplification. Ask what removed it. **4. Check the boundaries of the file.** A missing trailing newline, added CRLF line endings, or a lost final blank line are all real byte differences that a reader's eye slides over. They will churn the next diff and, in generated Go, they indicate the writer path changed. **5. Is the noise removed at the source?** Generated Go source should be run through `go/format`'s `Source` function before it is written or compared. Two benefits: diffs then show semantic change rather than indentation drift, and `Source` returns an error on input that does not parse — so a truncated or malformed generation fails the test loudly instead of being written to the fixture. That is an assertion `-update` genuinely cannot satisfy, which makes it worth more than the byte comparison itself. ```go formatted, err := format.Source(got) if err != nil { t.Fatalf("generator emitted source that does not parse: %v", err) } ``` **6. Was the tree clean when it was regenerated?** If the author ran `-update` with other work in progress, unrelated fixtures may have been rewritten in the same commit. `git status` before regenerating, and a fixture-only commit separate from the code change, make the diff readable. Some teams go further and commit the regenerated goldens as their own commit so a reviewer can look at the code change alone first. **7. Are there orphans?** A golden file that no test reads any longer is invisible: nothing fails, nothing warns. Deleting a test case should delete its fixture. If a directory of fixtures has grown past what anyone can enumerate, a small test that walks `testdata` and fails on files no case claimed is worth the twenty lines. ## The structural fix when review keeps being impossible If every change turns one enormous fixture red, the fixture is too coarse. One golden per case, named after the case, means the diff for a change to order handling touches `order.go.golden` only. Splitting a monolithic golden is usually cheap — the generator already knows which case it is producing — and it converts an unreviewable 400-line diff into three readable ones. And where a property matters more than the exact bytes (the output parses, every exported field appears, the header banner is present), assert that property directly in Go rather than trusting a reader to notice its absence in a wall of text. ## What to say in the review Be concrete: "lines 210-240 are removed and the description does not mention dropping the Validate method — is the generator erroring out on this schema?" is a review comment. "Regenerated, LGTM" is the failure this whole section is about.

  • Why do deletions in a regenerated golden's diff deserve more suspicion than additions?
    Additions usually correspond to the change the author set out to make. A large deletion more often means the generator stopped early — an error path returned partial output, and the update flag wrote that truncated result over the fixture without complaint. In the diff it reads as tidying up. Ask what removed those lines and expect a specific answer.
  • What check can you add that an -update rerun cannot make pass?
    Anything asserting a property of the output rather than its exact bytes. Running generated Go source through `go/format`'s `Source` is the best value: it fails on source that does not parse, so truncated or malformed generation errors out instead of being written to the fixture. Requiring the generated-code header banner, or that every schema field appears, works the same way.
  • The suite has one 400-line golden and every change turns it red. What do you do about it?
    Split it: one fixture per case, named after the case, so a change to one behaviour produces one small diff. The generator already knows which case it is emitting, so the split is cheap. Then move the invariants that matter — output parses, banner present, no duplicate declarations — into explicit assertions instead of relying on someone spotting their absence.

Approving a regenerated golden without reading it is countersigning a document you asked the other party to fill in.

saying these in an interview costs you the question

  • Approves because CI is green under the update flag
  • Skims additions and never reads deletions
  • Regenerates fixtures from a dirty working tree
  • Treats a fixture diff as generated noise not worth reading
  • Leaves goldens no test reads any more in place