diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a4ab100..bebdeef3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -335,6 +335,49 @@ true until the next version shipped. ### Fixed +- `test/harness_selftest.sh` can no longer exit 0 having evaluated nothing (#934). + + Handed a `pg_config` the box does not have, it printed four lines, never reached its + summary, and **exited 0**. Measured on main: + + /usr/local/pg18a/bin/pg_config rc=0 1246 lines checks run: 610 + /usr/local/pgNOPE/bin/pg_config rc=0 4 lines no summary at all + + A caller cannot tell the second from the first. It is not hypothetical: it cost a + whole mutation round, because the control and the mutated arm both reported rc=0 with + zero FAIL lines -- which reads exactly like "the mutation changed nothing", the + conclusion the run existed to test. The log being 4 lines instead of 1246 was the only + thing that gave it away. The default argument is `/usr/local/pg17/bin/pg_config`, which + the audit container does not have, so the wrong invocation is the easy one to make. + + TWO CAUSES, and the second is the one that generalises. `_bindir` was assigned from a + command that had failed, so every later PATH was wrong; and part 010, which is + SOURCED, then took its own skip path and called `exit 0` -- which exits the DRIVER + rather than the part. + + The driver now refuses the argument before sourcing anything, on two predicates + because one is not enough: a `pg_config` can exist and be executable and still answer + nothing, which is the shape that produced the empty `_bindir`. Both of part 010's skip + paths now exit 66, the status `lib.sh` calls `PGC_EXIT_SKIPPED`, paired with the + `SKIPPED (ran no checks)` line the runners already require beside it. + + `test/selftest/430-the-self-test-must-not-report.sh` holds the arms, including a + heredoc-aware sweep requiring that **no** part exits 0 -- closing the class rather + than the two instances. The sweep has to be heredoc-aware because the parts generate + fixture scripts that legitimately end in `exit 0`: it sees 2 sites before this change + and 0 after, where a flat `grep -c 'exit 0'` sees 13 before and 24 after -- the flat + count moves because the new part's own fixtures add ten, for a reason that has nothing + to do with the defect. It sees `exit 0` and a bare `exit`; it cannot decide `exit $?` + or `exit "$rc"`, and each of those is zero in the tree today. + + After: `rc=2` and a named refusal for a missing `pg_config`, for a path that is not a + directory, and for one that answers nothing; `604 passed + 0 failed + 0 unrunnable` + with a real one. + + The part's own comment at line 3 already said why this mattered -- "a quiet skip means + the guard stops being tested that run without anyone noticing" -- four lines above the + first `exit 0`. + - The vacuity guard's PLACEMENT is now a checked property, because a guard in a teardown cannot fail the test it guards (#432). diff --git a/test/harness_selftest.sh b/test/harness_selftest.sh index 7a6e8698..e755ec7c 100755 --- a/test/harness_selftest.sh +++ b/test/harness_selftest.sh @@ -24,7 +24,37 @@ set -uo pipefail PGC_SELFTEST_PG_CONFIG="${1:-/usr/local/pg17/bin/pg_config}" + +# REFUSE A pg_config THIS BOX DOES NOT HAVE, before anything is sourced (#934). +# +# `_bindir` used to be assigned from a command that had failed, and the suite then +# ran on with every PATH wrong. Part 010 took its own skip path, called `exit 0`, +# and because a part is SOURCED that exited the DRIVER -- 4 lines of output, no +# summary, status 0. A caller cannot tell that from the 610-check pass it looks +# like. Measured on main: +# +# /usr/local/pg18a/bin/pg_config rc=0 1246 lines checks run: 610 +# /usr/local/pgNOPE/bin/pg_config rc=0 4 lines no summary at all +# +# The default above is `/usr/local/pg17`, which the audit container does not have, +# so the wrong invocation is the EASY one to make -- and it cost a whole mutation +# round, because the control and the mutation both reported rc=0 with zero FAIL +# lines, which reads exactly like "the mutation changed nothing". +# +# TWO PREDICATES, because one is not enough. A `pg_config` can exist and be +# executable and still answer nothing: that is the shape that produced the empty +# `_bindir`, so `-x` alone would have accepted it. +if [ ! -x "$PGC_SELFTEST_PG_CONFIG" ]; then + echo "FATAL no-pg-config: $PGC_SELFTEST_PG_CONFIG is not an executable pg_config" + echo " pass one as the first argument, e.g. /usr/local/pg18a/bin/pg_config" + exit 2 +fi _bindir="$("$PGC_SELFTEST_PG_CONFIG" --bindir)" +if [ -z "$_bindir" ] || [ ! -d "$_bindir" ]; then + echo "FATAL no-pg-config: $PGC_SELFTEST_PG_CONFIG --bindir gave [$_bindir]," + echo " which is not a directory, so every PATH built from it would be wrong" + exit 2 +fi # ---- the checks themselves live in test/selftest/, one file per subject ------ # diff --git a/test/selftest/010-stand-up-a-squatter-on-a.sh b/test/selftest/010-stand-up-a-squatter-on-a.sh index cd56d170..df0492c2 100644 --- a/test/selftest/010-stand-up-a-squatter-on-a.sh +++ b/test/selftest/010-stand-up-a-squatter-on-a.sh @@ -15,10 +15,17 @@ for _try in $(seq 1 20); do break fi done +# A SOURCED PART MUST NOT exit 0. This file is sourced by harness_selftest.sh, so +# `exit` here ends the DRIVER -- and a zero status said "610 checks passed" when the +# truth was "nothing ran" (#934). 66 is the status lib.sh calls PGC_EXIT_SKIPPED and +# the runners already pair with a `SKIPPED (ran no checks)` line; it is spelled as a +# literal because lib.sh arrives in part 020 and this part runs first, and part 430 +# asserts the literal still equals the constant so the two cannot drift. if [ "$SQ_PORT" = 0 ]; then echo "SKIP could not find a free port for the squatter cluster" + echo "$(basename "${BASH_SOURCE[0]}"): SKIPPED (ran no checks)" rm -rf "$SQ_DIR" - exit 0 + exit 66 fi _runpg=(env) if [ "$(id -u)" = "0" ]; then @@ -49,10 +56,12 @@ sq_datadir() { -d postgres -At -c 'SHOW data_directory' 2>/dev/null } +# Same reason as the port bail above: a zero here ends the driver saying success. if [ -z "$(sq_datadir)" ]; then echo "SKIP could not stand up a squatter cluster to test against" + echo "$(basename "${BASH_SOURCE[0]}"): SKIPPED (ran no checks)" squatter_down - exit 0 + exit 66 fi echo "-- squatter listening on $SQ_PORT ($(sq_datadir))" diff --git a/test/selftest/430-the-self-test-must-not-report.sh b/test/selftest/430-the-self-test-must-not-report.sh new file mode 100644 index 00000000..4237bd74 --- /dev/null +++ b/test/selftest/430-the-self-test-must-not-report.sh @@ -0,0 +1,135 @@ +# ---- the self-test must not report success having evaluated nothing (#934) ---- +# +# Handed a `pg_config` that does not exist, this suite printed four lines, never +# reached its summary, and EXITED 0. Measured on main before this part: +# +# bash test/harness_selftest.sh /usr/local/pg18a/bin/pg_config rc=0 1246 lines checks run: 610 +# bash test/harness_selftest.sh /usr/local/pgNOPE/bin/pg_config rc=0 4 lines no summary at all +# +# TWO CAUSES, and the second is the one that generalises. `_bindir` was assigned +# from a command that failed, so every later PATH was wrong -- and part 010, which +# is SOURCED, then took its own skip path and called `exit 0`, which exits the +# DRIVER rather than the part. A caller cannot tell that from a real pass. +# +# IT COST A MEASUREMENT ROUND TO FIND. A control and a mutation both reported rc=0 +# with zero FAIL lines, which reads exactly like "the mutation changed nothing" -- +# the conclusion the run existed to test. The log being 4 lines instead of 1246 is +# the only thing that gave it away. +# +# The default is `/usr/local/pg17/bin/pg_config`, which this box does not have, so +# the wrong invocation is the EASY one to make. + +_hs="$PGC_TESTDIR/harness_selftest.sh" +check "premise: the driver this part is about is where it is expected" \ + "$([ -f "$_hs" ] && echo yes || echo no)" "yes" + +# A pg_config that does not exist. The guard has to refuse BEFORE sourcing any +# part, so this cannot recurse: it dies at the argument check. +_h934_absent="$(bash "$_hs" /usr/local/pgNOPE934/bin/pg_config 2>&1)" +_h934_absent_rc=$? +check "handed a pg_config that does not exist, the self-test refuses instead of exiting 0" \ + "$([ "$_h934_absent_rc" != 0 ] && echo refused || echo "exited 0")" "refused" +# NOT just the path: bash's own "No such file or directory" already named it, so an +# arm matching the path alone passed before the guard existed. The token is what +# makes this arm about the refusal rather than about bash's diagnostics. +check "and the refusal is the guard's own, naming the path it could not use" \ + "$(printf '%s' "$_h934_absent" | grep -c 'no-pg-config.*pgNOPE934')" "1" +check "and it does not pretend to have run checks" \ + "$(printf '%s' "$_h934_absent" | grep -cE '^checks run:')" "0" + +# A pg_config that EXISTS and answers nothing. This is the shape that produced the +# empty `_bindir`: the command succeeded, so `[ -x ]` alone would accept it. +_h934_stub="$PGC_WORKDIR/pg_config_silent_934" +printf '#!/bin/sh\nexit 0\n' > "$_h934_stub" +chmod +x "$_h934_stub" +_h934_silent="$(bash "$_hs" "$_h934_stub" 2>&1)" +_h934_silent_rc=$? +check "premise: the stub is executable, so an -x test alone would accept it" \ + "$([ -x "$_h934_stub" ] && echo yes || echo no)" "yes" +check "premise: and it answers --bindir with nothing" \ + "$("$_h934_stub" --bindir | wc -c)" "0" +check "a pg_config whose --bindir is empty is refused too, not run with a broken PATH" \ + "$([ "$_h934_silent_rc" != 0 ] && echo refused || echo "exited 0")" "refused" + +# THE CONTROL, without running the whole suite again. Invoking the driver with a +# GOOD pg_config would re-run every part, so the control is the two predicates the +# guard tests, applied to the pg_config THIS run was given -- plus the existence of +# this run, which is the guard having accepted it. +check "control: the pg_config this run was handed satisfies the first predicate" \ + "$([ -x "$PGC_SELFTEST_PG_CONFIG" ] && echo yes || echo no)" "yes" +check "control: and the second, so a good pg_config is not refused" \ + "$([ -d "$("$PGC_SELFTEST_PG_CONFIG" --bindir)" ] && echo yes || echo no)" "yes" + +# ---- and no part may exit 0, because a part is SOURCED ------------------------ +# +# `exit 0` in a sourced part exits the DRIVER, before `pgc_summary`, with a status +# that says success. Part 010 did it on both of its skip paths -- and its own +# comment, four lines above the first one, says why that is the hazard: "a quiet +# skip means the guard stops being tested that run without anyone noticing". +# +# HEREDOC-AWARE, because the parts generate fixture scripts that legitimately end +# in `exit 0`, and a flat grep counts those as offences. +# +# THE REAL NUMBERS ARGUE IT BETTER THAN A ROUND ONE. An earlier version of this +# comment said a flat grep sees "13 either way", which is true of main and false of +# the branch it ships in: on this head it sees 22, because the fixtures BELOW add ten +# `exit 0` lines of their own. So the flat count moves for a reason that has nothing +# to do with the defect, which is the argument: +# +# heredoc-aware sweep 2 before the fix, 0 after +# flat grep -c 'exit 0' 13 before, 24 after +# +# Reported by @jdatcmd, who checked the number against the tree rather than the +# sentence. +# +# WHICH SPELLINGS THIS SEES, stated because a rule about the literal `0` is not the +# whole of "a sourced part must not end the driver claiming success". It sees +# `exit 0` and a BARE `exit`, which ends with the last command's status and is very +# often 0. It cannot decide `exit $?` or `exit "$rc"`, where the status is computed: +# flagging those would refuse a part that legitimately exits non-zero. Measured on +# this head, each of the three is **zero**, so nothing is open -- but the next reader +# should not assume the class is closed. Raised by @jdatcmd. +_h934_sweep() { + awk ' + FNR == 1 { hd = "" } + hd != "" { if ($0 == hd || $0 ~ "^[ \t]*" hd "[ \t]*$") hd = ""; next } + /<<-?[ \t]*'\''?[A-Za-z_][A-Za-z0-9_]*'\''?[ \t]*$/ { + if ($0 !~ /^[ \t]*#/) { + t = $0; sub(/.*<<-?[ \t]*/, "", t); gsub(/'\''/, "", t) + sub(/[ \t]*$/, "", t); hd = t; next + } + } + /^[ \t]*exit[ \t]+0[ \t]*$/ { printf "%s:%d\n", FILENAME, FNR } + /^[ \t]*exit[ \t]*$/ { printf "%s:%d\n", FILENAME, FNR } + ' "$@" +} +check "premise: the sweep reads every selftest part" \ + "$([ "$(ls "$PGC_TESTDIR"/selftest/*.sh | wc -l)" -ge 30 ] && echo yes || echo no)" "yes" +check "premise: and it finds a planted exit 0 outside a heredoc" \ + "$(printf 'echo hi\nexit 0\n' > "$PGC_WORKDIR/p934.sh"; _h934_sweep "$PGC_WORKDIR/p934.sh" | grep -c .)" "1" +check "premise: and a BARE exit too, which ends with the last status and is usually 0" \ + "$(printf 'echo hi\nexit\n' > "$PGC_WORKDIR/p934e.sh"; _h934_sweep "$PGC_WORKDIR/p934e.sh" | grep -c .)" "1" +check "premise: while a deliberate non-zero exit is not an offence" \ + "$(printf 'echo hi\nexit 66\n' > "$PGC_WORKDIR/p934f.sh"; _h934_sweep "$PGC_WORKDIR/p934f.sh" | grep -c .)" "0" +check "premise: while a fixture script ending in exit 0 inside a heredoc is not an offence" \ + "$({ printf 'cat > /tmp/x <<%sEOF%s\n' "'" "'"; printf 'exit 0\n'; printf 'EOF\n'; } > "$PGC_WORKDIR/p934b.sh"; _h934_sweep "$PGC_WORKDIR/p934b.sh" | grep -c .)" "0" +# THE LITERAL AND THE CONSTANT MUST AGREE. Part 010 spells 66 rather than +# $PGC_EXIT_SKIPPED because lib.sh arrives in part 020 and 010 runs first. A second +# copy of a constant is a thing that drifts, so the copy is asserted against the +# original instead of being trusted. +check "part 010's skip status is the one lib.sh calls PGC_EXIT_SKIPPED" \ + "$(grep -cE '^PGC_EXIT_SKIPPED=66$' "$PGC_TESTDIR/lib.sh")" "1" +# `[[:space:]]` AND NOT `\t`: in POSIX ERE a backslash-t is a literal `t`, so the +# first version of this arm searched for "texit 66" and reported 0 against a file +# holding two. Third time that has cost me a measurement today, in three different +# sweeps. +check "and part 010 exits with exactly that status on its skip paths" \ + "$(grep -cE '^[[:space:]]+exit 66$' "$PGC_TESTDIR/selftest/010-stand-up-a-squatter-on-a.sh")" "2" +# ECHO LINES ONLY. Counting the phrase anywhere found three: the two the part emits +# and the one inside the comment above them explaining why they are there. A sweep +# that counts its own documentation is the trap the raises scan records. +check "and pairs each with the marker the runners require beside the status" \ + "$(grep -cE '^[[:space:]]+echo .*SKIPPED \(ran no checks\)' "$PGC_TESTDIR/selftest/010-stand-up-a-squatter-on-a.sh")" "2" + +check "no selftest part exits 0, which would exit the driver before its summary" \ + "$(_h934_sweep "$PGC_TESTDIR"/selftest/*.sh | grep -c . || true)" "0"