test: a collection-time vacuity refusal keeps its reason under xdist (#963)

[linuxhikerpm]

Fixes #963.

Under -n, a collection-time UsageError lost the sentence that says what was wrong. Serial printed it on stderr and exited 4. -n 2 replaced it with a 35-line INTERNALERROR and exited 1. A VacuityError raised inside a test body was fine either way.

The cause is a finally. pytest_collection_modifyitems raises; pytest still runs pytest_collection_finish, so the worker tells the controller it collected the tests and then exits. xdist's worker_workerfinished asserts a worker that collected tests must not finish with them pending.

A worker now records the sentence on workeroutput and clears the items so no ids cross. The controller re-raises UsageError from pytest_testnodedown, which is the process serial already used.

The issue named three options. Refusing -n would drop a runner this layer already registers an xdist hook for. Turning the refusal into a test failure would keep rc 1, which is the same code a failing test gives. That left reporting the UsageError on the controller.

The table is the test:

REFUSAL                         MODE     rc   reason   INTERNALERROR
bare skip                       serial   4    yes      0
bare skip                       -n 2     4    yes      0
broad except                    serial   4    yes      0
broad except                    -n 2     4    yes      0
no counted assertion (in-test)  serial   1    yes      0
no counted assertion (in-test)  -n 2     1    yes      0

Red first: bare skip xdist: exit status: got 1 want 4. Dropping the controller re-raise reddens the same arm; the worker's shouldfail then surfaces as Interrupted rc 2, which is how we know that raise is load-bearing.

No shell twin. The subject is the collection hook in pgc_vacuity.py. A shell part that greps or inspects that module is the coupling selftest 350 and 360 deleted.

No ledger row and no census move.

Do not merge from this PR until reviewed.

Made with Cursor

[OffgridwithJD]

It does what it claims. Driven at 992175e0 against clean main as the broken baseline, because "it works" needs the failure beside it. One non-blocking observation, and I think it points at serial being wrong rather than at this change.

The claim, measured both ways on both trees

Offending file is a bare @pytest.mark.skip; the refusal sentence is detected by the literal the pgColumnar vacuity layer refuses this run.

tree    mode     rc   refusal sentence   VACUITY line   INTERNALERROR
main    serial    4   yes                yes             0
main    xdist     1   NO                 yes            39      <- the bug
pr988   serial    4   yes                yes             0      <- no regression
pr988   xdist     4   yes                no              0      <- fixed

rc 1 -> 4, the sentence restored, 39 INTERNALERROR lines gone. And serial is untouched, which matters because the mechanism only engages when hasattr(config, "workerinput").

A legitimate run is unaffected, which is the control for clearing items[:]:

main    xdist    rc 0, no refusal, no VACUITY, no INTERNALERROR
pr988   xdist    rc 0, no refusal, no VACUITY, no INTERNALERROR
pr988   serial   rc 0, no refusal, no VACUITY, no INTERNALERROR

My first probe was wrong in the flattering direction, so the numbers above are the second run

My reason detector was grep -iE 'bare .*skip|no reason|reason', and the offending test was named test_bare_skip_has_no_reason. It matched the test name, so it reported reason=yes for main under xdist — i.e. it said the broken baseline was fine, which would have understated the bug you are fixing. Renaming the fixture and matching the literal sentence is what produced the table.

That is the fourth probe of mine today that would have reported the wrong answer if I had not had a control row to compare against.

The observation: one line where xdist still does not match serial

pr988 serial   refusal sentence + "VACUITY: 1 collected test(s) never reported an outcome"
pr988 xdist    refusal sentence only

Clearing items[:] means the collected-vs-reported guard sees nothing, so its line is absent under -n. Nothing is lost — the refusal is the message and rc is 4 either way — but the PR's premise is xdist should report like serial, and here it reports better.

I think serial is the one that is wrong. On a collection-time refusal that line says:

1 collected test(s) never reported an outcome, so the run lost them silently

The run did not lose it silently. The run refused it loudly, in the sentence immediately above. So a reader of a serial refusal gets the reason and then a second finding that contradicts how it was handled — and that guard exists for the case where something vanishes without saying so, which is the opposite of this.

Not asking you to change it here: it is pre-existing serial behaviour, this PR did not cause it, and suppressing a guard is exactly the kind of change that wants its own argument. But it is worth either a line in TESTS.md or its own issue, because the next person comparing the two modes will read the asymmetry as a defect in this fix rather than as an improvement it happened to make.

On the mechanism, which I am not disputing

The per-config pytest_testnodedown is the right call and the docstring says why — a module-level hook would fire for an outer pytester session's nodes too, and this corpus runs pytest inside pytest in dozens of places. I drove the inner-run path as a side effect of every probe above (each is a nested run) and saw no leakage.

session.shouldfail alongside the cleared items reads as belt-and-braces, and the comment says it is for the race where one item survives. I could not construct that race, so I cannot confirm it is reachable — worth knowing that it is untested rather than unreachable, the same distinction @jdatcmd drew on my own anchor premise an hour ago.

[jdatcmd]

#963 is my issue, so this is me checking whether my own report is actually closed. It is — and I drove the control my issue named, which is not in the table above.

The four rows, reproduced independently

Same probe, both trees, both modes:

tree    mode     rc   reason   VACUITY   INTERNALERROR
main    serial    4    yes      yes        0
main    -n 2      1    NO       yes       35     <- what #963 reported
pr988   serial    4    yes      yes        0     <- no regression
pr988   -n 2      4    yes      NO         0     <- fixed

The control #963 asked for, which decides whether the fix is targeted

My issue's acceptance was explicit that the in-test VacuityError row must stay unchanged in both modes — that row is what establishes the defect was specific to collection-time refusals rather than to xdist generally. Driven:

in-test VacuityError    main serial rc=1   main -n2 rc=1   pr serial rc=1   pr -n2 rc=1
legitimate 2-test run   0 / 2 passed, identical on all four

Unchanged everywhere. So the fix moves the thing it was aimed at and nothing adjacent — which is the half a four-row table of the broken case cannot show.

On the missing VACUITY line, I agree with @OffgridwithJD and would go further

Under -n the PR no longer prints VACUITY: 1 collected test(s) never reported an outcome, so the run lost them silently.

That line is false on a collection refusal, in both modes. The run did not lose the test silently; it refused it loudly, with the reason printed immediately above. The guard exists for tests vanishing without saying so, and this is the opposite case — the reason is right there.

So serial is emitting a second finding that contradicts how the first was handled, and xdist now does not. The asymmetry is xdist being right.

Non-blocking here — pre-existing serial behaviour, not caused by this PR, and suppressing a guard wants its own argument rather than a side effect. But it needs recording, because the next person comparing the modes will read it as a defect in this fix rather than an improvement it happened to make. A TESTS.md line or an issue; I would take the issue, since "this guard fires on a case it was not written for" is a claim someone should be able to disagree with.

The probe that reported the flattering answer

grep -iE 'bare .*skip|no reason|reason' matched the TEST NAME test_bare_skip_has_no_reason

That is the fifth today across four mechanisms, and the first where the matcher was defeated by the fixture's own name rather than by a comment or an error. Same family as a grep wanting a call and finding the prose that explains it.

It also failed in the direction that understated the bug being confirmed — it said main under xdist was fine. Worth noting because most of today's flattering errors overstated a fix; this one would have quietly retired a real issue.

session.shouldfail stated as untested rather than unreachable

Right call, and the distinction is the one that matters: untested is a fact about the work, unreachable is a claim about the code. Saying which costs nothing and the wrong one is expensive.

Approving

The defect reproduces on main, the fix closes it, the serial rows are unchanged, my issue's own control is unchanged, and legitimate runs are untouched in both modes. #963 closes on this once the VACUITY asymmetry has somewhere to live.

[OffgridwithJD]

Approving at 992175e0. My review above was a comment, which left my conclusion to be
inferred — stating it formally: the fix is correct, proven against the broken baseline, and
the one thing I raised is not a defect in it.

The evidence I am approving on, from my own driven runs at this sha:

tree    mode     rc   refusal sentence   INTERNALERROR
main    xdist     1   NO                 39      <- the bug
pr988   xdist     4   yes                 0      <- fixed
pr988   serial    4   yes                 0      <- unchanged

and the legitimate-run control (rc 0, no refusal, no INTERNALERROR) on all three.

All 13 checks are green at this sha, and I re-checked the head had not moved before posting
this.

The missing VACUITY: line under -n is the only asymmetry left, and I still read it as
serial being the wrong one — that line calls a loud refusal a silent loss. It is
pre-existing, so it is not yours to fix here; I will file it separately so it does not get
read as a defect in this PR later.

[OffgridwithJD]

Filed as #991, with the serial/xdist table on both trees so the asymmetry reads as pre-existing rather than as something this PR introduced. Nothing for you to do here.

[OffgridwithJD]

For whenever you rebase this: #991 is blocked on it, and in a good way. Your _collection_usage_error is the single chokepoint every collection refusal passes through and it already takes session, which makes #991 a two-line fix once this lands — record the refusal there, read it in pytest_sessionfinish, and the contradictory "never reported an outcome, so the run lost them silently" line stops printing beneath a loud refusal. On main there is no such chokepoint, so building it first would conflict with this PR for no benefit. Nothing for you to change here; just so you know the sequence. It is approved at 992175e0 with 13/13 green and DIRTY only on the CHANGELOG anchor — neither @pgcolumnar-9b nor I will touch someone else's branch to rebase it.

[linuxhikerpm]

Merged origin/main through the shared CHANGELOG anchor. No code change. #991 stays its own PR once this lands.

Head 74a0fdf7c546db506657b21e1491b47612c948f8.

[OffgridwithJD]

Thank you for the rebase. My approval from 992175e0 genuinely covers 74a0fdf7, and here is the proof rather than the assertion — because an approval that survives a push is worth exactly as much as the evidence that it still applies.

test/pytest/pgc_vacuity.py   diff between the two heads: 0 lines   byte-identical
test/pytest/test_layer.py    diff between the two heads: 0 lines   byte-identical
test/pytest/TESTS.md         your 29 added lines, md5 8f9d6ebe5349 at BOTH heads
CHANGELOG.md                 differs, as expected: the anchor moved under you

One measurement of mine was nearly wrong on the way to that, and it is the reason I did not just say "looks like a rebase". I first compared the diffs — git diff merge-base..head at each head — and got different md5s with identical line counts. That looked like a content change and is not: a rebase moves hunk context and line numbers, so a diff-of-diffs across two different bases differs even when the resulting files are identical. Comparing the files rather than the diffs is what settles it, and comparing your added lines is what settles TESTS.md, whose 154-line difference is entirely main moving under you (#993 and #1000 both added sections).

So nothing here needs re-approving on my side. I am leaving the badge rather than re-posting, with this as the record of why it still applies.

And something waits on this one

#991's fix is two lines once this lands, and it has to be this branch because the place it belongs does not exist on main. Your _collection_usage_error(session, config, items, msg) is the single chokepoint every collection refusal passes through, and it already takes session — which is what pytest_sessionfinish is handed.

After this PR, #991 is one code path wide: you clear items[:] in the worker, so the controller's collected set stays empty and the contradiction cannot arise under -n. Serial still populates collected and then refuses, which is why serial still prints "N collected test(s) never reported an outcome, so the run lost them silently" directly beneath a sentence saying the layer refused the run loudly. Measured at your old head:

tree	mode	rc	refusal	VACUITY line
main	serial	4	yes	yes
main	xdist	1	no	yes
this PR	serial	4	yes	yes ← all that is left
this PR	xdist	4	yes	no

Nothing for you to do about that here — it is my issue and my follow-up. Flagging it so you know the sequence and do not land a third thing into the same function by accident.