diff --git a/HISTORY.asc b/HISTORY.asc index da32ce4..57aa422 100644 --- a/HISTORY.asc +++ b/HISTORY.asc @@ -1,5 +1,28 @@ 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`. + +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 @@ -43,7 +66,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..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/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. 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 @@ -729,7 +731,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..86e45d9 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. 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/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.

@@ -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:

@@ -1980,7 +1983,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 +2071,7 @@

diff --git a/verify-results-pgtap.sh b/verify-results-pgtap.sh index dd8d7e9..afd24a0 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,111 @@ 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" +# 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" + +# 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. 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 } + /^[ +-]/ { 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 + 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 +" + continue + fi + ;; + unrecognized) + blocked="$blocked (unrecognized content in $file) +" + continue + ;; + esac + blocked="$blocked $file +" + done <