From ab48adb4fac0072d1bbd9415c4fc3e58730a103e Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Wed, 16 Sep 2026 16:25:52 -0500 Subject: [PATCH 1/2] Make check-test-install-error-stop.sh order-sensitive for ON_ERROR_STOP The bare substring grep for ON_ERROR_STOP passed any file mentioning the variable at all, including one that only ever turns it off. Read \set ON_ERROR_STOP statements (and psql.sql inclusion) in file order and require an explicit on-value to have been seen; a later explicit off no longer masks the absence of an on. Addresses maintainer review feedback on #109: https://github.com/Postgres-Extensions/pgxntool/pull/109#discussion_r4030756377 Co-Authored-By: Claude Sonnet 5 --- test/bin/check-test-install-error-stop.sh | 34 +++++++++++++++++++---- 1 file changed, 28 insertions(+), 6 deletions(-) diff --git a/test/bin/check-test-install-error-stop.sh b/test/bin/check-test-install-error-stop.sh index 26c98e5..61f2700 100755 --- a/test/bin/check-test-install-error-stop.sh +++ b/test/bin/check-test-install-error-stop.sh @@ -12,8 +12,13 @@ # net so its absence is caught at build time instead of discovered the hard # way (issue #97). # -# A file passes if it either sets ON_ERROR_STOP itself, or sources -# test/pgxntool/psql.sql (which already sets it, among other things). +# A file passes if it explicitly turns ON_ERROR_STOP on at some point -- +# directly (`\set ON_ERROR_STOP on`/true/1/yes) or by sourcing +# test/pgxntool/psql.sql (which already turns it on, among other things). A +# later explicit turn-off doesn't undo an earlier turn-on. A file that only +# ever turns it off, or never mentions it at all, fails: a bare substring +# match on "ON_ERROR_STOP" would wrongly pass a file that only turns it off, +# so each `\set` needs its value read, in file order. # # Usage: check-test-install-error-stop.sh @@ -30,13 +35,30 @@ testdir="$1" install_dir="$testdir/install" missing=() +# Scans $1 in file order, tracking whether ON_ERROR_STOP has been explicitly +# turned on. Succeeds (exit 0) once an on-value is seen; a later off-value +# doesn't reset that. Sourcing psql.sql counts as turning it on, wherever it +# occurs in the file. +file_turns_error_stop_on() { + local f="$1" line value + while IFS= read -r line; do + if [[ "$line" =~ \\ir?[[:space:]]+.*psql\.sql ]]; then + return 0 + fi + if [[ "$line" =~ \\set[[:space:]]+ON_ERROR_STOP[[:space:]]+([^[:space:]]+) ]]; then + value="${BASH_REMATCH[1],,}" + case "$value" in + on|true|1|yes) return 0 ;; + esac + fi + done < "$f" + return 1 +} + 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 + if file_turns_error_stop_on "$f"; then continue fi From ff766b3e501641587bf9600dd5cd404338c2ddf0 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Sun, 4 Oct 2026 14:54:30 -0500 Subject: [PATCH 2/2] Only let an explicit ON_ERROR_STOP excuse a later turn-off A test/install file now passes check-test-install-error-stop.sh iff it either explicitly enables ON_ERROR_STOP itself (`\set ON_ERROR_STOP` on/ true/1/yes) anywhere, or includes test/pgxntool/psql.sql and never disables it (`\set ON_ERROR_STOP` off/false/0/no, or `\unset`). A user who sets ON_ERROR_STOP directly is assumed to know what they're doing if they also turn it off; a user who includes psql.sql may not know it sets it. Each offending file is now reported with its reason. README.asc, HISTORY.asc, base.mk and CLAUDE.md state the rule accordingly. Addresses maintainer review on #122: https://github.com/Postgres-Extensions/pgxntool/pull/122#pullrequestreview-5407836753 Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 2 +- HISTORY.asc | 4 +- README.asc | 4 +- README.html | 6 +-- base.mk | 3 +- test/bin/check-test-install-error-stop.sh | 61 ++++++++++++----------- 6 files changed, 44 insertions(+), 36 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 30bb6d9..4f57a00 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -203,7 +203,7 @@ When tests fail, examine the diff output carefully. The actual test output in `t **Exceptions to the above** -- `test-build` and `test/install` (both optional, see `README.asc`) don't follow the `test/results` vs `test/expected` model: - **test-build** runs first, in its own separate `pg_regress` pass over `test/build/*.sql`, and gates the main suite: if it fails, `test/install`/`test/sql` never run at all. It does compare actual vs expected normally (`test/build/results/` vs `test/build/expected/`) -- use `make build-results` to refresh its expected output, not `make results`. -- **test/install** does NOT get a real diff at all: its actual output is written to the exact same file as its expected output, so a content difference can never fail the build, no matter what changed. The only thing that still fails the build is a hard SQL error, and only if the file has `ON_ERROR_STOP` set (directly or via `\i test/pgxntool/psql.sql`) -- pgxntool checks for this by default. If a `test/install/*.sql` file is misbehaving, don't go looking for a diff; check whether it errored, and don't assume a stale-looking `.out` for it means anything. +- **test/install** does NOT get a real diff at all: its actual output is written to the exact same file as its expected output, so a content difference can never fail the build, no matter what changed. The only thing that still fails the build is a hard SQL error, and only if the file has `ON_ERROR_STOP` set (directly, or via `\i test/pgxntool/psql.sql` with no later turn-off) -- pgxntool checks for this by default. If a `test/install/*.sql` file is misbehaving, don't go looking for a diff; check whether it errored, and don't assume a stale-looking `.out` for it means anything. ## Key Implementation Details diff --git a/HISTORY.asc b/HISTORY.asc index b3e1fbb..c998616 100644 --- a/HISTORY.asc +++ b/HISTORY.asc @@ -56,7 +56,9 @@ written to the same file as their expected output, so a content difference can never fail the build. Without `ON_ERROR_STOP`, a hard SQL error was silently swallowed too, making the file "pass" regardless of what happened. `make test` now fails if a `test/install/*.sql` file doesn't set -`ON_ERROR_STOP` (directly, or via `\i test/pgxntool/psql.sql`); see +`ON_ERROR_STOP`: either with an explicit `\set ON_ERROR_STOP on` (after +which turning it off again is allowed), or via `\i test/pgxntool/psql.sql` +with no later turn-off; see `PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK` to disable. Since `test/install/*.out` was never really compared against anything, and is rewritten by every run, it's now gitignored -- stop committing it. diff --git a/README.asc b/README.asc index f1580d1..2321e5d 100644 --- a/README.asc +++ b/README.asc @@ -195,7 +195,7 @@ Without `test/install`, each test file typically needs to run `CREATE EXTENSION` **Key detail:** Install files and regular tests run in a single `pg_regress` invocation. This means the database is NOT dropped between install and test phases — state created by install files persists into the main test suite. -WARNING: **`+test/install/*.out+` is never actually compared against anything.** Unlike every other test type pgxntool supports, the `test/install` directory's actual output is written to the exact same file as its expected output, so there is no diff — a wrong or changed result will never fail the build, no matter how much it changes. The *only* thing that still fails the build is a hard SQL error, and only if the file sets `ON_ERROR_STOP` (directly, or via `\i test/pgxntool/psql.sql`) — without it, psql prints the error, keeps going, and the file "passes" regardless. pgxntool enforces this by default (`check-test-install-error-stop`, see `PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK` to disable), but that only guarantees errors are caught — it does not give you real output validation. If you need actual output comparison, put that logic in `test/sql` instead (or use pgTap assertions from within the install file itself). +WARNING: **`+test/install/*.out+` is never actually compared against anything.** Unlike every other test type pgxntool supports, the `test/install` directory's actual output is written to the exact same file as its expected output, so there is no diff — a wrong or changed result will never fail the build, no matter how much it changes. The *only* thing that still fails the build is a hard SQL error, and only if the file sets `ON_ERROR_STOP` (directly, or via `\i test/pgxntool/psql.sql`; a file relying on `psql.sql` must never turn `ON_ERROR_STOP` back off) — without it, psql prints the error, keeps going, and the file "passes" regardless. pgxntool enforces this by default (`check-test-install-error-stop`, see `PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK` to disable), but that only guarantees errors are caught — it does not give you real output validation. If you need actual output comparison, put that logic in `test/sql` instead (or use pgTap assertions from within the install file itself). ==== Update & Upgrade (U&U) Testing @@ -771,7 +771,7 @@ Default: auto-detected -- `yes` if `test/install/*.sql` files exist, `no` otherw === PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK * -Default: `yes`. Enables or disables a build-time check that every `test/install/*.sql` file sets `ON_ERROR_STOP` (directly, or via `\i test/pgxntool/psql.sql`) -- see <<_testinstall,test/install>> for why this is the only thing that makes a hard error in one of those files actually fail the build. Set to `no` to disable the check. +Default: `yes`. Enables or disables a build-time check that every `test/install/*.sql` file sets `ON_ERROR_STOP`: either with an explicit `\set ON_ERROR_STOP on` (after which turning it off again is allowed), or via `\i test/pgxntool/psql.sql` with no later turn-off -- see <<_testinstall,test/install>> for why this is the only thing that makes a hard error in one of those files actually fail the build. Set to `no` to disable the check. === PGXNTOOL_ENABLE_FS_INSTALL * diff --git a/README.html b/README.html index 630b006..03048b3 100644 --- a/README.html +++ b/README.html @@ -898,7 +898,7 @@

Warning -test/install/*.out is never actually compared against anything. Unlike every other test type pgxntool supports, the test/install directory’s actual output is written to the exact same file as its expected output, so there is no diff — a wrong or changed result will never fail the build, no matter how much it changes. The only thing that still fails the build is a hard SQL error, and only if the file sets ON_ERROR_STOP (directly, or via \i test/pgxntool/psql.sql) — without it, psql prints the error, keeps going, and the file "passes" regardless. pgxntool enforces this by default (check-test-install-error-stop, see PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK to disable), but that only guarantees errors are caught — it does not give you real output validation. If you need actual output comparison, put that logic in test/sql instead (or use pgTap assertions from within the install file itself). +test/install/*.out is never actually compared against anything. Unlike every other test type pgxntool supports, the test/install directory’s actual output is written to the exact same file as its expected output, so there is no diff — a wrong or changed result will never fail the build, no matter how much it changes. The only thing that still fails the build is a hard SQL error, and only if the file sets ON_ERROR_STOP (directly, or via \i test/pgxntool/psql.sql; a file relying on psql.sql must never turn ON_ERROR_STOP back off) — without it, psql prints the error, keeps going, and the file "passes" regardless. pgxntool enforces this by default (check-test-install-error-stop, see PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK to disable), but that only guarantees errors are caught — it does not give you real output validation. If you need actual output comparison, put that logic in test/sql instead (or use pgTap assertions from within the install file itself). @@ -2065,7 +2065,7 @@

8.11. PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK *

-

Default: yes. Enables or disables a build-time check that every test/install/*.sql file sets ON_ERROR_STOP (directly, or via \i test/pgxntool/psql.sql) — see test/install for why this is the only thing that makes a hard error in one of those files actually fail the build. Set to no to disable the check.

+

Default: yes. Enables or disables a build-time check that every test/install/*.sql file sets ON_ERROR_STOP: either with an explicit \set ON_ERROR_STOP on (after which turning it off again is allowed), or via \i test/pgxntool/psql.sql with no later turn-off — see test/install for why this is the only thing that makes a hard error in one of those files actually fail the build. Set to no to disable the check.

@@ -2134,7 +2134,7 @@

diff --git a/base.mk b/base.mk index e6b4a53..9ddc072 100644 --- a/base.mk +++ b/base.mk @@ -201,7 +201,8 @@ endif # 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. So it is # entirely up to each test/install/*.sql file to `\set ON_ERROR_STOP on` (or -# `\i test/pgxntool/psql.sql`, which already does) if it wants failures +# `\i test/pgxntool/psql.sql`, which already does, as long as the file never +# turns it back off) if it wants failures # caught at all. check-test-install-error-stop below enforces this by # default; see PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK to disable it. # diff --git a/test/bin/check-test-install-error-stop.sh b/test/bin/check-test-install-error-stop.sh index 61f2700..ba91c40 100755 --- a/test/bin/check-test-install-error-stop.sh +++ b/test/bin/check-test-install-error-stop.sh @@ -12,13 +12,14 @@ # net so its absence is caught at build time instead of discovered the hard # way (issue #97). # -# A file passes if it explicitly turns ON_ERROR_STOP on at some point -- -# directly (`\set ON_ERROR_STOP on`/true/1/yes) or by sourcing -# test/pgxntool/psql.sql (which already turns it on, among other things). A -# later explicit turn-off doesn't undo an earlier turn-on. A file that only -# ever turns it off, or never mentions it at all, fails: a bare substring -# match on "ON_ERROR_STOP" would wrongly pass a file that only turns it off, -# so each `\set` needs its value read, in file order. +# A file passes if it either: +# - explicitly enables ON_ERROR_STOP itself (`\set ON_ERROR_STOP` on/true/1/ +# yes) anywhere; it may then also disable it anywhere, or +# - includes test/pgxntool/psql.sql and never disables ON_ERROR_STOP +# (`\set ON_ERROR_STOP` off/false/0/no, or `\unset ON_ERROR_STOP`). +# Every other file fails. A user who explicitly enables ON_ERROR_STOP is +# assumed to know what they're doing if they also disable it; there's no +# reason to think a user who includes psql.sql knows it enables ON_ERROR_STOP. # # Usage: check-test-install-error-stop.sh @@ -35,44 +36,48 @@ testdir="$1" install_dir="$testdir/install" missing=() -# Scans $1 in file order, tracking whether ON_ERROR_STOP has been explicitly -# turned on. Succeeds (exit 0) once an on-value is seen; a later off-value -# doesn't reset that. Sourcing psql.sql counts as turning it on, wherever it -# occurs in the file. -file_turns_error_stop_on() { - local f="$1" line value +# Prints why $1 fails the rule in the header, or nothing if it passes. +error_stop_problem() { + local f="$1" line direct_on=no psql_sql=no disabled=no while IFS= read -r line; do if [[ "$line" =~ \\ir?[[:space:]]+.*psql\.sql ]]; then - return 0 - fi - if [[ "$line" =~ \\set[[:space:]]+ON_ERROR_STOP[[:space:]]+([^[:space:]]+) ]]; then - value="${BASH_REMATCH[1],,}" - case "$value" in - on|true|1|yes) return 0 ;; + psql_sql=yes + elif [[ "$line" =~ \\unset[[:space:]]+ON_ERROR_STOP([[:space:]]|$) ]]; then + disabled=yes + elif [[ "$line" =~ \\set[[:space:]]+ON_ERROR_STOP[[:space:]]+([^[:space:]]+) ]]; then + case "${BASH_REMATCH[1],,}" in + on|true|1|yes) direct_on=yes ;; + off|false|0|no) disabled=yes ;; esac fi done < "$f" - return 1 + + if [ "$direct_on" = yes ]; then + return + elif [ "$psql_sql" = no ]; then + echo "never sets ON_ERROR_STOP" + elif [ "$disabled" = yes ]; then + echo "includes psql.sql but also disables ON_ERROR_STOP" + fi } for f in "$install_dir"/*.sql; do [ -f "$f" ] || continue - - if file_turns_error_stop_on "$f"; then - continue + problem=$(error_stop_problem "$f") + if [ -n "$problem" ]; then + missing+=("$f: $problem") fi - - missing+=("$f") done if [ "${#missing[@]}" -gt 0 ]; then - error "the following test/install/*.sql files don't set ON_ERROR_STOP:" + error "the following test/install/*.sql files don't reliably set ON_ERROR_STOP:" printf ' %s\n' "${missing[@]}" >&2 error "test/install files run in their own self-comparing pg_regress entry" \ "(see the test/install comments in base.mk) -- without ON_ERROR_STOP, a" \ "hard SQL error is silently swallowed instead of failing the build." - die 1 "Add '\\set ON_ERROR_STOP on' near the top of the file, or" \ - "'\\i test/pgxntool/psql.sql' (which already sets it)." + die 1 "Add '\\set ON_ERROR_STOP on' near the top of the file. Including" \ + "test/pgxntool/psql.sql (which also sets it) is enough only if the file" \ + "never turns ON_ERROR_STOP off." fi # vi: expandtab ts=2 sw=2