diff --git a/CLAUDE.md b/CLAUDE.md index 3b5e227..ef920d8 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 results-build` 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 0b02dcd..06feeda 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 3c56ac7..596c892 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 191cd0a..4ffdaf2 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 fd796d3..186e894 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 26c98e5..ba91c40 100755 --- a/test/bin/check-test-install-error-stop.sh +++ b/test/bin/check-test-install-error-stop.sh @@ -12,8 +12,14 @@ # 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 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 @@ -30,27 +36,48 @@ testdir="$1" install_dir="$testdir/install" missing=() -for f in "$install_dir"/*.sql; do - [ -f "$f" ] || continue +# 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 + 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" - if grep -q 'ON_ERROR_STOP' "$f"; then - continue - fi - if grep -qE '\\ir? +.*psql\.sql' "$f"; then - continue + 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 +} - missing+=("$f") +for f in "$install_dir"/*.sql; do + [ -f "$f" ] || continue + problem=$(error_stop_problem "$f") + if [ -n "$problem" ]; then + missing+=("$f: $problem") + fi 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