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:51internal/config/store_postgres_db_test.go:50internal/config/store_postgres_ladder_test.go:48internal/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
Reviewed commit:
be11bdcb5. Note:origin/mainmoved to3e9660d06during the review; re-verify against currentmainbefore changing code, since a finding may have been fixed or moved.Where
internal/auth/store_postgres_db_test.go:51internal/config/store_postgres_db_test.go:50internal/config/store_postgres_ladder_test.go:48internal/config/store_postgres_unassigned_test.go:49t.Skipf("... could not setup postgres container: %v", err)sites ininternal/analytics/postgres_analytics_db_test.goandinternal/secrets/*_resolver_coverage_test.goWhat
The shared setup does:
t.Skipfon 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 malformedDOblock - 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
mainwith a fully green integration suite.It also compounds with the per-module CI gap (#1478) and with the fact that
pre-commit'sgo-testhook ispre-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:
t.Skipf. It is an environment fact.RunMigrationserror ->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
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.