Skip to content

build: make a failing golden readable and approvable - #99

Open
MPV wants to merge 2 commits into
masterfrom
claude/approval-tests-1-6-0-results-myr2vh
Open

build: make a failing golden readable and approvable#99
MPV wants to merge 2 commits into
masterfrom
claude/approval-tests-1-6-0-results-myr2vh

Conversation

@MPV

@MPV MPV commented Aug 10, 2026

Copy link
Copy Markdown
Owner

Follow-up to the go-approval-tests v1.14.0 bump (#24). Two Makefile targets, no test or production code touched.

1. A failing golden says nothing (build: print the approval diff on a failed golden)

v1.13.0 removed the reporter that printed the diff, so since #24 landed, make test — the command CONTRIBUTING.md and AGENTS.md tell every contributor to run before a PR — reports a golden failure like this:

Finding JetBrains IDE...
    kir_test.go:49: Failed Approval: received does not match approved.

No file names, no contents. v1.14.0 added APPROVAL_TESTS_USE_REPORTER; pointing it at the Systemout reporter restores a usable failure. Same broken golden, after:

approval files did not match
approved: .../kir_test.TestKind.Pod.stdout.approved.txt
received: .../kir_test.TestKind.Pod.stdout.received.txt
Received content:
nginx
gcr.io/google-containers/sidecar
busybox:1.28

Approved content:
wrong:image

CI is deliberately untouched: it runs go test -race ./... directly, and GitHub Actions sets CI=true, which already selects an equivalent reporter. (ContinuousIntegrationReporter is not the name to use outside CI — it no-ops unless it detects a CI environment and lets auto-detection take over.)

2. Nothing documented how to approve one (build: add make approve)

There were no instructions anywhere for updating a golden. That is how the newline change got approved: seventeen received files moved one at a time.

go-approval-tests ships approve_all.py for this, but it is the wrong thing to point contributors at — it is fetched from raw.githubusercontent.com by a fire-and-forget goroutine racing process exit, so whether it exists depends on how long the run took. From a wiped .approval_tests_temp:

command duration result
go test ./approvals/ ~0.02s logs only, no scripts
go test -race ./approvals/ ~1s scripts present

Its download path also fails silently by design, so offline it never arrives. A make target is deterministic, needs no network or Python, and sits beside the make test that produced the files.

Wholesale renaming is safe because a passing test deletes its own received file (approvals.go: _ = os.Remove(receivedFile)), so what remains after a full run is exactly the failures. That does not hold after a filtered go test -run ..., which is called out in both the Makefile comment and CONTRIBUTING.md.

Verification

  • make approve is a no-op when nothing failed (exit 0, no output).
  • With kir_test.TestKind.Pod.stdout and kir_test.TestList.exitcode deliberately broken, make test && make approve restores both and the suite passes.
  • make test, go vet ./..., gofmt -l . (empty), go test -race ./... all pass.

Both commits are build:, so neither cuts a release.


Generated by Claude Code

claude added 2 commits August 10, 2026 06:27
go-approval-tests v1.13.0 removed the reporter that printed the diff, so since
the v1.14.0 bump a failing golden locally reports nothing actionable — no file
names, no contents:

    Finding JetBrains IDE...
        kir_test.go:49: Failed Approval: received does not match approved.

v1.14.0 added APPROVAL_TESTS_USE_REPORTER; pointing it at the Systemout reporter
in `make test` restores a usable failure. Same broken golden, after:

    approval files did not match
    approved: .../kir_test.TestKind.Pod.stdout.approved.txt
    received: .../kir_test.TestKind.Pod.stdout.received.txt
    Received content:
    nginx
    gcr.io/google-containers/sidecar
    busybox:1.28

    Approved content:
    wrong:image

CI is deliberately left alone: it runs `go test -race ./...` directly, and
GitHub Actions sets CI=true, which already selects an equivalent reporter — the
diff is in the failing run on #24. Note ContinuousIntegrationReporter is not the
name to use outside CI: it no-ops unless it detects a CI environment and lets
the auto-detection chain take over.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MBt5arMEZnxpjgh3GREnri
Nothing in CONTRIBUTING.md or AGENTS.md said how to update a golden, so the only
way to find out was to read the library. That is how the newline change got
approved: seventeen received files moved one at a time.

go-approval-tests ships approve_all.py for this, but it is the wrong thing to
point contributors at. It is fetched from raw.githubusercontent.com by a
fire-and-forget goroutine that races process exit, so whether it exists depends
on how long the run took. From a wiped .approval_tests_temp:

    go test ./approvals/         (~0.02s)  logs only, no scripts
    go test -race ./approvals/   (~1s)     scripts present

Its download path also fails silently by design, so offline it never arrives at
all. A make target is deterministic, needs no network or Python, and sits next to
the `make test` that produced the files.

The rename is safe to do wholesale because a passing test deletes its own
received file (approvals.go: `_ = os.Remove(receivedFile)`), so what is left over
after a full run is exactly the failures. That does not hold after a filtered
`go test -run ...`, which is called out in the Makefile and CONTRIBUTING.md.

Verified: no-op with no failures, and with kir_test.TestKind.Pod.stdout and
kir_test.TestList.exitcode deliberately broken, `make test && make approve`
restores both and the suite passes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MBt5arMEZnxpjgh3GREnri
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants