From ddff2527540a8047d5e86eee6e7971b998da697d Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Sun, 26 Jul 2026 14:13:04 -0500 Subject: [PATCH 1/6] Add test coverage for pgxntool commit 1e74ab7 (parallel-build corruption, stale-expected check, base.mk fixes): - Fix versioned SQL file generation race that doubled file content - Add check-stale-expected to catch orphaned test/expected/*.out files - Fix EXTRA_CLEAN target, add PGXN_REMOTE override, add base.mk include guard New BATS coverage in test/standard/: - versioned-sql-race.bats: races two concurrent `make all` invocations against the same versioned-SQL target across 5 iterations, asserting the file is never doubled (issue #19) - check-stale-expected.bats: orphaned-file failure, the pg_regress alternate-output-file (_N.out) false-positive exemption plus a case proving it doesn't over-exempt, and the test/build/expected pair (issue #14) - base-mk-fixes.bats: EXTRA_CLEAN targets the real test/results/ dir, PGXN_REMOTE default and override for tag/rmtag, and a double-include Makefile confirming base.mk's new guard is warning-free (issues #7, #50, #53) Issue #28 (variable rename) deliberately has no new test -- pure mechanical rename, no behavior change, already covered by the existing suite's DATA/build-output coverage. Co-authored-by: Claude Sonnet 5 --- test/standard/base-mk-fixes.bats | 111 ++++++++++++++++++++++++ test/standard/check-stale-expected.bats | 107 +++++++++++++++++++++++ test/standard/versioned-sql-race.bats | 84 ++++++++++++++++++ 3 files changed, 302 insertions(+) create mode 100644 test/standard/base-mk-fixes.bats create mode 100644 test/standard/check-stale-expected.bats create mode 100644 test/standard/versioned-sql-race.bats diff --git a/test/standard/base-mk-fixes.bats b/test/standard/base-mk-fixes.bats new file mode 100644 index 0000000..e62def5 --- /dev/null +++ b/test/standard/base-mk-fixes.bats @@ -0,0 +1,111 @@ +#!/usr/bin/env bats + +# Test: assorted base.mk fixes (issues #7, #50, #53) +# +# These three fixes are grouped in one file because each is a small, +# self-contained check against base.mk mechanics with no PostgreSQL +# dependency: EXTRA_CLEAN's target directory, the PGXN_REMOTE override, and +# the base.mk double-include guard. + +load ../lib/helpers + +setup_file() { + setup_topdir + + load_test_env "base-mk-fixes" + ensure_foundation "$TEST_DIR" +} + +setup() { + load_test_env "base-mk-fixes" + cd_test_env +} + +# ============================================================================ +# #7 - EXTRA_CLEAN must target the real $(TESTOUT)/results/ directory +# ============================================================================ +# +# PGXS's pg_regress_clean_files unconditionally rm -rf's a top-level results/, +# but pg_regress actually writes to $(TESTOUT)/results/ (test/results/ by +# default, via --outputdir in REGRESS_OPTS). EXTRA_CLEAN used to list a +# nonexistent top-level results/ instead of the real directory, so `make +# clean` never removed actual test output. + +@test "EXTRA_CLEAN lists the real TESTOUT/results directory" { + run make print-EXTRA_CLEAN + assert_success + assert_contains "$output" "test/results/" +} + +@test "make clean removes the real test/results output directory" { + mkdir -p test/results + touch test/results/dummy.out + assert_dir_exists "test/results" + + run make clean + assert_success + assert_dir_not_exists "test/results" +} + +# ============================================================================ +# #53 - PGXN_REMOTE override for tag/rmtag/forcetag/dist +# ============================================================================ +# +# tag/rmtag hardcoded the "origin" remote, so a maintainer whose origin is a +# personal fork would silently re-tag the wrong repo. PGXN_REMOTE ?= origin +# preserves the default while allowing an override. + +@test "PGXN_REMOTE defaults to origin for tag and rmtag" { + run make -n tag 2>&1 + assert_success + assert_contains "$output" "git push origin" + + run make -n rmtag 2>&1 + assert_success + assert_contains "$output" "git fetch origin" +} + +@test "PGXN_REMOTE overrides the remote used by tag and rmtag" { + run make -n tag PGXN_REMOTE=upstream 2>&1 + assert_success + assert_contains "$output" "git push upstream" + assert_not_contains "$output" "git push origin" + + run make -n rmtag PGXN_REMOTE=upstream 2>&1 + assert_success + assert_contains "$output" "git fetch upstream" + assert_not_contains "$output" "git fetch origin" +} + +# ============================================================================ +# #50 - base.mk include guard +# ============================================================================ +# +# base.mk can end up included twice in one `make` run: an extension's own +# .mk module includes it, and the extension's Makefile includes it directly +# too. Without a guard, every target gets redefined, producing +# overriding-recipe/ignoring-old-recipe warnings at parse time. This is +# reproduced here by writing a throwaway Makefile that includes +# pgxntool/base.mk directly AND via an intermediate module -- mirroring the +# real-world scenario from the issue -- and confirming make parses it +# without those warnings. (Confirmed by hand that removing the ifndef/endif +# guard reproduces dozens of these warnings for this exact setup.) + +@test "base.mk tolerates being included twice in one make run" { + cat > double-include-module.mk <<'EOF' +include pgxntool/base.mk +EOF + cat > double-include-test.mk <<'EOF' +include pgxntool/base.mk +include double-include-module.mk +EOF + + run make -f double-include-test.mk -n all 2>&1 + assert_success + assert_not_contains "$output" "overriding recipe" + assert_not_contains "$output" "ignoring old recipe" + + rm -f double-include-module.mk double-include-test.mk +} + +# vi: expandtab sw=2 ts=2 diff --git a/test/standard/check-stale-expected.bats b/test/standard/check-stale-expected.bats new file mode 100644 index 0000000..ef70b76 --- /dev/null +++ b/test/standard/check-stale-expected.bats @@ -0,0 +1,107 @@ +#!/usr/bin/env bats + +# Test: check-stale-expected (issue #14) +# +# `make test` never caught a stale test/expected/*.out left behind after a +# test/sql/*.sql file was renamed or removed. check-stale-expected fails +# loudly instead, and is wired into TEST_DEPS so `make test` runs it first +# (before install/installcheck, so no PostgreSQL is needed to observe the +# failure). It checks two directory pairs: test/sql <-> test/expected, and +# test/build <-> test/build/expected. +# +# It also has to tolerate pg_regress's alternate expected-output files +# (test.out, test_0.out .. test_9.out -- see get_alternative_expectfile() in +# pg_regress.c), which a naive basename comparison would flag as orphaned +# since there's no matching test_1.sql etc. This is not a hypothetical edge +# case: pg_tle's own test suite ships pg_tle_perms_1.out/pg_tle_versions_1.out +# (see /root/git/pg_tle/test/expected/), and pgtap/pglogical/count_nulls use +# the same _N.out convention. This was the exact false positive caught in +# review before this fix landed, and is tested explicitly below. + +load ../lib/helpers + +setup_file() { + setup_topdir + + load_test_env "check-stale-expected" + ensure_foundation "$TEST_DIR" +} + +setup() { + load_test_env "check-stale-expected" + cd_test_env +} + +@test "check-stale-expected passes on clean template state" { + run make check-stale-expected + assert_success +} + +@test "check-stale-expected fails on an orphaned test/expected/*.out" { + touch test/expected/orphan_test.out + + run make check-stale-expected + assert_failure + assert_contains "$output" "orphan_test.out" + assert_contains "$output" "no corresponding" + + rm -f test/expected/orphan_test.out +} + +@test "make test fails fast on a stale expected file, before install/installcheck" { + # check-stale-expected is first in TEST_DEPS, so `make test` must fail here + # without ever needing PostgreSQL -- confirms the wiring, not just the + # standalone target. + touch test/expected/orphan_test.out + + run make test + assert_failure + assert_contains "$output" "orphan_test.out" + + rm -f test/expected/orphan_test.out +} + +@test "check-stale-expected does not false-flag pg_regress alternate files (_N.out)" { + # base.sql exists in the template, so base_1.out must be tolerated as an + # alternate expected file for it, not flagged as orphaned. + touch test/expected/base_1.out + + run make check-stale-expected + assert_success + + rm -f test/expected/base_1.out +} + +@test "check-stale-expected still fails when the alternate file's own base .sql is missing" { + # phantom_1.out has no phantom.sql either -- the _N.out exemption must not + # blanket-exempt every _N.out file, only ones whose stripped base matches + # a real .sql file. + touch test/expected/phantom_1.out + + run make check-stale-expected + assert_failure + assert_contains "$output" "phantom_1.out" + + rm -f test/expected/phantom_1.out +} + +@test "check-stale-expected also checks the test/build <-> test/build/expected pair" { + touch test/build/expected/orphan_build.out + + run make check-stale-expected + assert_failure + assert_contains "$output" "orphan_build.out" + + rm -f test/build/expected/orphan_build.out + + # And tolerates the same _N.out alternate-file convention there too. + # build_check.sql exists in the template. + touch test/build/expected/build_check_1.out + + run make check-stale-expected + assert_success + + rm -f test/build/expected/build_check_1.out +} + +# vi: expandtab sw=2 ts=2 diff --git a/test/standard/versioned-sql-race.bats b/test/standard/versioned-sql-race.bats new file mode 100644 index 0000000..5e56187 --- /dev/null +++ b/test/standard/versioned-sql-race.bats @@ -0,0 +1,84 @@ +#!/usr/bin/env bats + +# Test: Parallel-build safety of versioned SQL file generation (issue #19) +# +# The rule that generates sql/--.sql used to be two recipe +# lines: +# @echo '/* DO NOT EDIT - AUTO-GENERATED FILE */' > $(FILE) +# @cat sql/.sql >> $(FILE) +# Each recipe line is its own forked shell. If the SAME target gets rebuilt +# by two independent `make` processes running concurrently (e.g. two CI jobs +# sharing a checkout, or a developer running one make command while another +# is still in flight), the two processes' lines can interleave so that both +# truncate (line 1) before either appends (line 2) -- doubling the file's +# content. This was observed in practice: a 264-line file was found at 527 +# lines. +# +# The fix collapses the recipe to a single atomic `(...) > $(FILE)` redirect, +# so each firing writes the complete, correct content regardless of how many +# times, or how many concurrent processes, rebuild the target. +# +# This was confirmed by hand against a scratch repo before writing this test: +# racing two concurrent `make all` invocations against the old two-line +# recipe reproduced doubled output (e.g. 31 lines instead of 16) in roughly +# a third of trials; the fixed single-redirect recipe never doubled across +# 15+ trials. The loop below repeats the race to keep that same odds of +# catching a regression. + +load ../lib/helpers + +setup_file() { + setup_topdir + + load_test_env "versioned-sql-race" + ensure_foundation "$TEST_DIR" +} + +setup() { + load_test_env "versioned-sql-race" + cd_test_env +} + +@test "versioned SQL file has correct (non-doubled) content after a normal build" { + rm -f sql/pgxntool-test--0.1.1.sql + + run make all + assert_success + + local expected_lines + expected_lines=$(( $(wc -l < sql/pgxntool-test.sql) + 1 )) + local actual_lines + actual_lines=$(wc -l < sql/pgxntool-test--0.1.1.sql) + + [ "$actual_lines" -eq "$expected_lines" ] || error "Expected $expected_lines lines, got $actual_lines" +} + +@test "concurrent make invocations do not double the versioned SQL file" { + local expected_lines + expected_lines=$(( $(wc -l < sql/pgxntool-test.sql) + 1 )) + + local i + for i in 1 2 3 4 5; do + rm -f sql/pgxntool-test--0.1.1.sql + + # Race two independent `make` processes against the same target, as + # concurrent-make-test.bats does for cross-project races. Close FD 3 so + # BATS doesn't hang on the background children. + ( make all >/dev/null 2>&1 ) 3>&- & + local pid1=$! + ( make all >/dev/null 2>&1 ) 3>&- & + local pid2=$! + + wait $pid1 + wait $pid2 + + [ -f sql/pgxntool-test--0.1.1.sql ] || error "Iteration $i: versioned SQL file missing after concurrent make" + + local actual_lines + actual_lines=$(wc -l < sql/pgxntool-test--0.1.1.sql) + [ "$actual_lines" -eq "$expected_lines" ] || \ + error "Iteration $i: expected $expected_lines lines, got $actual_lines (doubled content would indicate issue #19 regressed)" + done +} + +# vi: expandtab sw=2 ts=2 From 92608da2c3eb3c5cd511308a28e5132dd606e268 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Sun, 26 Jul 2026 14:34:27 -0500 Subject: [PATCH 2/6] Add tests/updates for pgxntool commit 3dee465 (check-stale-expected as a script, ordering fix, variable rename): - check-stale-expected logic moved to check-stale-expected.sh - Now depends on installcheck directly so it runs after pg_regress - EXTENSION_VERSION_FILES renamed to EXTENSION__CURRENT_VERSION__FILES - Replace the old "fails fast, before install/installcheck" test with one that verifies the new ordering via a make -n test dry-run (pg_regress's invocation must appear before check-stale-expected.sh), plus a test proving make test only fails on a stale file after pg_regress has produced real results (test/results/*.out) - Add pgxntool/check-stale-expected.sh to the dist file manifest Co-authored-by: Claude Sonnet 5 --- test/lib/dist-expected-files.txt | 1 + test/standard/check-stale-expected.bats | 38 ++++++++++++++++++++++--- 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/test/lib/dist-expected-files.txt b/test/lib/dist-expected-files.txt index 7f4d5c7..fbf3fc4 100644 --- a/test/lib/dist-expected-files.txt +++ b/test/lib/dist-expected-files.txt @@ -68,6 +68,7 @@ pgxntool/_.gitignore pgxntool/.gitignore pgxntool/base.mk pgxntool/build_meta.sh +pgxntool/check-stale-expected.sh pgxntool/JSON.sh pgxntool/JSON.sh.LICENSE pgxntool/LICENSE diff --git a/test/standard/check-stale-expected.bats b/test/standard/check-stale-expected.bats index ef70b76..8ee9bab 100644 --- a/test/standard/check-stale-expected.bats +++ b/test/standard/check-stale-expected.bats @@ -32,6 +32,26 @@ setup() { cd_test_env } +@test "check-stale-expected depends on installcheck, so it runs after pg_regress, not before" { + # check-stale-expected must run AFTER pg_regress (installcheck), not + # before -- Make only guarantees order via a real dependency edge, not + # position in TEST_DEPS, so this is enforced by `check-stale-expected: + # installcheck` in base.mk. Verify via dry-run (no PostgreSQL needed): + # the real installcheck pg_regress invocation must appear before the + # check-stale-expected.sh call in the recipe order `make test` would run. + run make -n test + assert_success + + local pg_regress_line check_line + pg_regress_line=$(echo "$output" | grep -n "pg_regress " | tail -1 | cut -d: -f1) + check_line=$(echo "$output" | grep -n "check-stale-expected.sh" | head -1 | cut -d: -f1) + + [ -n "$pg_regress_line" ] || error "no pg_regress invocation found in 'make -n test' output" + [ -n "$check_line" ] || error "check-stale-expected.sh invocation not found in 'make -n test' output" + [ "$check_line" -gt "$pg_regress_line" ] || \ + error "check-stale-expected.sh (dry-run line $check_line) must come after pg_regress (line $pg_regress_line)" +} + @test "check-stale-expected passes on clean template state" { run make check-stale-expected assert_success @@ -48,16 +68,26 @@ setup() { rm -f test/expected/orphan_test.out } -@test "make test fails fast on a stale expected file, before install/installcheck" { - # check-stale-expected is first in TEST_DEPS, so `make test` must fail here - # without ever needing PostgreSQL -- confirms the wiring, not just the - # standalone target. +@test "make test fails on a stale expected file, but only after pg_regress has already run" { + # Unlike an early fail-fast design, check-stale-expected now depends on + # installcheck, so pg_regress must have already produced real actual-output + # files by the time make test fails on the stale file. test/results/*.out + # is written for every test regardless of pass/fail (regression.diffs is + # only written when a test actually differs, which the template's own + # passing suite never triggers) -- so its presence is the correct proof + # that pg_regress actually ran, not just that check-stale-expected itself + # failed. touch test/expected/orphan_test.out + rm -rf test/results run make test assert_failure assert_contains "$output" "orphan_test.out" + local out_count + out_count=$(find test/results -name '*.out' 2>/dev/null | wc -l) + [ "$out_count" -gt 0 ] || error "test/results has no .out files -- pg_regress apparently never ran before check-stale-expected failed" + rm -f test/expected/orphan_test.out } From a9c7ce9f828941dd3289a6ea7eb2333313af5e47 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Sun, 26 Jul 2026 15:15:30 -0500 Subject: [PATCH 3/6] Extend check-stale-expected coverage for pgxntool commit 33dfec3 (script move, file-type check, enable toggles) - check-stale-expected.sh moved to test/bin/check-stale-expected.sh - expected/ now also validated for non-*.out files (distinct error/exit code) - PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED and PGXNTOOL_CHECK_EXPECTED_FILE_TYPES toggles added - Update dist-expected-files.txt for the new script path/directory - Add coverage for the non-.out file check (direct script invocation, since make's own exit status masks the child's real code) and for PGXNTOOL_CHECK_EXPECTED_FILE_TYPES=no - Fix the ordering test to exclude test-build's own pg_regress invocation when locating the real installcheck one, since the TEST_DEPS reordering in this commit changed their relative dry-run output order (no ordering relationship was ever guaranteed between them -- only the explicit check-stale-expected: installcheck edge matters) Co-authored-by: Claude Sonnet 5 --- test/lib/dist-expected-files.txt | 3 +- test/standard/check-stale-expected.bats | 46 ++++++++++++++++++++++++- 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/test/lib/dist-expected-files.txt b/test/lib/dist-expected-files.txt index fbf3fc4..6c3909f 100644 --- a/test/lib/dist-expected-files.txt +++ b/test/lib/dist-expected-files.txt @@ -68,7 +68,6 @@ pgxntool/_.gitignore pgxntool/.gitignore pgxntool/base.mk pgxntool/build_meta.sh -pgxntool/check-stale-expected.sh pgxntool/JSON.sh pgxntool/JSON.sh.LICENSE pgxntool/LICENSE @@ -87,6 +86,8 @@ pgxntool/WHAT_IS_THIS # pgxntool test infrastructure pgxntool/test/ +pgxntool/test/bin/ +pgxntool/test/bin/check-stale-expected.sh pgxntool/test/deps.sql pgxntool/test/pgxntool/ pgxntool/test/pgxntool/finish.sql diff --git a/test/standard/check-stale-expected.bats b/test/standard/check-stale-expected.bats index 8ee9bab..844ee18 100644 --- a/test/standard/check-stale-expected.bats +++ b/test/standard/check-stale-expected.bats @@ -17,6 +17,11 @@ # (see /root/git/pg_tle/test/expected/), and pgtap/pglogical/count_nulls use # the same _N.out convention. This was the exact false positive caught in # review before this fix landed, and is tested explicitly below. +# +# It also fails (distinctly, exit code 2 vs 1) if expected/ contains any +# file that isn't *.out -- disable-able on its own via +# PGXNTOOL_CHECK_EXPECTED_FILE_TYPES=no, independent of disabling the +# whole check via PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED=no. load ../lib/helpers @@ -43,7 +48,16 @@ setup() { assert_success local pg_regress_line check_line - pg_regress_line=$(echo "$output" | grep -n "pg_regress " | tail -1 | cut -d: -f1) + # Exclude test-build's own (unrelated) pg_regress invocation, identified + # by its --outputdir=test/build. test-build has no dependency relationship + # with check-stale-expected -- position in TEST_DEPS is not an ordering + # guarantee (see comment above) -- so depending on where test-build lands + # in TEST_DEPS, its recipe can print before or after check-stale-expected's + # in this dry-run, which would make a plain "last pg_regress mention" + # search pick up the wrong invocation. Filtering it out leaves only the + # main suite's pg_regress call, whose ordering relative to + # check-stale-expected IS guaranteed (by the explicit dependency edge). + pg_regress_line=$(echo "$output" | grep -n "pg_regress " | grep -v -- '--outputdir=test/build' | tail -1 | cut -d: -f1) check_line=$(echo "$output" | grep -n "check-stale-expected.sh" | head -1 | cut -d: -f1) [ -n "$pg_regress_line" ] || error "no pg_regress invocation found in 'make -n test' output" @@ -134,4 +148,34 @@ setup() { rm -f test/build/expected/build_check_1.out } +@test "check-stale-expected fails with a distinct message/exit code for a non-.out file in expected/" { + touch test/expected/stray.txt + + # Invoke the script directly rather than through 'make': make's own exit + # status on any recipe failure is always 2 regardless of the underlying + # command's exit code, so verifying the script's own distinct exit codes + # (1 = orphaned .out, 2 = unexpected non-.out file, 3 = both) requires + # calling it directly. + run pgxntool/test/bin/check-stale-expected.sh test + assert_failure_with_status 2 + assert_contains "$output" "unexpected non-.out file" + assert_contains "$output" "stray.txt" + + # 'make check-stale-expected' should still surface the same message. + run make check-stale-expected + assert_failure + assert_contains "$output" "unexpected non-.out file" + + rm -f test/expected/stray.txt +} + +@test "PGXNTOOL_CHECK_EXPECTED_FILE_TYPES=no disables the non-.out file sub-check" { + touch test/expected/stray.txt + + run make check-stale-expected PGXNTOOL_CHECK_EXPECTED_FILE_TYPES=no + assert_success + + rm -f test/expected/stray.txt +} + # vi: expandtab sw=2 ts=2 From 4b2c28160c992d225369aeec751d9c73a1d043bb Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Mon, 27 Jul 2026 12:47:43 -0500 Subject: [PATCH 4/6] Fold two standalone test files into existing suites (issues #14, #19) Per maintainer feedback on this PR: avoid creating new test suites willy-nilly, since the goal is to reduce the amount of state changes needed in tests. - Delete test/standard/versioned-sql-race.bats (issue #19); its two @test blocks (and the explanatory header on the parallel-build race) now live in test/standard/concurrent-make-test.bats, reusing that file's existing single-extension REPO1_ENV instead of building a second dedicated environment. - Delete test/standard/check-stale-expected.bats (issue #14); its nine @test blocks (and the explanatory header) now live in test/standard/make-test.bats, whose existing `ensure_foundation` environment and cwd handling makes check-stale-expected.bats's own setup_file()/setup() redundant. This is also a better thematic home, since check-stale-expected is now literally a `make test` prerequisite. - Add `skip_if_no_postgres` to the moved "make test fails on a stale expected file, but only after pg_regress has already run" test, which runs a real `make test` and was missing the guard already used elsewhere in make-test.bats. Total unique test count is unchanged (224); full suite run clean. Changes only in pgxntool-test. No related changes in pgxntool. Co-Authored-By: Claude --- test/standard/check-stale-expected.bats | 181 ------------------------ test/standard/concurrent-make-test.bats | 79 +++++++++++ test/standard/make-test.bats | 166 ++++++++++++++++++++++ test/standard/versioned-sql-race.bats | 84 ----------- 4 files changed, 245 insertions(+), 265 deletions(-) delete mode 100644 test/standard/check-stale-expected.bats delete mode 100644 test/standard/versioned-sql-race.bats diff --git a/test/standard/check-stale-expected.bats b/test/standard/check-stale-expected.bats deleted file mode 100644 index 844ee18..0000000 --- a/test/standard/check-stale-expected.bats +++ /dev/null @@ -1,181 +0,0 @@ -#!/usr/bin/env bats - -# Test: check-stale-expected (issue #14) -# -# `make test` never caught a stale test/expected/*.out left behind after a -# test/sql/*.sql file was renamed or removed. check-stale-expected fails -# loudly instead, and is wired into TEST_DEPS so `make test` runs it first -# (before install/installcheck, so no PostgreSQL is needed to observe the -# failure). It checks two directory pairs: test/sql <-> test/expected, and -# test/build <-> test/build/expected. -# -# It also has to tolerate pg_regress's alternate expected-output files -# (test.out, test_0.out .. test_9.out -- see get_alternative_expectfile() in -# pg_regress.c), which a naive basename comparison would flag as orphaned -# since there's no matching test_1.sql etc. This is not a hypothetical edge -# case: pg_tle's own test suite ships pg_tle_perms_1.out/pg_tle_versions_1.out -# (see /root/git/pg_tle/test/expected/), and pgtap/pglogical/count_nulls use -# the same _N.out convention. This was the exact false positive caught in -# review before this fix landed, and is tested explicitly below. -# -# It also fails (distinctly, exit code 2 vs 1) if expected/ contains any -# file that isn't *.out -- disable-able on its own via -# PGXNTOOL_CHECK_EXPECTED_FILE_TYPES=no, independent of disabling the -# whole check via PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED=no. - -load ../lib/helpers - -setup_file() { - setup_topdir - - load_test_env "check-stale-expected" - ensure_foundation "$TEST_DIR" -} - -setup() { - load_test_env "check-stale-expected" - cd_test_env -} - -@test "check-stale-expected depends on installcheck, so it runs after pg_regress, not before" { - # check-stale-expected must run AFTER pg_regress (installcheck), not - # before -- Make only guarantees order via a real dependency edge, not - # position in TEST_DEPS, so this is enforced by `check-stale-expected: - # installcheck` in base.mk. Verify via dry-run (no PostgreSQL needed): - # the real installcheck pg_regress invocation must appear before the - # check-stale-expected.sh call in the recipe order `make test` would run. - run make -n test - assert_success - - local pg_regress_line check_line - # Exclude test-build's own (unrelated) pg_regress invocation, identified - # by its --outputdir=test/build. test-build has no dependency relationship - # with check-stale-expected -- position in TEST_DEPS is not an ordering - # guarantee (see comment above) -- so depending on where test-build lands - # in TEST_DEPS, its recipe can print before or after check-stale-expected's - # in this dry-run, which would make a plain "last pg_regress mention" - # search pick up the wrong invocation. Filtering it out leaves only the - # main suite's pg_regress call, whose ordering relative to - # check-stale-expected IS guaranteed (by the explicit dependency edge). - pg_regress_line=$(echo "$output" | grep -n "pg_regress " | grep -v -- '--outputdir=test/build' | tail -1 | cut -d: -f1) - check_line=$(echo "$output" | grep -n "check-stale-expected.sh" | head -1 | cut -d: -f1) - - [ -n "$pg_regress_line" ] || error "no pg_regress invocation found in 'make -n test' output" - [ -n "$check_line" ] || error "check-stale-expected.sh invocation not found in 'make -n test' output" - [ "$check_line" -gt "$pg_regress_line" ] || \ - error "check-stale-expected.sh (dry-run line $check_line) must come after pg_regress (line $pg_regress_line)" -} - -@test "check-stale-expected passes on clean template state" { - run make check-stale-expected - assert_success -} - -@test "check-stale-expected fails on an orphaned test/expected/*.out" { - touch test/expected/orphan_test.out - - run make check-stale-expected - assert_failure - assert_contains "$output" "orphan_test.out" - assert_contains "$output" "no corresponding" - - rm -f test/expected/orphan_test.out -} - -@test "make test fails on a stale expected file, but only after pg_regress has already run" { - # Unlike an early fail-fast design, check-stale-expected now depends on - # installcheck, so pg_regress must have already produced real actual-output - # files by the time make test fails on the stale file. test/results/*.out - # is written for every test regardless of pass/fail (regression.diffs is - # only written when a test actually differs, which the template's own - # passing suite never triggers) -- so its presence is the correct proof - # that pg_regress actually ran, not just that check-stale-expected itself - # failed. - touch test/expected/orphan_test.out - rm -rf test/results - - run make test - assert_failure - assert_contains "$output" "orphan_test.out" - - local out_count - out_count=$(find test/results -name '*.out' 2>/dev/null | wc -l) - [ "$out_count" -gt 0 ] || error "test/results has no .out files -- pg_regress apparently never ran before check-stale-expected failed" - - rm -f test/expected/orphan_test.out -} - -@test "check-stale-expected does not false-flag pg_regress alternate files (_N.out)" { - # base.sql exists in the template, so base_1.out must be tolerated as an - # alternate expected file for it, not flagged as orphaned. - touch test/expected/base_1.out - - run make check-stale-expected - assert_success - - rm -f test/expected/base_1.out -} - -@test "check-stale-expected still fails when the alternate file's own base .sql is missing" { - # phantom_1.out has no phantom.sql either -- the _N.out exemption must not - # blanket-exempt every _N.out file, only ones whose stripped base matches - # a real .sql file. - touch test/expected/phantom_1.out - - run make check-stale-expected - assert_failure - assert_contains "$output" "phantom_1.out" - - rm -f test/expected/phantom_1.out -} - -@test "check-stale-expected also checks the test/build <-> test/build/expected pair" { - touch test/build/expected/orphan_build.out - - run make check-stale-expected - assert_failure - assert_contains "$output" "orphan_build.out" - - rm -f test/build/expected/orphan_build.out - - # And tolerates the same _N.out alternate-file convention there too. - # build_check.sql exists in the template. - touch test/build/expected/build_check_1.out - - run make check-stale-expected - assert_success - - rm -f test/build/expected/build_check_1.out -} - -@test "check-stale-expected fails with a distinct message/exit code for a non-.out file in expected/" { - touch test/expected/stray.txt - - # Invoke the script directly rather than through 'make': make's own exit - # status on any recipe failure is always 2 regardless of the underlying - # command's exit code, so verifying the script's own distinct exit codes - # (1 = orphaned .out, 2 = unexpected non-.out file, 3 = both) requires - # calling it directly. - run pgxntool/test/bin/check-stale-expected.sh test - assert_failure_with_status 2 - assert_contains "$output" "unexpected non-.out file" - assert_contains "$output" "stray.txt" - - # 'make check-stale-expected' should still surface the same message. - run make check-stale-expected - assert_failure - assert_contains "$output" "unexpected non-.out file" - - rm -f test/expected/stray.txt -} - -@test "PGXNTOOL_CHECK_EXPECTED_FILE_TYPES=no disables the non-.out file sub-check" { - touch test/expected/stray.txt - - run make check-stale-expected PGXNTOOL_CHECK_EXPECTED_FILE_TYPES=no - assert_success - - rm -f test/expected/stray.txt -} - -# vi: expandtab sw=2 ts=2 diff --git a/test/standard/concurrent-make-test.bats b/test/standard/concurrent-make-test.bats index 78d885c..149dcf4 100644 --- a/test/standard/concurrent-make-test.bats +++ b/test/standard/concurrent-make-test.bats @@ -122,4 +122,83 @@ setup() { [ $status2 -eq 0 ] || error "multi-extension make test failed" } +# Test: Parallel-build safety of versioned SQL file generation (issue #19) +# +# The rule that generates sql/--.sql used to be two recipe +# lines: +# @echo '/* DO NOT EDIT - AUTO-GENERATED FILE */' > $(FILE) +# @cat sql/.sql >> $(FILE) +# Each recipe line is its own forked shell. If the SAME target gets rebuilt +# by two independent `make` processes running concurrently (e.g. two CI jobs +# sharing a checkout, or a developer running one make command while another +# is still in flight), the two processes' lines can interleave so that both +# truncate (line 1) before either appends (line 2) -- doubling the file's +# content. This was observed in practice: a 264-line file was found at 527 +# lines. +# +# The fix collapses the recipe to a single atomic `(...) > $(FILE)` redirect, +# so each firing writes the complete, correct content regardless of how many +# times, or how many concurrent processes, rebuild the target. +# +# This was confirmed by hand against a scratch repo before writing this test: +# racing two concurrent `make all` invocations against the old two-line +# recipe reproduced doubled output (e.g. 31 lines instead of 16) in roughly +# a third of trials; the fixed single-redirect recipe never doubled across +# 15+ trials. The loop below repeats the race to keep that same odds of +# catching a regression. +# +# Uses the single-extension repo (REPO1_ENV) built above -- the race is about +# a single repo's own concurrent `make` invocations, not the cross-project +# scenario the rest of this file tests. + +@test "versioned SQL file has correct (non-doubled) content after a normal build" { + local repo + repo=$(_repo_path "$REPO1_ENV") + cd "$repo" + + rm -f sql/pgxntool-test--0.1.1.sql + + run make all + assert_success + + local expected_lines + expected_lines=$(( $(wc -l < sql/pgxntool-test.sql) + 1 )) + local actual_lines + actual_lines=$(wc -l < sql/pgxntool-test--0.1.1.sql) + + [ "$actual_lines" -eq "$expected_lines" ] || error "Expected $expected_lines lines, got $actual_lines" +} + +@test "concurrent make invocations do not double the versioned SQL file" { + local repo + repo=$(_repo_path "$REPO1_ENV") + cd "$repo" + + local expected_lines + expected_lines=$(( $(wc -l < sql/pgxntool-test.sql) + 1 )) + + local i + for i in 1 2 3 4 5; do + rm -f sql/pgxntool-test--0.1.1.sql + + # Race two independent `make` processes against the same target, as + # the cross-project race above does. Close FD 3 so BATS doesn't hang + # on the background children. + ( make all >/dev/null 2>&1 ) 3>&- & + local pid1=$! + ( make all >/dev/null 2>&1 ) 3>&- & + local pid2=$! + + wait $pid1 + wait $pid2 + + [ -f sql/pgxntool-test--0.1.1.sql ] || error "Iteration $i: versioned SQL file missing after concurrent make" + + local actual_lines + actual_lines=$(wc -l < sql/pgxntool-test--0.1.1.sql) + [ "$actual_lines" -eq "$expected_lines" ] || \ + error "Iteration $i: expected $expected_lines lines, got $actual_lines (doubled content would indicate issue #19 regressed)" + done +} + # vi: expandtab sw=2 ts=2 diff --git a/test/standard/make-test.bats b/test/standard/make-test.bats index 4743275..71069b5 100755 --- a/test/standard/make-test.bats +++ b/test/standard/make-test.bats @@ -115,4 +115,170 @@ EOF assert_success } +# Test: check-stale-expected (issue #14) +# +# `make test` never caught a stale test/expected/*.out left behind after a +# test/sql/*.sql file was renamed or removed. check-stale-expected fails +# loudly instead, and is wired into TEST_DEPS so `make test` runs it first +# (before install/installcheck, so no PostgreSQL is needed to observe the +# failure). It checks two directory pairs: test/sql <-> test/expected, and +# test/build <-> test/build/expected. +# +# It also has to tolerate pg_regress's alternate expected-output files +# (test.out, test_0.out .. test_9.out -- see get_alternative_expectfile() in +# pg_regress.c), which a naive basename comparison would flag as orphaned +# since there's no matching test_1.sql etc. This is not a hypothetical edge +# case: pg_tle's own test suite ships pg_tle_perms_1.out/pg_tle_versions_1.out +# (see /root/git/pg_tle/test/expected/), and pgtap/pglogical/count_nulls use +# the same _N.out convention. This was the exact false positive caught in +# review before this fix landed, and is tested explicitly below. +# +# It also fails (distinctly, exit code 2 vs 1) if expected/ contains any +# file that isn't *.out -- disable-able on its own via +# PGXNTOOL_CHECK_EXPECTED_FILE_TYPES=no, independent of disabling the +# whole check via PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED=no. + +@test "check-stale-expected depends on installcheck, so it runs after pg_regress, not before" { + # check-stale-expected must run AFTER pg_regress (installcheck), not + # before -- Make only guarantees order via a real dependency edge, not + # position in TEST_DEPS, so this is enforced by `check-stale-expected: + # installcheck` in base.mk. Verify via dry-run (no PostgreSQL needed): + # the real installcheck pg_regress invocation must appear before the + # check-stale-expected.sh call in the recipe order `make test` would run. + run make -n test + assert_success + + local pg_regress_line check_line + # Exclude test-build's own (unrelated) pg_regress invocation, identified + # by its --outputdir=test/build. test-build has no dependency relationship + # with check-stale-expected -- position in TEST_DEPS is not an ordering + # guarantee (see comment above) -- so depending on where test-build lands + # in TEST_DEPS, its recipe can print before or after check-stale-expected's + # in this dry-run, which would make a plain "last pg_regress mention" + # search pick up the wrong invocation. Filtering it out leaves only the + # main suite's pg_regress call, whose ordering relative to + # check-stale-expected IS guaranteed (by the explicit dependency edge). + pg_regress_line=$(echo "$output" | grep -n "pg_regress " | grep -v -- '--outputdir=test/build' | tail -1 | cut -d: -f1) + check_line=$(echo "$output" | grep -n "check-stale-expected.sh" | head -1 | cut -d: -f1) + + [ -n "$pg_regress_line" ] || error "no pg_regress invocation found in 'make -n test' output" + [ -n "$check_line" ] || error "check-stale-expected.sh invocation not found in 'make -n test' output" + [ "$check_line" -gt "$pg_regress_line" ] || \ + error "check-stale-expected.sh (dry-run line $check_line) must come after pg_regress (line $pg_regress_line)" +} + +@test "check-stale-expected passes on clean template state" { + run make check-stale-expected + assert_success +} + +@test "check-stale-expected fails on an orphaned test/expected/*.out" { + touch test/expected/orphan_test.out + + run make check-stale-expected + assert_failure + assert_contains "$output" "orphan_test.out" + assert_contains "$output" "no corresponding" + + rm -f test/expected/orphan_test.out +} + +@test "make test fails on a stale expected file, but only after pg_regress has already run" { + skip_if_no_postgres + + # Unlike an early fail-fast design, check-stale-expected now depends on + # installcheck, so pg_regress must have already produced real actual-output + # files by the time make test fails on the stale file. test/results/*.out + # is written for every test regardless of pass/fail (regression.diffs is + # only written when a test actually differs, which the template's own + # passing suite never triggers) -- so its presence is the correct proof + # that pg_regress actually ran, not just that check-stale-expected itself + # failed. + touch test/expected/orphan_test.out + rm -rf test/results + + run make test + assert_failure + assert_contains "$output" "orphan_test.out" + + local out_count + out_count=$(find test/results -name '*.out' 2>/dev/null | wc -l) + [ "$out_count" -gt 0 ] || error "test/results has no .out files -- pg_regress apparently never ran before check-stale-expected failed" + + rm -f test/expected/orphan_test.out +} + +@test "check-stale-expected does not false-flag pg_regress alternate files (_N.out)" { + # base.sql exists in the template, so base_1.out must be tolerated as an + # alternate expected file for it, not flagged as orphaned. + touch test/expected/base_1.out + + run make check-stale-expected + assert_success + + rm -f test/expected/base_1.out +} + +@test "check-stale-expected still fails when the alternate file's own base .sql is missing" { + # phantom_1.out has no phantom.sql either -- the _N.out exemption must not + # blanket-exempt every _N.out file, only ones whose stripped base matches + # a real .sql file. + touch test/expected/phantom_1.out + + run make check-stale-expected + assert_failure + assert_contains "$output" "phantom_1.out" + + rm -f test/expected/phantom_1.out +} + +@test "check-stale-expected also checks the test/build <-> test/build/expected pair" { + touch test/build/expected/orphan_build.out + + run make check-stale-expected + assert_failure + assert_contains "$output" "orphan_build.out" + + rm -f test/build/expected/orphan_build.out + + # And tolerates the same _N.out alternate-file convention there too. + # build_check.sql exists in the template. + touch test/build/expected/build_check_1.out + + run make check-stale-expected + assert_success + + rm -f test/build/expected/build_check_1.out +} + +@test "check-stale-expected fails with a distinct message/exit code for a non-.out file in expected/" { + touch test/expected/stray.txt + + # Invoke the script directly rather than through 'make': make's own exit + # status on any recipe failure is always 2 regardless of the underlying + # command's exit code, so verifying the script's own distinct exit codes + # (1 = orphaned .out, 2 = unexpected non-.out file, 3 = both) requires + # calling it directly. + run pgxntool/test/bin/check-stale-expected.sh test + assert_failure_with_status 2 + assert_contains "$output" "unexpected non-.out file" + assert_contains "$output" "stray.txt" + + # 'make check-stale-expected' should still surface the same message. + run make check-stale-expected + assert_failure + assert_contains "$output" "unexpected non-.out file" + + rm -f test/expected/stray.txt +} + +@test "PGXNTOOL_CHECK_EXPECTED_FILE_TYPES=no disables the non-.out file sub-check" { + touch test/expected/stray.txt + + run make check-stale-expected PGXNTOOL_CHECK_EXPECTED_FILE_TYPES=no + assert_success + + rm -f test/expected/stray.txt +} + # vi: expandtab sw=2 ts=2 diff --git a/test/standard/versioned-sql-race.bats b/test/standard/versioned-sql-race.bats deleted file mode 100644 index 5e56187..0000000 --- a/test/standard/versioned-sql-race.bats +++ /dev/null @@ -1,84 +0,0 @@ -#!/usr/bin/env bats - -# Test: Parallel-build safety of versioned SQL file generation (issue #19) -# -# The rule that generates sql/--.sql used to be two recipe -# lines: -# @echo '/* DO NOT EDIT - AUTO-GENERATED FILE */' > $(FILE) -# @cat sql/.sql >> $(FILE) -# Each recipe line is its own forked shell. If the SAME target gets rebuilt -# by two independent `make` processes running concurrently (e.g. two CI jobs -# sharing a checkout, or a developer running one make command while another -# is still in flight), the two processes' lines can interleave so that both -# truncate (line 1) before either appends (line 2) -- doubling the file's -# content. This was observed in practice: a 264-line file was found at 527 -# lines. -# -# The fix collapses the recipe to a single atomic `(...) > $(FILE)` redirect, -# so each firing writes the complete, correct content regardless of how many -# times, or how many concurrent processes, rebuild the target. -# -# This was confirmed by hand against a scratch repo before writing this test: -# racing two concurrent `make all` invocations against the old two-line -# recipe reproduced doubled output (e.g. 31 lines instead of 16) in roughly -# a third of trials; the fixed single-redirect recipe never doubled across -# 15+ trials. The loop below repeats the race to keep that same odds of -# catching a regression. - -load ../lib/helpers - -setup_file() { - setup_topdir - - load_test_env "versioned-sql-race" - ensure_foundation "$TEST_DIR" -} - -setup() { - load_test_env "versioned-sql-race" - cd_test_env -} - -@test "versioned SQL file has correct (non-doubled) content after a normal build" { - rm -f sql/pgxntool-test--0.1.1.sql - - run make all - assert_success - - local expected_lines - expected_lines=$(( $(wc -l < sql/pgxntool-test.sql) + 1 )) - local actual_lines - actual_lines=$(wc -l < sql/pgxntool-test--0.1.1.sql) - - [ "$actual_lines" -eq "$expected_lines" ] || error "Expected $expected_lines lines, got $actual_lines" -} - -@test "concurrent make invocations do not double the versioned SQL file" { - local expected_lines - expected_lines=$(( $(wc -l < sql/pgxntool-test.sql) + 1 )) - - local i - for i in 1 2 3 4 5; do - rm -f sql/pgxntool-test--0.1.1.sql - - # Race two independent `make` processes against the same target, as - # concurrent-make-test.bats does for cross-project races. Close FD 3 so - # BATS doesn't hang on the background children. - ( make all >/dev/null 2>&1 ) 3>&- & - local pid1=$! - ( make all >/dev/null 2>&1 ) 3>&- & - local pid2=$! - - wait $pid1 - wait $pid2 - - [ -f sql/pgxntool-test--0.1.1.sql ] || error "Iteration $i: versioned SQL file missing after concurrent make" - - local actual_lines - actual_lines=$(wc -l < sql/pgxntool-test--0.1.1.sql) - [ "$actual_lines" -eq "$expected_lines" ] || \ - error "Iteration $i: expected $expected_lines lines, got $actual_lines (doubled content would indicate issue #19 regressed)" - done -} - -# vi: expandtab sw=2 ts=2 From ae3715f35f73e5ff70896ea590db14b227f9eb65 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Mon, 27 Jul 2026 12:54:17 -0500 Subject: [PATCH 5/6] Split base-mk-fixes.bats by code-under-test, not by bug (#7, #50, #53) Per maintainer feedback on this PR (continuing the fold started in 4b2c281): organize suites by what code they test, not by which bug report motivated the test. A suite of nothing but bug fixes is the anti-pattern being corrected. - EXTRA_CLEAN test(s) (#7 - make clean must remove the real test/results/ directory) move into test/standard/make-test.bats, placed with that file's existing test/output directory lifecycle tests. - PGXN_REMOTE test(s) (#53 - the remote used by tag/rmtag/forcetag/ dist) move into test/standard/dist-clean.bats, which already exercises those targets directly and whose shared foundation environment already configures an "origin" remote. - The base.mk double-include guard test (#50) has no natural existing home, since it tests base.mk's own inclusion mechanics rather than a user-facing feature. It moves into its own file, renamed from base-mk-fixes.bats to base-mk-include-guard.bats to name the tested behavior instead of referencing "fixes" or an issue number. - Delete test/standard/base-mk-fixes.bats. No other file references its old name. Also fix a stale comment in make-test.bats: the check-stale-expected header (carried over verbatim from check-stale-expected.bats in 4b2c281) said it runs "before install/installcheck, so no PostgreSQL is needed to observe the failure." That's no longer true -- an earlier review round on this PR added an explicit `check-stale-expected: installcheck` dependency edge so it runs AFTER pg_regress, which the file's own "runs after pg_regress, not before" test already verifies. The header now matches. Standard-suite @test count is unchanged (128) -- just relocated. Full suite (test-all) passes clean: 233/233. Changes only in pgxntool-test. No related changes in pgxntool. Co-Authored-By: Claude Sonnet 5 --- test/standard/base-mk-fixes.bats | 111 ----------------------- test/standard/base-mk-include-guard.bats | 46 ++++++++++ test/standard/dist-clean.bats | 32 +++++++ test/standard/make-test.bats | 35 ++++++- 4 files changed, 109 insertions(+), 115 deletions(-) delete mode 100644 test/standard/base-mk-fixes.bats create mode 100755 test/standard/base-mk-include-guard.bats diff --git a/test/standard/base-mk-fixes.bats b/test/standard/base-mk-fixes.bats deleted file mode 100644 index e62def5..0000000 --- a/test/standard/base-mk-fixes.bats +++ /dev/null @@ -1,111 +0,0 @@ -#!/usr/bin/env bats - -# Test: assorted base.mk fixes (issues #7, #50, #53) -# -# These three fixes are grouped in one file because each is a small, -# self-contained check against base.mk mechanics with no PostgreSQL -# dependency: EXTRA_CLEAN's target directory, the PGXN_REMOTE override, and -# the base.mk double-include guard. - -load ../lib/helpers - -setup_file() { - setup_topdir - - load_test_env "base-mk-fixes" - ensure_foundation "$TEST_DIR" -} - -setup() { - load_test_env "base-mk-fixes" - cd_test_env -} - -# ============================================================================ -# #7 - EXTRA_CLEAN must target the real $(TESTOUT)/results/ directory -# ============================================================================ -# -# PGXS's pg_regress_clean_files unconditionally rm -rf's a top-level results/, -# but pg_regress actually writes to $(TESTOUT)/results/ (test/results/ by -# default, via --outputdir in REGRESS_OPTS). EXTRA_CLEAN used to list a -# nonexistent top-level results/ instead of the real directory, so `make -# clean` never removed actual test output. - -@test "EXTRA_CLEAN lists the real TESTOUT/results directory" { - run make print-EXTRA_CLEAN - assert_success - assert_contains "$output" "test/results/" -} - -@test "make clean removes the real test/results output directory" { - mkdir -p test/results - touch test/results/dummy.out - assert_dir_exists "test/results" - - run make clean - assert_success - assert_dir_not_exists "test/results" -} - -# ============================================================================ -# #53 - PGXN_REMOTE override for tag/rmtag/forcetag/dist -# ============================================================================ -# -# tag/rmtag hardcoded the "origin" remote, so a maintainer whose origin is a -# personal fork would silently re-tag the wrong repo. PGXN_REMOTE ?= origin -# preserves the default while allowing an override. - -@test "PGXN_REMOTE defaults to origin for tag and rmtag" { - run make -n tag 2>&1 - assert_success - assert_contains "$output" "git push origin" - - run make -n rmtag 2>&1 - assert_success - assert_contains "$output" "git fetch origin" -} - -@test "PGXN_REMOTE overrides the remote used by tag and rmtag" { - run make -n tag PGXN_REMOTE=upstream 2>&1 - assert_success - assert_contains "$output" "git push upstream" - assert_not_contains "$output" "git push origin" - - run make -n rmtag PGXN_REMOTE=upstream 2>&1 - assert_success - assert_contains "$output" "git fetch upstream" - assert_not_contains "$output" "git fetch origin" -} - -# ============================================================================ -# #50 - base.mk include guard -# ============================================================================ -# -# base.mk can end up included twice in one `make` run: an extension's own -# .mk module includes it, and the extension's Makefile includes it directly -# too. Without a guard, every target gets redefined, producing -# overriding-recipe/ignoring-old-recipe warnings at parse time. This is -# reproduced here by writing a throwaway Makefile that includes -# pgxntool/base.mk directly AND via an intermediate module -- mirroring the -# real-world scenario from the issue -- and confirming make parses it -# without those warnings. (Confirmed by hand that removing the ifndef/endif -# guard reproduces dozens of these warnings for this exact setup.) - -@test "base.mk tolerates being included twice in one make run" { - cat > double-include-module.mk <<'EOF' -include pgxntool/base.mk -EOF - cat > double-include-test.mk <<'EOF' -include pgxntool/base.mk -include double-include-module.mk -EOF - - run make -f double-include-test.mk -n all 2>&1 - assert_success - assert_not_contains "$output" "overriding recipe" - assert_not_contains "$output" "ignoring old recipe" - - rm -f double-include-module.mk double-include-test.mk -} - -# vi: expandtab sw=2 ts=2 diff --git a/test/standard/base-mk-include-guard.bats b/test/standard/base-mk-include-guard.bats new file mode 100755 index 0000000..e74166b --- /dev/null +++ b/test/standard/base-mk-include-guard.bats @@ -0,0 +1,46 @@ +#!/usr/bin/env bats + +# Test: base.mk double-inclusion safety (issue #50) +# +# base.mk can end up included twice in one `make` run: an extension's own +# .mk module includes it, and the extension's Makefile includes it directly +# too. Without a guard, every target gets redefined, producing +# overriding-recipe/ignoring-old-recipe warnings at parse time. This is +# reproduced here by writing a throwaway Makefile that includes +# pgxntool/base.mk directly AND via an intermediate module -- mirroring the +# real-world scenario from the issue -- and confirming make parses it +# without those warnings. (Confirmed by hand that removing the ifndef/endif +# guard reproduces dozens of these warnings for this exact setup.) + +load ../lib/helpers + +setup_file() { + setup_topdir + + load_test_env "base-mk-include-guard" + ensure_foundation "$TEST_DIR" +} + +setup() { + load_test_env "base-mk-include-guard" + cd_test_env +} + +@test "base.mk tolerates being included twice in one make run" { + cat > double-include-module.mk <<'EOF' +include pgxntool/base.mk +EOF + cat > double-include-test.mk <<'EOF' +include pgxntool/base.mk +include double-include-module.mk +EOF + + run make -f double-include-test.mk -n all 2>&1 + assert_success + assert_not_contains "$output" "overriding recipe" + assert_not_contains "$output" "ignoring old recipe" + + rm -f double-include-module.mk double-include-test.mk +} + +# vi: expandtab sw=2 ts=2 diff --git a/test/standard/dist-clean.bats b/test/standard/dist-clean.bats index ffd39ed..3af3217 100644 --- a/test/standard/dist-clean.bats +++ b/test/standard/dist-clean.bats @@ -132,4 +132,36 @@ setup() { echo "$files" | grep -q "doc/.*\.asc" } +# ============================================================================ +# PGXN_REMOTE override for tag/rmtag/forcetag/dist (issue #53) +# ============================================================================ +# +# tag/rmtag hardcoded the "origin" remote, so a maintainer whose origin is a +# personal fork would silently re-tag the wrong repo. PGXN_REMOTE ?= origin +# preserves the default while allowing an override. Verified via `make -n` +# dry-runs against this file's own foundation environment, which already has +# an "origin" remote configured (see build_test_repo_from_template). + +@test "PGXN_REMOTE defaults to origin for tag and rmtag" { + run make -n tag 2>&1 + assert_success + assert_contains "$output" "git push origin" + + run make -n rmtag 2>&1 + assert_success + assert_contains "$output" "git fetch origin" +} + +@test "PGXN_REMOTE overrides the remote used by tag and rmtag" { + run make -n tag PGXN_REMOTE=upstream 2>&1 + assert_success + assert_contains "$output" "git push upstream" + assert_not_contains "$output" "git push origin" + + run make -n rmtag PGXN_REMOTE=upstream 2>&1 + assert_success + assert_contains "$output" "git fetch upstream" + assert_not_contains "$output" "git fetch origin" +} + # vi: expandtab sw=2 ts=2 diff --git a/test/standard/make-test.bats b/test/standard/make-test.bats index 71069b5..30023fa 100755 --- a/test/standard/make-test.bats +++ b/test/standard/make-test.bats @@ -80,6 +80,32 @@ setup() { assert_success } +# ============================================================================ +# EXTRA_CLEAN must target the real $(TESTOUT)/results/ directory (issue #7) +# ============================================================================ +# +# PGXS's pg_regress_clean_files unconditionally rm -rf's a top-level results/, +# but pg_regress actually writes to $(TESTOUT)/results/ (test/results/ by +# default, via --outputdir in REGRESS_OPTS). EXTRA_CLEAN used to list a +# nonexistent top-level results/ instead of the real directory, so `make +# clean` never removed actual test output. + +@test "EXTRA_CLEAN lists the real TESTOUT/results directory" { + run make print-EXTRA_CLEAN + assert_success + assert_contains "$output" "test/results/" +} + +@test "make clean removes the real test/results output directory" { + mkdir -p test/results + touch test/results/dummy.out + assert_dir_exists "test/results" + + run make clean + assert_success + assert_dir_not_exists "test/results" +} + # Unique database name tests # # Verify that make test uses a unique database name based on the project name @@ -119,10 +145,11 @@ EOF # # `make test` never caught a stale test/expected/*.out left behind after a # test/sql/*.sql file was renamed or removed. check-stale-expected fails -# loudly instead, and is wired into TEST_DEPS so `make test` runs it first -# (before install/installcheck, so no PostgreSQL is needed to observe the -# failure). It checks two directory pairs: test/sql <-> test/expected, and -# test/build <-> test/build/expected. +# loudly instead. It runs AFTER install/installcheck (pg_regress), via an +# explicit `check-stale-expected: installcheck` dependency edge in base.mk, +# not as an early fail-fast check -- see the "runs after pg_regress, not +# before" test below. It checks two directory pairs: test/sql <-> +# test/expected, and test/build <-> test/build/expected. # # It also has to tolerate pg_regress's alternate expected-output files # (test.out, test_0.out .. test_9.out -- see get_alternative_expectfile() in From 48961fa4be5360a84e2f4be3a053de081734011a Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Mon, 27 Jul 2026 16:16:39 -0500 Subject: [PATCH 6/6] Add stub-based check-stale-expected.sh invocation test, split script-logic tests Add tests/updates for pgxntool commit 0fe9dfc (pass PGXNTOOL_CHECK_EXPECTED_FILE_TYPES to check-stale-expected.sh as an arg instead of an env var; add PGXNTOOL_CHECK_STALE_EXPECTED_SCRIPT so tests can stub out just the script path without faking the whole PGXNTOOL_DIR tree): - check-stale-expected.sh now takes the file-type check toggle as a second positional argument (default yes) rather than reading it from the environment; base.mk passes the make variable through as that arg. - New PGXNTOOL_CHECK_STALE_EXPECTED_SCRIPT variable names exactly which script the check-stale-expected recipe invokes (default $(PGXNTOOL_DIR)/test/bin/check-stale-expected.sh). - Add helpers.bash:make_stub_script, a general-purpose helper that writes a throwaway stub script (caller-chosen filename under BATS_TEST_TMPDIR, configurable exit code, stdout text, marker file) to prove a script was/wasn't invoked, or that its exit status/output was correctly propagated -- not specific to check-stale-expected.sh. Writes directly to BATS_TEST_TMPDIR/ rather than mktemp, since bats-core already gives each test a fresh, unique tmpdir. - Split check-stale-expected's pure script-logic tests (orphan detection, the pg_regress _N.out alternate-file convention, the build/expected pair, the file-type sub-check, and argument validation) out of make-test.bats into check-stale-expected-script.bats, invoking the script directly against a bare scratch directory -- no foundation, make, or Postgres needed for those. - make-test.bats now owns only Makefile-integration concerns and no longer duplicates script decision logic: pg_regress ordering (dry-run), that the recipe passes the right positional args (dry-run), that PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED=no genuinely skips invoking the script (stub-based), that `make` correctly propagates a stub's exit status/output either way (stub-based, replacing two tests that duplicated the real script's decision logic), and one real end-to-end `make test` failure on a stale file. - Same layering fix applied to the EXTRA_CLEAN/PGXS `clean` test: replaced a real mkdir+touch+`make clean`+assert-deleted test (re-verifying PGXS's own established `clean`/EXTRA_CLEAN deletion mechanics) with a `make -n clean` dry-run check that test/results/ actually appears in the real clean recipe -- proving pgxntool's EXTRA_CLEAN entry percolates into the recipe without needing to invoke or verify PGXS's own mechanics. - Fix assert_contains/assert_not_contains in test/lib/assertions.bash to show the haystack/needle on failure instead of a bare exit status, matching the diagnostic quality assert_success/assert_failure already meet (gap found while researching bats-assert as prior art for the stubbing/layering work above; see pgxntool-test#44 for that research). - Generalize CLAUDE.md's layered-testing principle beyond the check-stale-expected script/Makefile split: don't re-test an underlying framework's own established behavior (PGXS's clean/EXTRA_CLEAN mechanics is a second concrete instance) -- only test that pgxntool's own code correctly wires into/configures it. Trimmed the inline "Layering" essay in make-test.bats to a one-line pointer at this CLAUDE.md section, and led the ordering test's comment with a plain-language summary line. Co-Authored-By: Claude --- CLAUDE.md | 26 +++ test/lib/assertions.bash | 19 +- test/lib/helpers.bash | 42 +++++ .../standard/check-stale-expected-script.bats | 148 +++++++++++++++ test/standard/make-test.bats | 174 ++++++++---------- 5 files changed, 308 insertions(+), 101 deletions(-) create mode 100644 test/standard/check-stale-expected-script.bats diff --git a/CLAUDE.md b/CLAUDE.md index 6539512..02c82bf 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -183,6 +183,32 @@ default_version = '2.5.0' # DO NOT REMOVE: trailing comment exercises parse_cont **Combine simple sanity checks into a single `@test`**: BATS has per-test overhead (setup, teardown, process spawning). Avoid creating a dedicated `@test` for a single simple assertion. Instead, add the check inline into an existing related `@test`, or consolidate multiple simple one-liner checks into a single `@test "...: simple sanity checks"` block. Reserve dedicated `@test` blocks for checks that need their own setup/teardown or that test meaningfully distinct behaviors. +### Test Each Layer for What It Actually Owns, Not What Another Layer Already Covers + +**General rule**: don't re-test behavior that's already established and owned somewhere else -- an underlying framework/dependency's own documented mechanics, or a different layer of pgxntool's own code. Only test that *pgxntool's own code* correctly wires into, configures, or triggers that other layer. This applies whether the "other layer" is a script pgxntool invokes, or an external build framework (PGXS) pgxntool builds on top of. + +- **A script's own decision logic** (parsing, comparisons, what makes it succeed or fail) belongs in a test file that invokes the script directly -- no Make, no foundation environment, no Postgres. Exhaustive edge-case coverage belongs here, since it's cheap here. +- **An external framework's own established behavior** (e.g. PGXS's `clean`/`EXTRA_CLEAN` deletion mechanics) is that framework's responsibility, not pgxntool's to re-verify. What pgxntool IS responsible for is correctly *populating* the configuration PGXS consumes (e.g. `EXTRA_CLEAN` containing the right entry) and that the configuration genuinely *reaches* the mechanism it's supposed to configure (e.g. that entry actually appearing in the real `clean` recipe, not just in an isolated variable dump) -- not that PGXS then does the right thing with it, which is PGXS's own well-established behavior. +- **The Makefile's own responsibility**, in either case, is wiring: does the recipe invoke the right script/target with the right arguments/variables, does it run at the correct point relative to other targets, does disabling a feature actually skip invoking it, and does `make` correctly propagate the invoked script's exit status and output. None of this requires re-running the invoked code's real decision logic -- it should be verified with a stub standing in for the real script, a `make -n` dry-run, or both, so the Makefile-layer tests would pass or fail identically no matter what the real invoked code's internals did. + +Concrete example 1 -- `check-stale-expected` (script: `../pgxntool/test/bin/check-stale-expected.sh`; recipe: `../pgxntool/base.mk`): +- `test/standard/check-stale-expected-script.bats` invokes the script directly against a bare scratch directory and owns all of the decision-logic coverage: orphan detection, the pg_regress `_N.out` alternate-file convention, the `test/build`/`test/build/expected` pair, the file-type sub-check, and argument validation. +- `test/standard/make-test.bats` owns only the Makefile-integration concerns: the `check-stale-expected: installcheck` ordering guarantee (via `make -n` dry-run), that the recipe passes the right positional args (also via dry-run), that `PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED=no` genuinely skips invoking the script (via a stub -- see below), that `make` correctly propagates a stub's exit status/output either way, and one real end-to-end `make test` failure on a stale file (needs actual pg_regress output to prove the ordering held in practice). It does **not** re-test orphan/alternate-file/file-type decision logic -- that would just be the same assertion running twice, once cheaply and once expensively. + +Concrete example 2 -- `EXTRA_CLEAN`/PGXS (`test/standard/make-test.bats`): `make clean` actually deleting `test/results/` is PGXS's own `pg_regress_clean_files`/`EXTRA_CLEAN` mechanism at work -- established, external behavior pgxntool doesn't need to re-verify by creating a real directory, running `make clean`, and checking it's gone. What pgxntool owns is (a) `EXTRA_CLEAN` listing the right entry (`make print-EXTRA_CLEAN`) and (b) that entry actually percolating into the real `clean` recipe's command line (`make -n clean` dry-run) -- proving pgxntool's own wiring is correct without needing to invoke or verify PGXS's deletion mechanics at all. + +#### Proving a Script Was/Wasn't Invoked (or That Its Result Was Propagated Correctly) + +When a Makefile-layer test needs to prove a script genuinely was (or wasn't) called, or that its exit status/output was correctly bubbled up -- not just that a failure from it was tolerated, which is a materially weaker claim -- prefer a dedicated "which script path to run" make variable over faking out an entire directory tree, and stub the script rather than relying on its real behavior. + +Concrete example: the `check-stale-expected` recipe in `../pgxntool/base.mk` invokes `$(PGXNTOOL_CHECK_STALE_EXPECTED_SCRIPT)` (default `$(PGXNTOOL_DIR)/test/bin/check-stale-expected.sh`) instead of a hardcoded path. `make_stub_script` in `test/lib/helpers.bash` writes a throwaway stub (configurable exit code, stdout text, marker file) to prove either that the script was never invoked (`PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED=no` test) or that `make` correctly propagates the script's exit status/output (the dedicated propagation test) -- both in `test/standard/make-test.bats`, both pointing `PGXNTOOL_CHECK_STALE_EXPECTED_SCRIPT` at the stub instead of touching `PGXNTOOL_DIR`. + +This is deliberately narrower than overriding `PGXNTOOL_DIR` itself. `PGXNTOOL_DIR` is read by many unrelated targets (`META.json`, `control.mk`, `verify-results`, `pgtle.sh`, etc.), so faking it out wholesale to intercept one script would require a full copy of the real directory just to swap that one file -- expensive, and it defeats the purpose of the narrower variable. Reach for "add a dedicated variable, then stub just that seam" before reaching for a directory-level fake or an in-place edit of a live, shared checkout (e.g. temporarily `mv`-ing the real script aside). + +**Why not a BATS-ecosystem mocking library (e.g. [bats-mock](https://github.com/grayhemp/bats-mock) and its forks)?** That library's actual mechanism is PATH-shadowing: it symlinks a wrapper (`binstub`) into a directory prepended to `PATH`, so any invocation of the command *by bare name* resolves to the wrapper instead, which records the call and compares it against an expected "stub plan" (`stub`/`unstub`). That's a solid, general convention -- but it only intercepts commands resolved via `PATH` lookup. `base.mk` invokes `check-stale-expected.sh` by an explicit path (`$(PGXNTOOL_DIR)/test/bin/check-stale-expected.sh`), which bypasses `PATH` entirely, so PATH-shadowing would not have intercepted this call without a larger change to how the recipe invokes the script. A dedicated "which path to invoke" make variable is the idiom that actually fits this codebase's invocation style -- and it isn't a one-off improvisation: `PG_CONFIG` and `ASCIIDOC` (see `../pgxntool/base.mk`) already establish the same "overridable path/binary variable" pattern for other externally-invoked tools, for the same reason. `make_stub_script` itself borrows bats-mock's underlying idea (a throwaway executable with configurable exit code/output, standing in for the real one) without adding a dependency, and is written generally enough to reuse for any future script invocation that's parameterized the same way -- it isn't specific to check-stale-expected.sh. + +When adding a new script invocation that later tests might need to intercept, consider referencing it through its own overridable make variable from the start rather than a hardcoded path -- retrofitting testability later is more expensive than designing for it up front. + ## File Structure ``` diff --git a/test/lib/assertions.bash b/test/lib/assertions.bash index d2696ab..39ebc65 100644 --- a/test/lib/assertions.bash +++ b/test/lib/assertions.bash @@ -139,16 +139,31 @@ assert_repo_clean() { # String Assertions +# Assert that haystack contains needle as a literal substring +# Usage: assert_contains "$output" "expected substring" +# Shows the haystack and needle on failure, matching the diagnostic quality +# of assert_success/assert_failure above (bare exit status alone gives no +# clue what was actually being compared). assert_contains() { local haystack=$1 local needle=$2 - echo "$haystack" | grep -qF "$needle" + if ! echo "$haystack" | grep -qF "$needle"; then + out "Expected to find: $needle" + out "In: $haystack" + error "assert_contains failed (see above)" + fi } +# Assert that haystack does NOT contain needle as a literal substring +# Usage: assert_not_contains "$output" "unexpected substring" assert_not_contains() { local haystack=$1 local needle=$2 - ! echo "$haystack" | grep -qF "$needle" + if echo "$haystack" | grep -qF "$needle"; then + out "Expected NOT to find: $needle" + out "In: $haystack" + error "assert_not_contains failed (see above)" + fi } # Semantic Validators diff --git a/test/lib/helpers.bash b/test/lib/helpers.bash index 7983fc3..0700ed2 100644 --- a/test/lib/helpers.bash +++ b/test/lib/helpers.bash @@ -896,6 +896,48 @@ ensure_foundation() { debug 3 "ensure_foundation: Foundation copied successfully" } +# make_stub_script [stdout_text] [marker_file] +# +# Writes a standalone stub script to "$BATS_TEST_TMPDIR/": prints +# [stdout_text] (if given) to stdout, touches [marker_file] (if given), then +# exits with . Echoes the stub's path on stdout. +# +# BATS_TEST_TMPDIR is already a fresh, unique directory per test (bats-core +# creates it automatically), so writing directly to a caller-chosen filename +# there is enough -- no mktemp needed for uniqueness. +# +# General-purpose building block for "swap in a fake script and assert it +# was/wasn't called, or that its exit status/output was correctly +# propagated" -- pair it with whatever variable names the real script's +# path in the code under test (e.g. PGXNTOOL_CHECK_STALE_EXPECTED_SCRIPT for +# check-stale-expected.sh). See "Proving a Script Was/Wasn't Invoked" in +# CLAUDE.md for the reasoning, including why this is a make-variable +# override rather than the PATH-shadowing technique BATS-ecosystem mocking +# libraries (e.g. bats-mock) use: those intercept a bare command name +# resolved via PATH, which doesn't apply here since base.mk invokes the +# script by an explicit path, not a bare name. +# +# Examples: +# stub=$(make_stub_script invoked-stub 1 "" "$marker") # "was it called" +# stub=$(make_stub_script fail-stub 5 "some message") # exit-status/output propagation +make_stub_script() { + local filename="$1" + local exit_code="${2:-0}" + local stdout_text="${3:-}" + local marker_file="${4:-}" + local stub="$BATS_TEST_TMPDIR/$filename" + + { + echo '#!/usr/bin/env bash' + [ -n "$stdout_text" ] && printf 'echo %q\n' "$stdout_text" + [ -n "$marker_file" ] && printf 'touch %q\n' "$marker_file" + printf 'exit %q\n' "$exit_code" + } > "$stub" + chmod +x "$stub" + + echo "$stub" +} + # ============================================================================ # PostgreSQL Availability Detection # ============================================================================ diff --git a/test/standard/check-stale-expected-script.bats b/test/standard/check-stale-expected-script.bats new file mode 100644 index 0000000..7a2f45a --- /dev/null +++ b/test/standard/check-stale-expected-script.bats @@ -0,0 +1,148 @@ +#!/usr/bin/env bats + +# Test: check-stale-expected.sh - pure script-logic unit tests +# +# These tests exercise test/bin/check-stale-expected.sh directly against a +# bare scratch directory -- no foundation environment, no `make`, no +# PostgreSQL. They cover the script's own file-comparison logic: orphaned +# .out detection, the pg_regress _N.out alternate-file convention (see +# get_alternative_expectfile() in pg_regress.c), the test/build <-> +# test/build/expected pair, the non-.out file-type sub-check (now the +# script's second positional argument -- see check-stale-expected.sh's own +# Usage comment), and basic argument validation. +# +# Tests that need real Make/PostgreSQL integration (ordering relative to +# pg_regress, the PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED=no target-skipping +# behavior, and a real `make test` failure on a stale file) stay in +# make-test.bats. + +load ../lib/helpers +load ../lib/assertions + +setup_file() { + setup_topdir + load_test_env "check-stale-expected-script" +} + +setup() { + load_test_env "check-stale-expected-script" + export SCRIPT="$PGXNREPO/test/bin/check-stale-expected.sh" + + # Fresh, empty scratch directory per test -- no foundation/TEST_REPO needed. + export TESTDIR="$BATS_TEST_TMPDIR/testdir" + mkdir -p "$TESTDIR/sql" "$TESTDIR/expected" +} + +@test "check-stale-expected.sh: passes when expected/ exactly mirrors sql/" { + touch "$TESTDIR/sql/foo.sql" + touch "$TESTDIR/expected/foo.out" + + run "$SCRIPT" "$TESTDIR" + assert_success +} + +@test "check-stale-expected.sh: fails on an orphaned expected/*.out with no corresponding sql/*.sql" { + touch "$TESTDIR/sql/foo.sql" + touch "$TESTDIR/expected/foo.out" + touch "$TESTDIR/expected/orphan.out" + + run "$SCRIPT" "$TESTDIR" + assert_failure_with_status 1 + assert_contains "$output" "orphan.out" + assert_contains "$output" "no corresponding" +} + +@test "check-stale-expected.sh: tolerates a pg_regress alternate _N.out file when the base .sql exists" { + touch "$TESTDIR/sql/foo.sql" + touch "$TESTDIR/expected/foo.out" + touch "$TESTDIR/expected/foo_1.out" + + run "$SCRIPT" "$TESTDIR" + assert_success +} + +@test "check-stale-expected.sh: still fails when an alternate _N.out's own base .sql is missing" { + # phantom_1.out has no phantom.sql -- the _N.out exemption must not + # blanket-exempt every _N.out file, only ones whose stripped base matches + # a real .sql file. + touch "$TESTDIR/expected/phantom_1.out" + + run "$SCRIPT" "$TESTDIR" + assert_failure_with_status 1 + assert_contains "$output" "phantom_1.out" +} + +@test "check-stale-expected.sh: also checks the build <-> build/expected pair" { + mkdir -p "$TESTDIR/build/expected" + touch "$TESTDIR/build/build_check.sql" + touch "$TESTDIR/build/expected/build_check.out" + touch "$TESTDIR/build/expected/orphan_build.out" + + run "$SCRIPT" "$TESTDIR" + assert_failure_with_status 1 + assert_contains "$output" "orphan_build.out" +} + +@test "check-stale-expected.sh: build <-> build/expected pair also tolerates the _N.out convention" { + mkdir -p "$TESTDIR/build/expected" + touch "$TESTDIR/build/build_check.sql" + touch "$TESTDIR/build/expected/build_check.out" + touch "$TESTDIR/build/expected/build_check_1.out" + + run "$SCRIPT" "$TESTDIR" + assert_success +} + +@test "check-stale-expected.sh: non-.out file in expected/ fails distinctly, defaulting to check enabled" { + touch "$TESTDIR/sql/foo.sql" + touch "$TESTDIR/expected/foo.out" + touch "$TESTDIR/expected/stray.txt" + + run "$SCRIPT" "$TESTDIR" + assert_failure_with_status 2 + assert_contains "$output" "unexpected non-.out file" + assert_contains "$output" "stray.txt" +} + +@test "check-stale-expected.sh: check-file-types=yes explicitly enables the non-.out file check" { + touch "$TESTDIR/sql/foo.sql" + touch "$TESTDIR/expected/foo.out" + touch "$TESTDIR/expected/stray.txt" + + run "$SCRIPT" "$TESTDIR" yes + assert_failure_with_status 2 + assert_contains "$output" "unexpected non-.out file" +} + +@test "check-stale-expected.sh: check-file-types=no disables the non-.out file check" { + touch "$TESTDIR/sql/foo.sql" + touch "$TESTDIR/expected/foo.out" + touch "$TESTDIR/expected/stray.txt" + + run "$SCRIPT" "$TESTDIR" no + assert_success +} + +@test "check-stale-expected.sh: exit code is a bitmask of both failure classes" { + touch "$TESTDIR/expected/orphan.out" + touch "$TESTDIR/expected/stray.txt" + + run "$SCRIPT" "$TESTDIR" + assert_failure_with_status 3 + assert_contains "$output" "orphan.out" + assert_contains "$output" "stray.txt" +} + +@test "check-stale-expected.sh: usage error when called with no testdir argument" { + run "$SCRIPT" + assert_failure_with_status 1 + assert_contains "$output" "Usage:" +} + +@test "check-stale-expected.sh: usage error when called with more than two arguments" { + run "$SCRIPT" "$TESTDIR" yes extra + assert_failure_with_status 1 + assert_contains "$output" "Usage:" +} + +# vi: expandtab ts=2 sw=2 diff --git a/test/standard/make-test.bats b/test/standard/make-test.bats index 30023fa..c0077c5 100755 --- a/test/standard/make-test.bats +++ b/test/standard/make-test.bats @@ -96,14 +96,17 @@ setup() { assert_contains "$output" "test/results/" } -@test "make clean removes the real test/results output directory" { - mkdir -p test/results - touch test/results/dummy.out - assert_dir_exists "test/results" - - run make clean +@test "EXTRA_CLEAN's test/results/ entry actually percolates into the clean recipe" { + # Layering (see CLAUDE.md): whether `rm -rf` actually deletes a directory + # is PGXS's own established `clean`/EXTRA_CLEAN mechanism, not pgxntool's + # to re-verify. What pgxntool IS responsible for is correctly populating + # EXTRA_CLEAN (covered above) AND that entry genuinely reaching the real + # `clean` recipe -- in case something upstream never wires EXTRA_CLEAN into + # `clean`'s dependency graph at all. Verify via dry-run, so this doesn't + # need to invoke or verify PGXS's own deletion mechanics. + run make -n clean assert_success - assert_dir_not_exists "test/results" + assert_contains "$output" "test/results/" } # Unique database name tests @@ -148,30 +151,22 @@ EOF # loudly instead. It runs AFTER install/installcheck (pg_regress), via an # explicit `check-stale-expected: installcheck` dependency edge in base.mk, # not as an early fail-fast check -- see the "runs after pg_regress, not -# before" test below. It checks two directory pairs: test/sql <-> -# test/expected, and test/build <-> test/build/expected. -# -# It also has to tolerate pg_regress's alternate expected-output files -# (test.out, test_0.out .. test_9.out -- see get_alternative_expectfile() in -# pg_regress.c), which a naive basename comparison would flag as orphaned -# since there's no matching test_1.sql etc. This is not a hypothetical edge -# case: pg_tle's own test suite ships pg_tle_perms_1.out/pg_tle_versions_1.out -# (see /root/git/pg_tle/test/expected/), and pgtap/pglogical/count_nulls use -# the same _N.out convention. This was the exact false positive caught in -# review before this fix landed, and is tested explicitly below. +# before" test below. # -# It also fails (distinctly, exit code 2 vs 1) if expected/ contains any -# file that isn't *.out -- disable-able on its own via -# PGXNTOOL_CHECK_EXPECTED_FILE_TYPES=no, independent of disabling the -# whole check via PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED=no. +# See also: CLAUDE.md's testing-layering section, and +# check-stale-expected-script.bats for the script's own decision-logic tests +# (not duplicated here). @test "check-stale-expected depends on installcheck, so it runs after pg_regress, not before" { + # Capture dry-run make output and ensure that pg_regress is called before + # check-stale-expected.sh. + # # check-stale-expected must run AFTER pg_regress (installcheck), not # before -- Make only guarantees order via a real dependency edge, not # position in TEST_DEPS, so this is enforced by `check-stale-expected: - # installcheck` in base.mk. Verify via dry-run (no PostgreSQL needed): - # the real installcheck pg_regress invocation must appear before the - # check-stale-expected.sh call in the recipe order `make test` would run. + # installcheck` in base.mk. No PostgreSQL needed for this: the recipe + # order in `make -n test` output is enough to prove the dependency edge + # is real. run make -n test assert_success @@ -195,19 +190,51 @@ EOF } @test "check-stale-expected passes on clean template state" { + # Not a decision-logic test (no orphan/alternate-file scenario is + # crafted) -- this is an end-to-end smoke check that the real template + # stays in the passing state the Template Requirements section of + # CLAUDE.md requires, exercised through the real recipe and real script. run make check-stale-expected assert_success } -@test "check-stale-expected fails on an orphaned test/expected/*.out" { - touch test/expected/orphan_test.out +@test "check-stale-expected recipe invokes the script with TESTDIR and the file-types arg" { + # base.mk's responsibility, not the script's: does the recipe actually + # pass the right positional arguments? Verified via dry-run (no + # Postgres/script execution needed) for both the default value and an + # explicit override, so this only exercises the make plumbing. + run make -n check-stale-expected + assert_success + assert_contains "$output" "test/bin/check-stale-expected.sh test yes" - run make check-stale-expected - assert_failure - assert_contains "$output" "orphan_test.out" - assert_contains "$output" "no corresponding" + run make -n check-stale-expected PGXNTOOL_CHECK_EXPECTED_FILE_TYPES=no + assert_success + assert_contains "$output" "test/bin/check-stale-expected.sh test no" +} - rm -f test/expected/orphan_test.out +@test "PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED=no: make test never invokes check-stale-expected.sh" { + skip_if_no_postgres + + # Disabling via this variable drops the check-stale-expected target from + # TEST_DEPS (and its own definition) entirely -- see base.mk -- so the + # script must never even be invoked, not merely have a failure from it + # ignored. That's a materially stronger claim than "make test succeeds + # despite a stale file", so prove it directly: point + # PGXNTOOL_CHECK_STALE_EXPECTED_SCRIPT -- the one variable the + # check-stale-expected recipe actually invokes (see base.mk) -- at a stub + # that only touches a marker file and fails. No need to fake out + # PGXNTOOL_DIR itself, since this variable is the sole thing standing + # between the target and the real script. If the marker never appears, + # the script was genuinely never called. This is base.mk's + # target-skipping behavior, not the script's decision logic, so a stub + # (not the real script) is exactly what should stand in here. + local marker="$BATS_TEST_TMPDIR/check-stale-expected-invoked" + local stub_script + stub_script=$(make_stub_script check-stale-expected-stub 1 "" "$marker") + + run make test PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED=no PGXNTOOL_CHECK_STALE_EXPECTED_SCRIPT="$stub_script" + assert_success + assert_file_not_exists "$marker" } @test "make test fails on a stale expected file, but only after pg_regress has already run" { @@ -235,77 +262,26 @@ EOF rm -f test/expected/orphan_test.out } -@test "check-stale-expected does not false-flag pg_regress alternate files (_N.out)" { - # base.sql exists in the template, so base_1.out must be tolerated as an - # alternate expected file for it, not flagged as orphaned. - touch test/expected/base_1.out - - run make check-stale-expected - assert_success - - rm -f test/expected/base_1.out -} - -@test "check-stale-expected still fails when the alternate file's own base .sql is missing" { - # phantom_1.out has no phantom.sql either -- the _N.out exemption must not - # blanket-exempt every _N.out file, only ones whose stripped base matches - # a real .sql file. - touch test/expected/phantom_1.out - - run make check-stale-expected +@test "make correctly propagates check-stale-expected.sh's exit status and output" { + # base.mk's responsibility, not the script's decision logic (the real + # script's distinct exit codes and messages are already covered directly + # in check-stale-expected-script.bats): does `make check-stale-expected` + # correctly surface whatever PGXNTOOL_CHECK_STALE_EXPECTED_SCRIPT does? A + # stub that deterministically prints a message and exits nonzero must + # make the target (and `make`'s own recipe-failure handling) fail and + # show that message; a stub that exits 0 must let it pass -- regardless + # of what the real script would have decided for the same directory. + local stub_script + stub_script=$(make_stub_script fail-stub 5 "STUB SENTINEL MESSAGE") + + run make check-stale-expected PGXNTOOL_CHECK_STALE_EXPECTED_SCRIPT="$stub_script" assert_failure - assert_contains "$output" "phantom_1.out" + assert_contains "$output" "STUB SENTINEL MESSAGE" - rm -f test/expected/phantom_1.out -} - -@test "check-stale-expected also checks the test/build <-> test/build/expected pair" { - touch test/build/expected/orphan_build.out - - run make check-stale-expected - assert_failure - assert_contains "$output" "orphan_build.out" - - rm -f test/build/expected/orphan_build.out + stub_script=$(make_stub_script pass-stub 0) - # And tolerates the same _N.out alternate-file convention there too. - # build_check.sql exists in the template. - touch test/build/expected/build_check_1.out - - run make check-stale-expected + run make check-stale-expected PGXNTOOL_CHECK_STALE_EXPECTED_SCRIPT="$stub_script" assert_success - - rm -f test/build/expected/build_check_1.out -} - -@test "check-stale-expected fails with a distinct message/exit code for a non-.out file in expected/" { - touch test/expected/stray.txt - - # Invoke the script directly rather than through 'make': make's own exit - # status on any recipe failure is always 2 regardless of the underlying - # command's exit code, so verifying the script's own distinct exit codes - # (1 = orphaned .out, 2 = unexpected non-.out file, 3 = both) requires - # calling it directly. - run pgxntool/test/bin/check-stale-expected.sh test - assert_failure_with_status 2 - assert_contains "$output" "unexpected non-.out file" - assert_contains "$output" "stray.txt" - - # 'make check-stale-expected' should still surface the same message. - run make check-stale-expected - assert_failure - assert_contains "$output" "unexpected non-.out file" - - rm -f test/expected/stray.txt -} - -@test "PGXNTOOL_CHECK_EXPECTED_FILE_TYPES=no disables the non-.out file sub-check" { - touch test/expected/stray.txt - - run make check-stale-expected PGXNTOOL_CHECK_EXPECTED_FILE_TYPES=no - assert_success - - rm -f test/expected/stray.txt } # vi: expandtab sw=2 ts=2