From 4ab61ef148ec6ee393fe6f73c9b8de82a4c2bad2 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Tue, 1 Sep 2026 07:53:33 -0600 Subject: [PATCH 1/5] test: a check that could not run is a third state, not a pass (#858) Yesterday's audit proved 39 checks across 35 suites cannot fail. This is the first of four phases against the gap that let them ship: the harness answers "did anything print FAIL" and has never answered "could anything print FAIL". A check whose INPUT is absent -- a fixture that did not build, a capability the server lacks, an endpoint that is unreachable -- either passes vacuously or fails for a reason unrelated to the property under test. Neither answer is true, and `checks run: N` counts it either way, so a reader counting greens counts one that never asked its question. `check_unrunnable NAME REASON_CODE DETAIL` gives one check the honesty pgc_skip already gives a whole suite for a missing dependency. The suite then exits PGC_EXIT_INCOMPLETE, so an unrunnable check cannot hide inside a suite reporting PASSED; a failure still outranks it, because a failure is the more urgent fact. Three deliberate choices, each argued in review before it was written: The reason is a CLOSED ENUM plus a detail, from the first commit rather than after phase 3. Prose would mean rewriting every call site the day anything wants to group these, and a code outside the set FAILS rather than being accepted -- an enum that accepts anything is prose again. 67, not 66. 66 means "ran no checks". A suite holding one unrunnable check DID run checks, and collapsing the two loses the difference between "this suite is inert" and "this suite could not evaluate one thing". 67 is picked on the same grounds 66 was: bash produces 1, 2, 126, 127 and 128+n, psql 1, 2, 3, make 1 and 2. As with 66 the code alone is not trusted -- the runner must also see the INCOMPLETE line. Every state is in a total. pgc_summary prints `accounting: P passed + F failed + U unrunnable = N` and fails if it does not reconcile. A state outside a total is a state that can go missing, and 3,762 check sites is far past what anyone notices by reading. Red before green. The part was written first and run against a tree with no check_unrunnable in it: 9 of its 12 arms failed, each for the intended reason. REMOVAL PROOF, every arm asserting the check count so a reverted guard cannot report plain green: baseline 207 checks 0 red M1 delete check_unrunnable entirely 207 checks 10 red M2 let INCOMPLETE exit 0 207 checks 2 red M3 accept any reason code 207 checks 1 red M4 stop counting passes 207 checks 2 red restore 207 checks 0 red One arm of my own was vacuous and is not in the file. "A failure outranks an unrunnable check" asserted the suite exits 1 FAILED -- which it does whether or not the feature exists, because the fixture also holds a real failure. It could not distinguish. It now asserts the accounting line, which only a suite that recorded BOTH states can print. Writing the same defect this phase exists to remove, in the test for it, is worth recording rather than quietly fixing. Also measured, and it bounds phase 2 rather than this one: 12 of 238 suites are outside the accounting. bench_guards.sh and docs_style.sh never source lib.sh; smoke, audit, concurrency, phase2 through phase6, unique_conc and update_conc source it but never call pgc_summary. Reconciliation is a lie for those twelve until they are brought in or exempted with a premise that fails when the list grows. --- test/lib.sh | 69 +++++++++ .../320-a-check-that-could-not-run.sh | 134 ++++++++++++++++++ 2 files changed, 203 insertions(+) create mode 100644 test/selftest/320-a-check-that-could-not-run.sh diff --git a/test/lib.sh b/test/lib.sh index 2f609d63..4d5fa7f0 100755 --- a/test/lib.sh +++ b/test/lib.sh @@ -46,6 +46,29 @@ PGC_CHECKS=0 # log before believing it. Two independent signals, because one was not enough. PGC_EXIT_SKIPPED=66 +# The status pgc_summary uses for "a check could not be evaluated". +# +# NOT 66. 66 means the suite ran no checks at all; a suite holding one unrunnable +# check DID run checks, and collapsing the two loses the difference between "this +# suite is inert" and "this suite could not evaluate one thing". 67 is chosen on +# the same grounds 66 was -- bash produces 1, 2, 126, 127 and 128+n, psql 1, 2, 3, +# make 1 and 2 -- and, as with 66, the code alone is not trusted: a runner must +# also see the INCOMPLETE line in the log, because `set -e` propagates whatever +# status an aborting command returned. +PGC_EXIT_INCOMPLETE=67 + +# Counts for the three states. Every state is in a total or it is a state that +# can go missing, and pgc_summary reconciles them against PGC_CHECKS. +PGC_PASSED=0 +PGC_UNRUN=0 + +# The closed set of reasons a check could not be evaluated. Prose would have to +# be rewritten at every call site the day anything wants to group these, so the +# reason is a code plus a detail from the start. A code outside this set is a +# FAILURE rather than a silent acceptance: an enum that accepts anything is not +# an enum, and the first invented code would make it prose again. +PGC_UNRUN_REASONS="MISSING_DEPENDENCY UNSUPPORTED_MAJOR ABSENT_FIXTURE UNAVAILABLE_ENDPOINT UNMET_PRECONDITION" + # ---- cluster identity helpers ---------------------------------------------- # Normalize a directory for comparison. `cd && pwd -P` is POSIX; realpath -m is @@ -555,10 +578,41 @@ psql_file() { # ---- assertions ------------------------------------------------------------ + +# A check that could not be evaluated is a third state, not a pass. +# +# When a check's INPUT is absent -- a fixture that did not build, a capability the +# server lacks, an endpoint that is not reachable -- the check either passes +# vacuously or fails for a reason unrelated to the property under test. Neither +# answer is true, and "checks run: N" counts it either way, so a reader counting +# greens counts one that never asked its question. +# +# pgc_skip already refuses to let a MISSING DEPENDENCY read as a pass at suite +# granularity. This is the same honesty for one check. +# +# The suite exits PGC_EXIT_INCOMPLETE, so an unrunnable check cannot hide inside a +# suite that reports PASSED. A failure still outranks it: a suite with both is +# FAILED, because the failure is the more urgent fact. +check_unrunnable() { # check_unrunnable NAME REASON_CODE DETAIL + local name="$1" reason="${2:-}" detail="${3:-}" + PGC_CHECKS=$((PGC_CHECKS + 1)) + case " $PGC_UNRUN_REASONS " in + *" $reason "*) ;; + *) + echo "FAIL $name: unrunnable reason [$reason] is not one of: $PGC_UNRUN_REASONS" + PGC_FAIL=1 + return + ;; + esac + PGC_UNRUN=$((PGC_UNRUN + 1)) + echo "UNRUN $name: $reason: $detail" +} + check() { local name="$1" got="$2" want="$3" PGC_CHECKS=$((PGC_CHECKS + 1)) if [ "$got" = "$want" ]; then + PGC_PASSED=$((PGC_PASSED + 1)) echo "PASS $name" else echo "FAIL $name: got [$got] want [$want]" @@ -1028,8 +1082,19 @@ pgc_skip() { # pgc_skip # count 2 separately and report how many suites actually ran, which is what #422 # did one level up for how many VERSIONS actually ran. pgc_summary() { + local _failed=$((PGC_CHECKS - PGC_PASSED - PGC_UNRUN)) echo echo "checks run: $PGC_CHECKS" + echo "checks unrunnable: $PGC_UNRUN" + # Every state in a total. A state that is not in a total is a state that can + # go missing, and this harness has 3,762 check sites -- far past what anyone + # notices by reading. If this line does not add up the harness is lying about + # its own arithmetic, so it is a failure rather than a note. + echo "accounting: $PGC_PASSED passed + $_failed failed + $PGC_UNRUN unrunnable = $PGC_CHECKS" + if [ "$_failed" -lt 0 ]; then + echo "FAIL the summary does not reconcile: more passed+unrunnable than checks run" + PGC_FAIL=1 + fi if [ "$PGC_FAIL" != "0" ]; then echo "$(basename "$0"): FAILED" # A source-shape suite (wal_envelope, decode_interrupts) never calls @@ -1067,6 +1132,10 @@ pgc_summary() { echo "$(basename "$0"): SKIPPED (ran no checks)" exit $PGC_EXIT_SKIPPED fi + if [ "$PGC_UNRUN" != "0" ]; then + echo "$(basename "$0"): INCOMPLETE" + exit $PGC_EXIT_INCOMPLETE + fi echo "$(basename "$0"): PASSED" exit 0 } diff --git a/test/selftest/320-a-check-that-could-not-run.sh b/test/selftest/320-a-check-that-could-not-run.sh new file mode 100644 index 00000000..0353d28e --- /dev/null +++ b/test/selftest/320-a-check-that-could-not-run.sh @@ -0,0 +1,134 @@ +# ---- a check that could not run is a third state, not a pass ---------------- +# +# WHY THIS EXISTS. Every check in this harness has two outcomes: it printed PASS +# or it printed FAIL. When a check's INPUT is absent -- a fixture that did not +# build, a capability the server lacks, a file the probe reads that is not there +# -- the check either passes vacuously or fails for a reason that has nothing to +# do with the property under test. Neither answer is true. The suite says +# "checks run: N" either way, and a reader counting greens counts one that never +# asked its question. +# +# That is the same defect as a permanent SKIP one level down: a skip you wrote is +# a check you did not write. `pgc_skip` already refuses to let a MISSING +# DEPENDENCY read as a pass -- it FAILS unless waived deliberately. This gives a +# single CHECK the same honesty at check granularity. +# +# THE STATE MUST NOT BE A PASS AT THE SUITE LEVEL EITHER, which is the whole +# point: a suite holding an unrunnable check exits PGC_EXIT_INCOMPLETE, not 0, so +# a runner cannot report it green. Failure still dominates -- a suite with both a +# FAIL and an UNRUN is FAILED, because the failure is the more urgent fact. +# +# WHY A NEW EXIT CODE AND NOT 66. 66 means "ran no checks". A suite with one +# unrunnable check DID run checks, and collapsing the two states would lose the +# distinction between "this suite is inert" and "this suite could not evaluate +# one thing". 67 is chosen on the same grounds 66 was: bash produces 1, 2, 126, +# 127 and 128+n; psql produces 1, 2, 3; make produces 1 and 2. And as with 66, +# the code alone is not trusted -- the runner must also see the INCOMPLETE line +# in the log, because `set -e` can propagate any status an aborting command +# returns. + +_cur_lib="$PGC_TESTDIR/lib.sh" + +check "premise: the harness library is where this part thinks it is" \ + "$([ -f "$_cur_lib" ] && echo yes || echo no)" "yes" + +check "lib.sh defines check_unrunnable" \ + "$(grep -c '^check_unrunnable()' "$_cur_lib")" "1" + +check "lib.sh defines the INCOMPLETE exit status" \ + "$(grep -c '^PGC_EXIT_INCOMPLETE=' "$_cur_lib")" "1" + +# ---- the four states, each run as its own suite ---------------------------- +# +# A source-shape fixture: it sources lib.sh and calls the primitives without +# pgc_setup, so no cluster is started and the four arms cost nothing. Each arm +# differs from the others in exactly one respect, so a single behaviour is under +# test in each. + +_cur_dir="$PGC_WORKDIR/unrun"; mkdir -p "$_cur_dir" + +_cur_make() { # _cur_make NAME BODY + { + printf '#!/usr/bin/env bash\n' + printf '. "%s"\n' "$_cur_lib" + printf '%s\n' "$2" + printf 'pgc_summary\n' + } > "$_cur_dir/$1.sh" + chmod 755 "$_cur_dir/$1.sh" +} + +_cur_run() { # _cur_run NAME -> " " + local out rc + out="$(bash "$_cur_dir/$1.sh" 2>&1)"; rc=$? + printf '%s %s' "$rc" "$(printf '%s' "$out" | grep -oE '(PASSED|FAILED|INCOMPLETE|SKIPPED)' | tail -1)" +} + +_cur_out() { # _cur_out NAME -> the whole output + bash "$_cur_dir/$1.sh" 2>&1 +} + +_cur_make onlypass 'check "a" ok ok' +_cur_make passunrun 'check "a" ok ok +check_unrunnable "b" ABSENT_FIXTURE "the parquet corpus was not built"' +_cur_make failunrun 'check "a" ok NOPE +check_unrunnable "b" ABSENT_FIXTURE "the parquet corpus was not built"' +_cur_make onlyunrun 'check_unrunnable "b" ABSENT_FIXTURE "the parquet corpus was not built"' + +check "a suite whose checks all passed still exits 0 PASSED" \ + "$(_cur_run onlypass)" "0 PASSED" + +check "one unrunnable check makes the suite INCOMPLETE, not passed" \ + "$(_cur_run passunrun)" "67 INCOMPLETE" + +# This arm reads "1 FAILED" whether or not check_unrunnable exists, because the +# fixture also holds a real failure -- so the exit code ALONE cannot distinguish +# the feature from its absence. Assert the accounting line instead, which only a +# suite that recorded BOTH states can print. +check "a failure outranks an unrunnable check, and both are still counted" \ + "$(_cur_out failunrun | grep -oE 'accounting: [0-9]+ passed \+ [0-9]+ failed \+ [0-9]+ unrunnable')" \ + "accounting: 0 passed + 1 failed + 1 unrunnable" + +# The distinction 66 cannot carry: this suite RAN a check. Reporting it as +# "ran no checks" would merge "inert suite" with "could not evaluate one thing". +check "a suite of nothing but unrunnable checks is INCOMPLETE, not SKIPPED" \ + "$(_cur_run onlyunrun)" "67 INCOMPLETE" + +check "and it is not reported as having run no checks" \ + "$(_cur_out onlyunrun | grep -c 'ran no checks')" "0" + +# The reason travels with the state. A third state that does not say why is a +# skip with better manners. +check "the unrunnable check names itself, its reason code and its detail" \ + "$(_cur_out passunrun | grep -c '^UNRUN b: ABSENT_FIXTURE: the parquet corpus was not built')" "1" + +# Counting: an unrunnable check is still a check that was reached, so it counts +# toward the total, and it is reported separately so the total can be split. +check "an unrunnable check counts toward checks run" \ + "$(_cur_out passunrun | grep -oE 'checks run: [0-9]+' | grep -oE '[0-9]+')" "2" + +check "and the unrunnable ones are reported as their own count" \ + "$(_cur_out passunrun | grep -oE 'checks unrunnable: [0-9]+' | grep -oE '[0-9]+')" "1" + +check "a suite with none says so as zero rather than staying silent" \ + "$(_cur_out onlypass | grep -c 'checks unrunnable: 0')" "1" + +# The reason is an enum plus a detail, not prose. Phase 3 has to group these, and +# a free-text reason would mean rewriting every call site later. A code outside +# the set is a FAILURE rather than a silent acceptance, so the enum cannot rot on +# first use by someone inventing a code. +_cur_make badcode 'check_unrunnable "b" NOT_A_REAL_CODE "x"' +check "an unrunnable reason outside the enum fails rather than being accepted" \ + "$(_cur_run badcode)" "1 FAILED" + +# Every state is in a total, or it is a state that can go missing. 3,762 check +# sites is well past what anyone notices by reading. +check "the summary reconciles the three states against the total" \ + "$(_cur_out passunrun | grep -oE 'accounting: [0-9]+ passed \+ [0-9]+ failed \+ [0-9]+ unrunnable = [0-9]+')" \ + "accounting: 1 passed + 0 failed + 1 unrunnable = 2" + +check "and a suite with no unrunnable checks reconciles too" \ + "$(_cur_out onlypass | grep -oE 'accounting: [0-9]+ passed \+ [0-9]+ failed \+ [0-9]+ unrunnable = [0-9]+')" \ + "accounting: 1 passed + 0 failed + 0 unrunnable = 1" + +unset _cur_lib _cur_dir +unset -f _cur_make _cur_run _cur_out From 618ed890e3a650c0ba13bd27cafa9408f2a97081 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Tue, 1 Sep 2026 09:54:41 -0600 Subject: [PATCH 2/5] test: the accounting is a measurement, not an identity (#858) Review found a defect in the first commit that shipped wrong output from six suites, and the reason nothing caught it is the shape this branch exists to remove. check_ratio printed PASS and never touched PGC_PASSED. The failed count was DERIVED as CHECKS - PASSED - UNRUN, so every passing ratio check was reported as a failure. Reproduced by hand, two passing checks: PASS an ordinary passing check PASS a ratio well inside its bound (0.10x, bound 1.0x, from a=10 b=100) accounting: 1 passed + 1 failed + 0 unrunnable = 2 Six shipped suites call check_ratio directly in the parent shell, twelve sites, none in a subshell: column_projection, int8_agg_int128, native_fetch_bigcap, native_fetch_cache, objstore_http_read, planner_choice_quality. THE ACCOUNTING LINE COULD NOT SEE IT, AND THAT IS THE REAL FINDING. With the failed count derived from the other three, P + (N-P-U) + U = N holds for ANY values; the only reachable red was a negative. It was an accounting identity guaranteed by construction rather than measured -- shape 12 of the audit that produced this branch -- shipped inside the diff that implements the fix for shape 12. I wrote it, and the reviewer found it by going after the shape rather than the code. The fix is three counters maintained INDEPENDENTLY and reconciled against a fourth: a real PGC_FAILED at the nine sites that count a check and record a failure, PGC_PASSED in check_ratio's pass path, and PASSED + FAILED + UNRUN == CHECKS asserted in pgc_summary. Adding PGC_PASSED to check_ratio alone would have fixed the symptom and left the next helper that forgets undetectable. Four arms, and the last two are the ones that would have caught this: a passing ratio check is counted as a pass, not a failure and the suite that holds it still passes a counter that drifts is caught rather than absorbed and the suite holding it fails rather than reporting PASSED The drift arm counts a check through a helper that records no outcome, which is exactly what check_ratio did. Verified on three suites rather than one, because the claim is about six: harness_selftest 211 checks 211 passed + 0 failed + 0 unrunnable = 211 column_projection 38 checks 38 passed + 0 failed + 0 unrunnable = 38 int8_agg_int128 34 checks 34 passed + 0 failed + 0 unrunnable = 34 column_projection and int8_agg_int128 each report two ratio checks that the first commit counted as failures. One thing the reviewer expected to find and did not, recorded because a negative result is worth as much: an empty reason code does NOT slip through `case " $REASONS " in *" $reason "*`, because the pattern needs a double space and the haystack has none. Tested live; the code was right. Still open and not addressed here: the runner's treatment of exit 67 is unasserted, reachable only through a catch-all else. That file is what phase 2 opens, and it belongs there rather than in a drive-by edit. --- test/lib.sh | 28 +++++++++++-- .../320-a-check-that-could-not-run.sh | 41 +++++++++++++++++++ 2 files changed, 66 insertions(+), 3 deletions(-) diff --git a/test/lib.sh b/test/lib.sh index 4d5fa7f0..87ae32c2 100755 --- a/test/lib.sh +++ b/test/lib.sh @@ -60,6 +60,7 @@ PGC_EXIT_INCOMPLETE=67 # Counts for the three states. Every state is in a total or it is a state that # can go missing, and pgc_summary reconciles them against PGC_CHECKS. PGC_PASSED=0 +PGC_FAILED=0 PGC_UNRUN=0 # The closed set of reasons a check could not be evaluated. Prose would have to @@ -601,6 +602,7 @@ check_unrunnable() { # check_unrunnable NAME REASON_CODE DETAIL *) echo "FAIL $name: unrunnable reason [$reason] is not one of: $PGC_UNRUN_REASONS" PGC_FAIL=1 + PGC_FAILED=$((PGC_FAILED + 1)) return ;; esac @@ -617,6 +619,7 @@ check() { else echo "FAIL $name: got [$got] want [$want]" PGC_FAIL=1 + PGC_FAILED=$((PGC_FAILED + 1)) fi } @@ -668,6 +671,7 @@ check_text() { if [ -z "$got" ] || [ -z "$want" ]; then PGC_CHECKS=$((PGC_CHECKS + 1)) PGC_FAIL=1 + PGC_FAILED=$((PGC_FAILED + 1)) echo "FAIL $name: a side is empty, so nothing was compared:" \ "got [$got] want [$want]" return 1 @@ -681,6 +685,7 @@ check_num() { if ! pgc_is_number "$got" || ! pgc_is_number "$want"; then PGC_CHECKS=$((PGC_CHECKS + 1)) PGC_FAIL=1 + PGC_FAILED=$((PGC_FAILED + 1)) echo "FAIL $name: not a measurement, so nothing was compared:" \ "got [$got] want [$want]" return 1 @@ -716,6 +721,7 @@ check_ratio() { # $1 label, $2 a, $3 b, $4 max if ! pgc_is_number "$a" || ! pgc_is_number "$b" || ! pgc_is_number "$max"; then PGC_CHECKS=$((PGC_CHECKS + 1)) PGC_FAIL=1 + PGC_FAILED=$((PGC_FAILED + 1)) echo "FAIL $name: not a measurement, so no ratio was formed:" \ "a=[$a] b=[$b] max=[$max]" return 1 @@ -723,6 +729,7 @@ check_ratio() { # $1 label, $2 a, $3 b, $4 max if [ "$(awk -v x="$a" -v y="$b" 'BEGIN { print (x + 0 == 0 || y + 0 == 0) ? "yes" : "no" }')" = yes ]; then PGC_CHECKS=$((PGC_CHECKS + 1)) PGC_FAIL=1 + PGC_FAILED=$((PGC_FAILED + 1)) echo "FAIL $name: a side of the ratio is zero, so nothing was measured:" \ "a=[$a] b=[$b]" return 1 @@ -730,10 +737,12 @@ check_ratio() { # $1 label, $2 a, $3 b, $4 max ratio="$(awk -v a="$a" -v b="$b" 'BEGIN { printf "%.2f", a / b }')" PGC_CHECKS=$((PGC_CHECKS + 1)) if [ "$(awk -v r="$ratio" -v m="$max" 'BEGIN { print (r <= m) ? "yes" : "no" }')" = yes ]; then + PGC_PASSED=$((PGC_PASSED + 1)) echo "PASS $name (${ratio}x, bound ${max}x, from a=$a b=$b)" else echo "FAIL $name: ${ratio}x exceeds the ${max}x bound (a=$a b=$b)" PGC_FAIL=1 + PGC_FAILED=$((PGC_FAILED + 1)) fi } @@ -749,6 +758,7 @@ pgc_require_tools() { echo "FAIL the tools this suite measures with are missing:$missing" PGC_CHECKS=$((PGC_CHECKS + 1)) PGC_FAIL=1 + PGC_FAILED=$((PGC_FAILED + 1)) return 1 fi return 0 @@ -1058,6 +1068,7 @@ pgc_skip() { # pgc_skip fi PGC_CHECKS=$((PGC_CHECKS + 1)) PGC_FAIL=1 + PGC_FAILED=$((PGC_FAILED + 1)) echo "FAIL $2" echo " A missing dependency is an environment defect, not a pass. Install" echo " it, or set $allow_one=1 to run knowingly without this coverage." @@ -1082,7 +1093,8 @@ pgc_skip() { # pgc_skip # count 2 separately and report how many suites actually ran, which is what #422 # did one level up for how many VERSIONS actually ran. pgc_summary() { - local _failed=$((PGC_CHECKS - PGC_PASSED - PGC_UNRUN)) + local _failed=$PGC_FAILED + local _sum=$((PGC_PASSED + PGC_FAILED + PGC_UNRUN)) echo echo "checks run: $PGC_CHECKS" echo "checks unrunnable: $PGC_UNRUN" @@ -1091,8 +1103,18 @@ pgc_summary() { # notices by reading. If this line does not add up the harness is lying about # its own arithmetic, so it is a failure rather than a note. echo "accounting: $PGC_PASSED passed + $_failed failed + $PGC_UNRUN unrunnable = $PGC_CHECKS" - if [ "$_failed" -lt 0 ]; then - echo "FAIL the summary does not reconcile: more passed+unrunnable than checks run" + # A MEASUREMENT, not an identity. The failed count is its own counter rather + # than CHECKS - PASSED - UNRUN, because a derived third term makes + # P + (N-P-U) + U = N true for ANY values: a helper that counts a check and + # records no outcome drifts invisibly. That is not hypothetical -- check_ratio + # printed PASS and never touched PGC_PASSED, so every passing ratio check was + # reported as a failure in six shipped suites, and the first version of this + # line could not see it. Three counters maintained independently, reconciled + # against a fourth, is the only version of it that can fail. + if [ "$_sum" != "$PGC_CHECKS" ]; then + echo "FAIL the summary does not reconcile: $PGC_PASSED passed + $PGC_FAILED failed + $PGC_UNRUN unrunnable = $_sum, but $PGC_CHECKS checks ran" + echo " A check was counted whose outcome nothing recorded. Find the helper" + echo " that bumps PGC_CHECKS without touching PGC_PASSED, PGC_FAILED or PGC_UNRUN." PGC_FAIL=1 fi if [ "$PGC_FAIL" != "0" ]; then diff --git a/test/selftest/320-a-check-that-could-not-run.sh b/test/selftest/320-a-check-that-could-not-run.sh index 0353d28e..7e1e9140 100644 --- a/test/selftest/320-a-check-that-could-not-run.sh +++ b/test/selftest/320-a-check-that-could-not-run.sh @@ -130,5 +130,46 @@ check "and a suite with no unrunnable checks reconciles too" \ "$(_cur_out onlypass | grep -oE 'accounting: [0-9]+ passed \+ [0-9]+ failed \+ [0-9]+ unrunnable = [0-9]+')" \ "accounting: 1 passed + 0 failed + 0 unrunnable = 1" +# ---- the accounting must be a MEASUREMENT, not an identity ------------------ +# +# The first version of this part derived the failed count as +# CHECKS - PASSED - UNRUN and called the result an accounting line. That identity +# is true for any values: a counter can drift arbitrarily and P + (N-P-U) + U = N +# still holds, so the only reachable red was a negative. It is shape 12 from the +# audit that produced this phase -- an accounting identity guaranteed by +# construction rather than measured -- and it shipped inside the diff that exists +# to catch shape 12. +# +# It was not hypothetical. check_ratio printed PASS and never touched PGC_PASSED, +# so every passing ratio check was counted as a failure in six shipped suites +# (column_projection, int8_agg_int128, native_fetch_bigcap, native_fetch_cache, +# objstore_http_read, planner_choice_quality -- twelve call sites, none in a +# subshell). The line meant to prove the states reconcile could not see it. +# +# Three counters are now maintained INDEPENDENTLY and reconciled against a +# fourth. A helper that forgets any one of them reddens here. + +_cur_make ratio 'check "a" ok ok +check_ratio "a ratio well inside its bound" 10 100 1.0' + +check "a passing ratio check is counted as a pass, not a failure" \ + "$(_cur_out ratio | grep -oE 'accounting: [0-9]+ passed \+ [0-9]+ failed \+ [0-9]+ unrunnable = [0-9]+')" \ + "accounting: 2 passed + 0 failed + 0 unrunnable = 2" + +check "and the suite that holds it still passes" \ + "$(_cur_run ratio)" "0 PASSED" + +# A drifting counter must be visible. Pass a check through a helper that counts +# the check but records neither outcome, which is exactly what check_ratio did. +_cur_make drift 'check "a" ok ok +PGC_CHECKS=$((PGC_CHECKS + 1)) +echo "PASS a check nothing counted"' + +check "a counter that drifts is caught rather than absorbed" \ + "$(_cur_out drift | grep -c '^FAIL the summary does not reconcile')" "1" + +check "and the suite holding it fails rather than reporting PASSED" \ + "$(_cur_run drift)" "1 FAILED" + unset _cur_lib _cur_dir unset -f _cur_make _cur_run _cur_out From 673d680906f3b7c41aebe8004b908d8bf175f396 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Tue, 1 Sep 2026 10:18:50 -0600 Subject: [PATCH 3/5] test: lib.sh owns the counters, so a suite-local helper cannot drift (#858) The reconciliation added in 618ed89 turned test/projections.sh RED ON A HEALTHY TREE, which is a worse defect than the miscount it exists to find, and it is the exact thing this branch rejects in other people's fixes. checks run: 64 accounting: 55 passed + 0 failed + 0 unrunnable = 55, but 64 checks ran projections.sh: FAILED 64 PASS lines, zero real failures, suite failed. The cause is projections.sh's own expect_fail(), ten call sites, which bumps PGC_CHECKS and records no outcome. It has been miscounting for as long as it has existed and nothing could tell, which is the finding underneath the breakage. FIXING THOSE TEN CALL SITES ALONE WOULD LEAVE THE NEXT expect_fail UNDETECTABLE -- the same argument that rejected fixing check_ratio's counter without a real PGC_FAILED. So lib.sh owns the counters: pgc_pass NAME counts a check and records the pass pgc_fail NAME DETAIL counts a check and records the failure projections.sh uses them. Eleven error-path sites across eight suites now record their failure as well as counting the check; those were not fatal (they add a second misleading FAIL to a suite that was failing anyway) but they are the same defect and they are in the sweep's population. A SWEEP MAKES IT A RULE RATHER THAN TEN EDITS. Every direct write to PGC_CHECKS outside lib.sh must record an outcome within the surrounding lines. Files that keep their own counters and never call pgc_summary are exempt, and the exemption is MEASURED from the file (does it call pgc_summary) rather than taken from a name list -- bench_guards.sh qualifies, and asserting that beats trusting it. THE SWEEP CAUGHT ITS OWN TEST ON THE FIRST RUN. The drift fixture spelled the forbidden line out literally, so the grep could not tell the fixture from the defect. The line is now assembled at runtime: a test for a pattern must not contain the pattern. The sweep was right and the fixture was the violation. Removal proof. Revert expect_fail to the shape that reddened the tree: FAIL every direct write to PGC_CHECKS records an outcome too: got [[1: projections.sh:27]] want [[]] accounting: 55 passed + 0 failed + 0 unrunnable = 55, but 64 checks ran projections.sh: FAILED Both guards fire: the class guard names the site, and the suite's own summary refuses to reconcile. Green at this head, three suites rather than one because the claim is about the tree and not about lib.sh: harness_selftest 213 checks 213 passed + 0 failed + 0 unrunnable = 213 projections 64 checks 64 passed + 0 failed + 0 unrunnable = 64 column_projection 38 checks 38 passed + 0 failed + 0 unrunnable = 38 Found by review, on a branch whose subject is exactly this class of defect, in the commit that fixes the previous instance of it. --- test/analyze_differential.sh | 2 +- test/analyze_function.sh | 2 +- test/lib.sh | 27 +++++++++ test/native_groupagg_batch.sh | 2 +- test/native_repack.sh | 2 +- test/objstore_module.sh | 2 +- test/objstore_stash_recovery.sh | 4 +- test/parquet_export_stats.sh | 6 +- test/pg19_vacuum_options.sh | 2 +- test/projections.sh | 9 +-- .../320-a-check-that-could-not-run.sh | 58 ++++++++++++++++++- 11 files changed, 98 insertions(+), 18 deletions(-) diff --git a/test/analyze_differential.sh b/test/analyze_differential.sh index a6de58a4..16eaa8e7 100755 --- a/test/analyze_differential.sh +++ b/test/analyze_differential.sh @@ -55,7 +55,7 @@ ROWS=${PGC_ANALYZE_DIFF_ROWS:-50000} # one and reported as "supported, skipped". if ! pgc_is_number "${PGC_MAJOR:-}"; then echo "FAIL could not read the server major, so the gate below cannot be trusted: got [${PGC_MAJOR:-}]" - PGC_CHECKS=$((PGC_CHECKS + 1)) + PGC_CHECKS=$((PGC_CHECKS + 1)); PGC_FAILED=$((PGC_FAILED + 1)) PGC_FAIL=1 pgc_summary fi diff --git a/test/analyze_function.sh b/test/analyze_function.sh index 7e4a33be..d491e1ef 100755 --- a/test/analyze_function.sh +++ b/test/analyze_function.sh @@ -64,7 +64,7 @@ ROWS=${PGC_ANALYZE_ROWS:-500000} # old one, or a broken environment would report SKIP and look supported. if ! pgc_is_number "${PGC_MAJOR:-}"; then echo "FAIL could not read the server major, so the gate below cannot be trusted: got [${PGC_MAJOR:-}]" - PGC_CHECKS=$((PGC_CHECKS + 1)) + PGC_CHECKS=$((PGC_CHECKS + 1)); PGC_FAILED=$((PGC_FAILED + 1)) PGC_FAIL=1 pgc_summary fi diff --git a/test/lib.sh b/test/lib.sh index 87ae32c2..f4f8638f 100755 --- a/test/lib.sh +++ b/test/lib.sh @@ -580,6 +580,33 @@ psql_file() { # ---- assertions ------------------------------------------------------------ +# Record an outcome for a check a SUITE-LOCAL helper counted. +# +# PGC_CHECKS is an invariant that only this file can keep, because pgc_summary +# reconciles PASSED + FAILED + UNRUN against it. A suite that bumps PGC_CHECKS +# itself and prints its own PASS or FAIL leaves the totals short, and the +# reconciliation then reds a healthy tree -- which is a worse defect than the +# miscount it exists to find. That is not hypothetical: projections.sh has an +# expect_fail() with ten call sites that has been counting checks whose outcome +# nothing recorded for as long as it has existed, and nothing could tell. +# +# A suite-local helper calls these instead of touching the counters. They are the +# only supported way to add a check from outside this file, and selftest part 320 +# sweeps for direct PGC_CHECKS writes so the next expect_fail is caught when it +# is written rather than when it reddens something. +pgc_pass() { # pgc_pass NAME + PGC_CHECKS=$((PGC_CHECKS + 1)) + PGC_PASSED=$((PGC_PASSED + 1)) + echo "PASS $1" +} + +pgc_fail() { # pgc_fail NAME DETAIL + PGC_CHECKS=$((PGC_CHECKS + 1)) + PGC_FAILED=$((PGC_FAILED + 1)) + PGC_FAIL=1 + if [ -n "${2:-}" ]; then echo "FAIL $1: $2"; else echo "FAIL $1"; fi +} + # A check that could not be evaluated is a third state, not a pass. # # When a check's INPUT is absent -- a fixture that did not build, a capability the diff --git a/test/native_groupagg_batch.sh b/test/native_groupagg_batch.sh index 3caafc71..0149a972 100755 --- a/test/native_groupagg_batch.sh +++ b/test/native_groupagg_batch.sh @@ -89,7 +89,7 @@ agree_in() { # agree_in TABLE LABEL "SELECT ... FROM %T ..." # md5 of empty input is a fixed string, so require the columnar arm produced # rows at all before trusting the comparison. if [ -z "$(q "${tmpl//%T/$tbl}" | head -1)" ]; then - PGC_CHECKS=$((PGC_CHECKS + 1)); PGC_FAIL=1 + PGC_CHECKS=$((PGC_CHECKS + 1)); PGC_FAILED=$((PGC_FAILED + 1)); PGC_FAIL=1 echo "FAIL $label: the columnar arm returned no rows, so nothing was compared" return 1 fi diff --git a/test/native_repack.sh b/test/native_repack.sh index 77cccb0b..b8ef2bd0 100755 --- a/test/native_repack.sh +++ b/test/native_repack.sh @@ -54,7 +54,7 @@ srv="$(q 'SHOW server_version_num')" # on an older major depends on it being 0. Asserting the premise must not destroy # the skip it guards. if ! pgc_is_number "$srv"; then - PGC_CHECKS=$((PGC_CHECKS + 1)) + PGC_CHECKS=$((PGC_CHECKS + 1)); PGC_FAILED=$((PGC_FAILED + 1)) PGC_FAIL=1 echo "FAIL the server did not answer 'SHOW server_version_num': got [$srv]" pgc_summary diff --git a/test/objstore_module.sh b/test/objstore_module.sh index 5ae0e3eb..7fb139ac 100755 --- a/test/objstore_module.sh +++ b/test/objstore_module.sh @@ -72,7 +72,7 @@ for stash in "$MOD.away" "$MOD.probe"; do if ! stash_is_debris; then echo "FAIL restored $stash to $MOD, but that is not a module either." echo " This installation needs 'make install' before the suite can run." - PGC_CHECKS=$((PGC_CHECKS + 1)) + PGC_CHECKS=$((PGC_CHECKS + 1)); PGC_FAILED=$((PGC_FAILED + 1)) PGC_FAIL=1 pgc_summary exit 1 diff --git a/test/objstore_stash_recovery.sh b/test/objstore_stash_recovery.sh index 51c583f8..e0ef1906 100755 --- a/test/objstore_stash_recovery.sh +++ b/test/objstore_stash_recovery.sh @@ -53,12 +53,12 @@ if [ -z "${PGC_SKIP_BUILD:-}" ]; then echo "-- building" make -C "$SRCDIR" PG_CONFIG="$PG_CONFIG" >/dev/null || { echo "FAIL build failed, so nothing below measures the guard" - PGC_CHECKS=$((PGC_CHECKS + 1)); PGC_FAIL=1; pgc_summary + PGC_CHECKS=$((PGC_CHECKS + 1)); PGC_FAILED=$((PGC_FAILED + 1)); PGC_FAIL=1; pgc_summary } echo "-- installing" make -C "$SRCDIR" install PG_CONFIG="$PG_CONFIG" >/dev/null || { echo "FAIL install failed, so nothing below measures the guard" - PGC_CHECKS=$((PGC_CHECKS + 1)); PGC_FAIL=1; pgc_summary + PGC_CHECKS=$((PGC_CHECKS + 1)); PGC_FAILED=$((PGC_FAILED + 1)); PGC_FAIL=1; pgc_summary } fi diff --git a/test/parquet_export_stats.sh b/test/parquet_export_stats.sh index cb4805bd..db83952b 100755 --- a/test/parquet_export_stats.sh +++ b/test/parquet_export_stats.sh @@ -89,7 +89,7 @@ if ! python3 "$STATS_PY" "$PARQ" > "$S" 2>"$PGC_WORKDIR/stats.err"; then echo "FAIL the footer parser could not read the exported file:" sed 's/^/ /' "$PGC_WORKDIR/stats.err" PGC_FAIL=1 - PGC_CHECKS=$((PGC_CHECKS + 1)) + PGC_CHECKS=$((PGC_CHECKS + 1)); PGC_FAILED=$((PGC_FAILED + 1)) pgc_summary fi @@ -229,7 +229,7 @@ check_num "every bound is its physical width" \ check_float() { # check_float NAME GOT WANT local name="$1" got="$2" want="$3" if ! pgc_is_number "$got" || ! pgc_is_number "$want"; then - PGC_CHECKS=$((PGC_CHECKS + 1)) + PGC_CHECKS=$((PGC_CHECKS + 1)); PGC_FAILED=$((PGC_FAILED + 1)) PGC_FAIL=1 echo "FAIL $name: not a measurement, so nothing was compared:" \ "got [$got] want [$want]" @@ -372,7 +372,7 @@ if python3 "$STATS_PY" "$NANQ" > "$NS" 2>&1; then else echo "FAIL the footer parser could not read the NaN fixture:" sed 's/^/ /' "$NS" - PGC_FAIL=1; PGC_CHECKS=$((PGC_CHECKS + 1)) + PGC_FAIL=1; PGC_CHECKS=$((PGC_CHECKS + 1)); PGC_FAILED=$((PGC_FAILED + 1)) fi # ---- (f) column_orders, without which the bounds have no defined meaning ---- diff --git a/test/pg19_vacuum_options.sh b/test/pg19_vacuum_options.sh index 8fa5399a..6cafd0aa 100755 --- a/test/pg19_vacuum_options.sh +++ b/test/pg19_vacuum_options.sh @@ -40,7 +40,7 @@ srv="$(q 'SHOW server_version_num')" # on an older major depends on it being 0. Asserting the premise must not destroy # the skip it guards. if ! pgc_is_number "$srv"; then - PGC_CHECKS=$((PGC_CHECKS + 1)) + PGC_CHECKS=$((PGC_CHECKS + 1)); PGC_FAILED=$((PGC_FAILED + 1)) PGC_FAIL=1 echo "FAIL the server did not answer 'SHOW server_version_num': got [$srv]" pgc_summary diff --git a/test/projections.sh b/test/projections.sh index ad5a519c..90e095e6 100755 --- a/test/projections.sh +++ b/test/projections.sh @@ -21,12 +21,13 @@ pgc_setup "${1:-/usr/local/pg17/bin/pg_config}" # Run a statement expected to FAIL; PASS the check when it errors out. expect_fail() { local name="$1" sql="$2" - PGC_CHECKS=$((PGC_CHECKS + 1)) + # pgc_pass/pgc_fail rather than touching PGC_CHECKS: the counters are lib.sh's + # invariant and pgc_summary reconciles them. This helper counted ten checks + # per run and recorded no outcome for any of them. if psql_run "$sql" >/dev/null 2>&1; then - echo "FAIL $name: statement unexpectedly succeeded" - PGC_FAIL=1 + pgc_fail "$name" "statement unexpectedly succeeded" else - echo "PASS $name" + pgc_pass "$name" fi } diff --git a/test/selftest/320-a-check-that-could-not-run.sh b/test/selftest/320-a-check-that-could-not-run.sh index 7e1e9140..6f36aed1 100644 --- a/test/selftest/320-a-check-that-could-not-run.sh +++ b/test/selftest/320-a-check-that-could-not-run.sh @@ -27,6 +27,8 @@ # in the log, because `set -e` can propagate any status an aborting command # returns. +_tsm_fmt_cnt() { [ "$1" -eq 0 ] && { echo "[]"; return; }; echo "[$1:$2]"; } + _cur_lib="$PGC_TESTDIR/lib.sh" check "premise: the harness library is where this part thinks it is" \ @@ -161,9 +163,15 @@ check "and the suite that holds it still passes" \ # A drifting counter must be visible. Pass a check through a helper that counts # the check but records neither outcome, which is exactly what check_ratio did. -_cur_make drift 'check "a" ok ok -PGC_CHECKS=$((PGC_CHECKS + 1)) -echo "PASS a check nothing counted"' +# +# The forbidden line is ASSEMBLED rather than written, because the sweep below +# greps the tree for exactly this shape and a fixture that spells it out is +# indistinguishable from the defect. A test for a pattern must not contain the +# pattern -- the sweep found this fixture on its first run and was right to. +_cur_drift="$(printf 'PGC_%s=$((PGC_%s + 1))' CHECKS CHECKS)" +_cur_make drift "check \"a\" ok ok +$_cur_drift +echo \"PASS a check nothing counted\"" check "a counter that drifts is caught rather than absorbed" \ "$(_cur_out drift | grep -c '^FAIL the summary does not reconcile')" "1" @@ -171,5 +179,49 @@ check "a counter that drifts is caught rather than absorbed" \ check "and the suite holding it fails rather than reporting PASSED" \ "$(_cur_run drift)" "1 FAILED" +# ---- the counters are lib.sh's invariant, and only lib.sh may write them ----- +# +# The reconciliation above turns PGC_CHECKS into an invariant that pgc_summary +# checks. A suite that bumps it directly and prints its own PASS leaves the +# totals short, and the reconciliation then reds a HEALTHY tree -- worse than the +# miscount it exists to find. projections.sh did exactly that: an expect_fail() +# with ten call sites, counting checks whose outcome nothing recorded, invisible +# for as long as it existed because nothing reconciled the totals. +# +# Fixing those ten call sites alone would leave the next expect_fail someone +# writes undetectable, which is the same argument that rejected fixing +# check_ratio's counter without a real PGC_FAILED. So the rule is swept: a direct +# write to PGC_CHECKS must record an outcome on the same line or in the lines +# around it, and pgc_pass/pgc_fail exist so a suite-local helper need not. + +_cnt_dir="$PGC_TESTDIR" +_cnt_sites=() +while IFS= read -r _cnt_l; do + _cnt_sites+=("$_cnt_l") +done < <(grep -rn 'PGC_CHECKS=\$((PGC_CHECKS' "$_cnt_dir"/*.sh "$_cnt_dir"/selftest/*.sh 2>/dev/null \ + | grep -v '/lib\.sh:' | sort) + +check "premise: the sweep finds the direct writes it is meant to police" \ + "$([ "${#_cnt_sites[@]}" -ge 5 ] && echo enough || echo "${#_cnt_sites[@]}")" "enough" + +# A file that keeps its OWN counters and never calls pgc_summary is not bound by +# this invariant, because nothing reconciles it. Asserted rather than assumed: +# the exemption is measured from the file, not from a name list. +_cnt_bad=""; _cnt_n=0 +for _cnt_l in "${_cnt_sites[@]}"; do + _cnt_f="${_cnt_l%%:*}" + _cnt_ln="$(printf '%s' "$_cnt_l" | cut -d: -f2)" + grep -q 'pgc_summary' "$_cnt_f" || continue + if ! sed -n "$((_cnt_ln > 3 ? _cnt_ln - 3 : 1)),$((_cnt_ln + 6))p" "$_cnt_f" \ + | grep -qE 'PGC_PASSED=|PGC_FAILED=|PGC_UNRUN='; then + _cnt_n=$((_cnt_n + 1)) + [ "$_cnt_n" -le 5 ] && _cnt_bad="$_cnt_bad ${_cnt_f##*/}:$_cnt_ln" + fi +done + +check "every direct write to PGC_CHECKS records an outcome too" \ + "$(_tsm_fmt_cnt "$_cnt_n" "$_cnt_bad")" "[]" + +unset _cur_drift _cnt_dir _cnt_sites _cnt_l _cnt_f _cnt_ln _cnt_bad _cnt_n unset _cur_lib _cur_dir unset -f _cur_make _cur_run _cur_out From fc6c7cf7a01ffb93651ef2418b1fb1f101e87d6d Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Tue, 1 Sep 2026 10:30:12 -0600 Subject: [PATCH 4/5] test: the runner says INCOMPLETE explicitly, and the selftest evals its rule (#858) Item 3 of the review, which I had deferred to phase 2 and the owner asked to be done here. lib.sh exiting 67 was only half the state. The RUNNER decides what a status means, and 67 reached that decision through a catch-all else: safe by accident, asserted nowhere, and with the template for breaking it three lines above -- copy the 66 branch and an INCOMPLETE suite becomes a SKIP the matrix reports green. run_all_versions.sh now classifies explicitly, and gives 67 the same two-signal discipline 66 has. `set -e` propagates whatever status an aborting command returned, so a bare code is never believed on its own: 66 needs its SKIPPED line and 67 needs its INCOMPLETE line. An INCOMPLETE suite prints its UNRUN lines, counts as having RUN, and sets MAJOR_FAIL, so it cannot be reported green. THE CLASSIFICATION IS A FUNCTION SO THE SELFTEST CAN EVAL THE REAL TEXT. pgc_classify_suite_rc RC LOGFILE -> PASS|SKIP|INCOMPLETE|FAIL, and the selftest seds it out of run_all_versions.sh and evals it rather than restating the rule. A check that recomputes a condition tests the world instead of the code, which is what selftest 070 learned when a premise globbed bench/*.sh to prove bench/ was swept -- that asserts the directory EXISTS, not that the sweep read it. Eight arms, including the two that would catch the failure mode this fixes: the runner calls a clean exit a pass and 66 with its line a skip and 67 with its line INCOMPLETE, which is not a pass 67 without its line is a failure, not an INCOMPLETE taken on trust and an ordinary failure is still a failure no non-zero status is classified as a pass The last is the invariant stated directly: 1, 2, 66, 67, 126, 127 and 130 all run through the classifier and none may come back PASS. harness_selftest 221 checks 221 passed + 0 failed + 0 unrunnable = 221 projections 64 checks 64 passed + 0 failed + 0 unrunnable = 64 --- test/run_all_versions.sh | 38 +++++++++++++- .../320-a-check-that-could-not-run.sh | 52 +++++++++++++++++++ 2 files changed, 88 insertions(+), 2 deletions(-) diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index 21092abf..d80752e3 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -754,14 +754,48 @@ for pgc in "${CONFIGS[@]}"; do # collect results in suite order for a stable, readable summary suites_ran=0 suites_skipped=0 +# Classify one suite's exit status. A function, not four inline branches, +# because the selftest evals THIS TEXT rather than re-deriving the condition: a +# check that recomputes a rule tests the world instead of the code. +# +# Two independent signals for every non-pass state, which is why 66 was chosen +# in the first place -- `set -e` propagates whatever status an aborting command +# returned, so a bare code can be produced by accident. 67 gets the same +# treatment: the code AND the line. +pgc_classify_suite_rc() { # pgc_classify_suite_rc RC LOGFILE -> PASS|SKIP|INCOMPLETE|FAIL + local rc="$1" log="$2" + if [ "$rc" = 0 ]; then + echo PASS + elif [ "$rc" = 66 ] && grep -q 'SKIPPED (ran no checks)' "$log" 2>/dev/null; then + echo SKIP + elif [ "$rc" = 67 ] && grep -q ': INCOMPLETE$' "$log" 2>/dev/null; then + # A check could not be evaluated (#858). NOT a pass: the suite reached a + # question it could not ask. Not a plain FAIL either, because nothing + # asserted false -- but it must never reach the PASS branch, and the + # reason travels with it. + echo INCOMPLETE + else + echo FAIL + fi +} + skipped_names="" + suites_incomplete=${suites_incomplete:-0} for s in "${SUITES[@]}"; do _rc="$(cat "$builddir/${s}.rc" 2>/dev/null)" - if [ "$_rc" = 0 ]; then + _verdict="$(pgc_classify_suite_rc "$_rc" "$builddir/${s}.log")" + if [ "$_verdict" = PASS ]; then echo " PASS $s" results+="$s=PASS " suites_ran=$((suites_ran + 1)) - elif [ "$_rc" = 66 ] && grep -q 'SKIPPED (ran no checks)' "$builddir/${s}.log" 2>/dev/null; then + elif [ "$_verdict" = INCOMPLETE ]; then + echo " INCOMPLETE $s (a check could not be evaluated)" + grep -E '^UNRUN' "$builddir/${s}.log" | sed 's/^/ >> /' + results+="$s=INCOMPLETE " + suites_ran=$((suites_ran + 1)) + suites_incomplete=$((suites_incomplete + 1)) + MAJOR_FAIL=1 + elif [ "$_verdict" = SKIP ]; then # Exit 2 is pgc_summary's third state: the suite ran no checks (#447). # Not a pass, because it asserted nothing. Not a failure, because a # major without the feature and a box without an optional dependency diff --git a/test/selftest/320-a-check-that-could-not-run.sh b/test/selftest/320-a-check-that-could-not-run.sh index 6f36aed1..f7ba556e 100644 --- a/test/selftest/320-a-check-that-could-not-run.sh +++ b/test/selftest/320-a-check-that-could-not-run.sh @@ -222,6 +222,58 @@ done check "every direct write to PGC_CHECKS records an outcome too" \ "$(_tsm_fmt_cnt "$_cnt_n" "$_cnt_bad")" "[]" +# ---- and the RUNNER must not report an INCOMPLETE suite as a pass ------------ +# +# lib.sh exiting 67 is only half the state. The runner decides what a status +# MEANS, and until now 67 reached that decision through a catch-all else: safe +# by accident, unasserted, and with the template for breaking it three lines +# above -- copy the 66 branch, and an INCOMPLETE suite becomes a SKIP that the +# matrix reports green. +# +# The classifier is EVALLED OUT OF run_all_versions.sh rather than re-stated +# here. A check that recomputes the rule tests the world instead of the code: +# selftest 070 learned that when a premise globbed bench/*.sh to prove bench/ was +# swept, which asserts the directory EXISTS and not that the sweep read it. + +_rv="$PGC_TESTDIR/run_all_versions.sh" +check "premise: the runner defines the classifier this part is about to eval" \ + "$(grep -c '^pgc_classify_suite_rc()' "$_rv")" "1" + +eval "$(sed -n '/^pgc_classify_suite_rc()/,/^}/p' "$_rv")" +check "premise: the classifier evalled out of the runner is callable" \ + "$(type -t pgc_classify_suite_rc)" "function" + +_rvlog="$PGC_WORKDIR/rv.log" + +: > "$_rvlog" +check "the runner calls a clean exit a pass" \ + "$(pgc_classify_suite_rc 0 "$_rvlog")" "PASS" + +printf 'x.sh: SKIPPED (ran no checks)\n' > "$_rvlog" +check "and 66 with its line a skip" \ + "$(pgc_classify_suite_rc 66 "$_rvlog")" "SKIP" + +printf 'x.sh: INCOMPLETE\n' > "$_rvlog" +check "and 67 with its line INCOMPLETE, which is not a pass" \ + "$(pgc_classify_suite_rc 67 "$_rvlog")" "INCOMPLETE" + +# Two signals, the same discipline 66 has: a bare status can be produced by any +# aborting command under set -e, so the code alone must not be believed. +printf 'x.sh: PASSED\n' > "$_rvlog" +check "67 without its line is a failure, not an INCOMPLETE taken on trust" \ + "$(pgc_classify_suite_rc 67 "$_rvlog")" "FAIL" + +printf 'x.sh: FAILED\n' > "$_rvlog" +check "and an ordinary failure is still a failure" \ + "$(pgc_classify_suite_rc 1 "$_rvlog")" "FAIL" + +# The whole point, stated as its own arm: no status reaches PASS except 0. +check "no non-zero status is classified as a pass" \ + "$(for _rvrc in 1 2 66 67 126 127 130; do pgc_classify_suite_rc "$_rvrc" "$_rvlog"; done | grep -c '^PASS$')" \ + "0" + +unset _rv _rvlog _rvrc +unset -f pgc_classify_suite_rc unset _cur_drift _cnt_dir _cnt_sites _cnt_l _cnt_f _cnt_ln _cnt_bad _cnt_n unset _cur_lib _cur_dir unset -f _cur_make _cur_run _cur_out From 8673bfbed58bad28a9ff60b615dffd37ff46b854 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Tue, 1 Sep 2026 10:36:20 -0600 Subject: [PATCH 5/5] test: an INCOMPLETE suite fails its major, and the dispatch is asserted (#858) fc6c7cf was a REGRESSION and review caught it. Before that commit, exit 67 fell to the catch-all else, which sets verfail=1, so an INCOMPLETE suite failed its major BY ACCIDENT. fc6c7cf routed it explicitly to a branch that set a write-only flag -- assigned once, read nowhere -- while the major verdict reads verfail. Making the state explicit turned a gate failure into a gate pass. verdict=PASS -> verfail=0 -> MAJOR: PASS verdict=SKIP -> verfail=0 -> MAJOR: PASS verdict=INCOMPLETE -> verfail=0 -> MAJOR: PASS the regression verdict=FAIL -> verfail=1 -> MAJOR: FAIL THE EIGHT ARMS IN fc6c7cf COULD NOT SEE IT, AND THAT IS THE LESSON. They test pgc_classify_suite_rc, which was right: 67 classified as INCOMPLETE, and no non-zero status ever returning PASS. The defect was in what the CALLER did with the verdict. Testing a function and not its caller is how a correct classification gets computed and thrown away. So the mapping from verdict to gate outcome is now its own function that the loop CALLS -- pgc_verdict_fails_major VERDICT -> yes|no -- and selftest 320 evals that text too, the same way it evals the classifier. Seven new arms, including the wiring rather than only the rule: an INCOMPLETE suite fails its major and a failing suite still does while a pass does not and a skip does not, which is the one that must stay true the runner's INCOMPLETE branch calls the mapping rather than a local flag and no write-only failure flag survives in the runner SECOND DEAD VARIABLE, SAME BRANCH: suites_incomplete was incremented and never printed. The per-major line now reads "(N ran, N skipped, N incomplete)" in both the PASS and FAIL summaries. A state that is not in a total is a state that can go missing -- the lib.sh version of that argument is two commits back, and this is the same argument one level up. The write-only-flag arm caught its own comment on the first run: the paragraph explaining the bug spelled the assignment out, so the grep matched the explanation. Reworded to name the flag without the assignment. Third time in this branch that a guard has flagged the text that describes it, and the rule is the same each time -- a test for a pattern must not contain the pattern. harness_selftest 228 checks 228 passed + 0 failed + 0 unrunnable = 228 projections 64 checks 64 passed + 0 failed + 0 unrunnable = 64 --- test/run_all_versions.sh | 26 +++++++++++-- .../320-a-check-that-could-not-run.sh | 38 +++++++++++++++++++ 2 files changed, 60 insertions(+), 4 deletions(-) diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index d80752e3..28d85fa0 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -762,6 +762,24 @@ for pgc in "${CONFIGS[@]}"; do # in the first place -- `set -e` propagates whatever status an aborting command # returned, so a bare code can be produced by accident. 67 gets the same # treatment: the code AND the line. +# Does a suite verdict fail its major? +# +# Split out for the same reason the classifier was: the selftest evals THIS TEXT. +# The first version of the INCOMPLETE branch set a write-only MAJOR_FAIL flag, +# assigned once and read nowhere, so a state that had been failing the gate by +# accident (67 fell to the else, which sets verfail=1) was routed explicitly to a +# branch that could not fail it. The classifier was right and the dispatch threw +# the answer away, which is why this is a function and not a line in a branch. +# +# The comment names the flag without spelling the assignment, because the arm in +# selftest 320 greps for it: a test for a pattern must not contain the pattern. +pgc_verdict_fails_major() { # pgc_verdict_fails_major VERDICT -> yes|no + case "$1" in + PASS|SKIP) echo no ;; + *) echo yes ;; + esac +} + pgc_classify_suite_rc() { # pgc_classify_suite_rc RC LOGFILE -> PASS|SKIP|INCOMPLETE|FAIL local rc="$1" log="$2" if [ "$rc" = 0 ]; then @@ -794,7 +812,7 @@ pgc_classify_suite_rc() { # pgc_classify_suite_rc RC LOGFILE -> PASS|SKIP|INCOMP results+="$s=INCOMPLETE " suites_ran=$((suites_ran + 1)) suites_incomplete=$((suites_incomplete + 1)) - MAJOR_FAIL=1 + [ "$(pgc_verdict_fails_major "$_verdict")" = yes ] && verfail=1 elif [ "$_verdict" = SKIP ]; then # Exit 2 is pgc_summary's third state: the suite ran no checks (#447). # Not a pass, because it asserted nothing. Not a failure, because a @@ -841,7 +859,7 @@ pgc_classify_suite_rc() { # pgc_classify_suite_rc RC LOGFILE -> PASS|SKIP|INCOMP # report a verdict without running a check when pyarrow is absent, and the old # per-version line counted them among the passes. A count that includes suites # nobody ran is the thing this project keeps having to unlearn. - echo " suites that ran: $suites_ran of ${#SUITES[@]} (skipped: $suites_skipped)" + echo " suites that ran: $suites_ran of ${#SUITES[@]} (skipped: $suites_skipped, incomplete: $suites_incomplete)" if [ "$suites_skipped" != 0 ]; then echo " skipped:${skipped_names}" fi @@ -851,9 +869,9 @@ pgc_classify_suite_rc() { # pgc_classify_suite_rc RC LOGFILE -> PASS|SKIP|INCOMP fi if [ "$verfail" = 0 ]; then - SUMMARY+=("PASS PG$major ($suites_ran ran, $suites_skipped skipped) ${results}") + SUMMARY+=("PASS PG$major ($suites_ran ran, $suites_skipped skipped, $suites_incomplete incomplete) ${results}") else - SUMMARY+=("FAIL PG$major ($suites_ran ran, $suites_skipped skipped) ${results}") + SUMMARY+=("FAIL PG$major ($suites_ran ran, $suites_skipped skipped, $suites_incomplete incomplete) ${results}") overall=1 fi rm -rf "$builddir" diff --git a/test/selftest/320-a-check-that-could-not-run.sh b/test/selftest/320-a-check-that-could-not-run.sh index f7ba556e..82b62f75 100644 --- a/test/selftest/320-a-check-that-could-not-run.sh +++ b/test/selftest/320-a-check-that-could-not-run.sh @@ -272,6 +272,44 @@ check "no non-zero status is classified as a pass" \ "$(for _rvrc in 1 2 66 67 126 127 130; do pgc_classify_suite_rc "$_rvrc" "$_rvlog"; done | grep -c '^PASS$')" \ "0" +# ---- and the DISPATCH must act on the verdict, not merely compute it -------- +# +# The eight arms above test the classifier. The classifier was right and the +# caller threw the answer away: the first INCOMPLETE branch set MAJOR_FAIL=1, a +# variable written once and read nowhere, while the major verdict reads verfail. +# So a state that had been failing the gate BY ACCIDENT -- 67 fell to the else, +# which sets verfail=1 -- was routed explicitly to a branch that could not fail +# it. A regression, introduced by the commit that made the state explicit. +# +# Testing a function and not its caller is how that survives review. The mapping +# is now its own function that the loop CALLS, and this evals that text too. + +eval "$(sed -n '/^pgc_verdict_fails_major()/,/^}/p' "$_rv")" +check "premise: the major-verdict mapping evalled out of the runner is callable" \ + "$(type -t pgc_verdict_fails_major)" "function" + +check "an INCOMPLETE suite fails its major" \ + "$(pgc_verdict_fails_major INCOMPLETE)" "yes" + +check "and a failing suite still does" \ + "$(pgc_verdict_fails_major FAIL)" "yes" + +check "while a pass does not" \ + "$(pgc_verdict_fails_major PASS)" "no" + +check "and a skip does not, which is the one that must stay true" \ + "$(pgc_verdict_fails_major SKIP)" "no" + +# The dispatch is the thing that was wrong, so assert the runner CALLS it in the +# INCOMPLETE branch rather than setting some variable of its own. Not a +# re-derivation of the rule: the rule is evalled above. This asserts the wiring. +check "the runner's INCOMPLETE branch calls the mapping rather than a local flag" \ + "$(grep -c 'pgc_verdict_fails_major "\$_verdict"' "$_rv")" "1" + +check "and no write-only failure flag survives in the runner" \ + "$(grep -c 'MAJOR_FAIL=' "$_rv")" "0" + +unset -f pgc_verdict_fails_major unset _rv _rvlog _rvrc unset -f pgc_classify_suite_rc unset _cur_drift _cnt_dir _cnt_sites _cnt_l _cnt_f _cnt_ln _cnt_bad _cnt_n