fix(test): integration suites convert a real migration failure into a PASS via t.Skipf #1597

Reviewed commit: be11bdcb5. Note: origin/main moved to 3e9660d06 during the review; re-verify against current main before changing code, since a finding may have been fixed or moved.

Where

• internal/auth/store_postgres_db_test.go:51
• internal/config/store_postgres_db_test.go:50
• internal/config/store_postgres_ladder_test.go:48
• internal/config/store_postgres_unassigned_test.go:49
• Plus 40+ sibling t.Skipf("... could not setup postgres container: %v", err) sites in internal/analytics/postgres_analytics_db_test.go and internal/secrets/*_resolver_coverage_test.go

What

The shared setup does:

if err := migrations.RunMigrations(...); err != nil {
    container.Cleanup(ctx)
    t.Skipf("Skipping DB test: cannot run migrations: %v", err)
    return nil
}

t.Skipf on a migration failure conflates two different things. "The container would not start" is an environment fact and a legitimate skip. "The migrations would not apply" is the failure of the thing under test, reported as green.

Failure scenario

Break any SQL file under internal/database/postgres/migrations/ - a syntax error, a duplicate version number, a malformed DO block - and the entire Postgres integration suite reports skipped, i.e. green, instead of red. Nothing distinguishes "no Docker on this runner" from "the schema is broken".

This is the same failure class the repo has already been bitten by twice: duplicate migration numbers that only surfaced in CI on the merge ref, and the #1422 HEAD-pinned assertion that broke the next migration PR rather than its own. In both cases the cost came from the defect being invisible at the point it was introduced. This skip is the mechanism that keeps it invisible: a migration defect can reach main with a fully green integration suite.

It also compounds with the per-module CI gap (#1478) and with the fact that pre-commit's go-test hook is pre-push-stage only and never runs in CI. There are few places left where a broken migration would be caught before deploy, and this converts one of them into a pass.

Fix direction

Split the two conditions in the shared setup:

• Container start failure -> keep t.Skipf. It is an environment fact.
• RunMigrations error -> t.Fatalf. Migrating is the thing under test; a failure to migrate is a test failure.

Verify the change the way the repo's own convention requires: introduce a deliberately broken migration locally, confirm the suite goes red (it currently goes green), then revert. A change that leaves the suite green under a broken migration has not fixed this.

Related

• fix(test): pin 000086 migration test to version 86, not HEAD #1436 / fix(auth): enforce four-eyes principle on purchase approve #1422 - the migration-test-pinning lesson; same "migration defect surfaces in the wrong place" family.
• internal/config: UpsertRecommendations integration tests fail FK without seeding cloud_accounts #1294 (internal/config: UpsertRecommendations integration tests fail FK without seeding cloud_accounts) - same suite; worth confirming that failure is not currently being absorbed by a skip.
• ci: go vet/lint steps don't use the per-module loop convention [cli part] #1478 - the per-module CI gap that removes the other places a schema break might be caught.