From 7830a70f3fc65887f4a4059e2090cdd5e90d3097 Mon Sep 17 00:00:00 2001 From: Bobbie Soedirgo Date: Tue, 4 Aug 2026 14:34:50 +0800 Subject: [PATCH] ci: fail the build when the test suite fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .pre-commit-config.yaml | 7 ++ bin/check_lints.py | 137 +++++++++++++++++++++++++++++++++ dockerfiles/docker-compose.yml | 11 ++- 3 files changed, 154 insertions(+), 1 deletion(-) create mode 100644 bin/check_lints.py diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 840e776..6186cb4 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -29,3 +29,10 @@ repos: language: python always_run: true pass_filenames: false + + - id: check-lints + name: Check lints are fully registered + entry: python bin/check_lints.py + language: python + always_run: true + pass_filenames: false diff --git a/bin/check_lints.py b/bin/check_lints.py new file mode 100644 index 0000000..b563206 --- /dev/null +++ b/bin/check_lints.py @@ -0,0 +1,137 @@ +"""Check that every lint is fully registered. + +Adding a lint means touching more than `lints/`: the view has to be loaded by the +test harness, unioned in the compatibility test, documented, and linked in the +docs nav. Each of those is easy to forget, and forgetting most of them fails +quietly -- the suite still goes green, the docs page just never appears. + +This script fails loudly instead. For every `lints/NNNN_.sql` it asserts: + + 1. the view is named `lint."NNNN_"` (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 that doc page + +It also checks that no two lints share a number, and that every doc page is in +the nav. Run via pre-commit, or directly: + + python bin/check_lints.py +""" + +import re +import sys +from pathlib import Path + +LINTS_DIR = Path("lints") +DOCS_DIR = Path("docs") +TEST_SQL_DIR = Path("test/sql") +TEST_EXPECTED_DIR = Path("test/expected") +INSTALLCHECK = Path("bin/installcheck") +UNIONABLE = TEST_SQL_DIR / "queries_are_unionable.sql" +MKDOCS = Path("mkdocs.yaml") + +NO_REGRESS_TEST = { + # bloat stats are not stable under pg_regress + "0020_table_bloat" +} + +STEM_RE = re.compile(r"^(\d{4})_[a-z0-9_]+$") + + +def find_by_number(directory: Path, number: str, suffix: str) -> list[Path]: + return sorted(directory.glob(f"{number}_*{suffix}")) + + +def check() -> list[str]: + errors: list[str] = [] + + installcheck = INSTALLCHECK.read_text() + unionable = UNIONABLE.read_text() + mkdocs = MKDOCS.read_text() + + seen_numbers = {} + + for lint_path in sorted(LINTS_DIR.glob("*.sql")): + stem = lint_path.stem + match = STEM_RE.match(stem) + if not match: + errors.append( + f"{lint_path}: name must be NNNN_snake_case (four digits, " + f"underscore, lowercase)" + ) + continue + number = match.group(1) + + if number in seen_numbers: + errors.append( + f"{lint_path}: lint number {number} is already used by " + f"{seen_numbers[number]}; renumber one of them" + ) + seen_numbers[number] = lint_path + + # 1. view name matches the file stem + if f'create view lint."{stem}"' not in lint_path.read_text().lower(): + errors.append( + f'{lint_path}: must declare `create view lint."{stem}"` ' + f"(view name has to match the file name)" + ) + + # 2. loaded by the test harness + if f"-f lints/{number}*.sql" not in installcheck: + errors.append( + f"{lint_path}: not loaded by {INSTALLCHECK}; add " + f"`-f lints/{number}*.sql` before `-d contrib_regression`" + ) + + # 3. unioned in the column-compatibility test + if f'lint."{stem}"' not in unionable: + errors.append( + f"{lint_path}: not covered by {UNIONABLE}; add " + f'`union all select * from lint."{stem}"`' + ) + + # 4. documented + docs = find_by_number(DOCS_DIR, number, ".md") + if not docs: + errors.append(f"{lint_path}: missing docs page {DOCS_DIR}/{number}_*.md") + + # 5. tested + if stem in NO_REGRESS_TEST: + continue + if not find_by_number(TEST_SQL_DIR, number, ".sql"): + errors.append(f"{lint_path}: missing test {TEST_SQL_DIR}/{number}_*.sql") + if not find_by_number(TEST_EXPECTED_DIR, number, ".out"): + errors.append( + f"{lint_path}: missing expected output " + f"{TEST_EXPECTED_DIR}/{number}_*.out" + ) + + # Step 6: every doc page is reachable from the nav. Covers docs for lints + # that are not SQL views too (e.g. 0012, an auth config check). + for doc in sorted(DOCS_DIR.glob("[0-9]*.md")): + if doc.name not in mkdocs: + errors.append( + f"{doc}: not listed in {MKDOCS}; add it under `nav:` -> `Lints:`" + ) + + return sorted(set(errors)) + + +def main() -> None: + errors = check() + if errors: + print(f"{len(errors)} problem(s) found:\n", file=sys.stderr) + for error in errors: + print(f" - {error}", file=sys.stderr) + print( + "\nSee .claude/skills/new-lint/SKILL.md for the full checklist.", + file=sys.stderr, + ) + raise SystemExit(1) + print(f"all {len(list(LINTS_DIR.glob('*.sql')))} lints are fully registered") + + +if __name__ == "__main__": + main() diff --git a/dockerfiles/docker-compose.yml b/dockerfiles/docker-compose.yml index 4f62b91..99b2535 100644 --- a/dockerfiles/docker-compose.yml +++ b/dockerfiles/docker-compose.yml @@ -18,4 +18,13 @@ services: command: - bash - -c - - "./bin/installcheck; cp /home/splinter/regression.diffs /home/splinter/regression.out /home/splinter/results/* /home/splinter/results_out/ 2>/dev/null || true" + # Copy the artifacts out whether or not the suite passed, then exit with + # installcheck's status. + - | + ./bin/installcheck + status=$$? + cp /home/splinter/regression.diffs \ + /home/splinter/regression.out \ + /home/splinter/results/* \ + /home/splinter/results_out/ 2>/dev/null || true + exit $$status