Repository navigation
ci: assert every Postgres-gated package executed tests (APA-83 P10) - #157
Conversation
`make integration` runs only ./internal/repository/postgres/... and ./internal/adapters/..., and the anti-fake-green step counted `--- PASS` for internal/repository/postgres alone. Eight further packages gate on TEST_POSTGRES_DSN and self-skip when it is absent. Measured on main@93219b1: with Postgres down those 10 packages produce 60 skips and internal/repository/postgres reports 0 passes / 20 skips — while the other nine still report 532 passing tests in aggregate. An aggregate threshold would therefore pass with Postgres completely absent. Replace the single-package count with check_pg_gated_packages.py, which runs each required package verbosely and requires at least one passing test per package. Per-package is the point: a busy package cannot mask a silent one. The gate fails closed on a missing TEST_POSTGRES_DSN before running anything, so an unset variable cannot masquerade as coverage. Scope (APA-83 P10 only): - Mockoon intentionally excluded; its placeholder startup and informational-only probe are tracked separately as P12. - The `pull_request` trigger is intentionally unchanged; PR-execution and required-check design are a separate proposal. - Branch protection untouched; no existing check weakened. Evidence: - positive: with Postgres up, all 10 packages execute (postgres 20 pass / 0 skip); gate exits 0. - negative 1: TEST_POSTGRES_DSN unset -> exit 1 before running tests. - negative 2: DSN pointing at an unreachable port -> exit 1, reporting internal/repository/postgres pass=0 skip=20 while the other nine pass. - negative 3 (unit): a package with zero passing tests is under-run; a busy package cannot mask a silent one; an absent package entry is under-run. - enumeration guard: the unit test re-discovers every package referencing TEST_POSTGRES_DSN and fails if one is absent from the gate. Verified non-vacuous by adding a gated test to an unlisted package (fails) and removing it (passes). Residual skips with Postgres up are gated on external services, not the database: localgcp (app), GCW emulator (worker, workflow), live matrix (tools), and one APA-59 trajectory (orchestrate). No live-provider test bodies executed. Release remains NOT QUALIFIED.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review defect: run_package discarded subprocess returncode, so evaluate logic only asked "did this package produce at least one --- PASS line?". Reproduced with a real failing test in internal/ingest against a live Postgres: `go test` exited 1, but the gate reported ok ./internal/ingest pass=16 skip=0 PASS: all 10 Postgres-gated packages executed tests. EXIT: 0 A package with passing tests and a real failure was classified healthy, letting Postgres-backed failures through a green gate. Fix: retain returncode and treat it as an independent failure condition. - PackageResult carries returncode, passed, and skipped. - evaluate() reports two distinct problems per package: a nonzero `go test` exit (failure, build error, panic) and a package that ran no tests. Both block. - Reporting keeps pass/skip counts alongside exit= so an infra failure stays diagnosable. Same real failing test after the fix: ::error::./internal/ingest: go test exited 1 (pass=16 skip=0) FAIL: 1 Postgres-gated package(s) did not run clean. EXIT: 1 Regressions added: nonzero exit with passing tests blocks; build error is reported; all-skipped package still reported; missing package entry reported; healthy run is clean. Full suite 54 passed. No live-provider test bodies executed. Release remains NOT QUALIFIED.
Aparnap2
left a comment
There was a problem hiding this comment.
Re-review of exact head 7601e67: the prior blocking defect is corrected. PackageResult retains the subprocess return code; evaluate() independently rejects nonzero go-test exits even when passing tests exist; and the regression test exercises that mixed pass/nonzero-exit case. Build-error, all-skipped, missing-package, healthy-run, DSN fail-closed, and package-enumeration coverage are present. Scope remains P10 only: Mockoon/P12, deferred pull_request-trigger proposal B, and branch protection are unchanged. Author reports local live-Postgres reproduction and 54 tests passing. Note: GitHub does not permit approving one's own PR, so this is a review comment rather than a formal approval. Release qualification remains NOT QUALIFIED.
Why
make integrationruns only./internal/repository/postgres/... ./internal/adapters/..., and the anti-fake-green step counted--- PASSforinternal/repository/postgresalone. Eight further packages gate onTEST_POSTGRES_DSNand self-skip when it is absent.The measured gap
With Postgres absent on
main@93219b1:internal/repository/postgresThat 532 is the whole argument. An aggregate
>= 1— or even>= 100— threshold passes with Postgres completely down. The previous single-package count was the only thing preventing that.With Postgres up, the same set runs:
internal/repository/postgres20 pass / 0 skip, and the aggregate rises to 579.Fix
scripts/ci/check_pg_gated_packages.pyruns each required package verbosely and requires at least one passing test per package. Per-package is the point — a busy package cannot mask a silent one.It also fails closed on an unset
TEST_POSTGRES_DSNbefore running anything, so an unset variable cannot masquerade as coverage.Evidence
Positive (Postgres up):
Negative 1 — DSN unset: exits 1 before running any test.
Negative 2 — DSN set but Postgres unreachable: exits 1, and the report is the proof:
Nine packages still read "ok" while the service is down. Only the per-package rule catches it.
Negative 3 — unit-level rule tests: a zero-pass package is under-run; a busy package cannot mask a silent one; an absent package entry is under-run.
Enumeration guard (non-vacuous): the unit test re-discovers every package referencing
TEST_POSTGRES_DSNand fails if one is missing from the gate. Verified by adding a gated test to an unlisted package —AssertionError: Postgres-gated packages absent from the gate: ['./internal/claims']— then removing it (passes).Residual skips are not Postgres-related
With Postgres up, the remaining skips are gated on external services: localgcp (
app), GCW emulator (worker,workflow), live matrix (tools), one APA-59 trajectory (orchestrate). PG coverage itself is fully exercised.Scope — P10 only
Per the approved scope decision:
pull_requesttrigger unchanged — PR-execution and required-check design are a separate proposal (deferred B).Other gates
pytest tests/unit52 passed (8 new) ·ruff check .clean · mypy strict clean, now 10 source files · offlinego test ./...exit 0 ·go test -raceoninternal/claimsclean ·gofmtclean · YAML valid.No live-provider test bodies executed. Release remains NOT QUALIFIED.
Refs APA-83 (P10). Next isolated slice: P12 Mockoon.