From b95c9a376f827ff141b8bd5811b680caba4a609a Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Tue, 15 Sep 2026 17:27:16 -0500 Subject: [PATCH 1/5] Cover verify-results-pgtap.sh's regression.diffs classification Paired with pgxntool commit ec7c7ee, which lets `make results` seed the first expected output for a brand-new test (pgxntool issue #119). New `test/standard/verify-results-pgtap-script.bats` owns the script's decision logic, invoked directly against a scratch directory with no Make, foundation environment or PostgreSQL: the no-baseline exemption, a real mismatch still blocking, an unblessed file holding a SQL error still blocking, unrecognized `regression.diffs` content failing closed, classification driven by unprefixed `diff `/`@@ ` headers rather than diffed content, and the pre-existing `not ok`/TODO/plan-mismatch scan. `make-test.bats` keeps only what needs a real `pg_regress` run: that `make results` really does seed a new test's first expected output end-to-end, and that `make verify-results` invokes the script and propagates its failure. Its TODO and plan-mismatch cases moved to the script-level file rather than paying for a full `make verify-results` run each to re-test logic decided entirely inside the script. Co-Authored-By: Claude Opus 5 (1M context) --- test/standard/make-test.bats | 63 +++--- .../standard/verify-results-pgtap-script.bats | 205 ++++++++++++++++++ 2 files changed, 234 insertions(+), 34 deletions(-) create mode 100644 test/standard/verify-results-pgtap-script.bats diff --git a/test/standard/make-test.bats b/test/standard/make-test.bats index 9b29821..d39d50a 100755 --- a/test/standard/make-test.bats +++ b/test/standard/make-test.bats @@ -13,8 +13,9 @@ # (issue #79) # - check-stale-expected catches orphaned test/expected/*.out files (issue #14) # - `make test` exits non-zero on a real regression.diffs mismatch (issue #49) -# - verify-results blocks `make results` when tests are failing, detects -# pgtap failures, and can be disabled +# - verify-results blocks `make results` when tests are failing, lets a +# brand-new test's first expected output through (issue #119), and can be +# disabled. Its own pass/fail logic lives in verify-results-pgtap-script.bats load ../lib/helpers @@ -529,54 +530,48 @@ EOF assert_success } -@test "verify-results detects pgtap failures in result files" { +@test "make results seeds the first expected output for a brand-new test (issue #119)" { skip_if_no_postgres - mkdir -p test/results - cat > test/results/pgtap_fail.out <<'EOF' -1..2 -ok 1 - passing test -not ok 2 - failing test -EOF - - run make verify-results - assert_failure - assert_contains "$output" "pgtap failure detected" + # base.mk touches an empty test/expected/.out for any test/sql/*.sql + # that lacks one (pg_regress aborts on a missing expected file), so + # pg_regress reports a difference no matter how clean the new test's actual + # output is. This is the one case that needs a real pg_regress run: it + # proves the diff pg_regress writes against that placeholder really is the + # shape verify-results-pgtap.sh treats as "no baseline yet" rather than as + # a failure. The script's own classification is covered cheaply in + # verify-results-pgtap-script.bats. + cp test/sql/pgxntool-test.sql test/sql/brand_new.sql - rm -f test/results/pgtap_fail.out -} - -@test "verify-results ignores pgtap TODO failures" { - skip_if_no_postgres - - mkdir -p test/results - cat > test/results/pgtap_todo.out <<'EOF' -1..1 -not ok 1 - known issue # TODO fix later -EOF + run make results + assert_success + assert_file_exists "test/expected/brand_new.out" - run make verify-results + # The seeded baseline is what the test actually produces. + run make test assert_success - rm -f test/results/pgtap_todo.out + rm -f test/sql/brand_new.sql test/expected/brand_new.out } -@test "verify-results detects pgtap plan mismatch" { +@test "verify-results propagates a pgtap failure the script detects" { skip_if_no_postgres + # Wiring only -- that `make verify-results` really invokes the script and + # surfaces its failure. Which pgtap output counts as a failure (TODO + # exclusion, plan mismatches) is decided by the script and tested there. mkdir -p test/results - cat > test/results/pgtap_plan.out <<'EOF' -1..3 -ok 1 - test one -ok 2 - test two -# Looks like you planned 3 tests but ran 2 + cat > test/results/pgtap_fail.out <<'EOF' +1..2 +ok 1 - passing test +not ok 2 - failing test EOF run make verify-results assert_failure - assert_contains "$output" "pgtap plan mismatch" + assert_contains "$output" "pgtap failure detected" - rm -f test/results/pgtap_plan.out + rm -f test/results/pgtap_fail.out } # ============================================================================ diff --git a/test/standard/verify-results-pgtap-script.bats b/test/standard/verify-results-pgtap-script.bats new file mode 100644 index 0000000..438dd9e --- /dev/null +++ b/test/standard/verify-results-pgtap-script.bats @@ -0,0 +1,205 @@ +#!/usr/bin/env bats + +# Test: verify-results-pgtap.sh - pure script-logic unit tests +# +# These tests exercise pgxntool's verify-results-pgtap.sh directly against a +# bare scratch directory -- no foundation environment, no `make`, no +# PostgreSQL. They own all of the script's decision logic: which pgtap output +# in results/*.out counts as a failure, and which regression.diffs entries +# block `make results` (a real mismatch against an existing baseline) versus +# which are a brand-new test that has no expected output yet (issue #119). +# +# Tests that need real Make/PostgreSQL integration stay in make-test.bats: +# that `make verify-results` invokes this script and propagates its exit +# status, and that a real pg_regress run against base.mk's empty +# expected-file placeholder actually produces the "@@ -0,0" diff shape the +# classification below keys on. + +load ../lib/helpers +load ../lib/assertions + +setup_file() { + setup_topdir + load_test_env "verify-results-pgtap-script" +} + +setup() { + load_test_env "verify-results-pgtap-script" + export SCRIPT="$PGXNREPO/verify-results-pgtap.sh" + + # Fresh, empty scratch directory per test -- no foundation/TEST_REPO needed. + export TESTOUT="$BATS_TEST_TMPDIR/testout" + mkdir -p "$TESTOUT/results" +} + +# Record $2 as test $1's pg_regress output file. +write_results() { + printf '%s\n' "$2" > "$TESTOUT/results/$1.out" +} + +# Append the regression.diffs block pg_regress would write for test $1: the +# "diff " header, the ---/+++ file lines, then $2 +# as the diff body (hunk headers included). +append_diff_block() { + local name=$1 body=$2 + { + printf 'diff -U3 %s %s\n' "$TESTOUT/expected/$name.out" "$TESTOUT/results/$name.out" + printf -- '--- %s\t2026-01-01 00:00:00.000000000 +0000\n' "$TESTOUT/expected/$name.out" + printf -- '+++ %s\t2026-01-01 00:00:00.000000000 +0000\n' "$TESTOUT/results/$name.out" + printf '%s\n' "$body" + } >> "$TESTOUT/regression.diffs" +} + +@test "verify-results-pgtap.sh: passes on all-ok output with no regression.diffs" { + write_results passing '1..1 +ok 1' + + run "$SCRIPT" "$TESTOUT" + assert_success +} + +@test "verify-results-pgtap.sh: allows a brand-new test that has no expected output yet (issue #119)" { + # base.mk touches an empty test/expected/.out for a test that lacks + # one, so pg_regress always reports a difference against it: a single + # "@@ -0,0" hunk. That is unwinnable to block on -- creating that first + # expected file is what `make results` is for. + write_results newtest '1..1 +ok 1' + append_diff_block newtest '@@ -0,0 +1,2 @@ ++1..1 ++ok 1' + + run "$SCRIPT" "$TESTOUT" + assert_success + assert_contains "$output" "no expected output yet" + assert_contains "$output" "newtest.out" +} + +@test "verify-results-pgtap.sh: still blocks a real mismatch against an existing baseline" { + write_results oldtest '1..1 +ok 1' + append_diff_block oldtest '@@ -1,2 +1,2 @@ + 1..1 +-ok 1 - renamed ++ok 1' + + run "$SCRIPT" "$TESTOUT" + assert_failure_with_status 1 + assert_contains "$output" "Tests are failing" + assert_contains "$output" "Cannot run 'make results'" + assert_contains "$output" "oldtest.out" +} + +@test "verify-results-pgtap.sh: a real mismatch still blocks alongside a brand-new test" { + write_results newtest '1..1 +ok 1' + append_diff_block newtest '@@ -0,0 +1,2 @@ ++1..1 ++ok 1' + write_results oldtest '1..1 +ok 1' + append_diff_block oldtest '@@ -1,2 +1,2 @@ + 1..1 +-ok 1 - renamed ++ok 1' + + run "$SCRIPT" "$TESTOUT" + assert_failure_with_status 1 + assert_contains "$output" "Tests are failing" + assert_contains "$output" "oldtest.out" +} + +@test "verify-results-pgtap.sh: refuses to bless a brand-new test whose output holds a SQL error" { + # ON_ERROR_STOP aborts the script at the error, so pgtap emits neither + # 'not ok' nor its plan-mismatch line and the scan above finds nothing. + write_results errtest '1..2 +ok 1 +ERROR: 42P01: relation "nope" does not exist' + append_diff_block errtest '@@ -0,0 +1,3 @@ ++1..2 ++ok 1 ++ERROR: 42P01: relation "nope" does not exist' + + run "$SCRIPT" "$TESTOUT" + assert_failure_with_status 1 + assert_contains "$output" "errtest.out" +} + +@test "verify-results-pgtap.sh: classifies by unprefixed headers, not by diffed content" { + # Every content line in a diff carries a ' ', '+' or '-' prefix, so output + # that itself contains block or hunk headers must not be mistaken for one. + write_results tricky '1..1 +ok 1' + append_diff_block tricky '@@ -0,0 +1,4 @@ ++diff -U3 a.out b.out ++@@ -1,2 +1,2 @@ ++1..1 ++ok 1' + + run "$SCRIPT" "$TESTOUT" + assert_success + assert_contains "$output" "no expected output yet" +} + +@test "verify-results-pgtap.sh: a deleted '-- comment' line doesn't read as a file header" { + # A removed "-- comment" renders as "--- comment", which is why the + # classification keys on "diff "/"@@ " lines rather than the ---/+++ pair. + write_results commented '1..1 +ok 1' + append_diff_block commented '@@ -1,3 +1,2 @@ + 1..1 +--- vi: expandtab ts=2 sw=2 ++ok 1' + + run "$SCRIPT" "$TESTOUT" + assert_failure_with_status 1 + assert_contains "$output" "commented.out" +} + +@test "verify-results-pgtap.sh: blocks on regression.diffs content it cannot classify" { + write_results passing '1..1 +ok 1' + printf 'truncated junk with no diff header\n' > "$TESTOUT/regression.diffs" + + run "$SCRIPT" "$TESTOUT" + assert_failure_with_status 1 + assert_contains "$output" "unrecognized" +} + +@test "verify-results-pgtap.sh: detects a pgtap failure" { + write_results failing '1..2 +ok 1 +not ok 2 - broken' + + run "$SCRIPT" "$TESTOUT" + assert_failure_with_status 1 + assert_contains "$output" "pgtap failure detected" + assert_contains "$output" "not ok 2 - broken" +} + +@test "verify-results-pgtap.sh: ignores a TODO failure" { + write_results todo '1..1 +not ok 1 - known issue # TODO fix later' + + run "$SCRIPT" "$TESTOUT" + assert_success +} + +@test "verify-results-pgtap.sh: detects a pgtap plan mismatch" { + write_results planned '1..3 +ok 1 +ok 2 +# Looks like you planned 3 tests but ran 2' + + run "$SCRIPT" "$TESTOUT" + assert_failure_with_status 1 + assert_contains "$output" "pgtap plan mismatch" +} + +@test "verify-results-pgtap.sh: usage error when called with no TESTOUT argument" { + run "$SCRIPT" + assert_failure + assert_contains "$output" "Usage:" +} + +# vi: expandtab ts=2 sw=2 From 0b74e36de5902c3ee85e5327f1ab5c4d069bb946 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Tue, 15 Sep 2026 17:32:59 -0500 Subject: [PATCH 2/5] Clean up the brand-new test's results file too `make results` blesses every `test/results/*.out` into `test/expected/`, so a leftover `results/brand_new.out` would reappear as an orphaned expected file for `check-stale-expected` the next time any test in this environment runs `make results`. Co-Authored-By: Claude Opus 5 (1M context) --- test/standard/make-test.bats | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/standard/make-test.bats b/test/standard/make-test.bats index d39d50a..f154d30 100755 --- a/test/standard/make-test.bats +++ b/test/standard/make-test.bats @@ -551,7 +551,9 @@ EOF run make test assert_success - rm -f test/sql/brand_new.sql test/expected/brand_new.out + # results/ too: `make results` blesses every results/*.out into expected/, + # so a leftover here would reappear as an orphan for check-stale-expected. + rm -f test/sql/brand_new.sql test/expected/brand_new.out test/results/brand_new.out } @test "verify-results propagates a pgtap failure the script detects" { From 8737c048b8cc6418a6d109bd48277e7d2b5d2b2c Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Tue, 15 Sep 2026 17:56:51 -0500 Subject: [PATCH 3/5] Pin the fail-closed branches Warden found unguarded Paired with pgxntool commit 93aa10f. The script's fail-closed claims were asserted in the PR body but only partly tested, and two of them weren't true: an empty `regression.diffs` and junk following a valid block both passed. Adds script-level coverage for an empty `regression.diffs`, a block with no hunks (also what a pre-12 context diff degrades to) and one with only some `@@ -0,0` hunks, unrecognized content after a valid block, and both of psql's real error renderings -- column 0 and the `psql:::` prefix an \i'd file produces -- plus the passing test whose description merely mentions `ERROR:`, which used to be blocked. The end-to-end `make results` test now asserts the "no expected output yet" message, so it can't pass on a file that appeared some other way. Co-Authored-By: Claude Opus 5 (1M context) --- test/standard/make-test.bats | 4 + .../standard/verify-results-pgtap-script.bats | 93 +++++++++++++++++++ 2 files changed, 97 insertions(+) diff --git a/test/standard/make-test.bats b/test/standard/make-test.bats index f154d30..13a1470 100755 --- a/test/standard/make-test.bats +++ b/test/standard/make-test.bats @@ -545,6 +545,10 @@ EOF run make results assert_success + # The NOTE proves the no-baseline branch is what let this through, rather + # than the file appearing for some other reason. + assert_contains "$output" "no expected output yet" + assert_contains "$output" "brand_new.out" assert_file_exists "test/expected/brand_new.out" # The seeded baseline is what the test actually produces. diff --git a/test/standard/verify-results-pgtap-script.bats b/test/standard/verify-results-pgtap-script.bats index 438dd9e..c9473af 100644 --- a/test/standard/verify-results-pgtap-script.bats +++ b/test/standard/verify-results-pgtap-script.bats @@ -112,6 +112,12 @@ ok 1' @test "verify-results-pgtap.sh: refuses to bless a brand-new test whose output holds a SQL error" { # ON_ERROR_STOP aborts the script at the error, so pgtap emits neither # 'not ok' nor its plan-mismatch line and the scan above finds nothing. + # + # Both of psql's real renderings have to block. pg_regress feeds psql on + # stdin, so an error in the test file itself starts at column 0, while one + # inside an \i'd file (setup.sql/finish.sql, which every pgxntool test + # sources) carries a "psql::: " prefix -- a column-anchored + # match would sail straight past the second. write_results errtest '1..2 ok 1 ERROR: 42P01: relation "nope" does not exist' @@ -123,6 +129,36 @@ ERROR: 42P01: relation "nope" does not exist' run "$SCRIPT" "$TESTOUT" assert_failure_with_status 1 assert_contains "$output" "errtest.out" + + rm -f "$TESTOUT/regression.diffs" + write_results errtest '1..2 +ok 1 +psql:test/pgxntool/setup.sql:3: ERROR: 42P01: relation "nope" does not exist' + append_diff_block errtest '@@ -0,0 +1,3 @@ ++1..2 ++ok 1 ++psql:test/pgxntool/setup.sql:3: ERROR: 42P01: relation "nope" does not exist' + + run "$SCRIPT" "$TESTOUT" + assert_failure_with_status 1 + assert_contains "$output" "errtest.out" +} + +@test "verify-results-pgtap.sh: blesses a passing test whose description merely mentions ERROR:" { + # psql writes two spaces after the severity; a pgtap description quoting an + # error message does not, and isn't at the start of a line or after ": ". + # Without that distinction a legitimately-passing new test could never have + # its first expected output created. + write_results described '1..1 +ok 1 - raises ERROR: division by zero' + append_diff_block described '@@ -0,0 +1,2 @@ ++1..1 ++ok 1 - raises ERROR: division by zero' + + run "$SCRIPT" "$TESTOUT" + assert_success + assert_contains "$output" "no expected output yet" + assert_contains "$output" "described.out" } @test "verify-results-pgtap.sh: classifies by unprefixed headers, not by diffed content" { @@ -156,6 +192,63 @@ ok 1' assert_contains "$output" "commented.out" } +@test "verify-results-pgtap.sh: blocks when regression.diffs exists but is empty" { + # pg_regress truncates the file at startup and removes it again on a clean + # finish, so an empty one means it died before comparing anything -- and + # base.mk's `.IGNORE: installcheck` lets `make results` reach here anyway. + # results/ then holds whatever an earlier run left, so passing this would + # bless stale output. + write_results stale '1..1 +ok 1' + : > "$TESTOUT/regression.diffs" + + run "$SCRIPT" "$TESTOUT" + assert_failure_with_status 1 + assert_contains "$output" "pg_regress did not complete" +} + +@test "verify-results-pgtap.sh: blocks a block whose hunks aren't all '@@ -0,0'" { + # Two fail-closed shapes that aren't a clean no-baseline block: a header + # with no hunks at all (what a pre-12 context diff looks like here), and a + # block that only partly diffed against an empty file. + write_results nohunk '1..1 +ok 1' + append_diff_block nohunk '' + + run "$SCRIPT" "$TESTOUT" + assert_failure_with_status 1 + assert_contains "$output" "nohunk.out" + + rm -f "$TESTOUT/regression.diffs" + write_results mixed '1..1 +ok 1' + append_diff_block mixed '@@ -0,0 +1,1 @@ ++1..1 +@@ -3,1 +4,1 @@ +-was here ++ok 1' + + run "$SCRIPT" "$TESTOUT" + assert_failure_with_status 1 + assert_contains "$output" "mixed.out" +} + +@test "verify-results-pgtap.sh: blocks when unrecognized content follows a valid block" { + # The whole file has to fail closed, not just refuse to classify: a block + # that reads as blessable is no reason to ignore text after it that fits no + # part of the diff format. + write_results newtest '1..1 +ok 1' + append_diff_block newtest '@@ -0,0 +1,2 @@ ++1..1 ++ok 1' + printf 'diff: /nonexistent: No such file or directory\n' >> "$TESTOUT/regression.diffs" + + run "$SCRIPT" "$TESTOUT" + assert_failure_with_status 1 + assert_contains "$output" "unrecognized content" +} + @test "verify-results-pgtap.sh: blocks on regression.diffs content it cannot classify" { write_results passing '1..1 ok 1' From 98b434b501e6d19550e37b3e5b97706734482254 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Tue, 15 Sep 2026 17:57:58 -0500 Subject: [PATCH 4/5] Guard diff's no-newline marker against the junk check `\ No newline at end of file` is part of the diff format, so the new unrecognized-content check must not read it as junk -- doing so would block the first bless of any test whose output lacks a trailing newline. Co-Authored-By: Claude Opus 5 (1M context) --- test/standard/verify-results-pgtap-script.bats | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/standard/verify-results-pgtap-script.bats b/test/standard/verify-results-pgtap-script.bats index c9473af..402401a 100644 --- a/test/standard/verify-results-pgtap-script.bats +++ b/test/standard/verify-results-pgtap-script.bats @@ -247,6 +247,19 @@ ok 1' run "$SCRIPT" "$TESTOUT" assert_failure_with_status 1 assert_contains "$output" "unrecognized content" + + # ...but diff's own no-newline marker is part of the format, not junk. + # Treating it as junk would block a legitimate first bless of any test + # whose output doesn't end in a newline. + rm -f "$TESTOUT/regression.diffs" + append_diff_block newtest '@@ -0,0 +1,2 @@ ++1..1 ++ok 1 +\ No newline at end of file' + + run "$SCRIPT" "$TESTOUT" + assert_success + assert_contains "$output" "no expected output yet" } @test "verify-results-pgtap.sh: blocks on regression.diffs content it cannot classify" { From 6a4858e937c37746922f3ccbafcd24d18964629d Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Tue, 15 Sep 2026 18:00:29 -0500 Subject: [PATCH 5/5] Pin the remaining unclassifiable-content branch Diff body lines with no `diff ` header before them produce no classification rather than an unrecognized one, so they reach a separate guard from the junk case above it. Both have to block; only one was tested. Co-Authored-By: Claude Opus 5 (1M context) --- test/standard/verify-results-pgtap-script.bats | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/standard/verify-results-pgtap-script.bats b/test/standard/verify-results-pgtap-script.bats index 402401a..dd60920 100644 --- a/test/standard/verify-results-pgtap-script.bats +++ b/test/standard/verify-results-pgtap-script.bats @@ -270,6 +270,14 @@ ok 1' run "$SCRIPT" "$TESTOUT" assert_failure_with_status 1 assert_contains "$output" "unrecognized" + + # Diff body lines with no header before them yield no classification at + # all rather than an unrecognized one, which has to block just the same. + printf ' 1..1\n+ok 1\n' > "$TESTOUT/regression.diffs" + + run "$SCRIPT" "$TESTOUT" + assert_failure_with_status 1 + assert_contains "$output" "unrecognized" } @test "verify-results-pgtap.sh: detects a pgtap failure" {