From 44e2e9b95c6c2f0957361e5a60135ee9fd920aaf Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Wed, 9 Sep 2026 20:21:43 -0600 Subject: [PATCH 1/8] test: the matrix must reconcile registered suites against accounted ones (#916) `run_all_versions.sh` printed `suites that ran: N of M` and never checked it, and ten registered suites exit 0 having never counted a check. `pgc_classify_suite_rc` maps rc=0 to PASS with no further question, so those ten are counted among the suites that "ran" -- the overcount #447 added that line to stop, one level further down. A count cannot close this. Two errors of opposite sign cancel, and an exempt list maintained by hand makes the count agree by construction: the check then measures the list rather than the run. A total derived from the collect loop is worse still, because that loop visits every registered name and any sum over it is an identity. So membership is derived from a property each suite carries, and two readings taken from different places are reconciled as SETS, in both directions: declared the suite's own text calls pgc_summary observed the suite's log carries the `accounting:` line pgc_summary prints before every one of its four exit paths Neither is a number and neither is hand-maintained. A suite that stops calling pgc_summary moves between the sets on its own, and the two directions catch opposite mistakes: declared-but-not-accounted is a suite that died before it could account, and accounted-but-not-declared is a stale reading of the source. A third term is the driver's own record of suites it chose not to dispatch. PGC_SKIP_TIMING drops four on every CI run; they declare accounting and correctly produce none. The record is written by the branch that makes the decision, not inferred from the log that branch forges, and a suite in both that record and the observed set is reported as its own distinct fault. `checks run: 251` is not what discriminates: bench_guards, docs_style and pg_upgrade print their own. `accounting:` is produced by pgc_summary and by nothing else in the tree. Removal proofs, each measured rather than argued: strip the comment-stripping from the declaration reader -> "a comment mentioning pgc_summary is not a declaration" reddens delete the record from the skip branch -> "the skip branch records the suite it did not dispatch" reddens drop the sort before comm -> "the identity catches comm reading unsorted input" reddens, 20/20 compute inputs FROM the buckets instead of the files -> NOTHING reddens, which is why inputs is counted by a separate route and why that is said out loud in both harnesses The declaration reader's first version piped sed into `grep -q`, and this suite runs under `set -o pipefail`. grep -q exits the moment it matches, sed takes EPIPE, and pipefail reports the pipeline as failed even though grep matched -- so a suite that plainly calls pgc_summary read as not declaring accounting. It is a race, so it passed every fixture and failed only on the real population, naming the two longest suites, analyze_function and hilbert_curve. Selftest 040 carries the same story from #473 and #476. The fix is grep -c, which reads to EOF; a regression arm in both harnesses pins it with a 40,000-line fixture and shows the grep -q shape still gets it wrong there while agreeing on a short file. Both harnesses, one implementation: the pytest half drives the shell functions out of run_all_versions.sh rather than reimplementing them, because a Python twin would agree with itself. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK --- test/pytest/TESTS.md | 92 ++++- test/pytest/test_suite_accounting.py | 281 ++++++++++++++ test/run_all_versions.sh | 172 +++++++++ .../390-a-registered-suite-must-account.sh | 346 ++++++++++++++++++ 4 files changed, 885 insertions(+), 6 deletions(-) create mode 100644 test/pytest/test_suite_accounting.py create mode 100644 test/selftest/390-a-registered-suite-must-account.sh diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index f759f5b3..b93b2cf9 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -59,9 +59,10 @@ behaviour, the source of that number is named. - [11. test_zonemap_boundaries.py: exact boundaries](#11-test_zonemap_boundariespy-exact-boundaries) - [12. test_saop_element_pushdown.py: scattered set pruning](#12-test_saop_element_pushdownpy-scattered-set-pruning) - [13. test_hilbert_locality.py: what the Hilbert curve buys](#13-test_hilbert_localitypy-what-the-hilbert-curve-buys) -- [14. Adding a test](#14-adding-a-test) -- [15. What this corpus does NOT yet refuse](#15-what-this-corpus-does-not-yet-refuse) -- [16. Traps this corpus records](#16-traps-this-corpus-records) +- [14. test_suite_accounting.py: the matrix accounting for its own suites](#14-test_suite_accountingpy-the-matrix-accounting-for-its-own-suites) +- [15. Adding a test](#15-adding-a-test) +- [16. What this corpus does NOT yet refuse](#16-what-this-corpus-does-not-yet-refuse) +- [17. Traps this corpus records](#17-traps-this-corpus-records) ## 1. How to read a test in here @@ -1012,7 +1013,86 @@ pins the eight counts exactly. Its header records why -- for a CURVE change the digest pins upstream catch it first and the integers add nothing, so their real domain is a changed READER at an unchanged layout. -## 14. Adding a test +## 14. test_suite_accounting.py: the matrix accounting for its own suites + +`test_suite_accounting.py` holds the matrix runner to its own arithmetic. + +`run_all_versions.sh` prints `suites that ran: N of M` and never checks it, and ten +registered suites exit 0 having never counted a check. Counted among the suites that +"ran", they are the overcount #447 added that line to stop, one level further down. + +A count cannot close this. Two errors of opposite sign cancel, and an exempt list +maintained by hand makes the count agree by construction -- the check then measures +the list rather than the run. So membership is derived from a property each suite +carries, and the two readings are reconciled as SETS, in both directions: + +| reading | where it comes from | +| --- | --- | +| declared | the suite's own text calls `pgc_summary` | +| observed | the suite's log carries the `accounting:` line `pgc_summary` prints before every exit path | + +Neither is a number and neither is hand-maintained. A suite that stops calling +`pgc_summary` moves between the sets on its own. + +These tests drive the SHELL functions out of `run_all_versions.sh` rather than +reimplementing them in Python. A Python twin would be a second implementation and +would agree with itself; the house rule asks for two observers of one implementation. +They run under `set -o pipefail`, because `harness_selftest.sh` does. + +### `test_a_suite_that_calls_pgc_summary_declares_accounting` + +The property is the CALL. A comment mentioning `pgc_summary`, and a longer name +containing it, are both refused -- a claim satisfied by prose is the failure the whole +design exists to avoid. + +### `test_the_accounting_line_is_read_on_every_exit_path` + +Pass, failure, skip and incomplete all carry the line, which is what makes it the +runtime twin of the declaration rather than a synonym for PASSED. + +### `test_the_reconciliation_names_both_directions` + +Declared-but-not-accounted is a suite that died before reaching its summary; today +that reads PASS whenever the shell happened to exit 0. Accounted-but-not-declared is a +stale reading of the source, which a hand-maintained list can never report. + +### `test_opposite_errors_do_not_cancel` + +Both directions are reported from one run. One error masking the other is exactly what +a count cannot distinguish from correctness. + +### `test_the_driver_s_own_non_dispatch_record_excuses_only_what_it_names` + +`PGC_SKIP_TIMING` drops four suites on every CI run; they declare accounting and +correctly produce none. The driver records that decision where it makes it, rather +than leaving it to be inferred from the log the driver forges. The record excuses only +what it names, and a suite that both accounted and was recorded as never dispatched +fails. + +### `test_the_printed_identity_can_actually_fail` + +`inputs == sum(buckets)` is printed beside every reconciliation. Computing `inputs` +FROM the buckets makes the line true for any values and reddens nothing, which is why +it is counted from the two files by a separate route. Dropping the sort before `comm` +makes the totals diverge, and that is the fault the identity guards. + +### `test_the_partition_over_the_registered_suites_adds_up` + +The readers run over the real registered suite list. No count is asserted: how many +suites are exempt is not a fact about correctness, and pinning it would be a second +copy of the list this design removes. What is asserted is that the partition covers +the population and that both buckets are occupied. + +### `test_the_declaration_reader_survives_pipefail_on_a_long_suite` + +A regression arm. The first implementation piped `sed` into `grep -q`; grep exits on +match, sed takes EPIPE, and `pipefail` reports the pipeline as failed. The reader +answered "no" for a suite that plainly calls `pgc_summary`. It is a race, so it +reproduces on long files and not short ones -- it passed every fixture and failed only +on the real population, naming the two longest suites. Selftest 040 carries the same +story from #473 and #476. + +## 15. Adding a test 0. **Write it twice.** Every test in this tree ships as a `.sh` suite and a pytest test **in the same change** (jd, 2026-09-09). Not ported later, not one or the @@ -1039,7 +1119,7 @@ domain is a changed READER at an unchanged layout. failed the selftest on both majors of the matrix, which is how it was found. A new directory under `test/` inherits every rule the old ones follow. -## 15. What this corpus does NOT yet refuse +## 16. What this corpus does NOT yet refuse `VACUITY_MODES.md` is the inventory: 79 ways a pytest harness can report a pass while asserting nothing, 73 of them demonstrated by an actual run. **This layer refuses 25 @@ -1051,7 +1131,7 @@ Read it before adding a test. The gaps most likely to affect a new test are that same family satisfies it, and that a write is not required to have written anything. Both are named there with the refusal each needs. -## 16. Traps this corpus records +## 17. Traps this corpus records Recorded because each one produced a confident wrong result before it was caught, and all are the same family as the defect the layer exists to prevent. diff --git a/test/pytest/test_suite_accounting.py b/test/pytest/test_suite_accounting.py new file mode 100644 index 00000000..e5b99ee6 --- /dev/null +++ b/test/pytest/test_suite_accounting.py @@ -0,0 +1,281 @@ +"""The matrix must reconcile the suites it registered against the ones that accounted. + +`run_all_versions.sh` prints "suites that ran: N of M" and never checks it, and ten +registered suites exit 0 having never counted a check. Counted among the suites that +"ran", they are the overcount #447 added that line to stop, one level further down. + +A count cannot close this. Two errors of opposite sign cancel, and an exempt list +maintained by hand makes the count agree by construction. So membership is derived +from a property each suite carries -- its own text calls `pgc_summary`, and its log +carries the `accounting:` line `pgc_summary` prints before every exit path -- and the +two readings are reconciled as SETS, in both directions. + +These tests drive the SHELL functions out of `run_all_versions.sh` rather than +reimplementing them in Python. A Python twin would be a second implementation and +would agree with itself; the house rule asks for two observers of one implementation. +""" + +import pathlib +import subprocess + +REPO = pathlib.Path(__file__).resolve().parents[2] +RUNNER = REPO / "test" / "run_all_versions.sh" + + +def _extract(name): + """The text of one shell function, taken from the runner itself.""" + out, keep = [], False + for line in RUNNER.read_text().splitlines(): + if line.startswith(f"{name}() "): + keep = True + if keep: + out.append(line) + if line == "}": + break + return "\n".join(out) + + +def _call(name, *args, mutate=None): + """Run one of the runner's functions and return (stdout, returncode).""" + body = _extract(name) + if mutate: + body = mutate(body) + # `set -o pipefail` is not decoration: harness_selftest.sh runs under it, and a + # pipeline inside one of these functions behaves differently with it on. Driving + # them without it would test a shell the harness never uses. + script = ("set -uo pipefail\n" + body + "\n" + + " ".join([name] + [f'"{a}"' for a in args]) + "\n") + r = subprocess.run(["bash", "-c", script], capture_output=True, text=True) + return r.stdout, r.returncode + + +def _write(tmp_path, name, text): + p = tmp_path / name + p.write_text(text) + return str(p) + + +# ---- the declaration reader ------------------------------------------------- + + +def test_a_suite_that_calls_pgc_summary_declares_accounting(tmp_path, expect): + """The property is the CALL. A mention is not one, and neither is a longer name. + + The easy wrong implementation is a bare grep, and it reads a suite that only + explains why it cannot account as though it does -- a claim satisfied by prose, + which is the failure this whole approach exists to refuse. + """ + calls = _write(tmp_path, "calls.sh", '. lib.sh\ncheck "x" a a\npgc_summary\n') + silent = _write(tmp_path, "silent.sh", ". lib.sh\necho hi\nexit 0\n") + noted = _write(tmp_path, "noted.sh", ". lib.sh\n# cannot call pgc_summary here\nexit 0\n") + longer = _write(tmp_path, "longer.sh", ". lib.sh\npgc_summary_of_something\n") + + expect.text(_call("pgc_suite_declares_accounting", calls)[0].strip(), "yes", + "a suite that calls pgc_summary declares accounting") + expect.text(_call("pgc_suite_declares_accounting", silent)[0].strip(), "no", + "one that never calls it does not") + expect.text(_call("pgc_suite_declares_accounting", noted)[0].strip(), "no", + "a comment mentioning it is not a declaration") + expect.text(_call("pgc_suite_declares_accounting", longer)[0].strip(), "no", + "a longer name containing it is not a declaration") + expect.text(_call("pgc_suite_declares_accounting", str(tmp_path / "gone.sh"))[0].strip(), + "no", "an absent file declares nothing rather than erroring") + + +# ---- the observation reader ------------------------------------------------- + + +def test_the_accounting_line_is_read_on_every_exit_path(tmp_path, expect): + """`pgc_summary` prints it before pass, failure, skip and incomplete alike. + + That is what makes it the runtime twin of the declaration rather than a synonym + for PASSED: a suite that failed still reached its summary and still accounted. + """ + shapes = { + "pass": "accounting: 3 passed + 0 failed + 0 unrunnable = 3\nx.sh: PASSED\n", + "fail": "accounting: 1 passed + 2 failed + 0 unrunnable = 3\nx.sh: FAILED\n", + "skip": "accounting: 0 passed + 0 failed + 0 unrunnable = 0\nx.sh: SKIPPED (ran no checks)\n", + "inc": "accounting: 2 passed + 0 failed + 1 unrunnable = 3\nx.sh: INCOMPLETE\n", + } + for shape, text in shapes.items(): + log = _write(tmp_path, f"{shape}.log", text) + expect.text(_call("pgc_log_shows_accounting", log)[0].strip(), "yes", + f"a {shape} log shows accounting") + + bare = _write(tmp_path, "bare.log", "x.sh: PASSED\n") + prose = _write(tmp_path, "prose.log", "this suite prints accounting: in prose\n") + expect.text(_call("pgc_log_shows_accounting", bare)[0].strip(), "no", + "a PASSED claim without the line shows none") + expect.text(_call("pgc_log_shows_accounting", prose)[0].strip(), "no", + "and prose containing the word is not the line") + expect.text(_call("pgc_log_shows_accounting", str(tmp_path / "gone.log"))[0].strip(), + "no", "an absent log shows none rather than erroring") + + +# ---- the reconciliation ----------------------------------------------------- + + +def test_the_reconciliation_names_both_directions(tmp_path, expect): + """Declared-but-not-accounted and accounted-but-not-declared are opposite faults. + + The first is a suite that died before reaching its summary -- today that reads + PASS whenever the shell happened to exit 0. The second is a stale reading of the + source, which is the failure a hand-maintained exempt list can never report. + """ + d = _write(tmp_path, "d", "alpha\nbeta\ngamma\n") + o = _write(tmp_path, "o", "alpha\nbeta\ngamma\n") + expect.num(_call("pgc_reconcile_accounting", d, o)[1], 0, "equal sets reconcile") + + o = _write(tmp_path, "o", "alpha\nbeta\n") + out, rc = _call("pgc_reconcile_accounting", d, o) + expect.num(rc, 1, "a declared suite that never accounted is caught") + expect.num(out.count("declared but never accounted: gamma"), 1, "and is named") + + d = _write(tmp_path, "d", "alpha\nbeta\n") + o = _write(tmp_path, "o", "alpha\nbeta\ngamma\n") + out, rc = _call("pgc_reconcile_accounting", d, o) + expect.num(rc, 1, "an undeclared suite that DID account is caught") + expect.num(out.count("accounted but never declared: gamma"), 1, + "and is named as the opposite fault") + + +def test_opposite_errors_do_not_cancel(tmp_path, expect): + """One error masking the other is exactly what a count cannot distinguish.""" + d = _write(tmp_path, "d", "alpha\ndelta\n") + o = _write(tmp_path, "o", "alpha\ngamma\n") + out, rc = _call("pgc_reconcile_accounting", d, o) + expect.num(rc, 1, "a run wrong in both directions fails") + expect.num(out.count("declared but never accounted: delta") + + out.count("accounted but never declared: gamma"), 2, + "and both directions are reported, not one") + + +def test_the_driver_s_own_non_dispatch_record_excuses_only_what_it_names(tmp_path, expect): + """PGC_SKIP_TIMING drops four suites on every CI run. + + They declare accounting and correctly produce none, because nothing ran them. + Without a term for that the check goes red for the one reason that is not a + defect. The term is recorded by the branch that makes the decision rather than + inferred from the log that branch forges. + """ + d = _write(tmp_path, "d", "alpha\nbeta\ngamma\n") + o = _write(tmp_path, "o", "alpha\nbeta\n") + nd = _write(tmp_path, "nd", "gamma\n") + expect.num(_call("pgc_reconcile_accounting", d, o, nd)[1], 0, + "a suite the driver never dispatched reconciles") + expect.num(_call("pgc_reconcile_accounting", d, o)[1], 1, + "and without that record the same run is still caught") + + # A suite cannot both have reached its summary and not have been dispatched. + d = _write(tmp_path, "d", "alpha\nbeta\n") + o = _write(tmp_path, "o", "alpha\nbeta\n") + nd = _write(tmp_path, "nd", "beta\n") + out, rc = _call("pgc_reconcile_accounting", d, o, nd) + expect.num(rc, 1, "a suite both accounted and recorded as never dispatched fails") + expect.num(out.count("both accounted and recorded as never dispatched: beta"), 1, + "and is named as that fault rather than one of the other two") + + d = _write(tmp_path, "d", "alpha\n") + o = _write(tmp_path, "o", "alpha\n") + nd = _write(tmp_path, "nd", "zeta\n") + out, _ = _call("pgc_reconcile_accounting", d, o, nd) + expect.num(out.count("accounted but never declared: zeta"), 1, + "the record cannot introduce a suite the source never declared") + + +def test_the_printed_identity_can_actually_fail(tmp_path, expect): + """inputs == sum(buckets) is printed beside every reconciliation. Can it be false? + + Measured rather than argued. Computing inputs FROM the buckets makes the line + P + D + O == P + D + O and nothing reddens -- which is why inputs is counted from + the two files by a separate route. Dropping the sort before comm makes the buckets + garbage and the totals diverge, and that is the fault this identity guards: + selftest 070's subject arriving in a second place. + """ + unsorted = lambda b: (b + .replace('LC_ALL=C sort -u "$_decl" 2>/dev/null', + 'cat "$_decl" 2>/dev/null') + .replace('LC_ALL=C sort -u "$_obs" 2>/dev/null', + 'cat "$_obs" 2>/dev/null')) + + body = _extract("pgc_reconcile_accounting") + expect.num(body.count('LC_ALL=C sort -u "$_decl"'), 1, + "premise: the real function sorts its declared side") + expect.num(unsorted(body).count('LC_ALL=C sort -u "$_decl"'), 0, + "premise: the mutation applied -- the twin no longer does") + + d = _write(tmp_path, "d", "gamma\nbeta\nalpha\n") + o = _write(tmp_path, "o", "delta\ngamma\nbeta\n") + out_twin, _ = _call("pgc_reconcile_accounting", d, o, mutate=unsorted) + out_real, _ = _call("pgc_reconcile_accounting", d, o) + expect.num(out_twin.count("does not add up"), 1, + "the identity catches comm reading unsorted input") + expect.num(out_real.count("does not add up"), 0, + "and does not fire on the same input unmutated, so the arm is not noise") + + +# ---- the readers, over the real population ---------------------------------- + + +def test_the_partition_over_the_registered_suites_adds_up(expect): + """A reader that works on fixtures and not on the 251 registered suites has been + tested against the world it was written for. + + No count is asserted. How many suites are exempt is not a fact about correctness, + and pinning it here would be a second copy of the hand-maintained list this design + exists to remove. What is asserted is that the partition covers the population and + that both buckets are occupied -- a reader answering the same way for everything + would satisfy every fixture above. + """ + listed = subprocess.run(["bash", str(RUNNER), "--list-suites"], + capture_output=True, text=True).stdout.split() + expect.at_least(len(listed), 1, "premise: the runner listed its suites") + + body = _extract("pgc_suite_declares_accounting") + script = body + '\nfor f in "$@"; do pgc_suite_declares_accounting "$f"; done\n' + paths = [str(REPO / "test" / f"{s}.sh") for s in listed] + verdicts = subprocess.run(["bash", "-c", script, "_"] + paths, + capture_output=True, text=True).stdout.split() + + yes, no = verdicts.count("yes"), verdicts.count("no") + print(f" registered={len(listed)} | declares={yes}, does not={no} | sum={yes + no}") + expect.num(yes + no, len(listed), "the partition covers every registered suite") + expect.at_least(no, 1, "the reader does not answer yes for every suite") + expect.at_least(yes, 1, "nor no for every one of them") + + +def test_the_declaration_reader_survives_pipefail_on_a_long_suite(tmp_path, expect): + """A regression arm for a bug this file's first implementation shipped. + + It piped `sed` into `grep -q`. grep -q exits the moment it matches, closing the + pipe while sed is still writing; sed takes EPIPE, and under `set -o pipefail` the + pipeline reports that failure even though grep matched. The reader then answered + "no" for a suite that plainly calls pgc_summary. + + It is a race, so it reproduces on long files and not short ones -- which is why it + passed every fixture above and failed only on the real population, naming the two + longest suites. Selftest 040 carries the same story from #473 and #476. + """ + big = tmp_path / "big.sh" + big.write_text(". lib.sh\npgc_summary\n" + "".join(f"echo padding {i}\n" for i in range(40000))) + expect.at_least(len(big.read_text().splitlines()), 10000, + "premise: the fixture is long enough to lose the race") + + expect.text(_call("pgc_suite_declares_accounting", str(big))[0].strip(), "yes", + "a long suite that calls pgc_summary still declares accounting") + + # The arm can fail: the grep -q shape is the one that gets this wrong. Restated + # here rather than extracted, because the wrong version is no longer in the tree. + twin = ("set -uo pipefail\n" + "sed 's/#.*$//' \"$1\" | grep -qE '(^|[^_[:alnum:]])pgc_summary([^_[:alnum:]]|$)'" + " && echo yes || echo no\n") + got = subprocess.run(["bash", "-c", twin, "_", str(big)], + capture_output=True, text=True).stdout.strip() + expect.text(got, "no", "the grep -q shape is the one that gets this wrong under pipefail") + + short = tmp_path / "short.sh" + short.write_text(". lib.sh\npgc_summary\n") + got_short = subprocess.run(["bash", "-c", twin, "_", str(short)], + capture_output=True, text=True).stdout.strip() + expect.text(got_short, "yes", + "and it agrees on a SHORT file, which is why it survived review") diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index 084319d3..bfdfe2e0 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -782,6 +782,11 @@ for pgc in "${CONFIGS[@]}"; do # else would produce one and the run would be classified a failure. echo 66 >"$builddir/${s}.rc" echo "$s.sh: SKIPPED (ran no checks)" >"$builddir/${s}.log" + # Record the decision where it is made (#916). This suite calls + # pgc_summary and will produce no accounting line, because it was + # never run; the reconciliation needs that said by the driver rather + # than guessed from the log the driver just forged. + printf '%s\n' "$s" >>"$builddir/accounting.notdispatched" continue fi port=$((BASE_PORT++)) @@ -853,6 +858,151 @@ pgc_classify_suite_rc() { # pgc_classify_suite_rc RC LOGFILE -> PASS|SKIP|INCOMP # would leave every count at zero while every arm that drives this function # still passed -- the same shape of defect as the write-only flag, and just as # invisible to a green run. +# ---- accounting membership, derived rather than listed ----------------------- +# +# The matrix prints "suites that ran: N of M" and never checks it, and ten +# registered suites exit 0 having never counted a check. They assert things in +# their own way; the harness simply cannot see their checks. Counted among the +# suites that "ran", they are the same overcount #447 added that line to stop, +# one level further down. +# +# A COUNT cannot fix this. Two errors of opposite sign cancel, and an exempt list +# maintained by hand makes the count agree by construction -- the check then +# measures the list rather than the run. So membership is derived from a property +# each suite carries, and the two readings are reconciled as SETS, in both +# directions. + +pgc_suite_declares_accounting() { # pgc_suite_declares_accounting FILE -> yes|no + # A suite participates in check accounting exactly when it calls pgc_summary, + # which is the only thing that prints the accounting line and sets the status + # pgc_classify_suite_rc reads. + # + # Comments are stripped first. A suite that explains in prose why it cannot + # account would otherwise read as one that does, which is the failure mode + # this whole approach exists to avoid: a claim satisfied by a mention. + # grep -c, NOT grep -q, and the reason is the bug this harness has already + # paid for once. `grep -q` exits the moment it matches, which closes the pipe + # while sed is still writing; sed takes EPIPE and exits non-zero, and under + # `set -o pipefail` the PIPELINE reports that failure even though grep + # matched. It is a race between the two, so it reproduces on large files and + # not small ones, and it names DIFFERENT innocent suites each run. + # + # That is not a hypothetical. Selftest 040 carries the same story from #473 + # and #476, and the first version of this function reproduced it exactly: + # analyze_function and hilbert_curve -- two of the longest suites -- read as + # not declaring accounting inside the selftest and as declaring it outside. + # + # grep -c reads to EOF, so sed never sees a closed pipe. + local _f="$1" _n + [ -f "$_f" ] || { echo no; return 0; } + _n="$(sed 's/#.*$//' "$_f" \ + | grep -cE '(^|[^_[:alnum:]])pgc_summary([^_[:alnum:]]|$)' || true)" + if [ "${_n:-0}" -gt 0 ]; then + echo yes + else + echo no + fi +} + +pgc_log_shows_accounting() { # pgc_log_shows_accounting LOGFILE -> yes|no + # The runtime twin of the declaration above. pgc_summary prints this line on + # EVERY exit path -- pass, failure, skip and incomplete -- before it decides + # the status, so its presence says "this suite reached its summary" and not + # "this suite passed". Anchored and fully shaped, so the word appearing in a + # suite's own prose cannot satisfy it. + local _log="$1" + [ -f "$_log" ] || { echo no; return 0; } + if grep -qE '^accounting: [0-9]+ passed \+ [0-9]+ failed \+ [0-9]+ unrunnable = [0-9]+$' "$_log"; then + echo yes + else + echo no + fi +} + +pgc_reconcile_accounting() { # pgc_reconcile_accounting DECLARED OBSERVED [NOTDISPATCHED] -> 0 ok, 1 asymmetric + # Set equality in both directions. The two directions catch opposite + # mistakes and neither can stand in for the other: + # + # declared but never accounted the suite died before reaching its + # summary. Today rc=0 makes that a PASS. + # accounted but never declared the reading of the source is stale. + # + # comm needs both sides sorted under the same collation; selftest 070 is the + # record of what an unsorted input costs here. + # The third list is the driver's own record of suites it chose not to + # dispatch -- PGC_SKIP_TIMING drops four on every CI run. Those suites DO + # declare accounting and correctly produced none, so without this term the + # reconciliation goes red for the one reason that is not a defect. + # + # It is recorded by the branch that makes the decision, not inferred from the + # log that branch forges. Inferring it would mean trusting a marker the driver + # wrote on the suite's behalf, which is the kind of claim this check exists to + # stop. + local _decl="$1" _obs="$2" _nd="${3:-}" _rc=0 + local _dfile _ofile _ndfile _obsonly _both _donly _oonly _clash + local _nboth _ndonly _noonly _nclash _inputs _sum _n + + _dfile="$(mktemp)"; _ofile="$(mktemp)"; _ndfile="$(mktemp)"; _obsonly="$(mktemp)" + LC_ALL=C sort -u "$_decl" 2>/dev/null | sed '/^$/d' >"$_dfile" + LC_ALL=C sort -u "$_obs" 2>/dev/null | sed '/^$/d' >"$_obsonly" + if [ -n "$_nd" ] && [ -f "$_nd" ]; then + LC_ALL=C sort -u "$_nd" 2>/dev/null | sed '/^$/d' >"$_ndfile" + fi + # The observed side is what accounted PLUS what was deliberately not run. + LC_ALL=C sort -u "$_obsonly" "$_ndfile" | sed '/^$/d' >"$_ofile" + + # A suite cannot both have reached its summary and not have been dispatched. + # If it is in both lists one of the two readings is wrong, and the union above + # would hide that by absorbing it. + _clash="$(LC_ALL=C comm -12 "$_obsonly" "$_ndfile")" + _nclash="$(printf '%s' "$_clash" | grep -c . || true)" + + _both="$(LC_ALL=C comm -12 "$_dfile" "$_ofile")" + _donly="$(LC_ALL=C comm -23 "$_dfile" "$_ofile")" + _oonly="$(LC_ALL=C comm -13 "$_dfile" "$_ofile")" + + _nboth="$(printf '%s' "$_both" | grep -c . || true)" + _ndonly="$(printf '%s' "$_donly" | grep -c . || true)" + _noonly="$(printf '%s' "$_oonly" | grep -c . || true)" + + # inputs is counted from the FILES, independently of the three buckets. A + # derived total makes the identity below true for any values, which is the + # shape pgc_summary's own comment warns about: three counters reconciled + # against a fourth is the only version of this line that can fail. + _inputs="$(LC_ALL=C sort -u "$_dfile" "$_ofile" | grep -c . || true)" + _sum=$(( _nboth + _ndonly + _noonly )) + rm -f "$_dfile" "$_ofile" "$_ndfile" "$_obsonly" + + if [ "$_nclash" != 0 ]; then + while IFS= read -r _n; do + [ -n "$_n" ] && echo " both accounted and recorded as never dispatched: $_n" + done <<<"$_clash" + _rc=1 + fi + + if [ "$_ndonly" != 0 ]; then + while IFS= read -r _n; do + [ -n "$_n" ] && echo " declared but never accounted: $_n" + done <<<"$_donly" + _rc=1 + fi + if [ "$_noonly" != 0 ]; then + while IFS= read -r _n; do + [ -n "$_n" ] && echo " accounted but never declared: $_n" + done <<<"$_oonly" + _rc=1 + fi + + # Printed from the data on every path, green included, per the house rule + # that any list-derived claim shows inputs == sum(buckets). + echo " accounting reconciliation: inputs=$_inputs | both=$_nboth, declared only=$_ndonly, accounted only=$_noonly | sum=$_sum" + if [ "$_inputs" != "$_sum" ]; then + echo " the reconciliation does not add up: $_inputs names across both lists, $_sum in the buckets" + _rc=1 + fi + return $_rc +} + pgc_tally_suite() { # pgc_tally_suite NAME VERDICT LOGFILE local _name="$1" _verdict="$2" _log="$3" if [ "$_verdict" = PASS ]; then @@ -925,6 +1075,28 @@ pgc_tally_suite() { # pgc_tally_suite NAME VERDICT LOGFILE # 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. + # Reconcile what the suites SAY they account for against what this run + # OBSERVED (#916). Two readings taken from different places -- the suite's own + # text, and the log it produced -- so neither can satisfy the other by + # construction. A count over SUITES could not do this: the collect loop above + # visits every registered name, so any total derived from it is an identity. + _acc_declared="$builddir/accounting.declared" + _acc_observed="$builddir/accounting.observed" + _acc_notdisp="$builddir/accounting.notdispatched" + : >"$_acc_declared" + : >"$_acc_observed" + [ -f "$_acc_notdisp" ] || : >"$_acc_notdisp" + for s in "${SUITES[@]}"; do + [ "$(pgc_suite_declares_accounting "$builddir/test/${s}.sh")" = yes ] \ + && printf '%s\n' "$s" >>"$_acc_declared" + [ "$(pgc_log_shows_accounting "$builddir/${s}.log")" = yes ] \ + && printf '%s\n' "$s" >>"$_acc_observed" + done + if ! pgc_reconcile_accounting "$_acc_declared" "$_acc_observed" "$_acc_notdisp"; then + echo " PG$major cannot account for every registered suite, which is not a pass" + verfail=1 + fi + echo " suites that ran: $suites_ran of ${#SUITES[@]} (skipped: $suites_skipped, incomplete: $suites_incomplete)" if [ "$suites_skipped" != 0 ]; then echo " skipped:${skipped_names}" diff --git a/test/selftest/390-a-registered-suite-must-account.sh b/test/selftest/390-a-registered-suite-must-account.sh new file mode 100644 index 00000000..9d50e959 --- /dev/null +++ b/test/selftest/390-a-registered-suite-must-account.sh @@ -0,0 +1,346 @@ +# ---- a registered suite must account for its checks, or say it cannot ------- +# +# The matrix prints "suites that ran: N of M" and never checks it. Two different +# things hide behind that line. +# +# The first is arithmetic nobody does. ran + skipped + incomplete is printed +# beside M and never compared with it, so a suite whose verdict the tally loop +# drops leaves the sum short and the line still reads plausibly. +# +# The second is worse, because it is live today. pgc_classify_suite_rc maps rc=0 +# to PASS with no further question, and ten registered suites exit 0 having never +# called pgc_summary. They assert things -- concurrency prints its own failure and +# exits non-zero -- but the harness cannot count their checks, and nothing says +# so. They are counted among the suites that "ran", which is the exact overcount +# #447 added that line to stop, one level further down. +# +# So the fix is NOT a number. A count cannot do this job: two errors of opposite +# sign cancel, and an exempt list maintained by hand makes the count agree by +# construction -- the check then measures the list, not the run. +# +# Instead, derive MEMBERSHIP from a property each suite carries, and assert set +# equality in BOTH directions: +# +# declared the suite's own text calls pgc_summary +# observed the suite's log carries the "accounting:" line pgc_summary prints +# on every exit path, before it decides the status +# +# Neither is a number and neither is hand-maintained. A suite that stops calling +# pgc_summary moves between the sets on its own, and the two directions catch +# opposite mistakes: declared-but-not-observed is a suite that died before it +# could account, and observed-but-not-declared is a stale reading of the source. +# +# The functions are EVALLED OUT OF run_all_versions.sh, per selftest 320: a check +# that restates the rule tests the world instead of the code. +# --------------------------------------------------------------------------- + +_rv="$PGC_TESTDIR/run_all_versions.sh" + +check "premise: the runner defines the declaration reader this part evals" \ + "$(grep -c '^pgc_suite_declares_accounting()' "$_rv")" "1" +check "premise: the runner defines the observation reader this part evals" \ + "$(grep -c '^pgc_log_shows_accounting()' "$_rv")" "1" +check "premise: the runner defines the reconciliation this part evals" \ + "$(grep -c '^pgc_reconcile_accounting()' "$_rv")" "1" + +eval "$(sed -n '/^pgc_suite_declares_accounting()/,/^}/p' "$_rv")" +eval "$(sed -n '/^pgc_log_shows_accounting()/,/^}/p' "$_rv")" +eval "$(sed -n '/^pgc_reconcile_accounting()/,/^}/p' "$_rv")" +check "premise: the declaration reader evalled out of the runner is callable" \ + "$(type -t pgc_suite_declares_accounting)" "function" +check "premise: the observation reader evalled out of the runner is callable" \ + "$(type -t pgc_log_shows_accounting)" "function" +check "premise: the reconciliation evalled out of the runner is callable" \ + "$(type -t pgc_reconcile_accounting)" "function" + +_acc="$PGC_WORKDIR/acc"; mkdir -p "$_acc" + +# ---- the declaration reader ------------------------------------------------ + +printf '. "$(dirname "$0")/lib.sh"\ncheck "x" a a\npgc_summary\n' > "$_acc/declares.sh" +check "a suite that calls pgc_summary declares accounting" \ + "$(pgc_suite_declares_accounting "$_acc/declares.sh")" "yes" + +printf '. "$(dirname "$0")/lib.sh"\necho hi\nexit 0\n' > "$_acc/silent.sh" +check "a suite that never calls it does not" \ + "$(pgc_suite_declares_accounting "$_acc/silent.sh")" "no" + +# A MENTION is not a call. The easy wrong implementation is a bare grep, and it +# reads a suite that only explains why it cannot account as though it does. +printf '. "$(dirname "$0")/lib.sh"\n# this suite cannot call pgc_summary: it has no cluster\nexit 0\n' \ + > "$_acc/mentions.sh" +check "a comment mentioning pgc_summary is not a declaration" \ + "$(pgc_suite_declares_accounting "$_acc/mentions.sh")" "no" + +# Nor is a longer name that contains it. +printf '. "$(dirname "$0")/lib.sh"\npgc_summary_of_something\n' > "$_acc/prefix.sh" +check "a longer name containing pgc_summary is not a declaration" \ + "$(pgc_suite_declares_accounting "$_acc/prefix.sh")" "no" + +check "a file that does not exist declares nothing rather than erroring" \ + "$(pgc_suite_declares_accounting "$_acc/absent.sh")" "no" + +# ---- the observation reader ------------------------------------------------ +# +# pgc_summary prints the accounting line before every exit path, so it is present +# on a pass, a failure, a skip and an incomplete alike. That is what makes it the +# runtime twin of the declaration rather than a synonym for PASSED. + +printf 'checks run: 3\nchecks unrunnable: 0\naccounting: 3 passed + 0 failed + 0 unrunnable = 3\nx.sh: PASSED\n' > "$_acc/pass.log" +check "a passing log shows accounting" "$(pgc_log_shows_accounting "$_acc/pass.log")" "yes" + +printf 'accounting: 1 passed + 2 failed + 0 unrunnable = 3\nx.sh: FAILED\n' > "$_acc/fail.log" +check "and so does a failing one, which is the point" \ + "$(pgc_log_shows_accounting "$_acc/fail.log")" "yes" + +printf 'accounting: 0 passed + 0 failed + 0 unrunnable = 0\nx.sh: SKIPPED (ran no checks)\n' > "$_acc/skip.log" +check "and a skip, which reached the summary and counted zero" \ + "$(pgc_log_shows_accounting "$_acc/skip.log")" "yes" + +printf 'accounting: 2 passed + 0 failed + 1 unrunnable = 3\nx.sh: INCOMPLETE\n' > "$_acc/inc.log" +check "and an incomplete" "$(pgc_log_shows_accounting "$_acc/inc.log")" "yes" + +printf 'x.sh: PASSED\n' > "$_acc/bare.log" +check "a log claiming PASSED without the accounting line shows none" \ + "$(pgc_log_shows_accounting "$_acc/bare.log")" "no" + +printf 'this suite prints the word accounting: in prose\n' > "$_acc/prose.log" +check "and prose containing the word does not count as the line" \ + "$(pgc_log_shows_accounting "$_acc/prose.log")" "no" + +check "an absent log shows no accounting rather than erroring" \ + "$(pgc_log_shows_accounting "$_acc/absent.log")" "no" + +# ---- the reconciliation, in both directions -------------------------------- + +_declared="$_acc/declared"; _observed="$_acc/observed" + +printf 'alpha\nbeta\ngamma\n' > "$_declared" +printf 'alpha\nbeta\ngamma\n' > "$_observed" +check "equal sets reconcile" \ + "$(pgc_reconcile_accounting "$_declared" "$_observed" >/dev/null 2>&1 && echo ok || echo asymmetric)" "ok" + +# The direction that catches the false green: a suite said it would account and +# no accounting line appeared, so it died before reaching pgc_summary. Today that +# reads PASS whenever the shell happened to exit 0. +printf 'alpha\nbeta\n' > "$_observed" +check "a declared suite that produced no accounting is caught" \ + "$(pgc_reconcile_accounting "$_declared" "$_observed" >/dev/null 2>&1 && echo ok || echo asymmetric)" "asymmetric" +check "and it is NAMED, so the reader does not have to diff two lists" \ + "$(pgc_reconcile_accounting "$_declared" "$_observed" 2>&1 | grep -c '^[[:space:]]*declared but never accounted: gamma$')" "1" + +# The opposite direction: an accounting line from a suite whose source says it +# cannot produce one. That means the reading of the source is stale, and it is +# the failure an exempt list maintained by hand can never report. +printf 'alpha\nbeta\n' > "$_declared" +printf 'alpha\nbeta\ngamma\n' > "$_observed" +check "an undeclared suite that DID account is caught too" \ + "$(pgc_reconcile_accounting "$_declared" "$_observed" >/dev/null 2>&1 && echo ok || echo asymmetric)" "asymmetric" +check "and it is named as the opposite fault, not the same one" \ + "$(pgc_reconcile_accounting "$_declared" "$_observed" 2>&1 | grep -c '^[[:space:]]*accounted but never declared: gamma$')" "1" + +# Both at once must report both. One error masking the other is how a count +# passes while two suites are wrong in opposite directions -- the exact failure +# a count cannot distinguish from correctness. +printf 'alpha\ndelta\n' > "$_declared" +printf 'alpha\ngamma\n' > "$_observed" +check "opposite errors do not cancel: both directions are reported" \ + "$(pgc_reconcile_accounting "$_declared" "$_observed" 2>&1 | grep -cE '^[[:space:]]*(declared but never accounted: delta|accounted but never declared: gamma)$')" "2" + +# inputs == sum(buckets), printed from the data, per the house rule. +printf 'alpha\nbeta\ngamma\n' > "$_declared" +printf 'beta\ngamma\ndelta\n' > "$_observed" +check "the reconciliation prints inputs == sum(buckets)" \ + "$(pgc_reconcile_accounting "$_declared" "$_observed" 2>&1 | grep -c 'inputs=4 .*both=2.*declared only=1.*accounted only=1.*sum=4')" "1" + +# ---- and the RUNNER must call it, not merely define it ---------------------- +# +# Selftest 320 records what testing a function and not its caller costs here: the +# classifier was right and the loop threw the answer away. So pin the call site +# and pin that its result can fail the major. + +# Count CALLS, not mentions. The first version of this arm matched the comment +# on the definition line as readily as the call below it -- the same +# mention-for-a-call mistake pgc_suite_declares_accounting exists to refuse, +# committed by the arm that asserts it. +check "the runner calls the reconciliation, not merely defines it" \ + "$(grep -c '[^_[:alnum:]]pgc_reconcile_accounting "' "$_rv")" "1" +check "premise: and that count excludes the definition line, which mentions it" \ + "$(grep -c '^pgc_reconcile_accounting()' "$_rv")" "1" +check "and a failed reconciliation sets the per-major failure flag" \ + "$(grep -A6 'pgc_reconcile_accounting "\$_acc_declared"' "$_rv" | grep -c 'verfail=1')" "1" + +# ---- the suites the driver deliberately never ran --------------------------- +# +# PGC_SKIP_TIMING drops four suites on every CI run. They call pgc_summary and +# correctly produce no accounting line, because nothing executed them. Without a +# term for that the reconciliation goes red for the one reason that is not a +# defect, and a check that cries wolf on every CI run is a check nobody reads. +# +# The driver records the decision where it makes it. These arms hold that the +# term EXCUSES only what the driver actually recorded, and cannot be used to +# excuse anything else. + +_notdisp="$_acc/notdispatched" + +printf 'alpha\nbeta\ngamma\n' > "$_declared" +printf 'alpha\nbeta\n' > "$_observed" +printf 'gamma\n' > "$_notdisp" +check "a declared suite the driver never dispatched reconciles" \ + "$(pgc_reconcile_accounting "$_declared" "$_observed" "$_notdisp" >/dev/null 2>&1 && echo ok || echo asymmetric)" "ok" + +# The same inputs WITHOUT the record must still be caught, or the term is not +# doing any work and the arm above is satisfied by a function that ignores it. +check "and without that record the same run is still caught" \ + "$(pgc_reconcile_accounting "$_declared" "$_observed" >/dev/null 2>&1 && echo ok || echo asymmetric)" "asymmetric" + +# A suite cannot both have reached its summary and not have been dispatched. +# Taking the union would absorb this silently, so it is asserted on its own. +printf 'alpha\nbeta\n' > "$_declared" +printf 'alpha\nbeta\n' > "$_observed" +printf 'beta\n' > "$_notdisp" +check "a suite recorded as never dispatched that DID account is caught" \ + "$(pgc_reconcile_accounting "$_declared" "$_observed" "$_notdisp" >/dev/null 2>&1 && echo ok || echo asymmetric)" "asymmetric" +check "and it is named as that fault, not as one of the other two" \ + "$(pgc_reconcile_accounting "$_declared" "$_observed" "$_notdisp" 2>&1 \ + | grep -c '^[[:space:]]*both accounted and recorded as never dispatched: beta$')" "1" + +# The record cannot excuse a suite that never declared accounting in the first +# place: that is still the stale-reading direction. +printf 'alpha\n' > "$_declared" +printf 'alpha\n' > "$_observed" +printf 'zeta\n' > "$_notdisp" +check "the record cannot introduce a suite the source never declared" \ + "$(pgc_reconcile_accounting "$_declared" "$_observed" "$_notdisp" 2>&1 \ + | grep -c '^[[:space:]]*accounted but never declared: zeta$')" "1" + +# ---- and the DRIVER must write that record --------------------------------- +# +# The term is only honest if the branch that decides not to run a suite is the +# thing that records it. Pin the write to that branch, beside the forged log it +# sits next to. + +check "the skip branch records the suite it did not dispatch" \ + "$(grep -A8 'echo "\$s.sh: SKIPPED (ran no checks)" >"\$builddir/\${s}.log"' "$_rv" \ + | grep -c 'accounting.notdispatched')" "1" +check "and the reconciliation is given that record" \ + "$(grep -c 'pgc_reconcile_accounting "\$_acc_declared" "\$_acc_observed" "\$_acc_notdisp"' "$_rv")" "1" + +# ---- and the identity must be able to FAIL --------------------------------- +# +# inputs == sum(buckets) is printed beside every reconciliation, per the house +# rule. Printing it is not the same as checking it, and asserting it is worth +# nothing unless something can make it false. +# +# Measured, not argued. Two mutations: +# +# compute _inputs from the buckets instead of from the files -- the arithmetic +# above becomes P + D + O == P + D + O, and NOTHING reddens. That is why +# _inputs is counted from the two files by a separate route. +# +# drop the sort before comm -- comm then reports garbage buckets, and the +# totals diverge. That is the fault this identity actually guards, and it is +# selftest 070's subject arriving in a second place. +# +# So the arm is the second mutation, applied to a twin evalled here. + +eval "$(sed -n '/^pgc_reconcile_accounting()/,/^}/p' "$_rv" \ + | sed 's|LC_ALL=C sort -u "$_decl" 2>/dev/null|cat "$_decl" 2>/dev/null|' \ + | sed 's|LC_ALL=C sort -u "$_obs" 2>/dev/null|cat "$_obs" 2>/dev/null|' \ + | sed 's/^pgc_reconcile_accounting()/pgc_reconcile_unsorted_twin()/')" + +check "premise: the unsorted twin is callable" \ + "$(type -t pgc_reconcile_unsorted_twin)" "function" +# A clean pass reads the same whether the code is load-bearing or the mutation +# never applied, so assert the twin really lost its sort. +check "premise: the mutation applied -- the twin no longer sorts its inputs" \ + "$(type pgc_reconcile_unsorted_twin | grep -c 'LC_ALL=C sort -u \"\$_decl\"')" "0" +check "premise: and the real function still does" \ + "$(type pgc_reconcile_accounting | grep -c 'LC_ALL=C sort -u \"\$_decl\"')" "1" + +printf 'gamma\nbeta\nalpha\n' > "$_declared" +printf 'delta\ngamma\nbeta\n' > "$_observed" +check "the identity catches comm reading unsorted input" \ + "$(pgc_reconcile_unsorted_twin "$_declared" "$_observed" 2>&1 | grep -c 'does not add up')" "1" +check "and the real function reconciles the same input, so the arm is not noise" \ + "$(pgc_reconcile_accounting "$_declared" "$_observed" 2>&1 | grep -c 'does not add up')" "0" + +# ---- the readers, run over the REAL population ------------------------------ +# +# Everything above uses fixtures. A reader that works on four synthetic files and +# not on the 251 registered suites has been tested against the world it was +# written for. So run the declaration reader over the actual list and print the +# partition, per the rule that a list-derived claim shows inputs == sum(buckets). +# +# No count is asserted. The number of exempt suites is not a fact about +# correctness, and pinning it here would make this arm a second copy of a +# hand-maintained list -- which is the thing the whole design removes. + +_reg=0; _decl_n=0; _exempt_n=0 +while IFS= read -r _s; do + _reg=$((_reg + 1)) + if [ "$(pgc_suite_declares_accounting "$PGC_TESTDIR/${_s}.sh")" = yes ]; then + _decl_n=$((_decl_n + 1)) + else + _exempt_n=$((_exempt_n + 1)) + fi +done < <(listed_suites) + +echo " registered=$_reg | declares accounting=$_decl_n, does not=$_exempt_n | sum=$((_decl_n + _exempt_n))" + +check "premise: the registered list is not empty, so the partition means something" \ + "$([ "$_reg" -gt 0 ] && echo yes || echo no)" "yes" +check "the partition over the real suite list adds up" \ + "$((_decl_n + _exempt_n))" "$_reg" + +# Both buckets must be occupied, or the reader is answering the same way for +# everything and the arms above would pass just as happily. +check "the reader does not answer yes for every registered suite" \ + "$([ "$_exempt_n" -gt 0 ] && echo yes || echo no)" "yes" +check "nor no for every one of them" \ + "$([ "$_decl_n" -gt 0 ] && echo yes || echo no)" "yes" + +# ---- the declaration reader must survive `set -o pipefail` ------------------ +# +# A REGRESSION ARM. The first version of pgc_suite_declares_accounting piped sed +# into `grep -q`, and this file runs under `set -o pipefail`. grep -q exits the +# moment it matches, closing the pipe while sed is still writing; sed takes EPIPE +# and exits non-zero, and pipefail reports the whole pipeline as failed even +# though grep matched. The function then answered "no" for a suite that plainly +# calls pgc_summary. +# +# It was caught here, and only here: on the real population the two longest +# suites -- analyze_function and hilbert_curve -- read as not declaring +# accounting inside this run and as declaring it outside. Selftest 040 carries +# the same story from #473 and #476, where it named different innocent suites on +# every run. +# +# The arm is a file long enough to lose the race, with the call at the TOP so a +# matcher that exits early exits early. +_bigsuite="$_acc/big.sh" +{ + printf '. "$(dirname "$0")/lib.sh"\npgc_summary\n' + _i=0 + while [ "$_i" -lt 40000 ]; do printf 'echo padding line %s\n' "$_i"; _i=$((_i + 1)); done +} > "$_bigsuite" + +check "premise: pipefail is on, which is the condition the bug needs" \ + "$(set -o | grep -cE '^pipefail[[:space:]]+on$')" "1" +check "premise: the fixture is long enough to lose the race" \ + "$([ "$(wc -l < "$_bigsuite")" -gt 10000 ] && echo yes || echo no)" "yes" + +check "a long suite that calls pgc_summary still declares accounting" \ + "$(pgc_suite_declares_accounting "$_bigsuite")" "yes" + +# And prove the arm can fail. The twin is the SHAPE that was wrong, restated +# here rather than extracted, because the wrong version is no longer in the tree. +_grepq_twin() { # the original shape, restated as tightly as it can be + sed 's/#.*$//' "$1" \ + | grep -qE '(^|[^_[:alnum:]])pgc_summary([^_[:alnum:]]|$)' && echo yes || echo no +} +check "premise: the grep -q twin is a different function from the real one" \ + "$(type -t _grepq_twin)" "function" +check "the grep -q shape is the one that gets this wrong under pipefail" \ + "$(_grepq_twin "$_bigsuite")" "no" +check "and it agrees with the real reader on a SHORT file, which is why it survived review" \ + "$(_grepq_twin "$_acc/declares.sh")" "$(pgc_suite_declares_accounting "$_acc/declares.sh")" From 65bf56922818907ff9216cb2d8084bfd5b823b30 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Wed, 9 Sep 2026 20:34:29 -0600 Subject: [PATCH 2/8] test: an absent suite file is its own answer, and the stripper follows the shell (#916) Both from OffgridwithJD's review of #922, and both are real. AN ABSENT FILE IS NOT AN EXEMPT SUITE. pgc_suite_declares_accounting returned "no" for a file that does not exist as readily as for one that does not call pgc_summary. A registered suite whose .sh had vanished was therefore classified exempt, and the reconciliation read clean -- a suite disappearing from the matrix, inside the check whose whole subject is suites going missing from the accounting. It now answers "absent", the runner counts those and fails the major, and the real-population block in selftest 390 carries three buckets rather than folding absent back into exempt. Measured today: 251 registered, 0 absent. THE COMMENT STRIPPER NOW FOLLOWS THE SHELL'S RULE. `sed 's/#.*$//'` strips from ANY hash, so a hash inside a quoted string earlier on the line would hide a pgc_summary call after it. It now strips only a hash at line start or after whitespace, which is what the shell treats as starting a comment. Measured rather than assumed, over all 251 registered suites: three carry a line holding both a hash and pgc_summary -- analyze_function, hilbert_curve and projections -- and in every one the hash starts the line, so no suite was misread either way. The partition is 239/12 before and after. The shell rule still does not cover a hash after whitespace INSIDE a quoted string, so that residual is pinned by an arm over the corpus rather than left to be rediscovered: no registered suite may have a hash before a pgc_summary call on the same line. New arms in both harnesses: absent is distinguishable from exempt, a hash inside a word does not hide the call after it, a trailing comment does not either, an indented comment is still a comment, and every registered suite has a file. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK --- test/pytest/test_suite_accounting.py | 17 ++++- test/run_all_versions.sh | 27 ++++++-- .../390-a-registered-suite-must-account.sh | 68 ++++++++++++++++--- 3 files changed, 97 insertions(+), 15 deletions(-) diff --git a/test/pytest/test_suite_accounting.py b/test/pytest/test_suite_accounting.py index e5b99ee6..106474ec 100644 --- a/test/pytest/test_suite_accounting.py +++ b/test/pytest/test_suite_accounting.py @@ -78,8 +78,23 @@ def test_a_suite_that_calls_pgc_summary_declares_accounting(tmp_path, expect): "a comment mentioning it is not a declaration") expect.text(_call("pgc_suite_declares_accounting", longer)[0].strip(), "no", "a longer name containing it is not a declaration") + # An ABSENT file is its own answer. Reported by OffgridwithJD reviewing #922: + # folding it into "no" classifies a registered suite whose .sh has vanished as + # exempt, and the reconciliation then reads clean. expect.text(_call("pgc_suite_declares_accounting", str(tmp_path / "gone.sh"))[0].strip(), - "no", "an absent file declares nothing rather than erroring") + "absent", "an absent file is reported absent, not exempt") + + # The stripper follows the shell's rule -- a hash starts a comment at line + # start or after whitespace -- so a hash inside a word cannot hide the call. + inword = _write(tmp_path, "inword.sh", ". lib.sh\nX=a#b; pgc_summary\n") + trailing = _write(tmp_path, "trailing.sh", ". lib.sh\npgc_summary # trailing\n") + indented = _write(tmp_path, "indented.sh", ". lib.sh\n # pgc_summary here only\nexit 0\n") + expect.text(_call("pgc_suite_declares_accounting", inword)[0].strip(), "yes", + "a hash inside a word does not hide the call after it") + expect.text(_call("pgc_suite_declares_accounting", trailing)[0].strip(), "yes", + "a trailing comment after the call does not hide it") + expect.text(_call("pgc_suite_declares_accounting", indented)[0].strip(), "no", + "an indented comment is still a comment") # ---- the observation reader ------------------------------------------------- diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index bfdfe2e0..f9168bb5 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -893,9 +893,20 @@ pgc_suite_declares_accounting() { # pgc_suite_declares_accounting FILE -> yes|no # not declaring accounting inside the selftest and as declaring it outside. # # grep -c reads to EOF, so sed never sees a closed pipe. + # An ABSENT file is its own answer, not "does not declare accounting". + # Conflating them classifies a registered suite whose .sh has vanished as + # exempt, and the reconciliation then reads clean -- a suite disappearing + # from the matrix, inside the check whose whole subject is suites that go + # missing from the accounting. Reported by OffgridwithJD reviewing #922. local _f="$1" _n - [ -f "$_f" ] || { echo no; return 0; } - _n="$(sed 's/#.*$//' "$_f" \ + [ -f "$_f" ] || { echo absent; return 0; } + # Strip a `#` only where the shell would treat one as starting a comment: + # at the start of a line, or after whitespace. `sed 's/#.*$//'` strips from + # ANY hash, so a `#` inside a quoted string earlier on the line would hide a + # pgc_summary call after it. Measured across all 251 registered suites: three + # carry a line with both, and in every one the `#` starts the line, so no + # suite is misread today. The arm in selftest 390 keeps that true. + _n="$(sed 's/\(^\|[[:space:]]\)#.*$/\1/' "$_f" \ | grep -cE '(^|[^_[:alnum:]])pgc_summary([^_[:alnum:]]|$)' || true)" if [ "${_n:-0}" -gt 0 ]; then echo yes @@ -1086,12 +1097,20 @@ pgc_tally_suite() { # pgc_tally_suite NAME VERDICT LOGFILE : >"$_acc_declared" : >"$_acc_observed" [ -f "$_acc_notdisp" ] || : >"$_acc_notdisp" + _acc_absent=0 for s in "${SUITES[@]}"; do - [ "$(pgc_suite_declares_accounting "$builddir/test/${s}.sh")" = yes ] \ - && printf '%s\n' "$s" >>"$_acc_declared" + case "$(pgc_suite_declares_accounting "$builddir/test/${s}.sh")" in + yes) printf '%s\n' "$s" >>"$_acc_declared" ;; + absent) echo " registered but has no file: $s.sh" + _acc_absent=$((_acc_absent + 1)) ;; + esac [ "$(pgc_log_shows_accounting "$builddir/${s}.log")" = yes ] \ && printf '%s\n' "$s" >>"$_acc_observed" done + if [ "$_acc_absent" != 0 ]; then + echo " $_acc_absent registered suite(s) on PG$major have no file, which is not a pass" + verfail=1 + fi if ! pgc_reconcile_accounting "$_acc_declared" "$_acc_observed" "$_acc_notdisp"; then echo " PG$major cannot account for every registered suite, which is not a pass" verfail=1 diff --git a/test/selftest/390-a-registered-suite-must-account.sh b/test/selftest/390-a-registered-suite-must-account.sh index 9d50e959..d8dccf69 100644 --- a/test/selftest/390-a-registered-suite-must-account.sh +++ b/test/selftest/390-a-registered-suite-must-account.sh @@ -77,8 +77,52 @@ printf '. "$(dirname "$0")/lib.sh"\npgc_summary_of_something\n' > "$_acc/prefix. check "a longer name containing pgc_summary is not a declaration" \ "$(pgc_suite_declares_accounting "$_acc/prefix.sh")" "no" -check "a file that does not exist declares nothing rather than erroring" \ - "$(pgc_suite_declares_accounting "$_acc/absent.sh")" "no" +# An ABSENT file is its own answer. Reported by OffgridwithJD reviewing #922: +# folding it into "no" classifies a registered suite whose .sh has vanished as +# exempt, and the reconciliation then reads clean -- a suite disappearing from +# the matrix, inside the check whose subject is suites going missing from the +# accounting. +check "a file that does not exist is reported absent, not exempt" \ + "$(pgc_suite_declares_accounting "$_acc/absent.sh")" "absent" +check "and absent is distinguishable from a present file that does not declare" \ + "$([ "$(pgc_suite_declares_accounting "$_acc/absent.sh")" \ + = "$(pgc_suite_declares_accounting "$_acc/silent.sh")" ] && echo same || echo different)" \ + "different" + +# ---- the comment stripper follows the SHELL's rule ------------------------- +# +# `sed 's/#.*$//'` strips from ANY hash, so a `#` inside a quoted string earlier +# on the line hides a pgc_summary call after it. Also reported by OffgridwithJD. +# The stripper now only treats a hash at line start or after whitespace as a +# comment, which is what the shell does. +printf '. "$(dirname "$0")/lib.sh"\nX=a#b; pgc_summary\n' > "$_acc/hashinword.sh" +check "a hash inside a word does not hide the call after it" \ + "$(pgc_suite_declares_accounting "$_acc/hashinword.sh")" "yes" + +printf '. "$(dirname "$0")/lib.sh"\npgc_summary # and a trailing comment\n' > "$_acc/trailing.sh" +check "a trailing comment after the call does not hide it" \ + "$(pgc_suite_declares_accounting "$_acc/trailing.sh")" "yes" + +printf '. "$(dirname "$0")/lib.sh"\n # pgc_summary is only mentioned here\nexit 0\n' > "$_acc/indented.sh" +check "an indented comment is still a comment" \ + "$(pgc_suite_declares_accounting "$_acc/indented.sh")" "no" + +# The residual case the shell rule does NOT cover: a hash after whitespace INSIDE +# a quoted string. Measured over all 251 registered suites -- three carry a line +# with both a hash and pgc_summary, and in every one the hash starts the line. So +# no suite is misread today, and this arm keeps that true rather than leaving it +# to be rediscovered. +_hashline=0 +while IFS= read -r _hs; do + [ -f "$PGC_TESTDIR/${_hs}.sh" ] || continue + if grep -nE '(^|[^_[:alnum:]])pgc_summary([^_[:alnum:]]|$)' "$PGC_TESTDIR/${_hs}.sh" \ + | grep -qE '^[0-9]+:[^#]*[^[:space:]]#'; then + _hashline=$((_hashline + 1)) + echo " a hash precedes a pgc_summary call in $_hs.sh" + fi +done < <(listed_suites) +check "no registered suite has a hash before a pgc_summary call on the same line" \ + "$_hashline" "0" # ---- the observation reader ------------------------------------------------ # @@ -276,22 +320,26 @@ check "and the real function reconciles the same input, so the arm is not noise" # correctness, and pinning it here would make this arm a second copy of a # hand-maintained list -- which is the thing the whole design removes. -_reg=0; _decl_n=0; _exempt_n=0 +# THREE buckets, not two. Folding "absent" into "does not declare" is the +# conflation the reader was just fixed for, and repeating it here would leave the +# real population the one place it still happened. +_reg=0; _decl_n=0; _exempt_n=0; _absent_n=0 while IFS= read -r _s; do _reg=$((_reg + 1)) - if [ "$(pgc_suite_declares_accounting "$PGC_TESTDIR/${_s}.sh")" = yes ]; then - _decl_n=$((_decl_n + 1)) - else - _exempt_n=$((_exempt_n + 1)) - fi + case "$(pgc_suite_declares_accounting "$PGC_TESTDIR/${_s}.sh")" in + yes) _decl_n=$((_decl_n + 1)) ;; + absent) _absent_n=$((_absent_n + 1)); echo " registered but has no file: $_s.sh" ;; + *) _exempt_n=$((_exempt_n + 1)) ;; + esac done < <(listed_suites) -echo " registered=$_reg | declares accounting=$_decl_n, does not=$_exempt_n | sum=$((_decl_n + _exempt_n))" +echo " registered=$_reg | declares accounting=$_decl_n, does not=$_exempt_n, absent=$_absent_n | sum=$((_decl_n + _exempt_n + _absent_n))" check "premise: the registered list is not empty, so the partition means something" \ "$([ "$_reg" -gt 0 ] && echo yes || echo no)" "yes" check "the partition over the real suite list adds up" \ - "$((_decl_n + _exempt_n))" "$_reg" + "$((_decl_n + _exempt_n + _absent_n))" "$_reg" +check "every registered suite has a file" "$_absent_n" "0" # Both buckets must be occupied, or the reader is answering the same way for # everything and the arms above would pass just as happily. From 65dfa8a72cb64136477883688dd5cbdba711eca6 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Wed, 9 Sep 2026 21:02:52 -0600 Subject: [PATCH 3/8] test: show the accounting reader real producer output, and say what the twelve are (#916) All from OffgridwithJD's review, including two corrections to claims I had already pushed. THE CORPUS ARM I ADDED LAST COMMIT WAS INVERTED. It required a non-whitespace character before the hash, which is a hash inside a WORD -- the shape the stripper handles correctly -- so it flagged the safe case and was blind to the dangerous one it was named for. Reproduced here with their control: psql -c "SELECT 1 # note"; pgc_summary reader says NO, arm did not flag X=a#b; pgc_summary reader says YES, arm FLAGGED The arm no longer restates the hazard as a second pattern. It compares the reader's INPUT with its OUTPUT: count the call in the raw file, count it in the stripped text, and a lower stripped count means the stripper hid a call. That catches it for any spelling, cannot be inverted, and IS the measurement rather than depending on one staying true. THE READER HAD NEVER BEEN SHOWN THE PRODUCER'S OWN OUTPUT. Every log in both harnesses was a literal, and the format string lives a third time in pgc_summary. Three hand-written copies of one line: a wording drift in the PRODUCER leaves both harnesses green while the reader answers "no" for every real suite, which would redden the whole matrix on both majors having passed its own tests. Both harnesses now run a real two-line suite and feed the reader its actual stdout, with a reworded control so the arm can fail. THE TWELVE, MEASURED. My prose said ten suites "never counted a check" and my partition said twelve; both reviewers then compounded it, because a loose pattern for sourcing lib.sh matches portlib.sh. Measured with a tight one: NONE of the twelve sources test/lib.sh. Each defines its own check(), and ten keep no tally at all. The number is no longer written in prose anywhere; the reconciliation prints it at runtime. THE OVERCOUNT THE PR OPENS BY DESCRIBING IS NOW ACTUALLY REPORTED. #922's own CI showed "suites that ran: 242 of 251" still counting all twelve suites whose checks the harness cannot see -- so the change described a fix it did not make. The summary now breaks them out, from data already in hand at that point: suites that ran: 242 of 251 (skipped: 9, incomplete: 0) of those, N accounted for their checks and M did not Also: an absent-file answer the case now handles with a loud default arm rather than folding an unknown verdict into "does not declare"; a sentence on why the log reader deliberately does NOT distinguish absent from negative while the declaration reader does; a precise note that inputs == sum(buckets) cannot be false on the DATA -- for sets the identity always holds, measured over 400 random pairs -- and that what it guards is comm reading unsorted input; and ci.yml's "three wall-clock suites", which is stale at four and which this change is the one to falsify. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK --- .github/workflows/ci.yml | 2 +- test/pytest/TESTS.md | 19 ++- test/pytest/test_suite_accounting.py | 41 ++++++- test/run_all_versions.sh | 46 ++++++-- .../390-a-registered-suite-must-account.sh | 110 ++++++++++++++---- 5 files changed, 185 insertions(+), 33 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f2676c4f..52c3178b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -472,7 +472,7 @@ jobs: - name: Run the suite matrix on PG ${{ matrix.pg }} run: | set -euo pipefail - # PGC_SKIP_TIMING drops the three wall-clock suites: a shared runner + # PGC_SKIP_TIMING drops the four wall-clock suites: a shared runner # cannot hold a ratio still, and a gate that reds for reasons unrelated # to the change is worse than one that does not run. They stay in the # local matrix, which is where those numbers mean anything. diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index b93b2cf9..c6b62532 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -1017,8 +1017,12 @@ domain is a changed READER at an unchanged layout. `test_suite_accounting.py` holds the matrix runner to its own arithmetic. -`run_all_versions.sh` prints `suites that ran: N of M` and never checks it, and ten -registered suites exit 0 having never counted a check. Counted among the suites that +`run_all_versions.sh` prints `suites that ran: N of M` and never checks it, and twelve +registered suites exit 0 without ever calling `pgc_summary`. Measured with a pattern +tight enough to exclude `portlib.sh` -- a looser one matched it and gave both reviewers +of this change the same wrong answer: **none** of the twelve sources `test/lib.sh`. +Each defines its own `check()`, and ten keep no tally at all, so the harness cannot see +their checks. Counted among the suites that "ran", they are the overcount #447 added that line to stop, one level further down. A count cannot close this. Two errors of opposite sign cancel, and an exempt list @@ -1076,6 +1080,15 @@ FROM the buckets makes the line true for any values and reddens nothing, which i it is counted from the two files by a separate route. Dropping the sort before `comm` makes the totals diverge, and that is the fault the identity guards. +### `test_the_reader_accepts_the_line_the_producer_actually_emits` + +Every other log in the file is a literal, and the shell half types the same four again, +and the format string lives a third time in `pgc_summary`. Three hand-written copies of +one line: a wording drift in the **producer** leaves both harnesses green while the +reader answers "no" for every real suite, reddening the whole matrix on both majors. +So this arm runs a real suite and feeds the reader its actual stdout, with a reworded +control to show it can fail. + ### `test_the_partition_over_the_registered_suites_adds_up` The readers run over the real registered suite list. No count is asserted: how many @@ -1089,7 +1102,7 @@ A regression arm. The first implementation piped `sed` into `grep -q`; grep exit match, sed takes EPIPE, and `pipefail` reports the pipeline as failed. The reader answered "no" for a suite that plainly calls `pgc_summary`. It is a race, so it reproduces on long files and not short ones -- it passed every fixture and failed only -on the real population, naming the two longest suites. Selftest 040 carries the same +on the real population, naming two of the longest suites. Selftest 040 carries the same story from #473 and #476. ## 15. Adding a test diff --git a/test/pytest/test_suite_accounting.py b/test/pytest/test_suite_accounting.py index 106474ec..5d62b8e2 100644 --- a/test/pytest/test_suite_accounting.py +++ b/test/pytest/test_suite_accounting.py @@ -1,7 +1,11 @@ """The matrix must reconcile the suites it registered against the ones that accounted. -`run_all_versions.sh` prints "suites that ran: N of M" and never checks it, and ten -registered suites exit 0 having never counted a check. Counted among the suites that +`run_all_versions.sh` prints "suites that ran: N of M" and never checks it, and twelve +registered suites exit 0 without ever calling `pgc_summary`. Measured with a pattern +tight enough to exclude `portlib.sh` -- a looser one matched it and gave both reviewers +of this change the same wrong answer: **none** of the twelve sources `test/lib.sh`. +Each defines its own `check()`, and ten keep no tally at all, so the harness cannot see +their checks. Counted among the suites that "ran", they are the overcount #447 added that line to stop, one level further down. A count cannot close this. Two errors of opposite sign cancel, and an exempt list @@ -127,6 +131,39 @@ def test_the_accounting_line_is_read_on_every_exit_path(tmp_path, expect): "no", "an absent log shows none rather than erroring") +def test_the_reader_accepts_the_line_the_producer_actually_emits(tmp_path, expect): + """Every log above is a literal typed into this file, and the shell half types the + same four again, and the format string itself lives a third time in `pgc_summary`. + + Three hand-written copies of one line. A wording drift in the PRODUCER leaves both + harnesses green while the reader answers "no" for every real suite -- which would + redden the whole matrix on both majors, having passed its own tests. Found by + OffgridwithJD, who measured it: one realistic rewording of lib.sh flips the reader + on a real log with no arm going red. + + So run a REAL suite and feed the reader its actual stdout. This is the only arm in + the file that survives a change to the format. + """ + suite = tmp_path / "real.sh" + suite.write_text(f'. "{REPO / "test" / "lib.sh"}"\ncheck "x" a a\npgc_summary\n') + log = tmp_path / "real.log" + r = subprocess.run(["bash", str(suite)], capture_output=True, text=True) + log.write_text(r.stdout) + + expect.num(r.stdout.count("\naccounting: "), 1, + "premise: the real suite produced exactly one accounting line") + expect.text(_call("pgc_log_shows_accounting", str(log))[0].strip(), "yes", + "the reader accepts the line the producer actually emits") + + # The control that the arm is not simply insensitive. + drifted = tmp_path / "drifted.log" + drifted.write_text(r.stdout.replace("\naccounting: ", "\naccounting summary: ")) + expect.num(drifted.read_text().count("\naccounting: "), 0, + "premise: the drift changed the line the reader looks for") + expect.text(_call("pgc_log_shows_accounting", str(drifted))[0].strip(), "no", + "and a reworded producer line is refused, so the arm can fail") + + # ---- the reconciliation ----------------------------------------------------- diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index f9168bb5..5898ae7c 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -860,9 +860,13 @@ pgc_classify_suite_rc() { # pgc_classify_suite_rc RC LOGFILE -> PASS|SKIP|INCOMP # invisible to a green run. # ---- accounting membership, derived rather than listed ----------------------- # -# The matrix prints "suites that ran: N of M" and never checks it, and ten -# registered suites exit 0 having never counted a check. They assert things in -# their own way; the harness simply cannot see their checks. Counted among the +# The matrix prints "suites that ran: N of M" and never checks it, and twelve +# registered suites exit 0 without ever calling pgc_summary. Measured, with a +# pattern tight enough to exclude portlib.sh -- a looser one matched it and gave +# both reviewers of this change the same wrong answer: NONE of the twelve sources +# test/lib.sh. Each defines its own check(), and ten of them keep no tally at all. +# So the harness cannot see their checks. The number is not written elsewhere on +# purpose; the reconciliation prints it at runtime. Counted among the # suites that "ran", they are the same overcount #447 added that line to stop, # one level further down. # @@ -921,6 +925,12 @@ pgc_log_shows_accounting() { # pgc_log_shows_accounting LOGFILE -> yes|no # the status, so its presence says "this suite reached its summary" and not # "this suite passed". Anchored and fully shaped, so the word appearing in a # suite's own prose cannot satisfy it. + # Unlike the declaration reader above, this one deliberately does NOT + # distinguish an absent log from a present one carrying no accounting line. + # Both mean the same thing here -- this suite did not reach its summary -- + # and a declared suite with no log at all is exactly the catch. The asymmetry + # between the two readers is intentional and is noted because it is the kind + # of thing that reads as an oversight later. local _log="$1" [ -f "$_log" ] || { echo no; return 0; } if grep -qE '^accounting: [0-9]+ passed \+ [0-9]+ failed \+ [0-9]+ unrunnable = [0-9]+$' "$_log"; then @@ -977,9 +987,14 @@ pgc_reconcile_accounting() { # pgc_reconcile_accounting DECLARED OBSERVED [NOTDI _noonly="$(printf '%s' "$_oonly" | grep -c . || true)" # inputs is counted from the FILES, independently of the three buckets. A - # derived total makes the identity below true for any values, which is the - # shape pgc_summary's own comment warns about: three counters reconciled - # against a fourth is the only version of this line that can fail. + # derived total makes the identity below true for any values. + # + # BUT BE PRECISE ABOUT WHAT IT CAN CATCH, because the next reader will go + # looking for a data case and there is not one: for sets, |D u O| always + # equals |D n O| + |D \ O| + |O \ D|. OffgridwithJD measured it -- 400 random + # set pairs, zero firings. What can make it false is comm being fed unsorted + # input, which produces buckets that are not a partition at all. It is a comm + # tripwire, and selftest 390 drives it with exactly that mutation. _inputs="$(LC_ALL=C sort -u "$_dfile" "$_ofile" | grep -c . || true)" _sum=$(( _nboth + _ndonly + _noonly )) rm -f "$_dfile" "$_ofile" "$_ndfile" "$_obsonly" @@ -1099,10 +1114,19 @@ pgc_tally_suite() { # pgc_tally_suite NAME VERDICT LOGFILE [ -f "$_acc_notdisp" ] || : >"$_acc_notdisp" _acc_absent=0 for s in "${SUITES[@]}"; do - case "$(pgc_suite_declares_accounting "$builddir/test/${s}.sh")" in + _acc_verdict="$(pgc_suite_declares_accounting "$builddir/test/${s}.sh")" + case "$_acc_verdict" in yes) printf '%s\n' "$s" >>"$_acc_declared" ;; + no) ;; absent) echo " registered but has no file: $s.sh" _acc_absent=$((_acc_absent + 1)) ;; + *) + # A default arm must not quietly name a real outcome. If the + # reader grows a fourth answer, this says so instead of filing + # it under "does not declare". + echo " pgc_suite_declares_accounting answered [$_acc_verdict] for $s, which is none of yes/no/absent" + _acc_absent=$((_acc_absent + 1)) + ;; esac [ "$(pgc_log_shows_accounting "$builddir/${s}.log")" = yes ] \ && printf '%s\n' "$s" >>"$_acc_observed" @@ -1116,7 +1140,15 @@ pgc_tally_suite() { # pgc_tally_suite NAME VERDICT LOGFILE verfail=1 fi + # How many of the suites counted as having RUN actually accounted for their + # checks (#916). Ten registered suites exit 0 having never called pgc_summary; + # counting them among the suites that ran is the overcount #447 added this + # line to stop, one level further down, and printing the total without this + # breakdown leaves it exactly where it was. Raised by OffgridwithJD, who was + # right that the drift detector alone does not close it. + _acc_ran="$(grep -c . "$_acc_observed" 2>/dev/null || true)" echo " suites that ran: $suites_ran of ${#SUITES[@]} (skipped: $suites_skipped, incomplete: $suites_incomplete)" + echo " of those, $_acc_ran accounted for their checks and $((suites_ran - _acc_ran)) did not" if [ "$suites_skipped" != 0 ]; then echo " skipped:${skipped_names}" fi diff --git a/test/selftest/390-a-registered-suite-must-account.sh b/test/selftest/390-a-registered-suite-must-account.sh index d8dccf69..e7d8cb1e 100644 --- a/test/selftest/390-a-registered-suite-must-account.sh +++ b/test/selftest/390-a-registered-suite-must-account.sh @@ -8,11 +8,17 @@ # drops leaves the sum short and the line still reads plausibly. # # The second is worse, because it is live today. pgc_classify_suite_rc maps rc=0 -# to PASS with no further question, and ten registered suites exit 0 having never -# called pgc_summary. They assert things -- concurrency prints its own failure and -# exits non-zero -- but the harness cannot count their checks, and nothing says -# so. They are counted among the suites that "ran", which is the exact overcount -# #447 added that line to stop, one level further down. +# to PASS with no further question, and twelve registered suites exit 0 without +# ever calling pgc_summary. Measured, with a pattern tight enough to exclude +# portlib.sh -- a looser one matched it and gave both reviewers of this change the +# same wrong answer: NONE of the twelve sources test/lib.sh. Each defines its own +# check(), and ten of them keep no tally at all. So the harness cannot see their +# checks, and nothing says so. They are counted among the suites that "ran", which +# is the exact overcount #447 added that line to stop, one level further down. +# +# The number is deliberately not repeated in prose elsewhere. The reconciliation +# prints it at runtime, and a count in prose is the thing this repository keeps +# having to unlearn: nine collisions on one written number in a single day. # # So the fix is NOT a number. A count cannot do this job: two errors of opposite # sign cancel, and an exempt list maintained by hand makes the count agree by @@ -107,22 +113,50 @@ printf '. "$(dirname "$0")/lib.sh"\n # pgc_summary is only mentioned here\nexit check "an indented comment is still a comment" \ "$(pgc_suite_declares_accounting "$_acc/indented.sh")" "no" -# The residual case the shell rule does NOT cover: a hash after whitespace INSIDE -# a quoted string. Measured over all 251 registered suites -- three carry a line -# with both a hash and pgc_summary, and in every one the hash starts the line. So -# no suite is misread today, and this arm keeps that true rather than leaving it -# to be rediscovered. -_hashline=0 +# The residual the shell rule does not cover is a hash after whitespace INSIDE a +# quoted string, and the arm for it must not be a second spelling of the hazard. +# +# The first version of this arm WAS that, and it was INVERTED: it required a +# non-whitespace character before the hash, which is a hash inside a WORD -- the +# shape the stripper handles correctly -- so it flagged the safe case and was +# blind to the dangerous one. Found by OffgridwithJD, who built the control: +# +# psql -c "SELECT 1 # note"; pgc_summary the reader answered NO, unflagged +# X=a#b; pgc_summary the reader answered YES, FLAGGED +# +# So the arm no longer restates the hazard. It compares the reader's INPUT with +# its OUTPUT: count the call in the raw file, count it again in the stripped +# text, and if the stripped count is lower the stripper hid a call. That detects +# it for any spelling, present or future, cannot be inverted, and does not depend +# on a measurement staying true -- it IS the measurement. +_hash_pat='(^|[^_[:alnum:]])pgc_summary([^_[:alnum:]]|$)' +_hidden=0 while IFS= read -r _hs; do - [ -f "$PGC_TESTDIR/${_hs}.sh" ] || continue - if grep -nE '(^|[^_[:alnum:]])pgc_summary([^_[:alnum:]]|$)' "$PGC_TESTDIR/${_hs}.sh" \ - | grep -qE '^[0-9]+:[^#]*[^[:space:]]#'; then - _hashline=$((_hashline + 1)) - echo " a hash precedes a pgc_summary call in $_hs.sh" + _hf="$PGC_TESTDIR/${_hs}.sh" + [ -f "$_hf" ] || continue + _raw="$(grep -cE "$_hash_pat" "$_hf" || true)" + _str="$(sed 's/\(^\|[[:space:]]\)#.*$/\1/' "$_hf" | grep -cE "$_hash_pat" || true)" + if [ "$_str" -lt "$_raw" ]; then + # A call the stripper removed. Only a real one matters; a comment-only + # mention losing its line is the stripper working. + if [ "$(pgc_suite_declares_accounting "$_hf")" = no ] \ + && grep -qE "$_hash_pat" "$_hf"; then + _hidden=$((_hidden + 1)) + echo " the comment stripper hides a pgc_summary call in $_hs.sh" + fi fi done < <(listed_suites) -check "no registered suite has a hash before a pgc_summary call on the same line" \ - "$_hashline" "0" +check "the stripper hides no pgc_summary call in any registered suite" "$_hidden" "0" + +# And prove that arm can fire, on a file built to trip it. Without this the zero +# above is satisfied by an arm that never looks at anything. +printf '. "$(dirname "$0")/lib.sh"\npsql -c "SELECT 1 # note"; pgc_summary\n' > "$_acc/hidden.sh" +_h_raw="$(grep -cE "$_hash_pat" "$_acc/hidden.sh" || true)" +_h_str="$(sed 's/\(^\|[[:space:]]\)#.*$/\1/' "$_acc/hidden.sh" | grep -cE "$_hash_pat" || true)" +check "premise: the fixture really does hide its call from the stripper" \ + "$([ "$_h_str" -lt "$_h_raw" ] && echo hidden || echo "raw=$_h_raw str=$_h_str")" "hidden" +check "and the reader answers no on it, which is the wrong answer the arm catches" \ + "$(pgc_suite_declares_accounting "$_acc/hidden.sh")" "no" # ---- the observation reader ------------------------------------------------ # @@ -155,6 +189,41 @@ check "and prose containing the word does not count as the line" \ check "an absent log shows no accounting rather than erroring" \ "$(pgc_log_shows_accounting "$_acc/absent.log")" "no" +# ---- and the reader must be shown the PRODUCER's own output ----------------- +# +# Every log above is a literal typed into this file, and pytest types the same +# four again, and the format string itself lives a third time in lib.sh's +# pgc_summary. Three hand-written copies of one line: a wording drift in the +# PRODUCER leaves both harnesses green while the reader answers "no" for every +# real suite, which would redden the whole matrix on both majors having passed +# its own tests. +# +# That is the pipefail lesson surviving on the other reader, and it was found by +# OffgridwithJD, who measured it: one realistic rewording of lib.sh:1507 flips the +# reader to "no" on a real log with no arm going red. +# +# So run a REAL two-line suite and feed the reader its actual stdout. This is the +# only arm here that survives a change to the format. +_realsuite="$_acc/real.sh" +printf '. "%s/lib.sh"\ncheck "x" a a\npgc_summary\n' "$PGC_TESTDIR" > "$_realsuite" +_reallog="$_acc/real.log" +bash "$_realsuite" > "$_reallog" 2>&1 || true + +check "premise: the real suite ran and reached its summary" \ + "$(grep -c ': PASSED$' "$_reallog")" "1" +check "premise: and produced exactly one accounting line to be read" \ + "$(grep -c '^accounting: ' "$_reallog")" "1" +check "the reader accepts the line the producer actually emits" \ + "$(pgc_log_shows_accounting "$_reallog")" "yes" + +# The control that this arm is not simply insensitive: the same real log with its +# accounting line reworded must be refused. +sed 's/^accounting: /accounting summary: /' "$_reallog" > "$_acc/real_drifted.log" +check "premise: the drift changed the line the reader looks for" \ + "$(grep -c '^accounting: ' "$_acc/real_drifted.log")" "0" +check "and a reworded producer line is refused, so the arm can fail" \ + "$(pgc_log_shows_accounting "$_acc/real_drifted.log")" "no" + # ---- the reconciliation, in both directions -------------------------------- _declared="$_acc/declared"; _observed="$_acc/observed" @@ -357,8 +426,9 @@ check "nor no for every one of them" \ # though grep matched. The function then answered "no" for a suite that plainly # calls pgc_summary. # -# It was caught here, and only here: on the real population the two longest -# suites -- analyze_function and hilbert_curve -- read as not declaring +# It was caught here, and only here: on the real population two of the longest +# suites -- hilbert_curve, the longest at 1,899 lines, and analyze_function, the +# third at 806 -- read as not declaring # accounting inside this run and as declaring it outside. Selftest 040 carries # the same story from #473 and #476, where it named different innocent suites on # every run. From 324357c58d651542a1d4e62fd729668758222b8e Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Wed, 9 Sep 2026 21:03:15 -0600 Subject: [PATCH 4/8] test: the pipe-into-grep-q rule must sweep the directory that enforces it (#486) #923's `suites (PG 17)` went red on a pytest name that EXISTS, while PG 18 passed the same commit: FAIL every test the document names exists in the corpus: got [[1: test_the_partition_over_the_registered_suites_adds_up]] The cause is the shape selftest 080 already forbids. Its membership test was `printf '%s\n' "$ondisk" | grep -qxF "$name"`, and the selftest runs under `set -o pipefail`: grep -q exits the moment it matches, printf takes EPIPE, and pipefail reports the pipeline failed though the name WAS present. The name is then recorded absent. WHY IT SURVIVED, AND WHY IT SURFACED NOW. Selftest 080's sweep is deliberately non-recursive and never looked inside test/selftest/ -- the directory scoping was never a decision, it fell out of writing "$TESTDIR"/*.sh, exactly as the bench/ hole did before it. Three fragments held the forbidden shape: 350's corpus membership test, 300's directory coverage test, 340's Makefile sweep. At corpus size the writer is small enough to win the race on an idle machine, which is why it passed for so long. Measured, 170 names over 400 trials: printf | grep -qxF idle: 0 false absences under load: 6 grep -cxF <<< idle: 0 under load: 0 A four-way CI matrix is the loaded case, and adding names to the corpus made the window wider. This is the third appearance of this bug class here after #473 and #476, and the second today. All three sites now use grep -c on a here-string, and the sweep covers test/selftest/ with its own coverage arm, because 080 already records that a conditionally added glob narrows silently and a file-count premise cannot see it. THE EXEMPTION IS DERIVED, NOT LISTED. 080's own control is inside a quoted heredoc, and so is any deliberate demonstration of the shape; a line inside one is text being written to a file, not a pipeline the suite runs. Comment lines are excluded for the same reason -- the rule's explanation, and the note beside each site fixed here, necessarily spell the shape out. A filename allowlist would have to be maintained, and this rule exists because things that must be maintained are not. Two defects in the exemption itself, both found by running it rather than reading it. It printed NR where it meant FNR, so from the second file onwards it reported line numbers from a running total and no key matched -- the neighbouring question answered plausibly, since single-file runs agree because NR == FNR there. And it kept heredoc state across inputs, so it now resets per file. Its own arms pin both: the sweep sees both lines of a probe, the exemption covers the one inside the heredoc and not the one above it. The "did not swallow the corpus" premise is a PROPORTION rather than a guessed ceiling. The first version used a bare 2000 and went red at 2,502 heredoc lines in a corpus that was entirely healthy -- a hand-written number failing the way hand-written numbers fail here. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK --- .../080-no-suite-pipes-a-captured-string.sh | 106 +++++++++++++++++- .../300-a-test-script-must-be-runnable.sh | 4 +- .../340-the-binary-must-be-built-from.sh | 3 +- .../selftest/350-the-pytest-corpus-must-be.sh | 13 ++- 4 files changed, 122 insertions(+), 4 deletions(-) diff --git a/test/selftest/080-no-suite-pipes-a-captured-string.sh b/test/selftest/080-no-suite-pipes-a-captured-string.sh index 2db26fef..41157c8a 100644 --- a/test/selftest/080-no-suite-pipes-a-captured-string.sh +++ b/test/selftest/080-no-suite-pipes-a-captured-string.sh @@ -68,10 +68,70 @@ check "control: piping a large string into grep -q reports a match as absent" \ # explanation of the rule, and the deliberate piped() demo above, both match the # pattern they exist to describe. Sweeping the file that enforces a rule for # instances of that rule is selftest 260's mistake once removed. +# test/selftest/ IS SCANNED TOO, AND IT WAS NOT, for the same reason bench/ was +# not: the directory scoping was never a decision, it fell out of writing +# "$TESTDIR"/*.sh. Three fragments held the exact shape this rule forbids -- +# 350's corpus membership test, 300's directory-coverage test, 340's Makefile +# sweep -- inside the directory that enforces the rule. +# +# NOT LATENT. 350's reddened #923's `suites (PG 17)` on a pytest name that +# exists, while PG 18 passed the same commit. At corpus size the writer is small +# enough to win the race on an idle machine, which is why it survived: measured +# at 170 names over 400 trials, 0 false absences idle and 6 under load, against 0 +# either way for the here-string form. +# +# The exemption is DERIVED, not listed. This file's own control below is inside a +# quoted heredoc, and so is any other deliberate demonstration of the shape; a +# line inside one is text being written to a file, not a pipeline this suite +# runs. A filename allowlist would have to be maintained, and this rule exists +# because things that must be maintained are not. _epipe_globs=("$TESTDIR"/*.sh) [ -d "$TESTDIR/../bench" ] && _epipe_globs+=("$TESTDIR"/../bench/*.sh) +[ -d "$TESTDIR/selftest" ] && _epipe_globs+=("$TESTDIR"/selftest/*.sh) + +# file:line pairs that sit inside a quoted heredoc, computed from the files. +_epipe_heredoc_lines() { + awk ' + # Reset per FILE. awk keeps globals across inputs, so an unterminated + # heredoc in one file leaves the scanner inside one for every file after + # it -- and a later line that happens to equal the stale tag closes it in + # the wrong place. Measured: without this, 080s own line 26 fell OUTSIDE + # the exemption when the sweep ran over all 302 files, and inside it when + # the same function ran over that file alone. + FNR == 1 { inhd = 0; tag = "" } + !inhd && match($0, /<<[-]?'"'"'[A-Za-z_][A-Za-z0-9_]*'"'"'/) { + tag = substr($0, RSTART, RLENGTH) + gsub(/^<<[-]?'"'"'/, "", tag); gsub(/'"'"'$/, "", tag) + inhd = 1; next + } + inhd { + t = $0; gsub(/^[ \t]+|[ \t]+$/, "", t) + if (t == tag) { inhd = 0; next } + # FNR, not NR. NR is cumulative across inputs, so the second file + # onwards reports line numbers from a running total and no key ever + # matches the grep -n output it is compared against. It is the + # neighbouring question answered plausibly: single-file runs agree, + # because there NR == FNR. + print FILENAME ":" FNR ":" + } + ' "$@" +} +_epipe_hd="$(_epipe_heredoc_lines "${_epipe_globs[@]}" 2>/dev/null || true)" +# Comment lines are excluded first. A comment is not a pipeline this suite runs, +# and the rule's own explanation -- and the note beside every site that was fixed +# -- necessarily spells the shape out. A sweep that counts its own documentation +# is selftest 260's mistake. _epipe_hits="$(grep -nE '(echo|printf)[^|]*\|[[:space:]]*grep -[a-zA-Z]*q' \ - "${_epipe_globs[@]}" 2>/dev/null | grep -v '/harness_selftest.sh:' || true)" + "${_epipe_globs[@]}" 2>/dev/null \ + | grep -v '/harness_selftest.sh:' \ + | grep -vE '^[^:]+:[0-9]+:[[:space:]]*#' || true)" +# Drop hits whose file:line is inside a quoted heredoc. +if [ -n "$_epipe_hd" ] && [ -n "$_epipe_hits" ]; then + _epipe_hits="$(printf '%s\n' "$_epipe_hits" | while IFS= read -r _eh; do + _ehk="${_eh%%:*}:$(printf '%s' "$_eh" | cut -d: -f2):" + [ "$(grep -cxF "$_ehk" <<<"$_epipe_hd" || true)" != 0 ] || printf '%s\n' "$_eh" + done)" +fi _epipe_count="$(printf '%s' "$_epipe_hits" | grep -c . || true)" [ -n "$_epipe_hits" ] || _epipe_count=0 check "no suite pipes a captured string into an early-exit reader" \ @@ -118,3 +178,47 @@ _epipe_bench="$(printf '%s' "$_epipe_files" | grep -c '/bench/' || true)" check "and bench/ was in the scan, which is the hole this rule had" \ "$([ "${_epipe_bench:-0}" -ge 1 ] && echo yes || echo "no (bench files read: $_epipe_bench)")" "yes" +# selftest/ gets its own arm for exactly the reason bench/ does: its glob is +# added conditionally, and the file-count premise above passes with it silently +# absent. +_epipe_self="$(printf '%s' "$_epipe_files" | grep -c '/selftest/' || true)" +check "and selftest/ was in the scan, which is the hole that reddened #923" \ + "$([ "${_epipe_self:-0}" -ge 5 ] && echo yes || echo "no (selftest files read: $_epipe_self)")" "yes" + +# The heredoc exemption must EXEMPT something and must not exempt everything. +# Without the first, the control below is unreachable and this file cannot hold +# its own rule; without the second, the sweep is switched off and reports zero. +_epipe_hd_count="$(printf '%s' "$_epipe_hd" | grep -c . || true)" +check "premise: the heredoc exemption found heredoc lines to exempt" \ + "$([ "${_epipe_hd_count:-0}" -ge 1 ] && echo yes || echo "no ($_epipe_hd_count)")" "yes" +# A PROPORTION, not a guessed ceiling. The first version of this arm used a bare +# 2000 and went red at 2,502 heredoc lines in a corpus that was entirely healthy +# -- a hand-written number failing for the reason hand-written numbers fail here. +_epipe_total_lines="$(cat "${_epipe_globs[@]}" 2>/dev/null | grep -c '' || true)" +echo " epipe sweep: total lines=$_epipe_total_lines, inside a quoted heredoc=$_epipe_hd_count" +check "premise: the sweep read lines to classify" \ + "$([ "${_epipe_total_lines:-0}" -ge 1000 ] && echo yes || echo "no ($_epipe_total_lines)")" "yes" +check "premise: and the exemption covers a minority of them, not the corpus" \ + "$([ "$((_epipe_hd_count * 2))" -lt "${_epipe_total_lines:-0}" ] && echo yes \ + || echo "no ($_epipe_hd_count of $_epipe_total_lines)")" "yes" + +# The rule must still SEE a violation that is not in a heredoc. A sweep whose +# exemption is too broad reports zero for the same reason a correct one does. +_epipe_probe="$PGC_WORKDIR/epipe_probe.sh" +# The shape is assembled from a fragment rather than written out, so these +# generator lines do not themselves match the sweep and need no exemption. +_epipe_shape='x() { printf "%s" "$1" | grep -%s PATTERN; }' +{ + printf '%s\n' "$(printf "$_epipe_shape" '%s' q)" + printf 'cat > /dev/null <<%sDEMO%s\n' "'" "'" + printf '%s\n' "$(printf "$_epipe_shape" '%s' q)" + printf 'DEMO\n' +} > "$_epipe_probe" +_epipe_probe_hd="$(_epipe_heredoc_lines "$_epipe_probe" 2>/dev/null || true)" +check "the sweep's pattern sees both lines of the probe" \ + "$(grep -cE '(echo|printf)[^|]*\|[[:space:]]*grep -[a-zA-Z]*q' "$_epipe_probe")" "2" +check "and the heredoc exemption covers the one inside the heredoc, not the other" \ + "$(printf '%s' "$_epipe_probe_hd" | grep -c ':3:' || true)" "1" +check "and does not cover the live one above it" \ + "$(printf '%s' "$_epipe_probe_hd" | grep -c ':1:' || true)" "0" + diff --git a/test/selftest/300-a-test-script-must-be-runnable.sh b/test/selftest/300-a-test-script-must-be-runnable.sh index d8e6e438..ebe601e2 100644 --- a/test/selftest/300-a-test-script-must-be-runnable.sh +++ b/test/selftest/300-a-test-script-must-be-runnable.sh @@ -236,7 +236,9 @@ check_num "premise: the documents name a population of commands, not none" \ _tsm_uncovered="" for _tsm_d in "${_tsm_dirs[@]}"; do _tsm_b="$(basename "$_tsm_d")" - printf '%s\n' "$_tsm_named" | grep -q "^$_tsm_b/" \ + # grep -c on a here-string, not `printf | grep -q`; see selftest 080 and the + # note in 350. The piped form reports a present name as absent under load. + [ "$(grep -c "^$_tsm_b/" <<<"$_tsm_named" || true)" != 0 ] \ || _tsm_uncovered="$_tsm_uncovered $_tsm_b" done check "premise: and they name at least one command in every swept directory" \ diff --git a/test/selftest/340-the-binary-must-be-built-from.sh b/test/selftest/340-the-binary-must-be-built-from.sh index ad34dd2a..9f68399f 100644 --- a/test/selftest/340-the-binary-must-be-built-from.sh +++ b/test/selftest/340-the-binary-must-be-built-from.sh @@ -112,7 +112,8 @@ while IFS= read -r _bd_var; do _bd_name="${_bd_val##*/}" [ -n "$_bd_name" ] || continue _bd_seen=$(( _bd_seen + 1 )) - printf '%s\n' "$_bd_covered" | grep -qx "$_bd_name" || \ + # grep -c on a here-string, not `printf | grep -qx`; see selftest 080. + [ "$(grep -cx "$_bd_name" <<<"$_bd_covered" || true)" != 0 ] || \ _bd_missing="$_bd_missing $_bd_name" done < "[]" or "[n: a b c]" } | sort -u )" while IFS= read -r name; do [ -n "$name" ] || continue - printf '%s\n' "$ondisk" | grep -qxF "$name" && continue + # grep -cxF on a here-string, NOT `printf ... | grep -qxF`. grep -q exits + # the moment it matches, the printf takes EPIPE, and under this suite's + # `set -o pipefail` the pipeline reports failure though the name WAS + # present -- so a name in the corpus is reported absent. Selftest 080 + # states this rule and demonstrates it; its sweep is deliberately + # non-recursive and never looked in here. + # + # It is not latent. It reddened #923's `suites (PG 17)` on a name that + # exists, while PG 18 passed, and reproduces at this corpus size only + # under load: 170 names, 400 trials on a busy machine, 6 false absences + # piped and 0 on a here-string. + [ "$(grep -cxF "$name" <<<"$ondisk" || true)" != 0 ] && continue n=$((n + 1)); [ "$n" -le 6 ] && bad="$bad $name" done < <(grep -oE '`test_[A-Za-z0-9_]*(\.py)?`' "$doc" 2>/dev/null \ | tr -d '`' | sort -u) From 0ce14d79c91b6fa94e7b26f9a394e0521f9bfd2f Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Wed, 9 Sep 2026 21:17:31 -0600 Subject: [PATCH 5/8] test: the pipe-into-grep-q rule sweeps any producer, not just echo and printf (#486) Reported by OffgridwithJD against the previous commit: widening the DIRECTORIES left the PRODUCER scoped to echo and printf, so every ` | grep -q` was still unswept. Twenty-six sites, two of them in lib.sh and shared by every suite that asks whether a plan is a columnar scan. THE SCOPING REVERSAL, AND WHY. The rule's own stated principle is a reader that exits early AND whose EXIT STATUS is the answer being read. That is producer-independent: the writer takes EPIPE whether it is a builtin, a psql, an ldd or an ss. Scoping to echo and printf was a narrower implementation than the principle, justified in the file by "a pipeline out of psql or a file is a different question" -- which is true of `| head -1` used as TEXT and false of `| grep -q` used as a VERDICT. THE WORST SITE IS VACUITY, NOT A FALSE RED. native_vecskip.sh's "premise: and it is not the scalar scan" WANTS "no", so a spurious EPIPE answer makes that premise pass for the wrong reason. It is fixed first for that reason. NOT CLAIMED TO BE LYING TODAY. Measured, 200 trials per size on a loaded box, match always on line one so the answer is knowably yes: 1,892 bytes (EXPLAIN-sized) 0/200 wrong 8,893 bytes 1/200 <- first observed lie 66,894 bytes 21/40 288,894 bytes 40/40 control, match on the LAST line so grep reads to EOF: 0/200 at every size Latent, with no floor in the mechanism -- the probability rises with size rather than crossing a threshold, which refuted a clean pipe-capacity hypothesis. Reasoning about "small enough" is how #473, #476 and selftest 350 each survived, so the rule sweeps rather than reasons. `||` IS NOT A PIPE, and the first version of the widened pattern thought it was: `[ "$rc" = 124 ] || grep -q PAT <<<"$out"` is a fallback branch reading a here-string, with no writer process and so no EPIPE, and both fuzz suites were flagged for it. The leading [^|] excludes it, with an arm pinning that. My own deliberate twin in selftest 390 moved into a quoted heredoc, so the widened sweep exempts it by the property already built rather than by a line number -- a filename list is the thing this rule exists to avoid. THE MATRIX IS THE VERIFICATION HERE. lib.sh's pgc_is_columnar_scan and pgc_uses_row_fetch are shared by many suites, and eighteen suite files changed. The selftest cannot exercise them; the two `suites (PG N)` jobs can. Local: selftest exit 0, 553 checks, 0 failures, shellcheck rc=0 over test/, selftest/ and bench/. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK (cherry picked from commit 0d49e200cc37eb8b871e6e7eba84cab7b542a303) --- bench/build_citus.sh | 4 +- bench/build_timescaledb.sh | 4 +- bench/provision.sh | 4 +- test/arrow_import.sh | 9 ++- test/concurrency.sh | 6 +- test/entry_point_privilege.sh | 9 ++- test/lib.sh | 23 ++++++-- test/native_groupagg.sh | 10 +++- test/native_groupagg_batch.sh | 6 +- test/native_vecskip.sh | 16 ++++-- test/objstore_s3_read.sh | 4 +- test/parquet_import.sh | 9 ++- .../070-and-comm-s-two-inputs-must.sh | 6 +- .../080-no-suite-pipes-a-captured-string.sh | 57 ++++++++++++++++++- .../300-a-test-script-must-be-runnable.sh | 3 +- .../320-a-check-that-could-not-run.sh | 5 +- .../340-the-binary-must-be-built-from.sh | 4 +- .../390-a-registered-suite-must-account.sh | 27 ++++++--- test/sorted_mark_rename.sh | 10 +++- test/sorted_pathkeys.sh | 20 +++++-- test/unique_conc.sh | 6 +- test/update_conc.sh | 6 +- 22 files changed, 193 insertions(+), 55 deletions(-) diff --git a/bench/build_citus.sh b/bench/build_citus.sh index 0a4b2711..aa648f6f 100755 --- a/bench/build_citus.sh +++ b/bench/build_citus.sh @@ -28,7 +28,9 @@ echo "== building Citus $TAG against $("$PG_CONFIG" --version) ($PG_CONFIG)" # Benchmark arms must run on a non-assert PostgreSQL. An assert build's numbers # are invalid, so refuse one the way the rest of the bench tooling does. -if "$PG_CONFIG" --configure | grep -q -- '--enable-cassert'; then +# grep -c, not grep -q; see test/lib.sh's pgc_is_columnar_scan. +_cfg="$("$PG_CONFIG" --configure 2>/dev/null || true)" +if [ "$(grep -c -- '--enable-cassert' <<<"$_cfg" || true)" != 0 ]; then echo "REFUSING: $PG_CONFIG is an assert build; benchmark numbers from it are invalid" >&2 exit 1 fi diff --git a/bench/build_timescaledb.sh b/bench/build_timescaledb.sh index c975b0f0..e1f32bda 100755 --- a/bench/build_timescaledb.sh +++ b/bench/build_timescaledb.sh @@ -28,7 +28,9 @@ echo "== building TimescaleDB $VERSION against $("$PG_CONFIG" --version) ($PG_CO # Benchmark arms must run on a non-assert PostgreSQL. An assert build's numbers # are invalid, so refuse one the way the rest of the bench tooling does. -if "$PG_CONFIG" --configure | grep -q -- '--enable-cassert'; then +# grep -c, not grep -q; see test/lib.sh's pgc_is_columnar_scan. +_cfg="$("$PG_CONFIG" --configure 2>/dev/null || true)" +if [ "$(grep -c -- '--enable-cassert' <<<"$_cfg" || true)" != 0 ]; then echo "REFUSING: $PG_CONFIG is an assert build; benchmark numbers from it are invalid" >&2 exit 1 fi diff --git a/bench/provision.sh b/bench/provision.sh index b1c7a1f1..a94fcea7 100755 --- a/bench/provision.sh +++ b/bench/provision.sh @@ -76,7 +76,9 @@ pg_present() { [ -x "$PREFIX_ROOT/$1/bin/pg_config" ]; } # built with cassert would produce benchmark numbers that are quietly wrong, and # nothing else on the box would notice. pg_is_assert() { - "$PREFIX_ROOT/$1/bin/pg_config" --configure 2>/dev/null | grep -q -- '--enable-cassert' + # grep -c, not grep -q; see test/lib.sh's pgc_is_columnar_scan. + _cfg="$("$PREFIX_ROOT/$1/bin/pg_config" --configure 2>/dev/null || true)" + [ "$(grep -c -- '--enable-cassert' <<<"$_cfg" || true)" != 0 ] } check_pg() { diff --git a/test/arrow_import.sh b/test/arrow_import.sh index afee56bf..b38afad0 100755 --- a/test/arrow_import.sh +++ b/test/arrow_import.sh @@ -579,9 +579,12 @@ idx_count() { # force an index scan SELECT count(*) FROM ix_tgt WHERE id BETWEEN 100 AND 199;" | tail -1 } idx_plan_is_index_scan() { - q "$IDX_SETUP - EXPLAIN (COSTS OFF) SELECT count(*) FROM ix_tgt WHERE id BETWEEN 100 AND 199;" \ - | grep -qi 'Index.*Scan' && echo yes || echo no + # grep -c on a captured value, not a pipe into grep -q; see lib.sh's + # pgc_is_columnar_scan for the mechanism and the measurement. + local _plan + _plan="$(q "$IDX_SETUP + EXPLAIN (COSTS OFF) SELECT count(*) FROM ix_tgt WHERE id BETWEEN 100 AND 199;")" + [ "$(grep -ci 'Index.*Scan' <<<"$_plan" || true)" != 0 ] && echo yes || echo no } seq_count() { # force a sequential scan q "SET enable_indexscan = off; SET enable_bitmapscan = off; diff --git a/test/concurrency.sh b/test/concurrency.sh index a1fe004d..1bd2848c 100755 --- a/test/concurrency.sh +++ b/test/concurrency.sh @@ -67,7 +67,11 @@ LOGFILE="$WORKDIR/server.log" # postmaster. port_is_free() { # port -> 0 if nothing is listening on it if command -v ss >/dev/null 2>&1; then - ! ss -Htln "sport = :$1" 2>/dev/null | grep -q ":$1" + # grep -c on a captured value, not a pipe into grep -q. A spurious + # EPIPE here answers "nothing is listening" for a port that IS taken, + # and the suite then starts a cluster on an occupied port. + _pif="$(ss -Htln "sport = :$1" 2>/dev/null || true)" + [ "$(grep -c ":$1" <<<"$_pif" || true)" = 0 ] else # fall back to a connect probe: a refused connection means free ! (exec 3<>"/dev/tcp/127.0.0.1/$1") 2>/dev/null diff --git a/test/entry_point_privilege.sh b/test/entry_point_privilege.sh index 78bbd981..7c89c58e 100755 --- a/test/entry_point_privilege.sh +++ b/test/entry_point_privilege.sh @@ -307,9 +307,12 @@ as_ep() { # as_ep -> the SQLSTATE, or the literal noerror local out out="$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U t_ep -d "$PGC_DB" \ -At -v VERBOSITY=sqlstate -v ON_ERROR_STOP=0 -c "$1" 2>&1)" - printf '%s\n' "$out" | sed -n 's/^.*ERROR:[[:space:]]*\([0-9A-Z]\{5\}\).*$/\1/p' | head -1 \ - | grep -q . && printf '%s\n' "$out" | sed -n 's/^.*ERROR:[[:space:]]*\([0-9A-Z]\{5\}\).*$/\1/p' | head -1 \ - || echo noerror + # Extract once into a variable rather than twice through a pipeline whose + # STATUS is the answer: `... | grep -q .` reports "no sqlstate" whenever the + # writer takes EPIPE, and this function's answer is a SQLSTATE. + local _sqlstate + _sqlstate="$(sed -n 's/^.*ERROR:[[:space:]]*\([0-9A-Z]\{5\}\).*$/\1/p' <<<"$out" | head -1)" + if [ -n "$_sqlstate" ]; then printf '%s\n' "$_sqlstate"; else echo noerror; fi } # The premise that makes every SQLSTATE arm below mean anything: this role can diff --git a/test/lib.sh b/test/lib.sh index 3ea4d61f..8af23700 100755 --- a/test/lib.sh +++ b/test/lib.sh @@ -1249,9 +1249,17 @@ check_ratio_needs_quiet_machine() { # # EXPLAIN without ANALYZE is enough: the line comes from the plan rather than the # run, so the assertion costs a plan and does not execute the query. pgc_is_columnar_scan() { # query -> yes|no - env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres \ - -d "$PGC_DB" -At -c "EXPLAIN (COSTS OFF) $1" 2>/dev/null \ - | grep -q 'Columnar Projected Columns' && echo yes || echo no + # grep -c, NOT grep -q. grep -q exits the moment it matches, psql is still + # writing, and under a suite's `set -o pipefail` the pipeline reports failure + # though the pattern WAS present -- so this helper answers "no" for a plan that + # contains the line. Latent rather than live at EXPLAIN size (0 wrong in 200 + # trials at 1.9KB) but with no floor in the mechanism: measured 1/200 wrong at + # 8.9KB and 40/40 at 289KB, on a loaded machine. grep -c reads to EOF. + local _plan + _plan="$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres \ + -d "$PGC_DB" -At -c "EXPLAIN (COSTS OFF) $1" 2>/dev/null)" + [ "$(grep -c 'Columnar Projected Columns' <<<"$_plan" || true)" != 0 ] \ + && echo yes || echo no } # Does this query's plan drive the per-row fetch path? @@ -1278,9 +1286,12 @@ pgc_is_columnar_scan() { # query -> yes|no # report "no" for a query that does exercise the cache. Measured: an UPDATE over # 2,000 rows made 20,366 fetch_row calls under a Custom Scan plan (#797). pgc_uses_row_fetch() { # setup query -> yes|no - env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres \ - -d "$PGC_DB" -At -c "$1" -c "EXPLAIN (COSTS OFF) $2" 2>/dev/null \ - | grep -q 'Index Scan using' && echo yes || echo no + # grep -c, not grep -q; see pgc_is_columnar_scan above for the measurement. + local _plan + _plan="$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres \ + -d "$PGC_DB" -At -c "$1" -c "EXPLAIN (COSTS OFF) $2" 2>/dev/null)" + [ "$(grep -c 'Index Scan using' <<<"$_plan" || true)" != 0 ] \ + && echo yes || echo no } # Order-independent set hash of an arbitrary query's result. The row is cast to diff --git a/test/native_groupagg.sh b/test/native_groupagg.sh index c46c86b0..85b3dbbf 100755 --- a/test/native_groupagg.sh +++ b/test/native_groupagg.sh @@ -45,9 +45,13 @@ groupvec_off() { psql_run "ALTER DATABASE $PGC_DB SET pgcolumnar.enable_group_ve # "Columnar Vectorized Group Keys"; no other node emits it, so a positive grep is # proof of the node rather than an absence test that a fallback would also pass. pgc_is_groupvec() { # query -> yes|no - env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres \ - -d "$PGC_DB" -At -c "EXPLAIN (COSTS OFF) $1" 2>/dev/null \ - | grep -q 'Columnar Vectorized Group Keys' && echo yes || echo no + # grep -c on a captured plan, not a pipe into grep -q; see lib.sh's + # pgc_is_columnar_scan for the mechanism and the measurement. + local _plan + _plan="$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres \ + -d "$PGC_DB" -At -c "EXPLAIN (COSTS OFF) $1" 2>/dev/null)" + [ "$(grep -c 'Columnar Vectorized Group Keys' <<<"$_plan" || true)" != 0 ] \ + && echo yes || echo no } # toggle_diff LABEL "QUERY on t_col": same query, path off vs on, byte-exact. diff --git a/test/native_groupagg_batch.sh b/test/native_groupagg_batch.sh index 0149a972..cdf3cfb8 100755 --- a/test/native_groupagg_batch.sh +++ b/test/native_groupagg_batch.sh @@ -59,7 +59,11 @@ q1() { q "$1" | tail -1; } # Keys" is emitted by no other node, so a positive grep proves the node rather # than an absence test a fallback would also satisfy. is_groupvec() { # query -> yes|no - q "EXPLAIN (COSTS OFF) $1" | grep -q 'Columnar Vectorized Group Keys' \ + # grep -c on a captured value, not a pipe into grep -q; see lib.sh's + # pgc_is_columnar_scan for the mechanism and the measurement. + local _plan + _plan="$(q "EXPLAIN (COSTS OFF) $1")" + [ "$(grep -c 'Columnar Vectorized Group Keys' <<<"$_plan" || true)" != 0 ] \ && echo yes || echo no } diff --git a/test/native_vecskip.sh b/test/native_vecskip.sh index 48af9bb2..cadb9798 100755 --- a/test/native_vecskip.sh +++ b/test/native_vecskip.sh @@ -42,7 +42,12 @@ explain_of() { -d "$PGC_DB" -At -c "EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF, SUMMARY OFF) $1" 2>/dev/null } is_scalar_scan() { - explain_of "$1" | grep -q 'Columnar Projected Columns' && echo yes || echo no + # grep -c, not grep -q: see lib.sh's pgc_is_columnar_scan for why and for the + # measurement. This file holds the worst instance of the shape -- the arm at + # "premise: and it is not the scalar scan" WANTS "no", so a spurious EPIPE + # answer makes that premise pass for the wrong reason. Vacuity, not a red. + [ "$(explain_of "$1" | grep -c 'Columnar Projected Columns' || true)" != 0 ] \ + && echo yes || echo no } # The node, before any counter is read out of it. @@ -99,13 +104,16 @@ explain_agg() { # this query falls back to the scalar scan -- which DOES print the line, so the # check below would pass while testing nothing at all. check "premise: the aggregate arm really is a vectorized aggregate" \ - "$(explain_agg "$AGGQ" | grep -q 'Columnar Vectorized Aggregates' && echo yes || echo no)" \ + "$([ "$(explain_agg "$AGGQ" | grep -c 'Columnar Vectorized Aggregates' || true)" != 0 ] \ + && echo yes || echo no)" \ "yes" check "premise: and it is not the scalar scan" \ - "$(explain_agg "$AGGQ" | grep -q 'Columnar Projected Columns' && echo yes || echo no)" "no" + "$([ "$(explain_agg "$AGGQ" | grep -c 'Columnar Projected Columns' || true)" != 0 ] \ + && echo yes || echo no)" "no" check "the vectorized aggregate reports Columnar Vectors Skipped" \ - "$(explain_agg "$AGGQ" | grep -q 'Columnar Vectors Skipped' && echo yes || echo no)" \ + "$([ "$(explain_agg "$AGGQ" | grep -c 'Columnar Vectors Skipped' || true)" != 0 ] \ + && echo yes || echo no)" \ "yes" # Boundary and cross-vector ranges still return exactly the heap rows. diff --git a/test/objstore_s3_read.sh b/test/objstore_s3_read.sh index 28a95e51..9753f49e 100755 --- a/test/objstore_s3_read.sh +++ b/test/objstore_s3_read.sh @@ -166,7 +166,9 @@ pg_restart_env "AWS_ENDPOINT_URL='https://127.0.0.1:$S3_PORT'" \ "AWS_ACCESS_KEY_ID='$AKID'" "AWS_SECRET_ACCESS_KEY='$SECRET'" \ "AWS_REGION='$REGION'" MOD_SO="$(pgc_pg "$PGC_BINDIR/pg_config --pkglibdir" | tail -1)/pgcolumnar_objstore.so" -if pgc_pg "ldd '$MOD_SO'" 2>/dev/null | grep -q libssl; then +# grep -c, not grep -q; see lib.sh's pgc_is_columnar_scan. +_ldd_out="$(pgc_pg "ldd '$MOD_SO'" 2>/dev/null || true)" +if [ "$(grep -c libssl <<<"$_ldd_out" || true)" != 0 ]; then HTTPS_WANT="08006" else HTTPS_WANT="0A000" diff --git a/test/parquet_import.sh b/test/parquet_import.sh index aa99d567..aed0fb3d 100755 --- a/test/parquet_import.sh +++ b/test/parquet_import.sh @@ -167,9 +167,12 @@ idx_count() { # force an index scan SELECT count(*) FROM ix_tgt WHERE id BETWEEN 100 AND 199;" | tail -1 } idx_plan_is_index_scan() { - q "$IDX_SETUP - EXPLAIN (COSTS OFF) SELECT count(*) FROM ix_tgt WHERE id BETWEEN 100 AND 199;" \ - | grep -qi 'Index.*Scan' && echo yes || echo no + # grep -c on a captured value, not a pipe into grep -q; see lib.sh's + # pgc_is_columnar_scan for the mechanism and the measurement. + local _plan + _plan="$(q "$IDX_SETUP + EXPLAIN (COSTS OFF) SELECT count(*) FROM ix_tgt WHERE id BETWEEN 100 AND 199;")" + [ "$(grep -ci 'Index.*Scan' <<<"$_plan" || true)" != 0 ] && echo yes || echo no } seq_count() { # force a sequential scan q "SET enable_indexscan = off; SET enable_bitmapscan = off; diff --git a/test/selftest/070-and-comm-s-two-inputs-must.sh b/test/selftest/070-and-comm-s-two-inputs-must.sh index 391ade65..455d525d 100644 --- a/test/selftest/070-and-comm-s-two-inputs-must.sh +++ b/test/selftest/070-and-comm-s-two-inputs-must.sh @@ -19,7 +19,11 @@ check "premise: some suite still uses comm, or the check below is vacuous" \ _cm_unpinned="" for _f in $_cm_files; do # every `| sort` in a file that uses comm must carry LC_ALL=C - if grep -qE '\|[[:space:]]*sort' "$_f" && grep -E '\|[[:space:]]*sort' "$_f" | grep -qv 'LC_ALL=C'; then + # The second test is grep -c on a captured value, not a pipe into grep -qv; + # see selftest 080. The first reads a FILE and is not a pipeline at all. + _cm_sorts="$(grep -E '\|[[:space:]]*sort' "$_f" || true)" + if [ "$(grep -cE '\|[[:space:]]*sort' "$_f" || true)" != 0 ] \ + && [ "$(grep -cv 'LC_ALL=C' <<<"$_cm_sorts" || true)" != 0 ]; then _cm_unpinned="$_cm_unpinned $(basename "$_f")" fi done diff --git a/test/selftest/080-no-suite-pipes-a-captured-string.sh b/test/selftest/080-no-suite-pipes-a-captured-string.sh index 41157c8a..e19fedbd 100644 --- a/test/selftest/080-no-suite-pipes-a-captured-string.sh +++ b/test/selftest/080-no-suite-pipes-a-captured-string.sh @@ -121,7 +121,40 @@ _epipe_hd="$(_epipe_heredoc_lines "${_epipe_globs[@]}" 2>/dev/null || true)" # and the rule's own explanation -- and the note beside every site that was fixed # -- necessarily spells the shape out. A sweep that counts its own documentation # is selftest 260's mistake. -_epipe_hits="$(grep -nE '(echo|printf)[^|]*\|[[:space:]]*grep -[a-zA-Z]*q' \ +# ANY PRODUCER, not just echo and printf. This reverses a scoping decision +# recorded above, so the reason is recorded too. +# +# The rule's own stated principle is a reader that exits early AND whose EXIT +# STATUS is the answer being read. That is producer-independent: the writer takes +# EPIPE whether it is a builtin, a psql, an ldd or an ss. Scoping to echo and +# printf was a narrower implementation than the principle, justified by "a +# pipeline out of psql or a file is a different question" -- which is true of +# `| head -1` used as TEXT, and false of `| grep -q` used as a VERDICT. +# +# Twenty-six sites were outside the old pattern, two of them in lib.sh and shared +# by every suite that asks whether a plan is a columnar scan. The worst was +# native_vecskip.sh's "premise: and it is not the scalar scan", which WANTS "no": +# a spurious EPIPE answer makes that premise pass for the wrong reason, so the +# race is vacuity there rather than a false red. +# +# NOT CLAIMED TO BE LYING TODAY. Measured by OffgridwithJD, 200 trials per size on +# a loaded box, match always on line one so the answer is knowably yes: +# +# 1,892 bytes (EXPLAIN-sized) 0/200 wrong +# 8,893 bytes 1/200 <- first observed lie +# 66,894 bytes 21/40 +# 288,894 bytes 40/40 +# control, match on the LAST line so grep reads to EOF: 0/200 at every size +# +# It is LATENT, and the mechanism has no floor -- the probability rises with size +# rather than crossing a threshold, which refuted a clean pipe-capacity +# hypothesis. Reasoning about "small enough" is how #473, #476 and selftest 350 +# each survived, so the rule sweeps instead. +# The leading [^|] excludes the `||` OPERATOR. Widening from the echo/printf +# form lost that exclusion for free: `[ "$rc" = 124 ] || grep -q PAT <<<"$out"` +# is a fallback branch reading a here-string -- no writer process, so no EPIPE -- +# and the first version of the widened pattern flagged both fuzz suites for it. +_epipe_hits="$(grep -nE '[^|]\|[[:space:]]*grep[[:space:]]+-[a-zA-Z]*q' \ "${_epipe_globs[@]}" 2>/dev/null \ | grep -v '/harness_selftest.sh:' \ | grep -vE '^[^:]+:[0-9]+:[[:space:]]*#' || true)" @@ -216,9 +249,29 @@ _epipe_shape='x() { printf "%s" "$1" | grep -%s PATTERN; }' } > "$_epipe_probe" _epipe_probe_hd="$(_epipe_heredoc_lines "$_epipe_probe" 2>/dev/null || true)" check "the sweep's pattern sees both lines of the probe" \ - "$(grep -cE '(echo|printf)[^|]*\|[[:space:]]*grep -[a-zA-Z]*q' "$_epipe_probe")" "2" + "$(grep -cE '[^|]\|[[:space:]]*grep[[:space:]]+-[a-zA-Z]*q' "$_epipe_probe")" "2" check "and the heredoc exemption covers the one inside the heredoc, not the other" \ "$(printf '%s' "$_epipe_probe_hd" | grep -c ':3:' || true)" "1" check "and does not cover the live one above it" \ "$(printf '%s' "$_epipe_probe_hd" | grep -c ':1:' || true)" "0" +# The widening itself, asserted. A producer that is neither echo nor printf must +# be caught, or the reversal above is prose and the 26 sites come back. +_epipe_wide="$PGC_WORKDIR/epipe_wide.sh" +printf 'ldd /bin/sh | grep -%s libc && echo yes || echo no\n' q > "$_epipe_wide" +check "the sweep catches a producer that is neither echo nor printf" \ + "$(grep -cE '[^|]\|[[:space:]]*grep[[:space:]]+-[a-zA-Z]*q' "$_epipe_wide")" "1" +check "premise: and the old echo/printf pattern did NOT catch it" \ + "$(grep -cE '(echo|printf)[^|]*\|[[:space:]]*grep -[a-zA-Z]*q' "$_epipe_wide")" "0" + +# `||` IS NOT A PIPE. A fallback branch reading a here-string has no writer +# process and cannot take EPIPE. The first version of the widened pattern flagged +# both fuzz suites for exactly that, so it is pinned here rather than left to be +# rediscovered the next time the pattern is touched. +_epipe_oror="$PGC_WORKDIR/epipe_oror.sh" +printf '[ "$rc" = 124 ] || grep -%s PAT <<<"$out"\n' q > "$_epipe_oror" +check "the sweep does not mistake the || operator for a pipe" \ + "$(grep -cE '[^|]\|[[:space:]]*grep[[:space:]]+-[a-zA-Z]*q' "$_epipe_oror")" "0" +check "premise: and the line really does hold the reader it must not flag" \ + "$(grep -c 'grep -q PAT' "$_epipe_oror")" "1" + diff --git a/test/selftest/300-a-test-script-must-be-runnable.sh b/test/selftest/300-a-test-script-must-be-runnable.sh index ebe601e2..c68d6835 100644 --- a/test/selftest/300-a-test-script-must-be-runnable.sh +++ b/test/selftest/300-a-test-script-must-be-runnable.sh @@ -63,7 +63,8 @@ _tsm_root="$(cd "$PGC_TESTDIR/.." && pwd)" _tsm_has_shebang() { # _tsm_has_shebang FILE - head -1 "$1" 2>/dev/null | grep -q '^#!' + # grep -c on a captured line, not a pipe into grep -q; see selftest 080. + [ "$(head -1 "$1" 2>/dev/null | grep -c '^#!' || true)" != 0 ] } # ok -- shebang and bit, or neither: internally consistent 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 82b62f75..e6b56e12 100644 --- a/test/selftest/320-a-check-that-could-not-run.sh +++ b/test/selftest/320-a-check-that-could-not-run.sh @@ -212,8 +212,9 @@ 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 + # grep -c on a captured window, not a pipe into grep -q; see selftest 080. + _cnt_win="$(sed -n "$((_cnt_ln > 3 ? _cnt_ln - 3 : 1)),$((_cnt_ln + 6))p" "$_cnt_f")" + if [ "$(grep -cE 'PGC_PASSED=|PGC_FAILED=|PGC_UNRUN=' <<<"$_cnt_win" || true)" = 0 ]; then _cnt_n=$((_cnt_n + 1)) [ "$_cnt_n" -le 5 ] && _cnt_bad="$_cnt_bad ${_cnt_f##*/}:$_cnt_ln" fi diff --git a/test/selftest/340-the-binary-must-be-built-from.sh b/test/selftest/340-the-binary-must-be-built-from.sh index 9f68399f..584a01ca 100644 --- a/test/selftest/340-the-binary-must-be-built-from.sh +++ b/test/selftest/340-the-binary-must-be-built-from.sh @@ -877,7 +877,9 @@ printf 'x\n' > "$_lc/tree/pgcolumnar.control" _lc_have="" for _lc_l in C C.utf8 en_US.utf8; do - locale -a 2>/dev/null | grep -qx "$_lc_l" && _lc_have="$_lc_have $_lc_l" + # grep -c, not grep -q; see selftest 080. locale -a lists hundreds of names. + [ "$(locale -a 2>/dev/null | grep -cx "$_lc_l" || true)" != 0 ] \ + && _lc_have="$_lc_have $_lc_l" done _lc_count="$(printf '%s\n' $_lc_have | grep -c .)" diff --git a/test/selftest/390-a-registered-suite-must-account.sh b/test/selftest/390-a-registered-suite-must-account.sh index e7d8cb1e..98f933c1 100644 --- a/test/selftest/390-a-registered-suite-must-account.sh +++ b/test/selftest/390-a-registered-suite-must-account.sh @@ -451,14 +451,23 @@ check "a long suite that calls pgc_summary still declares accounting" \ "$(pgc_suite_declares_accounting "$_bigsuite")" "yes" # And prove the arm can fail. The twin is the SHAPE that was wrong, restated -# here rather than extracted, because the wrong version is no longer in the tree. -_grepq_twin() { # the original shape, restated as tightly as it can be - sed 's/#.*$//' "$1" \ - | grep -qE '(^|[^_[:alnum:]])pgc_summary([^_[:alnum:]]|$)' && echo yes || echo no -} -check "premise: the grep -q twin is a different function from the real one" \ - "$(type -t _grepq_twin)" "function" +# rather than extracted, because the wrong version is no longer in the tree. +# +# It lives in a QUOTED HEREDOC, like selftest 080's own control and for the same +# reason: 080 now sweeps every producer piped into an early-exit reader, so a +# deliberate demonstration of the forbidden shape has to be text being written to +# a file rather than a pipeline this suite runs. Exempted by property, not by a +# line number. +_grepq_twin_sh="$_acc/grepq_twin.sh" +cat > "$_grepq_twin_sh" <<'TWIN' +set -uo pipefail +sed 's/#.*$//' "$1" \ + | grep -qE '(^|[^_[:alnum:]])pgc_summary([^_[:alnum:]]|$)' && echo yes || echo no +TWIN +check "premise: the twin script was written and is runnable" \ + "$([ -s "$_grepq_twin_sh" ] && echo yes || echo no)" "yes" check "the grep -q shape is the one that gets this wrong under pipefail" \ - "$(_grepq_twin "$_bigsuite")" "no" + "$(bash "$_grepq_twin_sh" "$_bigsuite")" "no" check "and it agrees with the real reader on a SHORT file, which is why it survived review" \ - "$(_grepq_twin "$_acc/declares.sh")" "$(pgc_suite_declares_accounting "$_acc/declares.sh")" + "$(bash "$_grepq_twin_sh" "$_acc/declares.sh")" \ + "$(pgc_suite_declares_accounting "$_acc/declares.sh")" diff --git a/test/sorted_mark_rename.sh b/test/sorted_mark_rename.sh index 8acff70e..bfc3c46c 100755 --- a/test/sorted_mark_rename.sh +++ b/test/sorted_mark_rename.sh @@ -175,9 +175,13 @@ check "so vacuum_sorted on the child does the work" \ # This is why the cascade is not a cosmetic gap. #751 reads the same mark to # claim a pathkey, so a stale mark makes the planner drop the Sort. sorts() { - env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres -d "$PGC_DB" -At \ - -c "EXPLAIN (COSTS OFF) $1" 2>/dev/null \ - | grep -qE '^ *(->)? *(Incremental )?Sort' && echo yes || echo no + # grep -c on a captured value, not a pipe into grep -q; see lib.sh's + # pgc_is_columnar_scan for the mechanism and the measurement. + local _plan + _plan="$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres -d "$PGC_DB" -At \ + -c "EXPLAIN (COSTS OFF) $1" 2>/dev/null)" + [ "$(grep -cE '^ *(->)? *(Incremental )?Sort' <<<"$_plan" || true)" != 0 ] \ + && echo yes || echo no } psql_run "CREATE TABLE wp (k int, j int) PARTITION BY RANGE (k);" psql_run "CREATE TABLE wp1 PARTITION OF wp FOR VALUES FROM (0) TO (100000) USING pgcolumnar;" diff --git a/test/sorted_pathkeys.sh b/test/sorted_pathkeys.sh index 911a152d..aa4090cb 100755 --- a/test/sorted_pathkeys.sh +++ b/test/sorted_pathkeys.sh @@ -45,9 +45,13 @@ pgc_check_ordered_oracle # Does the plan for this query contain a Sort (or Incremental Sort) node? sorts() { # sorts QUERY -> yes|no - env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres \ - -d "$PGC_DB" -At -c "EXPLAIN (COSTS OFF) $1" 2>/dev/null \ - | grep -qE '^ *(->)? *(Incremental )?Sort' && echo yes || echo no + # grep -c on a captured value, not a pipe into grep -q; see lib.sh's + # pgc_is_columnar_scan for the mechanism and the measurement. + local _plan + _plan="$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres \ + -d "$PGC_DB" -At -c "EXPLAIN (COSTS OFF) $1" 2>/dev/null)" + [ "$(grep -cE '^ *(->)? *(Incremental )?Sort' <<<"$_plan" || true)" != 0 ] \ + && echo yes || echo no } inv() { # inversions on the lead column in the order the scan returns rows q "SELECT count(*) FROM (SELECT $2, lag($2) OVER () AS p FROM $1) s WHERE p > $2;" @@ -443,10 +447,14 @@ check "REFUSE: a recorded name that no longer resolves is not a claim" \ # sorts() runs one statement, so the SET has to travel with the connection. sorts_off() { - env PATH="$PGC_BINDIR:$PATH" PGOPTIONS="-c pgcolumnar.enable_sorted_pathkeys=off" \ + # grep -c on a captured value, not a pipe into grep -q; see lib.sh's + # pgc_is_columnar_scan for the mechanism and the measurement. + local _plan + _plan="$(env PATH="$PGC_BINDIR:$PATH" PGOPTIONS="-c pgcolumnar.enable_sorted_pathkeys=off" \ psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres -d "$PGC_DB" -At \ - -c "EXPLAIN (COSTS OFF) $1" 2>/dev/null \ - | grep -qE '^ *(->)? *(Incremental )?Sort' && echo yes || echo no + -c "EXPLAIN (COSTS OFF) $1" 2>/dev/null)" + [ "$(grep -cE '^ *(->)? *(Incremental )?Sort' <<<"$_plan" || true)" != 0 ] \ + && echo yes || echo no } check "premise: the claim is live with the GUC on" "$(sorts 'SELECT k FROM c ORDER BY k')" "no" check "control: pgcolumnar.enable_sorted_pathkeys = off restores the Sort" \ diff --git a/test/unique_conc.sh b/test/unique_conc.sh index ead78fb4..a09c03cb 100755 --- a/test/unique_conc.sh +++ b/test/unique_conc.sh @@ -69,7 +69,11 @@ LOGFILE="$WORKDIR/server.log" port_is_free() { if command -v ss >/dev/null 2>&1; then - ! ss -Htln "sport = :$1" 2>/dev/null | grep -q ":$1" + # grep -c on a captured value, not a pipe into grep -q. A spurious + # EPIPE here answers "nothing is listening" for a port that IS taken, + # and the suite then starts a cluster on an occupied port. + _pif="$(ss -Htln "sport = :$1" 2>/dev/null || true)" + [ "$(grep -c ":$1" <<<"$_pif" || true)" = 0 ] else ! (exec 3<>"/dev/tcp/127.0.0.1/$1") 2>/dev/null fi diff --git a/test/update_conc.sh b/test/update_conc.sh index bd677399..a85e8b3d 100755 --- a/test/update_conc.sh +++ b/test/update_conc.sh @@ -77,7 +77,11 @@ LOGFILE="$WORKDIR/server.log" port_is_free() { if command -v ss >/dev/null 2>&1; then - ! ss -Htln "sport = :$1" 2>/dev/null | grep -q ":$1" + # grep -c on a captured value, not a pipe into grep -q. A spurious + # EPIPE here answers "nothing is listening" for a port that IS taken, + # and the suite then starts a cluster on an occupied port. + _pif="$(ss -Htln "sport = :$1" 2>/dev/null || true)" + [ "$(grep -c ":$1" <<<"$_pif" || true)" = 0 ] else ! (exec 3<>"/dev/tcp/127.0.0.1/$1") 2>/dev/null fi From 09ec8b71ef9cf8c74ca9c1f34d3b09e203fcfbfb Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Wed, 9 Sep 2026 21:30:16 -0600 Subject: [PATCH 6/8] test: the partition arm could not fail, and nothing exercised the ^ anchor (#916) Both reported by OffgridwithJD against 0ce14d7, and both are the shapes this change exists to refuse, committed inside it. THE PARTITION ARM WAS AN IDENTITY. `_reg` was incremented in the same loop body as the three buckets, so their sum equalled it for ANY reader. Proven rather than argued: it passes with an always-yes reader and with an always-no reader alike, and adding the absent bucket did not change that. A total derived from the loop that produces the buckets cannot fail, which is the note I had just re-worded two functions away for the same reason. The population is now counted by a SECOND ROUTE -- the runner's own --list-suites -- so the arm fails when the classification does not see every registered suite: a future `continue`, a read that drops a line, a list that changes between the two reads. And it now says what it is. It is a COVERAGE check, not a check on the reader's correctness. The two arms below it, which require both buckets to be occupied, are what catch a reader that answers the same way for everything. Overrating it is how it survived as an identity. NOTHING EXERCISED THE ^ ANCHOR. pgc_log_shows_accounting's comment claims "anchored and fully shaped, so the word appearing in a suite's own prose cannot satisfy it", and the prose fixture is refused by the regex SHAPE rather than by the anchor -- so removing ^ from the reader left every arm in both harnesses green. The indented fixtures added earlier are for the DECLARATION reader and do not reach this one. The distinguishing input is a well-formed accounting line that does not start its line. Measured against a twin with the anchor removed, mutation asserted applied: indented line, real reader (anchored) no indented line, twin reader (unanchored) yes control, line-start, real reader yes Inert on real data -- 0 non-line-start occurrences across 246 PG17 logs and 244 PG18 -- so this closes a coverage gap rather than a live defect. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK --- test/pytest/test_suite_accounting.py | 26 ++++++++++++++-- .../390-a-registered-suite-must-account.sh | 31 +++++++++++++++++-- 2 files changed, 53 insertions(+), 4 deletions(-) diff --git a/test/pytest/test_suite_accounting.py b/test/pytest/test_suite_accounting.py index 5d62b8e2..8d68aa29 100644 --- a/test/pytest/test_suite_accounting.py +++ b/test/pytest/test_suite_accounting.py @@ -130,6 +130,18 @@ def test_the_accounting_line_is_read_on_every_exit_path(tmp_path, expect): expect.text(_call("pgc_log_shows_accounting", str(tmp_path / "gone.log"))[0].strip(), "no", "an absent log shows none rather than erroring") + # THE ^ ANCHOR, which nothing above exercises: the prose fixture is refused by + # the regex SHAPE, not by the anchor, so removing ^ from the reader left every + # arm here green. Reported by OffgridwithJD. The distinguishing input is a + # well-formed accounting line that does not start its line. + indented = _write(tmp_path, "indented.log", + " accounting: 3 passed + 0 failed + 0 unrunnable = 3\nx.sh: PASSED\n") + expect.num(pathlib.Path(indented).read_text() + .count("accounting: 3 passed + 0 failed + 0 unrunnable = 3"), 1, + "premise: the fixture carries a well-formed line, just indented") + expect.text(_call("pgc_log_shows_accounting", indented)[0].strip(), "no", + "an accounting line that does not start its line is refused") + def test_the_reader_accepts_the_line_the_producer_actually_emits(tmp_path, expect): """Every log above is a literal typed into this file, and the shell half types the @@ -290,8 +302,18 @@ def test_the_partition_over_the_registered_suites_adds_up(expect): capture_output=True, text=True).stdout.split() yes, no = verdicts.count("yes"), verdicts.count("no") - print(f" registered={len(listed)} | declares={yes}, does not={no} | sum={yes + no}") - expect.num(yes + no, len(listed), "the partition covers every registered suite") + absent = verdicts.count("absent") + print(f" registered={len(listed)} | declares={yes}, does not={no}, " + f"absent={absent} | sum={yes + no + absent}") + # A COVERAGE check, and only that. It fails when the classification does not + # see every registered suite -- a dropped path, a list that changed between + # the two reads. It is NOT a check on the reader's correctness: the two arms + # below, which require both buckets occupied, are what catch a reader that + # answers the same way for everything. The population is counted from the + # runner's own list, a different route from the verdicts. + expect.num(yes + no + absent, len(listed), + "the partition covers every registered suite") + expect.num(absent, 0, "every registered suite has a file") expect.at_least(no, 1, "the reader does not answer yes for every suite") expect.at_least(yes, 1, "nor no for every one of them") diff --git a/test/selftest/390-a-registered-suite-must-account.sh b/test/selftest/390-a-registered-suite-must-account.sh index 98f933c1..f9927805 100644 --- a/test/selftest/390-a-registered-suite-must-account.sh +++ b/test/selftest/390-a-registered-suite-must-account.sh @@ -186,6 +186,20 @@ printf 'this suite prints the word accounting: in prose\n' > "$_acc/prose.log" check "and prose containing the word does not count as the line" \ "$(pgc_log_shows_accounting "$_acc/prose.log")" "no" +# THE ^ ANCHOR, which nothing above exercises. The prose fixture is refused by the +# regex SHAPE, not by the anchor, so removing ^ from the reader left every arm +# green -- reported by OffgridwithJD. The distinguishing input is a well-formed +# accounting line that does NOT start the line, which is what a nested or indented +# suite run produces. Inert on real data today (0 non-line-start occurrences +# across 246 PG17 logs and 244 PG18), so this closes a coverage gap rather than a +# live defect. +printf ' accounting: 3 passed + 0 failed + 0 unrunnable = 3\nx.sh: PASSED\n' \ + > "$_acc/indented_acc.log" +check "premise: the fixture carries a well-formed accounting line, just indented" \ + "$(grep -c 'accounting: 3 passed + 0 failed + 0 unrunnable = 3' "$_acc/indented_acc.log")" "1" +check "an accounting line that does not start its line is refused" \ + "$(pgc_log_shows_accounting "$_acc/indented_acc.log")" "no" + check "an absent log shows no accounting rather than erroring" \ "$(pgc_log_shows_accounting "$_acc/absent.log")" "no" @@ -392,9 +406,22 @@ check "and the real function reconciles the same input, so the arm is not noise" # THREE buckets, not two. Folding "absent" into "does not declare" is the # conflation the reader was just fixed for, and repeating it here would leave the # real population the one place it still happened. -_reg=0; _decl_n=0; _exempt_n=0; _absent_n=0 +# The population is counted by a SECOND ROUTE, not by the loop that classifies it. +# The first version incremented _reg in the same loop body as the buckets, so the +# sum equalled it for ANY reader -- OffgridwithJD proved it passes with an +# always-yes reader and with an always-no reader alike. A total derived from the +# loop that produces the buckets is an identity, which is the shape this file +# spends its length refusing. +# +# WHAT THIS ARM IS, said plainly so the next reader does not overrate it: a +# COVERAGE check. It fails when the classification loop does not see every +# registered suite -- a future `continue`, a read that drops a line, a list that +# changes between the two reads. It is NOT a check on the reader's correctness; +# the two arms below it, which require both buckets to be occupied, are what +# catch a reader answering the same way for everything. +_reg="$(listed_suites | grep -c . || true)" +_decl_n=0; _exempt_n=0; _absent_n=0 while IFS= read -r _s; do - _reg=$((_reg + 1)) case "$(pgc_suite_declares_accounting "$PGC_TESTDIR/${_s}.sh")" in yes) _decl_n=$((_decl_n + 1)) ;; absent) _absent_n=$((_absent_n + 1)); echo " registered but has no file: $_s.sh" ;; From 4eca4d224c4df916e0842dbc62e67bc05585354c Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Thu, 10 Sep 2026 06:48:58 -0600 Subject: [PATCH 7/8] test: the registered set is an input, so a suite nothing accounts for fails (#916) Blocking review by @linuxhikerpm, and the finding is structural and correct. pgc_reconcile_accounting takes the DECLARED set and the OBSERVED set. Both are derived from the suites themselves, so a registered suite in NEITHER is outside the universe it reconciles. Driven from the function, on the head under review: accounting reconciliation: inputs=0 | both=0, declared only=0, accounted only=0 | sum=0 rc=0 with the registered set holding a name the function has no argument to see. As they put it: treating absence of a declaration as absence from the population preserves the overcount this change is named for. THE POPULATION IS NOW ITS OWN CHECK, and the registered set is its first input. Every registered suite lands in exactly one of four buckets: accounted its log shows it counted its checks not dispatched the driver recorded that it never ran it known debt named in a tracked debt file unaccounted none of the above -- FAILS, by name ACCOUNTED TAKES EITHER MECHANISM, both runtime-observable. pgc_summary's accounting line covers 239 suites. bench_guards and docs_style keep private counters and print their own `checks run:` without ever sourcing lib.sh, so a reader that knew only the first would call them unaccounted, which is false. Measured over all twelve non-declaring suites: exactly two print a runtime count, ten print none -- which is where the ten and the twelve come from, and why they are different numbers. Both mechanisms are derived rather than declared, so a suite that adopts either leaves the debt bucket on its own. That is the property that stops the debt file becoming a permission slip. THE DEBT FILE IS DEBT. test/suites_without_accounting.txt names the ten suites that count nothing at runtime, generated from a measurement rather than typed. It is tracked, so adding a name is a diff a reviewer sees -- which is why it is a file and not a number in the environment, per #858's own constraint. A name that starts accounting, or stops being registered, is REPORTED rather than fatal: a gate that reddens the moment someone fixes something teaches people not to. Failing the live population outright would redden CI ten times from the day it lands, which is the failure mode this family exists to prevent. Recording those ten by name makes an eleventh fail while the ten are excused, which is the difference between a burn-down and an exemption. Every one of the three ways out is asserted to actually let a suite out, or "unaccounted" would be a name for "always fails" and only the debt file would be doing any work. And inputs == sum(buckets) over the registered population CAN be false on the data, unlike the symmetry check's identity: a name can fall outside all four buckets. Both harnesses, and the arm @linuxhikerpm asked for verbatim: registered={alpha}, everything else empty, must fail and name alpha. One of my own arms was wrong on the way: it asserted the string "accounting.registered" appeared twice, which is a fact about how many times a variable is spelled. It now asserts the property -- that the registered file is written from the SUITES array itself. Evidence: selftest exit 0, 538 checks, 0 failures; 14 pytest; shellcheck rc=0 across test/, selftest/ and bench/; docs_style PASSED. The live population is the matrix's job and it has not run yet. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK --- test/pytest/TESTS.md | 40 ++++++ test/pytest/test_suite_accounting.py | 111 ++++++++++++++++ test/run_all_versions.sh | 111 ++++++++++++++++ .../390-a-registered-suite-must-account.sh | 122 ++++++++++++++++++ test/suites_without_accounting.txt | 27 ++++ 5 files changed, 411 insertions(+) create mode 100644 test/suites_without_accounting.txt diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index c6b62532..082fd3cd 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -1096,6 +1096,46 @@ suites are exempt is not a fact about correctness, and pinning it would be a sec copy of the list this design removes. What is asserted is that the partition covers the population and that both buckets are occupied. +### `test_the_accounted_reader_takes_either_runtime_mechanism` + +Two runtime-observable mechanisms exist: `pgc_summary`'s accounting line, used by 239 +suites, and a suite's own `checks run:` line, which `bench_guards` and `docs_style` +print from private counters without ever sourcing `lib.sh`. A reader that knew only the +first would call those two unaccounted, which is false. + +### `test_a_registered_suite_accounted_by_nothing_fails_by_name` + +The defect @linuxhikerpm blocked #922 on. `pgc_reconcile_accounting` takes the declared +and observed sets, both derived from the suites themselves, so a registered suite in +neither is **outside the universe it reconciles** — with all its inputs empty it reports +complete symmetry and returns 0, whatever `SUITES` holds. Treating absence of a +declaration as absence from the population preserves the overcount. + +`pgc_reconcile_population` takes the registered set as an input and puts every +registered suite in exactly one of four buckets: accounted, not dispatched, known debt, +or unaccounted — and unaccounted fails, by name. Each of the three ways out is asserted +to actually let a suite out, or the bucket would be a name for "always fails". + +### `test_the_debt_file_excuses_only_what_it_names` + +Debt is recorded by name rather than as a count, which is what makes it a burn-down: a +new unaccounted suite fails while the known ones are excused. Debt that is no longer +debt — a suite that now accounts, or one no longer registered — is reported, so the +burn-down cannot stall silently. Those two are reported rather than fatal: a gate that +reddens the moment someone *fixes* something teaches people not to fix things. + +### `test_the_population_partitions_and_prints_its_identity` + +`inputs == sum(buckets)` over the registered population. Unlike the symmetry check's +identity, this one can be false on the data: a name can genuinely fall outside all four +buckets. + +### `test_the_debt_file_is_tracked_and_holds_only_registered_suites` + +`test/suites_without_accounting.txt` is tracked so that adding a name is a diff a +reviewer sees — the whole reason it is a file and not a number in the environment. Every +name in it must be a registered suite. + ### `test_the_declaration_reader_survives_pipefail_on_a_long_suite` A regression arm. The first implementation piped `sed` into `grep -q`; grep exits on diff --git a/test/pytest/test_suite_accounting.py b/test/pytest/test_suite_accounting.py index 8d68aa29..9d52b53a 100644 --- a/test/pytest/test_suite_accounting.py +++ b/test/pytest/test_suite_accounting.py @@ -353,3 +353,114 @@ def test_the_declaration_reader_survives_pipefail_on_a_long_suite(tmp_path, expe capture_output=True, text=True).stdout.strip() expect.text(got_short, "yes", "and it agrees on a SHORT file, which is why it survived review") + + +# ---- the population, which the symmetry check cannot see -------------------- + + +def test_the_accounted_reader_takes_either_runtime_mechanism(tmp_path, expect): + """Two mechanisms exist in the tree, and both are runtime-observable. + + `pgc_summary` prints the accounting line and 239 suites use it. `bench_guards` + and `docs_style` keep private counters and print their own `checks run:`; they + never source `lib.sh`, so a reader that only knows the first would call them + unaccounted, which is false. Both are derived rather than declared, so a suite + that adopts either leaves the debt bucket on its own. + """ + lib = _write(tmp_path, "lib.log", + "accounting: 1 passed + 0 failed + 0 unrunnable = 1\nx.sh: PASSED\n") + own = _write(tmp_path, "own.log", "checks run: 9\ndocs_style.sh: PASSED\n") + neither = _write(tmp_path, "none.log", "some output\nPASSED\n") + expect.text(_call("pgc_log_shows_any_accounting", lib)[0].strip(), "yes", + "a log carrying lib.sh's accounting line is accounted") + expect.text(_call("pgc_log_shows_any_accounting", own)[0].strip(), "yes", + "and a log carrying only its own checks-run line is too") + expect.text(_call("pgc_log_shows_any_accounting", neither)[0].strip(), "no", + "a log carrying neither is not accounted") + + +def test_a_registered_suite_accounted_by_nothing_fails_by_name(tmp_path, expect): + """The defect @linuxhikerpm blocked on, stated as its own arm. + + `pgc_reconcile_accounting` takes the declared and observed sets, both derived + from the suites themselves, so a registered suite in neither is outside the + universe it reconciles -- with all its inputs empty it reports complete + symmetry and returns 0, whatever SUITES holds. Treating absence of a + declaration as absence from the population preserves the overcount. + """ + reg = _write(tmp_path, "reg", "alpha\n") + acct = _write(tmp_path, "acct", "") + nd = _write(tmp_path, "nd", "") + debt = _write(tmp_path, "debt", "") + out, rc = _call("pgc_reconcile_population", reg, acct, nd, debt) + expect.num(rc, 1, "a registered suite accounted by nothing fails") + expect.num(out.count("registered but accounted by nothing: alpha"), 1, + "and is named, which the symmetry check could never do") + + # Each way out must actually let it out, or the bucket is a name for + # "always fails" and only the debt file is doing any work. + for label, f in (("accounted", acct), ("not dispatched", nd), ("known debt", debt)): + pathlib.Path(acct).write_text("") + pathlib.Path(nd).write_text("") + pathlib.Path(debt).write_text("") + pathlib.Path(f).write_text("alpha\n") + expect.num(_call("pgc_reconcile_population", reg, acct, nd, debt)[1], 0, + f"a suite that is {label} passes") + + +def test_the_debt_file_excuses_only_what_it_names(tmp_path, expect): + """Recording debt by NAME rather than as a count is what makes this a + burn-down: a new unaccounted suite must fail while the known ones are excused, + and debt that is no longer debt must be reported so it cannot stall.""" + reg = _write(tmp_path, "reg", "alpha\nbeta\n") + acct = _write(tmp_path, "acct", "") + nd = _write(tmp_path, "nd", "") + debt = _write(tmp_path, "debt", "alpha\n") + out, rc = _call("pgc_reconcile_population", reg, acct, nd, debt) + expect.num(out.count("registered but accounted by nothing: beta"), 1, + "a NEW unaccounted suite fails while the known debt is excused") + expect.num(out.count("registered but accounted by nothing: alpha"), 0, + "and the excused one is not named as a failure") + + reg = _write(tmp_path, "reg", "alpha\n") + acct = _write(tmp_path, "acct", "alpha\n") + debt = _write(tmp_path, "debt", "alpha\n") + out, _ = _call("pgc_reconcile_population", reg, acct, nd, debt) + expect.num(out.count("listed as debt but now accounts: alpha"), 1, + "a suite that now accounts but is still listed as debt is reported") + + debt = _write(tmp_path, "debt", "gone\n") + out, _ = _call("pgc_reconcile_population", reg, acct, nd, debt) + expect.num(out.count("listed as debt but not registered: gone"), 1, + "and debt naming a suite that is not registered is reported") + + +def test_the_population_partitions_and_prints_its_identity(tmp_path, expect): + """inputs == sum(buckets) over the REGISTERED population -- the identity the + symmetry check could not offer, because there a name can genuinely fall + outside all four buckets.""" + reg = _write(tmp_path, "reg", "a\nb\nc\nd\n") + acct = _write(tmp_path, "acct", "a\n") + nd = _write(tmp_path, "nd", "b\n") + debt = _write(tmp_path, "debt", "c\n") + out, rc = _call("pgc_reconcile_population", reg, acct, nd, debt) + expect.num(out.count("registered=4 | accounted=1, not dispatched=1, " + "known debt=1, unaccounted=1 | sum=4"), 1, + "the population partitions and prints inputs == sum(buckets)") + expect.num(rc, 1, "and the one outside every excuse still fails") + + +def test_the_debt_file_is_tracked_and_holds_only_registered_suites(expect): + """It is a tracked file so that adding a name is a diff a reviewer sees -- + which is the whole reason it is not a number in the environment.""" + debt = REPO / "test" / "suites_without_accounting.txt" + expect.text("yes" if debt.exists() else "no", "yes", "the debt file is in the tree") + names = [l.strip() for l in debt.read_text().splitlines() + if l.strip() and not l.startswith("#")] + expect.at_least(len(names), 1, "premise: it names some debt to check") + + listed = subprocess.run(["bash", str(RUNNER), "--list-suites"], + capture_output=True, text=True).stdout.split() + strays = sorted(set(names) - set(listed)) + print(f" debt={len(names)} registered={len(listed)} strays={strays}") + expect.num(len(strays), 0, "every name in the debt file is a registered suite") diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index 5898ae7c..0b7d685a 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -940,6 +940,99 @@ pgc_log_shows_accounting() { # pgc_log_shows_accounting LOGFILE -> yes|no fi } +pgc_log_shows_any_accounting() { # pgc_log_shows_any_accounting LOGFILE -> yes|no + # Did this suite count its checks AT RUNTIME, by any mechanism the log shows? + # + # Two exist in the tree. lib.sh's pgc_summary prints the accounting line, and + # 239 suites use it. bench_guards and docs_style keep private counters and + # print their own `checks run: N`; they never source lib.sh, so the first + # reader cannot see them, and calling them unaccounted would be false. + # + # Both are runtime-observable and derived rather than declared, so a suite + # that adopts either mechanism leaves the debt bucket on its own -- which is + # the property that keeps the debt file from becoming a permission slip. + local _log="$1" + [ -f "$_log" ] || { echo no; return 0; } + if [ "$(grep -cE '^accounting: [0-9]+ passed \+ [0-9]+ failed \+ [0-9]+ unrunnable = [0-9]+$' "$_log" || true)" != 0 ] \ + || [ "$(grep -cE '^checks run: [0-9]+$' "$_log" || true)" != 0 ]; then + echo yes + else + echo no + fi +} + +pgc_reconcile_population() { # pgc_reconcile_population REGISTERED ACCOUNTED NOTDISPATCHED DEBT -> 0 ok, 1 unaccounted + # THE REGISTERED SET IS AN INPUT. pgc_reconcile_accounting reconciles the + # declared set against the observed one, and both are derived from the suites + # themselves -- so a registered suite in NEITHER is outside the universe it + # reconciles. Driven from that function with all its inputs empty: it prints + # `inputs=0 | both=0 ... sum=0` and returns 0, whatever SUITES holds. + # + # Reported by @linuxhikerpm, structurally: treating absence of a declaration + # as absence from the population preserves the overcount this change is named + # for. So the population is reconciled separately, over its own four buckets, + # and every registered suite must land in exactly one: + # + # accounted its log shows it counted its checks, by either mechanism + # not dispatched the driver recorded that it never ran it + # known debt named in the tracked debt file + # unaccounted none of the above -- FAILS, by name + # + # The debt file is DEBT, not an exemption: it is tracked, so adding a name is + # a diff a reviewer sees, and a name that starts accounting or stops being + # registered is reported so the burn-down cannot stall silently. + local _reg="$1" _acct="$2" _nd="${3:-}" _debt="${4:-}" _rc=0 + local _rf _af _ndf _df _unacc _stale_acct _stale_reg _n + local _nreg _nacc _nnd _ndebt _nunacc _sum + + _rf="$(mktemp)"; _af="$(mktemp)"; _ndf="$(mktemp)"; _df="$(mktemp)" + LC_ALL=C sort -u "$_reg" 2>/dev/null | sed '/^$/d' >"$_rf" + LC_ALL=C sort -u "$_acct" 2>/dev/null | sed '/^$/d' >"$_af" + [ -n "$_nd" ] && [ -f "$_nd" ] && LC_ALL=C sort -u "$_nd" | sed '/^$/d' >"$_ndf" + [ -n "$_debt" ] && [ -f "$_debt" ] && \ + grep -vE '^[[:space:]]*(#|$)' "$_debt" | LC_ALL=C sort -u >"$_df" + + # Buckets, in precedence order, so each registered name lands in exactly one. + local _t1 _t2 + _t1="$(mktemp)"; _t2="$(mktemp)" + LC_ALL=C comm -23 "$_rf" "$_af" >"$_t1" # registered, not accounted + LC_ALL=C comm -23 "$_t1" "$_ndf" >"$_t2" # ... nor not-dispatched + _unacc="$(LC_ALL=C comm -23 "$_t2" "$_df")" # ... nor known debt + + _nreg="$(grep -c . "$_rf" || true)" + _nacc="$(LC_ALL=C comm -12 "$_rf" "$_af" | grep -c . || true)" + _nnd="$(LC_ALL=C comm -12 "$_t1" "$_ndf" | grep -c . || true)" + _ndebt="$(LC_ALL=C comm -12 "$_t2" "$_df" | grep -c . || true)" + _nunacc="$(printf '%s' "$_unacc" | grep -c . || true)" + _sum=$(( _nacc + _nnd + _ndebt + _nunacc )) + + # Debt that is no longer debt. Reported rather than fatal: a burn-down that + # reddens the gate the moment someone FIXES something teaches people not to. + _stale_acct="$(LC_ALL=C comm -12 "$_df" "$_af")" + _stale_reg="$(LC_ALL=C comm -23 "$_df" "$_rf")" + rm -f "$_rf" "$_af" "$_ndf" "$_df" "$_t1" "$_t2" + + if [ "$_nunacc" != 0 ]; then + while IFS= read -r _n; do + [ -n "$_n" ] && echo " registered but accounted by nothing: $_n" + done <<<"$_unacc" + _rc=1 + fi + while IFS= read -r _n; do + [ -n "$_n" ] && echo " listed as debt but now accounts: $_n" + done <<<"$_stale_acct" + while IFS= read -r _n; do + [ -n "$_n" ] && echo " listed as debt but not registered: $_n" + done <<<"$_stale_reg" + + echo " population reconciliation: registered=$_nreg | accounted=$_nacc, not dispatched=$_nnd, known debt=$_ndebt, unaccounted=$_nunacc | sum=$_sum" + if [ "$_nreg" != "$_sum" ]; then + echo " the population does not add up: $_nreg registered, $_sum in the buckets" + _rc=1 + fi + return $_rc +} + pgc_reconcile_accounting() { # pgc_reconcile_accounting DECLARED OBSERVED [NOTDISPATCHED] -> 0 ok, 1 asymmetric # Set equality in both directions. The two directions catch opposite # mistakes and neither can stand in for the other: @@ -1140,6 +1233,24 @@ pgc_tally_suite() { # pgc_tally_suite NAME VERDICT LOGFILE verfail=1 fi + # THE POPULATION, which the symmetry check above cannot see (#916, reported by + # @linuxhikerpm). Its inputs are both derived from the suites, so a registered + # suite in neither is outside the universe it reconciles. Here the registered + # set IS the input, and a name accounted by nothing fails by name. + _acc_registered="$builddir/accounting.registered" + _acc_accounted="$builddir/accounting.accounted" + printf '%s\n' "${SUITES[@]}" >"$_acc_registered" + : >"$_acc_accounted" + for s in "${SUITES[@]}"; do + [ "$(pgc_log_shows_any_accounting "$builddir/${s}.log")" = yes ] \ + && printf '%s\n' "$s" >>"$_acc_accounted" + done + if ! pgc_reconcile_population "$_acc_registered" "$_acc_accounted" \ + "$_acc_notdisp" "$builddir/test/suites_without_accounting.txt"; then + echo " PG$major has a registered suite nothing accounts for, which is not a pass" + verfail=1 + fi + # How many of the suites counted as having RUN actually accounted for their # checks (#916). Ten registered suites exit 0 having never called pgc_summary; # counting them among the suites that ran is the overcount #447 added this diff --git a/test/selftest/390-a-registered-suite-must-account.sh b/test/selftest/390-a-registered-suite-must-account.sh index f9927805..e2537ef5 100644 --- a/test/selftest/390-a-registered-suite-must-account.sh +++ b/test/selftest/390-a-registered-suite-must-account.sh @@ -498,3 +498,125 @@ check "the grep -q shape is the one that gets this wrong under pipefail" \ check "and it agrees with the real reader on a SHORT file, which is why it survived review" \ "$(bash "$_grepq_twin_sh" "$_acc/declares.sh")" \ "$(pgc_suite_declares_accounting "$_acc/declares.sh")" + +# ---- the POPULATION, which the symmetry check above cannot see -------------- +# +# pgc_reconcile_accounting reconciles the DECLARED set against the OBSERVED one. +# Both are derived from the suites themselves, and the two directions catch +# opposite mistakes -- but the registered set is not one of its inputs, so a +# registered suite in NEITHER set is outside the universe being reconciled. +# Driven from the function: with all three files empty it prints +# `inputs=0 | both=0 ... sum=0` and returns 0, whatever SUITES holds. +# +# Reported by @linuxhikerpm, who put it structurally: treating absence of a +# declaration as absence from the population preserves the overcount. The title +# of this change claims to reconcile registered suites against accounted ones, +# and that claim needs the registered set as an input. +# +# So the population is its own check, over its own four buckets. A suite is +# ACCOUNTED when its log carries evidence it counted its checks -- either +# lib.sh's accounting line, or its own `checks run:` line, which is what +# bench_guards and docs_style print from private counters. Runtime-observable in +# both cases, and derived rather than declared, so a suite that adopts either +# mechanism leaves the debt bucket on its own. + +check "premise: the runner defines the population reconciliation" \ + "$(grep -c '^pgc_reconcile_population()' "$_rv")" "1" +check "premise: and the accounted reader that feeds it" \ + "$(grep -c '^pgc_log_shows_any_accounting()' "$_rv")" "1" + +eval "$(sed -n '/^pgc_log_shows_any_accounting()/,/^}/p' "$_rv")" +eval "$(sed -n '/^pgc_reconcile_population()/,/^}/p' "$_rv")" +check "premise: the population reconciliation is callable" \ + "$(type -t pgc_reconcile_population)" "function" + +# ---- the accounted reader takes EITHER mechanism ---------------------------- + +printf 'accounting: 1 passed + 0 failed + 0 unrunnable = 1\nx.sh: PASSED\n' > "$_acc/lib.log" +check "a log carrying lib.sh's accounting line is accounted" \ + "$(pgc_log_shows_any_accounting "$_acc/lib.log")" "yes" + +printf 'checks run: 9\ndocs_style.sh: PASSED\n' > "$_acc/self.log" +check "and a log carrying only its OWN checks-run line is accounted too" \ + "$(pgc_log_shows_any_accounting "$_acc/self.log")" "yes" + +printf 'some output\nPASSED\n' > "$_acc/none.log" +check "a log carrying neither is not accounted" \ + "$(pgc_log_shows_any_accounting "$_acc/none.log")" "no" + +# ---- the red arm @linuxhikerpm asked for, exactly as asked ------------------ + +_reg_f="$_acc/registered"; _acct_f="$_acc/accounted"; _debt_f="$_acc/debt" +printf 'alpha\n' > "$_reg_f"; : > "$_acct_f"; : > "$_notdisp"; : > "$_debt_f" +check "a registered suite that is accounted by nothing FAILS" \ + "$(pgc_reconcile_population "$_reg_f" "$_acct_f" "$_notdisp" "$_debt_f" >/dev/null 2>&1 \ + && echo ok || echo unaccounted)" "unaccounted" +check "and it is named, which the symmetry check could never do" \ + "$(pgc_reconcile_population "$_reg_f" "$_acct_f" "$_notdisp" "$_debt_f" 2>&1 \ + | grep -c '^[[:space:]]*registered but accounted by nothing: alpha$')" "1" + +# Each of the three ways out must actually let it out, or the bucket is a name +# for "always fails" and the debt file is the only thing doing any work. +printf 'alpha\n' > "$_acct_f"; : > "$_notdisp"; : > "$_debt_f" +check "a suite that accounted passes" \ + "$(pgc_reconcile_population "$_reg_f" "$_acct_f" "$_notdisp" "$_debt_f" >/dev/null 2>&1 \ + && echo ok || echo unaccounted)" "ok" +: > "$_acct_f"; printf 'alpha\n' > "$_notdisp" +check "a suite the driver never dispatched passes" \ + "$(pgc_reconcile_population "$_reg_f" "$_acct_f" "$_notdisp" "$_debt_f" >/dev/null 2>&1 \ + && echo ok || echo unaccounted)" "ok" +: > "$_notdisp"; printf 'alpha\n' > "$_debt_f" +check "a suite recorded as known debt passes" \ + "$(pgc_reconcile_population "$_reg_f" "$_acct_f" "$_notdisp" "$_debt_f" >/dev/null 2>&1 \ + && echo ok || echo unaccounted)" "ok" + +# The debt file excuses ONLY what it names. A new unaccounted suite must fail +# even while the known ten are excused -- that is the whole point of recording +# them by name rather than as a count. +printf 'alpha\nbeta\n' > "$_reg_f"; : > "$_acct_f"; printf 'alpha\n' > "$_debt_f" +check "a NEW unaccounted suite fails even while the known debt is excused" \ + "$(pgc_reconcile_population "$_reg_f" "$_acct_f" "$_notdisp" "$_debt_f" 2>&1 \ + | grep -c '^[[:space:]]*registered but accounted by nothing: beta$')" "1" +check "and the excused one is not named as a failure" \ + "$(pgc_reconcile_population "$_reg_f" "$_acct_f" "$_notdisp" "$_debt_f" 2>&1 \ + | grep -c '^[[:space:]]*registered but accounted by nothing: alpha$')" "0" + +# Debt that no longer exists is debt that should have been removed. A name in the +# file that is not registered, or that now accounts, means the file is stale -- +# and a stale debt file is how a burn-down stops burning down. +printf 'alpha\n' > "$_reg_f"; printf 'alpha\n' > "$_acct_f"; printf 'alpha\n' > "$_debt_f" +check "a suite that now accounts but is still listed as debt is reported" \ + "$(pgc_reconcile_population "$_reg_f" "$_acct_f" "$_notdisp" "$_debt_f" 2>&1 \ + | grep -c '^[[:space:]]*listed as debt but now accounts: alpha$')" "1" + +printf 'alpha\n' > "$_reg_f"; printf 'alpha\n' > "$_acct_f"; printf 'gone\n' > "$_debt_f" +check "and debt naming a suite that is not registered is reported too" \ + "$(pgc_reconcile_population "$_reg_f" "$_acct_f" "$_notdisp" "$_debt_f" 2>&1 \ + | grep -c '^[[:space:]]*listed as debt but not registered: gone$')" "1" + +# inputs == sum(buckets) over the REGISTERED population, which is the identity +# the symmetry check could not offer: a name can genuinely fall outside all four. +printf 'a\nb\nc\nd\n' > "$_reg_f" +printf 'a\n' > "$_acct_f"; printf 'b\n' > "$_notdisp"; printf 'c\n' > "$_debt_f" +check "the population partitions, and prints inputs == sum(buckets)" \ + "$(pgc_reconcile_population "$_reg_f" "$_acct_f" "$_notdisp" "$_debt_f" 2>&1 \ + | grep -c 'registered=4 .*accounted=1, not dispatched=1, known debt=1, unaccounted=1 | sum=4')" "1" + +# ---- and the RUNNER must call it, with the real registered set -------------- + +check "the runner calls the population reconciliation" \ + "$(grep -c '[^_[:alnum:]]pgc_reconcile_population "' "$_rv")" "1" +# The property, not a count of a substring: the registered file must be written +# from the SUITES array itself. The first version of this arm asserted the name +# appeared twice, which is a fact about how many times a variable is spelled -- +# it fails when the code is refactored and passes when the file is filled from +# the wrong source. +check "and the registered file is written from the SUITES array itself" \ + "$(grep -cF 'printf '"'"'%s\n'"'"' "${SUITES[@]}" >"$_acc_registered"' "$_rv")" "1" +check "and a failed population reconciliation fails the major" \ + "$(grep -A4 'pgc_reconcile_population "\$_acc_registered"' "$_rv" | grep -c 'verfail=1')" "1" + +# The debt file is tracked, so a change to it is a diff a reviewer sees -- which +# is the whole reason it is a file and not a number in the environment. +check "the debt file is in the tree" \ + "$([ -f "$PGC_TESTDIR/suites_without_accounting.txt" ] && echo yes || echo no)" "yes" diff --git a/test/suites_without_accounting.txt b/test/suites_without_accounting.txt new file mode 100644 index 00000000..674a67fd --- /dev/null +++ b/test/suites_without_accounting.txt @@ -0,0 +1,27 @@ +# Registered suites that count no checks at runtime -- DEBT, not an exemption. +# +# A suite here exits 0 without any observable accounting: it neither calls +# lib.sh's pgc_summary (which prints the "accounting:" line) nor prints its own +# "checks run:" line the way bench_guards and docs_style do from private +# counters. The matrix therefore reports it as a suite that "ran" while having no +# idea whether it asserted anything. +# +# THIS FILE MAY ONLY SHRINK. A name added here is a suite whose checks the harness +# cannot see, and adding one is a diff a reviewer sees -- which is the whole +# reason it is a tracked file and not a number in the environment. A name that +# starts accounting, or stops being registered, is reported by the matrix so the +# burn-down cannot stall silently. +# +# Generated from a measurement over test/run_all_versions.sh --list-suites, not +# typed. To regenerate, run the matrix and take the "registered but accounted by +# nothing" lines. +audit +concurrency +phase2 +phase3 +phase4 +phase5 +phase6 +smoke +unique_conc +update_conc From 9dfb63ef0067924144fade55963efc92316c21dd Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Thu, 10 Sep 2026 07:06:22 -0600 Subject: [PATCH 8/8] test: registered == sum(buckets) cannot fail on the data either (#916) Reported by OffgridwithJD, and it is the same class I re-worded two functions away in this very PR: the four buckets are built by successive subtraction FROM the registered set, so their sum equals it identically. Measured, 400 random four-set inputs: the identity fired 0 times while the real bucket findings fired on 353 of them. The line stays -- the house rule asks for inputs == sum(buckets) printed beside any list-derived claim -- but the comment now says what it can catch, because the next reader will otherwise go looking for a data case that does not exist. What it guards is comm being fed unsorted input, which produces buckets that are not a partition at all. TESTS.md and the pytest docstring said the opposite -- that this identity CAN be false on the data, unlike the symmetry check's. That was my claim and it was wrong; both are corrected, and both now point at the arms that carry the weight: the ones on the unaccounted bucket. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK --- test/pytest/TESTS.md | 9 ++++++--- test/pytest/test_suite_accounting.py | 11 ++++++++--- test/run_all_versions.sh | 11 +++++++++++ test/selftest/390-a-registered-suite-must-account.sh | 7 +++++-- 4 files changed, 30 insertions(+), 8 deletions(-) diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 082fd3cd..2f591ddd 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -1126,9 +1126,12 @@ reddens the moment someone *fixes* something teaches people not to fix things. ### `test_the_population_partitions_and_prints_its_identity` -`inputs == sum(buckets)` over the registered population. Unlike the symmetry check's -identity, this one can be false on the data: a name can genuinely fall outside all four -buckets. +`inputs == sum(buckets)` over the registered population, printed per the house rule. +Like the symmetry check's identity it **cannot** be false on the data — the four buckets +are built by successive subtraction from the registered set, so their sum equals it +identically, measured at 0 firings over 400 random four-set inputs while the real bucket +findings fired on 353. What it guards is `comm` reading unsorted input, which produces +buckets that are not a partition at all. ### `test_the_debt_file_is_tracked_and_holds_only_registered_suites` diff --git a/test/pytest/test_suite_accounting.py b/test/pytest/test_suite_accounting.py index 9d52b53a..764e5d63 100644 --- a/test/pytest/test_suite_accounting.py +++ b/test/pytest/test_suite_accounting.py @@ -436,9 +436,14 @@ def test_the_debt_file_excuses_only_what_it_names(tmp_path, expect): def test_the_population_partitions_and_prints_its_identity(tmp_path, expect): - """inputs == sum(buckets) over the REGISTERED population -- the identity the - symmetry check could not offer, because there a name can genuinely fall - outside all four buckets.""" + """inputs == sum(buckets) over the REGISTERED population, printed per the house rule. + + It cannot be false on the data: the buckets are built by successive subtraction + from the registered set, so their sum equals it identically -- 0 firings over 400 + random four-set inputs while the real bucket findings fired on 353. What it guards + is `comm` reading unsorted input. The arm below it, on the unaccounted bucket, is + the one that carries weight here. + """ reg = _write(tmp_path, "reg", "a\nb\nc\nd\n") acct = _write(tmp_path, "acct", "a\n") nd = _write(tmp_path, "nd", "b\n") diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index 0b7d685a..cb3a09ce 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -1025,6 +1025,17 @@ pgc_reconcile_population() { # pgc_reconcile_population REGISTERED ACCOUNTED NOT [ -n "$_n" ] && echo " listed as debt but not registered: $_n" done <<<"$_stale_reg" + # registered == sum(buckets), printed beside every reconciliation per the + # house rule. BE PRECISE ABOUT WHAT IT CAN CATCH, because the next reader will + # go looking for a data case and there is not one: the four buckets are built + # by successive subtraction FROM the registered set, so their sum equals it + # identically. OffgridwithJD measured it -- 400 random four-set inputs, zero + # firings, while the real bucket findings fired on 353 of them. + # + # What can make it false is comm being fed unsorted input, which produces + # buckets that are not a partition at all. It is a comm tripwire, exactly like + # the one on pgc_reconcile_accounting, and selftest 390 drives it there with + # that mutation. echo " population reconciliation: registered=$_nreg | accounted=$_nacc, not dispatched=$_nnd, known debt=$_ndebt, unaccounted=$_nunacc | sum=$_sum" if [ "$_nreg" != "$_sum" ]; then echo " the population does not add up: $_nreg registered, $_sum in the buckets" diff --git a/test/selftest/390-a-registered-suite-must-account.sh b/test/selftest/390-a-registered-suite-must-account.sh index e2537ef5..42321d4a 100644 --- a/test/selftest/390-a-registered-suite-must-account.sh +++ b/test/selftest/390-a-registered-suite-must-account.sh @@ -594,8 +594,11 @@ check "and debt naming a suite that is not registered is reported too" \ "$(pgc_reconcile_population "$_reg_f" "$_acct_f" "$_notdisp" "$_debt_f" 2>&1 \ | grep -c '^[[:space:]]*listed as debt but not registered: gone$')" "1" -# inputs == sum(buckets) over the REGISTERED population, which is the identity -# the symmetry check could not offer: a name can genuinely fall outside all four. +# inputs == sum(buckets) over the REGISTERED population, printed per the house +# rule. Like the symmetry check's, it cannot be false on the DATA -- the buckets +# are built by successive subtraction from the registered set, so their sum equals +# it identically. What it guards is comm reading unsorted input. The arms above, +# on the unaccounted bucket, are the ones that carry weight. printf 'a\nb\nc\nd\n' > "$_reg_f" printf 'a\n' > "$_acct_f"; printf 'b\n' > "$_notdisp"; printf 'c\n' > "$_debt_f" check "the population partitions, and prints inputs == sum(buckets)" \