Gate the main test suite on test-build; add build-results; require ON_ERROR_STOP in test/install - #109
Conversation
…_ERROR_STOP in test/install test-build had no real dependency edge on installcheck, so the main suite (test/install + test/sql) actually ran before test-build's own sanity check in the default config, and raced with it under `make -j`. Add an explicit `installcheck: test-build` edge (guarded to avoid recursing into test-build's own nested installcheck invocation) so a broken build blocks the main suite entirely -- its results would be meaningless otherwise. Add `make build-results` to refresh test/build/expected/*.out from the last test-build run, mirroring `make results` for the main suite. Refuses to bless any file whose actual output contains an ERROR: line. test/install/*.sql files never get a real diff -- their actual output overwrites their expected output in place, so a content difference can never fail the build. Add check-test-install-error-stop, enforcing that every test/install/*.sql file sets ON_ERROR_STOP (directly or via test/pgxntool/psql.sql), since that's the only thing that still turns a hard SQL error into a build failure. test/install/*.out is now gitignored since it's rewritten by every run and never meaningfully compared. Fixes Postgres-Extensions#108. Fixes Postgres-Extensions#97. Related changes in pgxntool-test: - Template's test/install file now sets ON_ERROR_STOP; its .out is no longer committed - Test coverage for the new ordering edge, build-results, and check-test-install-error-stop Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
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: Advanced 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 |
pg_regress hardcodes its psql invocation with no flag passthrough, and test/install shares that single invocation with test/sql -- so even a --launcher hack to inject the flag would break test/sql files that intentionally trigger and recover from an error.
| # state created by install files (tables, extensions, etc.) persists | ||
| # into the main test suite. | ||
| # | ||
| # IMPORTANT (for humans and AI agents reading this file): test/install does |
There was a problem hiding this comment.
ditch comment about human and ai
| # changed column, whatever) will NEVER fail this way, no matter how it | ||
| # changes -- see issue #97. | ||
| # | ||
| # The only thing that still fails the build is psql's own exit code: a |
There was a problem hiding this comment.
s/still fails/forces the build to fail/
| # The only thing that still fails the build is psql's own exit code: a | ||
| # statement that raises a hard error only aborts psql (non-zero exit, which | ||
| # pg_regress does report as a failure) if ON_ERROR_STOP is set. Without it, | ||
| # psql prints the error, keeps going, and exits 0 -- the file "passes" |
There was a problem hiding this comment.
Meh, assume reader understands psql will exit 0 without this
There was a problem hiding this comment.
Fixed in e444b08 — dropped that sentence.
| .PHONY: build-results | ||
| build-results: install | ||
| @$(PGXNTOOL_DIR)/run-test-build.sh $(TESTDIR) | ||
| $(MAKE) -C . _PGXNTOOL_TEST_BUILD_ACTIVE=yes REGRESS="$(TEST_BUILD_REGRESS)" REGRESS_OPTS="--inputdir=$(TESTDIR)/build --outputdir=$(TESTDIR)/build" installcheck |
There was a problem hiding this comment.
Why's this needed? Isn't this essentially what run-test-build.sh does already?
There was a problem hiding this comment.
Good question — added a comment explaining it in e444b08. build-results can't just depend on test-build: test-build's own recipe exits 1 on any regression.diffs, which is exactly the diff build-results exists to accept as the new baseline. So it re-runs run-test-build.sh + the installcheck invocation directly, without that check.
There was a problem hiding this comment.
Uh, so what if it flags the failure? As long as we know that's why it errored we can ignore it; it already created the results/* files that we're expecting.
There was a problem hiding this comment.
Simplified build-results in 184e8c1: it now runs -$(MAKE) -C . test-build (the leading - ignores test-build's own exit status) instead of duplicating run-test-build.sh + the installcheck invocation. By the time test-build's regression.diffs check fires, the actual output is already on disk, so there's nothing left to re-run. Also dropped the now-redundant install prerequisite on build-results, since test-build already carries that dependency.
Verified both paths in pgxntool-test's make-test.bats against this branch (33/33 pass): the stale-expected-output refresh (test-build fails internally, build-results still blesses and exits 0) and the ERROR-skip case (build-results still refuses to bless the errored file and exits 1, while still refreshing the clean one).
| STABLE | ||
| ------ | ||
| == `test-build` now gates the main test suite | ||
| Previously, `test-build` had no real dependency edge on `installcheck` -- |
There was a problem hiding this comment.
Meh, not worth mention here...
There was a problem hiding this comment.
Fixed in e444b08 — trimmed the check-stale-expected/race detail out of the HISTORY.asc entry, kept just the behavior change.
Cut internal pg_regress plumbing detail from the ON_ERROR_STOP comment block down to the actual design reason (forcing it globally would also apply to test/sql, breaking deliberate mid-file error tests) and drop the address-the-reader aside. Trim a similar over-explanation from the HISTORY.asc test-build entry. Note why build-results can't just depend on test-build (it aborts on the very diff build-results exists to accept).
…TOP (#79) Related pgxntool PR: Postgres-Extensions/pgxntool#109 Template's `test/install` file now sets `ON_ERROR_STOP` and stops committing its self-overwriting `.out` file, per pgxntool's new `check-test-install-error-stop` check. Test coverage added: - `installcheck:test-build` ordering edge, via `make -n` dry-run (issue #108) - `make build-results`: refreshes clean files, skips and reports files whose actual output contains `ERROR:`, and still exits non-zero on a partial skip - `check-test-install-error-stop`: script-level decision logic (direct `ON_ERROR_STOP`, via `psql.sql`, missing) and Makefile wiring (recipe args, disable via `PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK`, exit-status propagation) Covers pgxntool issues #108 and #97 (fixed by the paired pgxntool PR above, not by this one). Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
| @@ -1,3 +1,31 @@ | |||
| STABLE | |||
| ------ | |||
| == `test-build` now gates the main test suite | |||
There was a problem hiding this comment.
Minor fix, not worth mentioning
There was a problem hiding this comment.
Not sure which specific wording/detail this is flagging — I compared this header against the file's own conventions (underline length, backtick usage, blank-line spacing between entries) and against the PR body's parallel section and didn't find a concrete defect to fix. Could you point at the specific text? Flagging back to the coordinator in the meantime rather than guessing at an edit.
| .PHONY: build-results | ||
| build-results: install | ||
| @$(PGXNTOOL_DIR)/run-test-build.sh $(TESTDIR) | ||
| $(MAKE) -C . _PGXNTOOL_TEST_BUILD_ACTIVE=yes REGRESS="$(TEST_BUILD_REGRESS)" REGRESS_OPTS="--inputdir=$(TESTDIR)/build --outputdir=$(TESTDIR)/build" installcheck |
There was a problem hiding this comment.
Uh, so what if it flags the failure? As long as we know that's why it errored we can ignore it; it already created the results/* files that we're expecting.
| $(MAKE) -C . _PGXNTOOL_TEST_BUILD_ACTIVE=yes REGRESS="$(TEST_BUILD_REGRESS)" REGRESS_OPTS="--inputdir=$(TESTDIR)/build --outputdir=$(TESTDIR)/build" installcheck | ||
| @mkdir -p $(TESTDIR)/build/expected | ||
| @skipped=0; \ | ||
| for f in $(TESTDIR)/build/results/*.out; do \ |
There was a problem hiding this comment.
This will leave behind files that no longer exist, no? Add a test where we rename the extension .sql file to make sure we handle that correctly.
There was a problem hiding this comment.
Checked this: it doesn't leave orphans unnoticed. test/bin/check-stale-expected.sh already covers test/build/expected the same way it covers the main suite's test/expected (check_pair "$testdir/build" "$testdir/build/expected"), and check-stale-expected runs as part of installcheck's dependency chain, which this PR now makes depend on test-build. So a renamed/removed test/build/*.sql file's orphaned .out gets flagged on the next make test/installcheck — same as make results already leaves (and check-stale-expected already catches) for the main suite's own test/expected. build-results itself isn't meant to be the thing that catches this, any more than make results is.
The rename-based regression test you're asking for is a good idea, but it belongs in pgxntool-test (this repo has no test infra), and it's really a check-stale-expected test, not a build-results one — flagging back to the coordinator to open that separately rather than pulling a cross-repo PR into this one.
test-build already writes the actual output build-results needs before its own regression.diffs check exits 1 -- ignoring that exit status (leading `-`) avoids re-running run-test-build.sh + installcheck a second time.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| # a diff", which is exactly what build-results exists to bless. | ||
| .PHONY: build-results | ||
| build-results: | ||
| -$(MAKE) -C . test-build | ||
| @mkdir -p $(TESTDIR)/build/expected | ||
| @skipped=0; \ | ||
| for f in $(TESTDIR)/build/results/*.out; do \ | ||
| [ -f "$$f" ] || continue; \ | ||
| if grep -q 'ERROR:' "$$f"; then \ | ||
| echo "build-results: skipping $$f (actual output contains ERROR:)" >&2; \ | ||
| echo " If this is intentional, bless it by hand:" >&2; \ | ||
| echo " cp $$f $(TESTDIR)/build/expected/$$(basename "$$f")" >&2; \ | ||
| skipped=1; continue; \ | ||
| fi; \ | ||
| cp "$$f" $(TESTDIR)/build/expected/$$(basename "$$f"); \ | ||
| done; \ | ||
| [ "$$skipped" = 0 ] || exit 1 | ||
| endif |
There was a problem hiding this comment.
build-results can silently bless a stale/previous baseline instead of failing.
The leading - on -$(MAKE) -C . test-build (base.mk#L619-L621) ignores every failure mode of the sub-make, not just "there was a diff to bless" (the only case the surrounding comment justifies). If install fails to compile, run-test-build.sh errors out, or pg_regress never runs, no new test/build/results/*.out is written this run — but the copy loop still runs and copies whatever .out files a previous run left on disk into test/build/expected/, then exits 0 (since skipped only tracks the ERROR: content scan, not whether test-build actually produced fresh output). That directly contradicts this target's own documented contract in README.asc ("...exits non-zero so the skip can't go unnoticed"): here the skip does go unnoticed.
Suggested direction: distinguish "test-build failed only because of a diff to bless" from every other failure mode — e.g. check for test/build/regression.diffs (which pg_regress recreates each run and is what test-build's own recipe already checks for) before treating the sub-make's non-zero exit as benign, rather than blanket-ignoring it with -.
| for f in "$install_dir"/*.sql; do | ||
| [ -f "$f" ] || continue | ||
|
|
||
| if grep -q 'ON_ERROR_STOP' "$f"; then | ||
| continue | ||
| fi | ||
| if grep -qE '\\ir? +.*psql\.sql' "$f"; then | ||
| continue | ||
| fi | ||
|
|
||
| missing+=("$f") | ||
| done |
There was a problem hiding this comment.
The ON_ERROR_STOP detection here is too loose to reliably provide the protection this script exists for.
- Line 36,
grep -q 'ON_ERROR_STOP', is a bare substring match anywhere in the file — no anchor, no check of the\setvalue, no exclusion of\unset. A file containing\set ON_ERROR_STOP off,\unset ON_ERROR_STOP, or even just a comment like-- ON_ERROR_STOP not needed herepasses this check while providing none of the protection it's meant to guarantee. Given the PR's own framing — ON_ERROR_STOP is "the only thing that still turns a hard SQL error into a build failure" fortest/install— a check that a comment satisfies converts "unprotected" into "reported protected." - Line 39,
grep -qE '\\ir? +.*psql\.sql', has the same comment-blindness (-- see \i test/pgxntool/psql.sqlmatches), and.*psql\.sqlmatches any path merely ending inpsql.sql(e.g. a project's own unrelatedmypsql.sql). In the other direction, the literal-space separator misses a tab-separated\i<TAB>..., and\\ir? +doesn't match psql's long forms\include/\include_relative— nor does it match indirect inclusion (e.g.\i test/pgxntool/tap_setup.sql, which itself sourcespsql.sqland is explicitly suggested elsewhere in this PR's new README text as a way to add pgTap assertions to an install file).
Both directions are real: files that silently disable/never had the protection can pass, and some genuinely-protected files can be rejected. Worth tightening line 36 to require an actual enabling \set (rejecting off/0/\unset) and anchoring/making both greps comment-aware, since a silent false-pass here is worse than a false-fail (which at least fails loudly and has PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK=no as an escape hatch).
|
|
||
| NOTE: This also means a stale or simply wrong `test/build/expected/*.out` -- not just a genuinely broken build -- blocks the whole suite. See <<_build_results,build-results>> below for the supported way to refresh it, including how to handle a file that's *supposed* to show an error. | ||
|
|
There was a problem hiding this comment.
This new NOTE (and the similar WARNING added at README.asc#L195-L197) mixes literal *.out globs with asciidoc *emphasis* markup in the same sentence ("...test/build/expected/*.out... a file that's supposed to show an error"). Asciidoctor parses the * in the glob as an emphasis opener, and the committed README.html in this PR shows the result is genuinely garbled — not just a theoretical parsing quirk:
This also means a stale or simply wrong <code>test/build/expected/<strong>.out</code>not just a genuinely broken buildblocks the whole suite. See <a href="#_build_results">build-results</a> below for the supported way to refresh it, including how to handle a file thats *supposed</strong> to show an error.The glob's asterisk is lost (renders as test/build/expected/.out), and the bold run-on continues bolding the rest of the sentence. The line 196 WARNING has the same problem plus a stray literal backtick leaking into the output before ON_ERROR_STOP, and ~45 words of prose incorrectly wrapped inside a <code> span.
Fix: use passthrough/literal monospace so no substitution runs inside the glob, e.g. `+test/build/expected/*.out+` and `+test/install/*.out+`, then regenerate README.html.
| # a diff", which is exactly what build-results exists to bless. | ||
| .PHONY: build-results | ||
| build-results: | ||
| -$(MAKE) -C . test-build | ||
| @mkdir -p $(TESTDIR)/build/expected | ||
| @skipped=0; \ | ||
| for f in $(TESTDIR)/build/results/*.out; do \ | ||
| [ -f "$$f" ] || continue; \ | ||
| if grep -q 'ERROR:' "$$f"; then \ | ||
| echo "build-results: skipping $$f (actual output contains ERROR:)" >&2; \ | ||
| echo " If this is intentional, bless it by hand:" >&2; \ | ||
| echo " cp $$f $(TESTDIR)/build/expected/$$(basename "$$f")" >&2; \ | ||
| skipped=1; continue; \ | ||
| fi; \ | ||
| cp "$$f" $(TESTDIR)/build/expected/$$(basename "$$f"); \ | ||
| done; \ | ||
| [ "$$skipped" = 0 ] || exit 1 | ||
| endif |
There was a problem hiding this comment.
build-results can silently bless a stale/previous baseline instead of failing.
The leading - on -$(MAKE) -C . test-build (base.mk#L620) ignores every failure mode of the sub-make, not just "there was a diff to bless" (the only case the surrounding comment justifies). If install fails to compile, run-test-build.sh errors out, or pg_regress never runs, no new test/build/results/*.out is written this run — but the copy loop still runs and copies whatever .out files a previous run left on disk into test/build/expected/, then exits 0 (since skipped only tracks the ERROR: content scan, not whether test-build actually produced fresh output). That directly contradicts this target's own documented contract in README.asc ("...exits non-zero so the skip can't go unnoticed"): here the skip does go unnoticed.
Suggested direction: distinguish "test-build failed only because of a diff to bless" from every other failure mode — e.g. check for test/build/regression.diffs (which pg_regress recreates each run and is what test-build's own recipe already checks for) before treating the sub-make's non-zero exit as benign, rather than blanket-ignoring it with -.
test-build's new dependency edge on installcheck changes internal build ordering only; it has no user-visible effect worth a changelog entry.
…dering # Conflicts: # HISTORY.asc # README.html
Every failure path called core.setFailed() without ever setting run_tests, so test's if: needs.check-test-pr.outputs.run-tests == 'true' evaluated false and the job showed skipping -- zero test signal on exactly the PRs where a maintainer most needs one to decide whether to override the pairing requirement. Enforcement itself (the setFailed calls) is unchanged; each path now also sets run_tests/pgxntool_test_owner/ pgxntool_test_ref before failing, using the paired PR's own branch when one is known (ambiguous match, label contradiction, poll timeout) or canonical master otherwise. The already-passed and already-failed paired cases are untouched: their own CI result is the real signal already. test's with: block now consumes these outputs instead of a hardcoded Postgres-Extensions/master, so the reusable workflow actually runs against whichever ref check-test-pr decided on.
GitHub Actions implicitly ANDs a needs-based if: with success() unless the condition itself calls a status function, so test stayed skipped even after check-test-pr started setting run_tests=true on its failure paths -- confirmed live on this branch's own CI run. !cancelled() overrides that without also running on an actual workflow cancellation, which bare always() would.
1 participant
A broken extension build can no longer slip through
make test:test-buildnow genuinely gatestest/install/test/sql(fixing a race undermake -j), andtest/install/*.sqlfiles that omitON_ERROR_STOP— the only thing standing between a hard SQL error and a silent "pass" there — now fail the build instead of being silently swallowed.test-build now gates the main suite (Fixes #108)
test-buildhad no real dependency edge oninstallcheck, so in the default (serial) config the main suite happened to run after it only becausecheck-stale-expected's own edge pulled it in first — undermake -jthe two raced. Added an explicitinstallcheck: test-buildedge (guarded against recursing into test-build's own nestedinstallcheckinvocation) so a broken build blocks the main suite entirely; there's no point validating install/query behavior against a build that doesn't come up cleanly.Tradeoff: this also means a stale or wrong
test/build/expected/*.outblocks the whole suite, not just a genuinely broken build — hencebuild-resultsbelow.make build-results(supports the above)Refreshes
test/build/expected/*.outfrom the lasttest-buildrun, mirroringmake resultsfor the main suite. Refuses to bless any file whose actual output contains anERROR:line — accepting an errored build as the new baseline would defeat the point of test-build — and reports exactly which file to bless by hand if the error is intentional.test/installnow requiresON_ERROR_STOP(Fixes #97)test/install/*.sqlfiles never get a real diff: pg_regress writes their actual output to the exact same file as their expected output, so a content difference can never fail the build.ON_ERROR_STOP(set directly, or via\i test/pgxntool/psql.sql) is the only thing that still turns a hard SQL error into a build failure. Addedcheck-test-install-error-stop, enforcing that everytest/install/*.sqlfile sets it (PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECKto disable).test/install/*.outis now gitignored, since it's rewritten by every run and never meaningfully compared.All of the above is documented in
base.mk,README.asc, andCLAUDE.mdfor both humans and AI agents reading the code directly, since none of it is obvious from the code alone — including whyON_ERROR_STOPcan't just be forced onto pg_regress's psql invocation instead of checking each file (pg_regress's command line is hardcoded, and forcing it would also changetest/sqlsemantics).Fixes #108.
Fixes #97.
Related pgxntool-test PR (merged): Postgres-Extensions/pgxntool-test#79