From d2a87080c1b9012cd16d0473a2272e1098941827 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Fri, 11 Sep 2026 09:55:28 -0600 Subject: [PATCH 1/3] test/pytest: the session reports its totals, and the record stream is reconciled (#937 phase 3) A run now ends with `checks run: N` and an accounting line counted from the records, so the harness states what it did rather than leaving it to be inferred from pytest's test count. Five assertions across two tests is five, and an arm uses four claims in one test and one in another because a per-test count agrees with the record count whenever every test makes exactly one claim -- which is what a hand-written fixture reaches for first. THE OBVIOUS RECONCILIATION HERE IS VACUOUS BY CONSTRUCTION, and phase 1 made it so on purpose. `count` IS `len(self._records)`, so checking one against the other compares a value with its own definition. Partitioning the records into verdict buckets and asserting the parts sum to the whole is the same trap in a hat. #937 records that the shell side shipped `inputs == sum(buckets)` twice and that both were caught only by mutating them. So the two quantities arrive by different routes: held len(recorder.records), read in the process that RAN the test arrived the list read off the report AFTER it was built, crossing the report boundary and, under -n, a process boundary _UnrunnableCollector is why the second route has to exist: a worker's state is invisible to the controller. Verified under xdist -- 560 records collected on the controller from two workers, no offences, identical to the serial run. WHAT IT CATCHES: a record created after the report was built, one dropped or mangled in transport, a verdict outside the closed set. WHAT IT DOES NOT: a record present, transported, well-formed and wrong. That is phase 2's job, and it is said in the code so this does not read as a guarantee it is not. Both refusals are proven by injecting the failure from a conftest, because nothing in the tree drops a record and an arm waiting for a real defect is not evidence the check can fail. AND THE INJECTOR'S HOOK ORDERING IS LOAD-BEARING. My first version used trylast, making it the INNERMOST wrapper, so its post-yield ran before the layer attached anything: it saw empty user_properties, the inner run passed, and the arm read exactly like a reconciliation that does not fire. tryfirst fixes it and the reason is in the docstring. Removal proofs, each reddening only its own arms: held is never compared to arrived 1 red the verdict set is never checked 1 red the refusal never moves off zero 2 red the total counts TESTS not records 2 red Verified: pytest 247 driver-free and 345 full corpus against a live PG16 cluster, reconciling 846 records; harness_selftest 803/803 on PG16; docs_style 9/9. Also green under `-n 2`. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EbyGSaU93XYQr8aH4NrUiw --- CHANGELOG.md | 22 +++++ test/pytest/TESTS.md | 41 ++++++++- test/pytest/pgc_vacuity.py | 144 ++++++++++++++++++++++++++++-- test/pytest/test_check_records.py | 140 +++++++++++++++++++++++++++++ 4 files changed, 339 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d78160d6..8c1208e8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,28 @@ true until the next version shipped. ### Added +- The pytest harness reports its own check totals, and the record stream is + reconciled against what arrived (#937, third phase). + + A run now ends with `checks run: N` and an `accounting:` line counted from the + records, so the harness states what it did rather than leaving it to be + inferred from pytest's test count. Five assertions across two tests is five. + + The reconciliation compares two quantities that arrive by different routes: + what the recorder held, read in the process that ran the test, and what + arrived, read back off the report after it was built. Under `-n` the second + route crosses a process boundary. + + Reconciling the count against the records would have been vacuous, because + the second phase made the count `len(records)` on purpose. Partitioning the + records into verdict buckets and summing them is the same trap. Both compare + a value with its own definition. + + A record that does not arrive, or that carries a verdict outside the closed + set, refuses the run and names the test. Both refusals are proven by injecting + the failure from a conftest, because no code in the tree drops a record and an + arm that waits for a real defect is not evidence the check can fail. + - Every counted assertion in the pytest harness produces a record, and the count is derived from them (#937, first phase). diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 87b46565..66767dbd 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -2470,4 +2470,43 @@ message this layer never composes — there is no verdict for the call site to p `AssertionError` subclass: every `VacuityError` is raised *before* its record is taken, so no record exists to mark. That is scanned rather than trusted. -Still to come in #937: a session reconciliation that can fail. +### Phase 3: the session reconciles, and the check can fail + +| test | what it pins | +|---|---| +| `test_the_session_totals_are_reconciled` | the positive control: a clean run reports and does not refuse | +| `test_the_total_is_printed_from_the_records_not_from_the_test_count` | 5 claims across 2 tests is 5, not 2 | +| `test_a_record_lost_in_transport_is_refused` | **the arm this phase exists for** | +| `test_a_verdict_outside_the_closed_set_is_refused` | the schema half | + +**The obvious reconciliation here is vacuous by construction, and phase 1 made it +so on purpose.** `count` *is* `len(self._records)`, so checking one against the +other compares a value with its own definition. Partitioning the records into +PASS/FAIL/UNRUN and asserting the parts sum to the whole is the same trap wearing +a hat — the buckets are derived from the list being counted. #937 records that the +shell side shipped `inputs == sum(buckets)` **twice** and that both were caught +only by mutating them. + +So the two quantities come by different routes: + +``` +held len(recorder.records), read in the process that RAN the test +arrived the list read back off the report AFTER it was built -- crossing the + report boundary, and under -n a process boundary as well +``` + +`_UnrunnableCollector` is why the second route has to exist at all: a worker's +state is invisible to the controller, so the value travels on the report. +Measured on the pinned runner, `user_properties` survive that crossing intact. + +**What it catches:** a record created after the report was built, one dropped or +mangled in transport, and a verdict outside the closed set. **What it does not:** a +record that is present, transported, well-formed and wrong. That is phase 2's job, +and it is said here so this does not read as a guarantee it is not. + +**The arms inject the failure from a conftest**, because no in-tree code drops a +record — an arm that waits for a real defect to appear is not evidence the check +can fail. `tryfirst=True` on those hooks is load-bearing: a wrapper's post-`yield` +code runs in the reverse of call order, so `trylast` made the injector run *before* +the layer attached anything, the inner run passed, and the arm read exactly like a +reconciliation that does not fire. diff --git a/test/pytest/pgc_vacuity.py b/test/pytest/pgc_vacuity.py index f5613ceb..df6c5082 100644 --- a/test/pytest/pgc_vacuity.py +++ b/test/pytest/pgc_vacuity.py @@ -22,6 +22,7 @@ import ast import numbers import pathlib +import sys import functools import itertools @@ -222,6 +223,15 @@ def note_write(nodeid, cur): # nothing. EXIT_INCOMPLETE = 67 +# THE CLOSED SET A RECORD'S VERDICT MUST COME FROM. #937 phase 3. +# +# lib.sh carries the same four values and `pgc_record` refuses an unknown one +# rather than dropping the check -- dropping it would leave the count bumped with +# no outcome recorded, which is the reconciliation failure itself. SKIP has no +# counterpart here: a bare skip is refused at collection, and the honest form is +# expect.cannot_run(), which records UNRUN. +RECORD_VERDICTS = ("PASS", "FAIL", "UNRUN") + class VacuityError(AssertionError): """Raised when an assertion could not have failed, or asserted nothing.""" @@ -1137,6 +1147,67 @@ def pytest_runtest_logreport(self, report): +class _RecordCollector: + """Reconciles what each test's recorder HELD against what ARRIVED. #937 phase 3. + + THE OBVIOUS RECONCILIATION IS VACUOUS HERE, BY CONSTRUCTION, and phase 1 made + it so deliberately. `count` IS `len(self._records)`, so checking one against + the other compares a value with its own definition. Partitioning the records + into PASS/FAIL/UNRUN and asserting the parts sum to the whole is the same trap + in a hat -- the buckets are derived from the list being counted. #937 records + that the shell side shipped `inputs == sum(buckets)` twice and that both were + caught only by mutating them; a third would be worse for having been warned. + + So the two quantities come by different routes: + + held len(recorder.records), read in the process that RAN the test + arrived the list read back off the report AFTER it was built, crossing + the report boundary and, under -n, a process boundary too + + `_UnrunnableCollector` above is why the second route has to exist at all: a + worker's state is invisible to the controller, so the value travels on the + report. Measured on the pinned runner, user_properties survive that crossing + intact -- which is what makes this a reconciliation rather than a formality. + + WHAT IT CATCHES: a record created after the report was built, one dropped or + mangled in transport, and a verdict outside the closed set. WHAT IT DOES NOT: + a record that is present, transported and well-formed, and wrong. That is + phase 2's job, and saying so here keeps this from reading as a guarantee it + is not. + """ + + def __init__(self): + self.records = [] # (nodeid, verdict, name) + self.offences = [] + + def pytest_runtest_logreport(self, report): + if report.when != "call": + return + held = None + arrived = None + for key, value in getattr(report, "user_properties", ()): + if key == "pgc_records_held": + held = value + elif key == "pgc_records": + arrived = list(value) + if held is None and arrived is None: + return + if arrived is None: + arrived = [] + if held != len(arrived): + self.offences.append( + f"{report.nodeid}: the recorder held {held} record(s) and " + f"{len(arrived)} arrived" + ) + for verdict, name in arrived: + if verdict not in RECORD_VERDICTS: + self.offences.append( + f"{report.nodeid}: record {name!r} carries the verdict " + f"{verdict!r}, which is not one of {RECORD_VERDICTS}" + ) + self.records.append((report.nodeid, verdict, name)) + + @pytest.hookimpl(wrapper=True) def pytest_runtest_makereport(item, call): """Carry an unrunnable declaration out on the report itself. @@ -1150,32 +1221,88 @@ def pytest_runtest_makereport(item, call): if rec is not None and rec.unrunnable: reason, detail = rec.unrunnable report.user_properties.append(("pgc_unrunnable", f"{reason}\n{detail}")) + # BOTH ROUTES, and they are attached separately on purpose (#937 phase 3). + # `pgc_records_held` is a number read from the recorder HERE; `pgc_records` + # is the stream itself. Deriving the count from the stream on the far side + # would compare the stream with itself, which is the vacuous shape this + # phase exists to avoid. + # + # PLAIN TUPLES, not _Record objects: user_properties are serialised across + # the xdist boundary, and an object that failed to serialise would break + # the transport this check exists to watch. + if rec is not None: + report.user_properties.append(("pgc_records_held", rec.count)) + report.user_properties.append( + ("pgc_records", [(r.verdict, r.name) for r in rec.records])) return report def pytest_terminal_summary(terminalreporter): - """Print the third state, in lib.sh's shape. + """Print the third state in lib.sh's shape, then the run's own totals. `UNRUN : : `, then the count. A state that does not say why is a skip with better manners, and a state with no count cannot be reconciled against the total. + + THE TOTAL IS COUNTED FROM THE RECORDS, NOT FROM THE TESTS. Those agree + whenever every test makes exactly one claim, which is what a hand-written + fixture reaches for first -- so an arm in test_check_records.py uses four + claims in one test and one in another, where a per-test count would say 2 and + the records say 5. """ collector = getattr(terminalreporter.config, "pgc_unrunnable", None) - if collector is None or not collector.items: + if collector is not None and collector.items: + terminalreporter.write_line("") + for nodeid, reason, detail in collector.items: + terminalreporter.write_line(f"UNRUN {nodeid}: {reason}: {detail}") + terminalreporter.write_line(f"checks unrunnable: {len(collector.items)}") + + records = getattr(terminalreporter.config, "pgc_records", None) + if records is None or not records.records: return - terminalreporter.write_line("") - for nodeid, reason, detail in collector.items: - terminalreporter.write_line(f"UNRUN {nodeid}: {reason}: {detail}") - terminalreporter.write_line(f"checks unrunnable: {len(collector.items)}") + tally = {v: 0 for v in RECORD_VERDICTS} + for _nodeid, verdict, _name in records.records: + if verdict in tally: + tally[verdict] += 1 + terminalreporter.write_line(f"checks run: {len(records.records)}") + terminalreporter.write_line( + "accounting: " + + " + ".join(f"{tally[v]} {v.lower()}" for v in RECORD_VERDICTS) + + f" = {sum(tally.values())}" + ) def pytest_sessionfinish(session, exitstatus): - """An unrunnable test must not leave the run green. + """An unrunnable test must not leave the run green, and neither must a + record stream that does not reconcile. FAILURE STILL DOMINATES, exactly as in lib.sh: a run with both a failure and an unrunnable test is a failure, because the failure is the more urgent fact. So this only ever moves a run OFF zero, and never off a non-zero status. """ + # THE RECONCILIATION, FIRST, because it is a statement about whether the run + # can be believed at all rather than about one test (#937 phase 3). + # + # WRITTEN TO STDERR AND FORCED OFF ZERO rather than raised. A UsageError here + # is not reported cleanly -- the session is already finishing -- and this must + # not depend on an exception surviving a hook that other plugins also wrap. + records = getattr(session.config, "pgc_records", None) + if records is not None and records.offences: + sys.stderr.write( + "the pgColumnar vacuity layer refuses this run: the record stream " + "does not reconcile, so the totals above describe something other " + "than what the assertions did:\n" + ) + for offence in records.offences: + sys.stderr.write(f" {offence}\n") + sys.stderr.write( + " -- a record created after the report was built, or dropped in " + "transport, is invisible to every other check in this layer.\n" + ) + sys.stderr.flush() + if session.exitstatus == 0: + session.exitstatus = EXIT_INCOMPLETE + collector = getattr(session.config, "pgc_unrunnable", None) if collector is None or not collector.items: return @@ -1285,6 +1412,9 @@ def pytest_configure(config): config.pluginmanager.register(collector, "pgc_unrunnable_collector") config.pgc_unrunnable = collector config.pluginmanager.register(_RunShape(), f"pgc_runshape_{id(config)}") + records = _RecordCollector() + config.pluginmanager.register(records, f"pgc_records_{id(config)}") + config.pgc_records = records def pytest_addoption(parser): diff --git a/test/pytest/test_check_records.py b/test/pytest/test_check_records.py index 770e2527..63d2585d 100644 --- a/test/pytest/test_check_records.py +++ b/test/pytest/test_check_records.py @@ -410,3 +410,143 @@ def test_a_recording_method_takes_exactly_one_record_per_call(expect): deltas.append(e.count - before) expect.rows([str(d) for d in deltas], ["1"] * len(calls), "every call, including the delegating one, took exactly one record") + + +# ---- phase 3: the session's records reconcile, and the check can fail -------- +# +# THE OBVIOUS RECONCILIATION HERE IS VACUOUS BY CONSTRUCTION, and phase 1 is what +# made it so. `count` IS `len(self._records)`, so reconciling the count against +# the records compares a value with its own definition. #937 warns twice that the +# shell side shipped `inputs == sum(buckets)` that could not go red, and both were +# caught only by mutating them -- shipping a third would be worse for having been +# warned. +# +# Partitioning the records into PASS/FAIL/UNRUN and checking the parts sum to the +# whole is the same trap wearing a different hat: the buckets are derived from the +# list being counted. +# +# So the reconciliation is between two routes that are genuinely different: +# +# 1. what the recorder HELD, read in the process that ran the test +# 2. what ARRIVED, read back off the report after it was built -- crossing the +# report boundary, and under `-n` crossing a process boundary as well +# +# `_UnrunnableCollector` already records why that second route has to exist: a +# worker's own state is invisible to the controller, so the value has to travel on +# the report. Measured on the pinned runner, `user_properties` survive the xdist +# boundary intact, which is what makes route 2 available at all. + + +def test_the_session_totals_are_reconciled(pytester, expect): + """The positive control. A clean run reports its totals and does not refuse.""" + pytester.makepyfile( + """ + def test_two_claims(expect): + expect.num(1, 1, "first") + expect.num(2, 2, "second") + + def test_one_claim(expect): + expect.text("a", "a", "third") + """ + ) + result = pytester.runpytest("-p", "pgc_vacuity") + expect.outcomes(result, "a clean run passes", passed=2, failed=0) + result.stdout.fnmatch_lines(["*checks run: 3*"]) + + +def test_the_total_is_printed_from_the_records_not_from_the_test_count(pytester, expect): + """Three assertions across two tests is 3, not 2. A total that counted TESTS + would agree with the record count whenever every test made exactly one claim, + which is the case a hand-written fixture reaches for first.""" + pytester.makepyfile( + """ + def test_four_claims(expect): + expect.num(1, 1, "a") + expect.num(2, 2, "b") + expect.num(3, 3, "c") + expect.num(4, 4, "d") + + def test_one_claim(expect): + expect.num(5, 5, "e") + """ + ) + result = pytester.runpytest("-p", "pgc_vacuity") + result.stdout.fnmatch_lines(["*checks run: 5*"]) + expect.outcomes(result, "and the run itself is clean", passed=2, failed=0) + + +def test_a_record_lost_in_transport_is_refused(pytester, expect): + """THE ARM THIS PHASE EXISTS FOR, and it is written before the reconciliation. + + A conftest that drops one record on its way onto the report is exactly the + silent failure the two routes exist to catch: the recorder held three, two + arrived, and without a reconciliation the run reports 2 and nobody knows a + claim went missing. + + It has to be injected from a conftest because no in-tree code does this -- the + point of the arm is that the reconciliation CAN fail, and an arm that waits for + a real defect to appear is not evidence that it can. + + `tryfirst=True` IS LOAD-BEARING, NOT DECORATION. Both this hook and the + layer's are wrappers, and a wrapper's code after its `yield` runs in the + REVERSE of call order. My first version used `trylast`, which made this the + innermost wrapper, so it ran before the layer attached anything and saw an + empty `user_properties` -- the inner run then passed and the arm read exactly + like a reconciliation that does not fire. Measured: the debug print inside the + loop never executed. + """ + pytester.makepyfile( + """ + def test_three_claims(expect): + expect.num(1, 1, "a") + expect.num(2, 2, "b") + expect.num(3, 3, "c") + """ + ) + pytester.makeconftest( + """ + import pytest + + @pytest.hookimpl(wrapper=True, tryfirst=True) + def pytest_runtest_makereport(item, call): + report = yield + if call.when == "call": + for i, (key, value) in enumerate(report.user_properties): + if key == "pgc_records" and value: + report.user_properties[i] = (key, value[:-1]) + return report + """ + ) + result = pytester.runpytest("-p", "pgc_vacuity") + expect.run_failed(result, "a record that did not arrive is refused") + result.stderr.fnmatch_lines(["*held 3 record(s) and 2 arrived*"]) + + +def test_a_verdict_outside_the_closed_set_is_refused(pytester, expect): + """The schema half. A verdict the reader cannot key on is a record that says + nothing, and `pgc_record` refuses the same thing on the shell side rather than + dropping the check -- dropping it would leave the count bumped with no outcome, + which is the reconciliation failure itself.""" + pytester.makepyfile( + """ + def test_one_claim(expect): + expect.num(1, 1, "a") + """ + ) + pytester.makeconftest( + """ + import pytest + + @pytest.hookimpl(wrapper=True, tryfirst=True) + def pytest_runtest_makereport(item, call): + report = yield + if call.when == "call": + for i, (key, value) in enumerate(report.user_properties): + if key == "pgc_records" and value: + report.user_properties[i] = (key, [("SORTOF", n) for _, n in value]) + return report + """ + ) + result = pytester.runpytest("-p", "pgc_vacuity") + expect.run_failed(result, "a verdict outside the closed set is refused") + result.stderr.fnmatch_lines(["*SORTOF*"]) From 7b171053f80f7dbd07537d616aa7702a5380f84e Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Fri, 11 Sep 2026 14:17:14 -0600 Subject: [PATCH 2/3] test/pytest: correct what phase 3 claims to catch, and pin the limit (#937) The PR body, the CHANGELOG and the layer's own comment all said phase 3 catches "a record created after the report was built". It does not. Measured, by appending to the recorder from a hook outside this layer's: recorder now holds 4; report carries 3 checks run: 3 accounting: 3 pass + 0 fail + 0 unrun = 3 1 passed, rc=0 Both quantities are taken from ONE read of the recorder at ONE instant, so a later append is invisible to both and the run passes. It is not straightforwardly fixable either, which is the honest reason it is a limit rather than a TODO: the totals are BUILT from what arrived, and under -n the controller has no recorder to consult -- the worker's is in another process. So phase 3 is a TRANSPORT check, not a completeness check. Calling it the latter would be the third vacuous reconciliation #937 warns about, wearing the clothes of the two it already names. Corrected in all three places that carried the claim, and pinned by test_a_record_created_after_the_report_is_NOT_caught so it cannot be claimed away by the next person who reads the reconciliation and assumes what I assumed. Fourth claim of mine in this chain that no arm defended, and the first I found by attacking my own PR description before anyone reviewed it. Verified: 248 driver-free, docs_style 9/9. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EbyGSaU93XYQr8aH4NrUiw --- CHANGELOG.md | 6 ++++ test/pytest/TESTS.md | 18 +++++++++--- test/pytest/pgc_vacuity.py | 32 +++++++++++++++++---- test/pytest/test_check_records.py | 47 +++++++++++++++++++++++++++++++ 4 files changed, 94 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c1208e8..1dc670cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,12 @@ true until the next version shipped. the failure from a conftest, because no code in the tree drops a record and an arm that waits for a real defect is not evidence the check can fail. + It is a transport check and not a completeness check. A record created after + the report was built is invisible to it, because both quantities come from one + read of the recorder at one instant. That is a limit rather than an oversight: + the totals are built from what arrived, and under `-n` the controller has no + recorder to consult. An arm asserts the limit so it cannot be claimed away. + - Every counted assertion in the pytest harness produces a record, and the count is derived from them (#937, first phase). diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 66767dbd..b461a63d 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -2478,6 +2478,7 @@ taken, so no record exists to mark. That is scanned rather than trusted. | `test_the_total_is_printed_from_the_records_not_from_the_test_count` | 5 claims across 2 tests is 5, not 2 | | `test_a_record_lost_in_transport_is_refused` | **the arm this phase exists for** | | `test_a_verdict_outside_the_closed_set_is_refused` | the schema half | +| `test_a_record_created_after_the_report_is_NOT_caught` | **the limit**, pinned because I claimed the opposite | **The obvious reconciliation here is vacuous by construction, and phase 1 made it so on purpose.** `count` *is* `len(self._records)`, so checking one against the @@ -2499,10 +2500,19 @@ arrived the list read back off the report AFTER it was built -- crossing the state is invisible to the controller, so the value travels on the report. Measured on the pinned runner, `user_properties` survive that crossing intact. -**What it catches:** a record created after the report was built, one dropped or -mangled in transport, and a verdict outside the closed set. **What it does not:** a -record that is present, transported, well-formed and wrong. That is phase 2's job, -and it is said here so this does not read as a guarantee it is not. +**What it catches:** a record dropped or mangled between the report being built and +the report being read, and a verdict outside the closed set. + +**What it does not**, and the first version of this section claimed the first of +these wrongly: a record created **after** the report was built, because both +quantities come from one read of the recorder at one instant — measured, the +recorder held 4, the report carried 3, and the run passed; and a record that is +present, transported, well-formed and **wrong**, which is phase 2's job. + +So it is a transport check rather than a completeness check. The limit is not +straightforwardly fixable — the totals are built from what arrived, and under `-n` +the controller has no recorder to consult — so it is pinned by an arm instead of +described by a sentence. **The arms inject the failure from a conftest**, because no in-tree code drops a record — an arm that waits for a real defect to appear is not evidence the check diff --git a/test/pytest/pgc_vacuity.py b/test/pytest/pgc_vacuity.py index df6c5082..01e15b0b 100644 --- a/test/pytest/pgc_vacuity.py +++ b/test/pytest/pgc_vacuity.py @@ -1169,11 +1169,33 @@ class _RecordCollector: report. Measured on the pinned runner, user_properties survive that crossing intact -- which is what makes this a reconciliation rather than a formality. - WHAT IT CATCHES: a record created after the report was built, one dropped or - mangled in transport, and a verdict outside the closed set. WHAT IT DOES NOT: - a record that is present, transported and well-formed, and wrong. That is - phase 2's job, and saying so here keeps this from reading as a guarantee it - is not. + WHAT IT CATCHES: a record dropped or mangled between the report being built + and the report being read, and a verdict outside the closed set. + + WHAT IT DOES NOT, and the first version of this comment claimed the first of + these, wrongly: + + * A RECORD CREATED AFTER THE REPORT WAS BUILT. Both values are taken from one + read of the recorder at one instant, so a later append is invisible to both + and the run passes. Measured, by appending from a hook outside this layer's: + + recorder now holds 4; report carries 3 + checks run: 3 + accounting: 3 pass + 0 fail + 0 unrun = 3 + 1 passed, rc=0 + + It is not straightforwardly fixable either, and that is the honest reason it + is a limit rather than a TODO: the totals are BUILT from what arrived, and + under -n the controller has no recorder to consult -- the worker's is in + another process. An arm in test_check_records.py pins this so it cannot be + re-claimed. + + * A record that is present, transported and well-formed, and WRONG. That is + phase 2's job. + + So this is a transport check, not a completeness check, and calling it the + latter would be the third vacuous reconciliation #937 warns about wearing the + clothes of the two it already names. """ def __init__(self): diff --git a/test/pytest/test_check_records.py b/test/pytest/test_check_records.py index 63d2585d..a0d20e52 100644 --- a/test/pytest/test_check_records.py +++ b/test/pytest/test_check_records.py @@ -550,3 +550,50 @@ def pytest_runtest_makereport(item, call): result = pytester.runpytest("-p", "pgc_vacuity") expect.run_failed(result, "a verdict outside the closed set is refused") result.stderr.fnmatch_lines(["*SORTOF*"]) + + +def test_a_record_created_after_the_report_is_NOT_caught(pytester, expect): + """THE LIMIT, pinned so it cannot be re-claimed. I claimed the opposite. + + The PR body, the CHANGELOG and the layer's own comment all said phase 3 + catches a record created after the report was built. It does not, and the + reason is structural rather than an oversight: both quantities are taken from + ONE read of the recorder at ONE instant, so a later append is invisible to + both. + + It is also not straightforwardly fixable. The totals are built from what + ARRIVED, and under `-n` the controller has no recorder to consult -- the + worker's lives in another process. So this is a transport check, and the + honest thing is an arm that says where the edge is rather than a sentence + claiming there is none. + + Found by attacking my own PR description before anyone reviewed it, which is + the fourth claim of mine this chain that no arm defended. + """ + pytester.makepyfile( + """ + def test_three_claims(expect): + expect.num(1, 1, "a") + expect.num(2, 2, "b") + expect.num(3, 3, "c") + """ + ) + pytester.makeconftest( + """ + import pytest + import pgc_vacuity + + @pytest.hookimpl(wrapper=True, tryfirst=True) + def pytest_runtest_makereport(item, call): + report = yield + if call.when == "call": + rec = pgc_vacuity._RECORDERS.get(item.nodeid) + if rec is not None: + rec._records.append(pgc_vacuity._Record("a record created LATE")) + return report + """ + ) + result = pytester.runpytest("-p", "pgc_vacuity") + expect.outcomes(result, "a late record does NOT refuse the run -- this is the limit", + passed=1, failed=0) + result.stdout.fnmatch_lines(["*checks run: 3*"]) From b81134c3af820ee95faf5df868235a3821ef3547 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Fri, 11 Sep 2026 14:38:32 -0600 Subject: [PATCH 3/3] test/pytest: three arms strengthened and the limit widened, from review (#937 phase 3) @OffgridwithJD attacked the three things I asked about. Two landed. THE TOTALS ARM WAS WEAK. The fixture was all-PASS, so records 5 AND passes 5: a totals line counted from PASSES would have been indistinguishable from one counted from records, and only the test count was separated. Making one claim false and catching it gives three numbers that disagree: records 5 passes 4 tests 2 checks run: 5 accounting: 4 pass + 1 fail + 0 unrun = 5 A dead end is recorded so it is not retried: cannot_run contributes an UNRUN record but fails its own test, so an unrunnable fixture does not separate them either. THE ALIAS OBJECTION, AND THE ATTACK THAT FAILS. The transport arm drops a record with value[:-1], which COPIES -- so the unfair-in-my-favour reading is that the report carries the recorder's own list and the slice is the only reason it fires. An in-place value.pop() reaches whatever the report actually holds: still refused. That shows separate storage IN A SINGLE PROCESS, which the xdist run cannot, because serialisation copies everything by definition. It is the stronger half of the evidence and it was not in the PR. THE PINNED LIMIT NAMED HALF THE GAP. Both values come from one read of the recorder at one instant, so BOTH directions are blind: appended AFTER the read invisible, run passes removed BEFORE the read invisible, run passes The EARLIER half is the more reachable one, and my framing had it backwards: a late append needs someone outside the layer, an early loss is what a bug inside the recorder looks like. Both are now pinned. AND THE REASON I GAVE WAS WRONG TWICE OVER. It is not that the controller lacks a recorder under -n; the expect fixture's teardown pops the recorder, so nothing after makereport can read it in a single process either. "Not straightforwardly fixable" also overstated it: keeping the final COUNT in a session-level map past teardown and reconciling at worker-side sessionfinish would close it. Unbuilt, so it is named as a proposal rather than planned. Verified: 249 driver-free, docs_style 9/9. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EbyGSaU93XYQr8aH4NrUiw --- test/pytest/TESTS.md | 38 +++++--- test/pytest/pgc_vacuity.py | 37 +++++--- test/pytest/test_check_records.py | 147 +++++++++++++++++++++++++----- 3 files changed, 172 insertions(+), 50 deletions(-) diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index b461a63d..8104a84e 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -2475,10 +2475,11 @@ taken, so no record exists to mark. That is scanned rather than trusted. | test | what it pins | |---|---| | `test_the_session_totals_are_reconciled` | the positive control: a clean run reports and does not refuse | -| `test_the_total_is_printed_from_the_records_not_from_the_test_count` | 5 claims across 2 tests is 5, not 2 | +| `test_the_total_separates_records_from_passes_and_from_tests` | records 5, passes 4, tests 2 — three distinct numbers | +| `test_the_two_values_are_not_aliases_of_one_list` | the attack that fails: an **in-place** removal is refused too | | `test_a_record_lost_in_transport_is_refused` | **the arm this phase exists for** | | `test_a_verdict_outside_the_closed_set_is_refused` | the schema half | -| `test_a_record_created_after_the_report_is_NOT_caught` | **the limit**, pinned because I claimed the opposite | +| `test_the_recorder_is_only_observed_once_and_both_sides_of_that_are_blind` | **the limit**, pinned in both directions | **The obvious reconciliation here is vacuous by construction, and phase 1 made it so on purpose.** `count` *is* `len(self._records)`, so checking one against the @@ -2503,16 +2504,29 @@ Measured on the pinned runner, `user_properties` survive that crossing intact. **What it catches:** a record dropped or mangled between the report being built and the report being read, and a verdict outside the closed set. -**What it does not**, and the first version of this section claimed the first of -these wrongly: a record created **after** the report was built, because both -quantities come from one read of the recorder at one instant — measured, the -recorder held 4, the report carried 3, and the run passed; and a record that is -present, transported, well-formed and **wrong**, which is phase 2's job. - -So it is a transport check rather than a completeness check. The limit is not -straightforwardly fixable — the totals are built from what arrived, and under `-n` -the controller has no recorder to consult — so it is pinned by an arm instead of -described by a sentence. +**What it does not:** anything that changes the recorder outside the single +instant it is read, in **either** direction — a record appended after, or removed +before. Both are invisible and the run passes. It also does not catch a record +that is present, transported, well-formed and **wrong**; that is phase 2's job. + +The earlier version of this section named only the later half. The **earlier** half +is the more reachable one: a late append needs someone outside the layer, while an +early loss is what a bug inside the recorder would look like. + +**The reason is not xdist**, which an earlier version also claimed. The `expect` +fixture's teardown pops the recorder, so nothing after `makereport` can read it in +a single process either. And it is not unfixable: keeping the final *count* in a +session-level map past teardown and reconciling at worker-side `sessionfinish` +would close it — unbuilt and unmeasured, so named rather than planned. + +So it is a transport check rather than a completeness check, and both halves of the +gap are pinned by an arm instead of described by a sentence. + +**The two values are not aliases**, which is the objection worth recording because +the attack on it fails: the transport arm drops a record with a slice, which copies, +so an in-place `value.pop()` was injected instead — still refused. That shows +separate storage **in a single process**, which the xdist run cannot, since +serialisation copies everything by definition. **The arms inject the failure from a conftest**, because no in-tree code drops a record — an arm that waits for a real defect to appear is not evidence the check diff --git a/test/pytest/pgc_vacuity.py b/test/pytest/pgc_vacuity.py index 01e15b0b..10ded547 100644 --- a/test/pytest/pgc_vacuity.py +++ b/test/pytest/pgc_vacuity.py @@ -1175,20 +1175,29 @@ class _RecordCollector: WHAT IT DOES NOT, and the first version of this comment claimed the first of these, wrongly: - * A RECORD CREATED AFTER THE REPORT WAS BUILT. Both values are taken from one - read of the recorder at one instant, so a later append is invisible to both - and the run passes. Measured, by appending from a hook outside this layer's: - - recorder now holds 4; report carries 3 - checks run: 3 - accounting: 3 pass + 0 fail + 0 unrun = 3 - 1 passed, rc=0 - - It is not straightforwardly fixable either, and that is the honest reason it - is a limit rather than a TODO: the totals are BUILT from what arrived, and - under -n the controller has no recorder to consult -- the worker's is in - another process. An arm in test_check_records.py pins this so it cannot be - re-claimed. + * ANYTHING THAT CHANGES THE RECORDER OUTSIDE THE SINGLE INSTANT IT IS READ, + in EITHER direction. Both values come from one read, so: + + a record appended AFTER the read invisible, run passes + a record removed BEFORE the read invisible, run passes + + Measured both ways. The first version of this comment named only the later + half, and @OffgridwithJD injected the earlier one -- which is the MORE + reachable of the two, because a late append needs someone outside the layer + while an early loss is what a bug inside the recorder would look like. + + THE REASON IS NOT XDIST. An earlier version said the controller has no + recorder to consult under -n. The real reason needs no xdist: the `expect` + fixture's teardown pops the recorder, so nothing after makereport can read + it in a single process either. + + AND IT IS NOT UNFIXABLE, which that version also implied. @OffgridwithJD's + proposal: keep the final COUNT -- an int, not the records -- in a + session-level map that survives teardown, and reconcile the sum at + worker-side sessionfinish, where the worker has its own slice and needs + nothing from the controller. Unbuilt and unmeasured here, so it is named + rather than planned. An arm in test_check_records.py pins both halves of + the gap so neither can be claimed away. * A record that is present, transported and well-formed, and WRONG. That is phase 2's job. diff --git a/test/pytest/test_check_records.py b/test/pytest/test_check_records.py index a0d20e52..26f72654 100644 --- a/test/pytest/test_check_records.py +++ b/test/pytest/test_check_records.py @@ -454,25 +454,45 @@ def test_one_claim(expect): result.stdout.fnmatch_lines(["*checks run: 3*"]) -def test_the_total_is_printed_from_the_records_not_from_the_test_count(pytester, expect): - """Three assertions across two tests is 3, not 2. A total that counted TESTS - would agree with the record count whenever every test made exactly one claim, - which is the case a hand-written fixture reaches for first.""" +def test_the_total_separates_records_from_passes_and_from_tests(pytester, expect): + """THREE DISTINCT NUMBERS, because two were not enough (@OffgridwithJD). + + My first version used an all-PASS fixture: five claims across two tests, so + records 5 and passes 5. A totals line counted from PASSES would have been + indistinguishable from one counted from records, and only the test count was + separated. Measured on that fixture: + + checks run: 5 + accounting: 5 pass + 0 fail + 0 unrun = 5 + + Making one of the five claims false and catching it gives three numbers that + disagree, so the line can only be right for one reason: + + records 5 passes 4 tests 2 + + One dead end recorded so it is not tried again: `cannot_run` contributes an + UNRUN record but fails its own test, so an unrunnable fixture does not + separate them either. + """ pytester.makepyfile( """ def test_four_claims(expect): expect.num(1, 1, "a") expect.num(2, 2, "b") expect.num(3, 3, "c") - expect.num(4, 4, "d") + try: + expect.num(4, 99, "d -- deliberately false, and caught") + except AssertionError: + pass def test_one_claim(expect): expect.num(5, 5, "e") """ ) result = pytester.runpytest("-p", "pgc_vacuity") + expect.outcomes(result, "premise: two tests, both passing", passed=2, failed=0) result.stdout.fnmatch_lines(["*checks run: 5*"]) - expect.outcomes(result, "and the run itself is clean", passed=2, failed=0) + result.stdout.fnmatch_lines(["*accounting: 4 pass + 1 fail + 0 unrun = 5*"]) def test_a_record_lost_in_transport_is_refused(pytester, expect): @@ -522,6 +542,49 @@ def pytest_runtest_makereport(item, call): result.stderr.fnmatch_lines(["*held 3 record(s) and 2 arrived*"]) +def test_the_two_values_are_not_aliases_of_one_list(pytester, expect): + """The attack that FAILS, and it is stronger evidence than the xdist run. + + @OffgridwithJD's objection: the transport arm drops a record with `value[:-1]`, + which COPIES. So the unfair-in-my-favour reading is that the report carries the + recorder's own list and the only reason the arm fires is the slice. + + Mutating in place settles it. `value.pop()` reaches whatever object the report + actually holds, and the run is still refused -- so `held` and `arrived` are not + two views of one list. `held` is an int; `pgc_records` is a freshly built list + of fresh tuples; there is no shared object to reach. + + WHY THIS IS THE STRONGER HALF. The xdist run proves the comparison survives + serialisation. This proves the two values are not aliases, IN A SINGLE PROCESS, + which xdist cannot show because serialisation copies everything by definition. + """ + pytester.makepyfile( + """ + def test_three_claims(expect): + expect.num(1, 1, "a") + expect.num(2, 2, "b") + expect.num(3, 3, "c") + """ + ) + pytester.makeconftest( + """ + import pytest + + @pytest.hookimpl(wrapper=True, tryfirst=True) + def pytest_runtest_makereport(item, call): + report = yield + if call.when == "call": + for key, value in report.user_properties: + if key == "pgc_records" and value: + value.pop() # IN PLACE, not a slice + return report + """ + ) + result = pytester.runpytest("-p", "pgc_vacuity") + expect.run_failed(result, "an in-place removal is refused too") + result.stderr.fnmatch_lines(["*held 3 record(s) and 2 arrived*"]) + + def test_a_verdict_outside_the_closed_set_is_refused(pytester, expect): """The schema half. A verdict the reader cannot key on is a record that says nothing, and `pgc_record` refuses the same thing on the shell side rather than @@ -552,23 +615,36 @@ def pytest_runtest_makereport(item, call): result.stderr.fnmatch_lines(["*SORTOF*"]) -def test_a_record_created_after_the_report_is_NOT_caught(pytester, expect): - """THE LIMIT, pinned so it cannot be re-claimed. I claimed the opposite. - - The PR body, the CHANGELOG and the layer's own comment all said phase 3 - catches a record created after the report was built. It does not, and the - reason is structural rather than an oversight: both quantities are taken from - ONE read of the recorder at ONE instant, so a later append is invisible to - both. - - It is also not straightforwardly fixable. The totals are built from what - ARRIVED, and under `-n` the controller has no recorder to consult -- the - worker's lives in another process. So this is a transport check, and the - honest thing is an arm that says where the edge is rather than a sentence - claiming there is none. - - Found by attacking my own PR description before anyone reviewed it, which is - the fourth claim of mine this chain that no arm defended. +def test_the_recorder_is_only_observed_once_and_both_sides_of_that_are_blind( + pytester, expect): + """THE LIMIT, pinned in BOTH directions. My first version named half of it. + + Phase 3 compares two values taken from ONE read of the recorder at ONE + instant, so anything that changes the recorder outside that instant is + invisible. That has two halves and I pinned only the later one: + + a record appended AFTER the read invisible -- run passes + a record removed BEFORE the read invisible -- run passes + + @OffgridwithJD injected the second and got a clean pass: `checks run: 2`, + `accounting: 2 pass + 0 fail + 0 unrun = 2`, rc 0. **The early half is the + more reachable one**, and that is the part my framing got backwards: a late + append needs someone outside the layer to do it, while an early loss is what + a bug inside the recorder would look like. + + THE REASON IS NARROWER THAN I WROTE, TOO. I said the controller has no + recorder to consult under `-n`. The real reason needs no xdist at all: the + `expect` fixture's teardown pops the recorder (`pgc_vacuity.py`, the `expect` + fixture), so nothing after `makereport` can read it in a single process + either. + + AND "NOT STRAIGHTFORWARDLY FIXABLE" OVERSTATED IT. @OffgridwithJD's proposal: + keep the final COUNT -- an int, not the records -- in a session-level map that + survives teardown, and reconcile the sum at worker-side `sessionfinish`, where + the worker has its own slice and needs nothing from the controller. Today + `sessionfinish` returns early for workers, which is correct for the + collected-versus-reported check and is what forecloses this one. Unbuilt and + unmeasured, so it is a named proposal rather than a plan. """ pytester.makepyfile( """ @@ -594,6 +670,29 @@ def pytest_runtest_makereport(item, call): """ ) result = pytester.runpytest("-p", "pgc_vacuity") - expect.outcomes(result, "a late record does NOT refuse the run -- this is the limit", + expect.outcomes(result, "a LATE append does not refuse the run -- half the limit", passed=1, failed=0) result.stdout.fnmatch_lines(["*checks run: 3*"]) + + # THE OTHER HALF, and the more reachable one. trylast makes this the INNERMOST + # wrapper, so it runs BEFORE the layer reads the recorder -- the mirror of the + # tryfirst above. + pytester.makeconftest( + """ + import pytest + import pgc_vacuity + + @pytest.hookimpl(wrapper=True, trylast=True) + def pytest_runtest_makereport(item, call): + report = yield + if call.when == "call": + rec = pgc_vacuity._RECORDERS.get(item.nodeid) + if rec is not None and rec._records: + rec._records.pop() + return report + """ + ) + early = pytester.runpytest("-p", "pgc_vacuity") + expect.outcomes(early, "an EARLY removal does not refuse it either", + passed=1, failed=0) + early.stdout.fnmatch_lines(["*checks run: 2*"])