From 04c6bf594b49d883d81597aff3d9fa4191de1b45 Mon Sep 17 00:00:00 2001 From: Jonathon Leight Date: Thu, 6 Aug 2026 21:16:40 -0400 Subject: [PATCH] Speed up tests --- cmd/licenses/main_test.go | 77 +++++++++++++++++++++++-------------- internal/web/assets.go | 15 ++++++-- internal/web/assets_test.go | 27 +++++++++---- internal/web/env.go | 10 ++++- 4 files changed, 87 insertions(+), 42 deletions(-) diff --git a/cmd/licenses/main_test.go b/cmd/licenses/main_test.go index d747acd..58dbcb0 100644 --- a/cmd/licenses/main_test.go +++ b/cmd/licenses/main_test.go @@ -1,62 +1,81 @@ package main import ( + "slices" "sort" "testing" ) -// TestScanModulesIsHostIndependent is a regression test. +// probePkg is a single package whose test dependencies differ between Linux and +// macOS (via testcontainers/gopsutil). Listing one package keeps this test cheap +// — scanning ./... across the whole matrix costs ~9 `go list` runs per call, and +// doing that twice dominated the CI test step. +const probePkg = "./internal/store" + +func probeArgs() []string { + return []string{"list", "-deps", "-test", "-json", probePkg} +} + +func paths(mods map[string]moduleInfo) []string { + out := make([]string, 0, len(mods)) + for p := range mods { + out = append(out, p) + } + sort.Strings(out) + return out +} + +// TestListModulesPinsPlatform is a regression test. // // `go list -deps` answers for exactly one GOOS/GOARCH, and the module set // genuinely differs between them: Linux pulls in moby/sys/userns and -// tklauser/numcpus, macOS pulls in ebitengine/purego. scanModules used to +// tklauser/numcpus, macOS pulls in ebitengine/purego. listModules used to // inherit the host's platform, so THIRD-PARTY-NOTICES.md generated on a macOS // laptop listed 87 modules while the Linux CI runner computed 88 — the drift // check failed on every CI run and regenerating locally could not fix it. // -// scanModules now pins GOOS/GOARCH per invocation and unions a fixed matrix, so -// the host must not matter. Setting GOOS/GOARCH in the environment is what the -// old code was (wrongly) sensitive to, which is precisely what this asserts is -// no longer true. +// Two things have to hold, and checking only one of them is how this would rot: +// the requested platform must actually take effect, and the host's own +// GOOS/GOARCH must not leak through. // // This shells out to `go list`, so it needs a module cache — the same // requirement `go test ./...` already has. -func TestScanModulesIsHostIndependent(t *testing.T) { +func TestListModulesPinsPlatform(t *testing.T) { root, err := repoRoot() if err != nil { t.Fatalf("repoRoot: %v", err) } - scanAs := func(goos, goarch string) []string { + list := func(p platform) []string { t.Helper() - t.Setenv("GOOS", goos) - t.Setenv("GOARCH", goarch) - - mods, _, err := scanModules(root) + mods, err := listModules(root, p, probeArgs()) if err != nil { - t.Fatalf("scanModules as %s/%s: %v", goos, goarch, err) + t.Fatalf("listModules for %s/%s: %v", p.GOOS, p.GOARCH, err) } - paths := make([]string, 0, len(mods)) - for _, m := range mods { - paths = append(paths, m.Path) + if len(mods) == 0 { + t.Fatalf("listModules for %s/%s found nothing — is the module cache populated?", p.GOOS, p.GOARCH) } - sort.Strings(paths) - return paths + return paths(mods) } - asLinux := scanAs("linux", "amd64") - asDarwin := scanAs("darwin", "arm64") + linux := list(platform{GOOS: "linux", GOARCH: "amd64"}) + darwin := list(platform{GOOS: "darwin", GOARCH: "arm64"}) - if len(asLinux) == 0 { - t.Fatal("scanned no modules at all — is the module cache populated?") + // Positive control. Two things make these match: listModules ignoring the + // platform it was handed (the regression this guards — both lists then come + // from the host), or probePkg's dependencies no longer differing by + // platform, which would leave the test proving nothing. + if slices.Equal(linux, darwin) { + t.Fatalf("%s resolved identically for linux/amd64 and darwin/arm64 (%d modules each). "+ + "Either listModules is not applying the platform it was given, or probePkg no longer "+ + "has platform-specific dependencies and this test needs re-pointing.", probePkg, len(linux)) } - if len(asLinux) != len(asDarwin) { - t.Fatalf("module count depends on the host platform: linux/amd64 saw %d, darwin/arm64 saw %d", len(asLinux), len(asDarwin)) - } - for i := range asLinux { - if asLinux[i] != asDarwin[i] { - t.Errorf("module list depends on the host platform: linux/amd64 has %q where darwin/arm64 has %q", asLinux[i], asDarwin[i]) - } + + // The actual regression: the host must not influence the answer. + t.Setenv("GOOS", "darwin") + t.Setenv("GOARCH", "arm64") + if got := list(platform{GOOS: "linux", GOARCH: "amd64"}); !slices.Equal(got, linux) { + t.Errorf("a linux/amd64 listing changed when the host env said darwin/arm64: %d modules vs %d", len(got), len(linux)) } } diff --git a/internal/web/assets.go b/internal/web/assets.go index f53666e..a9cceaf 100644 --- a/internal/web/assets.go +++ b/internal/web/assets.go @@ -11,6 +11,7 @@ import ( "path" "strconv" "strings" + "sync" "github.com/andybalholm/brotli" ) @@ -43,10 +44,18 @@ type assetManifest struct { byHashed map[string]*staticAsset // "ui..js" -> asset (fingerprinted requests) } -// assets is the process-wide manifest, built from the embedded static FS at -// package init. The FS is embedded and deterministic, so any failure here is a +// assets is the process-wide manifest, built from the embedded static FS on +// first use. The FS is embedded and deterministic, so any failure here is a // build/programming error — panic to fail fast at startup (and in tests). -var assets = buildAssetManifest(staticFS, "static") +// +// Built lazily rather than at package init because construction brotli- and +// gzip-compresses ~1.3MB of static files at their best levels: ~1.5s normally, +// but ~17s under -race, which instruments every byte of that pure-CPU work. As +// a package-level var, every test binary importing this package paid it whether +// or not it ever served an asset — six of them did, dominating the CI test step. +// Env.SharedRoutes resolves it at registration, so a real server still +// compresses everything at startup and no request pays the cost. +var assets = sync.OnceValue(func() *assetManifest { return buildAssetManifest(staticFS, "static") }) func buildAssetManifest(fsys fs.FS, dir string) *assetManifest { sub, err := fs.Sub(fsys, dir) diff --git a/internal/web/assets_test.go b/internal/web/assets_test.go index a464252..8091eff 100644 --- a/internal/web/assets_test.go +++ b/internal/web/assets_test.go @@ -18,7 +18,7 @@ var fingerprintedRe = regexp.MustCompile(`^/static/ui\.[0-9a-f]{8}\.js$`) func TestAssetURLFingerprints(t *testing.T) { t.Parallel() - got := assets.URL("/static/ui.js") + got := assets().URL("/static/ui.js") if !fingerprintedRe.MatchString(got) { t.Fatalf("asset URL not fingerprinted: got %q, want /static/ui.<8hex>.js", got) } @@ -27,12 +27,12 @@ func TestAssetURLFingerprints(t *testing.T) { } // Unknown assets pass through unchanged so a stray reference still resolves. - if got := assets.URL("/static/does-not-exist.js"); got != "/static/does-not-exist.js" { + if got := assets().URL("/static/does-not-exist.js"); got != "/static/does-not-exist.js" { t.Fatalf("unknown asset should pass through, got %q", got) } // Extensions with a dotted stem keep the hash before the final extension. - if got := assets.URL("/static/tabler.min.css"); !regexp.MustCompile(`^/static/tabler\.min\.[0-9a-f]{8}\.css$`).MatchString(got) { + if got := assets().URL("/static/tabler.min.css"); !regexp.MustCompile(`^/static/tabler\.min\.[0-9a-f]{8}\.css$`).MatchString(got) { t.Fatalf("dotted-stem asset mis-fingerprinted: %q", got) } } @@ -41,7 +41,7 @@ func TestAssetURLFingerprints(t *testing.T) { // StripPrefix + handler path. func staticRouter() http.Handler { r := chi.NewRouter() - r.Handle("/static/*", http.StripPrefix("/static/", http.HandlerFunc(assets.serveHTTP))) + r.Handle("/static/*", http.StripPrefix("/static/", http.HandlerFunc(assets().serveHTTP))) return r } @@ -55,7 +55,7 @@ func TestStaticFingerprintedImmutable(t *testing.T) { } rec := httptest.NewRecorder() - srv.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, assets.URL("/static/ui.js"), nil)) + srv.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, assets().URL("/static/ui.js"), nil)) if rec.Code != http.StatusOK { t.Fatalf("fingerprinted asset: status %d, want 200", rec.Code) @@ -77,7 +77,7 @@ func TestStaticServesBrotli(t *testing.T) { t.Fatalf("read embedded asset: %v", err) } - req := httptest.NewRequest(http.MethodGet, assets.URL("/static/tabler.min.css"), nil) + req := httptest.NewRequest(http.MethodGet, assets().URL("/static/tabler.min.css"), nil) req.Header.Set("Accept-Encoding", "gzip, deflate, br") rec := httptest.NewRecorder() srv.ServeHTTP(rec, req) @@ -112,7 +112,7 @@ func TestStaticServesGzipWhenBrotliUnwanted(t *testing.T) { t.Fatalf("read embedded asset: %v", err) } - req := httptest.NewRequest(http.MethodGet, assets.URL("/static/ui.js"), nil) + req := httptest.NewRequest(http.MethodGet, assets().URL("/static/ui.js"), nil) req.Header.Set("Accept-Encoding", "gzip") // no br rec := httptest.NewRecorder() srv.ServeHTTP(rec, req) @@ -144,7 +144,7 @@ func TestStaticIdentityStillVaries(t *testing.T) { // No Accept-Encoding: serve raw, but still advertise that the resource varies // so shared caches don't hand a compressed copy to a client that can't decode. - req := httptest.NewRequest(http.MethodGet, assets.URL("/static/ui.js"), nil) + req := httptest.NewRequest(http.MethodGet, assets().URL("/static/ui.js"), nil) rec := httptest.NewRecorder() srv.ServeHTTP(rec, req) @@ -209,3 +209,14 @@ func TestStaticUnfingerprintedStillServed(t *testing.T) { t.Fatalf("un-fingerprinted asset should not be immutable, got Cache-Control %q", cc) } } + +// BenchmarkBuildAssetManifest measures what `assets` costs to construct, which +// is why it is built lazily rather than at package init. Run it with -race to +// see the number that mattered: the race detector instruments this pure-CPU +// compression heavily, and as a package-level var it was charged to every test +// binary that imported this package, not just the ones serving assets. +func BenchmarkBuildAssetManifest(b *testing.B) { + for b.Loop() { + buildAssetManifest(staticFS, "static") + } +} diff --git a/internal/web/env.go b/internal/web/env.go index e9ea2f0..3b2357d 100644 --- a/internal/web/env.go +++ b/internal/web/env.go @@ -208,7 +208,7 @@ var templateFuncs = template.FuncMap{ "ts": TimeElement, // asset maps a logical static path ("/static/ui.js") to its content-hashed, // immutably-cacheable URL. Use it for every /static/ reference in templates. - "asset": assets.URL, + "asset": func(l string) string { return assets().URL(l) }, } // tsFallbackLayouts maps a display kind to the Go layout used for the server-side @@ -446,7 +446,13 @@ func limitBody(next http.Handler) http.Handler { // row per report. func (e *Env) SharedRoutes(r chi.Router) { r.Get("/healthz", e.healthz) - r.Handle("/static/*", http.StripPrefix("/static/", http.HandlerFunc(assets.serveHTTP))) + // Resolve the manifest here, at registration, rather than inside the + // handler. Registration happens during server startup, so the assets are + // compressed before the first request exactly as they were when the + // manifest was a package-level var — while a test binary that never + // registers routes skips the work entirely. See the comment on `assets`. + m := assets() + r.Handle("/static/*", http.StripPrefix("/static/", http.HandlerFunc(m.serveHTTP))) if e.csp != nil { r.Post(CSPReportPath, e.csp.handleReport) }