From 895c198c61f64f2644d1f76319863284b8510952 Mon Sep 17 00:00:00 2001 From: JJ Eaton <58495321+jayleaton@users.noreply.github.com> Date: Thu, 22 Oct 2026 18:25:04 +0700 Subject: [PATCH 01/10] perf(devloop): make bench-devloop prints build, generate, lint, probe and per-package test timings Adds scripts/bench-devloop.sh (cold + warm), the Bench dev loop workflow (manual, or push a bench/* branch) and scripts/testbudget, which renders go test -json as plain go test output, lists the slowest tests and enforces per-package and per-test time budgets. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/bench-devloop.yml | 51 ++++++ Makefile | 10 ++ scripts/bench-devloop.sh | 233 +++++++++++++++++++++++++ scripts/testbudget/main.go | 261 ++++++++++++++++++++++++++++ 4 files changed, 555 insertions(+) create mode 100644 .github/workflows/bench-devloop.yml create mode 100755 scripts/bench-devloop.sh create mode 100644 scripts/testbudget/main.go diff --git a/.github/workflows/bench-devloop.yml b/.github/workflows/bench-devloop.yml new file mode 100644 index 0000000..3b43ef5 --- /dev/null +++ b/.github/workflows/bench-devloop.yml @@ -0,0 +1,51 @@ +# Dev-loop timings on a CI-shaped runner (`make bench-devloop` with cold and +# warm caches): build, generate, lint per linter, probes, and tests per +# package with and without -race. The report is the run's summary page. +# Run it by hand (Actions > Bench dev loop), or push any ref to a bench/* +# branch to measure it, e.g. the base of a perf PR and its head. +name: Bench dev loop + +on: + workflow_dispatch: + push: + branches: ['bench/**'] + +permissions: + contents: read + +concurrency: + group: bench-${{ github.ref }} + cancel-in-progress: true + +jobs: + bench: + runs-on: ubuntu-latest + timeout-minutes: 40 + env: + TEST_DATABASE_URL: postgres://postgres:postgres@127.0.0.1:5432/postgres?sslmode=disable + steps: + # Same Postgres as CI's db shard. + - name: Start Postgres + run: | + docker run -d --name pg -p 5432:5432 -e POSTGRES_PASSWORD=postgres \ + --tmpfs /var/lib/postgresql postgres:18-alpine \ + -c fsync=off -c synchronous_commit=off -c full_page_writes=off -c max_connections=200 + - uses: actions/checkout@v7 + # Read-only: the module cache CI saves. Build and lint caches start + # empty on purpose; the bench measures cold and warm itself. + - uses: actions/cache/restore@v6 + with: + path: ~/go/pkg/mod + key: v7-gomod-${{ runner.os }}-${{ hashFiles('go.sum') }} + restore-keys: v7-gomod-${{ runner.os }}- + - name: Go toolchain and modules + run: | + go version + go mod download + go mod download -modfile=tools/go.mod + - name: Wait for Postgres + run: | + for _ in $(seq 1 120); do docker exec pg pg_isready -q -h 127.0.0.1 -U postgres && exit 0; sleep 0.5; done + docker logs pg; exit 1 + - name: Bench + run: make bench-devloop BENCH_COLD=1 diff --git a/Makefile b/Makefile index 6fd2c35..7efaafc 100644 --- a/Makefile +++ b/Makefile @@ -99,6 +99,16 @@ ci: ## Run the same checks as CI affected: ## Show what CI checks for this branch (packages affected vs origin/main) ./scripts/affected.sh +.PHONY: bench-devloop +bench-devloop: ## Print dev-loop timings: build, generate, lint, probes, tests per package (BENCH_COLD=1: also cold caches) +ifdef TEST_DATABASE_URL + ./scripts/bench-devloop.sh +else + @$(MAKE) --no-print-directory test-db-up + @port=$$($(COMPOSE) port postgres-test 5432 | sed 's/.*://'); \ + TEST_DATABASE_URL="postgres://postgres:postgres@127.0.0.1:$$port/postgres?sslmode=disable" ./scripts/bench-devloop.sh +endif + .PHONY: migrate migrate: ## Run auth then app migrations against DATABASE_URL $(GO) run ./apps/web/cmd/server migrate diff --git a/scripts/bench-devloop.sh b/scripts/bench-devloop.sh new file mode 100755 index 0000000..ab2a95b --- /dev/null +++ b/scripts/bench-devloop.sh @@ -0,0 +1,233 @@ +#!/usr/bin/env bash +# Prints the dev-loop timing tables (markdown): build per binary (cold, warm, +# after a one-line edit) with size, cold start and memory; the generators; +# golangci-lint (whole run and per linter); the CI probes; and tests per +# package with and without -race, plus the slowest tests. `make bench-devloop` +# runs it; the "Bench dev loop" workflow runs it on GitHub Actions with +# BENCH_COLD=1 so numbers are comparable across branches. +# +# BENCH_COLD=1 also measures cold caches (a fresh GOCACHE and lint cache in a +# temp dir; your own caches are never touched). Tests that use Postgres, and +# cold start, need TEST_DATABASE_URL (`make bench-devloop` starts the test +# database) and psql. BENCH_ONLY=build,generate,lint,probes,tests picks +# sections. Appends the report to $GITHUB_STEP_SUMMARY when set. +set -euo pipefail +cd "$(dirname "$0")/.." + +GO=${GO:-go} +cold=${BENCH_COLD:-} +only=,${BENCH_ONLY:-build,generate,lint,probes,tests}, +want() { [[ $only == *",$1,"* ]]; } +mkdir -p .cache +tmp=$(mktemp -d "$PWD/.cache/bench-devloop.XXXXXX") # not /tmp: often RAM, and cold caches are GBs +pids=() dbs=() edited="" +cleanup() { + for p in "${pids[@]}"; do kill "$p" 2>/dev/null || true; done + for d in "${dbs[@]}"; do psql -q "$TEST_DATABASE_URL" -c "DROP DATABASE IF EXISTS $d WITH (FORCE)" >/dev/null 2>&1 || true; done + if [ -n "$edited" ]; then cp "$tmp/edited.orig" "$edited"; fi + chmod -R u+w "$tmp" 2>/dev/null || true + rm -rf "$tmp" +} +trap cleanup EXIT + +report=$tmp/report.md +log=$tmp/cmd.log +say() { printf '%s\n' "$*" | tee -a "$report"; } +now() { date +%s%N | cut -c1-13; } # milliseconds +secs() { awk -v d="$1" 'BEGIN { printf "%.1fs", d / 1000 }'; } + +# t : runs cmd quietly and prints its wall time; shows the log on failure. +t() { + local s + s=$(now) + if ! "$@" >"$log" 2>&1; then + echo "bench: failed: $*" >&2 + tail -50 "$log" >&2 + return 1 + fi + secs $(($(now) - s)) +} + +# cold : runs cmd with a fresh, empty build and lint cache. +cold() { + chmod -R u+w "$tmp/cold" 2>/dev/null || true + rm -rf "$tmp/cold" && mkdir -p "$tmp/cold/go" "$tmp/cold/lint" + GOCACHE=$tmp/cold/go GOLANGCI_LINT_CACHE=$tmp/cold/lint "$@" +} + +toolrun() { "$GO" tool -modfile=tools/go.mod "$@"; } + +say "## Dev loop timings" +say "" +say "$($GO version | cut -d' ' -f3-), $(nproc) CPUs, $(git rev-parse --short HEAD); $([ -n "$cold" ] && echo 'cold = empty build and lint cache, warm = second run' || echo 'warm caches only (BENCH_COLD=1 adds cold)')." +say "" + +# ---- build ---- +if want build; then +build() { CGO_ENABLED=0 "$GO" build -trimpath -ldflags='-s -w' -o "$@"; } + +# coldstart : migrates throwaway databases, starts the binary +# and prints " ". +coldstart() { + local name=$1 bin=$2 port app admin url pid s rss + command -v psql >/dev/null || return 1 + app=bench_app_$$ admin=bench_admin_$$ + url() { local u=${TEST_DATABASE_URL%%\?*}; echo "${u%/*}/$1?${TEST_DATABASE_URL#*\?}"; } + if [ ! -e "$tmp/migrated" ]; then + for d in $app $admin; do dbs+=("$d"); psql -q "$TEST_DATABASE_URL" -c "CREATE DATABASE $d" >/dev/null; done + DATABASE_URL=$(url $app) PUBLIC_URL=http://bench.test AUTH_SECRET=bench-secret-bench-secret-0123456789abcdef "$tmp/server" migrate >/dev/null 2>&1 + DATABASE_URL=$(url $app) ADMIN_DATABASE_URL=$(url $admin) ADMIN_PUBLIC_URL=http://bench.test ADMIN_AUTH_SECRET=bench-admin-secret-0123456789abcdef0123 \ + "$tmp/admin" migrate >/dev/null 2>&1 + touch "$tmp/migrated" + fi + port=$((20000 + RANDOM % 20000)) + s=$(now) + env -i PATH="$PATH" APP_ENV=development PORT=$port ADMIN_PORT=$port PUBLIC_URL=http://bench.test ADMIN_PUBLIC_URL=http://bench.test \ + AUTH_SECRET=bench-secret-bench-secret-0123456789abcdef ADMIN_AUTH_SECRET=bench-admin-secret-0123456789abcdef0123 \ + DATABASE_URL="$(url $app)" ADMIN_DATABASE_URL="$(url $admin)" "$bin" serve >"$tmp/$name.log" 2>&1 & + pid=$! + pids+=("$pid") + for _ in $(seq 1 1000); do + if curl -fsS -o /dev/null "http://127.0.0.1:$port/healthz" 2>/dev/null; then + local took=$(($(now) - s)) + rss=$(awk '/VmRSS/ { printf "%.0f MB", $2 / 1024 }' "/proc/$pid/status") + kill "$pid" + wait "$pid" 2>/dev/null || true + echo "${took}ms $rss" + return 0 + fi + sleep 0.01 + done + kill "$pid" 2>/dev/null || true + tail -20 "$tmp/$name.log" >&2 + return 1 +} + +say "### Build (CGO_ENABLED=0 -trimpath -ldflags='-s -w', as the Dockerfiles)" +say "" +# The edit adds code to the app's services package: it recompiles, and +# everything above it recompiles or relinks. +rows=() +for b in server:./apps/web/cmd/server:apps/web/internal/services/users.go admin:./apps/admin/cmd/admin:apps/admin/internal/services/users.go; do + IFS=: read -r name pkg edited <<<"$b" + cp "$edited" "$tmp/edited.orig" + c=- + if [ -n "$cold" ]; then c=$(cold t build "$tmp/$name" "$pkg"); fi + build "$tmp/$name" "$pkg" + w=$(t build "$tmp/$name" "$pkg") + printf '\nvar BenchEdit = %s\n' "$(now)" >>"$edited" # new code: recompile + relink + e=$(t build "$tmp/$name" "$pkg") + cp "$tmp/edited.orig" "$edited" + build "$tmp/$name" "$pkg" + edited="" + size=$(awk -v s="$(stat -c %s "$tmp/$name")" 'BEGIN { printf "%.1f MB", s / 1048576 }') + rows+=("$name|$c|$w|$e|$size") +done +say "| binary | cold | warm | after a one-line edit | size | cold start to /healthz | RSS after start |" +say "|---|--:|--:|--:|--:|--:|--:|" +for r in "${rows[@]}"; do + IFS='|' read -r name c w e size <<<"$r" + start=- rss=- + if [ -n "${TEST_DATABASE_URL:-}" ]; then + read -r start rss < <(coldstart "$name" "$tmp/$name" || echo "- -") || true + fi + say "| $name | $c | $w | $e | $size | $start | $rss |" +done +say "" +fi + +# ---- generate ---- +if want generate; then +say "### Generators (\`make generate\` steps)" +say "" +say "| step | cold | warm |" +say "|---|--:|--:|" +gen_sqlc() { toolrun sqlc generate -f packages/database/sqlc.yaml; } +gen_go() { "$GO" generate ./...; } +gen_templ() { for ui in apps/web/ui apps/admin/ui; do [ ! -d $ui ] || "$GO" tool templ generate -path $ui; done; } +for g in sqlc:gen_sqlc "go generate (mockgen)":gen_go templ:gen_templ "make generate":"make --no-print-directory generate"; do + name=${g%%:*} cmd=${g#*:} + c=- + if [ -n "$cold" ]; then c=$(cold t $cmd); fi + $cmd >/dev/null 2>&1 + say "| $name | $c | $(t $cmd) |" +done +if ! git diff --quiet -- '*.go'; then say ""; say "**generated code is stale** (git diff after generate)"; fi +say "" +fi + +# ---- lint ---- +if want lint; then +say "### golangci-lint" +say "" +lint=${GOLANGCI_LINT:-$("$GO" tool -modfile=tools/go.mod -n golangci-lint)} +say "| run | cold | warm |" +say "|---|--:|--:|" +c=- +if [ -n "$cold" ]; then c=$(cold t "$lint" run); fi +"$lint" run >/dev/null 2>&1 || true +say "| golangci-lint run ./... | $c | $(t "$lint" run) |" +say "| gofmt -l . | - | $(t gofmt -l .) |" +say "" +if [ -n "$cold" ]; then + # Each linter alone on an empty lint cache (warm Go build cache): its time + # includes loading and type-checking the packages, the first row alone. + say "Per linter, alone, empty lint cache (includes loading packages):" + say "" + say "| linter | time |" + say "|---|--:|" + mapfile -t linters < <("$lint" linters 2>/dev/null | sed -n '/^Enabled by your/,/^$/p' | sed -nE 's/^([a-z0-9]+):.*/\1/p') + for l in "${linters[@]}"; do + rm -rf "$tmp/lintonly" && mkdir -p "$tmp/lintonly" + say "| $l | $(GOLANGCI_LINT_CACHE=$tmp/lintonly t "$lint" run --enable-only "$l") |" + done + say "" +fi +fi + +# ---- probes ---- +if want probes; then +say "### Probes (prove the lint and test rules still bite)" +say "" +say "| script | warm |" +say "|---|--:|" +say "| check-test-boundaries.sh | $(t ./scripts/check-test-boundaries.sh) |" +say "| check-layering.sh | $(GOLANGCI_LINT=$lint t ./scripts/check-layering.sh) |" +say "| check-test-probes.sh | $(t ./scripts/check-test-probes.sh) |" +say "" +fi + +# ---- tests ---- +if want tests; then +say "### Tests (go test ./..., -count=1)" +say "" +[ -n "${TEST_DATABASE_URL:-}" ] || say "TEST_DATABASE_URL is not set: database tests skip." +"$GO" build -o "$tmp/testbudget" ./scripts/testbudget +# gotest : one run; prints its wall time, keeps the -json. +gotest() { + local name=$1 s + shift + s=$(now) + "$GO" test -count=1 -json "$@" ./... >"$tmp/$name.json" 2>"$log" || { + "$tmp/testbudget" -pkg 0 -test 0 -top 0 <"$tmp/$name.json" >&2 || true + cat "$log" >&2 + return 1 + } + secs $(($(now) - s)) +} +say "| run | cold (compile + run) | warm (run only) |" +say "|---|--:|--:|" +c=- +if [ -n "$cold" ]; then c=$(cold gotest cold-race -race -coverprofile="$tmp/cover.out"); fi +say "| -race -coverprofile (CI) | $c | $(gotest race -race -coverprofile="$tmp/cover.out") |" +c=- +if [ -n "$cold" ]; then c=$(cold gotest cold-plain); fi +say "| plain | $c | $(gotest plain) |" +say "" +say "Per package (test binary wall time, warm):" +say "" +"$tmp/testbudget" table "-race -cover=$tmp/race.json" "plain=$tmp/plain.json" | tee -a "$report" +say "" +fi + +if [ -n "${GITHUB_STEP_SUMMARY:-}" ]; then cat "$report" >>"$GITHUB_STEP_SUMMARY"; fi diff --git a/scripts/testbudget/main.go b/scripts/testbudget/main.go new file mode 100644 index 0000000..2ab0b49 --- /dev/null +++ b/scripts/testbudget/main.go @@ -0,0 +1,261 @@ +// Command testbudget keeps the test suite fast as it grows. It reads +// `go test -json` on stdin, prints what plain `go test` would (ok/FAIL lines, +// the output of failed tests, build errors), lists the slowest tests, and +// fails when a package's test binary or a single top-level test runs longer +// than its budget. `make test` and CI pipe every run through it. +// +// go test -race -json ./... | go run ./scripts/testbudget +// +// Budgets are wall time on a 2-vCPU CI runner under -race, test execution +// only (compiling is not counted). A test over budget is a design problem, +// not a flake: see AGENTS.md "Fast tests" before raising a budget. +// +// The table subcommand renders saved -json runs as markdown for +// `make bench-devloop`: +// +// testbudget table race=race.json norace=norace.json +package main + +import ( + "bufio" + "cmp" + "encoding/json" + "flag" + "fmt" + "io" + "os" + "slices" + "strings" + "time" +) + +// Default budgets. Raise one only with a reason in the commit message. +const ( + defaultPackageBudget = 30 * time.Second + defaultTestBudget = 5 * time.Second +) + +// event is one line of `go test -json` (cmd/test2json). +type event struct { + Action string + Package string + Test string + Elapsed float64 + Output string + ImportPath string // build-output, build-fail +} + +func main() { + if len(os.Args) > 1 && os.Args[1] == "table" { + os.Exit(table(os.Stdout, os.Args[2:])) + } + pkgBudget := flag.Duration("pkg", defaultPackageBudget, "budget for one package's test binary (0: off)") + testBudget := flag.Duration("test", defaultTestBudget, "budget for one top-level test (0: off)") + top := flag.Int("top", 10, "list this many slowest tests at the end") + flag.Parse() + os.Exit(run(os.Stdin, os.Stdout, *pkgBudget, *testBudget, *top)) +} + +type result struct { + name string // package, or "package Test" + elapsed time.Duration +} + +// run streams a go test -json run and returns the exit code. +func run(in io.Reader, out io.Writer, pkgBudget, testBudget time.Duration, top int) int { + w := bufio.NewWriter(out) + defer func() { _ = w.Flush() }() + + type key struct{ pkg, test string } + logs := map[key][]string{} + var pkgs, tests []result + failed := false + + sc := bufio.NewScanner(in) + sc.Buffer(make([]byte, 0, 1<<20), 1<<26) + for sc.Scan() { + line := sc.Bytes() + var e event + if len(line) == 0 || line[0] != '{' || json.Unmarshal(line, &e) != nil { + fmt.Fprintf(w, "%s\n", line) // not test2json: pass through + continue + } + k := key{e.Package, e.Test} + switch e.Action { + case "build-output": + fmt.Fprint(w, e.Output) + case "build-fail": + failed = true + case "output": + if !strings.HasPrefix(e.Output, "=== ") { + logs[k] = append(logs[k], e.Output) + } + case "pass", "skip", "fail": + lines := logs[k] + delete(logs, k) + d := seconds(e.Elapsed) + if e.Action == "fail" { + failed = true + for _, l := range lines { + fmt.Fprint(w, l) + } + break + } + if e.Test != "" { + if e.Action == "pass" && !strings.Contains(e.Test, "/") { + tests = append(tests, result{e.Package + " " + e.Test, d}) + } + break + } + // A passing package prints only its summary, as plain go test does. + for _, l := range lines { + if strings.HasPrefix(l, "ok ") || strings.HasPrefix(l, "?") || strings.HasPrefix(l, "\t") { + fmt.Fprint(w, l) + } + } + if e.Action == "pass" { + pkgs = append(pkgs, result{e.Package, d}) + } + } + } + if err := sc.Err(); err != nil { + fmt.Fprintln(w, "testbudget: read:", err) + return 1 + } + + byTime := func(a, b result) int { return cmp.Compare(b.elapsed, a.elapsed) } + slices.SortFunc(tests, byTime) + slices.SortFunc(pkgs, byTime) + if top > 0 && len(tests) > 0 { + fmt.Fprintf(w, "\nslowest tests:\n") + for _, t := range tests[:min(top, len(tests))] { + fmt.Fprintf(w, " %7.2fs %s\n", t.elapsed.Seconds(), t.name) + } + } + + over := overBudget(pkgs, pkgBudget, "package") + over = append(over, overBudget(tests, testBudget, "test")...) + if len(over) > 0 { + fmt.Fprintf(w, "\nFAIL over the test-time budget:\n") + for _, o := range over { + fmt.Fprintf(w, " %s\n", o) + } + fmt.Fprintf(w, "make it fast (AGENTS.md \"Fast tests\"): t.Parallel(), dbtest clones, fake clocks, no sleeps\n") + failed = true + } + if failed { + return 1 + } + return 0 +} + +func overBudget(rs []result, budget time.Duration, kind string) []string { + var over []string + for _, r := range rs { + if budget > 0 && r.elapsed > budget { + over = append(over, fmt.Sprintf("%s %s: %.2fs (budget %s)", kind, r.name, r.elapsed.Seconds(), budget)) + } + } + return over +} + +func seconds(s float64) time.Duration { return time.Duration(s * float64(time.Second)) } + +// table prints a markdown table of per-package test time, one column per +// name=file.json run, then the slowest tests of the first run. +func table(out io.Writer, args []string) int { + type col struct { + name string + pkgs map[string]time.Duration + tests []result + } + var cols []col + var order []string + seen := map[string]bool{} + for _, a := range args { + name, file, ok := strings.Cut(a, "=") + if !ok { + fmt.Fprintln(os.Stderr, "usage: testbudget table name=file.json ...") + return 2 + } + f, err := os.Open(file) + if err != nil { + fmt.Fprintln(os.Stderr, err) + return 1 + } + c := col{name: name, pkgs: map[string]time.Duration{}} + sc := bufio.NewScanner(f) + sc.Buffer(make([]byte, 0, 1<<20), 1<<26) + for sc.Scan() { + var e event + if json.Unmarshal(sc.Bytes(), &e) != nil || (e.Action != "pass" && e.Action != "fail") { + continue + } + if e.Test == "" { + c.pkgs[e.Package] = seconds(e.Elapsed) + if !seen[e.Package] { + seen[e.Package] = true + order = append(order, e.Package) + } + } else if !strings.Contains(e.Test, "/") { + c.tests = append(c.tests, result{e.Package + " " + e.Test, seconds(e.Elapsed)}) + } + } + _ = f.Close() + cols = append(cols, c) + } + if len(cols) == 0 { + return 0 + } + slices.SortFunc(order, func(a, b string) int { return cmp.Compare(cols[0].pkgs[b], cols[0].pkgs[a]) }) + + mod := commonPrefix(order) + fmt.Fprintf(out, "| package |") + for _, c := range cols { + fmt.Fprintf(out, " %s |", c.name) + } + fmt.Fprintf(out, "\n|---|%s\n", strings.Repeat("--:|", len(cols))) + totals := make([]time.Duration, len(cols)) + for _, p := range order { + fmt.Fprintf(out, "| %s |", strings.TrimPrefix(p, mod)) + for i, c := range cols { + if d, ok := c.pkgs[p]; ok { + totals[i] += d + fmt.Fprintf(out, " %.2fs |", d.Seconds()) + } else { + fmt.Fprintf(out, " - |") + } + } + fmt.Fprintln(out) + } + fmt.Fprintf(out, "| **sum** |") + for _, t := range totals { + fmt.Fprintf(out, " **%.2fs** |", t.Seconds()) + } + fmt.Fprintln(out) + + tests := cols[0].tests + slices.SortFunc(tests, func(a, b result) int { return cmp.Compare(b.elapsed, a.elapsed) }) + fmt.Fprintf(out, "\nSlowest top-level tests (%s):\n\n| test | time |\n|---|--:|\n", cols[0].name) + for _, t := range tests[:min(15, len(tests))] { + fmt.Fprintf(out, "| %s | %.2fs |\n", strings.TrimPrefix(t.name, mod), t.elapsed.Seconds()) + } + return 0 +} + +// commonPrefix is the module path shared by the packages ("" if fewer than two). +func commonPrefix(pkgs []string) string { + if len(pkgs) < 2 { + return "" + } + p := pkgs[0] + for _, s := range pkgs[1:] { + for !strings.HasPrefix(s, p) { + p = p[:len(p)-1] + } + } + if i := strings.LastIndex(p, "/"); i >= 0 { + return p[:i+1] + } + return "" +} From f8b016b02e0a8b8a15a37c6d03fc67132784581a Mon Sep 17 00:00:00 2001 From: JJ Eaton <58495321+jayleaton@users.noreply.github.com> Date: Thu, 22 Oct 2026 18:50:09 +0700 Subject: [PATCH 02/10] perf(test): every test runs in parallel, no sleeps, crawl checks each body once paralleltest + tparallel make t.Parallel() the rule (no exceptions were needed) and forbidigo bans time.Sleep in tests (synctest fake-clock sleeps go through one documented helper). Fixes that made it safe: the UI crawls no longer share a package-level crawlMethod, admin render tests stop mutating templ's global buffer size, TestDopplerErrors and the web services table get per-case fakes and controllers, and packages/auth migrates a template once and clones it (shared database and newDatabase) instead of migrating per database. The crawls split into parallel subtests and memoize the body-only checks: static assets come back byte-identical in every state, and re-scanning them was most of the crawl's time under -race. Co-Authored-By: Claude Opus 5.5 --- .golangci.yml | 13 ++ apps/admin/internal/api/admins_test.go | 1 + apps/admin/internal/api/audit_test.go | 2 + apps/admin/internal/api/route_test.go | 11 ++ apps/admin/internal/api/users_test.go | 4 + apps/admin/internal/config/config_test.go | 6 + apps/admin/internal/server/server_test.go | 5 + apps/admin/internal/services/admins_test.go | 7 + .../admin/internal/services/analytics_test.go | 1 + apps/admin/internal/services/audit_test.go | 1 + .../internal/services/authevents_test.go | 3 + .../admin/internal/services/reconcile_test.go | 8 + apps/admin/internal/services/services_test.go | 12 ++ apps/admin/internal/services/users_test.go | 7 + apps/admin/ui/crawl_test.go | 140 +++++++++++------- apps/admin/ui/handlers_test.go | 29 ++++ apps/admin/ui/i18n/i18n_test.go | 8 + apps/admin/ui/render_test.go | 69 ++++++--- apps/admin/ui/static/static_test.go | 2 + apps/web/internal/api/users_test.go | 10 ++ apps/web/internal/config/config_test.go | 6 + apps/web/internal/server/server_test.go | 4 + apps/web/internal/services/users_test.go | 45 ++++-- apps/web/ui/account_test.go | 16 ++ apps/web/ui/crawl_test.go | 126 ++++++++++------ apps/web/ui/guards_test.go | 7 + apps/web/ui/handlers_test.go | 12 ++ apps/web/ui/render_test.go | 3 + packages/auth/account_test.go | 22 ++- packages/auth/admin_test.go | 18 ++- packages/auth/events_test.go | 5 + packages/auth/flows_test.go | 34 ++++- packages/auth/keyring_test.go | 14 +- packages/auth/links_test.go | 4 + packages/auth/mailer_test.go | 10 +- packages/auth/main_test.go | 131 +++++++++++----- packages/auth/tokens_test.go | 15 ++ packages/auth/twofactor_test.go | 5 + packages/secrets/bootstrap_test.go | 8 +- packages/secrets/doppler_test.go | 49 ++++-- packages/secrets/fake_test.go | 6 + packages/secrets/manager_test.go | 65 +++++--- packages/secrets/redact_test.go | 16 ++ packages/secrets/reveal_test.go | 4 + 44 files changed, 731 insertions(+), 233 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 10e456a..32c2011 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -14,8 +14,10 @@ linters: - misspell - nilerr - noctx + - paralleltest # fast tests: every test calls t.Parallel() (AGENTS.md "Fast tests") - rowserrcheck - sqlclosecheck + - tparallel - unconvert - usestdlibvars - wastedassign @@ -32,6 +34,14 @@ linters: - pattern: ^auth\.Auth\.Users$ pkg: ^github\.com/jayleaton/ai-patterns-go/packages/auth$ msg: user administration (ban, roles, any user's sessions) is for apps/admin only + # Tests only (see exclusions): a test that sleeps is slow and flaky. + - pattern: ^time\.Sleep$ + msg: tests never sleep; wait on a channel or condition, or use testing/synctest for a fake clock (AGENTS.md "Fast tests") + # Top-level tests must call t.Parallel(); a test that cannot (shared + # global state) says why with //nolint:paralleltest // . + # Subtests are parallel where they are slow; tparallel keeps it consistent. + paralleltest: + ignore-missing-subtests: true # Layering: packages/database -> services -> transports (api, ui), for # apps/web and apps/admin alike. See AGENTS.md "Architecture". depguard: @@ -303,6 +313,9 @@ linters: - path: (apps/admin|packages/auth)/ linters: [forbidigo] text: is for apps/admin only + - path-except: _test\.go + linters: [forbidigo] + text: tests never sleep formatters: enable: diff --git a/apps/admin/internal/api/admins_test.go b/apps/admin/internal/api/admins_test.go index 0ba4b46..2f8ab4c 100644 --- a/apps/admin/internal/api/admins_test.go +++ b/apps/admin/internal/api/admins_test.go @@ -74,6 +74,7 @@ var adminRoutes = map[string]route{ // TestCreateAdminInviteBody covers every CreateAdminInvite rule: the address // is normalised and the name defaults to its local part. func TestCreateAdminInviteBody(t *testing.T) { + t.Parallel() long := strings.Repeat("x", 244) + "@example.com" // 256 characters for _, email := range []string{"", " ", "not an email", "Ada ", "ada@", "@example.com", long} { wantInvalid(t, "POST", r.AdminInvites, `{"email":"`+email+`","name":"Ada"}`, map[string]string{"email": emailRule}) diff --git a/apps/admin/internal/api/audit_test.go b/apps/admin/internal/api/audit_test.go index ce6f9c7..a608a99 100644 --- a/apps/admin/internal/api/audit_test.go +++ b/apps/admin/internal/api/audit_test.go @@ -39,6 +39,7 @@ var auditRoutes = map[string]route{ // TestListAuditLogQuery: filters are trimmed, the page size defaults, and the // bounds are inclusive. func TestListAuditLogQuery(t *testing.T) { + t.Parallel() cases := []struct { q url.Values want validate.ListAuditLog @@ -62,6 +63,7 @@ func TestListAuditLogQuery(t *testing.T) { // TestListAuditLogInvalid covers every ListAuditLog rule. func TestListAuditLogInvalid(t *testing.T) { + t.Parallel() limit := map[string]string{"limit": "must be 1-100"} cases := []struct { q string diff --git a/apps/admin/internal/api/route_test.go b/apps/admin/internal/api/route_test.go index 7c9f149..dc321c0 100644 --- a/apps/admin/internal/api/route_test.go +++ b/apps/admin/internal/api/route_test.go @@ -134,15 +134,18 @@ func wantOK(t *testing.T, method, path, body string, expect func(m svcs)) { // TestRoutes runs the boundary cases against every endpoint. The mocks fail // the test on any service call a case does not expect. func TestRoutes(t *testing.T) { + t.Parallel() routes := allRoutes(t) for _, name := range slices.Sorted(maps.Keys(routes)) { rt := routes[name] t.Run(name+"/no session is 401", func(t *testing.T) { + t.Parallel() _, h := newRouter(t) res := apitest.Do(t, h, apitest.Anonymous, rt.method, rt.path, rt.body) wantError(t, res, 401, "Unauthorized") }) t.Run(name+"/invalid input is 400", func(t *testing.T) { + t.Parallel() m, h := newRouter(t) path, body := rt.path, rt.body switch { @@ -163,12 +166,14 @@ func TestRoutes(t *testing.T) { }) for _, e := range append(slices.Clone(everyRoute), rt.errs...) { t.Run(fmt.Sprintf("%s/%v is %d", name, e.err, e.status), func(t *testing.T) { + t.Parallel() m, h := newRouter(t) rt.expect(m, e.err) wantError(t, apitest.Do(t, h, apitest.AsAdmin(grace), rt.method, rt.path, rt.body), e.status, e.msg) }) } t.Run(name+"/ok is 200", func(t *testing.T) { + t.Parallel() m, h := newRouter(t) rt.expect(m, nil) res := apitest.Do(t, h, apitest.AsAdmin(grace), rt.method, rt.path, rt.body) @@ -186,6 +191,7 @@ func TestRoutes(t *testing.T) { // TestCallerActor: handlers act as the signed-in admin, on their session, // carrying their 2FA state (the services' 2FA policy reads it). func TestCallerActor(t *testing.T) { + t.Parallel() want := services.Actor{AdminID: "a1", Email: "grace@example.com", SessionID: "session-a1", TwoFactorEnabled: true} if actor != want { t.Fatalf("actor = %+v, want %+v", actor, want) @@ -198,6 +204,7 @@ func TestCallerActor(t *testing.T) { // TestErrorMapping covers every service error kind once; it is the same // route wrapper for every endpoint. func TestErrorMapping(t *testing.T) { + t.Parallel() cases := []svcErr{ {services.ErrUserNotFound, 404, "User not found"}, {fmt.Errorf("auth: %w", services.ErrAdminExists), 409, "This person is already an admin"}, @@ -223,6 +230,7 @@ func TestErrorMapping(t *testing.T) { // that reads a body: none reaches the service (the mocks have no // expectations) and each names the body. func TestDecodeBody(t *testing.T) { + t.Parallel() bodies := []struct{ method, path string }{ {"POST", config.UserPath("u1", "/ban")}, {"PUT", config.UserPath("u1", "/role")}, @@ -265,6 +273,7 @@ func TestDecodeBody(t *testing.T) { // TestBodyLimit: the route wrapper caps request bodies. func TestBodyLimit(t *testing.T) { + t.Parallel() _, h := newRouter(t) res := apitest.Do(t, h, apitest.AsAdmin(grace), "POST", config.UserPath("u1", "/ban"), `{"banReason":"`+strings.Repeat("x", 2<<20)+`"}`) if res.Status != 400 || res.Details["body"] == "" { @@ -275,6 +284,7 @@ func TestBodyLimit(t *testing.T) { // TestPublic: the public wrapper needs no session and maps errors the same // way. func TestPublic(t *testing.T) { + t.Parallel() ok := api.Public(func(*http.Request) (api.Success, error) { return api.Success{Success: true}, nil }) res := apitest.Do(t, ok, apitest.Anonymous, "GET", "/", "") if res.Status != 200 { @@ -289,6 +299,7 @@ func TestPublic(t *testing.T) { // TestValidationErrorString: a validation error reads the same every time // (fields sorted), for logs and wrapped errors. func TestValidationErrorString(t *testing.T) { + t.Parallel() err := &validate.Error{Fields: map[string]string{"offset": "must be 0 or more", "limit": "must be 1-100"}} if got, want := err.Error(), "invalid input: limit: must be 1-100, offset: must be 0 or more"; got != want { t.Fatalf("Error() = %q, want %q", got, want) diff --git a/apps/admin/internal/api/users_test.go b/apps/admin/internal/api/users_test.go index a108ff9..f864282 100644 --- a/apps/admin/internal/api/users_test.go +++ b/apps/admin/internal/api/users_test.go @@ -104,6 +104,7 @@ var userRoutes = map[string]route{ // TestListUsersQuery: every query parameter is decoded, trimmed where it is // free text, and defaulted, and reaches the service as given. func TestListUsersQuery(t *testing.T) { + t.Parallel() cases := []struct { q url.Values want validate.ListUsers @@ -140,6 +141,7 @@ func TestListUsersQuery(t *testing.T) { // TestListUsersInvalid covers every ListUsers rule: bad queries never reach // the service and name each failing parameter. func TestListUsersInvalid(t *testing.T) { + t.Parallel() limit := map[string]string{"limit": "must be 1-100"} whole := func(k string) map[string]string { return map[string]string{k: "must be a whole number"} } cases := []struct { @@ -171,6 +173,7 @@ func TestListUsersInvalid(t *testing.T) { // TestBanUserBody covers every BanUser rule and its boundaries. func TestBanUserBody(t *testing.T) { + t.Parallel() path := config.UserPath("u2", "/ban") for _, body := range []string{ `{}`, @@ -206,6 +209,7 @@ func TestBanUserBody(t *testing.T) { // TestSetUserRoleBody covers the role shape (which roles exist is the // service's rule, so any well-formed name reaches it). func TestSetUserRoleBody(t *testing.T) { + t.Parallel() path := config.UserPath("u2", "/role") for _, body := range []string{ `{}`, diff --git a/apps/admin/internal/config/config_test.go b/apps/admin/internal/config/config_test.go index 7ba4cc0..0585e2a 100644 --- a/apps/admin/internal/config/config_test.go +++ b/apps/admin/internal/config/config_test.go @@ -25,6 +25,7 @@ func valid() map[string]string { } func TestLoadDevelopmentDefaults(t *testing.T) { + t.Parallel() kv := valid() kv["PORT"] = "3000" // the web app's; ADMIN_PORT unset falls back to it kv["ADMIN_BOOTSTRAP_EMAIL"] = " Root@Example.com " @@ -51,6 +52,7 @@ func TestLoadDevelopmentDefaults(t *testing.T) { } func TestLoadProductionDefaultsTo2FA(t *testing.T) { + t.Parallel() kv := valid() kv["APP_ENV"], kv["RESEND_API_KEY"] = "production", "re_x" cfg, err := Load(env(kv)) @@ -64,6 +66,7 @@ func TestLoadProductionDefaultsTo2FA(t *testing.T) { } func TestLoadReportsEveryProblem(t *testing.T) { + t.Parallel() _, err := Load(env(map[string]string{ "APP_ENV": "production", "ADMIN_PUBLIC_URL": "https://admin.example.com/path", "ADMIN_REQUIRE_2FA": "maybe", "ADMIN_BOOTSTRAP_EMAIL": "root", @@ -79,6 +82,7 @@ func TestLoadReportsEveryProblem(t *testing.T) { } func TestLoadRefusesSharedSecretsAndDatabases(t *testing.T) { + t.Parallel() kv := valid() kv["AUTH_SECRET"] = kv["ADMIN_AUTH_SECRET"] kv["ADMIN_DATABASE_URL"] = kv["DATABASE_URL"] @@ -89,6 +93,7 @@ func TestLoadRefusesSharedSecretsAndDatabases(t *testing.T) { } func TestLoadLiveSecrets(t *testing.T) { + t.Parallel() kv := valid() kv["ADMIN_AUTH_SECRET_PREVIOUS"] = strings.Repeat("p", 32) kv["ADMIN_AUTH_SECRET_GRACE"] = "24h" @@ -118,6 +123,7 @@ func TestLoadLiveSecrets(t *testing.T) { // A loaded Config prints, logs and marshals without its credentials, so // logging it (or a struct holding it) can never leak them. func TestConfigNeverPrintsSecrets(t *testing.T) { + t.Parallel() values := []string{"apppass-4242", "adminpass-5353", "admin-secret-" + strings.Repeat("z", 32), "re_resend_9999"} kv := valid() kv["DATABASE_URL"] = "postgres://app:" + values[0] + "@db/app" diff --git a/apps/admin/internal/server/server_test.go b/apps/admin/internal/server/server_test.go index d061f85..b9adc93 100644 --- a/apps/admin/internal/server/server_test.go +++ b/apps/admin/internal/server/server_test.go @@ -68,6 +68,7 @@ func serve(h http.Handler, req *http.Request) *httptest.ResponseRecorder { } func TestHealthzPingsEveryDatabase(t *testing.T) { + t.Parallel() get := func(dbs map[string]Pinger) *httptest.ResponseRecorder { return serve(newHandler(dbs, nil), httptest.NewRequest(http.MethodGet, config.APIRoutes.Healthz, nil)) } @@ -97,6 +98,7 @@ type staleSecrets struct{} func (staleSecrets) Healthy() error { return errors.New("stale") } func TestMountsAndMiddleware(t *testing.T) { + t.Parallel() h := newHandler(map[string]Pinger{"app": pinger{}}, stubUI) cases := []struct { name string @@ -125,6 +127,7 @@ func TestMountsAndMiddleware(t *testing.T) { // tokens), hidden from browsers; endpoints that would bypass the audit log // never are. func TestAuthEndpoints(t *testing.T) { + t.Parallel() allowed := []string{ "POST sign-in/email", "POST sign-out", "GET get-session", "POST set-password", "POST two-factor/enable", "POST two-factor/verify-totp", "POST two-factor/verify-backup-code", @@ -165,6 +168,7 @@ func TestAuthEndpoints(t *testing.T) { // When a deployment mounts the JSON API it is for scripts only: browsers // get the bare 404 of an unknown path, and so does a wrong method. func TestOptInAPIIsHiddenFromBrowsers(t *testing.T) { + t.Parallel() h := withAPI() for _, path := range []string{config.APIRoutes.Users, config.APIRoutes.Admins, config.APIRoutes.Audit, config.APIRoutes.Analytics} { if rec := serve(h, httptest.NewRequest(http.MethodGet, path, nil)); rec.Code != http.StatusUnauthorized || !strings.Contains(rec.Body.String(), `"Unauthorized"`) { @@ -195,6 +199,7 @@ func TestOptInAPIIsHiddenFromBrowsers(t *testing.T) { // Emailed set-password links carry a credential in the path; logs keep the // route only. func TestRequestLogsDropTokens(t *testing.T) { + t.Parallel() var logs strings.Builder h := New(Deps{Log: slog.New(slog.NewTextHandler(&logs, nil)), DBs: map[string]Pinger{}, Auth: noAuth{}}) serve(h, httptest.NewRequest(http.MethodGet, "/api/auth/reset-password/s3cret-token?callbackURL=%2Fset-password", nil)) diff --git a/apps/admin/internal/services/admins_test.go b/apps/admin/internal/services/admins_test.go index c7fe2c4..756a2cd 100644 --- a/apps/admin/internal/services/admins_test.go +++ b/apps/admin/internal/services/admins_test.go @@ -38,6 +38,7 @@ func adminAudit(actor services.Actor, action string, details map[string]string) } func TestListAdmins(t *testing.T) { + t.Parallel() svc, accounts, _ := newAdminService(t) accounts.EXPECT().ListAdmins(ctx).Return([]services.Admin{pending}, nil) @@ -59,6 +60,7 @@ func TestListAdmins(t *testing.T) { // failing change) of every mutation are in TestMutationsFailClosed. func TestInviteAdmin(t *testing.T) { + t.Parallel() svc, accounts, log := newAdminService(t) notFound := fmt.Errorf("get: %w", services.ErrRecordNotFound) @@ -157,6 +159,7 @@ func TestInviteAdmin(t *testing.T) { } func TestAdminActive(t *testing.T) { + t.Parallel() for _, c := range []struct { a services.Admin want bool @@ -168,6 +171,7 @@ func TestAdminActive(t *testing.T) { } func TestDisableAdmin(t *testing.T) { + t.Parallel() svc, accounts, log := newAdminService(t) gomock.InOrder( @@ -218,6 +222,7 @@ func TestDisableAdmin(t *testing.T) { } func TestEnableAdmin(t *testing.T) { + t.Parallel() svc, accounts, log := newAdminService(t) gomock.InOrder( @@ -244,6 +249,7 @@ func TestEnableAdmin(t *testing.T) { } func TestRemoveAdmin(t *testing.T) { + t.Parallel() svc, accounts, log := newAdminService(t) gomock.InOrder( @@ -289,6 +295,7 @@ func TestRemoveAdmin(t *testing.T) { } func TestBootstrap(t *testing.T) { + t.Parallel() svc, accounts, log := newAdminService(t) // The intent targets the email: the admin is created inside the change. diff --git a/apps/admin/internal/services/analytics_test.go b/apps/admin/internal/services/analytics_test.go index 7965983..e30c1bb 100644 --- a/apps/admin/internal/services/analytics_test.go +++ b/apps/admin/internal/services/analytics_test.go @@ -8,6 +8,7 @@ import ( ) func TestOverview(t *testing.T) { + t.Parallel() p := newPorts(t) svc := services.NewAnalyticsService(p.analytics, services.Policy{RequireTwoFactor: true}) want := services.Overview{TotalUsers: 10, VerifiedUsers: 8, BannedUsers: 1, NewUsers7d: 2, ActiveSessions: 5, GeneratedAt: time.Unix(1700000000, 0)} diff --git a/apps/admin/internal/services/audit_test.go b/apps/admin/internal/services/audit_test.go index 4077680..a909024 100644 --- a/apps/admin/internal/services/audit_test.go +++ b/apps/admin/internal/services/audit_test.go @@ -10,6 +10,7 @@ import ( ) func TestAuditList(t *testing.T) { + t.Parallel() p := newPorts(t) svc := services.NewAuditService(p.log, services.Policy{RequireTwoFactor: true}) in := validate.ListAuditLog{Action: "user.ban", TargetID: "u1", Limit: 10, Offset: 20} diff --git a/apps/admin/internal/services/authevents_test.go b/apps/admin/internal/services/authevents_test.go index 67377e8..97cf076 100644 --- a/apps/admin/internal/services/authevents_test.go +++ b/apps/admin/internal/services/authevents_test.go @@ -31,6 +31,7 @@ func selfAudit(id, email, action string, details map[string]string) services.New // No policy check by design: the events describe authentication itself, so // even an admin without 2FA is recorded. func TestSignedIn(t *testing.T) { + t.Parallel() svc, log := newAuthEventService(t) for _, method := range []string{services.SignInPassword, services.SignInTwoFactor, services.SignInSetPassword} { @@ -45,6 +46,7 @@ func TestSignedIn(t *testing.T) { } func TestSignedOut(t *testing.T) { + t.Parallel() svc, log := newAuthEventService(t) log.EXPECT().Append(ctx, selfAudit("a1", "grace@example.com", "admin.sign_out", nil)).Return(nil) @@ -57,6 +59,7 @@ func TestSignedOut(t *testing.T) { } func TestTwoFactorFailed(t *testing.T) { + t.Parallel() svc, log := newAuthEventService(t) // Attributed to the admin of the pending sign-in, with auth's reason. diff --git a/apps/admin/internal/services/reconcile_test.go b/apps/admin/internal/services/reconcile_test.go index 52b422b..1ecc403 100644 --- a/apps/admin/internal/services/reconcile_test.go +++ b/apps/admin/internal/services/reconcile_test.go @@ -45,6 +45,7 @@ func pendingEntry(id int64, action, targetID, targetLabel string, details map[st // Every action is judged by the state its change should have produced. func TestReconcileVerdicts(t *testing.T) { + t.Parallel() entryAt := time.Date(2026, 10, 22, 9, 0, 0, 0, time.UTC) user := func(u services.User, sessions ...services.Session) func(p ports) { return func(p ports) { @@ -113,6 +114,7 @@ func TestReconcileVerdicts(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { + t.Parallel() r, p := newReconciler(t) e := pendingEntry(entryID, c.action, c.target, labels[c.target], c.detail) p.log.EXPECT().Pending(ctx, staleCutoff(), 100).Return([]services.AuditEntry{e}, nil) @@ -141,6 +143,7 @@ func TestReconcileVerdicts(t *testing.T) { // An entry whose state cannot be read stays pending; the others are resolved // and the read errors are returned. func TestReconcileSkipsUnreadableState(t *testing.T) { + t.Parallel() r, p := newReconciler(t) errUsers := errors.New("app database down") errAdmins := errors.New("admin database down") @@ -170,6 +173,7 @@ func TestReconcileSkipsUnreadableState(t *testing.T) { // A failed outcome write stops the pass: the store is likely down. func TestReconcileStopsWhenResolveFails(t *testing.T) { + t.Parallel() r, p := newReconciler(t) errRead := errors.New("app database down") entries := []services.AuditEntry{ @@ -193,6 +197,7 @@ func TestReconcileStopsWhenResolveFails(t *testing.T) { } func TestReconcilePendingFails(t *testing.T) { + t.Parallel() r, p := newReconciler(t) p.log.EXPECT().Pending(ctx, staleCutoff(), 100).Return(nil, errDB) got, err := r.Reconcile(ctx, olderThan) @@ -203,6 +208,7 @@ func TestReconcilePendingFails(t *testing.T) { } func TestReconcileNothingPending(t *testing.T) { + t.Parallel() r, p := newReconciler(t) p.log.EXPECT().Pending(ctx, staleCutoff(), 100).Return(nil, nil) if got, err := r.Reconcile(ctx, olderThan); err != nil || got != (services.ReconcileResult{}) { @@ -213,6 +219,7 @@ func TestReconcileNothingPending(t *testing.T) { // A full batch that made progress is followed by the next one, until a // batch comes back short. func TestReconcileBatches(t *testing.T) { + t.Parallel() r, p := newReconciler(t) batch := func(from, n int) []services.AuditEntry { entries := make([]services.AuditEntry, n) @@ -235,6 +242,7 @@ func TestReconcileBatches(t *testing.T) { // An unreadable entry in a full batch does not end the pass: the next read // asks for one more row (the skipped entry comes back first) and skips it. func TestReconcileReadsPastSkippedEntries(t *testing.T) { + t.Parallel() r, p := newReconciler(t) errRead := errors.New("app database down") unreadable := pendingEntry(1, "user.ban", "u1", "ada@example.com", nil) diff --git a/apps/admin/internal/services/services_test.go b/apps/admin/internal/services/services_test.go index 89d174a..148ef2c 100644 --- a/apps/admin/internal/services/services_test.go +++ b/apps/admin/internal/services/services_test.go @@ -119,9 +119,11 @@ func policyCalls(p ports, policy services.Policy) map[string]func(services.Actor // Every method refuses before touching a port: the mocks have no // expectations, so any call fails the test. func TestServicesRefuseUnauthorizedActors(t *testing.T) { + t.Parallel() policy := services.Policy{RequireTwoFactor: true} for name := range policyCalls(newPorts(t), policy) { t.Run(name, func(t *testing.T) { + t.Parallel() call := policyCalls(newPorts(t), policy)[name] // mocks bound to this subtest wantErr(t, call(services.Actor{}), services.ErrForbidden) // no actor is nobody noTwoFactor := grace @@ -139,6 +141,7 @@ func TestServicesRefuseUnauthorizedActors(t *testing.T) { // Without the 2FA requirement any admin may act, and the operator always may. func TestPolicyAllowsAdminsAndSystem(t *testing.T) { + t.Parallel() for _, policy := range []services.Policy{{}, {RequireTwoFactor: true}} { for _, actor := range []services.Actor{cli, {AdminID: "a2", Email: "bob@example.com"}} { if policy.RequireTwoFactor && !actor.System { @@ -155,6 +158,7 @@ func TestPolicyAllowsAdminsAndSystem(t *testing.T) { } func TestSystemActor(t *testing.T) { + t.Parallel() want := services.Actor{AdminID: "system:cli", Email: "cli", System: true} if cli != want { t.Fatalf("SystemActor = %+v, want %+v", cli, want) @@ -163,6 +167,7 @@ func TestSystemActor(t *testing.T) { // Kind sentinels match every error of their kind; specific errors only themselves. func TestErrorKinds(t *testing.T) { + t.Parallel() cases := []struct { err, target error want bool @@ -391,6 +396,7 @@ func mutations() []mutation { // failed intent write stops the change (the change ports have no // expectations, so calling one fails the test), and the outcome follows it. func TestMutationsFailClosed(t *testing.T) { + t.Parallel() for _, m := range mutations() { // sequence expects the reads, the intent, the change failing with // portErr and its outcome write (returning resolveErr), in order. @@ -404,6 +410,7 @@ func TestMutationsFailClosed(t *testing.T) { } t.Run(m.name+"/succeeded", func(t *testing.T) { + t.Parallel() p := newPorts(t) sequence(p, nil, succeeded, nil) if err := m.call(p); err != nil { @@ -412,6 +419,7 @@ func TestMutationsFailClosed(t *testing.T) { }) t.Run(m.name+"/intent not written", func(t *testing.T) { + t.Parallel() p := newPorts(t) gomock.InOrder(append(m.reads(p), p.log.EXPECT().Begin(ctx, m.intent).Return(int64(0), errDB))...) err := m.call(p) @@ -422,6 +430,7 @@ func TestMutationsFailClosed(t *testing.T) { }) t.Run(m.name+"/outcome not written", func(t *testing.T) { + t.Parallel() p := newPorts(t) sequence(p, nil, succeeded, errResolve) err := m.call(p) @@ -432,12 +441,14 @@ func TestMutationsFailClosed(t *testing.T) { }) t.Run(m.name+"/refused", func(t *testing.T) { + t.Parallel() p := newPorts(t) sequence(p, m.refusal, failed(m.want.Error()), nil) wantErr(t, m.call(p), m.want) }) t.Run(m.name+"/refused, outcome not written", func(t *testing.T) { + t.Parallel() p := newPorts(t) sequence(p, m.refusal, failed(m.want.Error()), errResolve) err := m.call(p) @@ -448,6 +459,7 @@ func TestMutationsFailClosed(t *testing.T) { }) t.Run(m.name+"/raw error", func(t *testing.T) { + t.Parallel() p := newPorts(t) sequence(p, errDB, internalFailure, nil) wantInternal(t, m.call(p), errDB) diff --git a/apps/admin/internal/services/users_test.go b/apps/admin/internal/services/users_test.go index 2bf2929..bc79b16 100644 --- a/apps/admin/internal/services/users_test.go +++ b/apps/admin/internal/services/users_test.go @@ -26,6 +26,7 @@ func userAudit(action string, details map[string]string) services.NewAuditEntry } func TestListUsers(t *testing.T) { + t.Parallel() svc, dir, _ := newUserService(t) in := validate.ListUsers{ Search: "ada", SearchField: "name", Role: "admin", Banned: ptr(true), @@ -53,6 +54,7 @@ func TestListUsers(t *testing.T) { } func TestGetUser(t *testing.T) { + t.Parallel() svc, dir, _ := newUserService(t) sessions := []services.Session{{ID: "s1"}} @@ -79,6 +81,7 @@ func TestGetUser(t *testing.T) { // failing change) of every mutation are in TestMutationsFailClosed. func TestBanUser(t *testing.T) { + t.Parallel() svc, dir, log := newUserService(t) detail := services.UserDetail{User: adaUser} @@ -116,6 +119,7 @@ func TestBanUser(t *testing.T) { } func TestUnbanUser(t *testing.T) { + t.Parallel() svc, dir, log := newUserService(t) banned := adaUser banned.Banned = true @@ -139,6 +143,7 @@ func TestUnbanUser(t *testing.T) { } func TestSetRole(t *testing.T) { + t.Parallel() svc, dir, log := newUserService(t) detail := services.UserDetail{User: adaUser} in := validate.SetUserRole{Role: "admin"} @@ -175,6 +180,7 @@ func TestSetRole(t *testing.T) { } func TestRevokeSession(t *testing.T) { + t.Parallel() svc, dir, log := newUserService(t) detail := services.UserDetail{User: adaUser} @@ -202,6 +208,7 @@ func TestRevokeSession(t *testing.T) { } func TestRevokeSessions(t *testing.T) { + t.Parallel() svc, dir, log := newUserService(t) gomock.InOrder( diff --git a/apps/admin/ui/crawl_test.go b/apps/admin/ui/crawl_test.go index 831ff0c..c09cbd0 100644 --- a/apps/admin/ui/crawl_test.go +++ b/apps/admin/ui/crawl_test.go @@ -127,6 +127,7 @@ var crawlForm = url.Values{ } func TestCrawlCoversEveryRoute(t *testing.T) { + t.Parallel() rec := &patternRecorder{} ctrl := gomock.NewController(t) ui.New(ui.Deps{ @@ -156,6 +157,7 @@ func TestCrawlCoversEveryRoute(t *testing.T) { var escapeHatch = regexp.MustCompile(`templ\.(Raw|Unsafe\w*|SafeScript\w*|SafeCSS\w*|JSUnsafeFuncCall|ComponentScript)\b`) func TestNoUnescapedOutput(t *testing.T) { + t.Parallel() err := filepath.WalkDir(".", func(path string, d fs.DirEntry, err error) error { if err != nil || d.IsDir() || strings.HasSuffix(path, "_templ.go") || strings.HasSuffix(path, "_test.go") || (!strings.HasSuffix(path, ".templ") && !strings.HasSuffix(path, ".go")) { @@ -176,17 +178,21 @@ func TestNoUnescapedOutput(t *testing.T) { } func TestCrawlExposesNothing(t *testing.T) { + t.Parallel() for _, failing := range []bool{false, true} { for _, signedIn := range []bool{false, true} { - h := crawlUI(t, failing, signedIn) for _, c := range crawlTargets { - method, _, _ := strings.Cut(c.pattern, " ") - for _, path := range c.paths { - for _, ds := range []bool{false, true} { - name := strings.Join([]string{method, path, state(failing, "failing", "ok"), state(signedIn, "signed-in", "anonymous"), state(ds, "datastar", "plain")}, " ") - checkResponse(t, name, method, path, crawl(h, method, path, ds)) + t.Run(strings.Join([]string{c.pattern, state(failing, "failing", "ok"), state(signedIn, "signed-in", "anonymous")}, " "), func(t *testing.T) { + t.Parallel() + cr := crawlUI(t, failing, signedIn) + method, _, _ := strings.Cut(c.pattern, " ") + for _, path := range c.paths { + for _, ds := range []bool{false, true} { + name := strings.Join([]string{method, path, state(ds, "datastar", "plain")}, " ") + checkResponse(t, name, method, path, cr.crawl(method, path, ds)) + } } - } + }) } } } @@ -199,9 +205,17 @@ func state(b bool, yes, no string) string { return no } +// crawler is one real UI handler and the method of the request it is +// serving. Each subtest has its own and crawls one request at a time. +type crawler struct { + h http.Handler + method string +} + // crawlUI is the real UI handler over mocks that answer anything: test data, // or (failing) internal errors carrying canary text. -func crawlUI(t *testing.T, failing, signedIn bool) http.Handler { +func crawlUI(t *testing.T, failing, signedIn bool) *crawler { + cr := &crawler{} ctrl := gomock.NewController(t) a := mocks.NewMockAuth(ctrl) us, ad, au, an := mocks.NewMockUsers(ctrl), mocks.NewMockAdmins(ctrl), mocks.NewMockAudit(ctrl), mocks.NewMockAnalytics(ctrl) @@ -218,37 +232,37 @@ func crawlUI(t *testing.T, failing, signedIn bool) http.Handler { detail := services.UserDetail{User: user, Sessions: []services.Session{{ID: "us1", CreatedAt: now, ExpiresAt: now.Add(time.Hour), IPAddress: &ip, UserAgent: &agent}}} audit := services.AuditPage{Total: 1, Limit: 50, Entries: []services.AuditEntry{{ID: 1, ActorID: "a1", ActorEmail: "root@example.com", Action: services.ActionUserBan, TargetType: "user", TargetID: "u1", TargetLabel: "ada " + xssProbe, Details: map[string]string{"reason": xssProbe}, CreatedAt: now}}} - a.EXPECT().SignIn(notOnGET(t, "SignIn"), gomock.Any(), gomock.Any()).Return(sess, fail).AnyTimes() - a.EXPECT().SignOut(notOnGET(t, "SignOut"), gomock.Any()).Return(fail).AnyTimes() - a.EXPECT().CompleteSetPassword(notOnGET(t, "CompleteSetPassword"), gomock.Any(), gomock.Any()).Return(sess, fail).AnyTimes() - a.EXPECT().VerifyTwoFactor(notOnGET(t, "VerifyTwoFactor"), gomock.Any()).Return(sess, "trust-tok", fail).AnyTimes() + a.EXPECT().SignIn(cr.notOnGET(t, "SignIn"), gomock.Any(), gomock.Any()).Return(sess, fail).AnyTimes() + a.EXPECT().SignOut(cr.notOnGET(t, "SignOut"), gomock.Any()).Return(fail).AnyTimes() + a.EXPECT().CompleteSetPassword(cr.notOnGET(t, "CompleteSetPassword"), gomock.Any(), gomock.Any()).Return(sess, fail).AnyTimes() + a.EXPECT().VerifyTwoFactor(cr.notOnGET(t, "VerifyTwoFactor"), gomock.Any()).Return(sess, "trust-tok", fail).AnyTimes() // StartTwoFactorSetup runs on GET /two-factor/setup on purpose: it needs // the HttpOnly challenge cookie a set-password or sign-in POST just set, // which a link scanner never has, and is not behind an emailed link. a.EXPECT().StartTwoFactorSetup(gomock.Any()).Return(setup, fail).AnyTimes() - a.EXPECT().EnableTwoFactor(notOnGET(t, "EnableTwoFactor"), gomock.Any(), gomock.Any()).Return(setup, fail).AnyTimes() - a.EXPECT().ConfirmTwoFactor(notOnGET(t, "ConfirmTwoFactor"), gomock.Any(), gomock.Any()).Return(fail).AnyTimes() - a.EXPECT().SetSessionCookie(notOnGET(t, "SetSessionCookie"), gomock.Any()).AnyTimes() - a.EXPECT().ClearSessionCookie(notOnGET(t, "ClearSessionCookie")).AnyTimes() - a.EXPECT().SetTwoFactorCookie(notOnGET(t, "SetTwoFactorCookie"), gomock.Any()).AnyTimes() - a.EXPECT().SetTrustDeviceCookie(notOnGET(t, "SetTrustDeviceCookie"), gomock.Any()).AnyTimes() + a.EXPECT().EnableTwoFactor(cr.notOnGET(t, "EnableTwoFactor"), gomock.Any(), gomock.Any()).Return(setup, fail).AnyTimes() + a.EXPECT().ConfirmTwoFactor(cr.notOnGET(t, "ConfirmTwoFactor"), gomock.Any(), gomock.Any()).Return(fail).AnyTimes() + a.EXPECT().SetSessionCookie(cr.notOnGET(t, "SetSessionCookie"), gomock.Any()).AnyTimes() + a.EXPECT().ClearSessionCookie(cr.notOnGET(t, "ClearSessionCookie")).AnyTimes() + a.EXPECT().SetTwoFactorCookie(cr.notOnGET(t, "SetTwoFactorCookie"), gomock.Any()).AnyTimes() + a.EXPECT().SetTrustDeviceCookie(cr.notOnGET(t, "SetTrustDeviceCookie"), gomock.Any()).AnyTimes() us.EXPECT().ListUsers(gomock.Any(), gomock.Any(), gomock.Any()).Return(services.UserPage{Users: []services.User{user}, Total: 1, Limit: 50}, fail).AnyTimes() us.EXPECT().GetUser(gomock.Any(), gomock.Any(), gomock.Any()).Return(detail, fail).AnyTimes() - us.EXPECT().BanUser(notOnGET(t, "BanUser"), gomock.Any(), gomock.Any(), gomock.Any()).Return(fail).AnyTimes() - us.EXPECT().UnbanUser(notOnGET(t, "UnbanUser"), gomock.Any(), gomock.Any()).Return(fail).AnyTimes() - us.EXPECT().SetRole(notOnGET(t, "SetRole"), gomock.Any(), gomock.Any(), gomock.Any()).Return(fail).AnyTimes() - us.EXPECT().RevokeSession(notOnGET(t, "RevokeSession"), gomock.Any(), gomock.Any(), gomock.Any()).Return(fail).AnyTimes() - us.EXPECT().RevokeSessions(notOnGET(t, "RevokeSessions"), gomock.Any(), gomock.Any()).Return(fail).AnyTimes() + us.EXPECT().BanUser(cr.notOnGET(t, "BanUser"), gomock.Any(), gomock.Any(), gomock.Any()).Return(fail).AnyTimes() + us.EXPECT().UnbanUser(cr.notOnGET(t, "UnbanUser"), gomock.Any(), gomock.Any()).Return(fail).AnyTimes() + us.EXPECT().SetRole(cr.notOnGET(t, "SetRole"), gomock.Any(), gomock.Any(), gomock.Any()).Return(fail).AnyTimes() + us.EXPECT().RevokeSession(cr.notOnGET(t, "RevokeSession"), gomock.Any(), gomock.Any(), gomock.Any()).Return(fail).AnyTimes() + us.EXPECT().RevokeSessions(cr.notOnGET(t, "RevokeSessions"), gomock.Any(), gomock.Any()).Return(fail).AnyTimes() ad.EXPECT().ListAdmins(gomock.Any(), gomock.Any()).Return([]services.Admin{{ID: "a1", Name: "Root " + xssProbe, Email: "root@example.com", CreatedAt: now, TwoFactorEnabled: true, HasPassword: true}}, fail).AnyTimes() - ad.EXPECT().InviteAdmin(notOnGET(t, "InviteAdmin"), gomock.Any(), gomock.Any()).Return(services.Admin{ID: "a2", Email: "new@example.com", CreatedAt: now}, fail).AnyTimes() - ad.EXPECT().DisableAdmin(notOnGET(t, "DisableAdmin"), gomock.Any(), gomock.Any()).Return(fail).AnyTimes() - ad.EXPECT().EnableAdmin(notOnGET(t, "EnableAdmin"), gomock.Any(), gomock.Any()).Return(fail).AnyTimes() - ad.EXPECT().RemoveAdmin(notOnGET(t, "RemoveAdmin"), gomock.Any(), gomock.Any()).Return(fail).AnyTimes() + ad.EXPECT().InviteAdmin(cr.notOnGET(t, "InviteAdmin"), gomock.Any(), gomock.Any()).Return(services.Admin{ID: "a2", Email: "new@example.com", CreatedAt: now}, fail).AnyTimes() + ad.EXPECT().DisableAdmin(cr.notOnGET(t, "DisableAdmin"), gomock.Any(), gomock.Any()).Return(fail).AnyTimes() + ad.EXPECT().EnableAdmin(cr.notOnGET(t, "EnableAdmin"), gomock.Any(), gomock.Any()).Return(fail).AnyTimes() + ad.EXPECT().RemoveAdmin(cr.notOnGET(t, "RemoveAdmin"), gomock.Any(), gomock.Any()).Return(fail).AnyTimes() au.EXPECT().List(gomock.Any(), gomock.Any(), gomock.Any()).Return(audit, fail).AnyTimes() an.EXPECT().Overview(gomock.Any(), gomock.Any()).Return(services.Overview{TotalUsers: 3, VerifiedUsers: 2, BannedUsers: 1, GeneratedAt: now}, fail).AnyTimes() - return ui.New(ui.Deps{ + cr.h = ui.New(ui.Deps{ Auth: a, Users: us, Admins: ad, Audit: au, Analytics: an, UserFrom: func(context.Context) (ui.Admin, ui.Session, bool) { return admin, sess, signedIn @@ -257,19 +271,16 @@ func crawlUI(t *testing.T, failing, signedIn bool) http.Handler { SecureCookies: true, Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), }).Handler() + return cr } -// crawlMethod is the method of the request being crawled (tests run the -// crawl sequentially). -var crawlMethod string - // notOnGET matches any argument but fails the test when a state-changing // port is called while a GET is served: mail scanners and link previews // fetch links, so opening a page (an emailed link above all) must never // spend a token, sign in or change anything. -func notOnGET(t *testing.T, port string) gomock.Matcher { +func (cr *crawler) notOnGET(t *testing.T, port string) gomock.Matcher { return gomock.Cond(func(any) bool { - if crawlMethod == http.MethodGet { + if cr.method == http.MethodGet { t.Errorf("GET called %s: only a POST may change state", port) } return true @@ -277,12 +288,12 @@ func notOnGET(t *testing.T, port string) gomock.Matcher { } // crawl sends what a browser would. Streams are cut after their first write. -func crawl(h http.Handler, method, path string, ds bool) *httptest.ResponseRecorder { +func (cr *crawler) crawl(method, path string, ds bool) *httptest.ResponseRecorder { var body io.Reader if method == http.MethodPost { body = strings.NewReader(crawlForm.Encode()) } - crawlMethod = method + cr.method = method ctx, cancel := context.WithCancel(context.Background()) defer cancel() r := httptest.NewRequestWithContext(ctx, method, "https://admin.example"+path, body) @@ -296,7 +307,7 @@ func crawl(h http.Handler, method, path string, ds bool) *httptest.ResponseRecor r.Header.Set("Datastar-Request", "true") } rec := &firstEvent{ResponseRecorder: httptest.NewRecorder(), cancel: cancel} - h.ServeHTTP(rec, r) + cr.h.ServeHTTP(rec, r) return rec.ResponseRecorder } @@ -318,14 +329,7 @@ func (f *firstEvent) Flush() { func checkResponse(t *testing.T, name, method, path string, rec *httptest.ResponseRecorder) { t.Helper() h, body := rec.Header(), rec.Body.String() - for _, re := range leaks { - if strings.HasPrefix(h.Get("Content-Type"), "text/javascript") && re.String() != "CANARY" && re.String() != `/api/` { - continue // the Datastar bundle is pinned (TestDatastarPin); only check it for secrets and API paths - } - if m := re.FindString(body); m != "" { - t.Errorf("%s: body contains %q (%s)", name, m, re) - } - } + checkBody(t, name, h.Get("Content-Type"), body) for k, vs := range h { for _, v := range vs { if strings.Contains(v, "CANARY") || strings.Contains(v, "/api/") { @@ -417,25 +421,57 @@ func checkResponse(t *testing.T, name, method, path string, rec *httptest.Respon } } + // Open-redirect probes never become a destination. + if loc := h.Get("Location"); strings.Contains(loc, evil) { + t.Errorf("%s: redirects to %s", name, loc) + } +} + +// checkedBodies holds the bodies (content type + body) that passed checkBody. +// Static assets come back byte-identical in every state and mode, and +// re-scanning them is most of the crawl's time under -race; a failing body +// is not stored, so it is reported for every response that carries it. +var checkedBodies sync.Map + +// checkBody runs the checks that depend only on the response body. +func checkBody(t *testing.T, name, contentType, body string) { + t.Helper() + key := contentType + "\x00" + body + if _, ok := checkedBodies.Load(key); ok { + return + } + ok := true + errorf := func(format string, args ...any) { + t.Helper() + ok = false + t.Errorf(format, args...) + } + for _, re := range leaks { + if strings.HasPrefix(contentType, "text/javascript") && re.String() != "CANARY" && re.String() != `/api/` { + continue // the Datastar bundle is pinned (TestDatastarPin); only check it for secrets and API paths + } + if m := re.FindString(body); m != "" { + errorf("%s: body contains %q (%s)", name, m, re) + } + } // Injected markup stays text, and never breaks out of a JS string in a // Datastar expression. if strings.Contains(body, `1234<", ">1000<", ">7<", ">55<", ">321<", "Jan 2, 2026 03:04 UTC", "placeholder") @@ -335,6 +343,7 @@ func TestAnalyticsPage(t *testing.T) { // --- sign-in flows --- func TestLogin(t *testing.T) { + t.Parallel() e := newEnv(t) form := url.Values{"email": {" root@example.com "}, "password": {"pw"}} @@ -354,6 +363,7 @@ func TestLogin(t *testing.T) { } func TestSignInChallenges(t *testing.T) { + t.Parallel() e := newEnv(t) form := url.Values{"email": {"root@example.com"}, "password": {"pw"}} second := &ui.TwoFactorChallenge{Token: "c1", Methods: []string{"totp"}} @@ -378,6 +388,7 @@ func TestSignInChallenges(t *testing.T) { } func TestSetPassword(t *testing.T) { + t.Parallel() e := newEnv(t) pw := func(token, p, c string) url.Values { return url.Values{"token": {token}, "password": {p}, "confirmPassword": {c}} @@ -399,6 +410,7 @@ func TestSetPassword(t *testing.T) { } func TestTwoFactor(t *testing.T) { + t.Parallel() e := newEnv(t) page := e.do("GET", config.AppRoutes.TwoFactor, nil).Body.String() for _, w := range []string{`autocomplete="one-time-code"`, `inputmode="numeric"`, `name="method" value="backup_code"`, `name="trustDevice"`, "data-indicator:_loading"} { @@ -436,6 +448,7 @@ const ( var totp = ui.TOTPSetup{Secret: "JBSWY3DPEHPK3PXP", URI: "otpauth://totp/Acme:root?secret=JBSWY3DPEHPK3PXP", BackupCodes: []string{"aaaa-1111", "bbbb-2222"}} func TestTwoFactorSetupSignedOut(t *testing.T) { + t.Parallel() e := newEnv(t) e.auth.EXPECT().StartTwoFactorSetup(gomock.Any()).Return(totp, nil) assertPage(t, e.do("GET", config.AppRoutes.TwoFactorSetup, nil), http.StatusOK, @@ -479,6 +492,7 @@ func TestTwoFactorSetupSignedOut(t *testing.T) { } func TestTwoFactorSetupSignedIn(t *testing.T) { + t.Parallel() e := newEnv(t, func(d *ui.Deps) { d.RequireTwoFactor = true }) noTwoFA := ui.Admin{ID: "a1", Email: "root@example.com"} noTwoFAActor := as(noTwoFA) @@ -503,6 +517,7 @@ func TestTwoFactorSetupSignedIn(t *testing.T) { } func TestLogout(t *testing.T) { + t.Parallel() e := newEnv(t) e.auth.EXPECT().SignOut(gomock.Any(), "tok-1").Return(nil) e.auth.EXPECT().ClearSessionCookie(gomock.Any()) @@ -550,8 +565,10 @@ func userActions() []userAction { } func TestUserActions(t *testing.T) { + t.Parallel() for _, a := range userActions() { t.Run(a.name, func(t *testing.T) { + t.Parallel() e := newEnv(t) // no session assertRedirectScript(t, e.do("POST", a.path, a.form, datastar), config.AppRoutes.Login) @@ -603,6 +620,7 @@ func TestUserActions(t *testing.T) { } func TestUserActionsInvalidInput(t *testing.T) { + t.Parallel() e := newEnv(t) cases := []struct { path string @@ -633,6 +651,7 @@ func TestUserActionsInvalidInput(t *testing.T) { // --- admins --- func TestInviteAdmin(t *testing.T) { + t.Parallel() e := newEnv(t) form := url.Values{"email": {" New@Example.com "}, "name": {"Nia"}} assertRedirectScript(t, e.do("POST", routes.AdminInvite, form, datastar), config.AppRoutes.Login) @@ -672,6 +691,7 @@ func TestInviteAdmin(t *testing.T) { } func TestAdminsPageRowActions(t *testing.T) { + t.Parallel() e := newEnv(t) e.admins.EXPECT().ListAdmins(gomock.Any(), actor).Return([]services.Admin{ {ID: "a1", Email: "root@example.com", HasPassword: true, TwoFactorEnabled: true}, @@ -720,12 +740,14 @@ func adminActions() []adminAction { } func TestAdminActions(t *testing.T) { + t.Parallel() refreshed := []services.Admin{ {ID: "a1", Email: "root@example.com", HasPassword: true}, {ID: "a2", Email: "other@example.com", HasPassword: true, Disabled: true}, } for _, a := range adminActions() { t.Run(a.name, func(t *testing.T) { + t.Parallel() e := newEnv(t) path := a.path("a2") // no session @@ -793,6 +815,7 @@ func TestAdminActions(t *testing.T) { } func TestAdminActionsRejectCrossOrigin(t *testing.T) { + t.Parallel() e := newEnv(t) for _, a := range adminActions() { rec := e.do("POST", a.path("a2"), url.Values{}, signedIn, func(r *http.Request) { r.Header.Set("Sec-Fetch-Site", "cross-site") }) @@ -803,6 +826,7 @@ func TestAdminActionsRejectCrossOrigin(t *testing.T) { } func TestAuditAdminActions(t *testing.T) { + t.Parallel() e := newEnv(t) entry := func(action string, d map[string]string) services.AuditEntry { return services.AuditEntry{Action: action, ActorEmail: "root@example.com", TargetType: services.TargetAdmin, TargetID: "a2", TargetLabel: "other@example.com", Details: d, CreatedAt: created} @@ -841,6 +865,7 @@ func TestAuditAdminActions(t *testing.T) { // unknown" with what that means; known failure reasons and the reconciler's // reasons are translated, anything else shows as recorded. func TestAuditStatuses(t *testing.T) { + t.Parallel() e := newEnv(t) entry := func(status, reason string, reconciled bool) services.AuditEntry { return services.AuditEntry{Action: services.ActionUserBan, ActorEmail: "root@example.com", TargetType: services.TargetUser, TargetID: "u1", TargetLabel: "ada@example.com", CreatedAt: created, Status: status, Reason: reason, Reconciled: reconciled} @@ -891,6 +916,7 @@ func TestAuditStatuses(t *testing.T) { // --- shell --- func TestLocaleSwitch(t *testing.T) { + t.Parallel() e := newEnv(t) rec := e.do("POST", routes.Locale, url.Values{"locale": {"ja"}, "next": {"/users?x=1"}}) assertSeeOther(t, rec, "/users?x=1") @@ -914,6 +940,7 @@ func TestLocaleSwitch(t *testing.T) { } func TestActionsRejectCrossOrigin(t *testing.T) { + t.Parallel() e := newEnv(t) rec := e.do("POST", config.AppRoutes.Login, url.Values{"email": {"root@example.com"}, "password": {"pw"}}, func(r *http.Request) { r.Header.Set("Sec-Fetch-Site", "cross-site") @@ -924,6 +951,7 @@ func TestActionsRejectCrossOrigin(t *testing.T) { } func TestNewRequiresDeps(t *testing.T) { + t.Parallel() defer func() { if recover() == nil { t.Fatal("New without UserFrom did not panic") @@ -934,6 +962,7 @@ func TestNewRequiresDeps(t *testing.T) { } func TestDefaultAppName(t *testing.T) { + t.Parallel() e := newEnv(t, func(d *ui.Deps) { d.AppName = ""; d.Logger = nil }) assertPage(t, e.do("GET", config.AppRoutes.Login, nil), http.StatusOK, "— Admin") } diff --git a/apps/admin/ui/i18n/i18n_test.go b/apps/admin/ui/i18n/i18n_test.go index 16792de..286781b 100644 --- a/apps/admin/ui/i18n/i18n_test.go +++ b/apps/admin/ui/i18n/i18n_test.go @@ -20,6 +20,7 @@ var keyLiteral = regexp.MustCompile(`"((?:common|components|pages|errors)\.[A-Za // TestUsedKeysExistInEveryLocale fails when a template or handler uses a key // that any locale is missing. func TestUsedKeysExistInEveryLocale(t *testing.T) { + t.Parallel() used := map[string][]string{} // key -> files root := ".." // apps/admin/ui err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { @@ -61,6 +62,7 @@ func TestUsedKeysExistInEveryLocale(t *testing.T) { // TestLocalesHaveSameKeys keeps every locale complete, not just the used keys. func TestLocalesHaveSameKeys(t *testing.T) { + t.Parallel() want := Keys(DefaultLocale) for _, l := range Locales { got := Keys(l) @@ -83,6 +85,7 @@ func TestLocalesHaveSameKeys(t *testing.T) { } func TestNegotiate(t *testing.T) { + t.Parallel() cases := []struct { name, cookie, accept string want Locale @@ -99,6 +102,7 @@ func TestNegotiate(t *testing.T) { } for _, c := range cases { t.Run(c.name, func(t *testing.T) { + t.Parallel() r := httptest.NewRequest(http.MethodGet, "/", nil) if c.accept != "" { r.Header.Set("Accept-Language", c.accept) @@ -114,12 +118,14 @@ func TestNegotiate(t *testing.T) { } func TestMissingKeyRendersKey(t *testing.T) { + t.Parallel() if got := (Translator{Locale: Ja}).T("pages.nope"); got != "pages.nope" { t.Fatalf("got %q", got) } } func TestTf(t *testing.T) { + t.Parallel() tr := Translator{Locale: Ja} if got := tr.Tf("pages.admins.inviteSent", "a@b.c"); got != "a@b.c に招待を送信しました" { t.Fatalf("ja: %q", got) @@ -132,6 +138,7 @@ func TestTf(t *testing.T) { // TestPlaceholdersMatch keeps the placeholders of a message the same in // every locale, so Tf fills them all. func TestPlaceholdersMatch(t *testing.T) { + t.Parallel() ph := regexp.MustCompile(`\{[0-9]\}`) for _, k := range Keys(DefaultLocale) { want := ph.FindAllString(Translator{Locale: DefaultLocale}.T(k), -1) @@ -147,6 +154,7 @@ func TestPlaceholdersMatch(t *testing.T) { } func TestDates(t *testing.T) { + t.Parallel() d := time.Date(2026, 3, 4, 5, 6, 0, 0, time.UTC) if got := (Translator{Locale: Ja}).DateTime(d); got != "2026年3月4日 05:06 UTC" { t.Errorf("ja: %q", got) diff --git a/apps/admin/ui/render_test.go b/apps/admin/ui/render_test.go index afaf8ef..fad5be9 100644 --- a/apps/admin/ui/render_test.go +++ b/apps/admin/ui/render_test.go @@ -114,11 +114,13 @@ func renderCases() []renderCase { // locale and checks the heading text, that no key or placeholder leaked // through, the html lang, and the signed-in header. func TestPagesRenderInEveryLocale(t *testing.T) { + t.Parallel() pages := renderCases() for _, l := range i18n.Locales { tr := i18n.Translator{Locale: l} for _, p := range pages { t.Run(string(l)+"/"+p.name, func(t *testing.T) { + t.Parallel() html := renderIn(t, l, view.State{Authenticated: p.authed, Email: "root@example.com", Section: view.SectionUsers}, p.c) if m := rawKey.FindString(html); m != "" { t.Errorf("untranslated key %q in output", m) @@ -149,6 +151,7 @@ func TestPagesRenderInEveryLocale(t *testing.T) { // TestFormsHaveLoadingIndicators keeps a Datastar indicator on every action // form (GET filter forms flip a signal on submit instead). func TestFormsHaveLoadingIndicators(t *testing.T) { + t.Parallel() at := time.Now() detail := services.UserDetail{User: services.User{ID: "u1", Banned: true}, Sessions: []services.Session{{ID: "s1", ExpiresAt: at}}} for _, c := range []templ.Component{ @@ -188,34 +191,54 @@ func (w *hangUpAfter) WriteString(s string) (int, error) { return len(s), nil } +// oneByteBuffers are templ buffers that pass each write straight to the +// connection, one per render case. A Buffer takes its size from the global +// templruntime.DefaultBufferSize when it is first Reset and keeps it, so they +// are made here, during package initialisation, before any test runs: +// setting the global inside a test would race with the parallel tests' +// renders. +var oneByteBuffers = func() []*templruntime.Buffer { + size := templruntime.DefaultBufferSize + templruntime.DefaultBufferSize = 1 + defer func() { templruntime.DefaultBufferSize = size }() + bufs := make([]*templruntime.Buffer, len(renderCases())) + for i := range bufs { + bufs[i] = &templruntime.Buffer{} + bufs[i].Reset(io.Discard) + } + return bufs +}() + // TestRenderStopsWhenClientHangsUp fails the connection at every write of // every page: Render must stop and return the write error (ui.render logs // it), never panic or swallow it. A 1-byte templ buffer makes each write // reach the connection. func TestRenderStopsWhenClientHangsUp(t *testing.T) { - size := templruntime.DefaultBufferSize - templruntime.DefaultBufferSize = 1 - defer func() { templruntime.DefaultBufferSize = size }() + t.Parallel() ctx := view.WithState(i18n.WithTranslator(context.Background(), i18n.Translator{Locale: i18n.En}), view.State{Authenticated: true}) - for _, p := range renderCases() { - for n := 0; ; n++ { - b := &templruntime.Buffer{} - b.Reset(&hangUpAfter{n: n}) - err := p.c.Render(ctx, b) - if err == nil { - break + for i, p := range renderCases() { + t.Run(p.name, func(t *testing.T) { + t.Parallel() + b := oneByteBuffers[i] + if b.Size() != 1 { + t.Fatalf("templ buffer of %d bytes, want 1", b.Size()) } - if !errors.Is(err, errHangUp) { - t.Fatalf("%s: write %d: %v", p.name, n, err) + for n := 0; ; n++ { + b.Reset(&hangUpAfter{n: n}) + err := p.c.Render(ctx, b) + if err == nil { + break + } + if !errors.Is(err, errHangUp) { + t.Fatalf("write %d: %v", n, err) + } } - } - } - canceled, cancel := context.WithCancel(ctx) - cancel() - for _, p := range renderCases() { - if err := p.c.Render(canceled, io.Discard); !errors.Is(err, context.Canceled) { - t.Errorf("%s rendered for a canceled request: %v", p.name, err) - } + canceled, cancel := context.WithCancel(ctx) + cancel() + if err := p.c.Render(canceled, io.Discard); !errors.Is(err, context.Canceled) { + t.Errorf("rendered for a canceled request: %v", err) + } + }) } } @@ -223,6 +246,7 @@ func TestRenderStopsWhenClientHangsUp(t *testing.T) { // tone and a translated label, the pending hint, and reconciled reasons // chosen by status rather than shown as recorded. func TestAuditStatus(t *testing.T) { + t.Parallel() tones := map[string]uikit.Tone{ services.StatusSucceeded: uikit.Success, services.StatusFailed: uikit.Error, services.StatusPending: uikit.Warning, services.StatusUnknown: uikit.Neutral, @@ -254,6 +278,7 @@ func TestAuditStatus(t *testing.T) { } func TestSection(t *testing.T) { + t.Parallel() for path, want := range map[string]string{ "/users": view.SectionUsers, "/users/u1": view.SectionUsers, "/admins": view.SectionAdmins, "/audit": view.SectionAudit, "/analytics": view.SectionAnalytics, "/login": "", "/usersx": "", @@ -270,6 +295,7 @@ var qrRun = regexp.MustCompile(`M(\d+) (\d+)h(\d+)v1h-(\d+)z`) // TestTOTPQR redraws the SVG path module by module and compares it with the // encoder's matrix, quiet zone included. func TestTOTPQR(t *testing.T) { + t.Parallel() uri := "otpauth://totp/Acme:root%40example.com?secret=JBSWY3DPEHPK3PXP&issuer=Acme" q, ok := totpQR(uri) if !ok { @@ -328,6 +354,7 @@ func TestTOTPQR(t *testing.T) { // Remove asks in a dialog that names the admin (escaped), while the form // itself posts straight to the remove action, so it works without JS. func TestRemoveAdminAsksInADialog(t *testing.T) { + t.Parallel() email := `o"brien's &co@example.com` html := renderIn(t, i18n.En, view.State{Authenticated: true}, AdminsPage(AdminsData{SelfID: "a1", Admins: []services.Admin{{ID: "a2", Email: email, HasPassword: true}}})) for _, want := range []string{ @@ -346,6 +373,7 @@ func TestRemoveAdminAsksInADialog(t *testing.T) { // A failure recorded without a service error is translated, never shown raw. func TestAuditReasonInternal(t *testing.T) { + t.Parallel() for _, l := range i18n.Locales { ctx := i18n.WithTranslator(context.Background(), i18n.Translator{Locale: l}) if got, want := auditReason(ctx, services.ReasonInternal), i18n.T(ctx, "pages.audit.internalReason"); got != want || got == services.ReasonInternal && l != i18n.En { @@ -357,6 +385,7 @@ func TestAuditReasonInternal(t *testing.T) { // The audit page says how its two kinds of entries are recorded: admin // changes fail closed, authentication events are best-effort. func TestAuditPageExplainsRecording(t *testing.T) { + t.Parallel() for _, l := range i18n.Locales { html := renderIn(t, l, view.State{Authenticated: true}, AuditPage(AuditData{})) if !strings.Contains(html, templ.EscapeString(i18n.Translator{Locale: l}.T("pages.audit.howRecorded"))) { diff --git a/apps/admin/ui/static/static_test.go b/apps/admin/ui/static/static_test.go index ac8a80a..1034f38 100644 --- a/apps/admin/ui/static/static_test.go +++ b/apps/admin/ui/static/static_test.go @@ -10,6 +10,7 @@ import ( ) func TestDatastarPin(t *testing.T) { + t.Parallel() b, err := FS.ReadFile(DatastarFile) if err != nil { t.Fatal(err) @@ -24,6 +25,7 @@ func TestDatastarPin(t *testing.T) { } func TestHandlerCaching(t *testing.T) { + t.Parallel() h := Handler() rec := httptest.NewRecorder() h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, URL(CSSFile), nil)) diff --git a/apps/web/internal/api/users_test.go b/apps/web/internal/api/users_test.go index f4faefe..a9fb0c8 100644 --- a/apps/web/internal/api/users_test.go +++ b/apps/web/internal/api/users_test.go @@ -149,17 +149,21 @@ func wantError(t *testing.T, res apitest.Response, status int, msg string) { // TestUserRoutes runs the four boundary cases against every endpoint. The // mock fails the test on any service call a case does not expect. func TestUserRoutes(t *testing.T) { + t.Parallel() for name, rt := range routes { t.Run(name+"/no session is 401", func(t *testing.T) { + t.Parallel() _, h := newRouter(t) wantError(t, apitest.Do(t, h, apitest.Anonymous, rt.method, rt.path, rt.body), 401, "Unauthorized") }) t.Run(name+"/other user is 403", func(t *testing.T) { + t.Parallel() svc, h := newRouter(t) rt.expect(svc.EXPECT(), services.ErrForbidden) wantError(t, apitest.Do(t, h, apitest.AsUser(ada), rt.method, rt.path, rt.body), 403, "Forbidden") }) t.Run(name+"/invalid input is 400", func(t *testing.T) { + t.Parallel() svc, h := newRouter(t) body := rt.invalid if body == "" { // no body to get wrong: the service rejects the request @@ -172,6 +176,7 @@ func TestUserRoutes(t *testing.T) { } }) t.Run(name+"/ok is 200", func(t *testing.T) { + t.Parallel() svc, h := newRouter(t) rt.expect(svc.EXPECT(), nil) res := apitest.Do(t, h, apitest.AsUser(ada), rt.method, rt.path, rt.body) @@ -188,6 +193,7 @@ func TestUserRoutes(t *testing.T) { // TestErrorMapping covers every service error kind once; it is the same // route wrapper for every endpoint. func TestErrorMapping(t *testing.T) { + t.Parallel() cases := []struct { err error status int @@ -212,6 +218,7 @@ func TestErrorMapping(t *testing.T) { // that use them: invalid bodies never reach the service (the mock has no // expectations) and name the failing field. func TestDecode(t *testing.T) { + t.Parallel() cases := []struct { method, path, body, field string }{ @@ -238,6 +245,7 @@ func TestDecode(t *testing.T) { // TestDecodeNamesJSONTypes: a wrong type is described in JSON terms, never // with the Go type it decodes into. func TestDecodeNamesJSONTypes(t *testing.T) { + t.Parallel() for body, want := range map[string]string{ `{"name":1}`: "name must be a string", `{"name":["x"]}`: "name must be a string", @@ -254,6 +262,7 @@ func TestDecodeNamesJSONTypes(t *testing.T) { // TestEmailCallbackDefault: without a callbackURL the link lands on the // account page. func TestEmailCallbackDefault(t *testing.T) { + t.Parallel() svc, h := newRouter(t) svc.EXPECT().RequestEmailChange(anyCtx, actor, "u1", validate.UpdateUserEmail{NewEmail: "ada@new.example", CallbackURL: config.AppRoutes.EmailChanged}) if res := apitest.Do(t, h, apitest.AsUser(ada), "POST", r.MeEmail, `{"newEmail":"ada@new.example"}`); res.Status != 200 { @@ -263,6 +272,7 @@ func TestEmailCallbackDefault(t *testing.T) { // TestBodyLimit: the route wrapper caps request bodies. func TestBodyLimit(t *testing.T) { + t.Parallel() _, h := newRouter(t) res := apitest.Do(t, h, apitest.AsUser(ada), "PATCH", r.Me, `{"name":"`+strings.Repeat("x", 2<<20)+`"}`) if res.Status != 400 || res.Details["body"] == "" { diff --git a/apps/web/internal/config/config_test.go b/apps/web/internal/config/config_test.go index 445d457..78fec7f 100644 --- a/apps/web/internal/config/config_test.go +++ b/apps/web/internal/config/config_test.go @@ -17,6 +17,7 @@ func env(kv map[string]string) func(string) string { } func TestLoadDevelopmentDefaults(t *testing.T) { + t.Parallel() cfg, err := Load(env(map[string]string{ "PUBLIC_URL": "http://dev-box:3000/", "DATABASE_URL": "postgres://x", @@ -40,6 +41,7 @@ func TestLoadDevelopmentDefaults(t *testing.T) { } func TestLoadProductionRequiresProviders(t *testing.T) { + t.Parallel() _, err := Load(env(map[string]string{"APP_ENV": "production", "PUBLIC_URL": "https://app.example.com/path"})) if err == nil { t.Fatal("want error") @@ -52,6 +54,7 @@ func TestLoadProductionRequiresProviders(t *testing.T) { } func TestProductionFeatures(t *testing.T) { + t.Parallel() f := FeaturesFor(Production) if !f.EmailVerification || f.MobileDevOrigins { t.Fatalf("production features = %+v", f) @@ -59,6 +62,7 @@ func TestProductionFeatures(t *testing.T) { } func TestLoadSecretsProvider(t *testing.T) { + t.Parallel() base := map[string]string{"PUBLIC_URL": "http://dev-box:3000", "DATABASE_URL": "postgres://x", "AUTH_SECRET": strings.Repeat("s", 32)} with := func(kv map[string]string) map[string]string { m := map[string]string{} @@ -117,6 +121,7 @@ func TestLoadSecretsProvider(t *testing.T) { } func TestLoadLiveDatabaseAndAuthSecrets(t *testing.T) { + t.Parallel() a, b := strings.Repeat("a", 32), strings.Repeat("b", 32) cfg, err := Load(env(map[string]string{ "PUBLIC_URL": "http://dev-box:3000", "DATABASE_URL": "postgres://x", @@ -172,6 +177,7 @@ func TestLoadLiveDatabaseAndAuthSecrets(t *testing.T) { // logging it (or a struct holding it) can never leak them, before or after // the secrets are loaded. func TestConfigNeverPrintsSecrets(t *testing.T) { + t.Parallel() values := []string{"dbpass-4242", "auth-secret-" + strings.Repeat("z", 32), "re_resend_9999", "dp.st.doppler_7777"} kv := map[string]string{ "PUBLIC_URL": "http://dev-box:3000", diff --git a/apps/web/internal/server/server_test.go b/apps/web/internal/server/server_test.go index bcdd48c..a9ef584 100644 --- a/apps/web/internal/server/server_test.go +++ b/apps/web/internal/server/server_test.go @@ -65,6 +65,7 @@ func serve(h http.Handler, req *http.Request) *httptest.ResponseRecorder { } func TestHealthz(t *testing.T) { + t.Parallel() ok := serve(newHandler(pinger{}, nil), httptest.NewRequest(http.MethodGet, "/healthz", nil)) down := serve(newHandler(pinger{errors.New("down")}, nil), httptest.NewRequest(http.MethodGet, "/healthz", nil)) fresh := serve(newHandlerWith(pinger{}, secretsHealth{}, nil), httptest.NewRequest(http.MethodGet, "/healthz", nil)) @@ -78,6 +79,7 @@ func TestHealthz(t *testing.T) { } func TestMountsAndMiddleware(t *testing.T) { + t.Parallel() ui := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path == "/boom" { panic("boom: /home/dev/secret.go:12") @@ -114,6 +116,7 @@ func TestMountsAndMiddleware(t *testing.T) { // could send there (fetch, form post, preflight, typed-in URL) gets the bare // 404 of an unknown path, with no CORS headers. func TestAPIIsHiddenFromBrowsers(t *testing.T) { + t.Parallel() h := newHandler(pinger{}, http.NotFoundHandler()) browser := map[string]func(*http.Request){ "same-origin fetch": func(r *http.Request) { @@ -208,6 +211,7 @@ func TestAPIIsHiddenFromBrowsers(t *testing.T) { // Emailed reset links carry a credential in the path; logs keep the route only. func TestRequestLogsDropTokens(t *testing.T) { + t.Parallel() var logs strings.Builder h := New(Deps{ Log: slog.New(slog.NewTextHandler(&logs, nil)), DB: pinger{}, Auth: noAuth{}, diff --git a/apps/web/internal/services/users_test.go b/apps/web/internal/services/users_test.go index c9370ef..4a13050 100644 --- a/apps/web/internal/services/users_test.go +++ b/apps/web/internal/services/users_test.go @@ -48,34 +48,44 @@ func wantInternal(t *testing.T, err, cause error) { // Every method refuses to act on another user before touching a port: the // mocks have no expectations, so any call fails the test. func TestUserServiceForbidsOtherUsers(t *testing.T) { - svc, _, _ := newService(t) - calls := map[string]func(services.Actor, string) error{ - "GetProfile": func(a services.Actor, id string) error { _, err := svc.GetProfile(ctx, a, id); return err }, - "UpdateProfile": func(a services.Actor, id string) error { + t.Parallel() + type service = *services.UserService + calls := map[string]func(service, services.Actor, string) error{ + "GetProfile": func(svc service, a services.Actor, id string) error { _, err := svc.GetProfile(ctx, a, id); return err }, + "UpdateProfile": func(svc service, a services.Actor, id string) error { _, err := svc.UpdateProfile(ctx, a, id, validate.UpdateUserSettings{Name: ptr("Eve")}) return err }, - "RequestEmailChange": func(a services.Actor, id string) error { + "RequestEmailChange": func(svc service, a services.Actor, id string) error { return svc.RequestEmailChange(ctx, a, id, validate.UpdateUserEmail{NewEmail: "x@example.com"}) }, - "ChangePassword": func(a services.Actor, id string) error { + "ChangePassword": func(svc service, a services.Actor, id string) error { return svc.ChangePassword(ctx, a, id, validate.UpdateUserPassword{CurrentPassword: "password123", NewPassword: "password456"}) }, - "ListSessions": func(a services.Actor, id string) error { _, err := svc.ListSessions(ctx, a, id); return err }, - "RevokeSession": func(a services.Actor, id string) error { return svc.RevokeSession(ctx, a, id, "s9") }, - "RevokeOtherSessions": func(a services.Actor, id string) error { return svc.RevokeOtherSessions(ctx, a, id) }, - "DeleteAccount": func(a services.Actor, id string) error { return svc.DeleteAccount(ctx, a, id) }, - "DataSummary": func(a services.Actor, id string) error { _, err := svc.DataSummary(ctx, a, id); return err }, + "ListSessions": func(svc service, a services.Actor, id string) error { + _, err := svc.ListSessions(ctx, a, id) + return err + }, + "RevokeSession": func(svc service, a services.Actor, id string) error { return svc.RevokeSession(ctx, a, id, "s9") }, + "RevokeOtherSessions": func(svc service, a services.Actor, id string) error { return svc.RevokeOtherSessions(ctx, a, id) }, + "DeleteAccount": func(svc service, a services.Actor, id string) error { return svc.DeleteAccount(ctx, a, id) }, + "DataSummary": func(svc service, a services.Actor, id string) error { + _, err := svc.DataSummary(ctx, a, id) + return err + }, } for name, call := range calls { t.Run(name, func(t *testing.T) { - wantErr(t, call(ada, "u2"), services.ErrForbidden) - wantErr(t, call(services.Actor{}, ""), services.ErrForbidden) // no actor is nobody + t.Parallel() + svc, _, _ := newService(t) // its own mocks: an unexpected call fails this subtest + wantErr(t, call(svc, ada, "u2"), services.ErrForbidden) + wantErr(t, call(svc, services.Actor{}, ""), services.ErrForbidden) // no actor is nobody }) } } func TestGetProfile(t *testing.T) { + t.Parallel() svc, repo, _ := newService(t) repo.EXPECT().GetByID(ctx, "u1").Return(adaUser, nil) if u, err := svc.GetProfile(ctx, ada, "u1"); err != nil || u != adaUser { @@ -92,6 +102,7 @@ func TestGetProfile(t *testing.T) { } func TestUpdateProfile(t *testing.T) { + t.Parallel() svc, repo, _ := newService(t) renamed := adaUser renamed.Name = "Ada L" @@ -117,6 +128,7 @@ func TestUpdateProfile(t *testing.T) { } func TestRequestEmailChange(t *testing.T) { + t.Parallel() svc, _, creds := newService(t) in := validate.UpdateUserEmail{NewEmail: "ada@new.example", CallbackURL: "/dashboard"} @@ -139,6 +151,7 @@ func TestRequestEmailChange(t *testing.T) { } func TestChangePassword(t *testing.T) { + t.Parallel() svc, _, creds := newService(t) in := validate.UpdateUserPassword{CurrentPassword: "password123", NewPassword: "password456", RevokeOtherSessions: true} @@ -171,6 +184,7 @@ func TestChangePassword(t *testing.T) { } func TestListSessions(t *testing.T) { + t.Parallel() svc, _, creds := newService(t) creds.EXPECT().ListSessions(ctx, "u1").Return([]services.Session{{ID: "s1"}, {ID: "s2"}}, nil) @@ -194,6 +208,7 @@ func TestListSessions(t *testing.T) { } func TestRevokeSession(t *testing.T) { + t.Parallel() svc, _, creds := newService(t) creds.EXPECT().RevokeSession(ctx, "u1", "s2").Return(nil) @@ -211,6 +226,7 @@ func TestRevokeSession(t *testing.T) { } func TestRevokeOtherSessions(t *testing.T) { + t.Parallel() svc, _, creds := newService(t) creds.EXPECT().RevokeOtherSessions(ctx, "u1", "s1").Return(nil) @@ -226,6 +242,7 @@ func TestRevokeOtherSessions(t *testing.T) { } func TestDeleteAccount(t *testing.T) { + t.Parallel() svc, repo, _ := newService(t) repo.EXPECT().Delete(ctx, "u1").Return(nil) @@ -241,6 +258,7 @@ func TestDeleteAccount(t *testing.T) { } func TestDataSummary(t *testing.T) { + t.Parallel() svc, repo, _ := newService(t) want := services.UserDataSummary{User: adaUser, Sessions: 2} @@ -260,6 +278,7 @@ func TestDataSummary(t *testing.T) { // Kind sentinels match every error of their kind; specific errors only themselves. func TestErrorKinds(t *testing.T) { + t.Parallel() cases := []struct { err, target error want bool diff --git a/apps/web/ui/account_test.go b/apps/web/ui/account_test.go index bedf85d..ee8e7b8 100644 --- a/apps/web/ui/account_test.go +++ b/apps/web/ui/account_test.go @@ -44,10 +44,13 @@ var accountActions = []struct { } func TestAccountAnonymous(t *testing.T) { + t.Parallel() e := newEnv(t, config.Features{}) assertSeeOther(t, e.do(http.MethodGet, config.AppRoutes.Account, nil), config.AppRoutes.Login) for _, a := range accountActions { t.Run(a.name, func(t *testing.T) { + t.Parallel() + e := newEnv(t, config.Features{}) assertRedirectScript(t, e.do(http.MethodPost, a.path, a.form, datastar), config.AppRoutes.Login) assertSeeOther(t, e.do(http.MethodPost, a.path, a.form), config.AppRoutes.Login) }) @@ -55,6 +58,7 @@ func TestAccountAnonymous(t *testing.T) { } func TestAccountPage(t *testing.T) { + t.Parallel() e := newEnv(t, config.Features{}) e.signedIn("sess-1") e.users.EXPECT().ListSessions(gomock.Any(), demoActor, "u1").Return(demoSessions, nil) @@ -91,9 +95,11 @@ func TestAccountPage(t *testing.T) { } func TestAccountHappyPaths(t *testing.T) { + t.Parallel() img := "https://example.com/a.png" t.Run("profile", func(t *testing.T) { + t.Parallel() e := newEnv(t, config.Features{}) e.signedIn("sess-1") e.users.EXPECT().UpdateProfile(gomock.Any(), demoActor, "u1", gomock.Cond(func(in validate.UpdateUserSettings) bool { @@ -110,6 +116,7 @@ func TestAccountHappyPaths(t *testing.T) { }) t.Run("email", func(t *testing.T) { + t.Parallel() e := newEnv(t, config.Features{}) e.signedIn("sess-1") e.users.EXPECT().RequestEmailChange(gomock.Any(), demoActor, "u1", validate.UpdateUserEmail{NewEmail: "ada@example.com", CallbackURL: config.AppRoutes.EmailChanged}).Return(nil) @@ -118,6 +125,7 @@ func TestAccountHappyPaths(t *testing.T) { }) t.Run("password", func(t *testing.T) { + t.Parallel() e := newEnv(t, config.Features{}) e.signedIn("sess-1") e.users.EXPECT().ChangePassword(gomock.Any(), demoActor, "u1", validate.UpdateUserPassword{CurrentPassword: "old-password", NewPassword: "new-password", RevokeOtherSessions: true}).Return(nil) @@ -132,6 +140,7 @@ func TestAccountHappyPaths(t *testing.T) { }) t.Run("sessions", func(t *testing.T) { + t.Parallel() e := newEnv(t, config.Features{}) e.signedIn("sess-1") e.users.EXPECT().RevokeSession(gomock.Any(), demoActor, "u1", "sess-2").Return(nil) @@ -147,6 +156,7 @@ func TestAccountHappyPaths(t *testing.T) { }) t.Run("delete", func(t *testing.T) { + t.Parallel() e := newEnv(t, config.Features{}) e.signedIn("sess-1") e.users.EXPECT().DeleteAccount(gomock.Any(), demoActor, "u1").Return(nil) @@ -162,6 +172,7 @@ func TestAccountHappyPaths(t *testing.T) { }) t.Run("without JS", func(t *testing.T) { + t.Parallel() e := newEnv(t, config.Features{}) e.signedIn("sess-1") e.users.EXPECT().UpdateProfile(gomock.Any(), demoActor, "u1", gomock.Any()).Return(services.User{ID: "u1", Name: "Ada"}, nil) @@ -172,6 +183,7 @@ func TestAccountHappyPaths(t *testing.T) { // TestAccountErrors renders every named service error and kind translated, // through the revoke-others action. func TestAccountErrors(t *testing.T) { + t.Parallel() cases := []struct { err error key string @@ -196,6 +208,7 @@ func TestAccountErrors(t *testing.T) { } for _, c := range cases { t.Run(c.err.Error(), func(t *testing.T) { + t.Parallel() e := newEnv(t, config.Features{}) e.signedIn("sess-1") e.users.EXPECT().RevokeOtherSessions(gomock.Any(), demoActor, "u1").Return(c.err) @@ -225,6 +238,7 @@ func TestAccountErrors(t *testing.T) { } for _, f := range forms { t.Run(f.name, func(t *testing.T) { + t.Parallel() e := newEnv(t, config.Features{}) e.signedIn("sess-1") f.expect(e) @@ -246,6 +260,7 @@ func TestAccountErrors(t *testing.T) { } for _, v := range validation { t.Run("invalid "+v.name, func(t *testing.T) { + t.Parallel() e := newEnv(t, config.Features{}) e.signedIn("sess-1") assertSSE(t, e.do(http.MethodPost, v.path, v.form, datastar, lang("ja")), ja(v.key)) @@ -283,6 +298,7 @@ func (s *sseReader) next() string { // first patch, a profile save from the Account page (account card + notice), // and the stream signing its tab out when another device revokes it. func TestLiveStream(t *testing.T) { + t.Parallel() e := newEnv(t, config.Features{}) srv := httptest.NewServer(e.h) defer srv.Close() diff --git a/apps/web/ui/crawl_test.go b/apps/web/ui/crawl_test.go index ac6ef86..c6f0f4a 100644 --- a/apps/web/ui/crawl_test.go +++ b/apps/web/ui/crawl_test.go @@ -141,6 +141,7 @@ var crawlForm = url.Values{ } func TestCrawlCoversEveryRoute(t *testing.T) { + t.Parallel() rec := &patternRecorder{} ui.New(ui.Deps{Auth: mocks.NewMockAuth(gomock.NewController(t)), Users: mocks.NewMockUsers(gomock.NewController(t))}).Routes(rec) var crawled []string @@ -166,6 +167,7 @@ func TestCrawlCoversEveryRoute(t *testing.T) { var escapeHatch = regexp.MustCompile(`templ\.(Raw|Unsafe\w*|SafeScript\w*|SafeCSS\w*|JSUnsafeFuncCall|ComponentScript)\b`) func TestNoUnescapedOutput(t *testing.T) { + t.Parallel() err := filepath.WalkDir(".", func(path string, d fs.DirEntry, err error) error { if err != nil || d.IsDir() || strings.HasSuffix(path, "_templ.go") || strings.HasSuffix(path, "_test.go") || (!strings.HasSuffix(path, ".templ") && !strings.HasSuffix(path, ".go")) { @@ -186,17 +188,23 @@ func TestNoUnescapedOutput(t *testing.T) { } func TestCrawlExposesNothing(t *testing.T) { + t.Parallel() for _, failing := range []bool{false, true} { for _, signedIn := range []bool{false, true} { - h := crawlUI(t, failing, signedIn) for _, c := range crawlTargets { method, _, _ := strings.Cut(c.pattern, " ") - for _, path := range c.paths { - for _, ds := range []bool{false, true} { - name := strings.Join([]string{method, path, state(failing, "failing", "ok"), state(signedIn, "signed-in", "anonymous"), state(ds, "datastar", "plain")}, " ") - checkResponse(t, name, method, path, crawl(h, method, path, ds)) + t.Run(state(failing, "failing", "ok")+"/"+state(signedIn, "signed-in", "anonymous")+"/"+c.pattern, func(t *testing.T) { + t.Parallel() + // One handler (and mock controller) per subtest, built for + // the one method it serves: notOnGET needs no shared state. + h := crawlUI(t, method, failing, signedIn) + for _, path := range c.paths { + for _, ds := range []bool{false, true} { + name := strings.Join([]string{method, path, state(failing, "failing", "ok"), state(signedIn, "signed-in", "anonymous"), state(ds, "datastar", "plain")}, " ") + checkResponse(t, name, method, path, crawl(h, method, path, ds)) + } } - } + }) } } } @@ -210,8 +218,9 @@ func state(b bool, yes, no string) string { } // crawlUI is the real UI handler over mocks that answer anything: test data, -// or (failing) internal errors carrying canary text. -func crawlUI(t *testing.T, failing, signedIn bool) http.Handler { +// or (failing) internal errors carrying canary text. It serves only requests +// with method, which notOnGET checks state-changing port calls against. +func crawlUI(t *testing.T, method string, failing, signedIn bool) http.Handler { ctrl := gomock.NewController(t) a, u := mocks.NewMockAuth(ctrl), mocks.NewMockUsers(ctrl) var fail error @@ -226,30 +235,30 @@ func crawlUI(t *testing.T, failing, signedIn bool) http.Handler { {ID: "sess-2", CreatedAt: time.Now(), ExpiresAt: time.Now().Add(time.Hour), IPAddress: &ip, UserAgent: &agent}, } - a.EXPECT().SignUpEmail(notOnGET(t, "SignUpEmail"), gomock.Any()).Return(user, sess, fail).AnyTimes() - a.EXPECT().SignInEmail(notOnGET(t, "SignInEmail"), gomock.Any()).Return(user, sess, fail).AnyTimes() - a.EXPECT().SignOut(notOnGET(t, "SignOut"), gomock.Any()).Return(fail).AnyTimes() - a.EXPECT().RequestPasswordReset(notOnGET(t, "RequestPasswordReset"), gomock.Any(), gomock.Any()).Return(fail).AnyTimes() - a.EXPECT().ResetPassword(notOnGET(t, "ResetPassword"), gomock.Any(), gomock.Any()).Return(fail).AnyTimes() - a.EXPECT().VerifyEmail(notOnGET(t, "VerifyEmail"), gomock.Any()).Return(user, sess, fail).AnyTimes() - a.EXPECT().SendVerificationEmail(notOnGET(t, "SendVerificationEmail"), gomock.Any(), gomock.Any()).Return(fail).AnyTimes() + a.EXPECT().SignUpEmail(notOnGET(t, method, "SignUpEmail"), gomock.Any()).Return(user, sess, fail).AnyTimes() + a.EXPECT().SignInEmail(notOnGET(t, method, "SignInEmail"), gomock.Any()).Return(user, sess, fail).AnyTimes() + a.EXPECT().SignOut(notOnGET(t, method, "SignOut"), gomock.Any()).Return(fail).AnyTimes() + a.EXPECT().RequestPasswordReset(notOnGET(t, method, "RequestPasswordReset"), gomock.Any(), gomock.Any()).Return(fail).AnyTimes() + a.EXPECT().ResetPassword(notOnGET(t, method, "ResetPassword"), gomock.Any(), gomock.Any()).Return(fail).AnyTimes() + a.EXPECT().VerifyEmail(notOnGET(t, method, "VerifyEmail"), gomock.Any()).Return(user, sess, fail).AnyTimes() + a.EXPECT().SendVerificationEmail(notOnGET(t, method, "SendVerificationEmail"), gomock.Any(), gomock.Any()).Return(fail).AnyTimes() a.EXPECT().CheckCallbackURL(gomock.Any()).DoAndReturn(func(cb string) error { if cb == "betterstack://verified" { // the mobile app's deep link return nil } return errors.New("untrusted callback") }).AnyTimes() - a.EXPECT().SetSessionCookie(notOnGET(t, "SetSessionCookie"), gomock.Any()).AnyTimes() - a.EXPECT().ClearSessionCookie(notOnGET(t, "ClearSessionCookie")).AnyTimes() + a.EXPECT().SetSessionCookie(notOnGET(t, method, "SetSessionCookie"), gomock.Any()).AnyTimes() + a.EXPECT().ClearSessionCookie(notOnGET(t, method, "ClearSessionCookie")).AnyTimes() u.EXPECT().GetProfile(gomock.Any(), gomock.Any(), gomock.Any()).Return(services.User(user), fail).AnyTimes() - u.EXPECT().UpdateProfile(notOnGET(t, "UpdateProfile"), gomock.Any(), gomock.Any(), gomock.Any()).Return(services.User(user), fail).AnyTimes() - u.EXPECT().RequestEmailChange(notOnGET(t, "RequestEmailChange"), gomock.Any(), gomock.Any(), gomock.Any()).Return(fail).AnyTimes() - u.EXPECT().ChangePassword(notOnGET(t, "ChangePassword"), gomock.Any(), gomock.Any(), gomock.Any()).Return(fail).AnyTimes() + u.EXPECT().UpdateProfile(notOnGET(t, method, "UpdateProfile"), gomock.Any(), gomock.Any(), gomock.Any()).Return(services.User(user), fail).AnyTimes() + u.EXPECT().RequestEmailChange(notOnGET(t, method, "RequestEmailChange"), gomock.Any(), gomock.Any(), gomock.Any()).Return(fail).AnyTimes() + u.EXPECT().ChangePassword(notOnGET(t, method, "ChangePassword"), gomock.Any(), gomock.Any(), gomock.Any()).Return(fail).AnyTimes() u.EXPECT().ListSessions(gomock.Any(), gomock.Any(), gomock.Any()).Return(sessions, fail).AnyTimes() - u.EXPECT().RevokeSession(notOnGET(t, "RevokeSession"), gomock.Any(), gomock.Any(), gomock.Any()).Return(fail).AnyTimes() - u.EXPECT().RevokeOtherSessions(notOnGET(t, "RevokeOtherSessions"), gomock.Any(), gomock.Any()).Return(fail).AnyTimes() - u.EXPECT().DeleteAccount(notOnGET(t, "DeleteAccount"), gomock.Any(), gomock.Any()).Return(fail).AnyTimes() + u.EXPECT().RevokeSession(notOnGET(t, method, "RevokeSession"), gomock.Any(), gomock.Any(), gomock.Any()).Return(fail).AnyTimes() + u.EXPECT().RevokeOtherSessions(notOnGET(t, method, "RevokeOtherSessions"), gomock.Any(), gomock.Any()).Return(fail).AnyTimes() + u.EXPECT().DeleteAccount(notOnGET(t, method, "DeleteAccount"), gomock.Any(), gomock.Any()).Return(fail).AnyTimes() u.EXPECT().DataSummary(gomock.Any(), gomock.Any(), gomock.Any()).Return(services.UserDataSummary{User: services.User(user), Sessions: 2}, fail).AnyTimes() return ui.New(ui.Deps{ @@ -264,17 +273,14 @@ func crawlUI(t *testing.T, failing, signedIn bool) http.Handler { }).Handler() } -// crawlMethod is the method of the request being crawled (tests run the -// crawl sequentially). -var crawlMethod string - // notOnGET matches any argument but fails the test when a state-changing -// port is called while a GET is served: mail scanners and link previews -// fetch links, so opening a page (an emailed link above all) must never -// spend a token, sign in or change anything. -func notOnGET(t *testing.T, port string) gomock.Matcher { +// port is called while a GET is served (method is the one method the +// handler under test serves): mail scanners and link previews fetch links, +// so opening a page (an emailed link above all) must never spend a token, +// sign in or change anything. +func notOnGET(t *testing.T, method, port string) gomock.Matcher { return gomock.Cond(func(any) bool { - if crawlMethod == http.MethodGet { + if method == http.MethodGet { t.Errorf("GET called %s: only a POST may change state", port) } return true @@ -287,7 +293,6 @@ func crawl(h http.Handler, method, path string, ds bool) *httptest.ResponseRecor if method == http.MethodPost { body = strings.NewReader(crawlForm.Encode()) } - crawlMethod = method ctx, cancel := context.WithCancel(context.Background()) defer cancel() r := httptest.NewRequestWithContext(ctx, method, "https://app.example"+path, body) @@ -323,14 +328,7 @@ func (f *firstEvent) Flush() { func checkResponse(t *testing.T, name, method, path string, rec *httptest.ResponseRecorder) { t.Helper() h, body := rec.Header(), rec.Body.String() - for _, re := range leaks { - if strings.HasPrefix(h.Get("Content-Type"), "text/javascript") && re.String() != "CANARY" && re.String() != `/api/` { - continue // the Datastar bundle is pinned (TestDatastarPin); only check it for secrets and API paths - } - if m := re.FindString(body); m != "" { - t.Errorf("%s: body contains %q (%s)", name, m, re) - } - } + checkBody(t, name, h.Get("Content-Type"), body) for k, vs := range h { for _, v := range vs { if strings.Contains(v, "CANARY") || strings.Contains(v, "/api/") { @@ -422,25 +420,57 @@ func checkResponse(t *testing.T, name, method, path string, rec *httptest.Respon } } + // Open-redirect probes never become a destination. + if loc := h.Get("Location"); strings.Contains(loc, evil) { + t.Errorf("%s: redirects to %s", name, loc) + } +} + +// checkedBodies holds the bodies (content type + body) that passed checkBody. +// Static assets come back byte-identical in every state and mode, and +// re-scanning them is most of the crawl's time under -race; a failing body +// is not stored, so it is reported for every response that carries it. +var checkedBodies sync.Map + +// checkBody runs the checks that depend only on the response body. +func checkBody(t *testing.T, name, contentType, body string) { + t.Helper() + key := contentType + "\x00" + body + if _, ok := checkedBodies.Load(key); ok { + return + } + ok := true + errorf := func(format string, args ...any) { + t.Helper() + ok = false + t.Errorf(format, args...) + } + for _, re := range leaks { + if strings.HasPrefix(contentType, "text/javascript") && re.String() != "CANARY" && re.String() != `/api/` { + continue // the Datastar bundle is pinned (TestDatastarPin); only check it for secrets and API paths + } + if m := re.FindString(body); m != "" { + errorf("%s: body contains %q (%s)", name, m, re) + } + } // Injected markup stays text, and never breaks out of a JS string in a // Datastar expression. if strings.Contains(body, ` files root := "." err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { @@ -70,6 +71,7 @@ func TestUsedKeysExistInEveryLocale(t *testing.T) { // TestLocalesHaveSameKeys keeps every locale complete, not just the used keys. func TestLocalesHaveSameKeys(t *testing.T) { + t.Parallel() want := i18n.Keys(i18n.DefaultLocale) for _, l := range i18n.Locales { got := i18n.Keys(l) @@ -92,6 +94,7 @@ func TestLocalesHaveSameKeys(t *testing.T) { } func TestNegotiate(t *testing.T) { + t.Parallel() cases := []struct { name, cookie, accept string want i18n.Locale @@ -108,6 +111,7 @@ func TestNegotiate(t *testing.T) { } for _, c := range cases { t.Run(c.name, func(t *testing.T) { + t.Parallel() r := httptest.NewRequest(http.MethodGet, "/", nil) if c.accept != "" { r.Header.Set("Accept-Language", c.accept) @@ -123,12 +127,14 @@ func TestNegotiate(t *testing.T) { } func TestMissingKeyRendersKey(t *testing.T) { + t.Parallel() if got := (i18n.Translator{Locale: i18n.Ja}).T("pages.nope"); got != "pages.nope" { t.Fatalf("got %q", got) } } func TestDatastarPin(t *testing.T) { + t.Parallel() b, err := static.FS.ReadFile(static.DatastarFile) if err != nil { t.Fatal(err) @@ -143,6 +149,7 @@ func TestDatastarPin(t *testing.T) { } func TestHandlerCaching(t *testing.T) { + t.Parallel() h := static.Handler() rec := httptest.NewRecorder() h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, static.URL(static.CSSFile), nil)) diff --git a/apps/web/ui/handlers_test.go b/apps/web/ui/handlers_test.go index 62a2ecd..deddc76 100644 --- a/apps/web/ui/handlers_test.go +++ b/apps/web/ui/handlers_test.go @@ -16,6 +16,7 @@ import ( ) func TestPages(t *testing.T) { + t.Parallel() e := newEnv(t, config.Features{TemplateWelcome: true}) pages := []string{ config.AppRoutes.Home, config.AppRoutes.Why, config.AppRoutes.Login, config.AppRoutes.Signup, @@ -53,10 +54,12 @@ func TestPages(t *testing.T) { } func TestLogin(t *testing.T) { + t.Parallel() form := url.Values{"email": {" demo@example.com "}, "password": {"password123"}} in := ui.SignInInput{Email: "demo@example.com", Password: "password123", CallbackURL: config.AppRoutes.VerifyEmailSuccess} t.Run("success", func(t *testing.T) { + t.Parallel() e := newEnv(t, config.Features{}) e.auth.EXPECT().SignInEmail(gomock.Any(), in).Return(demoUser, demoSession, nil) e.auth.EXPECT().SetSessionCookie(gomock.Any(), demoSession).Do(setCookie) @@ -68,6 +71,7 @@ func TestLogin(t *testing.T) { }) t.Run("without JS", func(t *testing.T) { + t.Parallel() e := newEnv(t, config.Features{}) e.auth.EXPECT().SignInEmail(gomock.Any(), in).Return(demoUser, demoSession, nil) e.auth.EXPECT().SetSessionCookie(gomock.Any(), demoSession) @@ -83,6 +87,7 @@ func TestLogin(t *testing.T) { // Every auth error the UI knows renders translated; unknown ones generically. for _, ae := range ui.AuthErrors { t.Run(ae.Key, func(t *testing.T) { + t.Parallel() e := newEnv(t, config.Features{}) e.auth.EXPECT().SignInEmail(gomock.Any(), in).Return(ui.User{}, ui.Session{}, ae.Err) if ae.Key == "pages.verifyEmail.description" { // unverified: a fresh link goes out @@ -98,6 +103,7 @@ func TestLogin(t *testing.T) { assertSSE(t, e.do(http.MethodPost, config.AppRoutes.Login, form, datastar), `id="form-error"`, en("pages.verifyEmail.checkEmail")) }) t.Run("unknown error", func(t *testing.T) { + t.Parallel() e := newEnv(t, config.Features{}) e.auth.EXPECT().SignInEmail(gomock.Any(), in).Return(ui.User{}, ui.Session{}, errors.New("db down")) assertSSE(t, e.do(http.MethodPost, config.AppRoutes.Login, form, datastar), en("errors.somethingWentWrong")) @@ -105,6 +111,7 @@ func TestLogin(t *testing.T) { } func TestSignup(t *testing.T) { + t.Parallel() form := url.Values{"name": {"Grace"}, "email": {"grace@example.com"}, "password": {"password123"}} in := ui.SignUpInput{Name: "Grace", Email: "grace@example.com", Password: "password123", CallbackURL: config.AppRoutes.VerifyEmailSuccess} @@ -123,6 +130,7 @@ func TestSignup(t *testing.T) { } func TestForgotAndResetPassword(t *testing.T) { + t.Parallel() e := newEnv(t, config.Features{}) e.auth.EXPECT().RequestPasswordReset(gomock.Any(), "demo@example.com", config.AppRoutes.ResetPassword).Return(nil) assertSSE(t, e.do(http.MethodPost, config.AppRoutes.ForgotPassword, url.Values{"email": {"demo@example.com"}}, datastar), `id="forgot-password"`, en("pages.forgotPassword.emailSent")) @@ -160,6 +168,7 @@ func TestForgotAndResetPassword(t *testing.T) { } func TestVerifyEmail(t *testing.T) { + t.Parallel() e := newEnv(t, config.Features{EmailVerification: true}) e.auth.EXPECT().SendVerificationEmail(gomock.Any(), "new@example.com", config.AppRoutes.VerifyEmailSuccess).Return(nil) assertSSE(t, e.do(http.MethodPost, config.AppRoutes.VerifyEmail, url.Values{"email": {"new@example.com"}}, datastar, lang("ja")), @@ -211,6 +220,7 @@ func TestVerifyEmail(t *testing.T) { } func TestLogout(t *testing.T) { + t.Parallel() e := newEnv(t, config.Features{}) e.signedIn("sess-1") e.auth.EXPECT().SignOut(gomock.Any(), demoSession.Token).Return(nil) @@ -219,6 +229,7 @@ func TestLogout(t *testing.T) { } func TestLocaleSwitch(t *testing.T) { + t.Parallel() e := newEnv(t, config.Features{}) rec := e.do(http.MethodPost, routes.Locale, url.Values{"locale": {"ja"}, "next": {"/login?x=1"}}) assertSeeOther(t, rec, "/login?x=1") @@ -246,6 +257,7 @@ func TestLocaleSwitch(t *testing.T) { } func TestActionsRejectCrossOrigin(t *testing.T) { + t.Parallel() e := newEnv(t, config.Features{}) rec := e.do(http.MethodPost, config.AppRoutes.Login, url.Values{"email": {"a@b.c"}}, func(r *http.Request) { r.Header.Set("Sec-Fetch-Site", "cross-site") diff --git a/apps/web/ui/render_test.go b/apps/web/ui/render_test.go index a13346c..e1b2ab6 100644 --- a/apps/web/ui/render_test.go +++ b/apps/web/ui/render_test.go @@ -32,6 +32,7 @@ func renderIn(t *testing.T, l i18n.Locale, st view.State, c templ.Component) str // TestPagesRenderInEveryLocale renders each parity page (and its states) in // every locale and checks the heading text and that no key leaked through. func TestPagesRenderInEveryLocale(t *testing.T) { + t.Parallel() user := User{ID: "u1", Name: "Ada", Email: "ada@example.com", CreatedAt: time.Date(2026, 1, 2, 0, 0, 0, 0, time.UTC)} ua := "Mozilla/5.0 (iPhone; CPU iPhone OS 18_0 like Mac OS X) Version/18.0 Mobile/15E148 Safari/604.1" sessions := []services.Session{{ID: "s1", Current: true, UserAgent: &ua}, {ID: "s2"}} @@ -73,6 +74,7 @@ func TestPagesRenderInEveryLocale(t *testing.T) { tr := i18n.Translator{Locale: l} for _, p := range pages { t.Run(string(l)+"/"+p.name, func(t *testing.T) { + t.Parallel() html := renderIn(t, l, view.State{Authenticated: p.authed}, p.c) if m := rawKey.FindString(html); m != "" { t.Errorf("untranslated key %q in output", m) @@ -94,6 +96,7 @@ func TestPagesRenderInEveryLocale(t *testing.T) { // TestWelcomeCopyMatches keeps the welcome copy complete in every locale. func TestWelcomeCopyMatches(t *testing.T) { + t.Parallel() en := renderIn(t, i18n.En, view.State{}, welcome.LandingPage()) ja := renderIn(t, i18n.Ja, view.State{}, welcome.LandingPage()) if !strings.Contains(en, "A good first house for your project") || !strings.Contains(ja, "プロジェクトの、最初の家") { diff --git a/packages/auth/account_test.go b/packages/auth/account_test.go index 1eaebe0..9adc040 100644 --- a/packages/auth/account_test.go +++ b/packages/auth/account_test.go @@ -27,6 +27,7 @@ func (h *harness) userID(headers hdr) string { } func TestChangePassword(t *testing.T) { + t.Parallel() h := newHarness(t, nil) email := uniqueEmail(t) cookie := hdr{"Cookie": cookieHeader(t, h.signUp(email, "old-password")), "Origin": testBase} @@ -87,6 +88,7 @@ func TestChangePassword(t *testing.T) { } func TestChangePasswordWithoutCredentialAccount(t *testing.T) { + t.Parallel() h := newHarness(t, nil) ctx := context.Background() id := generateID(32) @@ -102,6 +104,7 @@ func TestChangePasswordWithoutCredentialAccount(t *testing.T) { } func TestChangeEmail(t *testing.T) { + t.Parallel() h := newHarness(t, nil) email, taken, next := uniqueEmail(t), uniqueEmail(t), uniqueEmail(t) cookie := hdr{"Cookie": cookieHeader(t, h.signUp(email, "password123")), "Origin": testBase} @@ -182,6 +185,7 @@ func TestChangeEmail(t *testing.T) { } func TestChangeEmailLinkExpiresAndRechecksTheAddress(t *testing.T) { + t.Parallel() h := newHarness(t, nil) ctx := context.Background() first, second := uniqueEmail(t), uniqueEmail(t) @@ -232,6 +236,7 @@ func (h *harness) changeLinks() map[string]string { } func TestChangeEmailLinksAreBoundToTheirUserAndRequest(t *testing.T) { + t.Parallel() h := newHarness(t, nil) ctx := context.Background() a, b, c := uniqueEmail(t), uniqueEmail(t), uniqueEmail(t) @@ -295,21 +300,23 @@ func TestChangeEmailLinksAreBoundToTheirUserAndRequest(t *testing.T) { } func TestPendingEmailChangeIsCancelledByRecovery(t *testing.T) { + t.Parallel() ctx := context.Background() + // Each recovery fails the subtest that runs it (h.t), not the parent. for name, recover := range map[string]func(h *harness, userID, email string){ "change password": func(h *harness, userID, _ string) { if err := h.a.ChangePassword(ctx, userID, "password123", "new-password", false, ""); err != nil { - t.Fatal(err) + h.t.Fatal(err) } }, "revoke other sessions": func(h *harness, userID, _ string) { if err := h.a.RevokeOtherSessions(ctx, userID, ""); err != nil { - t.Fatal(err) + h.t.Fatal(err) } }, "password reset": func(h *harness, _, email string) { if err := h.a.RequestPasswordReset(ctx, email, "/r"); err != nil { - t.Fatal(err) + h.t.Fatal(err) } var link string for _, m := range h.mails() { @@ -317,12 +324,13 @@ func TestPendingEmailChangeIsCancelledByRecovery(t *testing.T) { link = m.url } } - if err := h.a.ResetPassword(ctx, mustQuery(t, link, "token"), "new-password"); err != nil { - t.Fatal(err) + if err := h.a.ResetPassword(ctx, mustQuery(h.t, link, "token"), "new-password"); err != nil { + h.t.Fatal(err) } }, } { t.Run(name, func(t *testing.T) { + t.Parallel() h := newHarness(t, nil) email, attacker := uniqueEmail(t), uniqueEmail(t) wantStatus(t, h.signUp(email, "password123"), http.StatusOK) @@ -340,6 +348,7 @@ func TestPendingEmailChangeIsCancelledByRecovery(t *testing.T) { } func TestChangeEmailNeedsAFreshSession(t *testing.T) { + t.Parallel() h := newHarness(t, nil) email := uniqueEmail(t) cookie := hdr{"Cookie": cookieHeader(t, h.signUp(email, "password123")), "Origin": testBase} @@ -352,6 +361,7 @@ func TestChangeEmailNeedsAFreshSession(t *testing.T) { } func TestChangePasswordConcurrencyAndPerUserLimit(t *testing.T) { + t.Parallel() h := newHarness(t, nil) ctx := context.Background() email := uniqueEmail(t) @@ -394,6 +404,7 @@ func TestChangePasswordConcurrencyAndPerUserLimit(t *testing.T) { } func TestChangeEmailHidesTakenAddressWhenVerificationRequired(t *testing.T) { + t.Parallel() h := newHarness(t, func(c *Config) { c.RequireEmailVerification = true }) ctx := context.Background() owner, taken := uniqueEmail(t), uniqueEmail(t) @@ -416,6 +427,7 @@ func TestChangeEmailHidesTakenAddressWhenVerificationRequired(t *testing.T) { } func TestSessionsListAndRevoke(t *testing.T) { + t.Parallel() h := newHarness(t, nil) ctx := context.Background() email, otherEmail := uniqueEmail(t), uniqueEmail(t) diff --git a/packages/auth/admin_test.go b/packages/auth/admin_test.go index 9fb8737..28447ff 100644 --- a/packages/auth/admin_test.go +++ b/packages/auth/admin_test.go @@ -10,6 +10,7 @@ import ( ) func TestBanAndUnban(t *testing.T) { + t.Parallel() h := newHarness(t, nil) ctx := context.Background() email := uniqueEmail(t) @@ -75,6 +76,7 @@ func TestBanAndUnban(t *testing.T) { } func TestAdminResetTwoFactorRecoversAccess(t *testing.T) { + t.Parallel() h := newHarness(t, with2FA(true)) ctx := context.Background() email := uniqueEmail(t) @@ -98,6 +100,7 @@ func TestAdminResetTwoFactorRecoversAccess(t *testing.T) { } func TestDisableSignUpKeepsInvites(t *testing.T) { + t.Parallel() h := newHarness(t, func(c *Config) { c.DisableSignUp = true }) ctx := context.Background() email := uniqueEmail(t) @@ -122,6 +125,7 @@ func TestDisableSignUpKeepsInvites(t *testing.T) { } func TestBannedUserGetsNoTwoFactorChallenge(t *testing.T) { + t.Parallel() h := newHarness(t, with2FA(false)) email := uniqueEmail(t) _, signedIn := h.enrol(email) @@ -132,11 +136,9 @@ func TestBannedUserGetsNoTwoFactorChallenge(t *testing.T) { } func TestListUsersAndAdminOperations(t *testing.T) { + t.Parallel() ctx := context.Background() - pool := newDatabase(t) - if err := Migrate(ctx, pool); err != nil { - t.Fatal(err) - } + pool := newDatabase(t) // lists and counts every user: not the shared database h := newHarness(t, nil) a, err := New(h.a.cfg, pool, h.mail) if err != nil { @@ -233,6 +235,7 @@ func TestListUsersAndAdminOperations(t *testing.T) { } func TestInviteSetPasswordFlow(t *testing.T) { + t.Parallel() h := newHarness(t, nil) ctx := context.Background() email := uniqueEmail(t) @@ -300,6 +303,7 @@ func TestInviteSetPasswordFlow(t *testing.T) { } func TestInviteContinuesIntoRequiredTwoFactor(t *testing.T) { + t.Parallel() h := newHarness(t, with2FA(true)) ctx := context.Background() email := uniqueEmail(t) @@ -338,11 +342,9 @@ func TestInviteContinuesIntoRequiredTwoFactor(t *testing.T) { } func TestSeparateInstancesDoNotCollide(t *testing.T) { + t.Parallel() ctx := context.Background() - appPool, adminPool := newDatabase(t), newDatabase(t) - if err := Migrate(ctx, appPool); err != nil { - t.Fatal(err) - } + appPool, adminPool := newDatabase(t), newEmptyDatabase(t) if err := MigrateTable(ctx, adminPool, "goose_admin_auth"); err != nil { t.Fatal(err) } diff --git a/packages/auth/events_test.go b/packages/auth/events_test.go index cc4b0ad..4961af6 100644 --- a/packages/auth/events_test.go +++ b/packages/auth/events_test.go @@ -54,6 +54,7 @@ func withEvents(l *eventLog, more ...func(*Config)) func(*Config) { } func TestEventsForAccountFlows(t *testing.T) { + t.Parallel() log := &eventLog{} h := newHarness(t, withEvents(log)) ctx := context.Background() @@ -169,6 +170,7 @@ func TestEventsForAccountFlows(t *testing.T) { } func TestTwoFactorEventsNameTheChallengedUser(t *testing.T) { + t.Parallel() log := &eventLog{} h := newHarness(t, withEvents(log, with2FA(false))) email := uniqueEmail(t) @@ -202,6 +204,7 @@ func TestTwoFactorEventsNameTheChallengedUser(t *testing.T) { // TestEventsNeverCarrySecrets serialises every event of a busy session and // looks for the tokens, passwords and codes involved. func TestEventsNeverCarrySecrets(t *testing.T) { + t.Parallel() log := &eventLog{} h := newHarness(t, withEvents(log, with2FA(false))) email := uniqueEmail(t) @@ -224,6 +227,7 @@ func TestEventsNeverCarrySecrets(t *testing.T) { } func TestEventHandlerCannotBreakAuth(t *testing.T) { + t.Parallel() // A panicking handler: auth keeps working and Shutdown still returns. panicky := newHarness(t, func(c *Config) { c.Events = func(context.Context, Event) { panic("boom") } }) email := uniqueEmail(t) @@ -344,6 +348,7 @@ func TestEventHandlerCannotBreakAuth(t *testing.T) { } func TestStandaloneUserAdminEvents(t *testing.T) { + t.Parallel() h := newHarness(t, nil) ctx := context.Background() email := uniqueEmail(t) diff --git a/packages/auth/flows_test.go b/packages/auth/flows_test.go index 139ea24..431e822 100644 --- a/packages/auth/flows_test.go +++ b/packages/auth/flows_test.go @@ -14,13 +14,14 @@ import ( ) func TestMigrateIsIdempotent(t *testing.T) { - needDB(t) + t.Parallel() + pool := newDatabase(t) // already migrated (a clone of the template) ctx := context.Background() - if err := Migrate(ctx, testPool); err != nil { + if err := Migrate(ctx, pool); err != nil { t.Fatal(err) } var version int64 - if err := testPool.QueryRow(ctx, `SELECT max(version_id) FROM goose_auth`).Scan(&version); err != nil { + if err := pool.QueryRow(ctx, `SELECT max(version_id) FROM goose_auth`).Scan(&version); err != nil { t.Fatal(err) } files, _ := fs.Glob(Migrations, "*.sql") @@ -29,13 +30,14 @@ func TestMigrateIsIdempotent(t *testing.T) { } for _, table := range []string{"user", "session", "account", "verification"} { var exists bool - if err := testPool.QueryRow(ctx, `SELECT to_regclass($1) IS NOT NULL`, `"`+table+`"`).Scan(&exists); err != nil || !exists { + if err := pool.QueryRow(ctx, `SELECT to_regclass($1) IS NOT NULL`, `"`+table+`"`).Scan(&exists); err != nil || !exists { t.Fatalf("table %q missing (err %v)", table, err) } } } func TestNewValidatesConfig(t *testing.T) { + t.Parallel() needDB(t) cases := map[string]Config{ "short secret": {Secret: "short", BaseURL: testBase}, @@ -62,6 +64,7 @@ func TestNewValidatesConfig(t *testing.T) { } func TestCookieSessionLifecycle(t *testing.T) { + t.Parallel() h := newHarness(t, nil) email := uniqueEmail(t) @@ -121,6 +124,7 @@ func TestCookieSessionLifecycle(t *testing.T) { } func TestBearerSession(t *testing.T) { + t.Parallel() h := newHarness(t, nil) email := uniqueEmail(t) wantStatus(t, h.signUp(email, "password123"), http.StatusOK) @@ -164,6 +168,7 @@ func TestBearerSession(t *testing.T) { } func TestSignInRejectsBadCredentials(t *testing.T) { + t.Parallel() h := newHarness(t, nil) email := uniqueEmail(t) wantStatus(t, h.signUp(email, "password123"), http.StatusOK) @@ -177,6 +182,7 @@ func TestSignInRejectsBadCredentials(t *testing.T) { } func TestSignUpValidation(t *testing.T) { + t.Parallel() h := newHarness(t, nil) wantCode(t, h.signUp(uniqueEmail(t), "short"), http.StatusBadRequest, "PASSWORD_TOO_SHORT") wantCode(t, h.signUp(uniqueEmail(t), strings.Repeat("é", 129)), http.StatusBadRequest, "PASSWORD_TOO_LONG") @@ -197,6 +203,7 @@ func TestSignUpValidation(t *testing.T) { } func TestEmailVerificationFlow(t *testing.T) { + t.Parallel() h := newHarness(t, func(c *Config) { c.RequireEmailVerification = true }) email := uniqueEmail(t) @@ -258,6 +265,7 @@ func TestEmailVerificationFlow(t *testing.T) { } func TestVerifyEmailRejectsBadTokens(t *testing.T) { + t.Parallel() h := newHarness(t, func(c *Config) { c.RequireEmailVerification = true }) email := uniqueEmail(t) wantStatus(t, h.signUp(email, "password123"), http.StatusOK) @@ -285,6 +293,7 @@ func TestVerifyEmailRejectsBadTokens(t *testing.T) { } func TestSendVerificationEmail(t *testing.T) { + t.Parallel() h := newHarness(t, func(c *Config) { c.RequireEmailVerification = true }) unverified := uniqueEmail(t) wantStatus(t, h.signUp(unverified, "password123"), http.StatusOK) @@ -319,6 +328,7 @@ func TestSendVerificationEmail(t *testing.T) { } func TestPasswordResetFlow(t *testing.T) { + t.Parallel() h := newHarness(t, nil) email := uniqueEmail(t) wantStatus(t, h.signUp(email, "old-password"), http.StatusOK) @@ -372,6 +382,7 @@ func TestPasswordResetFlow(t *testing.T) { } func TestPasswordResetTokenExpires(t *testing.T) { + t.Parallel() h := newHarness(t, nil) email := uniqueEmail(t) wantStatus(t, h.signUp(email, "old-password"), http.StatusOK) @@ -394,6 +405,7 @@ func TestPasswordResetTokenExpires(t *testing.T) { } func TestPasswordResetTokenIsSingleUseUnderConcurrency(t *testing.T) { + t.Parallel() h := newHarness(t, nil) email := uniqueEmail(t) wantStatus(t, h.signUp(email, "old-password"), http.StatusOK) @@ -422,6 +434,7 @@ func TestPasswordResetTokenIsSingleUseUnderConcurrency(t *testing.T) { } func TestRedirectTargetsMustBeTrusted(t *testing.T) { + t.Parallel() h := newHarness(t, nil) email := uniqueEmail(t) wantStatus(t, h.signUp(email, "password123"), http.StatusOK) @@ -448,6 +461,7 @@ func TestRedirectTargetsMustBeTrusted(t *testing.T) { } func TestOriginCheck(t *testing.T) { + t.Parallel() h := newHarness(t, nil) email := uniqueEmail(t) cookie := cookieHeader(t, h.signUp(email, "password123")) @@ -517,6 +531,7 @@ type httpResult struct { } func TestSessionRefreshAndExpiry(t *testing.T) { + t.Parallel() h := newHarness(t, nil) rec := h.signUp(uniqueEmail(t), "password123") cookie := cookieHeader(t, rec) @@ -573,6 +588,7 @@ func TestSessionRefreshAndExpiry(t *testing.T) { } func TestRequireUserAndUserFrom(t *testing.T) { + t.Parallel() h := newHarness(t, func(c *Config) { c.LoginPath = "/sign-in" }) email := uniqueEmail(t) cookie := cookieHeader(t, h.signUp(email, "password123")) @@ -607,6 +623,7 @@ func TestRequireUserAndUserFrom(t *testing.T) { } func TestServiceMethodsForHTMLTransport(t *testing.T) { + t.Parallel() h := newHarness(t, nil) ctx := context.Background() email := uniqueEmail(t) @@ -655,6 +672,7 @@ func TestServiceMethodsForHTMLTransport(t *testing.T) { // HostOnlyCookies: on https every cookie of the instance is __Host- (Path=/, // no Domain, Secure), and the __Secure- name is not read. func TestHostOnlyCookies(t *testing.T) { + t.Parallel() h := newHarness(t, func(c *Config) { c.BaseURL = "https://admin.test"; c.CookiePrefix = "admin"; c.HostOnlyCookies = true }) rec := h.signUp(uniqueEmail(t), "password123") c := sessionCookie(rec) @@ -675,6 +693,7 @@ func TestHostOnlyCookies(t *testing.T) { } func TestSecureCookieOnHTTPS(t *testing.T) { + t.Parallel() h := newHarness(t, func(c *Config) { c.BaseURL = "https://app.test" }) rec := h.signUp(uniqueEmail(t), "password123") c := sessionCookie(rec) @@ -691,6 +710,7 @@ func TestSecureCookieOnHTTPS(t *testing.T) { } func TestRateLimitedEndpoints(t *testing.T) { + t.Parallel() h := newHarness(t, func(c *Config) { c.RateLimiter = NewMemoryRateLimiter() }) email := uniqueEmail(t) for i := range 3 { @@ -721,6 +741,7 @@ func TestRateLimitedEndpoints(t *testing.T) { } func TestClientIPFromTrustedProxyHeader(t *testing.T) { + t.Parallel() h := newHarness(t, func(c *Config) { c.ClientIPHeader = "X-Forwarded-For" }) rec := h.do("POST", BasePath+"/sign-up/email", map[string]any{"name": "ip", "email": uniqueEmail(t), "password": "password123"}, hdr{"X-Forwarded-For": "6.6.6.6, 203.0.113.9", "User-Agent": "test-agent"}) @@ -732,6 +753,7 @@ func TestClientIPFromTrustedProxyHeader(t *testing.T) { } func TestOKEndpoint(t *testing.T) { + t.Parallel() h := newHarness(t, nil) rec := h.do("GET", BasePath+"/ok", nil, nil) wantStatus(t, rec, http.StatusOK) @@ -752,6 +774,7 @@ func mustQuery(t *testing.T, link, key string) string { const betterAuthHash = "0123456789abcdef0123456789abcdef:79729cb5bb1f0806ff13d495dfbe112886f07473fd22165fdad4415930961e224a5c3911b65af1d06f9d38cca6b0eb1a39717181f0abf4f30ff2ddb3bfc5d985" func TestImportedBetterAuthUserSignsInAndIsRehashed(t *testing.T) { + t.Parallel() h := newHarness(t, nil) ctx := context.Background() email := uniqueEmail(t) @@ -778,6 +801,7 @@ func TestImportedBetterAuthUserSignsInAndIsRehashed(t *testing.T) { } func TestSquattedSignUpIsReclaimedByEmailOwner(t *testing.T) { + t.Parallel() h := newHarness(t, func(c *Config) { c.RequireEmailVerification = true }) victim := uniqueEmail(t) link := func(i int) string { return pathOf(t, h.mails()[i].url) } @@ -816,6 +840,7 @@ func TestSquattedSignUpIsReclaimedByEmailOwner(t *testing.T) { } func TestResetRevokesOtherResetLinks(t *testing.T) { + t.Parallel() h := newHarness(t, nil) email := uniqueEmail(t) wantStatus(t, h.signUp(email, "old-password"), http.StatusOK) @@ -841,6 +866,7 @@ func TestResetRevokesOtherResetLinks(t *testing.T) { } func TestSignOutWithBearerAndCookieRevokesBoth(t *testing.T) { + t.Parallel() h := newHarness(t, nil) email := uniqueEmail(t) cookie := cookieHeader(t, h.signUp(email, "password123")) diff --git a/packages/auth/keyring_test.go b/packages/auth/keyring_test.go index c54292a..c53b47b 100644 --- a/packages/auth/keyring_test.go +++ b/packages/auth/keyring_test.go @@ -28,11 +28,7 @@ var ( // test database would rewrap the data keys under other tests. func rotationDB(t *testing.T) *pgxpool.Pool { t.Helper() - pool := newDatabase(t) - if err := Migrate(context.Background(), pool); err != nil { - t.Fatal(err) - } - return pool + return newDatabase(t) } // instance is a 2FA-enabled Auth on pool with the given secrets, sharing clock. @@ -77,6 +73,7 @@ func cookieValue(t *testing.T, header string) string { } func TestRotationKeepsSessionsLinksAndTwoFactor(t *testing.T) { + t.Parallel() pool := rotationDB(t) clock := &testClock{t: time.Now()} h := instance(t, pool, clock, secretA) @@ -154,6 +151,7 @@ func TestRotationKeepsSessionsLinksAndTwoFactor(t *testing.T) { } func TestGraceWindowIsSharedAcrossInstances(t *testing.T) { + t.Parallel() pool := rotationDB(t) clock := &testClock{t: time.Now()} first := instance(t, pool, clock, secretA) @@ -176,6 +174,7 @@ func TestGraceWindowIsSharedAcrossInstances(t *testing.T) { } func TestRotationWithoutPreviousRevokesAtOnceButKeepsTwoFactor(t *testing.T) { + t.Parallel() pool := rotationDB(t) clock := &testClock{t: time.Now()} h := instance(t, pool, clock, secretA) @@ -215,6 +214,7 @@ func TestRotationWithoutPreviousRevokesAtOnceButKeepsTwoFactor(t *testing.T) { } func TestDefaultSecretGraceCoversEverySignedLifetime(t *testing.T) { + t.Parallel() if g := DefaultSecretGrace(Config{}); g != DefaultSessionTTL { t.Fatalf("default = %s, want the session lifetime", g) } @@ -227,6 +227,7 @@ func TestDefaultSecretGraceCoversEverySignedLifetime(t *testing.T) { } func TestDataKeysOfExistingTwoFactorRowsAreKept(t *testing.T) { + t.Parallel() pool := rotationDB(t) ctx := context.Background() // A database from before "authKey": a 2FA row sealed with the key derived @@ -272,6 +273,7 @@ func TestDataKeysOfExistingTwoFactorRowsAreKept(t *testing.T) { } func TestRevokedSecretNeverVerifiesButUnwraps(t *testing.T) { + t.Parallel() pool := rotationDB(t) ctx := context.Background() clock := &testClock{t: time.Now()} @@ -307,6 +309,7 @@ func TestRevokedSecretNeverVerifiesButUnwraps(t *testing.T) { } func TestResignFollowsWhereTheTokenCameFrom(t *testing.T) { + t.Parallel() pool := rotationDB(t) clock := &testClock{t: time.Now()} h := instance(t, pool, clock, secretA) @@ -331,6 +334,7 @@ func TestResignFollowsWhereTheTokenCameFrom(t *testing.T) { } func TestFailedKeyStorageStillAppliesTheKeyring(t *testing.T) { + t.Parallel() pool := rotationDB(t) clock := &testClock{t: time.Now()} h := instance(t, pool, clock, secretA) diff --git a/packages/auth/links_test.go b/packages/auth/links_test.go index 887af06..f6eabbe 100644 --- a/packages/auth/links_test.go +++ b/packages/auth/links_test.go @@ -41,6 +41,7 @@ func wantAPILink(t *testing.T, link, pathPrefix, callbackURL string) { } func TestResetLinkShapes(t *testing.T) { + t.Parallel() h := newHarness(t, nil) ctx := context.Background() email := uniqueEmail(t) @@ -76,6 +77,7 @@ func TestResetLinkShapes(t *testing.T) { } func TestSetPasswordLinkShapes(t *testing.T) { + t.Parallel() h := newHarness(t, nil) ctx := context.Background() email := uniqueEmail(t) @@ -107,6 +109,7 @@ func TestSetPasswordLinkShapes(t *testing.T) { } func TestVerificationLinkShapes(t *testing.T) { + t.Parallel() ctx := context.Background() signUp := func(h *harness, callbackURL string) (string, string) { h.t.Helper() @@ -168,6 +171,7 @@ func TestVerificationLinkShapes(t *testing.T) { } func TestChangeEmailLinkShapes(t *testing.T) { + t.Parallel() ctx := context.Background() h := newHarness(t, func(c *Config) { c.VerifyEmailPath = verifyPage }) email := uniqueEmail(t) diff --git a/packages/auth/mailer_test.go b/packages/auth/mailer_test.go index 76bf1cd..5bb5871 100644 --- a/packages/auth/mailer_test.go +++ b/packages/auth/mailer_test.go @@ -21,6 +21,7 @@ type resendPayload struct { } func TestResendMailerSends(t *testing.T) { + t.Parallel() const link = "https://app.example.test/api/auth/verify-email?token=abc&next=/dashboard" tests := []struct { @@ -73,6 +74,7 @@ func TestResendMailerSends(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Parallel() var got resendPayload srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodPost { @@ -134,11 +136,12 @@ func TestResendMailerSends(t *testing.T) { } func TestResendMailerErrors(t *testing.T) { + t.Parallel() srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusUnprocessableEntity) _, _ = io.WriteString(w, `{"message":"invalid from address"}`) })) - defer srv.Close() + t.Cleanup(srv.Close) // after the parallel subtests, not when this function returns tests := []struct { name string @@ -152,6 +155,7 @@ func TestResendMailerErrors(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Parallel() err := tt.mailer.SendPasswordResetEmail(context.Background(), "user@example.test", "https://app.example.test/reset") if err == nil { t.Fatal("expected error") @@ -164,6 +168,7 @@ func TestResendMailerErrors(t *testing.T) { } func TestDevMailerLogsLink(t *testing.T) { + t.Parallel() tests := []struct { name string send func(m *DevMailer) error @@ -178,6 +183,7 @@ func TestDevMailerLogsLink(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Parallel() var buf bytes.Buffer m := NewDevMailer(slog.New(slog.NewTextHandler(&buf, nil))) if err := tt.send(m); err != nil { @@ -194,12 +200,14 @@ func TestDevMailerLogsLink(t *testing.T) { } func TestNewDevMailerNilLogger(t *testing.T) { + t.Parallel() if m := NewDevMailer(nil); m.Logger == nil { t.Fatal("expected default logger") } } func TestSwapMailerUsesTheLatestMailer(t *testing.T) { + t.Parallel() var keys []string srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { keys = append(keys, r.Header.Get("Authorization")) diff --git a/packages/auth/main_test.go b/packages/auth/main_test.go index 1be773c..e617a67 100644 --- a/packages/auth/main_test.go +++ b/packages/auth/main_test.go @@ -23,22 +23,29 @@ import ( ) // testPool is a pool on a throwaway database with the auth migrations -// applied, or nil when no database is configured. +// applied, or nil when no database is configured. Tests on it run in +// parallel, so they only touch rows they created (uniqueEmail, generateID); +// a test that reads table-wide state uses newDatabase instead. var testPool *pgxpool.Pool -// TestMain creates a fresh database (auth_test_) on the server named by -// TEST_DATABASE_URL or DATABASE_URL, so it never touches existing data and can -// run alongside other packages' tests. Without a URL the database tests skip, +// testDSN is the server URL (TEST_DATABASE_URL or DATABASE_URL), and +// templateDB a migrated template database on it that newDatabase clones. +var testDSN, templateDB string + +// TestMain migrates a template database (auth_tmpl_) once on the +// server named by TEST_DATABASE_URL or DATABASE_URL and clones it into the +// shared test database, so it never touches existing data and can run +// alongside other packages' tests. Without a URL the database tests skip, // except under CI where they fail. func TestMain(m *testing.M) { - dsn := os.Getenv("TEST_DATABASE_URL") - if dsn == "" { - dsn = os.Getenv("DATABASE_URL") + testDSN = os.Getenv("TEST_DATABASE_URL") + if testDSN == "" { + testDSN = os.Getenv("DATABASE_URL") } - if dsn == "" { + if testDSN == "" { os.Exit(m.Run()) } - code, err := runWithDatabase(m, dsn) + code, err := runWithDatabase(m) if err != nil { fmt.Fprintln(os.Stderr, "auth tests:", err) os.Exit(1) @@ -46,69 +53,113 @@ func TestMain(m *testing.M) { os.Exit(code) } -func runWithDatabase(m *testing.M, dsn string) (int, error) { +func runWithDatabase(m *testing.M) (int, error) { ctx := context.Background() - admin, err := connectWhenReady(ctx, dsn) + admin, err := connectWhenReady(ctx, testDSN) if err != nil { return 0, err } defer func() { _ = admin.Close(ctx) }() + // Random, not pid+time: concurrent or repeated test binaries must never pick // the same name (a collision fails TestMain before any test runs). - name := "auth_test_" + strings.ToLower(generateID(16)) - if _, err := admin.Exec(ctx, "CREATE DATABASE "+name); err != nil { + tmpl := "auth_tmpl_" + strings.ToLower(generateID(16)) + if _, err := admin.Exec(ctx, "CREATE DATABASE "+tmpl); err != nil { return 0, err } - defer func() { _, _ = admin.Exec(ctx, "DROP DATABASE "+name+" WITH (FORCE)") }() + defer func() { + // A template cannot be dropped; unmark it first. + _, _ = admin.Exec(ctx, "ALTER DATABASE "+tmpl+" WITH IS_TEMPLATE false") + _, _ = admin.Exec(ctx, "DROP DATABASE "+tmpl+" WITH (FORCE)") + }() + if err := migrateTemplate(ctx, admin, tmpl); err != nil { + return 0, err + } + templateDB = tmpl - cfg, err := pgxpool.ParseConfig(dsn) - if err != nil { + name := "auth_test_" + strings.ToLower(generateID(16)) + if _, err := admin.Exec(ctx, cloneSQL(name)); err != nil { return 0, err } - cfg.ConnConfig.Database = name - testPool, err = pgxpool.NewWithConfig(ctx, cfg) + defer func() { _, _ = admin.Exec(ctx, "DROP DATABASE "+name+" WITH (FORCE)") }() + testPool, err = poolOn(ctx, name) if err != nil { return 0, err } defer testPool.Close() - if err := Migrate(ctx, testPool); err != nil { - return 0, err - } return m.Run(), nil } -// newDatabase creates an empty database on the test server for one test, -// dropped when it ends, and returns a pool on it. +// migrateTemplate applies the auth migrations to tmpl and marks it a +// template nobody can connect to, which is what lets clones run concurrently. +func migrateTemplate(ctx context.Context, admin *pgx.Conn, tmpl string) error { + pool, err := poolOn(ctx, tmpl) + if err != nil { + return err + } + err = Migrate(ctx, pool) + pool.Close() + if err != nil { + return err + } + _, err = admin.Exec(ctx, "ALTER DATABASE "+tmpl+" WITH IS_TEMPLATE true ALLOW_CONNECTIONS false") + return err +} + +func cloneSQL(name string) string { + return "CREATE DATABASE " + name + " TEMPLATE " + templateDB + " STRATEGY FILE_COPY" +} + +// poolOn returns a pool on database name of the test server. +func poolOn(ctx context.Context, name string) (*pgxpool.Pool, error) { + cfg, err := pgxpool.ParseConfig(testDSN) + if err != nil { + return nil, err + } + cfg.ConnConfig.Database = name + return pgxpool.NewWithConfig(ctx, cfg) +} + +// newDatabase gives one test a migrated database of its own (a clone of the +// template), dropped when it ends: for tests that read table-wide state or +// change state other tests share (key rotation). func newDatabase(t *testing.T) *pgxpool.Pool { t.Helper() needDB(t) + return createDatabase(t, cloneSQL) +} + +// newEmptyDatabase gives one test an empty database (no migrations), dropped +// when it ends. +func newEmptyDatabase(t *testing.T) *pgxpool.Pool { + t.Helper() + needDB(t) + return createDatabase(t, func(name string) string { return "CREATE DATABASE " + name }) +} + +func createDatabase(t *testing.T, create func(name string) string) *pgxpool.Pool { + t.Helper() ctx := context.Background() - dsn := os.Getenv("TEST_DATABASE_URL") - if dsn == "" { - dsn = os.Getenv("DATABASE_URL") - } - admin, err := pgx.Connect(ctx, dsn) + admin, err := pgx.Connect(ctx, testDSN) if err != nil { t.Fatal(err) } name := "auth_extra_" + strings.ToLower(generateID(12)) - if _, err := admin.Exec(ctx, "CREATE DATABASE "+name); err != nil { - t.Fatal(err) - } - cfg, err := pgxpool.ParseConfig(dsn) - if err != nil { - t.Fatal(err) - } - cfg.ConnConfig.Database = name - pool, err := pgxpool.NewWithConfig(ctx, cfg) - if err != nil { + if _, err := admin.Exec(ctx, create(name)); err != nil { + _ = admin.Close(ctx) t.Fatal(err) } + pool, err := poolOn(ctx, name) t.Cleanup(func() { - pool.Close() + if pool != nil { + pool.Close() + } _, _ = admin.Exec(context.Background(), "DROP DATABASE "+name+" WITH (FORCE)") _ = admin.Close(context.Background()) }) + if err != nil { + t.Fatal(err) + } return pool } @@ -130,7 +181,7 @@ func connectWhenReady(ctx context.Context, dsn string) (*pgx.Conn, error) { if time.Now().After(deadline) || ctx.Err() != nil { return nil, fmt.Errorf("postgres not ready after 30s: %w", err) } - time.Sleep(200 * time.Millisecond) // readiness poll for the server, not test timing + time.Sleep(200 * time.Millisecond) //nolint:forbidigo // polls Postgres readiness before any test runs, not test timing } } diff --git a/packages/auth/tokens_test.go b/packages/auth/tokens_test.go index b878455..19dce3b 100644 --- a/packages/auth/tokens_test.go +++ b/packages/auth/tokens_test.go @@ -35,6 +35,7 @@ func unitAuthWith(secret string) *Auth { } func TestGenerateID(t *testing.T) { + t.Parallel() seen := map[string]bool{} for range 1000 { id := generateID(32) @@ -49,6 +50,7 @@ func TestGenerateID(t *testing.T) { } func TestSignedValueFormat(t *testing.T) { + t.Parallel() a := unitAuth() signed := a.signValue("abc") value, sig, _ := strings.Cut(signed, ".") @@ -67,6 +69,7 @@ func TestSignedValueFormat(t *testing.T) { } func TestVerifySigned(t *testing.T) { + t.Parallel() a := unitAuth() good := a.signValue("tok123") other := unitAuthWith(strings.Repeat("o", 40)).signValue("tok123") @@ -97,6 +100,7 @@ func TestVerifySigned(t *testing.T) { } func TestParseBearer(t *testing.T) { + t.Parallel() a := unitAuth() tok := generateID(32) cases := []struct { @@ -127,6 +131,7 @@ func TestParseBearer(t *testing.T) { } func TestVerificationToken(t *testing.T) { + t.Parallel() a := unitAuth() now := time.Unix(1_800_000_000, 0) a.now = func() time.Time { return now } @@ -232,6 +237,7 @@ func FuzzParseVerificationToken(f *testing.F) { } func TestMatchesOrigin(t *testing.T) { + t.Parallel() cases := []struct { url, pattern string relative bool @@ -281,6 +287,7 @@ func TestMatchesOrigin(t *testing.T) { } func TestNormalizeIP(t *testing.T) { + t.Parallel() cases := map[string]string{ "203.0.113.9": "203.0.113.9", "::ffff:203.0.113.9": "203.0.113.9", @@ -299,6 +306,7 @@ func TestNormalizeIP(t *testing.T) { } func TestMobileTrustedOrigins(t *testing.T) { + t.Parallel() if got := MobileTrustedOrigins(true); len(got) != 1 || got[0] != "betterstack://" { t.Fatalf("production = %v", got) } @@ -308,6 +316,7 @@ func TestMobileTrustedOrigins(t *testing.T) { } func TestPasswordHashing(t *testing.T) { + t.Parallel() p := argonParams{Memory: 64, Time: 1, Threads: 1, SaltLen: 16, KeyLen: 32} h1, err := hashPassword("hunter2hunter2", p) if err != nil { @@ -343,6 +352,7 @@ func TestPasswordHashing(t *testing.T) { } func TestBetterAuthScryptHash(t *testing.T) { + t.Parallel() if ok, err := verifyPassword("hunter2hunter2", betterAuthHash); !ok || err != nil { t.Fatalf("verify = %v %v", ok, err) } @@ -357,6 +367,7 @@ func TestBetterAuthScryptHash(t *testing.T) { } func TestPasswordHashDefaults(t *testing.T) { + t.Parallel() if testing.Short() { t.Skip("slow with -race") } @@ -374,6 +385,7 @@ func TestPasswordHashDefaults(t *testing.T) { } func TestPasswordLengthCountsCharacters(t *testing.T) { + t.Parallel() a := unitAuth() if err := a.checkPasswordLength("ééééééé"); !errors.Is(err, ErrPasswordTooShort) { // 7 chars, 14 bytes t.Fatalf("err = %v", err) @@ -384,6 +396,7 @@ func TestPasswordLengthCountsCharacters(t *testing.T) { } func TestMemoryRateLimiter(t *testing.T) { + t.Parallel() m := NewMemoryRateLimiter() now := time.Unix(0, 0) m.now = func() time.Time { return now } @@ -420,6 +433,7 @@ func (failingLimiter) Allow(context.Context, string, RateRule) (time.Duration, e } func TestRateLimiterFailureFailsOpen(t *testing.T) { + t.Parallel() a := unitAuth() a.limiter = failingLimiter{} ctx := context.WithValue(context.Background(), ctxKey{}, &requestMeta{ip: "1.2.3.4"}) @@ -429,6 +443,7 @@ func TestRateLimiterFailureFailsOpen(t *testing.T) { } func TestErrorMapping(t *testing.T) { + t.Parallel() if asError(&RateLimitError{RetryAfter: time.Second}) != ErrTooManyRequests { t.Fatal("rate limit error not mapped") } diff --git a/packages/auth/twofactor_test.go b/packages/auth/twofactor_test.go index 99c8423..f48f393 100644 --- a/packages/auth/twofactor_test.go +++ b/packages/auth/twofactor_test.go @@ -95,6 +95,7 @@ func (h *harness) challenge(email, password string, extra ...string) string { } func TestTwoFactorEnrolmentAndSignIn(t *testing.T) { + t.Parallel() h := newHarness(t, with2FA(false)) email := uniqueEmail(t) signedIn := hdr{"Cookie": cookieHeader(t, h.signUp(email, "password123")), "Origin": testBase} @@ -168,6 +169,7 @@ func TestTwoFactorEnrolmentAndSignIn(t *testing.T) { } func TestBackupCodesAreSingleUse(t *testing.T) { + t.Parallel() h := newHarness(t, with2FA(false)) email := uniqueEmail(t) setup, signedIn := h.enrol(email) @@ -196,6 +198,7 @@ func TestBackupCodesAreSingleUse(t *testing.T) { } func TestTrustedDeviceAndDisable(t *testing.T) { + t.Parallel() h := newHarness(t, with2FA(false)) email := uniqueEmail(t) setup, signedIn := h.enrol(email) @@ -235,6 +238,7 @@ func TestTrustedDeviceAndDisable(t *testing.T) { } func TestTwoFactorLockoutExpiryAndOff(t *testing.T) { + t.Parallel() h := newHarness(t, with2FA(false)) ctx := context.Background() email := uniqueEmail(t) @@ -279,6 +283,7 @@ func TestTwoFactorLockoutExpiryAndOff(t *testing.T) { } func TestRequiredTwoFactorEnrolsBeforeTheFirstSession(t *testing.T) { + t.Parallel() h := newHarness(t, with2FA(true)) email := uniqueEmail(t) diff --git a/packages/secrets/bootstrap_test.go b/packages/secrets/bootstrap_test.go index 08c10ed..14cbac3 100644 --- a/packages/secrets/bootstrap_test.go +++ b/packages/secrets/bootstrap_test.go @@ -16,6 +16,8 @@ func getter(kv map[string]string) func(string) string { } func TestBootstrapReportsEveryProblem(t *testing.T) { + t.Parallel() + requireKey := func(get func(string) string) []error { if get(key) == "" { return []error{errors.New(key + " is required")} @@ -57,6 +59,8 @@ func TestBootstrapReportsEveryProblem(t *testing.T) { } func TestBootstrapValidatesTheSnapshot(t *testing.T) { + t.Parallel() + validate := func(get func(string) string) []error { if !strings.HasPrefix(get(key), "re_") { return []error{errors.New(key + " must start with re_")} @@ -83,6 +87,8 @@ func TestBootstrapValidatesTheSnapshot(t *testing.T) { } func TestKeyring(t *testing.T) { + t.Parallel() + synctest.Test(t, func(t *testing.T) { a, b, c := strings.Repeat("a", 32), strings.Repeat("b", 32), strings.Repeat("c", 32) f := newFake(map[string]string{"SIGNING": " " + a + "\n"}) @@ -115,7 +121,7 @@ func TestKeyring(t *testing.T) { t.Fatal("keyring values logged") } m.Stop() - time.Sleep(time.Hour) + advance(time.Hour) if _, err := m.Keyring("SIGNING"); !errors.Is(err, secrets.ErrStale) { t.Fatalf("stale keyring: %v", err) } diff --git a/packages/secrets/doppler_test.go b/packages/secrets/doppler_test.go index 026cab0..c127c72 100644 --- a/packages/secrets/doppler_test.go +++ b/packages/secrets/doppler_test.go @@ -18,6 +18,8 @@ import ( // These run the Doppler adapter over a real socket (httptest.Server). func TestDopplerDownloadOverHTTP(t *testing.T) { + t.Parallel() + f := newFake(map[string]string{key: "re_live", "DOPPLER_CONFIG": "prd"}) var gotAccept string srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -59,31 +61,40 @@ func TestDopplerDownloadOverHTTP(t *testing.T) { } func TestDopplerErrors(t *testing.T) { - f := newFake(map[string]string{key: "re_secret_in_body"}) - srv := httptest.NewServer(f) - defer srv.Close() - at := func(token string) *secrets.Doppler { - return &secrets.Doppler{Token: secrets.NewSecret(token), APIURL: srv.URL, HTTPClient: srv.Client()} + t.Parallel() + + // Each subtest gets its own fake and server, so they run in parallel. + at := func(token string) func(*httptest.Server) *secrets.Doppler { + return func(srv *httptest.Server) *secrets.Doppler { + return &secrets.Doppler{Token: secrets.NewSecret(token), APIURL: srv.URL, HTTPClient: srv.Client()} + } + } + fixed := func(d *secrets.Doppler) func(*httptest.Server) *secrets.Doppler { + return func(*httptest.Server) *secrets.Doppler { return d } } tests := []struct { name string - setup func() - d *secrets.Doppler + setup func(*fakeDoppler) + d func(*httptest.Server) *secrets.Doppler want string }{ - {"missing token", func() {}, at(""), "missing token"}, - {"bad token", func() {}, at("dp.st.wrong_token"), "token rejected (401 Unauthorized)"}, - {"server error", func() { f.fail(500, "") }, at(testToken), "unexpected status 500"}, - {"oversize", func() { f.setRaw(`{"A":"` + strings.Repeat("x", 1<<20) + `"}`) }, at(testToken), "larger than"}, - {"unreachable", func() {}, &secrets.Doppler{Token: secrets.NewSecret(testToken), APIURL: "http://user:pass@127.0.0.1:1"}, "doppler: request"}, - {"bad url", func() {}, &secrets.Doppler{Token: secrets.NewSecret(testToken), APIURL: "http://[::1"}, "build request"}, + {"missing token", func(*fakeDoppler) {}, at(""), "missing token"}, + {"bad token", func(*fakeDoppler) {}, at("dp.st.wrong_token"), "token rejected (401 Unauthorized)"}, + {"server error", func(f *fakeDoppler) { f.fail(500, "") }, at(testToken), "unexpected status 500"}, + {"oversize", func(f *fakeDoppler) { f.setRaw(`{"A":"` + strings.Repeat("x", 1<<20) + `"}`) }, at(testToken), "larger than"}, + {"unreachable", func(*fakeDoppler) {}, fixed(&secrets.Doppler{Token: secrets.NewSecret(testToken), APIURL: "http://user:pass@127.0.0.1:1"}), "doppler: request"}, + {"bad url", func(*fakeDoppler) {}, fixed(&secrets.Doppler{Token: secrets.NewSecret(testToken), APIURL: "http://[::1"}), "build request"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - f.set(map[string]string{key: "re_secret_in_body"}) - tt.setup() - _, err := tt.d.Fetch(t.Context(), secrets.Snapshot{}) + t.Parallel() + + f := newFake(map[string]string{key: "re_secret_in_body"}) + srv := httptest.NewServer(f) + defer srv.Close() + tt.setup(f) + _, err := tt.d(srv).Fetch(t.Context(), secrets.Snapshot{}) if err == nil || !strings.Contains(err.Error(), tt.want) { t.Fatalf("err = %v, want %q", err, tt.want) } @@ -97,6 +108,8 @@ func TestDopplerErrors(t *testing.T) { } func TestDopplerRateLimit(t *testing.T) { + t.Parallel() + f := newFake(map[string]string{key: "re_a"}) srv := httptest.NewServer(f) defer srv.Close() @@ -121,6 +134,8 @@ func TestDopplerRateLimit(t *testing.T) { } func TestSecretNeverPrints(t *testing.T) { + t.Parallel() + s := secrets.NewSecret("re_plain_value") var sb strings.Builder slog.New(slog.NewJSONHandler(&sb, nil)).Info("x", "s", s, "group", slog.GroupValue(slog.Any("s", s))) @@ -168,6 +183,8 @@ func TestSecretNeverPrints(t *testing.T) { } func TestDopplerDoesNotFollowRedirects(t *testing.T) { + t.Parallel() + var followed bool srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path == "/elsewhere" { diff --git a/packages/secrets/fake_test.go b/packages/secrets/fake_test.go index 4698d3f..51fa159 100644 --- a/packages/secrets/fake_test.go +++ b/packages/secrets/fake_test.go @@ -162,3 +162,9 @@ func (r *recorder) values() []string { defer r.mu.Unlock() return append([]string(nil), r.seen...) } + +// advance advances the synctest fake clock; only valid inside a synctest bubble. +// Tests never sleep in real time: outside a bubble this would block. +func advance(d time.Duration) { + time.Sleep(d) //nolint:forbidigo // synctest: advances the fake clock, no real time passes +} diff --git a/packages/secrets/manager_test.go b/packages/secrets/manager_test.go index e81bec8..b284821 100644 --- a/packages/secrets/manager_test.go +++ b/packages/secrets/manager_test.go @@ -15,8 +15,9 @@ import ( const key = "RESEND_API_KEY" -// Every test runs in a synctest bubble: time.Sleep advances a fake clock once -// the Manager's goroutine is idle, so polling is exercised without real waits. +// Every test runs in a synctest bubble: advance (time.Sleep) moves a fake +// clock once the Manager's goroutine is idle, so polling is exercised without +// real waits. func start(t *testing.T, p secrets.Provider, o secrets.Options) *secrets.Manager { t.Helper() @@ -38,6 +39,8 @@ func get(t *testing.T, m *secrets.Manager, name string) string { } func TestRotationPickedUpWithinOneInterval(t *testing.T) { + t.Parallel() + synctest.Test(t, func(t *testing.T) { f := newFake(map[string]string{key: "re_old", "OTHER": "x"}) log, _ := newLogger() @@ -53,7 +56,7 @@ func TestRotationPickedUpWithinOneInterval(t *testing.T) { } f.set(map[string]string{key: "re_new"}) - time.Sleep(secrets.DefaultInterval + secrets.DefaultInterval/10) // one interval plus max jitter + advance(secrets.DefaultInterval + secrets.DefaultInterval/10) // one interval plus max jitter synctest.Wait() if got := get(t, m, key); got != "re_new" { @@ -68,7 +71,7 @@ func TestRotationPickedUpWithinOneInterval(t *testing.T) { } // Unchanged config: conditional request, 304, no hook call. - time.Sleep(2 * (secrets.DefaultInterval + secrets.DefaultInterval/10)) + advance(2 * (secrets.DefaultInterval + secrets.DefaultInterval/10)) synctest.Wait() if _, notMod := f.stats(); notMod != 2 { t.Fatalf("304s = %d, want 2", notMod) @@ -80,13 +83,15 @@ func TestRotationPickedUpWithinOneInterval(t *testing.T) { } func TestProviderDownKeepsLastGoodUntilMaxStaleness(t *testing.T) { + t.Parallel() + synctest.Test(t, func(t *testing.T) { f := newFake(map[string]string{key: "re_good"}) log, logs := newLogger() m := start(t, dopplerFor(f), secrets.Options{Required: []string{key}, Logger: log}) f.fail(503, "") - time.Sleep(secrets.DefaultMaxStaleness - time.Minute) + advance(secrets.DefaultMaxStaleness - time.Minute) synctest.Wait() if got := get(t, m, key); got != "re_good" { t.Fatalf("during outage Get = %q", got) @@ -98,7 +103,7 @@ func TestProviderDownKeepsLastGoodUntilMaxStaleness(t *testing.T) { t.Fatalf("status during outage = %+v", st) } - time.Sleep(2 * time.Minute) + advance(2 * time.Minute) synctest.Wait() if err := m.Healthy(); !errors.Is(err, secrets.ErrStale) { t.Fatalf("Healthy past max staleness = %v, want ErrStale", err) @@ -110,7 +115,7 @@ func TestProviderDownKeepsLastGoodUntilMaxStaleness(t *testing.T) { t.Fatal("staleness not reported in status") } // The next failed poll (within MaxBackoff) logs it as an error. - time.Sleep(secrets.DefaultMaxBackoff + secrets.DefaultMaxBackoff/10) + advance(secrets.DefaultMaxBackoff + secrets.DefaultMaxBackoff/10) synctest.Wait() if !strings.Contains(logs.String(), "past max staleness") { t.Fatal("staleness not logged") @@ -118,7 +123,7 @@ func TestProviderDownKeepsLastGoodUntilMaxStaleness(t *testing.T) { // Backoff is bounded: recovery is seen within MaxBackoff (+ jitter). f.recover() - time.Sleep(secrets.DefaultMaxBackoff + secrets.DefaultMaxBackoff/10) + advance(secrets.DefaultMaxBackoff + secrets.DefaultMaxBackoff/10) synctest.Wait() if err := m.Healthy(); err != nil { t.Fatalf("after recovery: %v", err) @@ -130,6 +135,8 @@ func TestProviderDownKeepsLastGoodUntilMaxStaleness(t *testing.T) { } func TestInvalidSnapshotRejected(t *testing.T) { + t.Parallel() + tests := []struct { name, raw, want string }{ @@ -144,6 +151,8 @@ func TestInvalidSnapshotRejected(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Parallel() + synctest.Test(t, func(t *testing.T) { f := newFake(map[string]string{key: "re_good"}) log, logs := newLogger() @@ -171,7 +180,7 @@ func TestInvalidSnapshotRejected(t *testing.T) { t.Fatalf("invalid snapshot was published: %q", got) } // The poll loop keeps rejecting it without publishing. - time.Sleep(secrets.DefaultInterval + secrets.DefaultInterval/10) + advance(secrets.DefaultInterval + secrets.DefaultInterval/10) synctest.Wait() if got := rec.values(); len(got) != 1 || get(t, m, key) != "re_good" { t.Fatalf("hook saw %v", got) @@ -185,13 +194,15 @@ func TestInvalidSnapshotRejected(t *testing.T) { } func TestRetryAfterIsHonored(t *testing.T) { + t.Parallel() + synctest.Test(t, func(t *testing.T) { f := newFake(map[string]string{key: "re_a"}) log, _ := newLogger() start(t, dopplerFor(f), secrets.Options{Logger: log}) f.fail(429, "120") - time.Sleep(10 * time.Minute) + advance(10 * time.Minute) synctest.Wait() times := f.requestTimes() // [Start, first poll (429), retry, ...] if len(times) < 3 { @@ -204,13 +215,15 @@ func TestRetryAfterIsHonored(t *testing.T) { } func TestBackoffIsBounded(t *testing.T) { + t.Parallel() + synctest.Test(t, func(t *testing.T) { f := newFake(map[string]string{key: "re_a"}) log, _ := newLogger() start(t, dopplerFor(f), secrets.Options{Logger: log, MaxBackoff: time.Minute, MaxStaleness: time.Hour}) f.fail(503, "") - time.Sleep(30 * time.Minute) + advance(30 * time.Minute) synctest.Wait() times := f.requestTimes() var gaps []time.Duration @@ -230,6 +243,8 @@ func TestBackoffIsBounded(t *testing.T) { } func TestStartFailsWithoutValidSnapshot(t *testing.T) { + t.Parallel() + synctest.Test(t, func(t *testing.T) { f := newFake(map[string]string{"OTHER": "x"}) log, _ := newLogger() @@ -268,6 +283,8 @@ func TestStartFailsWithoutValidSnapshot(t *testing.T) { } func TestHooks(t *testing.T) { + t.Parallel() + synctest.Test(t, func(t *testing.T) { f := newFake(map[string]string{key: "re_1", "OPTIONAL": "o"}) log, logs := newLogger() @@ -305,6 +322,8 @@ func TestHooks(t *testing.T) { } func TestStopEndsPolling(t *testing.T) { + t.Parallel() + synctest.Test(t, func(t *testing.T) { f := newFake(map[string]string{key: "re_a"}) log, _ := newLogger() @@ -315,7 +334,7 @@ func TestStopEndsPolling(t *testing.T) { t.Fatalf("values not readable after Stop: %q", got) } calls, _ := f.stats() - time.Sleep(time.Hour) + advance(time.Hour) synctest.Wait() if got, _ := f.stats(); got != calls { t.Fatalf("polled after Stop: %d calls, want %d", got, calls) @@ -335,6 +354,8 @@ func TestStopEndsPolling(t *testing.T) { } func TestEnvIsStartupOnly(t *testing.T) { + t.Parallel() + synctest.Test(t, func(t *testing.T) { log, _ := newLogger() m := secrets.NewManager(secrets.NewEnv(map[string]string{key: "re_env"}), secrets.Options{Required: []string{key}, Logger: log}) @@ -345,7 +366,7 @@ func TestEnvIsStartupOnly(t *testing.T) { t.Fatal(err) } m.Stop() - time.Sleep(24 * time.Hour) + advance(24 * time.Hour) if err := m.Healthy(); err != nil { t.Fatalf("env went stale: %v", err) } @@ -361,6 +382,8 @@ func TestEnvIsStartupOnly(t *testing.T) { } func TestNoSecretValuesInLogs(t *testing.T) { + t.Parallel() + synctest.Test(t, func(t *testing.T) { values := []string{"re_first_value", "re_second_value", "re_third_value", testToken} f := newFake(map[string]string{key: values[0]}) @@ -376,13 +399,13 @@ func TestNoSecretValuesInLogs(t *testing.T) { t.Cleanup(m.Stop) f.set(map[string]string{key: values[1]}) // rotation - time.Sleep(time.Minute + 6*time.Second) + advance(time.Minute + 6*time.Second) f.setRaw(`{"RESEND_API_KEY": ["` + values[2] + `"]}`) // malformed - time.Sleep(time.Minute + 6*time.Second) + advance(time.Minute + 6*time.Second) f.fail(500, "") // the fake echoes the config in its error body - time.Sleep(20 * time.Minute) + advance(20 * time.Minute) f.fail(401, "") - time.Sleep(10 * time.Minute) + advance(10 * time.Minute) synctest.Wait() out := logs.String() @@ -403,6 +426,8 @@ func TestNoSecretValuesInLogs(t *testing.T) { } func TestFailedHookIsRetriedAfterTheNextFetch(t *testing.T) { + t.Parallel() + synctest.Test(t, func(t *testing.T) { f := newFake(map[string]string{key: "re_1"}) log, _ := newLogger() @@ -417,11 +442,11 @@ func TestFailedHookIsRetriedAfterTheNextFetch(t *testing.T) { return flaky.hook(v) }) - time.Sleep(secrets.DefaultInterval + secrets.DefaultInterval/10) // unchanged poll: retry #1 fails + advance(secrets.DefaultInterval + secrets.DefaultInterval/10) // unchanged poll: retry #1 fails synctest.Wait() - time.Sleep(secrets.DefaultInterval + secrets.DefaultInterval/10) // retry #2 succeeds + advance(secrets.DefaultInterval + secrets.DefaultInterval/10) // retry #2 succeeds synctest.Wait() - time.Sleep(secrets.DefaultInterval + secrets.DefaultInterval/10) // nothing left to retry + advance(secrets.DefaultInterval + secrets.DefaultInterval/10) // nothing left to retry synctest.Wait() if got := flaky.values(); !slices.Equal(got, []string{"re_1"}) || calls != 3 { t.Fatalf("flaky hook calls=%d saw %v", calls, got) diff --git a/packages/secrets/redact_test.go b/packages/secrets/redact_test.go index e4604b5..cf03229 100644 --- a/packages/secrets/redact_test.go +++ b/packages/secrets/redact_test.go @@ -36,6 +36,8 @@ type stringer struct{ s string } func (s *stringer) String() string { return s.s } func TestRedactingHandlerKeys(t *testing.T) { + t.Parallel() + for _, key := range []string{ "password", "new_password", "Passwd", "passphrase", "secret", "AUTH_SECRET", "client-secret", "token", "session_token", "accessToken", "refresh.token", "csrfToken", "api_key", "apiKey", "X-API-Key", "APIKey", "apikey", @@ -75,6 +77,8 @@ func TestRedactingHandlerKeys(t *testing.T) { // Names that mention a credential without holding one stay readable, as an // attribute key, as name=value in a message, and inside error text. func TestRedactingHandlerFalsePositives(t *testing.T) { + t.Parallel() + for _, name := range []string{ // Facts about a credential: a word after it marks metadata. "set_password_page", "password_page", "reset_password_path", "token_ttl", "secret_name", @@ -101,6 +105,8 @@ func TestRedactingHandlerFalsePositives(t *testing.T) { } func TestRedactString(t *testing.T) { + t.Parallel() + for _, tt := range []struct{ in, want string }{ {"Authorization: Bearer abc.def-ghi", "Authorization: [redacted]"}, {"sent Bearer eyJhbGciOi.x_y== to upstream", "sent Bearer [redacted] to upstream"}, @@ -146,6 +152,8 @@ func TestRedactString(t *testing.T) { } func TestRedactingHandlerValues(t *testing.T) { + t.Parallel() + link := "https://a.example/api/auth/reset-password/tok123?callbackURL=%2F" out := redactLog(t, secrets.RedactOptions{}, func(l *slog.Logger) { l.Info("dial postgres://u:pw1@h/db", // the message too @@ -180,6 +188,8 @@ func TestRedactingHandlerValues(t *testing.T) { } func TestRedactingHandlerWithAttrsAndGroup(t *testing.T) { + t.Parallel() + out := redactLog(t, secrets.RedactOptions{}, func(l *slog.Logger) { l.With("token", "tok1", "dsn", "postgres://u:pw2@h").WithGroup("g").Info("m", "access_token", "tok3", "ok", "visible") }) @@ -198,6 +208,8 @@ func TestRedactingHandlerWithAttrsAndGroup(t *testing.T) { } func TestRedactingHandlerDevMailLinks(t *testing.T) { + t.Parallel() + link := "http://dev-box:3000/api/auth/reset-password/tok123?callbackURL=%2Freset" mail := func(l *slog.Logger) { // The exact shape packages/auth DevMailer logs. @@ -219,6 +231,8 @@ func TestRedactingHandlerDevMailLinks(t *testing.T) { }, false}, } { t.Run(tt.name, func(t *testing.T) { + t.Parallel() + out := redactLog(t, tt.opts, tt.do) if got := strings.Count(out, "tok123") == 2; got != tt.wantLink { t.Errorf("link readable = %v, want %v: %s", got, tt.wantLink, out) @@ -233,6 +247,8 @@ func TestRedactingHandlerDevMailLinks(t *testing.T) { // Maps log as groups and structs by their JSON, so a logged header set or // session never carries its credentials. func TestRedactingHandlerMapsAndStructs(t *testing.T) { + t.Parallel() + type session struct { ID string `json:"id"` Token string `json:"token"` diff --git a/packages/secrets/reveal_test.go b/packages/secrets/reveal_test.go index 69169c3..0004353 100644 --- a/packages/secrets/reveal_test.go +++ b/packages/secrets/reveal_test.go @@ -11,6 +11,8 @@ import ( ) func TestRevealValue(t *testing.T) { + t.Parallel() + if got := reveal.Value(secrets.NewSecret("re_round_trip")); got != "re_round_trip" { t.Fatalf("round trip: %q", got) } @@ -27,6 +29,8 @@ func TestRevealValue(t *testing.T) { // Keyring) could be called through an interface, outside the reach of // depguard. func TestNoMethodReturnsTheValue(t *testing.T) { + t.Parallel() + const value = "re_method_probe" sec := secrets.NewSecret(value) for _, s := range []reflect.Value{ From cd580b826bf77cefbf152d81ca18bb8bee5e1921 Mon Sep 17 00:00:00 2001 From: JJ Eaton <58495321+jayleaton@users.noreply.github.com> Date: Thu, 22 Oct 2026 18:50:09 +0700 Subject: [PATCH 03/10] perf(ci): test-time budgets, slowest tests in every run, probes reuse the test profile make test and CI pipe go test -json through scripts/testbudget: plain go test output, the slowest tests, and a failure when a package runs over 30s or a leaf test over 5s (make lint-tests proves both trip). The coverage probes plant their uncovered statements in the real profile instead of recompiling three packages under -race (CI runs them after the tests). AGENTS.md "Fast tests" documents the rules; make bench-devloop the measurements. Co-Authored-By: Claude Opus 5.5 --- .github/actions/check/action.yml | 19 +++++-- AGENTS.md | 15 +++++- Makefile | 10 ++-- README.md | 3 +- scripts/check-test-probes.sh | 92 ++++++++++++++++++++------------ scripts/testbudget/main.go | 30 ++++++++--- 6 files changed, 117 insertions(+), 52 deletions(-) diff --git a/.github/actions/check/action.yml b/.github/actions/check/action.yml index 294713a..b3b7fe8 100644 --- a/.github/actions/check/action.yml +++ b/.github/actions/check/action.yml @@ -105,10 +105,6 @@ runs: run: ./scripts/check-layering.sh env: GOLANGCI_LINT: golangci-lint - - name: Test boundary probes - if: inputs.mode != 'db' && inputs.full == 'true' - shell: bash - run: ./scripts/check-test-probes.sh # ---- tests ---- # With -coverprofile go test also compiles packages without tests (lint @@ -142,9 +138,22 @@ runs: chmod +x "$RUNNER_TEMP/wait-db" exec_flag=(-exec "$RUNNER_TEMP/wait-db") fi - go test -race "${exec_flag[@]}" -coverprofile=.cache/coverage.out $PACKAGES # word splitting intended + # testbudget prints plain go test output, the slowest tests, and fails + # a package or test over its time budget (AGENTS.md "Fast tests"). + set -o pipefail + go test -race -json "${exec_flag[@]}" -coverprofile=.cache/coverage.out $PACKAGES | # word splitting intended + go run ./scripts/testbudget TESTED="$PACKAGES" ./scripts/check-coverage.sh .cache/coverage.out + # After the tests: the coverage probes plant their uncovered statements in + # the run's profile (this job tested every probed package on a full run). + - name: Test boundary probes + if: inputs.mode != 'db' && inputs.full == 'true' + shell: bash + run: ./scripts/check-test-probes.sh + env: + PROFILE: .cache/coverage.out + - uses: actions/cache/save@v6 if: inputs.save == 'true' && inputs.packages != '' && inputs.mode == 'all' with: diff --git a/AGENTS.md b/AGENTS.md index fbeb1f4..377bacb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -67,7 +67,7 @@ The most common defect is a change that works on the path you tested and is miss - CI layout: jobs bill rounded up to whole minutes, so CI is ONE job (`ci`): affected-only generators, gofmt, golangci-lint and race tests (all packages on pushes to main, the merge queue and manual runs), plus the mobile app and `deploy/` module checks when those change. Docs-only PRs post `ci-ok` from that job with nothing to check; drafts start no jobs. Sharding is an opt-in fallback only: label a PR `ci:fanout` (takes effect on its next push) or run the workflow manually with `fanout`, which splits into `lint + test (db)` and `test (other)` for faster wall time at more billed minutes. Timeouts are ~2x measured averages (see the workflow comments). Shared steps live in `.github/actions/check`. Avoid adding jobs: fold new checks into the existing one. A push to main whose tree is identical to a PR merge commit that already passed CI is not tested again: `ci-ok` is posted from that result (`scripts/ci-reuse.sh`; PR results record the tested tree in their `ci-ok` description). Caches: modules keyed by go.sum; one build/lint cache keyed by commit, saved by main pushes that run and by a scheduled run every 4 hours (skipped when main has not moved), restored by prefix elsewhere. - Run the narrowest thing first: `go test ./apps/web/internal/services/...`. - Behaviour changes ship with tests at the boundaries below, and only there. -- A test that needs a sleep or timeout to pass is wrong. +- A test that needs a sleep or timeout to pass is wrong. See "Fast tests". - Test memory: `auth.New` hashes passwords with cheap argon2 settings inside test binaries (`testing.Testing()`), so suites that sign users up stay small under `-race`; only `TestPasswordHashDefaults` exercises production cost. A full cold `make test` peaks around 2 GB, almost all of it compiling, so don't lower `-p`. If a test or compile dies with `signal: killed`, check the cgroup's `memory.events`: `oom_kill 0` means something outside the repo sent the kill (another job on a shared machine), not a memory limit. - User-visible UI changes get one pass in a real browser, over the LAN or Tailscale URL when origins matter. @@ -91,6 +91,19 @@ The allowlist in `scripts/check-test-boundaries.sh` names the files that have no Mocks are generated, never written: `//go:generate go tool mockgen -source=.go -destination=mocks/.go -package=mocks` next to the interfaces, `make generate`, commit the output (CI fails on a stale mock). `mockgen` is a `tool` in the root `go.mod` so it always matches the `go.uber.org/mock` version the mocks import. +## Fast tests + +The suite runs on every change it can affect, under `-race` on 2-vCPU CI runners, so its speed is a feature that has to survive growth. Lint and `make test` enforce these; `make lint-tests` proves the budgets trip. + +- **Parallel.** Every test starts with `t.Parallel()` (`paralleltest`). Slow table tests make their cases parallel subtests too; `tparallel` keeps a test and its subtests consistent. Never share a gomock controller, `httptest.Server` or recorder across parallel subtests; build them per case. A test that must touch process-wide state stays serial with `//nolint:paralleltest // `; prefer removing the global instead (the crawl tests used to share `crawlMethod`). +- **No sleeps.** `time.Sleep` in a test fails lint (forbidigo). Wait on a channel, a condition or `synctest.Wait`; for intervals, expiry and backoff use `testing/synctest` (its fake clock: `packages/secrets` `advance`) or an injected clock (`auth`'s `testClock`). +- **One database per test, cloned.** Repository tests use `dbtest.New(t, template)`: a `CREATE DATABASE … TEMPLATE` clone of a database migrated once per server, so tests never migrate and never depend on each other's rows. `packages/auth` does the same in its `TestMain` (`newDatabase`); tests on its shared database use unique data (`uniqueEmail`). +- **Cheap crypto.** `auth.New` uses cheap argon2 inside test binaries; only `TestPasswordHashDefaults` pays production cost. +- **Table tests.** One test with a case table, not a copy per case. +- **Do expensive checks once.** Work that depends only on immutable input (a static asset's body in the crawl) is checked once per distinct input (`checkBody`), not once per request. +- **Budgets.** `make test` and CI pipe `go test -json` through `scripts/testbudget`: plain `go test` output, then the slowest tests, then a failure if a package's test binary runs over 30 s or a single test (a leaf test or subtest) over 5 s, measured on CI under `-race`. Over budget means split or speed up the test. Raise a budget (constants in `scripts/testbudget`) only with the reason in the commit. +- **Measure, don't guess.** `make bench-devloop` prints the timing tables (build, generate, lint per linter, probes, tests per package with and without `-race`); `BENCH_COLD=1` adds cold caches. The "Bench dev loop" workflow runs it on a CI-shaped runner: push the base and the change to `bench/` branches and compare their summaries. Delete the branches afterwards. + ## Admin app `apps/admin` is optional and copies the web app's layers exactly: `packages/database/adminrepo` (sqlc) and adapters over `packages/auth` → `apps/admin/internal/services` → `apps/admin/internal/api` (`/api/admin/v1`, not mounted by `cmd/admin`: the UI calls the services in-process) and `apps/admin/ui`, wired in `apps/admin/internal/server` and `apps/admin/cmd/admin` (`admin serve | migrate | invite `). Config, routes and validators are its own (`apps/admin/internal/{config,validate}`). diff --git a/Makefile b/Makefile index 7efaafc..668a2ee 100644 --- a/Makefile +++ b/Makefile @@ -72,16 +72,20 @@ lint-layering: ## Prove forbidden imports (code and tests) fail lint lint-tests: ## Prove misplaced test files and coverage drops fail ./scripts/check-test-probes.sh +# go test -json through scripts/testbudget: plain go test output, the slowest +# tests, and a failure when a package or test is over its time budget. +GOTEST := set -o pipefail; $(GO) test -race -json -coverprofile=.cache/coverage.out ./... | $(GO) run ./scripts/testbudget + .PHONY: test -test: ## go test -race against this checkout's throwaway Postgres (or TEST_DATABASE_URL), then coverage floors +test: ## go test -race against this checkout's throwaway Postgres (or TEST_DATABASE_URL), then time budgets and coverage floors @mkdir -p .cache ifdef TEST_DATABASE_URL - $(GO) test -race -coverprofile=.cache/coverage.out ./... + bash -c '$(GOTEST)' else @$(MAKE) --no-print-directory test-db-up @port=$$($(COMPOSE) port postgres-test 5432 | sed 's/.*://'); \ echo "test db: 127.0.0.1:$$port (project $(COMPOSE_PROJECT_NAME))"; \ - TEST_DATABASE_URL="postgres://postgres:postgres@127.0.0.1:$$port/postgres?sslmode=disable" $(GO) test -race -coverprofile=.cache/coverage.out ./... + TEST_DATABASE_URL="postgres://postgres:postgres@127.0.0.1:$$port/postgres?sslmode=disable" bash -c '$(GOTEST)' endif ./scripts/check-coverage.sh .cache/coverage.out diff --git a/README.md b/README.md index 951325d..221b81b 100644 --- a/README.md +++ b/README.md @@ -47,12 +47,13 @@ Each checkout gets its own compose project and database ports, derived from its | | | |---|---| | `make dev` | run the server with `.env` | -| `make test` | `go test -race ./...` against this checkout's throwaway tmpfs Postgres (or `TEST_DATABASE_URL`), then per-boundary coverage floors | +| `make test` | `go test -race ./...` against this checkout's throwaway tmpfs Postgres (or `TEST_DATABASE_URL`), then per-package and per-test time budgets and per-boundary coverage floors | | `make lint` | gofmt + golangci-lint (layering and test-import rules) + test file locations | | `make lint-layering` / `make lint-tests` | prove the layering, test-location and coverage rules still reject planted violations | | `make ci` | everything CI runs | | `make generate` | sqlc, mocks (`go generate`) and templ code generation | | `make css` | rebuild both apps' `app.css` (pinned Tailwind standalone CLI) | +| `make bench-devloop` | timing tables for the dev loop: build per binary (with size and cold start), generators, lint per linter, probes, tests per package with and without `-race`; `BENCH_COLD=1` adds cold caches | | `make build` / `make docker` | static binary / production image | | `make db-up` / `make db-down` | start / stop the local databases | | `make env` | print this checkout's compose project, ports and `DATABASE_URL` | diff --git a/scripts/check-test-probes.sh b/scripts/check-test-probes.sh index fece881..331594a 100755 --- a/scripts/check-test-probes.sh +++ b/scripts/check-test-probes.sh @@ -1,9 +1,12 @@ #!/usr/bin/env bash # Proves the test-boundary checks still bite: the clean tree passes # check-test-boundaries.sh, planted test files outside the boundaries are named -# by it, and planted uncovered statements sink services and the UI below their -# floors in check-coverage.sh. Probes are always removed. (depguard rules for test files -# are proven by check-layering.sh.) +# by it, planted uncovered statements sink services and the UI below their +# floors in check-coverage.sh, and scripts/testbudget fails a slow package and +# a slow test. Probes are always removed. (depguard rules for test files are +# proven by check-layering.sh.) +# PROFILE= reuses it +# (CI passes its test run's); otherwise the probed packages are tested here. set -euo pipefail cd "$(dirname "$0")/.." @@ -63,42 +66,61 @@ else fi rm -f "$nested" -# Coverage: ~200 uncovered statements sink each app's services floor. -profile=.cache/probe.out +# Coverage: a file of uncovered statements, added to a real profile, sinks +# each floor (~200 sink each app's services; apps/web/ui is ~3800 statements, +# so 300 drop it ~3 points). go test counts every statement of a package, so +# a planted profile block is what a planted untested file would produce. +probed=(apps/web/internal/services:200 apps/admin/internal/services:200 apps/web/ui:300) +profile=${PROFILE:-} mkdir -p .cache -paths+=("$profile") -for svc in apps/web/internal/services apps/admin/internal/services; do - probe=$svc/zz_probe.go - paths+=("$probe") - { - printf 'package services\n\nfunc zzCoverageProbe(x int) int {\n' - for i in $(seq 200); do printf '\tx += %d\n' "$i"; done - printf '\treturn x\n}\n' - } > "$probe" - if ! log=$(go test -race -coverprofile="$profile" "./$svc/" 2>&1); then - fail "go test on the coverage probe failed ($svc)"; echo "$log" - elif out=$(./scripts/check-coverage.sh "$profile" "$mod/$svc"); then - fail "201 uncovered statements did not fail the $svc floor: $out" +if [ -z "$profile" ]; then + profile=.cache/probe-clean.out + paths+=("$profile") + pkgs=() + for p in "${probed[@]}"; do pkgs+=("./${p%:*}"); done + if ! log=$(go test -coverprofile="$profile" "${pkgs[@]}" 2>&1); then + fail "go test on the probed packages failed"; echo "$log" + fi +fi +planted_profile=.cache/probe.out +paths+=("$planted_profile") +for p in "${probed[@]}"; do + dir=${p%:*} n=${p#*:} + if ! out=$(./scripts/check-coverage.sh "$profile" "$mod/$dir"); then + fail "$dir is below its floor before the probe: $out" + continue + fi + { cat "$profile"; printf '%s/zz_probe.go:1.1,%d.2 %d 0\n' "$mod/$dir" "$n" "$n"; } >"$planted_profile" + if out=$(./scripts/check-coverage.sh "$planted_profile" "$mod/$dir"); then + fail "$n uncovered statements did not fail the $dir floor: $out" else - pass "201 uncovered statements fail the $svc floor: ${out#FAIL }" + pass "$n uncovered statements fail the $dir floor: ${out#FAIL }" fi - rm -f "$probe" done -# The UI floor (apps/web/ui is ~3800 statements; 300 uncovered drop it ~3 points). -uiprobe=apps/web/ui/zz_probe.go -paths+=("$uiprobe") -{ - printf 'package ui\n\nfunc zzCoverageProbe(x int) int {\n' - for i in $(seq 300); do printf '\tx += %d\n' "$i"; done - printf '\treturn x\n}\n' -} > "$uiprobe" -if ! log=$(go test -race -coverprofile="$profile" ./apps/web/ui/ 2>&1); then - fail "go test on the UI coverage probe failed"; echo "$log" -elif out=$(./scripts/check-coverage.sh "$profile" "$mod/apps/web/ui"); then - fail "301 uncovered statements did not fail the UI floor: $out" -else - pass "301 uncovered statements fail the UI floor: ${out#FAIL }" -fi +# Test-time budgets (testbudget's defaults, which CI uses): a slow package +# and a slow test fail, a fast run passes. +budget=$(mktemp -d) +dirs+=("$budget") +go build -o "$budget/testbudget" ./scripts/testbudget +paths+=("$budget/testbudget") +run_event() { printf '{"Action":"%s","Package":"%s/zz_probe","Test":"%s","Elapsed":%s}\n' "$1" "$mod" "$2" "$3"; } +fast=$( { run_event pass TestFast 0.1; run_event pass "" 0.2; } | "$budget/testbudget" -top 0 2>&1) && + pass "a fast run passes the test-time budget" || { fail "a fast run fails the test-time budget"; echo "$fast"; } +# A 1s package holding a 900s subtest trips the test budget (its parent is +# not judged: it includes its subtests); a 900s package +# trips the package budget. +slow_test() { run_event pass TestSlow/case 900; run_event pass TestSlow 900; run_event pass "" 1; } +slow_package() { run_event pass "" 900; } +for kind in test package; do + if out=$("slow_$kind" | "$budget/testbudget" -top 0 2>&1); then + fail "a 900s $kind passed the test-time budget" + elif grep -q "^ $kind $mod/zz_probe" <<<"$out"; then + pass "a 900s $kind fails the test-time budget" + else + fail "testbudget did not name the slow $kind: $out" + fi +done +rm -f "$budget/testbudget" exit "$failed" diff --git a/scripts/testbudget/main.go b/scripts/testbudget/main.go index 2ab0b49..4b74499 100644 --- a/scripts/testbudget/main.go +++ b/scripts/testbudget/main.go @@ -1,8 +1,10 @@ // Command testbudget keeps the test suite fast as it grows. It reads // `go test -json` on stdin, prints what plain `go test` would (ok/FAIL lines, // the output of failed tests, build errors), lists the slowest tests, and -// fails when a package's test binary or a single top-level test runs longer -// than its budget. `make test` and CI pipe every run through it. +// fails when a package's test binary or a single test runs longer than its +// budget. A single test is a leaf: a test without subtests, or one subtest +// (a parent's time includes its parallel subtests; the package budget +// bounds their sum). `make test` and CI pipe every run through it. // // go test -race -json ./... | go run ./scripts/testbudget // @@ -50,7 +52,7 @@ func main() { os.Exit(table(os.Stdout, os.Args[2:])) } pkgBudget := flag.Duration("pkg", defaultPackageBudget, "budget for one package's test binary (0: off)") - testBudget := flag.Duration("test", defaultTestBudget, "budget for one top-level test (0: off)") + testBudget := flag.Duration("test", defaultTestBudget, "budget for one leaf test or subtest (0: off)") top := flag.Int("top", 10, "list this many slowest tests at the end") flag.Parse() os.Exit(run(os.Stdin, os.Stdout, *pkgBudget, *testBudget, *top)) @@ -102,7 +104,7 @@ func run(in io.Reader, out io.Writer, pkgBudget, testBudget time.Duration, top i break } if e.Test != "" { - if e.Action == "pass" && !strings.Contains(e.Test, "/") { + if e.Action == "pass" { tests = append(tests, result{e.Package + " " + e.Test, d}) } break @@ -124,6 +126,7 @@ func run(in io.Reader, out io.Writer, pkgBudget, testBudget time.Duration, top i } byTime := func(a, b result) int { return cmp.Compare(b.elapsed, a.elapsed) } + tests = leaves(tests) slices.SortFunc(tests, byTime) slices.SortFunc(pkgs, byTime) if top > 0 && len(tests) > 0 { @@ -159,6 +162,19 @@ func overBudget(rs []result, budget time.Duration, kind string) []string { return over } +// leaves drops tests that have subtests ("pkg Test" when "pkg Test/x" ran). +func leaves(rs []result) []result { + parents := map[string]bool{} + for _, r := range rs { + for i := len(r.name) - 1; i > 0; i-- { + if r.name[i] == '/' { + parents[r.name[:i]] = true + } + } + } + return slices.DeleteFunc(rs, func(r result) bool { return parents[r.name] }) +} + func seconds(s float64) time.Duration { return time.Duration(s * float64(time.Second)) } // table prints a markdown table of per-package test time, one column per @@ -197,7 +213,7 @@ func table(out io.Writer, args []string) int { seen[e.Package] = true order = append(order, e.Package) } - } else if !strings.Contains(e.Test, "/") { + } else { c.tests = append(c.tests, result{e.Package + " " + e.Test, seconds(e.Elapsed)}) } } @@ -234,9 +250,9 @@ func table(out io.Writer, args []string) int { } fmt.Fprintln(out) - tests := cols[0].tests + tests := leaves(cols[0].tests) slices.SortFunc(tests, func(a, b result) int { return cmp.Compare(b.elapsed, a.elapsed) }) - fmt.Fprintf(out, "\nSlowest top-level tests (%s):\n\n| test | time |\n|---|--:|\n", cols[0].name) + fmt.Fprintf(out, "\nSlowest tests (leaf tests and subtests, %s):\n\n| test | time |\n|---|--:|\n", cols[0].name) for _, t := range tests[:min(15, len(tests))] { fmt.Fprintf(out, "| %s | %.2fs |\n", strings.TrimPrefix(t.name, mod), t.elapsed.Seconds()) } From fb26d68d0058dce51da3142ecb1fd7bdde4677bd Mon Sep 17 00:00:00 2001 From: JJ Eaton <58495321+jayleaton@users.noreply.github.com> Date: Thu, 22 Oct 2026 19:30:45 +0700 Subject: [PATCH 04/10] fix(devloop): bench-devloop's warm test run compiles nothing The cold run uses a throwaway cache, so the warm run was compiling every -race variant; an untimed run now primes the cache first. Co-Authored-By: Claude Opus 5.5 --- scripts/bench-devloop.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/scripts/bench-devloop.sh b/scripts/bench-devloop.sh index ab2a95b..cc76f4c 100755 --- a/scripts/bench-devloop.sh +++ b/scripts/bench-devloop.sh @@ -217,11 +217,14 @@ gotest() { } say "| run | cold (compile + run) | warm (run only) |" say "|---|--:|--:|" +# Warm = run only: an untimed run first compiles everything into the cache. c=- if [ -n "$cold" ]; then c=$(cold gotest cold-race -race -coverprofile="$tmp/cover.out"); fi +gotest race -race -coverprofile="$tmp/cover.out" >/dev/null say "| -race -coverprofile (CI) | $c | $(gotest race -race -coverprofile="$tmp/cover.out") |" c=- if [ -n "$cold" ]; then c=$(cold gotest cold-plain); fi +gotest plain >/dev/null say "| plain | $c | $(gotest plain) |" say "" say "Per package (test binary wall time, warm):" From 69b924c852d394d96a28bc711039adf5eadcc86b Mon Sep 17 00:00:00 2001 From: JJ Eaton <58495321+jayleaton@users.noreply.github.com> Date: Thu, 22 Oct 2026 19:31:11 +0700 Subject: [PATCH 05/10] chore(devloop): make ci hands its coverage profile to the probes, as CI does Co-Authored-By: Claude Opus 5.5 --- Makefile | 4 ++-- README.md | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Makefile b/Makefile index 668a2ee..e7a1ce1 100644 --- a/Makefile +++ b/Makefile @@ -69,7 +69,7 @@ lint-layering: ## Prove forbidden imports (code and tests) fail lint ./scripts/check-layering.sh .PHONY: lint-tests -lint-tests: ## Prove misplaced test files and coverage drops fail +lint-tests: ## Prove misplaced test files, coverage drops and slow tests fail (PROFILE= reuses a test run) ./scripts/check-test-probes.sh # go test -json through scripts/testbudget: plain go test output, the slowest @@ -96,7 +96,7 @@ ci: ## Run the same checks as CI $(MAKE) lint test @# Alone: their temporary probe files would leak into the other checks under make -j. $(MAKE) lint-layering - $(MAKE) lint-tests + $(MAKE) lint-tests PROFILE=.cache/coverage.out # the probes reuse make test's profile $(MAKE) deploy-check .PHONY: affected diff --git a/README.md b/README.md index 221b81b..1f3fb52 100644 --- a/README.md +++ b/README.md @@ -49,7 +49,7 @@ Each checkout gets its own compose project and database ports, derived from its | `make dev` | run the server with `.env` | | `make test` | `go test -race ./...` against this checkout's throwaway tmpfs Postgres (or `TEST_DATABASE_URL`), then per-package and per-test time budgets and per-boundary coverage floors | | `make lint` | gofmt + golangci-lint (layering and test-import rules) + test file locations | -| `make lint-layering` / `make lint-tests` | prove the layering, test-location and coverage rules still reject planted violations | +| `make lint-layering` / `make lint-tests` | prove the layering, test-location, coverage and test-time rules still reject planted violations | | `make ci` | everything CI runs | | `make generate` | sqlc, mocks (`go generate`) and templ code generation | | `make css` | rebuild both apps' `app.css` (pinned Tailwind standalone CLI) | From 9fa0ec32bd82ec91d4ef32c1c01cda00c5870897 Mon Sep 17 00:00:00 2001 From: JJ Eaton <58495321+jayleaton@users.noreply.github.com> Date: Thu, 22 Oct 2026 19:33:32 +0700 Subject: [PATCH 06/10] perf(test): tests added by the UI overhaul and deploy run in parallel too Co-Authored-By: Claude Opus 5.5 --- apps/admin/ui/guards_test.go | 1 + apps/web/ui/account_test.go | 1 + apps/web/ui/guards_test.go | 1 + apps/web/ui/render_test.go | 1 + deploy/platform/platform_test.go | 13 +++++++++++++ 5 files changed, 17 insertions(+) diff --git a/apps/admin/ui/guards_test.go b/apps/admin/ui/guards_test.go index 81e93f4..e072550 100644 --- a/apps/admin/ui/guards_test.go +++ b/apps/admin/ui/guards_test.go @@ -23,6 +23,7 @@ var ( // (packages/uikit/theme.css): templates use the semantic color utilities, // never raw palette classes, and dark: only to swap icons. func TestTemplatesUseThemeTokens(t *testing.T) { + t.Parallel() scanned := 0 for _, root := range []string{".", "../../../packages/uikit"} { err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { diff --git a/apps/web/ui/account_test.go b/apps/web/ui/account_test.go index ee8e7b8..f6996b6 100644 --- a/apps/web/ui/account_test.go +++ b/apps/web/ui/account_test.go @@ -343,6 +343,7 @@ func TestLiveStream(t *testing.T) { // TestLiveStreamAccountDeleted: deleting the account on another device sends // every open dashboard home. func TestLiveStreamAccountDeleted(t *testing.T) { + t.Parallel() e := newEnv(t, config.Features{}) srv := httptest.NewServer(e.h) defer srv.Close() diff --git a/apps/web/ui/guards_test.go b/apps/web/ui/guards_test.go index b41dca3..1f1859e 100644 --- a/apps/web/ui/guards_test.go +++ b/apps/web/ui/guards_test.go @@ -177,6 +177,7 @@ var ( // (packages/uikit/theme.css): templates use the semantic color utilities, // never raw palette classes, and dark: only to swap icons. func TestTemplatesUseThemeTokens(t *testing.T) { + t.Parallel() scanned := 0 for _, root := range []string{".", "../../../packages/uikit"} { err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { diff --git a/apps/web/ui/render_test.go b/apps/web/ui/render_test.go index e1b2ab6..dff95da 100644 --- a/apps/web/ui/render_test.go +++ b/apps/web/ui/render_test.go @@ -111,6 +111,7 @@ func TestWelcomeCopyMatches(t *testing.T) { // a user agent with only a browser or only a system, none at all, and a user // without a name or email. func TestSessionAndAccountFallbacks(t *testing.T) { + t.Parallel() firefox, linux := "Mozilla/5.0 Gecko/20100101 Firefox/131.0", "X11; Linux x86_64" sessions := []services.Session{{ID: "a", UserAgent: &firefox}, {ID: "b", UserAgent: &linux}, {ID: "c"}} html := renderIn(t, i18n.En, view.State{Authenticated: true}, SessionsCard(sessions, Status{})) diff --git a/deploy/platform/platform_test.go b/deploy/platform/platform_test.go index d1f391c..bf2d4ef 100644 --- a/deploy/platform/platform_test.go +++ b/deploy/platform/platform_test.go @@ -59,6 +59,7 @@ func addresses(g map[string]any) []string { } func TestCheckedInPlatformYAMLIsValid(t *testing.T) { + t.Parallel() cfg, err := platform.Load("../platform.yaml") if err != nil { t.Fatal(err) @@ -70,6 +71,7 @@ func TestCheckedInPlatformYAMLIsValid(t *testing.T) { } func TestParseDefaults(t *testing.T) { + t.Parallel() cfg := parse(t, minimal) if !slices.Equal(cfg.Environments, platform.Environments{"production"}) || cfg.Branch != "main" || cfg.Secrets.Provider != platform.ProviderDoppler { t.Fatalf("defaults: %+v", cfg) @@ -85,6 +87,7 @@ func TestParseDefaults(t *testing.T) { } func TestParseEnvironmentProfiles(t *testing.T) { + t.Parallel() for _, in := range []string{"staging,production", "[staging, production]", "staging, production"} { cfg := parse(t, minimal+"environments: "+in+"\n") if !slices.Equal(cfg.Environments, platform.Environments{"staging", "production"}) { @@ -99,6 +102,7 @@ func TestParseEnvironmentProfiles(t *testing.T) { } func TestParseReportsEveryProblem(t *testing.T) { + t.Parallel() _, err := platform.Parse([]byte(`version: 2 project: "" repo: not-a-repo @@ -120,6 +124,7 @@ unmanaged_variables: ["a/b/c"] } func TestPreviewsNeedStaging(t *testing.T) { + t.Parallel() if _, err := platform.Parse([]byte(minimal + "previews: true\n")); err == nil { t.Fatal("previews copying production accepted") } @@ -129,12 +134,14 @@ func TestPreviewsNeedStaging(t *testing.T) { } func TestParseRejectsUnknownKeys(t *testing.T) { + t.Parallel() if _, err := platform.Parse([]byte(minimal + "admn: true\n")); err == nil { t.Fatal("typo accepted") } } func TestBuildWebOnly(t *testing.T) { + t.Parallel() g := graph(t, parse(t, minimal), "production") if got := addresses(g); !slices.Equal(got, []string{"database.postgres", "service.web"}) { t.Fatalf("resources: %v", got) @@ -180,6 +187,7 @@ func TestBuildWebOnly(t *testing.T) { } func TestBuildAdmin(t *testing.T) { + t.Parallel() cfg := parse(t, minimal+"admin: true\nenvironments: staging,qa,production\nadmin_bootstrap_email: ops@acme.dev\n") for _, env := range cfg.Environments { g := graph(t, cfg, env) @@ -204,12 +212,14 @@ func TestBuildAdmin(t *testing.T) { } func TestBuildUnknownEnvironment(t *testing.T) { + t.Parallel() if _, err := parse(t, minimal).Build("staging"); err == nil { t.Fatal("built an environment not in platform.yaml") } } func TestBuildPerEnvironmentBranch(t *testing.T) { + t.Parallel() cfg := parse(t, minimal+"environments: staging,production\nbranches: { production: release }\n") if b := resource(t, graph(t, cfg, "production"), "service.web")["source"].(map[string]any)["branch"]; b != "release" { t.Fatalf("production branch %v", b) @@ -220,6 +230,7 @@ func TestBuildPerEnvironmentBranch(t *testing.T) { } func TestBuildUploadModeHasNoSource(t *testing.T) { + t.Parallel() cfg := parse(t, "version: 1\nproject: acme\nrepo: \"\"\n") if !cfg.Upload() { t.Fatal("empty repo is upload mode") @@ -234,6 +245,7 @@ func TestBuildUploadModeHasNoSource(t *testing.T) { } func TestBuildRegionPinsEveryService(t *testing.T) { + t.Parallel() g := graph(t, parse(t, minimal+"region: europe-west4\n"), "production") web := resource(t, g, "service.web")["deploy"].(map[string]any) if web["multiRegionConfig"].(map[string]any)["europe-west4"].(map[string]any)["numReplicas"] != float64(1) { @@ -246,6 +258,7 @@ func TestBuildRegionPinsEveryService(t *testing.T) { } func TestSecretVarsFollowProvider(t *testing.T) { + t.Parallel() has := func(cfg platform.Config, service, name string) bool { return slices.ContainsFunc(cfg.SecretVars(), func(s platform.Secret) bool { return s.Service == service && s.Name == name }) } From c16a597d55575b255b6fbea4d0516c4449693550 Mon Sep 17 00:00:00 2001 From: JJ Eaton <58495321+jayleaton@users.noreply.github.com> Date: Thu, 22 Oct 2026 19:45:00 +0700 Subject: [PATCH 07/10] perf(test): crawl one subtest per state; bench A/B against a base ref on one machine Per-route crawl subtests built a handler and mock set each: twice the CPU of the serial crawl, which 2 vCPUs cannot pay back. One parallel subtest per state (web: and method) keeps the setup at main's level and still uses both CPUs. Separate CI runs of identical code varied by up to 50%, so the bench gains BENCH_BASE: base and HEAD test runs alternate on the same machine and their medians are compared (the workflow compares with main). Co-Authored-By: Claude Opus 5.5 --- .github/workflows/bench-devloop.yml | 23 ++++++++--- AGENTS.md | 4 +- README.md | 2 +- apps/admin/ui/crawl_test.go | 16 ++++---- apps/web/ui/crawl_test.go | 23 +++++++---- scripts/bench-devloop.sh | 54 ++++++++++++++++++++++-- scripts/testbudget/main.go | 64 ++++++++++++++++++----------- 7 files changed, 136 insertions(+), 50 deletions(-) diff --git a/.github/workflows/bench-devloop.yml b/.github/workflows/bench-devloop.yml index 3b43ef5..2feee24 100644 --- a/.github/workflows/bench-devloop.yml +++ b/.github/workflows/bench-devloop.yml @@ -1,12 +1,21 @@ # Dev-loop timings on a CI-shaped runner (`make bench-devloop` with cold and -# warm caches): build, generate, lint per linter, probes, and tests per -# package with and without -race. The report is the run's summary page. -# Run it by hand (Actions > Bench dev loop), or push any ref to a bench/* -# branch to measure it, e.g. the base of a perf PR and its head. +# warm caches): build, generate, lint per linter, probes, tests per package +# with and without -race, and an A/B of the CI test run against a base ref on +# the same machine (separate runs land on different machines; their noise +# hides changes of tens of percent). The report is the run's summary page. +# Run it by hand (Actions > Bench dev loop), or push a branch to bench/ +# to compare it with main. name: Bench dev loop on: workflow_dispatch: + inputs: + base: + description: Ref to compare against (A/B of the test run on the same machine) + default: origin/main + rounds: + description: Alternating A/B rounds (the median is compared) + default: '3' push: branches: ['bench/**'] @@ -20,9 +29,11 @@ concurrency: jobs: bench: runs-on: ubuntu-latest - timeout-minutes: 40 + timeout-minutes: 60 env: TEST_DATABASE_URL: postgres://postgres:postgres@127.0.0.1:5432/postgres?sslmode=disable + BENCH_BASE: ${{ inputs.base || 'origin/main' }} + BENCH_ROUNDS: ${{ inputs.rounds || '3' }} steps: # Same Postgres as CI's db shard. - name: Start Postgres @@ -31,6 +42,8 @@ jobs: --tmpfs /var/lib/postgresql postgres:18-alpine \ -c fsync=off -c synchronous_commit=off -c full_page_writes=off -c max_connections=200 - uses: actions/checkout@v7 + with: + fetch-depth: 0 # the A/B base ref # Read-only: the module cache CI saves. Build and lint caches start # empty on purpose; the bench measures cold and warm itself. - uses: actions/cache/restore@v6 diff --git a/AGENTS.md b/AGENTS.md index 377bacb..a82e97f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -95,14 +95,14 @@ Mocks are generated, never written: `//go:generate go tool mockgen -source=`; prefer removing the global instead (the crawl tests used to share `crawlMethod`). +- **Parallel.** Every test starts with `t.Parallel()` (`paralleltest`). Slow table tests make their cases parallel subtests too; `tparallel` keeps a test and its subtests consistent. Keep subtests coarse enough that their setup stays small next to their work: CI has 2 vCPUs, so a split that doubles CPU time gains nothing (the crawls run one subtest per state, not per route). Never share a gomock controller, `httptest.Server` or recorder across parallel subtests; build them per case. A test that must touch process-wide state stays serial with `//nolint:paralleltest // `; prefer removing the global instead (the crawl tests used to share `crawlMethod`). - **No sleeps.** `time.Sleep` in a test fails lint (forbidigo). Wait on a channel, a condition or `synctest.Wait`; for intervals, expiry and backoff use `testing/synctest` (its fake clock: `packages/secrets` `advance`) or an injected clock (`auth`'s `testClock`). - **One database per test, cloned.** Repository tests use `dbtest.New(t, template)`: a `CREATE DATABASE … TEMPLATE` clone of a database migrated once per server, so tests never migrate and never depend on each other's rows. `packages/auth` does the same in its `TestMain` (`newDatabase`); tests on its shared database use unique data (`uniqueEmail`). - **Cheap crypto.** `auth.New` uses cheap argon2 inside test binaries; only `TestPasswordHashDefaults` pays production cost. - **Table tests.** One test with a case table, not a copy per case. - **Do expensive checks once.** Work that depends only on immutable input (a static asset's body in the crawl) is checked once per distinct input (`checkBody`), not once per request. - **Budgets.** `make test` and CI pipe `go test -json` through `scripts/testbudget`: plain `go test` output, then the slowest tests, then a failure if a package's test binary runs over 30 s or a single test (a leaf test or subtest) over 5 s, measured on CI under `-race`. Over budget means split or speed up the test. Raise a budget (constants in `scripts/testbudget`) only with the reason in the commit. -- **Measure, don't guess.** `make bench-devloop` prints the timing tables (build, generate, lint per linter, probes, tests per package with and without `-race`); `BENCH_COLD=1` adds cold caches. The "Bench dev loop" workflow runs it on a CI-shaped runner: push the base and the change to `bench/` branches and compare their summaries. Delete the branches afterwards. +- **Measure, don't guess.** `make bench-devloop` prints the timing tables (build, generate, lint per linter, probes, tests per package with and without `-race`); `BENCH_COLD=1` adds cold caches, `BENCH_PKGS` narrows the packages, and `BENCH_BASE=` adds an A/B of the CI test run against that ref on the same machine (medians of alternating rounds). Separate CI runs land on different machines and vary by tens of percent, so compare a perf change with the A/B: push it to a `bench/` branch (the "Bench dev loop" workflow compares it with main) and delete the branch afterwards. ## Admin app diff --git a/README.md b/README.md index 1f3fb52..879f15f 100644 --- a/README.md +++ b/README.md @@ -53,7 +53,7 @@ Each checkout gets its own compose project and database ports, derived from its | `make ci` | everything CI runs | | `make generate` | sqlc, mocks (`go generate`) and templ code generation | | `make css` | rebuild both apps' `app.css` (pinned Tailwind standalone CLI) | -| `make bench-devloop` | timing tables for the dev loop: build per binary (with size and cold start), generators, lint per linter, probes, tests per package with and without `-race`; `BENCH_COLD=1` adds cold caches | +| `make bench-devloop` | timing tables for the dev loop: build per binary (with size and cold start), generators, lint per linter, probes, tests per package with and without `-race`; `BENCH_COLD=1` adds cold caches, `BENCH_BASE=` an A/B of the test run against that ref on the same machine | | `make build` / `make docker` | static binary / production image | | `make db-up` / `make db-down` | start / stop the local databases | | `make env` | print this checkout's compose project, ports and `DATABASE_URL` | diff --git a/apps/admin/ui/crawl_test.go b/apps/admin/ui/crawl_test.go index c09cbd0..632d603 100644 --- a/apps/admin/ui/crawl_test.go +++ b/apps/admin/ui/crawl_test.go @@ -179,21 +179,23 @@ func TestNoUnescapedOutput(t *testing.T) { func TestCrawlExposesNothing(t *testing.T) { t.Parallel() + // One parallel subtest per state, each with its own crawler (handler and + // mock controller) that requests the routes one at a time. for _, failing := range []bool{false, true} { for _, signedIn := range []bool{false, true} { - for _, c := range crawlTargets { - t.Run(strings.Join([]string{c.pattern, state(failing, "failing", "ok"), state(signedIn, "signed-in", "anonymous")}, " "), func(t *testing.T) { - t.Parallel() - cr := crawlUI(t, failing, signedIn) + t.Run(state(failing, "failing", "ok")+"/"+state(signedIn, "signed-in", "anonymous"), func(t *testing.T) { + t.Parallel() + cr := crawlUI(t, failing, signedIn) + for _, c := range crawlTargets { method, _, _ := strings.Cut(c.pattern, " ") for _, path := range c.paths { for _, ds := range []bool{false, true} { - name := strings.Join([]string{method, path, state(ds, "datastar", "plain")}, " ") + name := strings.Join([]string{method, path, state(failing, "failing", "ok"), state(signedIn, "signed-in", "anonymous"), state(ds, "datastar", "plain")}, " ") checkResponse(t, name, method, path, cr.crawl(method, path, ds)) } } - }) - } + } + }) } } } diff --git a/apps/web/ui/crawl_test.go b/apps/web/ui/crawl_test.go index c6f0f4a..edfc05d 100644 --- a/apps/web/ui/crawl_test.go +++ b/apps/web/ui/crawl_test.go @@ -189,19 +189,24 @@ func TestNoUnescapedOutput(t *testing.T) { func TestCrawlExposesNothing(t *testing.T) { t.Parallel() + // One parallel subtest per state and method, each with one handler (and + // mock controller) built for the method it serves: notOnGET needs no + // shared state, and a handler per state keeps setup off the CPU budget. for _, failing := range []bool{false, true} { for _, signedIn := range []bool{false, true} { - for _, c := range crawlTargets { - method, _, _ := strings.Cut(c.pattern, " ") - t.Run(state(failing, "failing", "ok")+"/"+state(signedIn, "signed-in", "anonymous")+"/"+c.pattern, func(t *testing.T) { + for _, method := range []string{http.MethodGet, http.MethodPost} { + t.Run(strings.Join([]string{state(failing, "failing", "ok"), state(signedIn, "signed-in", "anonymous"), method}, "/"), func(t *testing.T) { t.Parallel() - // One handler (and mock controller) per subtest, built for - // the one method it serves: notOnGET needs no shared state. h := crawlUI(t, method, failing, signedIn) - for _, path := range c.paths { - for _, ds := range []bool{false, true} { - name := strings.Join([]string{method, path, state(failing, "failing", "ok"), state(signedIn, "signed-in", "anonymous"), state(ds, "datastar", "plain")}, " ") - checkResponse(t, name, method, path, crawl(h, method, path, ds)) + for _, c := range crawlTargets { + if m, _, _ := strings.Cut(c.pattern, " "); m != method { + continue + } + for _, path := range c.paths { + for _, ds := range []bool{false, true} { + name := strings.Join([]string{method, path, state(failing, "failing", "ok"), state(signedIn, "signed-in", "anonymous"), state(ds, "datastar", "plain")}, " ") + checkResponse(t, name, method, path, crawl(h, method, path, ds)) + } } } }) diff --git a/scripts/bench-devloop.sh b/scripts/bench-devloop.sh index cc76f4c..7a92422 100755 --- a/scripts/bench-devloop.sh +++ b/scripts/bench-devloop.sh @@ -10,13 +10,18 @@ # temp dir; your own caches are never touched). Tests that use Postgres, and # cold start, need TEST_DATABASE_URL (`make bench-devloop` starts the test # database) and psql. BENCH_ONLY=build,generate,lint,probes,tests picks -# sections. Appends the report to $GITHUB_STEP_SUMMARY when set. +# sections, BENCH_PKGS the packages to test (default ./...). BENCH_BASE= adds an A/B of the CI test run (go test +# -race -coverprofile ./..., warm) on this machine: base and HEAD alternate for +# BENCH_ROUNDS rounds (default 3) and the medians are compared, which cancels +# the machine-to-machine noise of separate CI runs. +# Appends the report to $GITHUB_STEP_SUMMARY when set. set -euo pipefail cd "$(dirname "$0")/.." GO=${GO:-go} cold=${BENCH_COLD:-} only=,${BENCH_ONLY:-build,generate,lint,probes,tests}, +read -r -a pkgs <<<"${BENCH_PKGS:-./...}" want() { [[ $only == *",$1,"* ]]; } mkdir -p .cache tmp=$(mktemp -d "$PWD/.cache/bench-devloop.XXXXXX") # not /tmp: often RAM, and cold caches are GBs @@ -25,8 +30,10 @@ cleanup() { for p in "${pids[@]}"; do kill "$p" 2>/dev/null || true; done for d in "${dbs[@]}"; do psql -q "$TEST_DATABASE_URL" -c "DROP DATABASE IF EXISTS $d WITH (FORCE)" >/dev/null 2>&1 || true; done if [ -n "$edited" ]; then cp "$tmp/edited.orig" "$edited"; fi + if [ -d "$tmp/base" ]; then git worktree remove --force "$tmp/base" 2>/dev/null || true; fi chmod -R u+w "$tmp" 2>/dev/null || true rm -rf "$tmp" + git worktree prune 2>/dev/null || true } trap cleanup EXIT @@ -199,7 +206,7 @@ fi # ---- tests ---- if want tests; then -say "### Tests (go test ./..., -count=1)" +say "### Tests (go test ${pkgs[*]}, -count=1)" say "" [ -n "${TEST_DATABASE_URL:-}" ] || say "TEST_DATABASE_URL is not set: database tests skip." "$GO" build -o "$tmp/testbudget" ./scripts/testbudget @@ -208,7 +215,7 @@ gotest() { local name=$1 s shift s=$(now) - "$GO" test -count=1 -json "$@" ./... >"$tmp/$name.json" 2>"$log" || { + "$GO" test -count=1 -json "$@" "${pkgs[@]}" >"$tmp/$name.json" 2>"$log" || { "$tmp/testbudget" -pkg 0 -test 0 -top 0 <"$tmp/$name.json" >&2 || true cat "$log" >&2 return 1 @@ -233,4 +240,45 @@ say "" say "" fi +# ---- A/B ---- +if [ -n "${BENCH_BASE:-}" ] && want tests; then + rounds=${BENCH_ROUNDS:-3} + git worktree add -q --detach "$tmp/base" "$BENCH_BASE" + say "### A/B on this machine: $BENCH_BASE ($(git -C "$tmp/base" rev-parse --short HEAD)) vs HEAD ($(git rev-parse --short HEAD))" + say "" + say "go test -race -coverprofile ${pkgs[*]} (as CI), warm, $rounds alternating rounds; wall time per round and the median." + say "" + # abrun : prints the run's wall time in ms. + abrun() { + local s + s=$(now) + (cd "$1" && "$GO" test -count=1 -json -race -coverprofile="$tmp/ab-cover.out" "${pkgs[@]}" >"$2" 2>"$log") || { + "$tmp/testbudget" -pkg 0 -test 0 -top 0 <"$2" >&2 || true + cat "$log" >&2 + return 1 + } + echo $(($(now) - s)) + } + abrun "$tmp/base" "$tmp/ab-prime-base.json" >/dev/null # compile both into the cache + abrun . "$tmp/ab-prime-head.json" >/dev/null + base_ms=() head_ms=() base_json=() head_json=() + for i in $(seq 1 "$rounds"); do + base_ms+=("$(abrun "$tmp/base" "$tmp/ab-base-$i.json")") && base_json+=("$tmp/ab-base-$i.json") + head_ms+=("$(abrun . "$tmp/ab-head-$i.json")") && head_json+=("$tmp/ab-head-$i.json") + done + med() { printf '%s\n' "$@" | sort -n | awk '{ a[NR] = $1 } END { print a[int((NR + 1) / 2)] }'; } + list() { for m in "$@"; do secs "$m"; printf ' '; done; } + bm=$(med "${base_ms[@]}") hm=$(med "${head_ms[@]}") + say "| tree | rounds | median |" + say "|---|---|--:|" + say "| base | $(list "${base_ms[@]}")| $(secs "$bm") |" + say "| HEAD | $(list "${head_ms[@]}")| $(secs "$hm") |" + say "| change | | $(awk -v b="$bm" -v h="$hm" 'BEGIN { printf "%+.0f%%", 100 * (h - b) / b }') |" + say "" + say "Per package (median test binary wall time):" + say "" + "$tmp/testbudget" table "base=$(IFS=,; echo "${base_json[*]}")" "HEAD=$(IFS=,; echo "${head_json[*]}")" | tee -a "$report" + say "" +fi + if [ -n "${GITHUB_STEP_SUMMARY:-}" ]; then cat "$report" >>"$GITHUB_STEP_SUMMARY"; fi diff --git a/scripts/testbudget/main.go b/scripts/testbudget/main.go index 4b74499..9e726eb 100644 --- a/scripts/testbudget/main.go +++ b/scripts/testbudget/main.go @@ -13,9 +13,11 @@ // not a flake: see AGENTS.md "Fast tests" before raising a budget. // // The table subcommand renders saved -json runs as markdown for -// `make bench-devloop`: +// `make bench-devloop`, one column per run (a comma-separated list of +// repeated runs gives each package's median): // // testbudget table race=race.json norace=norace.json +// testbudget table base=b1.json,b2.json,b3.json head=h1.json,h2.json,h3.json package main import ( @@ -189,35 +191,46 @@ func table(out io.Writer, args []string) int { var order []string seen := map[string]bool{} for _, a := range args { - name, file, ok := strings.Cut(a, "=") + name, files, ok := strings.Cut(a, "=") if !ok { - fmt.Fprintln(os.Stderr, "usage: testbudget table name=file.json ...") + fmt.Fprintln(os.Stderr, "usage: testbudget table name=file.json[,file.json...] ...") return 2 } - f, err := os.Open(file) - if err != nil { - fmt.Fprintln(os.Stderr, err) - return 1 - } - c := col{name: name, pkgs: map[string]time.Duration{}} - sc := bufio.NewScanner(f) - sc.Buffer(make([]byte, 0, 1<<20), 1<<26) - for sc.Scan() { - var e event - if json.Unmarshal(sc.Bytes(), &e) != nil || (e.Action != "pass" && e.Action != "fail") { - continue + // Several files (repeated runs): each package's and test's median. + pkgRuns, testRuns := map[string][]time.Duration{}, map[string][]time.Duration{} + for file := range strings.SplitSeq(files, ",") { + f, err := os.Open(file) + if err != nil { + fmt.Fprintln(os.Stderr, err) + return 1 } - if e.Test == "" { - c.pkgs[e.Package] = seconds(e.Elapsed) - if !seen[e.Package] { - seen[e.Package] = true - order = append(order, e.Package) + sc := bufio.NewScanner(f) + sc.Buffer(make([]byte, 0, 1<<20), 1<<26) + for sc.Scan() { + var e event + if json.Unmarshal(sc.Bytes(), &e) != nil || (e.Action != "pass" && e.Action != "fail") { + continue + } + if e.Test == "" { + pkgRuns[e.Package] = append(pkgRuns[e.Package], seconds(e.Elapsed)) + if !seen[e.Package] { + seen[e.Package] = true + order = append(order, e.Package) + } + } else { + k := e.Package + " " + e.Test + testRuns[k] = append(testRuns[k], seconds(e.Elapsed)) } - } else { - c.tests = append(c.tests, result{e.Package + " " + e.Test, seconds(e.Elapsed)}) } + _ = f.Close() + } + c := col{name: name, pkgs: map[string]time.Duration{}} + for p, ds := range pkgRuns { + c.pkgs[p] = median(ds) + } + for t, ds := range testRuns { + c.tests = append(c.tests, result{t, median(ds)}) } - _ = f.Close() cols = append(cols, c) } if len(cols) == 0 { @@ -259,6 +272,11 @@ func table(out io.Writer, args []string) int { return 0 } +func median(ds []time.Duration) time.Duration { + slices.Sort(ds) + return ds[len(ds)/2] +} + // commonPrefix is the module path shared by the packages ("" if fewer than two). func commonPrefix(pkgs []string) string { if len(pkgs) < 2 { From 2d4b14a52c04cacbe3a0348268d7e7eb24e2af5b Mon Sep 17 00:00:00 2001 From: JJ Eaton <58495321+jayleaton@users.noreply.github.com> Date: Thu, 22 Oct 2026 20:24:24 +0700 Subject: [PATCH 08/10] perf(test): crawl routes as parallel subtests sharing one handler per state and method Per-state crawl subtests ran 6-11s each on CI's single job and tripped the 5s test budget. Routes are subtests again, now sharing their state's handler: at GOMAXPROCS=2 the crawls use main's CPU or less and finish 29-38% sooner. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 2 +- apps/admin/ui/crawl_test.go | 46 ++++++++++++++++++++++--------------- apps/web/ui/crawl_test.go | 22 ++++++++++-------- 3 files changed, 42 insertions(+), 28 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index a82e97f..7e195eb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -95,7 +95,7 @@ Mocks are generated, never written: `//go:generate go tool mockgen -source=`; prefer removing the global instead (the crawl tests used to share `crawlMethod`). +- **Parallel.** Every test starts with `t.Parallel()` (`paralleltest`). Slow table tests make their cases parallel subtests too; `tparallel` keeps a test and its subtests consistent. Build expensive fixtures once per group and share them with the parallel subtests (the crawls build one handler per state and method, then run each route as a subtest): CI has 2 vCPUs, so a split that doubles CPU time gains nothing, and a long subtest trips the budget. Judge CPU cost with `GOMAXPROCS=2`; on a big machine idle parallel workers inflate it. Never share a gomock controller, `httptest.Server` or recorder across parallel subtests; build them per case. A test that must touch process-wide state stays serial with `//nolint:paralleltest // `; prefer removing the global instead (the crawl tests used to share `crawlMethod`). - **No sleeps.** `time.Sleep` in a test fails lint (forbidigo). Wait on a channel, a condition or `synctest.Wait`; for intervals, expiry and backoff use `testing/synctest` (its fake clock: `packages/secrets` `advance`) or an injected clock (`auth`'s `testClock`). - **One database per test, cloned.** Repository tests use `dbtest.New(t, template)`: a `CREATE DATABASE … TEMPLATE` clone of a database migrated once per server, so tests never migrate and never depend on each other's rows. `packages/auth` does the same in its `TestMain` (`newDatabase`); tests on its shared database use unique data (`uniqueEmail`). - **Cheap crypto.** `auth.New` uses cheap argon2 inside test binaries; only `TestPasswordHashDefaults` pays production cost. diff --git a/apps/admin/ui/crawl_test.go b/apps/admin/ui/crawl_test.go index 632d603..f6e796b 100644 --- a/apps/admin/ui/crawl_test.go +++ b/apps/admin/ui/crawl_test.go @@ -179,23 +179,31 @@ func TestNoUnescapedOutput(t *testing.T) { func TestCrawlExposesNothing(t *testing.T) { t.Parallel() - // One parallel subtest per state, each with its own crawler (handler and - // mock controller) that requests the routes one at a time. + // One handler (and mock controller) per state and method keeps setup + // small; its routes run as parallel subtests so no single test is long. for _, failing := range []bool{false, true} { for _, signedIn := range []bool{false, true} { - t.Run(state(failing, "failing", "ok")+"/"+state(signedIn, "signed-in", "anonymous"), func(t *testing.T) { - t.Parallel() - cr := crawlUI(t, failing, signedIn) - for _, c := range crawlTargets { - method, _, _ := strings.Cut(c.pattern, " ") - for _, path := range c.paths { - for _, ds := range []bool{false, true} { - name := strings.Join([]string{method, path, state(failing, "failing", "ok"), state(signedIn, "signed-in", "anonymous"), state(ds, "datastar", "plain")}, " ") - checkResponse(t, name, method, path, cr.crawl(method, path, ds)) + for _, method := range []string{http.MethodGet, http.MethodPost} { + st := state(failing, "failing", "ok") + "/" + state(signedIn, "signed-in", "anonymous") + t.Run(st+"/"+method, func(t *testing.T) { + t.Parallel() + cr := crawlUI(t, method, failing, signedIn) + for _, c := range crawlTargets { + if m, _, _ := strings.Cut(c.pattern, " "); m != method { + continue } + t.Run(c.pattern, func(t *testing.T) { + t.Parallel() + for _, path := range c.paths { + for _, ds := range []bool{false, true} { + name := strings.Join([]string{method, path, st, state(ds, "datastar", "plain")}, " ") + checkResponse(t, name, method, path, cr.crawl(method, path, ds)) + } + } + }) } - } - }) + }) + } } } } @@ -207,8 +215,8 @@ func state(b bool, yes, no string) string { return no } -// crawler is one real UI handler and the method of the request it is -// serving. Each subtest has its own and crawls one request at a time. +// crawler is one real UI handler that serves one method (notOnGET reads it). +// Route subtests share it and send their requests concurrently. type crawler struct { h http.Handler method string @@ -216,8 +224,8 @@ type crawler struct { // crawlUI is the real UI handler over mocks that answer anything: test data, // or (failing) internal errors carrying canary text. -func crawlUI(t *testing.T, failing, signedIn bool) *crawler { - cr := &crawler{} +func crawlUI(t *testing.T, method string, failing, signedIn bool) *crawler { + cr := &crawler{method: method} ctrl := gomock.NewController(t) a := mocks.NewMockAuth(ctrl) us, ad, au, an := mocks.NewMockUsers(ctrl), mocks.NewMockAdmins(ctrl), mocks.NewMockAudit(ctrl), mocks.NewMockAnalytics(ctrl) @@ -295,7 +303,9 @@ func (cr *crawler) crawl(method, path string, ds bool) *httptest.ResponseRecorde if method == http.MethodPost { body = strings.NewReader(crawlForm.Encode()) } - cr.method = method + if method != cr.method { + panic("crawler for " + cr.method + " asked to send " + method) + } ctx, cancel := context.WithCancel(context.Background()) defer cancel() r := httptest.NewRequestWithContext(ctx, method, "https://admin.example"+path, body) diff --git a/apps/web/ui/crawl_test.go b/apps/web/ui/crawl_test.go index edfc05d..f49a2aa 100644 --- a/apps/web/ui/crawl_test.go +++ b/apps/web/ui/crawl_test.go @@ -189,25 +189,29 @@ func TestNoUnescapedOutput(t *testing.T) { func TestCrawlExposesNothing(t *testing.T) { t.Parallel() - // One parallel subtest per state and method, each with one handler (and - // mock controller) built for the method it serves: notOnGET needs no - // shared state, and a handler per state keeps setup off the CPU budget. + // One handler (and mock controller) per state and method, built for the + // method it serves (notOnGET needs no shared state), keeps setup small; + // its routes run as parallel subtests so no single test is long. for _, failing := range []bool{false, true} { for _, signedIn := range []bool{false, true} { for _, method := range []string{http.MethodGet, http.MethodPost} { - t.Run(strings.Join([]string{state(failing, "failing", "ok"), state(signedIn, "signed-in", "anonymous"), method}, "/"), func(t *testing.T) { + st := state(failing, "failing", "ok") + "/" + state(signedIn, "signed-in", "anonymous") + t.Run(st+"/"+method, func(t *testing.T) { t.Parallel() h := crawlUI(t, method, failing, signedIn) for _, c := range crawlTargets { if m, _, _ := strings.Cut(c.pattern, " "); m != method { continue } - for _, path := range c.paths { - for _, ds := range []bool{false, true} { - name := strings.Join([]string{method, path, state(failing, "failing", "ok"), state(signedIn, "signed-in", "anonymous"), state(ds, "datastar", "plain")}, " ") - checkResponse(t, name, method, path, crawl(h, method, path, ds)) + t.Run(c.pattern, func(t *testing.T) { + t.Parallel() + for _, path := range c.paths { + for _, ds := range []bool{false, true} { + name := strings.Join([]string{method, path, st, state(ds, "datastar", "plain")}, " ") + checkResponse(t, name, method, path, crawl(h, method, path, ds)) + } } - } + }) } }) } From 9e1a2ca0aee9f38def4671e27ce926262d949e71 Mon Sep 17 00:00:00 2001 From: JJ Eaton <58495321+jayleaton@users.noreply.github.com> Date: Thu, 22 Oct 2026 20:29:55 +0700 Subject: [PATCH 09/10] fix(ci): test-time budgets at about twice today's slowest (60s package, 10s test) 5s flaked on a CPU-bound scrypt test (5.28s) while a cold build cache compiled on the same 2 vCPUs. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 2 +- scripts/testbudget/main.go | 9 ++++++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 7e195eb..63e09d3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -101,7 +101,7 @@ The suite runs on every change it can affect, under `-race` on 2-vCPU CI runners - **Cheap crypto.** `auth.New` uses cheap argon2 inside test binaries; only `TestPasswordHashDefaults` pays production cost. - **Table tests.** One test with a case table, not a copy per case. - **Do expensive checks once.** Work that depends only on immutable input (a static asset's body in the crawl) is checked once per distinct input (`checkBody`), not once per request. -- **Budgets.** `make test` and CI pipe `go test -json` through `scripts/testbudget`: plain `go test` output, then the slowest tests, then a failure if a package's test binary runs over 30 s or a single test (a leaf test or subtest) over 5 s, measured on CI under `-race`. Over budget means split or speed up the test. Raise a budget (constants in `scripts/testbudget`) only with the reason in the commit. +- **Budgets.** `make test` and CI pipe `go test -json` through `scripts/testbudget`: plain `go test` output, then the slowest tests, then a failure if a package's test binary runs over 60 s or a single test (a leaf test or subtest) over 10 s, measured on CI under `-race` (about twice today's slowest, so a doubling fails). Over budget means split or speed up the test. Raise a budget (constants in `scripts/testbudget`) only with the reason in the commit. - **Measure, don't guess.** `make bench-devloop` prints the timing tables (build, generate, lint per linter, probes, tests per package with and without `-race`); `BENCH_COLD=1` adds cold caches, `BENCH_PKGS` narrows the packages, and `BENCH_BASE=` adds an A/B of the CI test run against that ref on the same machine (medians of alternating rounds). Separate CI runs land on different machines and vary by tens of percent, so compare a perf change with the A/B: push it to a `bench/` branch (the "Bench dev loop" workflow compares it with main) and delete the branch afterwards. ## Admin app diff --git a/scripts/testbudget/main.go b/scripts/testbudget/main.go index 9e726eb..a4488ab 100644 --- a/scripts/testbudget/main.go +++ b/scripts/testbudget/main.go @@ -33,10 +33,13 @@ import ( "time" ) -// Default budgets. Raise one only with a reason in the commit message. +// Default budgets, about twice the slowest package (~25 s) and test (~5 s, +// CPU-bound scrypt) seen on CI under -race while the cold build cache also +// compiles: tight enough to catch a doubling, loose enough not to flake. +// Raise one only with a reason in the commit message. const ( - defaultPackageBudget = 30 * time.Second - defaultTestBudget = 5 * time.Second + defaultPackageBudget = 60 * time.Second + defaultTestBudget = 10 * time.Second ) // event is one line of `go test -json` (cmd/test2json). From e3e3c3272337b68ef454db1f73d6c8dc0244cdb9 Mon Sep 17 00:00:00 2001 From: JJ Eaton <58495321+jayleaton@users.noreply.github.com> Date: Thu, 22 Oct 2026 20:47:39 +0700 Subject: [PATCH 10/10] perf(test): deploy tests added in #76 run in parallel Co-Authored-By: Claude Opus 5.5 --- deploy/platform/platform_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/deploy/platform/platform_test.go b/deploy/platform/platform_test.go index bf2d4ef..17d5467 100644 --- a/deploy/platform/platform_test.go +++ b/deploy/platform/platform_test.go @@ -275,6 +275,7 @@ func TestSecretVarsFollowProvider(t *testing.T) { } func TestResolveAuto(t *testing.T) { + t.Parallel() cfg := parse(t, "version: 1\nproject: auto\nrepo: auto\n") for _, tc := range []struct{ origin, dir, project, repo string }{ {"https://github.com/acme/shop.git", "/src/x", "shop", "acme/shop"}, @@ -304,12 +305,14 @@ func TestResolveAuto(t *testing.T) { } func TestBuildRefusesUnresolvedAuto(t *testing.T) { + t.Parallel() if _, err := parse(t, "version: 1\nproject: auto\nrepo: auto\n").Build("production"); err == nil { t.Fatal("built a graph with repo: auto") } } func TestForceUploadDropsSource(t *testing.T) { + t.Parallel() cfg := parse(t, minimal) cfg.ForceUpload = true if _, ok := resource(t, graph(t, cfg, "production"), "service.web")["source"]; ok { @@ -318,6 +321,7 @@ func TestForceUploadDropsSource(t *testing.T) { } func TestEditKeepsCommentsAndValidates(t *testing.T) { + t.Parallel() src := []byte("# top\nversion: 1\nproject: auto # name\nrepo: auto\nenvironments: [production]\nresources:\n web: { cpu: 1 }\n") out, changed, err := platform.Edit(src, []platform.Key{{"project", "shop"}, {"environments", "staging,production"}, {"admin", "true"}, {"region", ""}}) if err != nil {