test/pytest: a guard in a teardown cannot fail the test it guards (#432) - #931
Conversation
…mmandprompt#432) Measured, the same AssertionError raised from two places: from a pytest_runtest_call wrapper 1 failed from a fixture teardown 1 passed, 1 error pytest has already recorded the call phase as passed by the time a teardown runs, so a refusal there arrives as a separate ERROR on the same node-id and the test's own outcome stays `passed`. Anything counting passes -- --pgc-expect-tests, a CI summary, a human reading "N passed" -- sees a pass. NOTHING WAS BROKEN. This layer's vacuity guard is already in the call-phase wrapper, which is why a test that concludes nothing is FAILED rather than passed-with-an-error. What was missing is that nothing said so. Moving it into the `expect` fixture's teardown is a plausible-looking refactor -- the fixture is right there, it already has a teardown, and the guard reads as cleanup -- and it would have turned every vacuous test from `failed` into `passed` with an error beside it. TWO ARMS, AND THE SECOND IS THE CONTROL. Without it the first passes whatever phase the guard is in: "a vacuous test fails" is equally true of a correctly placed guard and of no guard at all standing next to an unrelated failure. So the second arm asserts the teardown shape really does report a pass, which states the mode as a measurement rather than as a warning. PROVED BY MOVING IT. With the call-phase raise disabled and the same refusal placed after the `expect` fixture's yield, the first arm reddens; restored, 11 passed. NARROWED, NOT CLOSED, and VACUITY_MODES.md 3.7 says which half. Closed: this layer's own guard placement. Still open: the general shape -- a guard anyone adds later in a teardown still cannot fail its test, and nothing refuses that. I would rather leave that written down than claim the family. MEASURED pytest corpus 164 passed harness_selftest 538 checks, 538 passed + 0 failed + 0 unrunnable, rc 0 the new arms 2, and the first reddens when the guard moves to the teardown Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
jdatcmd
left a comment
There was a problem hiding this comment.
Approving at 3d87b4c0, 12/12 SUCCESS. I verified the claim, the removal proof, and the control separately, because the control is what makes the first arm worth anything and you say so yourself.
The claim reproduces, independently of your fixtures. Two files of my own, the same AssertionError from two places:
call phase -> FAILED test_call.py::test_x -> 1 failed
teardown -> ERROR test_teardown.py::test_x -> 1 passed, 1 error
And the thing that matters, what a pass-counter reads off the teardown run:
1 passed
1 error
So --pgc-expect-tests and every summary would see a pass. The table in your body is exact.
The removal proof holds. I moved the guard the way the refactor would — deleted the VacuityError from the pytest_runtest_call wrapper and raised the identical error after the expect fixture's yield:
FAILED test_runshape.py::test_the_vacuity_guard_fails_the_test_rather_than_erroring_beside_it
1 failed, 10 passed
Exactly the arm that claims the guard fails the test, and nothing else. Baseline 11 passed; pgc_vacuity.py restored byte-identical.
The control is a real assertion, not a shape. expect.outcomes(result, ..., passed=1, failed=0, errors=1) reddens on each field independently — I mutated each of the three in turn and got 1 failed every time. So it pins the outcome triple rather than merely observing that something happened. It also cannot be reddened by any change to this layer, which is correct for what it is: it pins an assumption about pytest, and if pytest ever stopped reporting a teardown failure this way, the placement argument would need revisiting and this is the arm that would say so.
It still composes. #927 landed on main while I was reviewing this, so I merged your branch onto the new main locally: clean, 4 files changed, 121 insertions(+), 2 deletions(-), no conflict.
On the scoping, which is the part I would have argued about if you had not. "Nothing was broken — what was missing is that nothing said so" is the honest framing, and narrowing 3.7 rather than claiming the family is the right call. A guard anyone adds later in a teardown still cannot fail its test, the totals correctly do not move, and the mode stays in section 3. That is a measurement with its limits stated, which is worth more than a closed row that is not really closed.
Nothing blocking, nothing I could break.
One conflict, CHANGELOG.md: both sides append a bullet at the top of the same section. Resolved by keeping BOTH, since choosing one loses a shipped entry. TESTS.md and VACUITY_MODES.md auto-merged -- commandprompt#931 narrowed section 3.7 without moving a mode, so the 27 / 45 / 72 counts this branch composed still hold. The corpus on the merged tree: 205 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
…080 sweep fix One conflict, CHANGELOG.md, and both sides append a bullet at the top of the same section -- kept both. commandprompt#927 deleted test/selftest/440 on main and this branch never touched it, so nothing else met. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
…harness-guards branch Two conflicts, both additive. CHANGELOG.md: two regions, both sides appending at the top of the same section -- kept both. TESTS.md: both sides number a section, and the raises section main landed at 18 collides with this branch, so it is renumbered 20 and its TOC entry and anchor follow. Verified structurally: 20 headings, 20 TOC entries, numbers contiguous 1..20, titles identical between the two lists, every TOC anchor equal to the anchor GitHub derives from its heading. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
Two corrections from @OffgridwithJD's review, and the first one is the document's own rule 3 catching the document. "39 call sites" was 36 calls plus the 3 definitions. The pattern `[^_a-z]_sh(` matches `def _sh(` as readily as a call, which is the same class of error as `[a-z_]+\.sh` matching `sharedir` -- already written three lines above as the thing not to do. Counted with ast now, and the entry says how, because a number in this section has to be re-derivable or it does not belong here. The heading said 4 python files. Three are on main; the fourth arrives with PR #923. The entry always said so, the heading did not, and a reader who stops at the bold line gets a count that is wrong today. Recounted against main at aa53c1b, after #927 and #931 landed: still 3 python files and 7 shell files. test_raises_sqlstate.py, new on main, adds neither -- it drives pytester, not the shell. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK
#927, #931 and #930 landed while this waited on review, so main gained three CHANGELOG entries and two TESTS.md sections. CHANGELOG: both sides append at the top of the same section and neither replaces anything, so the union is the resolution. TESTS.md: the numbering collided. This branch inserted its file section at 15 and pushed "Adding a test", "What this corpus does NOT yet refuse" and "Traps this corpus records" to 16-18; main kept those at 15-17 and appended its two new file sections as 18 and 19. Auto-merge produced two sections numbered 18. Resolved main's way, because main's convention is now to append a new file section after the tail sections: this branch's section becomes 20, and the three tail sections go back to main's 15, 16 and 17. That renumbers one section of this branch rather than two of main's. Checked rather than eyeballed, because an anchor that stops resolving does not announce itself: 20 headings against 20 TOC entries, every TOC text equal to its heading, every anchor equal to what GitHub derives from that heading, and the numbering contiguous 1..20. On the composed tree: selftest 350 41 checks 0 failed, selftest 400 64 checks 0 failed, selftest 080 15 checks 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK
#923 moved five times while this waited, and #921, #927, #930, #931 and #932 landed on main underneath it. Composing found two things that a clean merge would not have. TESTS.md: the base now carries 22 sections, and this branch had inserted the ledger at 16. Auto-merge kept the base's tail sections AND this branch's copies of them, so the file would have had two of each. Resolved by keeping only the ledger section from this side, renumbered to 23, where the base's own 22 already ends. Checked rather than eyeballed: 23 headings against 23 TOC entries, every TOC title equal to its heading, every anchor equal to GitHub's derivation, numbering contiguous 1..23, no duplicate heading. NO_CLUSTER gains test_mutation_ledger.py. #921's classifier arrived on the base and immediately named it: membership_report: [1: undeclared:test_mutation_ledger.py] It drives test/pgc_ledger.py, a python tool rather than the shell harness, so it needs neither a cluster nor psycopg. Measured rather than assumed: 9 passed in a venv with no driver, and driver_dependent() agrees. The derived job goes to 11 files and 179 passed. THE LEDGER IS REGENERATED, AND THAT IS THE POINT OF THIS MERGE RATHER THAN A SIDE EFFECT. #923 added 17 checks to selftest 400 and converted 25 skip sites, none of which the committed ledger had ever seen. The gate refuses a check it has never seen, so the composed tree would have failed CI for a reason with nothing to do with either change. Regenerating is the documented repair, and the budget file says so. From a real run of the composed tree, not a synthesised log: harness_selftest.sh: PASSED, rc=0 checks run: 735 | accounting: 735 passed + 0 failed + 0 unrunnable + 0 skipped ledger: 701 rows -> 734 | never=734, ever red=0 gate: new this run=0 Reconciled: 735 records == 732 distinct (suite, part, name) + 3 names that each appear twice in one run, and 0 log triples are missing from the ledger, which is exactly what the gate refuses. All 734 rows carry five fields and none ends in a tab. The budget's asserted census follows to 734. Two ledger rows do not appear in this log -- selftest 330's "all three runner functions", which #923 changed to five. rename-scan reports appeared=0, vanished=2. THEY ARE LEFT DELIBERATELY: this log is PG17 only, and pruning rows that a single major did not produce would delete checks that legitimately run elsewhere. The gate refuses unseen checks, not unused rows. Gates on the composed tree: 350 53/53, 400 81/81, 410 96 checks 0 failed, 080 15/15, shellcheck rc=0 over the whole harness, driver-free job 11 files 179 passed, membership_report []. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK
`INSERT ... SELECT ... WHERE false` writes nothing and raises nothing. psycopg
reports `INSERT 0 0` with a `rowcount` of 0, and nothing in the corpus read either
field -- so the fixture a test meant to build did not exist, and every assertion
below it compared two empty things. That is `insert-wrote-no-rows`, entry 2 on
VACUITY_MODES.md's own list of what to add next.
Measured before building: 12 write statements across 5 files, and `rowcount` read
at exactly one site, which does not assert on it.
THE COMMAND TAG DECIDES, NOT THE ROW COUNT. `SELECT 0` and `INSERT 0 0` both carry
`rowcount == 0`, so a guard keyed on the count alone would refuse every test whose
last statement was a SELECT over an empty result -- a legitimate and common
assertion. `statusmessage` is the server's own command tag, so this guard never
parses SQL. Measured on PG 18 against a pgcolumnar table:
statusmessage rowcount statement
CREATE TABLE -1 CREATE TABLE t (i int) USING pgcolumnar
INSERT 0 5 5 INSERT INTO t SELECT g FROM generate_series(1,5) g
INSERT 0 0 0 INSERT ... WHERE false
UPDATE 0 0 UPDATE t SET i = i WHERE i > 100
DELETE 0 0 DELETE FROM t WHERE i > 100
SELECT 0 0 SELECT * FROM t WHERE false
SET -1 SET search_path TO public
TRUNCATE TABLE -1 TRUNCATE t
A DELIBERATE ZERO STAYS WRITABLE. A DELETE that must match nothing is a real
negative control, so `expect.wrote(cur, 0, name)` both compares the count and marks
the write as named. An unnamed zero fails the test; naming a count does not excuse
a wrong one; and a `rowcount` of -1 is refused rather than compared, for the reason
`expect.rowcount` already records.
IN THE CALL PHASE, NOT A TEARDOWN. #931 measured that a guard run as a teardown
fixture reports the test it guards as PASSED and fails separately, so a reader sees
a green test beside an error.
TWO PROPERTIES, TWO FILES, AND THE SPLIT IS MEASURED RATHER THAN ASSERTED. The
classifier and the refusal live in test_writes_wrote_rows.py, which needs no
database: a stub cursor carrying the two measured fields exercises them exactly.
Whether the connection the tests actually use is watched is a different claim that
no driver-free arm can make, and test_the_connection_the_tests_use_is_watched makes
it through a real `INSERT ... WHERE false`, on both `conn.execute` and a cursor the
connection handed out -- 24 and 42 sites in the corpus, so a proxy watching only the
connection would leave most of it unwatched.
Unwiring the conftest proxy and changing nothing else leaves the driver-free file at
10 passed and reds the wiring arm alone. That is #917's defect in miniature: its
pytest twin tested the reconciler's body and left the runner's CALL to it
uncovered, so removing the call kept the pytest half green at 9 passed while the
shell half went red by one.
PROVE BY REMOVAL, five mutations, each applied by exact string match with the file
asserted to still parse:
control free 10 passed wiring 9 passed
no refusal free 2 failed wiring 9 passed
command tag ignored free 2 failed wiring 1 failed
every statement is a write free 4 failed wiring 1 failed
connection not wrapped free 10 PASSED wiring 1 failed
acknowledgement not recorded free 1 failed wiring 1 failed
FALSE-POSITIVE BUDGET FIRST, because a guard that reddens a legitimate write is
worse than the mode: the full corpus with a cluster is 249 passed, and none of the
12 write sites already in the corpus reddened.
TWO OF MY OWN ARMS WERE WRONG, recorded because each produced a green that meant
nothing:
* An arm asserting only `failed=1` passed before the feature existed. The inner
test called a function not yet written, got an AttributeError, and the outer arm
read that as the comparison failing. Naming the numbers is what makes the red
the right red.
* A multi-word pattern can straddle pytest's word wrap. `expect.refusal` anchors
each pattern to one `E` line and pytest wraps a long traceback line, so matching
"wrote no rows" failed against a message containing it -- which reads exactly
like "the guard did not fire". The refusal now leads with the mode's own
kebab-case id, which is one token and cannot be split.
Docs carried in the same commit, per jd's rule. VACUITY_MODES.md moves the mode to
section 2 and keeps a back-reference where it was, and its stated totals go 27 -> 28
refused and 45 -> 44 not refused -- checked by the corpus's own arms, which is how I
learned the numbers rather than deciding them. TESTS.md gains section 22, and the
paragraph in section 18 that listed this gap is rewritten rather than deleted,
because a reader who knew the gap needs to find out where it went.
Gate on this tree:
harness_selftest 588 checks, 588 passed + 0 failed + 0 unrunnable, PASSED
driver-free job 10 files, 171 passed, psycopg absent from the venv
full corpus 249 passed with a cluster on pg18a
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
The same
AssertionError, raised from two places:pytest_runtest_callwrapper1 failed1 passed, 1 errorBy the time a teardown runs, pytest has already recorded the call phase as passed. A refusal there arrives as a separate
ERRORon the same node-id and the test's own outcome stayspassed. Anything counting passes —--pgc-expect-tests, a CI summary, a human reading "N passed" — sees a pass.Nothing was broken
This layer's vacuity guard is already in the call-phase wrapper, which is why a test that concludes nothing is
failedrather than passed-with-an-error. What was missing is that nothing said so.Moving it into the
expectfixture's teardown is a plausible-looking refactor — the fixture is right there, it already has a teardown, and a guard reads as cleanup — and it would have turned every vacuous test in the corpus fromfailedintopassedwith an error beside it. The corpus would still have exited non-zero, so it is not silent; but the per-test verdict, which is what--pgc-expect-testsand every summary read, would have said pass.Two arms, and the second is the control
Without the control the first arm passes whatever phase the guard is in: "a vacuous test fails" is equally true of a correctly placed guard and of no guard at all standing next to an unrelated failure. So the second arm asserts that the teardown shape really does report a pass, which states the mode as a measurement rather than as a warning.
Proved by moving it: with the call-phase raise disabled and the same refusal placed after the
expectfixture'syield, the first arm reddens. Restored, 11 passed.Narrowed, not closed
VACUITY_MODES.md3.7 now says which half is which.I would rather leave that written down than claim the family. The inventory's counts are therefore unchanged — the mode stays in section 3, which is why none of the totals move in this diff.
Evidence
Relationship to my other open PRs
Independent, off
mainatf0f1f40, like #926, #927 and #930 — none blocks another. This one touchestest_runshape.py,TESTS.mdsection 10,VACUITY_MODES.md3.7 andCHANGELOG.md.It will conflict with #927 and #930 in
VACUITY_MODES.mdandCHANGELOG.md— the append-at-the-top kind, not a code conflict. Those two also move a mode from section 3 to section 2 and so edit the count rows; this one does not, because 3.7 is narrowed rather than closed. Whichever lands last, I will resolve and re-run both harnesses rather than adjust numbers by hand.🤖 Generated with Claude Code
https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a