fix(test): fail integration suites on migration failure instead of skipping

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

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
 }
 

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
 }

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)
 }
 

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)

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
 }