run-tests.sh (Claude /test skill): fail on non-zero exit, zero tests, or plan mismatch

.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".

.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,10 +74,20 @@ 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 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.
+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))
@@ -118,18 +127,22 @@ 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
 TOTAL=0
 PASSED=0
 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
+      PLAN_TOTAL=$((PLAN_TOTAL + BASH_REMATCH[1]))
+    fi
   elif [[ "$line" =~ ^ok[[:space:]] ]]; then
     TOTAL=$((TOTAL + 1))
     if [[ "$line" =~ \#[[:space:]]*skip ]]; then
@@ -143,16 +156,11 @@ 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"
 {
   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"
@@ -168,19 +176,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 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")
+[ "$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 +217,8 @@ fi
 SUMMARY=$(cat <<EOF
 STATE: DONE
 STATUS: $STATUS
-SUITES: $SUITES  TOTAL: $TOTAL  PASSED: $PASSED  FAILED: $FAILED  SKIPPED: $SKIPPED
+${REASON_LINE:+$REASON_LINE
+}SUITES: $SUITES  TOTAL: $TOTAL  PASSED: $PASSED  FAILED: $FAILED  SKIPPED: $SKIPPED
 EXIT_CODE: $EXIT_CODE
 
 FAILURES:

Makefile

@@ -4,20 +4,25 @@ all: test
 # Capture git status once at Make parse time
 GIT_DIRTY := $(shell git status --porcelain 2>/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
 # Cleans environments then runs one independent test, which auto-runs foundation
 # 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)
 

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
 

test/standard/run-tests-script.bats

@@ -0,0 +1,170 @@
+#!/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 <command> <exit_code> [stdout_text] -- shadow <command> 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"
+  assert_file_not_exists "$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: 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
+  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"
+
+  # 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
+}