From 37c0af90cb343772b5044806b6837894f4e4d053 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Mon, 12 Oct 2026 18:23:40 -0500 Subject: [PATCH 1/2] run-tests.sh: fail on non-zero exit, zero tests, or TAP plan mismatch (#95) The command's exit status is now captured in the parent shell, and STATUS is FAIL when the command exited non-zero, no tests ran, or the TAP plans declare more tests than were reported. A REASON line explains the failure. The Makefile gains a test/bats/bin/bats target that initializes the submodule; every target that runs bats (and run-tests.sh's direct .bats path) depends on it. Co-Authored-By: Claude Sonnet 5.5 --- .claude/skills/test/SKILL.md | 2 +- .claude/skills/test/scripts/run-tests.sh | 45 ++++--- Makefile | 17 ++- README.md | 2 +- test/standard/run-tests-script.bats | 148 +++++++++++++++++++++++ 5 files changed, 192 insertions(+), 22 deletions(-) create mode 100644 test/standard/run-tests-script.bats diff --git a/.claude/skills/test/SKILL.md b/.claude/skills/test/SKILL.md index bc5437c..a2d4c55 100644 --- a/.claude/skills/test/SKILL.md +++ b/.claude/skills/test/SKILL.md @@ -45,7 +45,7 @@ When the background task completes, read the summary from the task output. **CRITICAL RULES:** -1. If STATUS is `FAIL`: **STOP immediately**. Report failures to user. Do not proceed with any other work. +1. If STATUS is `FAIL`: **STOP immediately**. Report failures to user. Do not proceed with any other work. `FAIL` also covers a non-zero command exit, zero tests run, and a TAP plan/count mismatch, even with no `not ok` lines; the `REASON:` line after `STATUS:` says which applied. 2. If STATUS is `PASS_WITH_SKIPS`: **STOP and report**. Skips are test problems that need investigation, not things to dismiss. 3. Only STATUS `PASS` with zero skips is acceptable. 4. **Never rationalize** failures or skips as "pre-existing", "expected", or "unrelated". diff --git a/.claude/skills/test/scripts/run-tests.sh b/.claude/skills/test/scripts/run-tests.sh index 13dd90d..c0a6ee7 100755 --- a/.claude/skills/test/scripts/run-tests.sh +++ b/.claude/skills/test/scripts/run-tests.sh @@ -75,10 +75,15 @@ SKIPPED=0 CURRENT_SUITE="" EXIT_CODE=0 -# Run tests, capturing output and parsing TAP in real-time -{ - $CMD 2>&1 || EXIT_CODE=$? -} | tee "$FULL_LOG" | while IFS= read -r line; do +# A direct .bats run needs the submodule; let make initialize it (output goes to the full log). +if [[ "$TARGET" == *.bats ]]; then + CMD="make test/bats/bin/bats && $CMD" +fi + +# Run tests, capturing output and parsing TAP in real-time. The while loop runs in a +# subshell, so the command's status is taken from PIPESTATUS in the parent. Under pipefail +# a zero there means tee or the read loop failed, which still counts as a failure. +bash -c "$CMD" 2>&1 | tee "$FULL_LOG" | while IFS= read -r line; do # Detect new suite (TAP plan line: 1..N) if [[ "$line" =~ ^1\.\. ]]; then SUITES=$((SUITES + 1)) @@ -118,7 +123,7 @@ CURRENT_SUITE: $CURRENT_SUITE LAST: $line INNER_EOF fi -done +done || { EXIT_CODE=${PIPESTATUS[0]}; [ "$EXIT_CODE" -ne 0 ] || EXIT_CODE=1; } # The pipe subshell loses counter values, so re-parse from the full log SUITES=0 @@ -126,10 +131,14 @@ TOTAL=0 PASSED=0 FAILED=0 SKIPPED=0 +PLAN_TOTAL=0 while IFS= read -r line; do if [[ "$line" =~ ^1\.\. ]]; then SUITES=$((SUITES + 1)) + if [[ "$line" =~ ^1\.\.([0-9]+)$ ]]; then + PLAN_TOTAL=$((PLAN_TOTAL + BASH_REMATCH[1])) + fi elif [[ "$line" =~ ^ok[[:space:]] ]]; then TOTAL=$((TOTAL + 1)) if [[ "$line" =~ \#[[:space:]]*skip ]]; then @@ -143,11 +152,6 @@ while IFS= read -r line; do fi done < "$FULL_LOG" -# Determine exit code from the log if pipe lost it -if [ "$FAILED" -gt 0 ] && [ "$EXIT_CODE" -eq 0 ]; then - EXIT_CODE=1 -fi - # Extract failures with diagnostics > "$ERRORS_LOG" { @@ -168,19 +172,31 @@ fi > "$SKIPS_LOG" grep -E '^ok .* # skip' "$FULL_LOG" >> "$SKIPS_LOG" 2>/dev/null || true -# Determine overall status -if [ "$FAILED" -gt 0 ]; then +# Determine overall status. FAIL is checked first and covers a run that executed nothing +# (or fewer tests than the TAP plans declared), whatever the command's exit code was. +REASONS=() +[ "$EXIT_CODE" -ne 0 ] && REASONS+=("command exited $EXIT_CODE") +[ "$TOTAL" -eq 0 ] && REASONS+=("no tests ran") +[ "$PLAN_TOTAL" -ne "$TOTAL" ] && REASONS+=("TAP plan declared $PLAN_TOTAL tests, $TOTAL reported") +[ "$FAILED" -gt 0 ] && REASONS+=("$FAILED test(s) failed") +if [ "${#REASONS[@]}" -gt 0 ]; then STATUS="FAIL" + REASON_LINE="REASON: $(IFS=';'; echo "${REASONS[*]}" | sed 's/;/; /g')" + [ "$EXIT_CODE" -ne 0 ] || EXIT_CODE=1 elif [ "$SKIPPED" -gt 0 ]; then STATUS="PASS_WITH_SKIPS" + REASON_LINE="" else STATUS="PASS" + REASON_LINE="" fi # Build failure section -FAILURE_LINES="" if [ -s "$ERRORS_LOG" ]; then FAILURE_LINES=$(cat "$ERRORS_LOG") +elif [ "$STATUS" = "FAIL" ]; then + FAILURE_LINES=" (no TAP failures parsed; last 20 lines of full log:) +$(tail -n 20 "$FULL_LOG" | sed 's/^/ /')" else FAILURE_LINES=" (none)" fi @@ -197,7 +213,8 @@ fi SUMMARY=$(cat </dev/null) +# Initialize the BATS submodule on demand (a fresh clone or worktree has it empty). +# Every target that runs test/bats/bin/bats depends on this file. +test/bats/bin/bats: + git submodule update --init --recursive test/bats + # Build fresh foundation environment (clean + create) # Foundation is the base TEST_REPO that all tests depend on # See test/lib/foundation.bats for detailed explanation of why foundation.bats # is both a test and a library .PHONY: foundation -foundation: clean-envs +foundation: clean-envs test/bats/bin/bats @test/bats/bin/bats test/lib/foundation.bats # Test recursion and pollution detection @@ -17,7 +22,7 @@ foundation: clean-envs # as a prerequisite. This validates that recursion and pollution detection work correctly. # Note: Doesn't matter which independent test we use, we just pick the fastest one (doc). .PHONY: test-recursion -test-recursion: clean-envs +test-recursion: clean-envs test/bats/bin/bats @echo "Testing recursion with clean environment..." @test/bats/bin/bats test/standard/doc.bats @@ -42,7 +47,7 @@ ALL_INDEPENDENT_TESTS := $(STANDARD_TESTS) $(EXTRA_TESTS) # first with a clean environment, we exercise these systems before running the full suite. # If recursion is broken, we want to know immediately, not after running all tests. .PHONY: test-setup -test-setup: +test-setup: test/bats/bin/bats ifneq ($(GIT_DIRTY),) @echo "Git repo is dirty (uncommitted changes detected)" @echo "Running recursion test first to validate test infrastructure..." @@ -61,7 +66,7 @@ endif # If we only ran the last test, prerequisite tests would run but their results # wouldn't appear in the output. .PHONY: test -test: +test: test/bats/bin/bats @echo @echo "Tip: Use 'make test-extra' for additional tests, or 'make test-all' for everything" @echo @@ -75,7 +80,7 @@ test: # These tests have additional requirements (like multiple pg_tle versions installed) # Use test-all to run standard tests plus extra tests together .PHONY: test-extra -test-extra: +test-extra: test/bats/bin/bats ifneq ($(EXTRA_TESTS),) @echo @echo "Tip: Use 'make test-all' to run both standard and extra tests" @@ -92,7 +97,7 @@ endif # Run regular test suite PLUS extra/optional tests # This passes all test files to bats in a single invocation for proper TAP output .PHONY: test-all -test-all: +test-all: test/bats/bin/bats @$(MAKE) test-setup @test/bats/bin/bats $(SEQUENTIAL_TESTS) $(STANDARD_TESTS) $(EXTRA_TESTS) diff --git a/README.md b/README.md index 8755875..c0caf2b 100644 --- a/README.md +++ b/README.md @@ -20,7 +20,7 @@ The tests use relative paths to access pgxntool, so maintaining this structure i - rsync - asciidoctor (for documentation tests) -BATS (Bash Automated Testing System) is included as a git submodule at `test/bats/`. +BATS (Bash Automated Testing System) is included as a git submodule at `test/bats/`; `make` initializes it on first use. ### PostgreSQL Configuration diff --git a/test/standard/run-tests-script.bats b/test/standard/run-tests-script.bats new file mode 100644 index 0000000..0ca64a7 --- /dev/null +++ b/test/standard/run-tests-script.bats @@ -0,0 +1,148 @@ +#!/usr/bin/env bats + +# Test: .claude/skills/test/scripts/run-tests.sh - STATUS/REASON/exit-code logic +# +# This covers the repo's own dev tooling (the /test skill's runner and the +# Makefile's submodule target), not pgxntool. The runner is invoked directly +# from a scratch cwd (its log dir and lock are keyed on pwd) with a stub `make` +# first on PATH, so no foundation environment, real Make, or PostgreSQL is needed. + +load ../lib/helpers +load ../lib/assertions + +setup_file() { + setup_topdir +} + +setup() { + export RUN_TESTS="$TOPDIR/.claude/skills/test/scripts/run-tests.sh" + export WORK="$BATS_TEST_TMPDIR/work" + mkdir -p "$WORK" + cd "$WORK" + # Otherwise the runner shells out to pg_lsclusters to derive PGPORT + unset PGCLUSTER +} + +teardown() { + # The runner keeps its log dir and lock under /tmp, not under BATS_TEST_TMPDIR + rm -rf "/tmp/pgxntool-test-logs$(cd "$WORK" && pwd | sed 's/\//_/g')" +} + +# use_stub [stdout_text] -- shadow on PATH +use_stub() { + local name="$1" + shift + mkdir -p "$BATS_TEST_TMPDIR/bin" + make_stub_script "bin/$name" "$@" > /dev/null + export PATH="$BATS_TEST_TMPDIR/bin:$PATH" +} + +@test "run-tests.sh: non-zero exit with no TAP output fails with the command's own code (#95)" { + use_stub make 2 "boom" + + run "$RUN_TESTS" test-all + assert_failure_with_status 2 + assert_contains "$output" "STATUS: FAIL" + assert_contains "$output" "REASON: command exited 2; no tests ran" + assert_contains "$output" "EXIT_CODE: 2" + assert_contains "$output" "(no TAP failures parsed; last 20 lines of full log:)" + assert_contains "$output" " boom" +} + +@test "run-tests.sh: exit 0 with no tests fails and exits 1" { + use_stub make 0 + + run "$RUN_TESTS" test-all + assert_failure_with_status 1 + assert_contains "$output" "STATUS: FAIL" + assert_contains "$output" "REASON: no tests ran" + assert_contains "$output" "EXIT_CODE: 1" +} + +@test "run-tests.sh: non-zero exit with only ok lines fails and passes the code through" { + use_stub make 7 $'1..2\nok 1 a\nok 2 b' + + run "$RUN_TESTS" test-all + assert_failure_with_status 7 + assert_contains "$output" "STATUS: FAIL" + assert_contains "$output" "REASON: command exited 7" + assert_contains "$output" "EXIT_CODE: 7" +} + +@test "run-tests.sh: not ok lines fail the run and are listed under FAILURES" { + use_stub make 0 $'1..2\nok 1 a\nnot ok 2 b' + + run "$RUN_TESTS" test-all + assert_failure_with_status 1 + assert_contains "$output" "STATUS: FAIL" + assert_contains "$output" "REASON: 1 test(s) failed" + assert_contains "$output" "not ok 2 b" +} + +@test "run-tests.sh: TAP plan/count mismatch fails" { + use_stub make 0 $'1..3\nok 1 a\nok 2 b' + + run "$RUN_TESTS" test-all + assert_failure_with_status 1 + assert_contains "$output" "STATUS: FAIL" + assert_contains "$output" "REASON: TAP plan declared 3 tests, 2 reported" +} + +@test "run-tests.sh: PASS and PASS_WITH_SKIPS exit 0 with no REASON line" { + use_stub make 0 $'1..2\nok 1 a\nok 2 b # skip why' + + run "$RUN_TESTS" test-all + assert_success + assert_contains "$output" "STATUS: PASS_WITH_SKIPS" + assert_not_contains "$output" "REASON:" + + use_stub make 0 $'1..2\nok 1 a\nok 2 b' + + run "$RUN_TESTS" test-all + assert_success + assert_contains "$output" "STATUS: PASS" + assert_not_contains "$output" "STATUS: PASS_WITH_SKIPS" + assert_not_contains "$output" "REASON:" +} + +@test "run-tests.sh: .bats path runs 'make test/bats/bin/bats' first and reports either failure" { + # Stub make fails: bats must not run and make's code is passed through + use_stub make 4 "submodule trouble" + mkdir -p "$WORK/test/bats/bin" + make_stub_script "work/test/bats/bin/bats" 0 $'1..1\nok 1 a' "$WORK/bats-ran" > /dev/null + + run "$RUN_TESTS" some.bats + assert_failure_with_status 4 + assert_contains "$output" "REASON: command exited 4; no tests ran" + assert_contains "$output" "submodule trouble" + [ ! -e "$WORK/bats-ran" ] + + # Stub make succeeds: bats exiting non-zero without TAP fails with its code + use_stub make 0 + make_stub_script "work/test/bats/bin/bats" 3 > /dev/null + + run "$RUN_TESTS" some.bats + assert_failure_with_status 3 + assert_contains "$output" "STATUS: FAIL" + assert_contains "$output" "REASON: command exited 3; no tests ran" +} + +@test "run-tests.sh: real make error for an unknown target fails" { + run "$RUN_TESTS" no-such-target + assert_failure_with_status 2 + assert_contains "$output" "STATUS: FAIL" +} + +@test "Makefile: every target that calls test/bats/bin/bats depends on the submodule-init target" { + cd "$TOPDIR" + + # -B with -n shows the recipe without running it; -p dumps the rule database. + run make -n -B -p test/bats/bin/bats + assert_success + assert_contains "$output" "git submodule update --init --recursive test/bats" + + local target + for target in foundation test-recursion test-setup test test-extra test-all; do + grep -Eq "^$target:.* test/bats/bin/bats( |$)" <<< "$output" + done +} From 11cae600f753c3890d5b849537acbca45cc0ec9f Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Mon, 12 Oct 2026 18:37:09 -0500 Subject: [PATCH 2/2] run-tests.sh: pass arguments literally instead of through bash -c The command is built as an argv array and run directly, so a bats filter such as 'foo|wc -l' stays one argument. For direct .bats runs, 'make test/bats/bin/bats' runs as its own step first. Also reads a final unterminated log line, derives the Makefile-wiring test's target list from make's rule database, and tightens two test assertions. Co-Authored-By: Claude Sonnet 5.5 --- .claude/skills/test/scripts/run-tests.sh | 28 ++++++++++++---------- test/standard/run-tests-script.bats | 30 ++++++++++++++++++++---- 2 files changed, 42 insertions(+), 16 deletions(-) diff --git a/.claude/skills/test/scripts/run-tests.sh b/.claude/skills/test/scripts/run-tests.sh index c0a6ee7..3994ace 100755 --- a/.claude/skills/test/scripts/run-tests.sh +++ b/.claude/skills/test/scripts/run-tests.sh @@ -46,15 +46,14 @@ STATUS_FILE="$LOG_DIR/status" # Determine what to run TARGET="${1:-test-all}" shift 2>/dev/null || true -EXTRA_ARGS="$*" -# Build the command +# Build the argv; arguments are passed through literally, never re-parsed by a shell. if [[ "$TARGET" == *.bats ]]; then # Specific bats file(s) - CMD="test/bats/bin/bats $TARGET $EXTRA_ARGS" + CMD=(test/bats/bin/bats "$TARGET" "$@") else # Make target - CMD="make $TARGET" + CMD=(make "$TARGET") fi # Initialize status file @@ -75,15 +74,20 @@ SKIPPED=0 CURRENT_SUITE="" EXIT_CODE=0 -# A direct .bats run needs the submodule; let make initialize it (output goes to the full log). -if [[ "$TARGET" == *.bats ]]; then - CMD="make test/bats/bin/bats && $CMD" -fi +# A direct .bats run needs the submodule; let make initialize it first (output goes to the +# full log, and a failure there is the run's failure). +run_cmd() { + if [[ "$TARGET" == *.bats ]]; then + make test/bats/bin/bats && "${CMD[@]}" + else + "${CMD[@]}" + fi +} # Run tests, capturing output and parsing TAP in real-time. The while loop runs in a # subshell, so the command's status is taken from PIPESTATUS in the parent. Under pipefail # a zero there means tee or the read loop failed, which still counts as a failure. -bash -c "$CMD" 2>&1 | tee "$FULL_LOG" | while IFS= read -r line; do +run_cmd 2>&1 | tee "$FULL_LOG" | while IFS= read -r line; do # Detect new suite (TAP plan line: 1..N) if [[ "$line" =~ ^1\.\. ]]; then SUITES=$((SUITES + 1)) @@ -133,7 +137,7 @@ FAILED=0 SKIPPED=0 PLAN_TOTAL=0 -while IFS= read -r line; do +while IFS= read -r line || [ -n "$line" ]; do if [[ "$line" =~ ^1\.\. ]]; then SUITES=$((SUITES + 1)) if [[ "$line" =~ ^1\.\.([0-9]+)$ ]]; then @@ -156,7 +160,7 @@ done < "$FULL_LOG" > "$ERRORS_LOG" { IN_FAILURE=false - while IFS= read -r line; do + while IFS= read -r line || [ -n "$line" ]; do if [[ "$line" =~ ^not[[:space:]]ok[[:space:]] ]]; then IN_FAILURE=true echo "$line" >> "$ERRORS_LOG" @@ -173,7 +177,7 @@ done < "$FULL_LOG" grep -E '^ok .* # skip' "$FULL_LOG" >> "$SKIPS_LOG" 2>/dev/null || true # Determine overall status. FAIL is checked first and covers a run that executed nothing -# (or fewer tests than the TAP plans declared), whatever the command's exit code was. +# (or a different number of tests than the TAP plans declared), whatever the command's exit code was. REASONS=() [ "$EXIT_CODE" -ne 0 ] && REASONS+=("command exited $EXIT_CODE") [ "$TOTAL" -eq 0 ] && REASONS+=("no tests ran") diff --git a/test/standard/run-tests-script.bats b/test/standard/run-tests-script.bats index 0ca64a7..2ad5cea 100644 --- a/test/standard/run-tests-script.bats +++ b/test/standard/run-tests-script.bats @@ -115,7 +115,7 @@ use_stub() { assert_failure_with_status 4 assert_contains "$output" "REASON: command exited 4; no tests ran" assert_contains "$output" "submodule trouble" - [ ! -e "$WORK/bats-ran" ] + assert_file_not_exists "$WORK/bats-ran" # Stub make succeeds: bats exiting non-zero without TAP fails with its code use_stub make 0 @@ -127,6 +127,22 @@ use_stub() { assert_contains "$output" "REASON: command exited 3; no tests ran" } +@test "run-tests.sh: extra arguments reach bats literally, not as shell syntax" { + use_stub make 0 + mkdir -p "$WORK/test/bats/bin" + cat > "$WORK/test/bats/bin/bats" <<'EOF' +#!/usr/bin/env bash +printf '%s\n' "$@" > "$BATS_TEST_TMPDIR/bats-argv" +echo "1..1" +echo "ok 1 a" +EOF + chmod +x "$WORK/test/bats/bin/bats" + + run "$RUN_TESTS" some.bats --filter 'foo|wc -l' + assert_success + [ "$(cat "$BATS_TEST_TMPDIR/bats-argv")" = $'some.bats\n--filter\nfoo|wc -l' ] +} + @test "run-tests.sh: real make error for an unknown target fails" { run "$RUN_TESTS" no-such-target assert_failure_with_status 2 @@ -141,8 +157,14 @@ use_stub() { assert_success assert_contains "$output" "git submodule update --init --recursive test/bats" - local target - for target in foundation test-recursion test-setup test test-extra test-all; do - grep -Eq "^$target:.* test/bats/bin/bats( |$)" <<< "$output" + # Targets whose recipe invokes bats, read from the rule database + local targets target + targets=$(awk '/^[A-Za-z0-9_.\/-]+:/ {t=$1; sub(/:$/,"",t)} + /^\t.*test\/bats\/bin\/bats/ && t != "" {print t}' <<< "$output" | sort -u) + [ -n "$targets" ] || error "no target with a bats recipe found in make's rule database" + + for target in $targets; do + grep -Eq "^$target:.* test/bats/bin/bats( |$)" <<< "$output" || + error "$target does not depend on test/bats/bin/bats" done }