diff --git a/internal/analytics/postgres_analytics_db_test.go b/internal/analytics/postgres_analytics_db_test.go index 0016f5365..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,20 +52,17 @@ 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() // 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 @@ -126,20 +133,17 @@ 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() // 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 @@ -260,20 +264,17 @@ 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() // 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 @@ -341,20 +342,17 @@ 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() // 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 @@ -406,20 +404,17 @@ 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() // 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 @@ -469,20 +464,17 @@ 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() // 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 @@ -515,20 +507,17 @@ 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() // 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 @@ -575,20 +564,17 @@ 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() // 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 @@ -601,16 +587,13 @@ 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() // 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/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/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_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 new file mode 100644 index 000000000..7560cbb39 --- /dev/null +++ b/internal/database/postgres/testhelpers/false_green_skip_guard_test.go @@ -0,0 +1,329 @@ +package testhelpers + +import ( + "fmt" + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "sort" + "strings" + "testing" +) + +// 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 +// 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. +// +// Parse-only, so it needs no build tag and still sees the files behind +// //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. +// +// 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. +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 + parsed := 0 + calls := map[string]int{} + + 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": + 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 + } + if !isGuardedSource(path) { + return nil + } + // 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. 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 + } + 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 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) + } + } + 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 parsed file(s)", calls, parsed) + + 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")) + } +} + +// 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, 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) { + 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], + }) + } + } + } + } + + // 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: + scan(v.List) + case *ast.CaseClause: + scan(v.Body) + } + if name := guardedCallName(n); name != "" { + calls[name]++ + } + 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 + } +} diff --git a/internal/database/postgres/testhelpers/postgres.go b/internal/database/postgres/testhelpers/postgres.go index 71bf95106..11fc95d9f 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" @@ -43,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) } @@ -73,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) } @@ -86,6 +87,55 @@ 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. +// +// 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() + + // 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 { 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") + } +}