Nothing catches a subtest that calls a fixture built on its parent test's t #226

New Issue

2026-10-16T01:56:24Z

cgalo5758 commented

2026-10-16 01:56:24 +00:00

What a contributor runs into

A World (internal/testkit/world/world.go:43-48), a dbtest.Q (internal/testkit/dbtest/query.go:39-48) and most fixtures a package writes for itself keep the testing.TB they were built with and fail through it. docs/testing.md states the rule for the test kit: a Q belongs to the test or subtest that built it ("Opening the database"), a subtest builds its own World ("Fixtures"), and a browser helper never stores t (the browser tests' rules under the end-to-end tests). Nothing checks the rule, and a table test breaks it easily: it builds one fixture on the parent's t and calls the fixture's methods inside each t.Run closure.

A rough scan by syntax finds about 30 such closures in 9 test files of internal/server. Two of them:

TestMemberRouting_AsPlanSwitchTimingRoutes (internal/server/grant_resumption_member_test.go:596) builds w := newWikiWorld(t). Each subtest calls w.orgOf (line 614), which fails with w.t.Fatalf (line 104), on the parent's t.
TestCheckoutRefusesATierBelowOrAtAGrantedRung (internal/server/billing_checkout_offer_db_test.go:158) builds w := newCheckoutOfferWorld(t). Each subtest calls w.postReal, whose post fails with w.t.Fatalf("Checkout reached Stripe: …") (line 120).

Review found the same pattern five more times while tests moved onto the kit, and each was fixed by hand.

Why it costs

When a subtest calls Fatal on its parent's t, Go logs the message under the parent, fails the subtest with "subtest may have called FailNow on a parent test" instead of the fixture's message, and stops the parent, so the parent's remaining subtests do not run. A cleanup that a fixture registers through the stored t runs when the parent ends, not when the subtest does. Whoever reads the output looks for the failure in the wrong test, and a table loses every row after the one that failed. Review keeps catching the pattern because nothing else does.

Where

The test files the scan names, all in internal/server; docs/testing.md. The project lint (internal/lint, go run . lint) parses Go without type information today. scripts/sqlc-unused already loads every package and its test variants with types through golang.org/x/tools/go/packages.

Done when

A check in make lint reports a t.Run closure that uses a value declared outside the closure whose type is *testing.T or testing.TB, or holds one in a field, directly or by embedding: a World, a Q, a package's own fixture. It reports a method call on such a value and the value passed to a function. Reading a plain field, such as an id, passes, and so does q.Tx(), the rebinding docs/testing.md prescribes. The check needs type information, so it either sits beside scripts/sqlc-unused or brings typed loading into the project lint.
The closures it finds today are fixed: each subtest builds its own fixture, or the fixture's methods take the subtest's t.
A subtest that calls a method of a fixture built on its parent's t fails the check.
docs/testing.md states the rule once for every value that holds a t, from the kit or not, and names the check.

## What a contributor runs into A World (`internal/testkit/world/world.go:43-48`), a `dbtest.Q` (`internal/testkit/dbtest/query.go:39-48`) and most fixtures a package writes for itself keep the `testing.TB` they were built with and fail through it. `docs/testing.md` states the rule for the test kit: a Q belongs to the test or subtest that built it ("Opening the database"), a subtest builds its own World ("Fixtures"), and a browser helper never stores `t` (the browser tests' rules under the end-to-end tests). Nothing checks the rule, and a table test breaks it easily: it builds one fixture on the parent's `t` and calls the fixture's methods inside each `t.Run` closure. A rough scan by syntax finds about 30 such closures in 9 test files of `internal/server`. Two of them: - `TestMemberRouting_AsPlanSwitchTimingRoutes` (`internal/server/grant_resumption_member_test.go:596`) builds `w := newWikiWorld(t)`. Each subtest calls `w.orgOf` (line 614), which fails with `w.t.Fatalf` (line 104), on the parent's `t`. - `TestCheckoutRefusesATierBelowOrAtAGrantedRung` (`internal/server/billing_checkout_offer_db_test.go:158`) builds `w := newCheckoutOfferWorld(t)`. Each subtest calls `w.postReal`, whose `post` fails with `w.t.Fatalf("Checkout reached Stripe: …")` (line 120). Review found the same pattern five more times while tests moved onto the kit, and each was fixed by hand. ## Why it costs When a subtest calls `Fatal` on its parent's `t`, Go logs the message under the parent, fails the subtest with "subtest may have called FailNow on a parent test" instead of the fixture's message, and stops the parent, so the parent's remaining subtests do not run. A cleanup that a fixture registers through the stored `t` runs when the parent ends, not when the subtest does. Whoever reads the output looks for the failure in the wrong test, and a table loses every row after the one that failed. Review keeps catching the pattern because nothing else does. ## Where The test files the scan names, all in `internal/server`; `docs/testing.md`. The project lint (`internal/lint`, `go run . lint`) parses Go without type information today. `scripts/sqlc-unused` already loads every package and its test variants with types through `golang.org/x/tools/go/packages`. ## Done when - A check in `make lint` reports a `t.Run` closure that uses a value declared outside the closure whose type is `*testing.T` or `testing.TB`, or holds one in a field, directly or by embedding: a World, a Q, a package's own fixture. It reports a method call on such a value and the value passed to a function. Reading a plain field, such as an id, passes, and so does `q.Tx()`, the rebinding `docs/testing.md` prescribes. The check needs type information, so it either sits beside `scripts/sqlc-unused` or brings typed loading into the project lint. - The closures it finds today are fixed: each subtest builds its own fixture, or the fixture's methods take the subtest's `t`. - A subtest that calls a method of a fixture built on its parent's `t` fails the check. - `docs/testing.md` states the rule once for every value that holds a `t`, from the kit or not, and names the check.

cgalo5758 added the

labels 2026-10-16 01:56:24 +00:00

Sign in to join this conversation.