From dc35f6e9a9e55a99c02876ce9c834864f06b9f9f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 14 Aug 2026 00:55:19 +0200 Subject: [PATCH 1/4] fix(test): fail integration suites on migration failure instead of skipping The DB-backed suites answered two very different errors with the same t.Skipf: a Postgres testcontainer that would not start, and a set of migrations that would not apply. The first is an environment fact and a legitimate skip. The second is the failure of the thing under test, reported as green. Breaking any SQL file under internal/database/postgres/migrations made the whole integration suite report skipped, with nothing distinguishing "no Docker on this runner" from "the schema is broken". Split the two conditions. testhelpers.RequirePostgresContainer probes the Docker provider directly via testcontainers SkipIfProviderIsNotHealthy, which is the only thing allowed to skip, and requires the container past that probe. Migration errors become require.NoError. false_green_skip_guard_test.go keeps the class closed for tests not yet written: it parses every _test.go in the repo and fails on any if statement that answers an error from SetupPostgresContainer, RunMigrations, MigrateToVersion or RollbackMigrations with a skip. It is parse-only, so it needs no build tag and still sees the files behind //go:build integration. A tripwire fails the guard if it ever finds zero guarded calls, so its own detection cannot break silently. Verified in both directions against a real Docker daemon: with a deliberately invalid migration the suite now FAILs (exit 1) where it previously SKIPped (exit 0), and inside a container with no Docker socket it still SKIPs rather than failing. Closes #1597 --- .../analytics/postgres_analytics_db_test.go | 61 +-- internal/auth/store_postgres_db_test.go | 21 +- internal/config/store_postgres_db_test.go | 38 +- internal/config/store_postgres_ladder_test.go | 17 +- .../config/store_postgres_unassigned_test.go | 16 +- ...0078_monthly_summary_nested_rollup_test.go | 5 +- ...rchase_history_marketplace_listing_test.go | 7 +- ..._purchase_history_account_id_width_test.go | 17 +- .../false_green_skip_guard_test.go | 391 ++++++++++++++++++ .../database/postgres/testhelpers/postgres.go | 29 ++ 10 files changed, 482 insertions(+), 120 deletions(-) create mode 100644 internal/database/postgres/testhelpers/false_green_skip_guard_test.go diff --git a/internal/analytics/postgres_analytics_db_test.go b/internal/analytics/postgres_analytics_db_test.go index 0016f5365..4d0c557e7 100644 --- a/internal/analytics/postgres_analytics_db_test.go +++ b/internal/analytics/postgres_analytics_db_test.go @@ -48,14 +48,11 @@ func TestPostgresAnalyticsStore_SaveSnapshot_DB(t *testing.T) { defer cancel() // Setup test container - container, err := testhelpers.SetupPostgresContainer(ctx, t) - if err != nil { - t.Skipf("Skipping test: could not setup postgres container: %v", err) - } + container := testhelpers.RequirePostgresContainer(ctx, t) defer container.Cleanup(ctx) // Run migrations - err = migrations.RunMigrations(ctx, container.DB.Pool(), getMigrationsPath(), "", "") + err := migrations.RunMigrations(ctx, container.DB.Pool(), getMigrationsPath(), "", "") require.NoError(t, err) // Create store @@ -132,14 +129,11 @@ func TestPostgresAnalyticsStore_QuerySavings_DB(t *testing.T) { defer cancel() // Setup test container - container, err := testhelpers.SetupPostgresContainer(ctx, t) - if err != nil { - t.Skipf("Skipping test: could not setup postgres container: %v", err) - } + container := testhelpers.RequirePostgresContainer(ctx, t) defer container.Cleanup(ctx) // Run migrations - err = migrations.RunMigrations(ctx, container.DB.Pool(), getMigrationsPath(), "", "") + err := migrations.RunMigrations(ctx, container.DB.Pool(), getMigrationsPath(), "", "") require.NoError(t, err) // Create store @@ -266,14 +260,11 @@ func TestPostgresAnalyticsStore_QueryByProvider_DB(t *testing.T) { defer cancel() // Setup test container - container, err := testhelpers.SetupPostgresContainer(ctx, t) - if err != nil { - t.Skipf("Skipping test: could not setup postgres container: %v", err) - } + container := testhelpers.RequirePostgresContainer(ctx, t) defer container.Cleanup(ctx) // Run migrations - err = migrations.RunMigrations(ctx, container.DB.Pool(), getMigrationsPath(), "", "") + err := migrations.RunMigrations(ctx, container.DB.Pool(), getMigrationsPath(), "", "") require.NoError(t, err) // Create store @@ -347,14 +338,11 @@ func TestPostgresAnalyticsStore_QueryByService_DB(t *testing.T) { defer cancel() // Setup test container - container, err := testhelpers.SetupPostgresContainer(ctx, t) - if err != nil { - t.Skipf("Skipping test: could not setup postgres container: %v", err) - } + container := testhelpers.RequirePostgresContainer(ctx, t) defer container.Cleanup(ctx) // Run migrations - err = migrations.RunMigrations(ctx, container.DB.Pool(), getMigrationsPath(), "", "") + err := migrations.RunMigrations(ctx, container.DB.Pool(), getMigrationsPath(), "", "") require.NoError(t, err) // Create store @@ -412,14 +400,11 @@ func TestPostgresAnalyticsStore_BulkInsertSnapshots_DB(t *testing.T) { defer cancel() // Setup test container - container, err := testhelpers.SetupPostgresContainer(ctx, t) - if err != nil { - t.Skipf("Skipping test: could not setup postgres container: %v", err) - } + container := testhelpers.RequirePostgresContainer(ctx, t) defer container.Cleanup(ctx) // Run migrations - err = migrations.RunMigrations(ctx, container.DB.Pool(), getMigrationsPath(), "", "") + err := migrations.RunMigrations(ctx, container.DB.Pool(), getMigrationsPath(), "", "") require.NoError(t, err) // Create store @@ -475,14 +460,11 @@ func TestPostgresAnalyticsStore_PartitionManagement_DB(t *testing.T) { defer cancel() // Setup test container - container, err := testhelpers.SetupPostgresContainer(ctx, t) - if err != nil { - t.Skipf("Skipping test: could not setup postgres container: %v", err) - } + container := testhelpers.RequirePostgresContainer(ctx, t) defer container.Cleanup(ctx) // Run migrations - err = migrations.RunMigrations(ctx, container.DB.Pool(), getMigrationsPath(), "", "") + err := migrations.RunMigrations(ctx, container.DB.Pool(), getMigrationsPath(), "", "") require.NoError(t, err) // Create store @@ -521,14 +503,11 @@ func TestPostgresAnalyticsStore_QueryMonthlyTotals_DB(t *testing.T) { defer cancel() // Setup test container - container, err := testhelpers.SetupPostgresContainer(ctx, t) - if err != nil { - t.Skipf("Skipping test: could not setup postgres container: %v", err) - } + container := testhelpers.RequirePostgresContainer(ctx, t) defer container.Cleanup(ctx) // Run migrations - err = migrations.RunMigrations(ctx, container.DB.Pool(), getMigrationsPath(), "", "") + err := migrations.RunMigrations(ctx, container.DB.Pool(), getMigrationsPath(), "", "") require.NoError(t, err) // Create store @@ -581,14 +560,11 @@ func TestPostgresAnalyticsStore_RefreshMaterializedViews_DB(t *testing.T) { defer cancel() // Setup test container - container, err := testhelpers.SetupPostgresContainer(ctx, t) - if err != nil { - t.Skipf("Skipping test: could not setup postgres container: %v", err) - } + container := testhelpers.RequirePostgresContainer(ctx, t) defer container.Cleanup(ctx) // Run migrations - err = migrations.RunMigrations(ctx, container.DB.Pool(), getMigrationsPath(), "", "") + err := migrations.RunMigrations(ctx, container.DB.Pool(), getMigrationsPath(), "", "") require.NoError(t, err) // Create store @@ -607,10 +583,7 @@ func TestPostgresAnalyticsStore_Close_DB(t *testing.T) { defer cancel() // Setup test container - container, err := testhelpers.SetupPostgresContainer(ctx, t) - if err != nil { - t.Skipf("Skipping test: could not setup postgres container: %v", err) - } + container := testhelpers.RequirePostgresContainer(ctx, t) defer container.Cleanup(ctx) // Create store diff --git a/internal/auth/store_postgres_db_test.go b/internal/auth/store_postgres_db_test.go index cbd2b9c01..ed6928b28 100644 --- a/internal/auth/store_postgres_db_test.go +++ b/internal/auth/store_postgres_db_test.go @@ -40,22 +40,19 @@ func setupAuthTestDB(t *testing.T) *database.Connection { ctx, cancel := context.WithTimeout(context.Background(), 120*time.Second) defer cancel() - container, err := testhelpers.SetupPostgresContainer(ctx, t) - if err != nil { - t.Skipf("Skipping DB test: cannot start PostgreSQL container: %v", err) - return nil - } - - if err := migrations.RunMigrations(ctx, container.DB.Pool(), getAuthTestMigrationsPath(), "", ""); err != nil { - container.Cleanup(ctx) - t.Skipf("Skipping DB test: cannot run migrations: %v", err) - return nil - } - + container := testhelpers.RequirePostgresContainer(ctx, t) t.Cleanup(func() { container.Cleanup(context.Background()) }) + // Migrating is the thing under test here, not a precondition of it: a + // migration that will not apply is a broken schema, and skipping on it would + // report the whole suite green on exactly the defect it exists to catch + // (issue #1597). + require.NoError(t, + migrations.RunMigrations(ctx, container.DB.Pool(), getAuthTestMigrationsPath(), "", ""), + "migrations failed to apply to a fresh database") + return container.DB } diff --git a/internal/config/store_postgres_db_test.go b/internal/config/store_postgres_db_test.go index c7311979a..0b8bfeac3 100644 --- a/internal/config/store_postgres_db_test.go +++ b/internal/config/store_postgres_db_test.go @@ -37,34 +37,26 @@ func setupTestContainerDB(t *testing.T) *database.Connection { ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) defer cancel() - container, err := testhelpers.SetupPostgresContainer(ctx, t) - if err != nil { - t.Skipf("Skipping DB test: cannot start PostgreSQL container: %v", err) - return nil - } + container := testhelpers.RequirePostgresContainer(ctx, t) + t.Cleanup(func() { + container.Cleanup(context.Background()) + }) - // Run migrations - err = migrations.RunMigrations(ctx, container.DB.Pool(), getTestMigrationsPath(), "", "") - if err != nil { - container.Cleanup(ctx) - t.Skipf("Skipping DB test: cannot run migrations: %v", err) - return nil - } + // Migrating is the thing under test here, not a precondition of it: a + // migration that will not apply is a broken schema, and skipping on it would + // report the whole suite green on exactly the defect it exists to catch + // (issue #1597). + require.NoError(t, + migrations.RunMigrations(ctx, container.DB.Pool(), getTestMigrationsPath(), "", ""), + "migrations failed to apply to a fresh database") // The store code uses ON CONFLICT (execution_id) but the migration schema // does not create a UNIQUE constraint on execution_id. Add it for tests. - _, err = container.DB.Exec(ctx, + _, err := container.DB.Exec(ctx, "CREATE UNIQUE INDEX IF NOT EXISTS idx_purchase_executions_execution_id_unique ON purchase_executions(execution_id)") - if err != nil { - container.Cleanup(ctx) - t.Skipf("Skipping DB test: cannot add unique index on execution_id: %v", err) - return nil - } - - // Register cleanup - t.Cleanup(func() { - container.Cleanup(context.Background()) - }) + require.NoError(t, err, + "could not add the unique index on execution_id the ON CONFLICT paths need; "+ + "a migrated schema this DDL cannot be applied to is a failure, not a reason to skip") return container.DB } diff --git a/internal/config/store_postgres_ladder_test.go b/internal/config/store_postgres_ladder_test.go index 2a993727f..6bee687b2 100644 --- a/internal/config/store_postgres_ladder_test.go +++ b/internal/config/store_postgres_ladder_test.go @@ -34,19 +34,18 @@ import ( ) // setupLadderStore starts a container, runs migrations, and returns a store. -// It skips (not fails) when Docker is unavailable so the suite degrades -// gracefully in environments without a container runtime. +// It skips (not fails) only when this environment has no usable Docker daemon, +// so the suite degrades gracefully without a container runtime. A migration that +// will not apply is a broken schema, not a missing runtime, and fails loudly +// (issue #1597). func setupLadderStore(ctx context.Context, t *testing.T) *PostgresStore { t.Helper() - container, err := testhelpers.SetupPostgresContainer(ctx, t) - if err != nil { - t.Skipf("Skipping integration test: cannot start PostgreSQL container: %v", err) - } + container := testhelpers.RequirePostgresContainer(ctx, t) t.Cleanup(func() { container.Cleanup(context.Background()) }) - if err := migrations.RunMigrations(ctx, container.DB.Pool(), getTestMigrationsPath(), "", ""); err != nil { - t.Skipf("Skipping integration test: migrations failed: %v", err) - } + require.NoError(t, + migrations.RunMigrations(ctx, container.DB.Pool(), getTestMigrationsPath(), "", ""), + "migrations failed to apply to a fresh database") return NewPostgresStore(container.DB) } diff --git a/internal/config/store_postgres_unassigned_test.go b/internal/config/store_postgres_unassigned_test.go index 9c98d1e0e..74655da33 100644 --- a/internal/config/store_postgres_unassigned_test.go +++ b/internal/config/store_postgres_unassigned_test.go @@ -37,18 +37,14 @@ func TestPostgresStoreDB_ListPurchasePlans_UnassignedBucket(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 120*time.Second) defer cancel() - container, err := testhelpers.SetupPostgresContainer(ctx, t) - if err != nil { - t.Skipf("Skipping integration test: cannot start PostgreSQL container: %v", err) - return - } + container := testhelpers.RequirePostgresContainer(ctx, t) t.Cleanup(func() { container.Cleanup(context.Background()) }) - err = migrations.RunMigrations(ctx, container.DB.Pool(), getTestMigrationsPath(), "", "") - if err != nil { - t.Skipf("Skipping integration test: migrations failed: %v", err) - return - } + // A migration that will not apply is a broken schema, not a missing + // container runtime: fail rather than report the suite green (issue #1597). + require.NoError(t, + migrations.RunMigrations(ctx, container.DB.Pool(), getTestMigrationsPath(), "", ""), + "migrations failed to apply to a fresh database") conn := container.DB store := NewPostgresStore(conn) diff --git a/internal/database/postgres/migrations/000078_monthly_summary_nested_rollup_test.go b/internal/database/postgres/migrations/000078_monthly_summary_nested_rollup_test.go index 9efb42fa3..cecb390e4 100644 --- a/internal/database/postgres/migrations/000078_monthly_summary_nested_rollup_test.go +++ b/internal/database/postgres/migrations/000078_monthly_summary_nested_rollup_test.go @@ -35,10 +35,7 @@ func TestAnalyticsNestedRollup_COR02(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) defer cancel() - container, err := testhelpers.SetupPostgresContainer(ctx, t) - if err != nil { - t.Skipf("Skipping test: could not setup postgres container: %v", err) - } + container := testhelpers.RequirePostgresContainer(ctx, t) defer container.Cleanup(ctx) require.NoError(t, migrations.RunMigrations(ctx, container.DB.Pool(), getMigrationsPath(), "", "")) diff --git a/internal/database/postgres/migrations/000087_purchase_history_marketplace_listing_test.go b/internal/database/postgres/migrations/000087_purchase_history_marketplace_listing_test.go index 0aa2609dc..642e5c193 100644 --- a/internal/database/postgres/migrations/000087_purchase_history_marketplace_listing_test.go +++ b/internal/database/postgres/migrations/000087_purchase_history_marketplace_listing_test.go @@ -26,10 +26,7 @@ func TestMigration087_MarketplaceColumns(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) defer cancel() - container, err := testhelpers.SetupPostgresContainer(ctx, t) - if err != nil { - t.Skipf("Skipping test: could not setup postgres container: %v", err) - } + container := testhelpers.RequirePostgresContainer(ctx, t) defer container.Cleanup(ctx) require.NoError(t, migrations.RunMigrations(ctx, container.DB.Pool(), getMigrationsPath(), "", ""), @@ -51,7 +48,7 @@ func TestMigration087_MarketplaceColumns(t *testing.T) { 'standard', 'ril-087-test', 'active' )` - _, err = container.DB.Pool().Exec(ctx, insertSQL, time.Now()) + _, err := container.DB.Pool().Exec(ctx, insertSQL, time.Now()) require.NoError(t, err, "INSERT referencing offering_class, listing_id, listing_state must succeed after migration 000087") // Verify the values round-trip correctly. diff --git a/internal/database/postgres/migrations/000095_purchase_history_account_id_width_test.go b/internal/database/postgres/migrations/000095_purchase_history_account_id_width_test.go index 0b5597b34..4bb313f9b 100644 --- a/internal/database/postgres/migrations/000095_purchase_history_account_id_width_test.go +++ b/internal/database/postgres/migrations/000095_purchase_history_account_id_width_test.go @@ -109,10 +109,7 @@ func TestMigration095_PurchaseHistoryAccountIDWidth(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) defer cancel() - container, err := testhelpers.SetupPostgresContainer(ctx, t) - if err != nil { - t.Skipf("Skipping test: could not setup postgres container: %v", err) - } + container := testhelpers.RequirePostgresContainer(ctx, t) defer container.Cleanup(ctx) pool := container.DB.Pool() @@ -201,10 +198,7 @@ func TestMigration095_DownNarrowsWhenSafe(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) defer cancel() - container, err := testhelpers.SetupPostgresContainer(ctx, t) - if err != nil { - t.Skipf("Skipping test: could not setup postgres container: %v", err) - } + container := testhelpers.RequirePostgresContainer(ctx, t) defer container.Cleanup(ctx) pool := container.DB.Pool() @@ -240,10 +234,7 @@ func TestMigration095_DownRefusesToTruncate(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) defer cancel() - container, err := testhelpers.SetupPostgresContainer(ctx, t) - if err != nil { - t.Skipf("Skipping test: could not setup postgres container: %v", err) - } + container := testhelpers.RequirePostgresContainer(ctx, t) defer container.Cleanup(ctx) pool := container.DB.Pool() @@ -256,7 +247,7 @@ func TestMigration095_DownRefusesToTruncate(t *testing.T) { "setup: an Azure audit row must be insertable after 000095") // One step, so exactly 000095's down migration runs (see the sibling test). - err = migrations.RollbackMigrations(ctx, pool, getMigrationsPath(), 1) + err := migrations.RollbackMigrations(ctx, pool, getMigrationsPath(), 1) require.Error(t, err, "rolling back 000095 with an over-long account_id present must fail rather than truncate") assert.Contains(t, err.Error(), "refusing to narrow purchase_history.account_id", diff --git a/internal/database/postgres/testhelpers/false_green_skip_guard_test.go b/internal/database/postgres/testhelpers/false_green_skip_guard_test.go new file mode 100644 index 000000000..8deb5fd03 --- /dev/null +++ b/internal/database/postgres/testhelpers/false_green_skip_guard_test.go @@ -0,0 +1,391 @@ +package testhelpers + +import ( + "fmt" + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "sort" + "strings" + "testing" +) + +// This guard closes the class of defect behind #1597 for tests not yet written, +// rather than only the call sites that exist today. +// +// t.Skip reports a test as neither passed nor failed. That is the right answer +// for "this environment cannot run the test at all" and the wrong answer for +// anything the test was written to detect. The DB-backed suites used it for +// both: a testcontainer that would not start AND a set of migrations that would +// not apply took the same t.Skipf branch, so breaking any SQL file under +// internal/database/postgres/migrations turned the whole integration suite +// green. +// +// The rule enforced here is not "no skips". It is that a skip predicate must +// test the precondition it claims to. Docker being absent is probed directly by +// RequirePostgresContainer; that is the only thing allowed to skip. Whether a +// container came up and whether migrations applied are results, not +// preconditions, so an error from either has to fail. +// +// The check is parse-only, so it costs milliseconds and needs no build tags, +// which matters because every file it guards is behind //go:build integration +// and would otherwise be invisible to a default `go test`. + +// guardedCalls are the calls whose error must never be answered with a skip, +// mapped to why. +var guardedCalls = map[string]string{ + "SetupPostgresContainer": "a container that will not start while Docker is healthy is a " + + "real failure; use testhelpers.RequirePostgresContainer, which probes Docker itself " + + "and skips only on that", + "RunMigrations": "migrating is the thing under test; a migration that will not apply is a broken schema and must fail", + "MigrateToVersion": "migrating is the thing under test; a migration that will not apply is a broken schema and must fail", + "RollbackMigrations": "rolling back is the thing under test; a down migration that will not apply is a broken schema and must fail", +} + +// skipFuncs are the *testing.T methods that end a test without a verdict. +var skipFuncs = map[string]bool{"Skip": true, "Skipf": true, "SkipNow": true} + +type finding struct { + pos token.Position + call string + skipFn string + rationale string +} + +func (f finding) String() string { + return fmt.Sprintf("%s: t.%s() answers an error from %s -- %s", f.pos, f.skipFn, f.call, f.rationale) +} + +func TestNoSkipOnDatabaseSetupFailure(t *testing.T) { + root := repoRootDir(t) + + fset := token.NewFileSet() + var findings []finding + files, calls := 0, 0 + + err := filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + switch d.Name() { + case ".git", "node_modules", "vendor", ".terraform", "frontend": + return filepath.SkipDir + } + return nil + } + if !strings.HasSuffix(path, "_test.go") { + return nil + } + files++ + // A file that does not parse is the compiler's problem, not this + // guard's: a package that does not compile has no passing tests to + // falsely report green either. + f, parseErr := parser.ParseFile(fset, path, nil, 0) + if parseErr != nil { + return nil + } + n, fs := inspectFile(fset, f) + calls += n + findings = append(findings, fs...) + return nil + }) + if err != nil { + t.Fatalf("walk %s: %v", root, err) + } + + // "Found nothing" and "looked at nothing" are the same color on a terminal, + // and telling them apart is precisely what this guard is about. The tripwire + // counts guarded CALLS rather than the if statements checking them: once the + // suites are fixed they check with require.NoError and hardly any of the if + // shape survives, so a count of those would sit near zero and the guard could + // lose its file discovery, its parsing or its name matching without saying so. + if calls == 0 { + t.Fatal("guard found zero calls to any guarded function; its detection has broken " + + "and it would report success on a repo that skips on every migration failure") + } + t.Logf("found %d guarded call(s) across %d test file(s)", calls, files) + + if len(findings) > 0 { + msgs := make([]string, 0, len(findings)) + for _, f := range findings { + msgs = append(msgs, f.String()) + } + sort.Strings(msgs) + t.Errorf("%d test setup site(s) turn a real failure into a skip:\n\n%s", + len(findings), strings.Join(msgs, "\n\n")) + } +} + +// inspectFile reports how many guarded calls the file makes and which of them +// are answered with a skip. The repo-wide run and the self-test both go through +// it, so the self-test cannot pass against logic the real run does not have. +// +// A finding is an if statement that mentions a guarded call in its init or +// condition and skips in its body. That covers `if err := f(); err != nil` and +// the `x, err := f()` / `if err != nil` pair alike: for the latter the call is +// resolved through the preceding assignment in the same statement list. Each +// list gets its own assignment scope, because reusing `err` across tests in one +// file is the norm and a file-wide map would attribute one test's call to +// another's check. +func inspectFile(fset *token.FileSet, f *ast.File) (int, []finding) { + calls := 0 + var findings []finding + + scan := func(list []ast.Stmt) { + assigned := map[string]string{} + for _, stmt := range list { + switch s := stmt.(type) { + case *ast.AssignStmt: + recordAssignment(s, assigned) + case *ast.IfStmt: + call := guardedCallIn(s, assigned) + if call == "" { + continue + } + if fn, pos, found := skipCallIn(fset, s.Body); found { + findings = append(findings, finding{ + pos: pos, call: call, skipFn: fn, rationale: guardedCalls[call], + }) + } + } + } + } + + // Every statement list that can hold an assignment followed by its check is + // scanned with its own scope. Descending unconditionally means nested + // blocks, closures and subtests are reached as lists in their own right. + ast.Inspect(f, func(n ast.Node) bool { + switch v := n.(type) { + case *ast.BlockStmt: + scan(v.List) + case *ast.CaseClause: + scan(v.Body) + } + if guardedCallName(n) != "" { + calls++ + } + return true + }) + + return calls, findings +} + +// recordAssignment notes `x, err := SetupPostgresContainer(...)` so a following +// `if err != nil` is recognized as guarding that call, and clears the note when +// the same name is rebound to something else. +func recordAssignment(s *ast.AssignStmt, assigned map[string]string) { + if len(s.Rhs) != 1 { + return + } + call := guardedCallName(s.Rhs[0]) + for _, lhs := range s.Lhs { + id, ok := lhs.(*ast.Ident) + if !ok || id.Name == "_" { + continue + } + if call == "" { + delete(assigned, id.Name) + continue + } + assigned[id.Name] = call + } +} + +// guardedCallIn returns the guarded call whose error the if statement checks, +// either inline in its init/condition or through a preceding assignment. +func guardedCallIn(s *ast.IfStmt, assigned map[string]string) string { + for _, n := range []ast.Node{s.Init, s.Cond} { + if n == nil { + continue + } + var found string + ast.Inspect(n, func(x ast.Node) bool { + if name := guardedCallName(x); name != "" { + found = name + return false + } + return true + }) + if found != "" { + return found + } + } + + // `if err != nil` on its own: resolve err through the preceding assignment. + var found string + ast.Inspect(s.Cond, func(x ast.Node) bool { + id, ok := x.(*ast.Ident) + if !ok { + return true + } + if call, ok := assigned[id.Name]; ok { + found = call + return false + } + return true + }) + return found +} + +// guardedCallName returns the guarded function name a node calls, if any. It +// matches on the function name alone: qualified (migrations.RunMigrations) and +// unqualified in-package calls are the same defect, and these names are +// distinctive enough that matching the selector too would only buy false +// negatives from an unexpected package alias. +func guardedCallName(n ast.Node) string { + call, ok := n.(*ast.CallExpr) + if !ok { + return "" + } + var name string + switch fun := call.Fun.(type) { + case *ast.SelectorExpr: + name = fun.Sel.Name + case *ast.Ident: + name = fun.Name + } + if _, guarded := guardedCalls[name]; guarded { + return name + } + return "" +} + +// skipCallIn reports the first t.Skip / t.Skipf / t.SkipNow inside a block, +// including nested blocks, since a skip behind a further condition still ends +// the test without a verdict. +func skipCallIn(fset *token.FileSet, body *ast.BlockStmt) (string, token.Position, bool) { + var name string + var pos token.Position + ast.Inspect(body, func(n ast.Node) bool { + if name != "" { + return false + } + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || !skipFuncs[sel.Sel.Name] { + return true + } + name = sel.Sel.Name + pos = fset.Position(call.Pos()) + return false + }) + return name, pos, name != "" +} + +func repoRootDir(t *testing.T) string { + t.Helper() + dir, err := os.Getwd() + if err != nil { + t.Fatalf("getwd: %v", err) + } + for { + if _, err := os.Stat(filepath.Join(dir, "go.mod")); err == nil { + return dir + } + parent := filepath.Dir(dir) + if parent == dir { + t.Fatal("no go.mod found above the working directory") + } + dir = parent + } +} + +// TestFalseGreenSkipGuardDetects exercises the detector on synthetic source +// covering each shape it must separate. Without it the guard could break into +// reporting nothing and the repo-wide run above would still look green, which is +// the failure mode #1597 is about, one level up. +func TestFalseGreenSkipGuardDetects(t *testing.T) { + const src = `package p + +func TestInlineSkip(t *testing.T) { + if err := migrations.RunMigrations(ctx, pool, path, "", ""); err != nil { + t.Skipf("migrations failed: %v", err) // want: finding + } +} + +func TestTwoStatementSkip(t *testing.T) { + container, err := testhelpers.SetupPostgresContainer(ctx, t) + if err != nil { + t.Skipf("no container: %v", err) // want: finding + } + _ = container +} + +func TestNestedSkip(t *testing.T) { + err := migrations.MigrateToVersion(ctx, pool, path, 42) + if err != nil { + if strings.Contains(err.Error(), "dirty") { + t.Skip("dirty") // want: finding, a nested skip still ends without a verdict + } + } +} + +func TestSubtestSkip(t *testing.T) { + t.Run("sub", func(t *testing.T) { + err := migrations.RollbackMigrations(ctx, pool, path, 1) + if err != nil { + t.Skipf("rollback failed: %v", err) // want: finding, closures are walked + } + }) +} + +func TestFails(t *testing.T) { + if err := migrations.RunMigrations(ctx, pool, path, "", ""); err != nil { + t.Fatalf("migrations failed: %v", err) // want: inspected, no finding + } +} + +func TestRebindClearsAssociation(t *testing.T) { + _, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + err = os.Setenv("X", "1") + if err != nil { + t.Skip("unrelated") // want: not inspected, err no longer holds the guarded call + } +} + +func TestUnrelatedSkip(t *testing.T) { + if os.Getenv("SKIP_DB_TESTS") != "" { + t.Skip("opt-out") // want: not inspected, an explicit environment opt-out + } +} +` + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "synthetic.go", src, 0) + if err != nil { + t.Fatalf("parse: %v", err) + } + + calls, findings := inspectFile(fset, f) + + // One guarded call in each function except TestUnrelatedSkip, which makes + // none. This is the same count the repo-wide tripwire relies on. + if calls != 6 { + t.Errorf("calls = %d guarded call(s), want 6", calls) + } + + // Line numbers of the four t.Skip* calls that must be reported. + wantLines := map[int]bool{5: true, 12: true, 21: true, 30: true} + got := map[int]bool{} + for _, f := range findings { + got[f.pos.Line] = true + } + if len(findings) != len(wantLines) { + msgs := make([]string, 0, len(findings)) + for _, f := range findings { + msgs = append(msgs, f.String()) + } + t.Fatalf("findings = %d, want %d:\n%s", len(findings), len(wantLines), strings.Join(msgs, "\n")) + } + for line := range wantLines { + if !got[line] { + t.Errorf("no finding on line %d", line) + } + } +} diff --git a/internal/database/postgres/testhelpers/postgres.go b/internal/database/postgres/testhelpers/postgres.go index 71bf95106..00aa661db 100644 --- a/internal/database/postgres/testhelpers/postgres.go +++ b/internal/database/postgres/testhelpers/postgres.go @@ -9,6 +9,7 @@ import ( "github.com/LeanerCloud/CUDly/internal/database" "github.com/jackc/pgx/v5" + "github.com/stretchr/testify/require" "github.com/testcontainers/testcontainers-go" "github.com/testcontainers/testcontainers-go/modules/postgres" "github.com/testcontainers/testcontainers-go/wait" @@ -86,6 +87,34 @@ func SetupPostgresContainer(ctx context.Context, t *testing.T) (*PostgresContain }, nil } +// RequirePostgresContainer starts a PostgreSQL test container for t, skipping +// the test only when this environment has no usable Docker provider and failing +// loudly for every other error. +// +// Drawing that line is the whole point (issue #1597). "No Docker daemon on this +// machine" is an environment fact and the one legitimate reason to skip, so it +// is probed explicitly before anything is started. Past that probe the daemon is +// known healthy, which makes a container that still refuses to come up -- a +// missing image, an exhausted host, a database that never accepts connections -- +// a real failure. Reporting it as a skip would turn a broken run green, which is +// indistinguishable from a run that had nothing to say. +// +// Callers remain responsible for Cleanup, matching SetupPostgresContainer. +func RequirePostgresContainer(ctx context.Context, t *testing.T) *PostgresContainer { + t.Helper() + + // Probes the Docker provider and skips with its own diagnostic when the + // daemon is unreachable or unhealthy. + testcontainers.SkipIfProviderIsNotHealthy(t) + + container, err := SetupPostgresContainer(ctx, t) + require.NoError(t, err, + "Docker is healthy but the PostgreSQL test container did not come up; "+ + "this is a real failure, not an environment without Docker") + + return container +} + // Cleanup terminates the test container and closes database connection. func (c *PostgresContainer) Cleanup(ctx context.Context) error { if c.DB != nil { From bfa12c22e2fbae6542c6038d5bcc9aaff8aa0599 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 14 Aug 2026 01:20:30 +0200 Subject: [PATCH 2/4] fix(test): make the false-green guard fail per guarded name, not only on a total blackout Adversarial review of the previous commit found three real gaps. The anti-vacuity tripwire fired only when the guard found zero guarded calls anywhere. Measured on this tree that total is 188, so it was unreachable from any partial regression: excluding one directory from the walk, or breaking three of the four names, left the tripwire green while the findings for those names silently went to zero. The floor is now per name, which is what the comment always claimed to cover. The walk only parsed _test.go, but this fix moved the skip decision into testhelpers/postgres.go, a non-test file. A future helper that skipped on a container error would have been written in exactly the place the guard could not see. It now also parses the non-test files of the testhelpers and testutil packages. internal/database/coverage_extra_test.go skipped on buildPoolConfig and pgxpool.NewWithConfig errors. Neither does any I/O -- the first parses and validates a DSN, the second builds a lazy pool -- so a failure there is a code defect, not an absent database, and this file carries no //go:build integration tag, so it runs on every default go test. Same defect class as #1597, wider blast radius. Also corrects the RequirePostgresContainer doc, which claimed an exhausted host would fail: SkipIfProviderIsNotHealthy skips on any unhealthy daemon, so the probe owns the whole environment side of the line. Refs #1597 --- internal/database/coverage_extra_test.go | 14 +- .../false_green_skip_guard_test.go | 121 +++++++++++------- .../database/postgres/testhelpers/postgres.go | 15 ++- 3 files changed, 91 insertions(+), 59 deletions(-) diff --git a/internal/database/coverage_extra_test.go b/internal/database/coverage_extra_test.go index 0a62a2173..3f103953d 100644 --- a/internal/database/coverage_extra_test.go +++ b/internal/database/coverage_extra_test.go @@ -258,14 +258,16 @@ func newLazyPool(t *testing.T) *pgxpool.Pool { ConnectTimeout: time.Second, LogLevel: "error", } + // Neither of these touches the network: buildPoolConfig only parses and + // validates the DSN, and NewWithConfig with MinConnections 0 builds a lazy + // pool. A failure is therefore a code defect, not an absent database, and + // skipping on it would report the suite green on exactly that defect + // (issue #1597). poolConfig, err := buildPoolConfig(cfg, "testpass") - if err != nil { - t.Skipf("cannot build pool config: %v", err) - } + require.NoError(t, err, "buildPoolConfig does no I/O; a failure here is a code defect") + pool, err := pgxpool.NewWithConfig(context.Background(), poolConfig) - if err != nil { - t.Skipf("cannot create lazy pool: %v", err) - } + require.NoError(t, err, "NewWithConfig builds a lazy pool; a failure here is a code defect") return pool } diff --git a/internal/database/postgres/testhelpers/false_green_skip_guard_test.go b/internal/database/postgres/testhelpers/false_green_skip_guard_test.go index 8deb5fd03..44e665158 100644 --- a/internal/database/postgres/testhelpers/false_green_skip_guard_test.go +++ b/internal/database/postgres/testhelpers/false_green_skip_guard_test.go @@ -15,23 +15,18 @@ import ( // This guard closes the class of defect behind #1597 for tests not yet written, // rather than only the call sites that exist today. // -// t.Skip reports a test as neither passed nor failed. That is the right answer -// for "this environment cannot run the test at all" and the wrong answer for -// anything the test was written to detect. The DB-backed suites used it for -// both: a testcontainer that would not start AND a set of migrations that would -// not apply took the same t.Skipf branch, so breaking any SQL file under -// internal/database/postgres/migrations turned the whole integration suite -// green. -// -// The rule enforced here is not "no skips". It is that a skip predicate must -// test the precondition it claims to. Docker being absent is probed directly by -// RequirePostgresContainer; that is the only thing allowed to skip. Whether a +// The rule is not "no skips". It is that a skip predicate must test the +// precondition it claims to. Docker being absent is probed directly by +// RequirePostgresContainer and is the only thing allowed to skip; whether a // container came up and whether migrations applied are results, not // preconditions, so an error from either has to fail. // -// The check is parse-only, so it costs milliseconds and needs no build tags, -// which matters because every file it guards is behind //go:build integration -// and would otherwise be invisible to a default `go test`. +// Parse-only, so it needs no build tag and still sees the files behind +// //go:build integration that a default `go test` never compiles. +// +// Deliberate boundary: this finds skips, not every way to report a false green. +// `if err != nil { t.Logf(...); return }` reports PASS, which is worse, but it +// is indistinguishable from an ordinary early return without type information. // guardedCalls are the calls whose error must never be answered with a skip, // mapped to why. @@ -63,7 +58,8 @@ func TestNoSkipOnDatabaseSetupFailure(t *testing.T) { fset := token.NewFileSet() var findings []finding - files, calls := 0, 0 + files := 0 + calls := map[string]int{} err := filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error { if err != nil { @@ -76,7 +72,7 @@ func TestNoSkipOnDatabaseSetupFailure(t *testing.T) { } return nil } - if !strings.HasSuffix(path, "_test.go") { + if !isGuardedSource(path) { return nil } files++ @@ -87,8 +83,10 @@ func TestNoSkipOnDatabaseSetupFailure(t *testing.T) { if parseErr != nil { return nil } - n, fs := inspectFile(fset, f) - calls += n + perName, fs := inspectFile(fset, f) + for name, n := range perName { + calls[name] += n + } findings = append(findings, fs...) return nil }) @@ -97,16 +95,23 @@ func TestNoSkipOnDatabaseSetupFailure(t *testing.T) { } // "Found nothing" and "looked at nothing" are the same color on a terminal, - // and telling them apart is precisely what this guard is about. The tripwire - // counts guarded CALLS rather than the if statements checking them: once the - // suites are fixed they check with require.NoError and hardly any of the if - // shape survives, so a count of those would sit near zero and the guard could - // lose its file discovery, its parsing or its name matching without saying so. - if calls == 0 { - t.Fatal("guard found zero calls to any guarded function; its detection has broken " + - "and it would report success on a repo that skips on every migration failure") + // and telling them apart is precisely what this guard is about. The floor is + // per name, not a total: a total stays comfortably non-zero while an excluded + // directory or one broken name silently drops that name's findings to zero. + var missing []string + for name := range guardedCalls { + if calls[name] == 0 { + missing = append(missing, name) + } } - t.Logf("found %d guarded call(s) across %d test file(s)", calls, files) + if len(missing) > 0 { + sort.Strings(missing) + t.Fatalf("guard saw no call to %s anywhere: its file discovery, parsing or name "+ + "matching has regressed and findings for it would silently be zero. If the call "+ + "is genuinely gone from the repo, drop it from guardedCalls deliberately.", + strings.Join(missing, ", ")) + } + t.Logf("found %v across %d file(s)", calls, files) if len(findings) > 0 { msgs := make([]string, 0, len(findings)) @@ -119,19 +124,35 @@ func TestNoSkipOnDatabaseSetupFailure(t *testing.T) { } } -// inspectFile reports how many guarded calls the file makes and which of them -// are answered with a skip. The repo-wide run and the self-test both go through -// it, so the self-test cannot pass against logic the real run does not have. +// isGuardedSource reports whether a file can hold a skip decision: any test +// file, plus the non-test files of the shared test-helper packages. #1597 moved +// the decision into testhelpers/postgres.go, so that is now exactly where the +// next helper that skips would be written. +func isGuardedSource(path string) bool { + if strings.HasSuffix(path, "_test.go") { + return true + } + if !strings.HasSuffix(path, ".go") { + return false + } + switch filepath.Base(filepath.Dir(path)) { + case "testhelpers", "testutil": + return true + } + return false +} + +// inspectFile reports how many times the file calls each guarded function and +// which of those are answered with a skip. The repo-wide run and the self-test +// both go through it, so the self-test cannot pass against logic the real run +// does not have. // // A finding is an if statement that mentions a guarded call in its init or -// condition and skips in its body. That covers `if err := f(); err != nil` and -// the `x, err := f()` / `if err != nil` pair alike: for the latter the call is -// resolved through the preceding assignment in the same statement list. Each -// list gets its own assignment scope, because reusing `err` across tests in one -// file is the norm and a file-wide map would attribute one test's call to -// another's check. -func inspectFile(fset *token.FileSet, f *ast.File) (int, []finding) { - calls := 0 +// condition and skips in its body, which covers `if err := f(); err != nil` and +// the `x, err := f()` / `if err != nil` pair alike. Each statement list gets its +// own assignment scope, since reusing `err` across tests in one file is the norm. +func inspectFile(fset *token.FileSet, f *ast.File) (map[string]int, []finding) { + calls := map[string]int{} var findings []finding scan := func(list []ast.Stmt) { @@ -154,9 +175,8 @@ func inspectFile(fset *token.FileSet, f *ast.File) (int, []finding) { } } - // Every statement list that can hold an assignment followed by its check is - // scanned with its own scope. Descending unconditionally means nested - // blocks, closures and subtests are reached as lists in their own right. + // Descending unconditionally means nested blocks, closures and subtests are + // reached as statement lists in their own right, each with its own scope. ast.Inspect(f, func(n ast.Node) bool { switch v := n.(type) { case *ast.BlockStmt: @@ -164,8 +184,8 @@ func inspectFile(fset *token.FileSet, f *ast.File) (int, []finding) { case *ast.CaseClause: scan(v.Body) } - if guardedCallName(n) != "" { - calls++ + if name := guardedCallName(n); name != "" { + calls[name]++ } return true }) @@ -364,10 +384,17 @@ func TestUnrelatedSkip(t *testing.T) { calls, findings := inspectFile(fset, f) - // One guarded call in each function except TestUnrelatedSkip, which makes - // none. This is the same count the repo-wide tripwire relies on. - if calls != 6 { - t.Errorf("calls = %d guarded call(s), want 6", calls) + // Per-name counts, the same measurement the repo-wide floor relies on. + wantCalls := map[string]int{ + "RunMigrations": 2, // TestInlineSkip, TestFails + "SetupPostgresContainer": 2, // TestTwoStatementSkip, TestRebindClearsAssociation + "MigrateToVersion": 1, + "RollbackMigrations": 1, + } + for name, want := range wantCalls { + if calls[name] != want { + t.Errorf("calls[%q] = %d, want %d", name, calls[name], want) + } } // Line numbers of the four t.Skip* calls that must be reported. diff --git a/internal/database/postgres/testhelpers/postgres.go b/internal/database/postgres/testhelpers/postgres.go index 00aa661db..59130fc4f 100644 --- a/internal/database/postgres/testhelpers/postgres.go +++ b/internal/database/postgres/testhelpers/postgres.go @@ -91,14 +91,17 @@ func SetupPostgresContainer(ctx context.Context, t *testing.T) (*PostgresContain // the test only when this environment has no usable Docker provider and failing // loudly for every other error. // -// Drawing that line is the whole point (issue #1597). "No Docker daemon on this -// machine" is an environment fact and the one legitimate reason to skip, so it -// is probed explicitly before anything is started. Past that probe the daemon is -// known healthy, which makes a container that still refuses to come up -- a -// missing image, an exhausted host, a database that never accepts connections -- -// a real failure. Reporting it as a skip would turn a broken run green, which is +// Drawing that line is the whole point (issue #1597). "No usable Docker daemon" +// is an environment fact and the one legitimate reason to skip, so it is probed +// explicitly before anything is started. Past that probe the daemon answered a +// health check, which makes a container that still refuses to come up -- a +// missing image, a database that never accepts connections -- a real failure. +// Reporting it as a skip would turn a broken run green, which is // indistinguishable from a run that had nothing to say. // +// The probe owns the whole environment side of that line, so a daemon that is +// present but unhealthy skips too, rather than failing. +// // Callers remain responsible for Cleanup, matching SetupPostgresContainer. func RequirePostgresContainer(ctx context.Context, t *testing.T) *PostgresContainer { t.Helper() From c7da3440bcb127aee71335d97696ce34feb84a6b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 14 Aug 2026 01:40:13 +0200 Subject: [PATCH 3/4] fix(test): terminate leaked containers and stop overclaiming the skip guard's reach Adversarial review of the previous two commits found four things worth fixing. SetupPostgresContainer terminated the container when NewConnection failed but not when Host or MappedPort did, so those two paths returned an error with the container still running and nothing left holding a handle to call Cleanup on. The reviewer hit exactly that path: MappedPort returned `port "5432/tcp" not found` under container churn, and the container leaked. This matters more after this PR than before it, because the paths that used to end in a skip now end in a failure, so a bad run accumulates live containers rather than skipped tests. All three post-Run error paths now go through one terminate helper. That helper does not forward its caller's context. The error it cleans up after is very often the context expiring in the first place, and Terminate on a dead context returns immediately without stopping anything, so forwarding would have left exactly the leak the helper exists to prevent. It derives a detached context instead. TestTerminateAfterErrorSurvivesDeadContext pins this against a canceled parent and an expired deadline, and fails by assertion on both if the detach is removed while its live-parent case keeps passing. The guard's header claimed it "closes the class of defect behind #1597 for tests not yet written". It does not, and the claim is the kind that stops the next reader from looking. Probing inspectFile directly found four shapes it does not detect: a skip behind a helper, a switch case rather than an if, an assignment in the parent with the check inside a t.Run closure, and a wrapper that returns the guarded call's error. Two are realistic, and "extract a setup helper" is the natural next refactor over this code. The header now says tripwire rather than proof, and rather than listing the shapes in prose where they would drift, TestFalseGreenSkipGuardKnownBypasses executes all four. Each case also asserts the guarded call stays countable, so a bypass hides a finding but cannot also silence the per-name floor. If the guard is later widened to catch one, that test fails and says to move the case into the detects test. The walk pruned any directory named frontend at any depth rather than the TypeScript tree at the repo root. terraform/modules/frontend was being pruned by that; it holds no Go files, so nothing was lost, but a Go package named frontend would have been dropped silently. The prune is now anchored to the root. files++ ran before parser.ParseFile, so the reassuring total in the log counted files that may never have been inspected, which is the same "found nothing and looked at nothing are the same color" reasoning the per-name floor exists for. It now counts only parsed files: 386, which is exactly the 382 test files plus the 4 non-test files in testhelpers and testutil, so no file is failing to parse today. Also renames analytics' skipIfNoDocker to skipIfDBTestsOptedOut. It never probed Docker; it reads SKIP_DB_TESTS. The old name is what makes a reader of #1597 believe a suite reporting nothing was accounted for. The opt-out itself stays: it is a caller's decision rather than an error answered with a skip, and nothing in the tree sets the variable outside tests. Refs #1597 --- .../analytics/postgres_analytics_db_test.go | 34 +++-- .../false_green_skip_guard_test.go | 130 ++++++++++++++++-- .../database/postgres/testhelpers/postgres.go | 24 +++- .../testhelpers/postgres_terminate_test.go | 85 ++++++++++++ 4 files changed, 247 insertions(+), 26 deletions(-) create mode 100644 internal/database/postgres/testhelpers/postgres_terminate_test.go diff --git a/internal/analytics/postgres_analytics_db_test.go b/internal/analytics/postgres_analytics_db_test.go index 4d0c557e7..1e76fcb9d 100644 --- a/internal/analytics/postgres_analytics_db_test.go +++ b/internal/analytics/postgres_analytics_db_test.go @@ -19,14 +19,24 @@ import ( ) // These tests run against a real PostgreSQL database using testcontainers. -// They will be skipped if Docker is not available or if SKIP_DB_TESTS is set. // To run these tests: go test ./internal/analytics/ -v // To skip these tests: SKIP_DB_TESTS=1 go test ./internal/analytics/ -func skipIfNoDocker(t *testing.T) { +// skipIfDBTestsOptedOut honors the SKIP_DB_TESTS opt-out. It was called +// skipIfNoDocker, which is what a reader of #1597 would flag: the name promised +// a Docker probe, so a suite that silently reported nothing looked accounted +// for. It never probed anything. Docker is probed by RequirePostgresContainer, +// which each of these tests calls, and which fails rather than skips once the +// daemon answers a health check. +// +// The remaining exposure is the opt-out itself: setting SKIP_DB_TESTS zeroes +// this suite without failing. That is deliberate and it is the caller's +// decision, not an error answered with a skip, so the #1597 guard does not +// treat it as a finding. Nothing under .github/ sets the variable, so the suite +// really runs in CI today. +func skipIfDBTestsOptedOut(t *testing.T) { t.Helper() - // Skip if SKIP_DB_TESTS is set (CI without a live DB, or local opt-out). if os.Getenv("SKIP_DB_TESTS") != "" { t.Skip("Skipping database tests (SKIP_DB_TESTS is set)") } @@ -42,7 +52,7 @@ func getMigrationsPath() string { func f64ptr(v float64) *float64 { return &v } func TestPostgresAnalyticsStore_SaveSnapshot_DB(t *testing.T) { - skipIfNoDocker(t) + skipIfDBTestsOptedOut(t) ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) defer cancel() @@ -123,7 +133,7 @@ func TestPostgresAnalyticsStore_SaveSnapshot_DB(t *testing.T) { } func TestPostgresAnalyticsStore_QuerySavings_DB(t *testing.T) { - skipIfNoDocker(t) + skipIfDBTestsOptedOut(t) ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) defer cancel() @@ -254,7 +264,7 @@ func TestPostgresAnalyticsStore_QuerySavings_DB(t *testing.T) { } func TestPostgresAnalyticsStore_QueryByProvider_DB(t *testing.T) { - skipIfNoDocker(t) + skipIfDBTestsOptedOut(t) ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) defer cancel() @@ -332,7 +342,7 @@ func TestPostgresAnalyticsStore_QueryByProvider_DB(t *testing.T) { } func TestPostgresAnalyticsStore_QueryByService_DB(t *testing.T) { - skipIfNoDocker(t) + skipIfDBTestsOptedOut(t) ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) defer cancel() @@ -394,7 +404,7 @@ func TestPostgresAnalyticsStore_QueryByService_DB(t *testing.T) { } func TestPostgresAnalyticsStore_BulkInsertSnapshots_DB(t *testing.T) { - skipIfNoDocker(t) + skipIfDBTestsOptedOut(t) ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) defer cancel() @@ -454,7 +464,7 @@ func TestPostgresAnalyticsStore_BulkInsertSnapshots_DB(t *testing.T) { } func TestPostgresAnalyticsStore_PartitionManagement_DB(t *testing.T) { - skipIfNoDocker(t) + skipIfDBTestsOptedOut(t) ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) defer cancel() @@ -497,7 +507,7 @@ func TestPostgresAnalyticsStore_PartitionManagement_DB(t *testing.T) { } func TestPostgresAnalyticsStore_QueryMonthlyTotals_DB(t *testing.T) { - skipIfNoDocker(t) + skipIfDBTestsOptedOut(t) ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) defer cancel() @@ -554,7 +564,7 @@ func TestPostgresAnalyticsStore_QueryMonthlyTotals_DB(t *testing.T) { } func TestPostgresAnalyticsStore_RefreshMaterializedViews_DB(t *testing.T) { - skipIfNoDocker(t) + skipIfDBTestsOptedOut(t) ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) defer cancel() @@ -577,7 +587,7 @@ func TestPostgresAnalyticsStore_RefreshMaterializedViews_DB(t *testing.T) { } func TestPostgresAnalyticsStore_Close_DB(t *testing.T) { - skipIfNoDocker(t) + skipIfDBTestsOptedOut(t) ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) defer cancel() diff --git a/internal/database/postgres/testhelpers/false_green_skip_guard_test.go b/internal/database/postgres/testhelpers/false_green_skip_guard_test.go index 44e665158..f453310b7 100644 --- a/internal/database/postgres/testhelpers/false_green_skip_guard_test.go +++ b/internal/database/postgres/testhelpers/false_green_skip_guard_test.go @@ -12,8 +12,9 @@ import ( "testing" ) -// This guard closes the class of defect behind #1597 for tests not yet written, -// rather than only the call sites that exist today. +// This guard is a tripwire against the defect behind #1597, not a proof of its +// absence. It catches the direct shapes, which is what raises the cost of +// reintroducing the defect; it does not close the class. // // The rule is not "no skips". It is that a skip predicate must test the // precondition it claims to. Docker being absent is probed directly by @@ -22,11 +23,15 @@ import ( // preconditions, so an error from either has to fail. // // Parse-only, so it needs no build tag and still sees the files behind -// //go:build integration that a default `go test` never compiles. +// //go:build integration that a default `go test` never compiles. Having no +// type information is what buys that, and it is paid for in shapes this +// implementation does not see. They are deliberately not listed here in prose: +// TestFalseGreenSkipGuardKnownBypasses executes each one, so what this guard +// misses cannot quietly drift away from what the comment claims it misses. // -// Deliberate boundary: this finds skips, not every way to report a false green. -// `if err != nil { t.Logf(...); return }` reports PASS, which is worse, but it -// is indistinguishable from an ordinary early return without type information. +// It finds skips, not every way to report a false green: `if err != nil { +// t.Logf(...); return }` reports PASS, which is worse, but it is +// indistinguishable from an ordinary early return without type information. // guardedCalls are the calls whose error must never be answered with a skip, // mapped to why. @@ -58,7 +63,7 @@ func TestNoSkipOnDatabaseSetupFailure(t *testing.T) { fset := token.NewFileSet() var findings []finding - files := 0 + parsed := 0 calls := map[string]int{} err := filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error { @@ -67,7 +72,13 @@ func TestNoSkipOnDatabaseSetupFailure(t *testing.T) { } if d.IsDir() { switch d.Name() { - case ".git", "node_modules", "vendor", ".terraform", "frontend": + case ".git", "node_modules", "vendor", ".terraform": + return filepath.SkipDir + } + // Anchored, unlike the names above: it prunes the TypeScript tree + // at the repo root, and must not silently swallow a Go package + // that happens to be called frontend. + if path == filepath.Join(root, "frontend") { return filepath.SkipDir } return nil @@ -75,14 +86,15 @@ func TestNoSkipOnDatabaseSetupFailure(t *testing.T) { if !isGuardedSource(path) { return nil } - files++ // A file that does not parse is the compiler's problem, not this // guard's: a package that does not compile has no passing tests to - // falsely report green either. + // falsely report green either. It is counted only once it has actually + // been inspected, so the total below cannot overstate the coverage. f, parseErr := parser.ParseFile(fset, path, nil, 0) if parseErr != nil { return nil } + parsed++ perName, fs := inspectFile(fset, f) for name, n := range perName { calls[name] += n @@ -111,7 +123,7 @@ func TestNoSkipOnDatabaseSetupFailure(t *testing.T) { "is genuinely gone from the repo, drop it from guardedCalls deliberately.", strings.Join(missing, ", ")) } - t.Logf("found %v across %d file(s)", calls, files) + t.Logf("found %v across %d parsed file(s)", calls, parsed) if len(findings) > 0 { msgs := make([]string, 0, len(findings)) @@ -416,3 +428,99 @@ func TestUnrelatedSkip(t *testing.T) { } } } + +// TestFalseGreenSkipGuardKnownBypasses records what the guard does not catch, +// so the header's boundary is executed rather than asserted. Each case is a +// real false green that would slip past. +// +// This is documentation, not a requirement: widening the guard to catch one of +// these is an improvement, and the failure it produces here is the prompt to +// move that case into TestFalseGreenSkipGuardDetects and update the header. +// +// Every case still leaves the guarded call countable, which is the part that +// matters operationally: a bypass hides a finding, but it cannot also silence +// the per-name floor into reporting a clean repo it never looked at. +func TestFalseGreenSkipGuardKnownBypasses(t *testing.T) { + cases := []struct { + name string + why string + wantCall string + src string + }{ + { + name: "skip behind a helper", + why: "only a t.Skip* selector counts as a skip, so any wrapper hides it", + wantCall: "RunMigrations", + src: `package p +func TestX(t *testing.T) { + if err := migrations.RunMigrations(ctx, pool, path, "", ""); err != nil { + skipDB(t, err) + } +}`, + }, + { + name: "switch case rather than an if", + why: "a finding is an IfStmt; a CaseClause guard is never considered", + wantCall: "RunMigrations", + src: `package p +func TestX(t *testing.T) { + err := migrations.RunMigrations(ctx, pool, path, "", "") + switch { + case err != nil: + t.Skipf("nope: %v", err) + } +}`, + }, + { + name: "assignment in the parent, check inside a t.Run closure", + why: "each statement list carries its own scope, so err is unresolved in the closure", + wantCall: "SetupPostgresContainer", + src: `package p +func TestX(t *testing.T) { + _, err := testhelpers.SetupPostgresContainer(ctx, t) + t.Run("sub", func(t *testing.T) { + if err != nil { + t.Skip("no container") + } + }) +}`, + }, + { + name: "wrapper returning the guarded call's error", + why: "the if statement names only the wrapper, and there is no type information to follow it", + wantCall: "RunMigrations", + src: `package p +func setup(t *testing.T) error { + return migrations.RunMigrations(ctx, pool, path, "", "") +} +func TestX(t *testing.T) { + if err := setup(t); err != nil { + t.Skip("setup failed") + } +}`, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "synthetic.go", tc.src, 0) + if err != nil { + t.Fatalf("parse: %v", err) + } + + calls, findings := inspectFile(fset, f) + + if len(findings) > 0 { + t.Errorf("the guard now catches this shape (%s). That is an improvement: "+ + "move the case into TestFalseGreenSkipGuardDetects and drop it from the "+ + "header's boundary list.", tc.why) + } + if calls[tc.wantCall] != 1 { + t.Errorf("calls[%q] = %d, want 1: the bypass must still leave the call "+ + "countable, or it would disarm the per-name floor as well", + tc.wantCall, calls[tc.wantCall]) + } + }) + } +} diff --git a/internal/database/postgres/testhelpers/postgres.go b/internal/database/postgres/testhelpers/postgres.go index 59130fc4f..11fc95d9f 100644 --- a/internal/database/postgres/testhelpers/postgres.go +++ b/internal/database/postgres/testhelpers/postgres.go @@ -44,11 +44,13 @@ func SetupPostgresContainer(ctx context.Context, t *testing.T) (*PostgresContain // Get connection details host, err := postgresContainer.Host(ctx) if err != nil { + terminateAfterError(ctx, postgresContainer, "container host lookup") return nil, fmt.Errorf("failed to get container host: %w", err) } port, err := postgresContainer.MappedPort(ctx, "5432") if err != nil { + terminateAfterError(ctx, postgresContainer, "container port lookup") return nil, fmt.Errorf("failed to get container port: %w", err) } @@ -74,9 +76,7 @@ func SetupPostgresContainer(ctx context.Context, t *testing.T) (*PostgresContain // Create database connection db, err := database.NewConnection(ctx, config, nil) if err != nil { - if termErr := postgresContainer.Terminate(ctx); termErr != nil { - log.Printf("testhelpers: failed to terminate postgres container after DB connect error: %v", termErr) - } + terminateAfterError(ctx, postgresContainer, "database connect") return nil, fmt.Errorf("failed to connect to database: %w", err) } @@ -87,6 +87,24 @@ func SetupPostgresContainer(ctx context.Context, t *testing.T) (*PostgresContain }, nil } +// terminateAfterError stops a container that started but will not be returned +// to the caller, so nothing is left to call Cleanup on it. Every error path +// after postgres.Run needs this: #1597 turned them from skips into failures, so +// a bad run now accumulates live containers where it used to accumulate skips. +// Ryuk would reap them eventually; not relying on that is cheaper than the bug. +func terminateAfterError(ctx context.Context, c testcontainers.Container, stage string) { + // Detached deliberately. The error that brought us here is very often the + // context expiring, and Terminate on an already-dead context returns + // immediately without stopping anything, so passing ctx straight through + // would leave exactly the leak this function exists to prevent. + termCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 30*time.Second) + defer cancel() + + if err := c.Terminate(termCtx); err != nil { + log.Printf("testhelpers: failed to terminate postgres container after %s failed: %v", stage, err) + } +} + // RequirePostgresContainer starts a PostgreSQL test container for t, skipping // the test only when this environment has no usable Docker provider and failing // loudly for every other error. diff --git a/internal/database/postgres/testhelpers/postgres_terminate_test.go b/internal/database/postgres/testhelpers/postgres_terminate_test.go new file mode 100644 index 000000000..182e249a2 --- /dev/null +++ b/internal/database/postgres/testhelpers/postgres_terminate_test.go @@ -0,0 +1,85 @@ +package testhelpers + +import ( + "context" + "errors" + "testing" + + "github.com/testcontainers/testcontainers-go" +) + +// recordingContainer captures the context Terminate is handed. The embedded +// interface is nil on purpose: only Terminate is exercised, and a call to +// anything else should panic rather than pass quietly. +type recordingContainer struct { + testcontainers.Container + called bool + ctxErr error +} + +func (r *recordingContainer) Terminate(ctx context.Context, _ ...testcontainers.TerminateOption) error { + r.called = true + r.ctxErr = ctx.Err() + return nil +} + +// failingContainer reports a teardown that itself fails. +type failingContainer struct { + testcontainers.Container + err error + called bool +} + +func (f *failingContainer) Terminate(context.Context, ...testcontainers.TerminateOption) error { + f.called = true + return f.err +} + +// TestTerminateAfterErrorSurvivesDeadContext pins the reason terminateAfterError +// does not simply forward its caller's context. The failures it cleans up after +// are frequently the context expiring, and Terminate on a canceled context +// returns without stopping anything, so forwarding would leave precisely the +// leaked container the helper exists to prevent. +func TestTerminateAfterErrorSurvivesDeadContext(t *testing.T) { + for _, tc := range []struct { + name string + ctx func() context.Context + }{ + {"canceled parent", func() context.Context { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + return ctx + }}, + {"expired deadline", func() context.Context { + ctx, cancel := context.WithTimeout(context.Background(), 0) + t.Cleanup(cancel) + return ctx + }}, + {"live parent", func() context.Context { return context.Background() }}, + } { + t.Run(tc.name, func(t *testing.T) { + c := &recordingContainer{} + terminateAfterError(tc.ctx(), c, "unit test") + + if !c.called { + t.Fatal("Terminate was never called, so the container would leak") + } + if c.ctxErr != nil { + t.Errorf("Terminate got an already-dead context (%v); it returns without "+ + "stopping the container, so the cleanup is a no-op", c.ctxErr) + } + }) + } +} + +// TestTerminateAfterErrorReportsFailure documents that a failing Terminate is +// logged rather than propagated: the caller is already returning the error that +// caused the cleanup, and losing that to a teardown error would be worse. +func TestTerminateAfterErrorReportsFailure(t *testing.T) { + c := &failingContainer{err: errors.New("boom")} + terminateAfterError(context.Background(), c, "unit test") + + if !c.called { + t.Fatal("Terminate was never called") + } +} From a079a1f2138c5e30192c41f9a40c9dabe4aeb0d4 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 18 Aug 2026 03:53:19 +0200 Subject: [PATCH 4/4] refactor(test): split the false-green guard's case tables into their own file false_green_skip_guard_test.go had grown to 526 lines against the project's 500-line limit. The file is new in this PR, so the violation is this change's own rather than pre-existing debt, and it is in scope to fix here. TestFalseGreenSkipGuardDetects and TestFalseGreenSkipGuardKnownBypasses move verbatim into false_green_skip_guard_cases_test.go in the same package. The detector itself, the repo-wide walk in TestNoSkipOnDatabaseSetupFailure, and the guardedCalls and skipFuncs tables stay where they were. The split falls on the natural seam: what the detector is stays in one file, the synthetic sources proving what it does and does not catch move to the other. The two files are now 329 and 205 lines. This is a pure move. 196 lines were transplanted byte for byte, with no logic, signature or name changes; the only authored lines are the new file's package clause and its four-import block. Neither file carries a build tag, before or after, which is what lets the guard keep seeing the files behind //go:build integration that a default `go test` never compiles. The package's test set is identical before and after: the same 5 top-level tests and 7 subtests, all passing, none skipped, both with and without -tags=integration. Complexity attribution is unchanged as well, with TestNoSkipOnDatabaseSetupFailure still at 14 and TestFalseGreenSkipGuardDetects still at 9, and both the CI and pre-commit gocyclo gates exclude _test.go. Refs #1597 --- .../false_green_skip_guard_cases_test.go | 205 ++++++++++++++++++ .../false_green_skip_guard_test.go | 197 ----------------- 2 files changed, 205 insertions(+), 197 deletions(-) create mode 100644 internal/database/postgres/testhelpers/false_green_skip_guard_cases_test.go diff --git a/internal/database/postgres/testhelpers/false_green_skip_guard_cases_test.go b/internal/database/postgres/testhelpers/false_green_skip_guard_cases_test.go new file mode 100644 index 000000000..dfc074c5c --- /dev/null +++ b/internal/database/postgres/testhelpers/false_green_skip_guard_cases_test.go @@ -0,0 +1,205 @@ +package testhelpers + +import ( + "go/parser" + "go/token" + "strings" + "testing" +) + +// TestFalseGreenSkipGuardDetects exercises the detector on synthetic source +// covering each shape it must separate. Without it the guard could break into +// reporting nothing and the repo-wide run above would still look green, which is +// the failure mode #1597 is about, one level up. +func TestFalseGreenSkipGuardDetects(t *testing.T) { + const src = `package p + +func TestInlineSkip(t *testing.T) { + if err := migrations.RunMigrations(ctx, pool, path, "", ""); err != nil { + t.Skipf("migrations failed: %v", err) // want: finding + } +} + +func TestTwoStatementSkip(t *testing.T) { + container, err := testhelpers.SetupPostgresContainer(ctx, t) + if err != nil { + t.Skipf("no container: %v", err) // want: finding + } + _ = container +} + +func TestNestedSkip(t *testing.T) { + err := migrations.MigrateToVersion(ctx, pool, path, 42) + if err != nil { + if strings.Contains(err.Error(), "dirty") { + t.Skip("dirty") // want: finding, a nested skip still ends without a verdict + } + } +} + +func TestSubtestSkip(t *testing.T) { + t.Run("sub", func(t *testing.T) { + err := migrations.RollbackMigrations(ctx, pool, path, 1) + if err != nil { + t.Skipf("rollback failed: %v", err) // want: finding, closures are walked + } + }) +} + +func TestFails(t *testing.T) { + if err := migrations.RunMigrations(ctx, pool, path, "", ""); err != nil { + t.Fatalf("migrations failed: %v", err) // want: inspected, no finding + } +} + +func TestRebindClearsAssociation(t *testing.T) { + _, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + err = os.Setenv("X", "1") + if err != nil { + t.Skip("unrelated") // want: not inspected, err no longer holds the guarded call + } +} + +func TestUnrelatedSkip(t *testing.T) { + if os.Getenv("SKIP_DB_TESTS") != "" { + t.Skip("opt-out") // want: not inspected, an explicit environment opt-out + } +} +` + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "synthetic.go", src, 0) + if err != nil { + t.Fatalf("parse: %v", err) + } + + calls, findings := inspectFile(fset, f) + + // Per-name counts, the same measurement the repo-wide floor relies on. + wantCalls := map[string]int{ + "RunMigrations": 2, // TestInlineSkip, TestFails + "SetupPostgresContainer": 2, // TestTwoStatementSkip, TestRebindClearsAssociation + "MigrateToVersion": 1, + "RollbackMigrations": 1, + } + for name, want := range wantCalls { + if calls[name] != want { + t.Errorf("calls[%q] = %d, want %d", name, calls[name], want) + } + } + + // Line numbers of the four t.Skip* calls that must be reported. + wantLines := map[int]bool{5: true, 12: true, 21: true, 30: true} + got := map[int]bool{} + for _, f := range findings { + got[f.pos.Line] = true + } + if len(findings) != len(wantLines) { + msgs := make([]string, 0, len(findings)) + for _, f := range findings { + msgs = append(msgs, f.String()) + } + t.Fatalf("findings = %d, want %d:\n%s", len(findings), len(wantLines), strings.Join(msgs, "\n")) + } + for line := range wantLines { + if !got[line] { + t.Errorf("no finding on line %d", line) + } + } +} + +// TestFalseGreenSkipGuardKnownBypasses records what the guard does not catch, +// so the header's boundary is executed rather than asserted. Each case is a +// real false green that would slip past. +// +// This is documentation, not a requirement: widening the guard to catch one of +// these is an improvement, and the failure it produces here is the prompt to +// move that case into TestFalseGreenSkipGuardDetects and update the header. +// +// Every case still leaves the guarded call countable, which is the part that +// matters operationally: a bypass hides a finding, but it cannot also silence +// the per-name floor into reporting a clean repo it never looked at. +func TestFalseGreenSkipGuardKnownBypasses(t *testing.T) { + cases := []struct { + name string + why string + wantCall string + src string + }{ + { + name: "skip behind a helper", + why: "only a t.Skip* selector counts as a skip, so any wrapper hides it", + wantCall: "RunMigrations", + src: `package p +func TestX(t *testing.T) { + if err := migrations.RunMigrations(ctx, pool, path, "", ""); err != nil { + skipDB(t, err) + } +}`, + }, + { + name: "switch case rather than an if", + why: "a finding is an IfStmt; a CaseClause guard is never considered", + wantCall: "RunMigrations", + src: `package p +func TestX(t *testing.T) { + err := migrations.RunMigrations(ctx, pool, path, "", "") + switch { + case err != nil: + t.Skipf("nope: %v", err) + } +}`, + }, + { + name: "assignment in the parent, check inside a t.Run closure", + why: "each statement list carries its own scope, so err is unresolved in the closure", + wantCall: "SetupPostgresContainer", + src: `package p +func TestX(t *testing.T) { + _, err := testhelpers.SetupPostgresContainer(ctx, t) + t.Run("sub", func(t *testing.T) { + if err != nil { + t.Skip("no container") + } + }) +}`, + }, + { + name: "wrapper returning the guarded call's error", + why: "the if statement names only the wrapper, and there is no type information to follow it", + wantCall: "RunMigrations", + src: `package p +func setup(t *testing.T) error { + return migrations.RunMigrations(ctx, pool, path, "", "") +} +func TestX(t *testing.T) { + if err := setup(t); err != nil { + t.Skip("setup failed") + } +}`, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "synthetic.go", tc.src, 0) + if err != nil { + t.Fatalf("parse: %v", err) + } + + calls, findings := inspectFile(fset, f) + + if len(findings) > 0 { + t.Errorf("the guard now catches this shape (%s). That is an improvement: "+ + "move the case into TestFalseGreenSkipGuardDetects and drop it from the "+ + "header's boundary list.", tc.why) + } + if calls[tc.wantCall] != 1 { + t.Errorf("calls[%q] = %d, want 1: the bypass must still leave the call "+ + "countable, or it would disarm the per-name floor as well", + tc.wantCall, calls[tc.wantCall]) + } + }) + } +} diff --git a/internal/database/postgres/testhelpers/false_green_skip_guard_test.go b/internal/database/postgres/testhelpers/false_green_skip_guard_test.go index f453310b7..7560cbb39 100644 --- a/internal/database/postgres/testhelpers/false_green_skip_guard_test.go +++ b/internal/database/postgres/testhelpers/false_green_skip_guard_test.go @@ -327,200 +327,3 @@ func repoRootDir(t *testing.T) string { dir = parent } } - -// TestFalseGreenSkipGuardDetects exercises the detector on synthetic source -// covering each shape it must separate. Without it the guard could break into -// reporting nothing and the repo-wide run above would still look green, which is -// the failure mode #1597 is about, one level up. -func TestFalseGreenSkipGuardDetects(t *testing.T) { - const src = `package p - -func TestInlineSkip(t *testing.T) { - if err := migrations.RunMigrations(ctx, pool, path, "", ""); err != nil { - t.Skipf("migrations failed: %v", err) // want: finding - } -} - -func TestTwoStatementSkip(t *testing.T) { - container, err := testhelpers.SetupPostgresContainer(ctx, t) - if err != nil { - t.Skipf("no container: %v", err) // want: finding - } - _ = container -} - -func TestNestedSkip(t *testing.T) { - err := migrations.MigrateToVersion(ctx, pool, path, 42) - if err != nil { - if strings.Contains(err.Error(), "dirty") { - t.Skip("dirty") // want: finding, a nested skip still ends without a verdict - } - } -} - -func TestSubtestSkip(t *testing.T) { - t.Run("sub", func(t *testing.T) { - err := migrations.RollbackMigrations(ctx, pool, path, 1) - if err != nil { - t.Skipf("rollback failed: %v", err) // want: finding, closures are walked - } - }) -} - -func TestFails(t *testing.T) { - if err := migrations.RunMigrations(ctx, pool, path, "", ""); err != nil { - t.Fatalf("migrations failed: %v", err) // want: inspected, no finding - } -} - -func TestRebindClearsAssociation(t *testing.T) { - _, err := testhelpers.SetupPostgresContainer(ctx, t) - require.NoError(t, err) - err = os.Setenv("X", "1") - if err != nil { - t.Skip("unrelated") // want: not inspected, err no longer holds the guarded call - } -} - -func TestUnrelatedSkip(t *testing.T) { - if os.Getenv("SKIP_DB_TESTS") != "" { - t.Skip("opt-out") // want: not inspected, an explicit environment opt-out - } -} -` - fset := token.NewFileSet() - f, err := parser.ParseFile(fset, "synthetic.go", src, 0) - if err != nil { - t.Fatalf("parse: %v", err) - } - - calls, findings := inspectFile(fset, f) - - // Per-name counts, the same measurement the repo-wide floor relies on. - wantCalls := map[string]int{ - "RunMigrations": 2, // TestInlineSkip, TestFails - "SetupPostgresContainer": 2, // TestTwoStatementSkip, TestRebindClearsAssociation - "MigrateToVersion": 1, - "RollbackMigrations": 1, - } - for name, want := range wantCalls { - if calls[name] != want { - t.Errorf("calls[%q] = %d, want %d", name, calls[name], want) - } - } - - // Line numbers of the four t.Skip* calls that must be reported. - wantLines := map[int]bool{5: true, 12: true, 21: true, 30: true} - got := map[int]bool{} - for _, f := range findings { - got[f.pos.Line] = true - } - if len(findings) != len(wantLines) { - msgs := make([]string, 0, len(findings)) - for _, f := range findings { - msgs = append(msgs, f.String()) - } - t.Fatalf("findings = %d, want %d:\n%s", len(findings), len(wantLines), strings.Join(msgs, "\n")) - } - for line := range wantLines { - if !got[line] { - t.Errorf("no finding on line %d", line) - } - } -} - -// TestFalseGreenSkipGuardKnownBypasses records what the guard does not catch, -// so the header's boundary is executed rather than asserted. Each case is a -// real false green that would slip past. -// -// This is documentation, not a requirement: widening the guard to catch one of -// these is an improvement, and the failure it produces here is the prompt to -// move that case into TestFalseGreenSkipGuardDetects and update the header. -// -// Every case still leaves the guarded call countable, which is the part that -// matters operationally: a bypass hides a finding, but it cannot also silence -// the per-name floor into reporting a clean repo it never looked at. -func TestFalseGreenSkipGuardKnownBypasses(t *testing.T) { - cases := []struct { - name string - why string - wantCall string - src string - }{ - { - name: "skip behind a helper", - why: "only a t.Skip* selector counts as a skip, so any wrapper hides it", - wantCall: "RunMigrations", - src: `package p -func TestX(t *testing.T) { - if err := migrations.RunMigrations(ctx, pool, path, "", ""); err != nil { - skipDB(t, err) - } -}`, - }, - { - name: "switch case rather than an if", - why: "a finding is an IfStmt; a CaseClause guard is never considered", - wantCall: "RunMigrations", - src: `package p -func TestX(t *testing.T) { - err := migrations.RunMigrations(ctx, pool, path, "", "") - switch { - case err != nil: - t.Skipf("nope: %v", err) - } -}`, - }, - { - name: "assignment in the parent, check inside a t.Run closure", - why: "each statement list carries its own scope, so err is unresolved in the closure", - wantCall: "SetupPostgresContainer", - src: `package p -func TestX(t *testing.T) { - _, err := testhelpers.SetupPostgresContainer(ctx, t) - t.Run("sub", func(t *testing.T) { - if err != nil { - t.Skip("no container") - } - }) -}`, - }, - { - name: "wrapper returning the guarded call's error", - why: "the if statement names only the wrapper, and there is no type information to follow it", - wantCall: "RunMigrations", - src: `package p -func setup(t *testing.T) error { - return migrations.RunMigrations(ctx, pool, path, "", "") -} -func TestX(t *testing.T) { - if err := setup(t); err != nil { - t.Skip("setup failed") - } -}`, - }, - } - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - fset := token.NewFileSet() - f, err := parser.ParseFile(fset, "synthetic.go", tc.src, 0) - if err != nil { - t.Fatalf("parse: %v", err) - } - - calls, findings := inspectFile(fset, f) - - if len(findings) > 0 { - t.Errorf("the guard now catches this shape (%s). That is an improvement: "+ - "move the case into TestFalseGreenSkipGuardDetects and drop it from the "+ - "header's boundary list.", tc.why) - } - if calls[tc.wantCall] != 1 { - t.Errorf("calls[%q] = %d, want 1: the bypass must still leave the call "+ - "countable, or it would disarm the per-name floor as well", - tc.wantCall, calls[tc.wantCall]) - } - }) - } -}