Update docs: make test now returns non-zero on regressions (pgxntool 2.3.0) - #60
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
…2.3.0) CLAUDE.md, RELEASE.md, and a ci.yml comment all claimed 'make test' never exits non-zero on failures (pgxntool marked installcheck .IGNORE). pgxntool 2.3.0 fixed this: test's own recipe now checks regression.diffs and exits non-zero when it's non-empty. verify-results remains the stricter, documented gate (it also confirms test actually ran), but the old blanket claim about make test is no longer accurate. Verified locally: make test exits 0 when clean and non-zero (with the diff printed) when a test is made to fail.
370bdad to
1dc632f
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| pgxntool's; pgxntool only wraps/seeds them. Don't assume they belong to pgxntool. | ||
| - `make test` does **not** return non-zero on test regressions — pgxntool marks | ||
| `installcheck` as `.IGNORE`. To actually detect failures, use `make verify-results` | ||
| (or inspect `test/regression.diffs`). This is why CI must gate on `verify-results`. | ||
| - `make test` returns non-zero on test regressions (pgxntool 2.3.0+): `installcheck` | ||
| itself is still marked `.IGNORE`, but `test`'s own recipe now checks | ||
| `test/regression.diffs` and exits non-zero if it's non-empty. `make verify-results` | ||
| remains the stricter, documented CI gate (it also confirms `test` actually ran), but | ||
| a bare `make test` failing locally is now a real signal too. | ||
| - `test/install/*.sql` runs once, committed, before the suite in the same `pg_regress` |
There was a problem hiding this comment.
The new claim that make verify-results "also confirms test actually ran" doesn't match pgxntool 2.3.0's actual Makefile: verify-results depends on $(TEST_DEPS), not on test (pgxntool/base.mk:417). base.mk's own comment (lines 404-409) explains this was deliberate — test's recipe now exits non-zero as soon as it sees a regression, which would abort the chain before verify-results got to run if it depended on test. So make verify-results never executes the test recipe; it only guarantees $(TEST_DEPS) completed (which includes installcheck, running the suite, but not the test target itself).
The same "confirms test actually ran" claim was added in two other places by this PR and has the same problem:
cat_tools/.github/workflows/ci.yml
Lines 314 to 317 in 1dc632f
Lines 42 to 44 in 1dc632f
Suggest rephrasing along the lines of: "verify-results does not depend on test; it depends on $(TEST_DEPS) directly (by design — see base.mk's comment on why) so test's early exit can't abort the check, and it applies a stricter pgtap-aware check on top."
1 participant
Stacked on #58 (pgxntool 2.3.0 sync).
CLAUDE.md, RELEASE.md, and a ci.yml comment all claimed `make test` never exits non-zero on failures (because pgxntool marked `installcheck` `.IGNORE`). pgxntool 2.3.0 fixes this: `test`'s own recipe now checks `test/regression.diffs` and exits non-zero when it's non-empty (issues #35/#49). `verify-results` remains the stricter, documented gate (it also confirms `test` actually ran), but the old blanket claim about `make test` never gating is no longer accurate.
Verified locally: `make test` exits 0 when clean, and exits non-zero (with the diff printed) when a test is deliberately made to fail.