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/dist-expected-files.txt b/test/lib/dist-expected-files.txt index 7f4d5c7..6c3909f 100644 --- a/test/lib/dist-expected-files.txt +++ b/test/lib/dist-expected-files.txt @@ -86,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/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/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/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/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/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 4743275..c0077c5 100755 --- a/test/standard/make-test.bats +++ b/test/standard/make-test.bats @@ -80,6 +80,35 @@ 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 "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_contains "$output" "test/results/" +} + # Unique database name tests # # Verify that make test uses a unique database name based on the project name @@ -115,4 +144,144 @@ 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. 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. +# +# 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. 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 + + 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" { + # 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 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 -n check-stale-expected PGXNTOOL_CHECK_EXPECTED_FILE_TYPES=no + assert_success + assert_contains "$output" "test/bin/check-stale-expected.sh test no" +} + +@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" { + 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 "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" "STUB SENTINEL MESSAGE" + + stub_script=$(make_stub_script pass-stub 0) + + run make check-stale-expected PGXNTOOL_CHECK_STALE_EXPECTED_SCRIPT="$stub_script" + assert_success +} + # vi: expandtab sw=2 ts=2