Add test coverage for parallel-build corruption, stale-expected check, base.mk fixes

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
 
 ```

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

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

test/lib/helpers.bash

@@ -896,6 +896,48 @@ ensure_foundation() {
   debug 3 "ensure_foundation: Foundation copied successfully"
 }
 
+# make_stub_script <filename> <exit_code> [stdout_text] [marker_file]
+#
+# Writes a standalone stub script to "$BATS_TEST_TMPDIR/<filename>": prints
+# [stdout_text] (if given) to stdout, touches [marker_file] (if given), then
+# exits with <exit_code>. 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
 # ============================================================================

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

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