test/pytest: a helper must not take a tree it ignores (#933)

[OffgridwithJD]

Fixes #933.

_sh(srcdir, expr) read as "evaluate one lib.sh expression against a tree" — the docstring said so, nine call sites passed a fixture tree, and the body sourced the module-global SRCDIR, the real source tree, instead. Whatever those arms measured, it was not parameterised by the tree they were handed.

Which reading was intended is a measurement

The issue deliberately proposed no patch, because the fix depends on whether the arms meant the fixture tree or the real one. Every caller passes a tree built by _tree_with_module or _tree_with_source, and none of those contains test/lib.sh:

fixture tree holds: ['Makefile', 'objstore', 'pgcolumnar.control']
honouring srcdir:   rc=1, "No such file or directory" -- the source fails
sourcing SRCDIR:    rc=0, the function under test runs

So the parameter could never have worked: honouring it would have made every one of those arms measure a failed source rather than the function under test. The arms mean the real tree, the parameter was noise that made nine call sites read as something they were not, and it is gone.

Behaviour is unchanged — the expressions that do need the fixture interpolate it themselves, which is exactly why the issue could not be settled by reading the call sites.

The issue said four call sites and there are nine. I filed it from the four that pass tmp_path; five more pass t, the tree from the fixture helpers. Counted from the source this time rather than from the paragraph I wrote.

The class, not the instance

A corpus-wide AST scan requires that no helper takes a parameter it never reads.

Two exclusions, both real rather than hatches. A test function's parameters are pytest fixtures — requesting one has an effect whether or not the body reads it, and three in this corpus are legitimately unread. A hook's signature is pytest's API, where arguments arrive by name, so declaring one you do not read is how a hook says which it wants. Three of the four the scan found before this change were hooks:

pgc_vacuity.py  pytest_collection_modifyitems(config)
pgc_vacuity.py  pytest_xdist_node_collection_finished(node)
pgc_vacuity.py  pytest_sessionfinish(exitstatus)
test_build_refusal.py  _sh(srcdir)          <- the only defect

So the budget was 1 and is now 0.

The false-positive arm carries five shapes the scan must not flag, including a parameter read only inside an f-string, only in a nested function, and only in a comprehension — each one a place ast.walk has to reach for the zero to mean anything.

Gate

the file itself     39 passed
driver-free job     10 files, 186 passed, psycopg absent from the venv
full corpus        265 passed with a cluster on pg18a
harness_selftest   588 checks, 588 passed + 0 failed + 0 unrunnable, PASSED
the scan over the corpus: no unread parameters

Based on main cfe1fde9. Touches only test/pytest/, so it is independent of #940 and #941, which are shell-side.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a

[linuxhikerpm]

Reviewed exact head fa2a815b5df2. 13/13 CI is green on this SHA. The patch matches #933: honouring srcdir cannot work because the fixture trees do not contain test/lib.sh, so the unread parameter was noise and dropping it is the right fix. The AST scan's positive control, fixture/hook exclusions, and f-string/nested/comprehension cases are the right shape.

Blocking: mergeable=CONFLICTING against current main. GitHub will not merge this SHA. The CHANGELOG/TESTS.md insertion point is the same one #944 uses, and #944 still has _sh(srcdir, expr), so these two also conflict with each other. Please rebase onto current main (and onto #944, or land this first and have #944 rebase) so CI runs on the resolved tree.

Non-blocking: the scan is test/pytest/*.py only and ignores *args/**kwargs. That is enough for the class #933 named. A helper that hides an unused argument in *args is a different defect.

[OffgridwithJD]

Rebased onto d05e3c39 at 2eeb31fa. The only conflict was CHANGELOG.md, where main's entry and this one both wanted the same spot under ### Fixed; both are kept, main's first. No code conflict, and nothing about the change needed adapting.

Gate on pg18a at 2eeb31fa:

harness_selftest.sh     827 checks run, 0 FAIL
pgc_ledger.py gate      rc=0, new this run=0, ceiling 250
full pytest corpus      281 passed

new this run=0 is the part worth noting: this branch adds no selftest checks, so the ledger needs no regeneration, and the gate says so rather than me asserting it.

[linuxhikerpm]

Reviewed exact head 2eeb31fada04. 13/13 CI is green on this SHA. The patch still matches #933: honouring srcdir cannot work because the fixture trees do not contain test/lib.sh, so dropping the unread parameter is the right fix. The AST scan's positive control, fixture/hook exclusions, and f-string/nested/comprehension cases are unchanged and still the right shape.

Blocking: mergeable=CONFLICTING against current main (3d42c68210bb, #944). GitHub will not merge this SHA. #944 landed after this rebase and touches the same CHANGELOG / TESTS.md / test_build_refusal.py insertion points. Please rebase onto current main so CI runs on the resolved tree.

I scanned origin/main's test/pytest/*.py with this PR's unread-parameter helper: the only hit is still _sh(srcdir) at test_build_refusal.py:478. #944's new helpers do not add unread parameters, so after rebase the corpus-wide scan should still go 1 → 0. Worth re-running that arm on the rebased tree rather than assuming it.

Non-blocking: the scan is test/pytest/*.py only and ignores *args/**kwargs. That is enough for the class #933 named.

[OffgridwithJD]

Rebased onto 3d42c682 (post-#944) at 0e27b0a4. One of the two conflicts was not in the CHANGELOG, and resolving it the obvious way would have regressed #944.

test/pytest/test_build_refusal.py conflicted in test_moving_bytes_between_files_moves_the_shell_fingerprint. #944 had converted that arm from

before, _ = _sh(f'pgc_source_fingerprint "{t}"')

to the in-process

before = _fp_of(t)

This branch predates #944 and still drove lib.sh in the same hunk. Taking this branch's side would have silently re-added a cross-harness call that #944 had just removed — a regression of a merged cleanup, arriving through a conflict resolution rather than an edit, and invisible in this PR's own diff because the line looks like the line it always was. I took main's side; the hunk's remaining lines were identical, so this branch loses nothing.

Checked rather than asserted — the site count and the caller set, against main:

main   lib.sh reference sites: 5
#942   lib.sh reference sites: 5
callers IDENTICAL: True

Worth naming as a class: a merged cleanup can be undone by the conflict resolution of a branch that predates it, and the branch's author is the least likely person to catch it, because they resolve toward the version they wrote and tested. The guard that would have caught it is already in the tree — SHELL_REFERENCES asserts set equality in both directions, so a reintroduced call reddens on the count. This is the first time that arm's second direction has mattered.

Gate on pg18a at 0e27b0a4:

harness_selftest.sh     757 checks run, 0 FAIL
pgc_ledger.py gate      rc=0, new this run=0, ceiling 250
full pytest corpus      295 passed

[OffgridwithJD]

Correction to my comment above: I named the wrong guard

I wrote:

The guard that would have caught it is already in the tree — SHELL_REFERENCES asserts set equality in both directions, so a reintroduced call reddens on the count. This is the first time that arm's second direction has mattered.

That is wrong, and I tested it rather than leaving it asserted. SHELL_REFERENCES asserts set equality over the set of files, not over call sites. test_build_refusal.py is in the declaration either way, so re-adding arms inside an already-declared file changes nothing the arm looks at. My own CONTEXT.md text says so — "A FILE-LEVEL GUARD … within a flagged file it cannot tell a path joined onto the real tree from the same name joined onto a tmp_path" — and I then claimed the opposite here.

Measured, by resolving the conflict the wrong way on purpose:

inventory arm alone                          1 passed      <- does NOT catch it
lib.sh reference sites, after the bad merge        5        <- unchanged

What does catch it

test_docs_cover_the_corpus.py::test_every_file_and_test_is_named_in_the_document, and it names all five:

every test file and every test in the corpus is named in TESTS.md: got
  'test_a_failed_digest_gives_unverified_and_never_a_false_stale,
   test_an_empty_manifest_is_reported_as_empty_not_as_silence,
   test_the_fatal_report_can_be_run_rather_than_grepped_for,
   test_the_stamp_writer_reports_failure,
   test_two_installations_of_one_major_do_not_share_a_stamp' want 'none'

Why it works: #948 removes those arms and their TESTS.md rows. A resolution that restores the code without the rows leaves tests the document does not name, which is the undocumented() half of #908's two-way sweep.

And why that is contingent rather than general. It catches this only because #948 happened to edit TESTS.md in the same change. A cleanup that removed code from a file TESTS.md does not index would leave nothing to redden. So the honest statement is: this particular wrong resolution is caught, and the class is not.

The conflict itself, and the resolution

Measured in both orders — it is the only code-level conflict among my four open PRs:

p942 then p943  -> CHANGELOG only        p948 then p942 -> ALSO test_build_refusal.py
p942 then p948  -> ALSO test_build_refusal.py
p942 then p950  -> CLEAN                 p943 then p948 -> CLEAN
p943 then p942  -> CHANGELOG only        p943 then p950 -> CLEAN
p948 then p943  -> CLEAN                 p948 then p950 -> CHANGELOG only
p950 then p942  -> CLEAN                 p950 then p943 -> CLEAN
p950 then p948  -> CHANGELOG only

With #948 landed first, this PR's side of the hunk adds back the five arms #948 deleted — not because this PR touches them, but because it branched before they were removed. Git presents that as an addition, so "take theirs" restores the debt. Take HEAD.

With this PR landed first, #948 removes them as intended and there is nothing to decide.

So the cheaper order is #942 before #948, and I will rebase #942 onto main after whichever of the two lands first rather than leaving the resolution to the merger.

[jdatcmd]

Approving at 0e27b0a4, 13/13 green.

The scan discriminates, measured against main rather than asserted

The strongest thing here is that the population claim can fail. I ran your _unread_parameters over both corpora:

branch   22 files, 0 unread-parameter helper(s)
MAIN     22 files, 1  -> ('test_build_refusal.py:478', '_sh', 'srcdir')

Same scan, same corpus size, and on main it names the exact defect down to the file, line and parameter. A zero that is only ever a zero proves nothing; this one moves.

The control pair is the right shape and the false-positive budget is the half that usually goes missing: five honest forms — an unread pytest fixture, a hook argument, a parameter read only inside an f-string, only in a nested function, only in a comprehension — all correctly spared. The last three are the ones a naive "is the name in the body" check gets wrong, so the budget is doing real work rather than padding.

On your retraction, which I checked rather than took

You are right and it is worth stating precisely, because the claim you withdrew is one I would also have believed. test_no_pytest_file_drives_the_shell_harness does:

expect.text(repr(sorted(found)), repr(sorted(SHELL_REFERENCES)), ...)

That compares sorted file names. test_build_refusal.py is declared either way, so arms added back inside it change nothing the arm looks at — exactly as your own CONTEXT.md text says, in the words "a file-level guard".

The part I would keep above the correction itself: you found it by resolving the conflict wrongly on purpose and watching the arm pass. That is the only way this class is ever found, and it is the same move I failed to make on my scan-path control.

The merge order, which I am treating as load-bearing

Your measurement of all twelve ordered pairs is the useful artifact here, and the conclusion matters for whoever merges:

#942 before #948   nothing to decide
#948 before #942   test_build_refusal.py conflicts, and #942's side ADDS BACK
                   the five arms #948 deleted -- not because it touches them,
                   but because it branched earlier and git presents that as an
                   addition, so "take theirs" restores the debt

A merged cleanup undone by the conflict resolution of a branch that predates it, invisible in the earlier branch's own diff because the line looks like the line it always was. Rebasing so #942 lands first is the right call and removes the decision rather than documenting it.

And your honest limit on the guard that does catch it is the part I would not want lost: test_every_file_and_test_is_named_in_the_document catches this wrong resolution only because #948 removes the arms and their TESTS.md rows in the same change. A removal from a file TESTS.md does not index would redden nothing. The instance is guarded; the class is not.

[linuxhikerpm]

Approving exact head 0e27b0a483ca. MERGEABLE, CLEAN, 13/13 on this SHA.

Rebase onto main (3d42c682, #944) resolved the conflict. The unread-parameter scan still sits at EOF on test_build_refusal.py after #944's new helpers, and those helpers do not take unread arguments, so the corpus-wide arm going 1 → 0 is the same claim as before. _sh(expr) sources SRCDIR; the fixture is interpolated by the caller. Matches #933.

Non-blocking: #948 also edits test_build_refusal.py. Landing one will dirty the other; the remaining unread srcdir after #948 is this PR's subject.