Skip to content

ci: fail the build when the test suite fails - #180

Merged
soedirgo merged 1 commit into
mainfrom
fix/ci-exit-code
Aug 5, 2026
Merged

ci: fail the build when the test suite fails#180
soedirgo merged 1 commit into
mainfrom
fix/ci-exit-code

Conversation

@soedirgo

@soedirgo soedirgo commented Aug 4, 2026

Copy link
Copy Markdown
Member

What kind of change does this PR introduce?

CI fix.

What is the current behavior?

The tests check is green even when the suite fails.

The test container runs:

bash -c "./bin/installcheck; cp ... /home/splinter/results_out/ 2>/dev/null || true"

installcheck's exit code is discarded by the ;, and the trailing || true guarantees the container exits 0. Every run reports success.

In #179, the new lint was never loaded by bin/installcheck, so its regress test errored:

test 0030_invalid_index ... ERROR:  relation "lint.0030_invalid_index" does not exist
FAILED
 1 of 29 tests failed.

What is the new behavior?

1. The container exits with installcheck's status. Artifacts are still copied out on failure, so results/regression.diffs remains available for debugging.

2. New bin/check_lints.py, wired in as a pre-commit hook (so it gates every PR via the existing pre-commit hooks workflow). Registering a lint touches five files outside lints/, and forgetting most of them fails quietly. For each lints/NNNN_<name>.sql it asserts:

  1. the view is named lint."NNNN_<name>" (matches the file stem)
  2. bin/installcheck loads it
  3. test/sql/queries_are_unionable.sql unions it
  4. docs/NNNN_*.md exists
  5. test/sql/NNNN_*.sql and test/expected/NNNN_*.out exist
  6. mkdocs.yaml links the doc page

Plus: no two lints share a number, and every numbered doc page is in the nav.

Run against #179's first commit, it reports exactly what was missing:

3 problem(s) found:

  - docs/0030_invalid_index.md: not listed in mkdocs.yaml; add it under `nav:` -> `Lints:`
  - lints/0030_invalid_index.sql: not covered by test/sql/queries_are_unionable.sql; add `union all select * from lint."0030_invalid_index"`
  - lints/0030_invalid_index.sql: not loaded by bin/installcheck; add `-f lints/0030*.sql` before `-d contrib_regression`

The test container ran `./bin/installcheck; cp ... || true`, so the trailing
copy decided the container's exit code and every run reported success. A
failing suite still produced a green check — see #179, where the new lint's
regress test errored with `relation "lint.0030_invalid_index" does not exist`
while the `tests` check stayed green.

Copy the artifacts out as before, then exit with installcheck's status.

Also add bin/check_lints.py, which catches the class of omission behind that
failure: a lint that is not loaded by bin/installcheck, not unioned in
queries_are_unionable.sql, undocumented, untested, or missing from the mkdocs
nav. It runs as a pre-commit hook, so it gates every PR.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@soedirgo
soedirgo marked this pull request as ready for review August 4, 2026 07:08
@soedirgo
soedirgo requested a review from a team August 4, 2026 07:16
@soedirgo
soedirgo merged commit af0013d into main Aug 5, 2026
3 checks passed
@soedirgo
soedirgo deleted the fix/ci-exit-code branch August 5, 2026 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants