Golang Test Review

/golang-test-review

A focused Go-test-engineering review. Run it over the changed *_test.go files (and any test harness / fixtures / test tooling) after writing tests and before merge. It does not replace code-reviewer — it adds the test-specific lens that general review misses.

How to run

1. Identify the changed test surface: git diff --stat <base>...HEAD -- '*_test.go' 'go/test/**' 'go/cmd/testlatency/**'.
2. Read each changed test file and score it against the checklist below.
3. Report findings by severity (CRITICAL / HIGH / MEDIUM / LOW), each with file:line, the rule violated, and a concrete fix. End with a verdict: APPROVE (no CRITICAL/HIGH) / CHANGES-REQUESTED.

Checklist

1. Behavior over surface (the highest-signal rule)

• Does the test pin the invariant under change, or just re-walk lines for coverage? A test that would still pass if the behavior broke is a no-op — flag it CRITICAL. (AGENTS.md Rule 9.)
• Are assertions specific (exact value / error) rather than "not nil" / "len > 0" where an exact check is possible?
• Negative + edge cases present, not only the happy path?

2. Determinism (non-negotiable)

• No dependence on ambient state: live repo, $HOME, real clock, network, .evolve/runs/. Tests build isolated state (t.TempDir() + git init, or the shared fixtures.NewWorkspace).
• No real time.Sleep for synchronization where an injected clock / channel / poll-with-deadline would be deterministic. Real sleeps belong only in build-tagged integration/e2e tiers.
• No ordering dependence between tests; no shared mutable package globals.

3. Parallel-safety

• t.Parallel() is present where safe — BUT never combined with t.Setenv (panics) or shared-mutable-state tests.
• Subtests that capture a loop variable are Go-1.22-safe or copy the variable.
• -race clean (the suite runs with -race).

4. Build-tag / cost-axis correctness (this repo's two-axis model)

• Slow tests (real subprocess: git/gh/tmux/bash; real timing) carry //go:build integration or //go:build e2e and are NOT in the fast default.
• Self-containment: an untagged (fast) test must not reference a symbol defined only in a tagged file — the default build would break. Shared helpers the fast suite needs live in an untagged file.
• Tag sits at the top, followed by a blank line, before package.

5. Harness reuse (no re-duplication)

• Uses go/test/fixtures (Workspace builder, Fake{Storage,Ledger,Runner,Bridge}, FixedClock, assertion facade) instead of a new local temp-dir builder, fake, clock, or must* helper. Reintroducing a local clone of a harness primitive is a HIGH finding.
• fixtures.FilePresent (pure bool) for skip preconditions — NOT acsassert.FileExists (which logs an Errorf and shouldn't gate a skip).

6. Naming & structure

• Behavior-named (TestShipGate_BlocksWhenRedCountNonZero), never cycle-pegged (TestC102_*).
• AAA (Arrange / Act / Assert) visually distinct; table-driven where it reduces repetition; t.Run subtests named for the case.
• t.Helper() on every helper so failures point at the caller.

7. Debuggability of failures

• Failure messages state got-vs-want and the value under test (got %q, want %q), not bare t.Fatal("failed").
• Skips explain why and what's missing (t.Skip("git not on PATH")).

8. Hygiene

• t.Cleanup / defer restore any mutated state; no leaked goroutines, temp files (prefer t.TempDir), or open handles.
• No time.Now()/rand without a seam where the result is asserted.

Severity guide

• CRITICAL: tautological/no-op test, non-deterministic test, -race violation, build-tag self-containment break.
• HIGH: missing negative/edge coverage on changed behavior, re-duplicated harness primitive, t.Setenv+t.Parallel combo, real sleep in fast tier.
• MEDIUM: weak assertions, missing t.Helper(), poor failure messages.
• LOW: naming, AAA structure, comment nits.

Source: mickeyyaya/evolve-loop — distributed by TomeVault.