From ec7c7ee8a4dc90c0fbc02e1c17b0decc110729f7 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Tue, 15 Sep 2026 17:27:04 -0500 Subject: [PATCH 1/3] Let `make results` seed a new test's first expected output `verify-results` blocked `make results` whenever `regression.diffs` existed at all. A test with no expected output file always lands in `regression.diffs` -- `base.mk` touches an empty `test/expected/.out` for it, because `pg_regress` aborts outright on a missing one -- so seeding that first expected file was structurally impossible no matter how clean the new test's actual output was. `verify-results` now classifies each `regression.diffs` block by its hunk headers: a block whose hunks all read `@@ -0,0 +N,M @@` was diffed against that empty placeholder and is let through; anything else still blocks. A test whose output matched any expected file, `_N.out` alternates included, never appears in `regression.diffs` at all, so nothing here re-derives a pass/fail `pg_regress` already decided. One case is still blocked: an as-yet-unblessed file containing a SQL error. `ON_ERROR_STOP` aborts the script before pgtap emits `not ok` or its plan-mismatch line, so the scan above cannot see it, and blessing it would make the error the new baseline. Fixes #119 Co-Authored-By: Claude Opus 5 (1M context) --- HISTORY.asc | 17 ++++++++- README.asc | 6 ++-- README.html | 8 ++--- verify-results-pgtap.sh | 76 ++++++++++++++++++++++++++++++++++++----- 4 files changed, 90 insertions(+), 17 deletions(-) diff --git a/HISTORY.asc b/HISTORY.asc index da32ce4..e2c3f6a 100644 --- a/HISTORY.asc +++ b/HISTORY.asc @@ -1,5 +1,20 @@ STABLE ------ +== Fix `make results` refusing to create a new test's first expected output +`verify-results` blocked `make results` whenever `regression.diffs` existed +at all, which made seeding the first `test/expected/.out` for a +brand-new test impossible: pgxntool creates an empty placeholder for a test +that has no expected file (pg_regress aborts outright on a missing one), so +pg_regress always reports a difference against it no matter how clean the +new test's actual output is. + +`verify-results` now reads what `regression.diffs` actually says. A test +that differs only from that empty placeholder is allowed through, provided +its own pgtap output is clean and contains no SQL error; a test that differs +from a real expected file still blocks `make results` as before. This +applies to the default `PGXNTOOL_VERIFY_RESULTS_MODE=pgtap`; +`PGXNTOOL_VERIFY_RESULTS_MODE=diffs` still blocks on any `regression.diffs`. + == Add `PGXNTOOL_ENABLE_FS_INSTALL` and `PGXNTOOL_ENABLE_PGXN_INSTALL` `test`/`verify-results` unconditionally filesystem-installed the extension (via PGXS's `install`), and `installcheck` unconditionally auto-installed @@ -43,7 +58,7 @@ None of these are documented anywhere as override points, but if you happened to reference one directly (unsupported, but possible), update to the new name. -Issues fixed in this release: #87, #21, #55, #90 +Issues fixed in this release: #87, #21, #55, #90, #119 2.3.0 ----- diff --git a/README.asc b/README.asc index bf48c80..14d63b6 100644 --- a/README.asc +++ b/README.asc @@ -237,8 +237,8 @@ Never run `make results` without first verifying the test changes are correct. T ==== verify-results safeguard By default, `make results` will refuse to run if your tests are failing, so you can't accidentally promote incorrect output into the new expected results. Which failures it looks for is controlled by `PGXNTOOL_VERIFY_RESULTS_MODE`: -`pgtap` (the default):: Scans `test/results/*.out` for pgTAP `not ok` lines (ignoring `# TODO` items) and plan-count mismatches, then also checks `test/results/regression.diffs` for any output mismatch pgTAP itself wouldn't catch. -`diffs`:: Only checks whether `test/results/regression.diffs` exists. +`pgtap` (the default):: Scans `test/results/*.out` for pgTAP `not ok` lines (ignoring `# TODO` items) and plan-count mismatches, then also checks `test/regression.diffs` for any output mismatch pgTAP itself wouldn't catch. A test that has no expected output file yet is the one exception: it always shows up in `regression.diffs` (pgxntool creates an empty expected file for it, because `pg_regress` aborts on a missing one), so `make results` is allowed to create its first expected output — provided its own pgTAP output is clean and free of SQL errors. +`diffs`:: Only checks whether `test/regression.diffs` exists, so it can't create a new test's first expected output; use `pgtap` mode, or `PGXNTOOL_ENABLE_VERIFY_RESULTS=no` for that one run. If tests are failing, `make results` prints an explanation of what failed and exits without touching `test/expected/`; fix the failures, then re-run `make results`. @@ -729,7 +729,7 @@ Default: `pgtap`. Controls how the <<_verify_results_safeguard,verify-results sa ==== pgtap -Scans `test/results/*.out` for pgTap `not ok` lines and plan mismatches, falling back to checking for `regression.diffs` too. Use this mode when your test suite uses pgTap. +Scans `test/results/*.out` for pgTap `not ok` lines and plan mismatches, falling back to checking `regression.diffs` too — except for a test that has no expected output yet, whose first expected file `make results` is allowed to create. Use this mode when your test suite uses pgTap. ==== diffs diff --git a/README.html b/README.html index c3428cd..449e9d9 100644 --- a/README.html +++ b/README.html @@ -1000,11 +1000,11 @@

pgtap (the default)
-

Scans test/results/*.out for pgTAP not ok lines (ignoring # TODO items) and plan-count mismatches, then also checks test/results/regression.diffs for any output mismatch pgTAP itself wouldn’t catch.

+

Scans test/results/*.out for pgTAP not ok lines (ignoring # TODO items) and plan-count mismatches, then also checks test/regression.diffs for any output mismatch pgTAP itself wouldn’t catch. A test that has no expected output file yet is the one exception: it always shows up in regression.diffs (pgxntool creates an empty expected file for it, because pg_regress aborts on a missing one), so make results is allowed to create its first expected output — provided its own pgTAP output is clean and free of SQL errors.

diffs
-

Only checks whether test/results/regression.diffs exists.

+

Only checks whether test/regression.diffs exists, so it can’t create a new test’s first expected output; use pgtap mode, or PGXNTOOL_ENABLE_VERIFY_RESULTS=no for that one run.

@@ -1980,7 +1980,7 @@

8.8.1. pgtap

-

Scans test/results/*.out for pgTap not ok lines and plan mismatches, falling back to checking for regression.diffs too. Use this mode when your test suite uses pgTap.

+

Scans test/results/*.out for pgTap not ok lines and plan mismatches, falling back to checking regression.diffs too — except for a test that has no expected output yet, whose first expected file make results is allowed to create. Use this mode when your test suite uses pgTap.

@@ -2068,7 +2068,7 @@

diff --git a/verify-results-pgtap.sh b/verify-results-pgtap.sh index dd8d7e9..6c4c727 100755 --- a/verify-results-pgtap.sh +++ b/verify-results-pgtap.sh @@ -2,7 +2,8 @@ # pgxntool/verify-results-pgtap.sh - Check pgtap results before 'make results' # # Scans pgtap output files for failures and plan mismatches, then checks -# regression.diffs as a fallback. Exits non-zero if any problems are found. +# regression.diffs as a fallback, ignoring entries for tests that have no +# expected output yet. Exits non-zero if any problems are found. # # Usage: verify-results-pgtap.sh TESTOUT # @@ -36,12 +37,69 @@ if [ $failed -ne 0 ]; then exit 1 fi -# Also check regression.diffs (output mismatch even if pgtap all passed) -if [ -r "$TESTOUT/regression.diffs" ]; then - echo "ERROR: Tests are failing. Cannot run 'make results'." - echo "Fix test failures first, then run 'make results'." - echo - echo "See $TESTOUT/regression.diffs for details:" - cat "$TESTOUT/regression.diffs" - exit 1 +# Also check regression.diffs (output mismatch even if pgtap all passed). +# +# A test whose output matched ANY of its expected files - including a +# pg_regress _N.out alternate - is left out of regression.diffs entirely, so +# every block in here is a test that matched none of them. The one exception +# is a test that has no expected output yet: pg_regress aborts outright on a +# missing expected file, so base.mk touches an empty test/expected/.out +# for every test/sql/*.sql lacking one, and diffing actual output against that +# placeholder always reports a difference. Blocking on that is unwinnable - +# seeding that first expected file is precisely what `make results` is for - +# so those blocks are skipped here. +diffs="$TESTOUT/regression.diffs" +if [ -s "$diffs" ]; then + # pg_regress heads each block with "diff " (see + # results_differ() in pg_regress.c), then appends the diff itself. Only + # those headers and diff hunk headers can start with "diff " or "@@ " + # unprefixed; every line carrying file content is prefixed with ' ', '+' + # or '-'. A block whose hunks all read "@@ -0,0 +N,M @@" was diffed + # against an empty expected file, i.e. that placeholder. + classified=$(awk ' + function flush() { + if (results == "") return + print (hunks > 0 && unblessed ? "unblessed" : "regression") "\t" results + } + /^diff / { flush(); results = $NF; unblessed = 1; hunks = 0; next } + /^@@ / { hunks++; if ($0 !~ /^@@ -0,0 /) unblessed = 0; next } + END { flush() } + ' "$diffs") + + blocked= + blessable= + while IFS="$(printf '\t')" read -r status file; do + # An unblessed file holding a hard SQL error is not a passing test the + # pgtap scan above was merely quiet about: ON_ERROR_STOP aborts the + # script, so pgtap emits neither 'not ok' nor its plan-mismatch line. + # Blessing that would make the error the baseline. + if [ "$status" = unblessed ] && [ -r "$file" ] && ! grep -q 'ERROR:' "$file"; then + blessable="$blessable $file +" + else + blocked="$blocked $file +" + fi + done < Date: Tue, 15 Sep 2026 17:30:38 -0500 Subject: [PATCH 2/3] Avoid a leading parenthesized expression in awk's print `print (cond ? a : b) "\t" c` sits next to awk's grouped-argument-list syntax; assigning the branch to a local first is unambiguous in every awk and reads better regardless. Co-Authored-By: Claude Opus 5 (1M context) --- verify-results-pgtap.sh | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/verify-results-pgtap.sh b/verify-results-pgtap.sh index 6c4c727..f41c39d 100755 --- a/verify-results-pgtap.sh +++ b/verify-results-pgtap.sh @@ -57,9 +57,10 @@ if [ -s "$diffs" ]; then # or '-'. A block whose hunks all read "@@ -0,0 +N,M @@" was diffed # against an empty expected file, i.e. that placeholder. classified=$(awk ' - function flush() { + function flush( status) { if (results == "") return - print (hunks > 0 && unblessed ? "unblessed" : "regression") "\t" results + status = (hunks > 0 && unblessed) ? "unblessed" : "regression" + print status "\t" results } /^diff / { flush(); results = $NF; unblessed = 1; hunks = 0; next } /^@@ / { hunks++; if ($0 !~ /^@@ -0,0 /) unblessed = 0; next } From 93aa10f3414aef4fff5ed82d797942543aca90a6 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Tue, 15 Sep 2026 17:56:39 -0500 Subject: [PATCH 3/3] Close three holes in the regression.diffs classification An empty `regression.diffs` was treated as nothing to check. pg_regress writes the file at startup and only removes it again on the clean exit path, so every bail leaves a zero-byte one behind -- and `base.mk`'s `.IGNORE: installcheck` means `make results` still gets here. With `test/results/` holding an earlier run's output, that blessed stale results whenever pg_regress died before comparing anything. It is now a refusal of its own, with a message saying why rather than reporting failing tests it never saw. Unrecognized text was only caught when it was the *whole* file, so junk following an otherwise-blessable block was ignored. Any line fitting neither a header nor a diff body line now blocks the whole file. The SQL-error guard matched `ERROR:` anywhere, which blocked a passing test whose pgtap description merely quotes an error message. psql writes two spaces after the severity and puts it at the start of a line, or after a `psql::: ` prefix when the statement came from an \i'd file -- matching that shape keeps both real renderings blocked without the false positive. Anchoring to the line start alone would have missed the prefixed form, which is what pgxntool's own setup.sql and finish.sql produce. Co-Authored-By: Claude Opus 5 (1M context) --- HISTORY.asc | 8 +++++ README.asc | 4 ++- README.html | 7 ++-- verify-results-pgtap.sh | 71 ++++++++++++++++++++++++++++++++--------- 4 files changed, 72 insertions(+), 18 deletions(-) diff --git a/HISTORY.asc b/HISTORY.asc index e2c3f6a..57aa422 100644 --- a/HISTORY.asc +++ b/HISTORY.asc @@ -15,6 +15,14 @@ from a real expected file still blocks `make results` as before. This applies to the default `PGXNTOOL_VERIFY_RESULTS_MODE=pgtap`; `PGXNTOOL_VERIFY_RESULTS_MODE=diffs` still blocks on any `regression.diffs`. +An empty `regression.diffs` is now its own, separately reported refusal: +pg_regress writes one at startup and removes it again when it finishes with +nothing to report, so an empty one left behind means it died before +comparing anything (unreachable server, interrupt) and `test/results/` still +holds an earlier run's output. `make results` previously blocked that case +only as a side effect of blocking on the file's existence, and says so +explicitly now. + == Add `PGXNTOOL_ENABLE_FS_INSTALL` and `PGXNTOOL_ENABLE_PGXN_INSTALL` `test`/`verify-results` unconditionally filesystem-installed the extension (via PGXS's `install`), and `installcheck` unconditionally auto-installed diff --git a/README.asc b/README.asc index 14d63b6..55f0609 100644 --- a/README.asc +++ b/README.asc @@ -237,11 +237,13 @@ Never run `make results` without first verifying the test changes are correct. T ==== verify-results safeguard By default, `make results` will refuse to run if your tests are failing, so you can't accidentally promote incorrect output into the new expected results. Which failures it looks for is controlled by `PGXNTOOL_VERIFY_RESULTS_MODE`: -`pgtap` (the default):: Scans `test/results/*.out` for pgTAP `not ok` lines (ignoring `# TODO` items) and plan-count mismatches, then also checks `test/regression.diffs` for any output mismatch pgTAP itself wouldn't catch. A test that has no expected output file yet is the one exception: it always shows up in `regression.diffs` (pgxntool creates an empty expected file for it, because `pg_regress` aborts on a missing one), so `make results` is allowed to create its first expected output — provided its own pgTAP output is clean and free of SQL errors. +`pgtap` (the default):: Scans `test/results/*.out` for pgTAP `not ok` lines (ignoring `# TODO` items) and plan-count mismatches, then also checks `test/regression.diffs` for any output mismatch pgTAP itself wouldn't catch. A test that has no expected output file yet is the one exception: it always shows up in `regression.diffs` (pgxntool creates an empty expected file for it, because `pg_regress` aborts on a missing one), so `make results` is allowed to create its first expected output — provided its own pgTAP output is clean and free of SQL errors. If a new test legitimately needs output containing a SQL error blessed, use `PGXNTOOL_ENABLE_VERIFY_RESULTS=no` for that one run. `diffs`:: Only checks whether `test/regression.diffs` exists, so it can't create a new test's first expected output; use `pgtap` mode, or `PGXNTOOL_ENABLE_VERIFY_RESULTS=no` for that one run. If tests are failing, `make results` prints an explanation of what failed and exits without touching `test/expected/`; fix the failures, then re-run `make results`. +It also refuses when `pg_regress` didn't finish at all — an unreachable server, an interrupt — since `test/results/` then still holds an earlier run's output and blessing it would make that stale output the new expected results. + To select a mode explicitly: ---- # In your Makefile diff --git a/README.html b/README.html index 449e9d9..86e45d9 100644 --- a/README.html +++ b/README.html @@ -1000,7 +1000,7 @@

pgtap (the default)
-

Scans test/results/*.out for pgTAP not ok lines (ignoring # TODO items) and plan-count mismatches, then also checks test/regression.diffs for any output mismatch pgTAP itself wouldn’t catch. A test that has no expected output file yet is the one exception: it always shows up in regression.diffs (pgxntool creates an empty expected file for it, because pg_regress aborts on a missing one), so make results is allowed to create its first expected output — provided its own pgTAP output is clean and free of SQL errors.

+

Scans test/results/*.out for pgTAP not ok lines (ignoring # TODO items) and plan-count mismatches, then also checks test/regression.diffs for any output mismatch pgTAP itself wouldn’t catch. A test that has no expected output file yet is the one exception: it always shows up in regression.diffs (pgxntool creates an empty expected file for it, because pg_regress aborts on a missing one), so make results is allowed to create its first expected output — provided its own pgTAP output is clean and free of SQL errors. If a new test legitimately needs output containing a SQL error blessed, use PGXNTOOL_ENABLE_VERIFY_RESULTS=no for that one run.

diffs
@@ -1012,6 +1012,9 @@

+

It also refuses when pg_regress didn’t finish at all — an unreachable server, an interrupt — since test/results/ then still holds an earlier run’s output and blessing it would make that stale output the new expected results.

+

+

To select a mode explicitly:

@@ -2068,7 +2071,7 @@

diff --git a/verify-results-pgtap.sh b/verify-results-pgtap.sh index f41c39d..afd24a0 100755 --- a/verify-results-pgtap.sh +++ b/verify-results-pgtap.sh @@ -49,38 +49,79 @@ fi # seeding that first expected file is precisely what `make results` is for - # so those blocks are skipped here. diffs="$TESTOUT/regression.diffs" + +# pg_regress truncates regression.diffs at startup and deletes it again on a +# clean finish with nothing to report, so an empty one means it died before +# comparing anything - an unreachable server, an interrupt. Whatever is in +# results/ is then left over from some earlier run, and base.mk's `.IGNORE: +# installcheck` means `make results` gets here anyway, so blessing it would +# bless stale output. +if [ -e "$diffs" ] && [ ! -s "$diffs" ]; then + echo "ERROR: pg_regress did not complete. Cannot run 'make results'." + echo "$diffs is empty, so nothing was actually compared;" + echo "anything in $TESTOUT/results/ is left over from an earlier run." + exit 1 +fi + if [ -s "$diffs" ]; then # pg_regress heads each block with "diff " (see # results_differ() in pg_regress.c), then appends the diff itself. Only # those headers and diff hunk headers can start with "diff " or "@@ " - # unprefixed; every line carrying file content is prefixed with ' ', '+' - # or '-'. A block whose hunks all read "@@ -0,0 +N,M @@" was diffed - # against an empty expected file, i.e. that placeholder. + # unprefixed; every line carrying file content is prefixed with ' ', '+', + # '-' or '\'. A block whose hunks all read "@@ -0,0 +N,M @@" was diffed + # against an empty expected file, i.e. that placeholder. Anything that + # fits none of those shapes is something this has no business + # interpreting, so it blocks the whole file. + # + # Hunk headers only exist in unified diffs. PostgreSQL 12 was the first + # to write them (11 and older use -C3 context diffs), so on those every + # block lands in the blocked pile - the same refusal `make results` gave + # before any of this existed. classified=$(awk ' function flush( status) { if (results == "") return status = (hunks > 0 && unblessed) ? "unblessed" : "regression" print status "\t" results } - /^diff / { flush(); results = $NF; unblessed = 1; hunks = 0; next } - /^@@ / { hunks++; if ($0 !~ /^@@ -0,0 /) unblessed = 0; next } - END { flush() } + /^diff / { flush(); results = $NF; unblessed = 1; hunks = 0; next } + /^@@ / { hunks++; if ($0 !~ /^@@ -0,0 /) unblessed = 0; next } + /^[ +-]/ { next } + /^\\/ { next } + /^$/ { next } + { unrecognized = 1 } + END { flush(); if (unrecognized) print "unrecognized\t" FILENAME } ' "$diffs") blocked= blessable= while IFS="$(printf '\t')" read -r status file; do - # An unblessed file holding a hard SQL error is not a passing test the - # pgtap scan above was merely quiet about: ON_ERROR_STOP aborts the - # script, so pgtap emits neither 'not ok' nor its plan-mismatch line. - # Blessing that would make the error the baseline. - if [ "$status" = unblessed ] && [ -r "$file" ] && ! grep -q 'ERROR:' "$file"; then - blessable="$blessable $file + case $status in + unblessed) + # An unblessed file holding a hard SQL error is not a passing test + # the pgtap scan above was merely quiet about: ON_ERROR_STOP aborts + # the script, so pgtap emits neither 'not ok' nor its plan-mismatch + # line. Blessing that would make the error the baseline. + # + # pg_regress feeds psql on stdin, so a failing statement in the + # test file itself reports as "ERROR: ..." at column 0, while one + # inside an \i'd file (setup.sql, finish.sql) reports as + # "psql:test/pgxntool/setup.sql:3: ERROR: ...". Matching either + # position, and psql's own two spaces after the severity, keeps a + # passing test whose description merely mentions "ERROR:" out of it. + if [ -r "$file" ] && ! grep -qE '(^|: )ERROR: ' "$file"; then + blessable="$blessable $file " - else - blocked="$blocked $file + continue + fi + ;; + unrecognized) + blocked="$blocked (unrecognized content in $file) +" + continue + ;; + esac + blocked="$blocked $file " - fi done <