test/pytest: a loud refusal is not a silent loss (#991) - #1005
Conversation
linuxhikerpm
left a comment
There was a problem hiding this comment.
The hatch is real and I drove it at 578932a80702b61ddfa3a49ff2ddb058b2312cc9.
main pgc_vacuity + these tests serial: lost them silently got 1 want 0
this head 7 passed, including both modes and the genuine-loss control
The serial row is the one #963 left, and the control still names test_vanishes_without_a_word when nothing refused. I would approve the code on that.
Blocking: docs_style.sh fails on this CHANGELOG. Three em dashes, got [3] want [0]. docs_style is a registered suite, so the PG17/PG18 jobs will red for a reason unrelated to the refusal. The three sites are the new #991 bullet (the "so — and" / "prints — a" / "worker — pytest" sentences). -- is the spelling the rest of Unreleased uses.
Do not merge from this review.
The layer refused a run and then contradicted itself:
ERROR: the pgColumnar vacuity layer refuses this run: a bare skip is refused...
VACUITY: 1 collected test(s) never reported an outcome, so the run lost them
silently: ...
The run did not lose it silently. It refused it LOUDLY, one line above. A reader
who typed one bare @pytest.mark.skip got the correct diagnosis plus a second
finding telling them a test vanished without saying so, and the natural response is
to hunt a lost test that was never lost.
`collected - reported` is the right set difference and the wrong MEANING: a silent
loss is when nobody said anything, and here the layer itself stopped the run. The
guard whose subject is a silent loss was firing on the one event that is its
opposite.
ONE ASSIGNMENT COVERS ALL FIVE REFUSAL SITES, because commandprompt#963 gave the layer a single
chokepoint. `_collection_usage_error` records the refusal before either branch;
`_RunShape.pytest_sessionfinish` skips the missing-outcome problem when it is set.
ONLY that problem -- the setup-skip one still prints, because a fixture removing
every test that depends on it is not something a refusal accounts for.
SERIAL WAS THE ONLY PATH LEFT, which is why the fix is this small:
main serial refusal + the silent-loss line
main xdist no refusal at all (that was commandprompt#963)
commandprompt#963 serial refusal + the silent-loss line <- what this closes
commandprompt#963 xdist refusal, no line
commandprompt#963 clears items[:] in the worker, so the controller's `collected` set is already
empty under -n.
Red to green, driven on both trees:
the new arms against main 1 failed -- the SERIAL row, as predicted
the xdist row PASSED on main, confirming commandprompt#963
closed that half
the same arms with the fix 3 passed, 11 checks
whole corpus on the branch 365 passed, 923 checks
THE SECOND ARM IS THE CONTROL and the first is worth nothing without it: an item
collected and never reported, with NO refusal anywhere, is still named and still
reddens the run. Dropping a problem from a reconciliation is one edit away from
dropping the guard. The fixture removes an item after the layer's hook recorded it
and without deselecting it -- the shape of a crashed worker, and `pytest_deselected`
is what tells a deliberate subset from a loss.
Two of this tree's own guards caught my omissions while I wrote it, and both
derive from the corpus rather than being told: test_docs_cover_the_corpus refused
the new tests until TESTS.md named them, and test_harness_deps then reported the
no-driver batch as rc 1 -- one root cause surfacing twice, because the docs guard
is IN that batch.
No shell check moved, so no ledger change: harness_selftest 907 checks on both
trees.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
578932a to
e11e1fc
Compare
linuxhikerpm
left a comment
There was a problem hiding this comment.
Re-reviewed at e11e1fc59e80cddefba65a7160bc98ec295bc6a3. CHANGELOG em dashes are gone (em 0 en 0), which is the docs_style.sh hatch that failed suites on the previous head.
Drove the two new tests:
this head 3 passed, 11 checks
main pgc_vacuity + these tests serial: lost them silently got 1 want 0
(xdist still 0, as #963 already emptied collected)
The control (test_a_genuine_silent_loss_is_still_reported) stayed green on both trees. Do not merge from this side.
jdatcmd
left a comment
There was a problem hiding this comment.
Approving. The diagnosis is right, the fix is at the one place that covers every surface, and the removal proof lands.
Verified rather than read
One assignment really does cover every call site. config._pgc_vacuity_refused = msg sits at line 1507, inside _collection_usage_error (1484), and there are exactly five call sites — 1535, 1543, 2108, 2154, 2218. So the claim in the comment holds by construction rather than by inspection of each caller.
Worth noting the composition: two of those five are surfaces that only started going through this function today. #963 routed the module-binding refusal; #986's conflict resolution routed the Expect-method refusal this afternoon. Before that merge, a method-stub refusal would have set no flag and would still have been reported as a silent loss. This PR lands on the tree where "every refusal comes through here" became true, and it would have been a partial fix a day ago.
Removal proof:
revert `if missing and not refused:` to `if missing:`
-> test_a_collection_refusal_is_not_also_reported_as_a_silent_loss[serial] FAILED
The full database-free batch on the branch: 267 passed, 637 checks, 0 fail.
One note, and your docstring already contains the answer
The mutation reddens [serial] and not [xdist]. Your own table says why:
#963 serial refusal + the silent-loss line <- what this closes
#963 xdist refusal, no line
Clearing items[:] in the worker had already emptied the controller's collected set, so the xdist row had no line to suppress before this PR. That makes the [xdist] arm a regression guard for #963's property, not a second measurement of this fix — it passes on the mutant and on the repair alike.
That is the right arm to have and I would not remove it. What I would add is one sentence saying so, because the docstring states the four-row history and leaves the reader to infer that only one of the two parametrized arms can fail for this change. Two green arms read as two confirmations; here they are one confirmation and one guard. Given how much of tonight went into arms that could not fail, naming the one that cannot fail for this reason seems worth the line.
The diagnosis is the part worth keeping
collected - reportedis the right set difference and the wrong MEANING: a silent loss is when nobody said anything, and here the layer itself is what stopped the run.
That is the whole bug in one sentence, and it is a shape that will recur: a set difference that is arithmetically correct and semantically wrong because the reason for the difference is not in the sets. The guard whose subject is silence fired on the loudest event the layer produces.
Verdict
Approved. Correct fix, correct place, proof both ways, and the limitation of the xdist arm is disclosed rather than papered over.
This PR was DIRTY and no CI had ever run on it: `statusCheckRollup` was EMPTY, which reads as "0 pending, 0 failing" and is not the same thing as green. The conflict is mine -- commandprompt#1007, commandprompt#1005 and commandprompt#1008 merged in the last hour and all three touch the two files below. TWO CONFLICTS, BOTH ADDITIVE COLLISIONS, BOTH RESOLVED BY KEEPING BOTH SIDES. docs/user-guide.md: two feature bullets, neither a revision of the other. test/pytest/TESTS.md: both sides appended a section and BOTH CALLED IT 28. commandprompt#1007's test_docs_join_clustering landed first and keeps 28; this PR's test_join_vector_agg becomes 29. Heading, contents-list entry and anchor moved together, because the corpus guard resolves every contents-list link and a renumbered heading with a stale anchor is a broken link that still looks right. That collision is commandprompt#996 in a different file: every change appends at one anchor with a sequential number, so any two of them conflict by construction. Verified on the merged tree: test_docs_cover_the_corpus 31 passed, 73 checks (every test named, every contents-list anchor resolves) the 14 database-free files 272 passed, 650 checks, 0 fail ledger census stated 1162, holds 1162: they agree THE LEDGER LOOKED WRONG AND IS NOT, which is worth recording because it is commandprompt#1004 meeting reality four hours after it was written. 1165 rows against 1162 `never`. Three rows now carry a last-red date -- two of commandprompt#1008's arms and this PR's own `unique join uses vectorized agg when GUC on`, which carries the mutation `drop JOINREL fold`. That is a removal proof recorded in the ledger, exactly what the column is for. `grep -c` over the file gives 1165 and the gate wants 1162, so the wrong derivation I shipped this afternoon would now produce a refusal on a correct tree. No code change; the C in this PR is untouched by the merge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EbyGSaU93XYQr8aH4NrUiw
The layer refused a run and then contradicted itself. One bare
@pytest.mark.skip:The run did not lose it silently. It refused it loudly, one line above. A reader who typed one bare skip got the correct diagnosis and then a second finding telling them a test vanished without saying so — and the natural response is to go hunting a lost test that was never lost.
collected - reportedis the right set difference and the wrong meaning. A silent loss is when nobody said anything; here the layer itself is what stopped the run. The guard whose entire subject is a silent loss was firing on the one event that is its opposite.The fix is one assignment, because #963 built the chokepoint
Every refusal in the layer routes through
_collection_usage_error, so recording it there covers all five call sites:and the reconciliation skips only that problem:
The setup-skip problem still prints. A fixture removing every test that depends on it is not something a refusal accounts for, and the two are independent findings.
This PR had to wait for #963 rather than being written on the base it was filed against: on
mainbefore that, theUsageErrorwas raised inline at each site and there was no single place to record it. Writing it earlier would have meant touching five raise sites or inventing the helper #963 was already adding.Serial was the only path left, which is why the change is this small
#963 clears
items[:]in the worker, so the controller'scollectedset is already empty under-nand the contradiction cannot arise there.Red to green, driven on both trees
The second arm is the control, and the first is worth nothing without it
Dropping a problem from a reconciliation is one edit away from dropping the guard. So
test_a_genuine_silent_loss_is_still_reportedasserts that an item collected and never reported, with no refusal anywhere, is still named and still reddens the run:Removed after the layer's own hook recorded it, and without deselecting — which is the shape of a crashed xdist worker.
pytest_deselectedis the hook that tells a deliberate subset from a loss, and nothing calls it there. The arm checks the sentence, the name of the vanished test, andrc=1.Two of this tree's guards caught my omissions, and it was one root cause
test_docs_cover_the_corpusrefused the new tests untilTESTS.mdnamed them.test_harness_depsthen reported the no-driver batch asrc=1— which looked like a second, unrelated failure and is not: the docs guard is inside that batch. I checked that my own tests pass with psycopg shimmed out before concluding it, rather than assuming the obvious culprit.Limits
harness_selftestis 907 checks on this tree and onmain. No shell check was added, removed or renamed.config, not the session.pytest_sessionfinishreceivessessionand reachessession.config; a worker's config is a different object from the controller's, which is why the worker branch sets it too even though the worker's ownsessionfinishreturns early.Part of #991. The issue also notes
session.exitstatus = 1being discarded on this path —UsageErrordecides the code aftersessionfinish— which is the right outcome reached by accident rather than by the guard deciding not to lower it. That is untouched here and #991 stays open for it.🤖 Generated with Claude Code
https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a