From 0e27b0a483ca7226bd68bacf1a92f7cfe37076f6 Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Thu, 10 Sep 2026 23:07:07 +0000 Subject: [PATCH] test/pytest: a helper must not take a tree it ignores (#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, not a judgement, and it decides the fix. 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` instead of the function. The arms mean the real tree, the parameter was noise, and it is gone. Behaviour is unchanged -- the expressions that DO need the fixture interpolate it themselves, which is why the issue could not be decided by reading the call sites alone. 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 of them it wants. Three of the four the scan found before this change were hooks: `pytest_collection_modifyitems(config)`, `pytest_xdist_node_collection_finished(node)` and `pytest_sessionfinish(exitstatus)`. Only `_sh` was a defect, so the budget was 1 and is now 0. The false-positive arm carries five shapes it 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 Fixes #933. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- CHANGELOG.md | 29 +++++++ test/pytest/TESTS.md | 40 ++++++++++ test/pytest/test_build_refusal.py | 122 +++++++++++++++++++++++++++--- 3 files changed, 182 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2e000528..593ac487 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -701,6 +701,35 @@ true until the next version shipped. deleted fourth; the end state is one. which never started. +- A test helper no longer takes a tree it ignores (#933). + + `_sh(srcdir, expr)` in `test/pytest/test_build_refusal.py` 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. Every caller passes a tree built by + `_tree_with_module` or `_tree_with_source`, and none 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, so the parameter was noise that made nine call sites read as something + they were not. It is gone, and behaviour is unchanged: the expressions that need the + fixture interpolate it themselves. + + A corpus-wide AST scan now requires that no helper takes a parameter it never reads, so + the class is closed rather than the instance. Two exclusions, both real: a TEST + function's parameters are pytest fixtures, and requesting one has an effect whether or + not the body reads it; and 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 were hooks -- `pytest_collection_modifyitems(config)`, + `pytest_xdist_node_collection_finished(node)`, `pytest_sessionfinish(exitstatus)` -- so + only `_sh` was a defect and the budget was 1, now 0. - The vacuity guard's PLACEMENT is now a checked property, because a guard in a teardown cannot fail the test it guards (#432). diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 5b187784..bbf7d845 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -764,6 +764,46 @@ a plausible wrong hash that neither the per-file sentinel nor the empty-manifest guard can see. It has not been observed. The boundary of this change is "the three observed variants are closed", not "the function is now infallible". +### A helper that took a tree and ignored it + +`_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 (#933). + +**Which reading was intended is a measurement, not a judgement.** 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. The arms mean the real +tree, the parameter was noise, and it is gone. The expressions that DO need the fixture +interpolate it themselves, which is why dropping it changes no behaviour. + +| test | asserts | +| --- | --- | +| `test_the_unread_parameter_scan_finds_one` | the scan fires on the shape `_sh` had | +| `test_the_unread_parameter_scan_spares_fixtures_and_hooks` | five shapes it must not flag | +| `test_no_helper_in_this_corpus_takes_a_parameter_it_never_reads` | the population, corpus-wide: one before, none now | + +**The class, not the instance.** The scan is corpus-wide because the defect was here and +the class is not. + +**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 — 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: +`pytest_collection_modifyitems(config)`, `pytest_xdist_node_collection_finished(node)` +and `pytest_sessionfinish(exitstatus)`. Only `_sh` was a defect, so the budget was 1 and +is now 0. + ## 6. test_docs_cover_the_corpus.py: this document, checked **THE SWEEP GOES BOTH WAYS NOW (#908).** `undocumented()` computes tests on disk diff --git a/test/pytest/test_build_refusal.py b/test/pytest/test_build_refusal.py index c8f97b1d..cdb0f872 100644 --- a/test/pytest/test_build_refusal.py +++ b/test/pytest/test_build_refusal.py @@ -475,8 +475,20 @@ def test_this_module_keeps_no_private_fingerprint(expect): SRCDIR = pathlib.Path(__file__).resolve().parents[2] -def _sh(srcdir, expr): - """Evaluate one lib.sh expression against a tree, and return its stdout.""" +def _sh(expr): + """Evaluate one `lib.sh` expression against THIS tree, and return its stdout. + + THE TREE IS NOT A PARAMETER, and it used to look like one. This took `srcdir`, + every caller passed a fixture tree, and the body sourced the module-global + `SRCDIR` regardless -- so nine call sites read as "evaluate against this tree" + while the tree was discarded (#933). + + It could not have been otherwise: the functions under test live in `lib.sh`, and + no fixture tree contains one. Measured -- honouring the parameter gives rc=1 and + `No such file or directory`, so every arm would have measured a failed `source` + instead of the function. The expressions that DO need the fixture interpolate it + themselves, which is why dropping the parameter changes no behaviour. + """ script = f'. "{SRCDIR}/test/lib.sh" || exit 1; {expr}' p = subprocess.run(["bash", "-c", script], capture_output=True, text=True) return p.stdout.strip(), p.returncode @@ -560,9 +572,9 @@ def _tree_with_module(tmp_path, name): def test_the_stamp_writer_reports_failure(tmp_path, expect): """`|| true` made both controllers' warning branches unreachable.""" - out, rc = _sh(tmp_path, 'pgc_write_source_stamp "/proc/pgc-twin" "deadbeef"') + out, rc = _sh('pgc_write_source_stamp "/proc/pgc-twin" "deadbeef"') expect.num(rc, 1, "the writer reports failure on an unwritable target") - ok, rc2 = _sh(tmp_path, f'pgc_write_source_stamp "{tmp_path}/s" "cafebabe"') + ok, rc2 = _sh(f'pgc_write_source_stamp "{tmp_path}/s" "cafebabe"') expect.num(rc2, 0, "control: and succeeds on a writable one") @@ -576,10 +588,10 @@ def test_two_installations_of_one_major_do_not_share_a_stamp(tmp_path, expect): f' --pkglibdir) echo "/usr/local/pg18{n}/lib" ;;\nesac\n') c.chmod(0o755) cfgs.append(c) - a, _ = _sh(tmp_path, f'pgc_source_stamp_path /tree "{cfgs[0]}"') - b, _ = _sh(tmp_path, f'pgc_source_stamp_path /tree "{cfgs[1]}"') + a, _ = _sh(f'pgc_source_stamp_path /tree "{cfgs[0]}"') + b, _ = _sh(f'pgc_source_stamp_path /tree "{cfgs[1]}"') expect.text(str(a != b), "True", "two prefixes of one major get different stamps") - a2, _ = _sh(tmp_path, f'pgc_source_stamp_path /tree "{cfgs[0]}"') + a2, _ = _sh(f'pgc_source_stamp_path /tree "{cfgs[0]}"') expect.text(a2, a, "control: the same pg_config twice gives the same path") @@ -627,11 +639,11 @@ def test_the_two_fingerprint_implementations_cover_the_same_inputs(tmp_path, exp ("the top-level Makefile", t / "Makefile", "all:\n\t$(MAKE) -C objstore # x\n"), ("the control file", t / "pgcolumnar.control", "y\n"), ): - sh_before, _ = _sh(t, f'pgc_source_fingerprint "{t}"') + sh_before, _ = _sh(f'pgc_source_fingerprint "{t}"') py_before = source_fingerprint(t) old = path.read_text() path.write_text(body) - sh_after, _ = _sh(t, f'pgc_source_fingerprint "{t}"') + sh_after, _ = _sh(f'pgc_source_fingerprint "{t}"') py_after = source_fingerprint(t) path.write_text(old) expect.text(f"{sh_after != sh_before} {py_after != py_before}", "True True", @@ -1134,3 +1146,95 @@ def test_a_symlinked_src_is_skipped_like_any_other_symlinked_build_dir(tmp_path, out2 = _mf_of(real_tree) expect.num(len([l for l in out2.splitlines() if l.startswith("src/a.c ")]), 1, "control: a real src directory is still hashed") + +# ---- a helper may not take a parameter it never reads (#933) ------------------- +# +# `_sh(srcdir, expr)` took a tree and ignored it: the docstring said "evaluate one +# lib.sh expression against a tree", nine call sites passed a fixture tree, and the +# body sourced the module-global SRCDIR -- the real source tree -- instead. +# +# WHICH READING WAS INTENDED IS A MEASUREMENT, not a judgement, and it settles the +# fix. 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, and the arms mean the real tree. The +# parameter was noise that made nine call sites read as something they were not. +# +# TEST FUNCTIONS ARE EXCLUDED, because their parameters are pytest FIXTURES: +# requesting one has an effect whether or not the body reads it, and three in this +# corpus are legitimately unread. +# +# PYTEST HOOKS ARE EXCLUDED TOO, and that is not a hatch. A hook's signature IS +# pytest's API -- arguments arrive by NAME -- so declaring a parameter you do not read +# is how a hook says which of them it wants. Three of the four the scan found before +# this change were hooks: `pytest_collection_modifyitems(config)`, +# `pytest_xdist_node_collection_finished(node)` and `pytest_sessionfinish(exitstatus)`. +# Only `_sh` was a defect, so the budget was 1 and is now 0. + + +def _unread_parameters(source, filename=""): + """-> [(function, parameter, line)] for helpers that ignore an argument.""" + import ast + + out = [] + for fn in ast.walk(ast.parse(source)): + if not isinstance(fn, (ast.FunctionDef, ast.AsyncFunctionDef)): + continue + if fn.name.startswith("test_") or fn.name.startswith("pytest_"): + continue + names = [a.arg for a in fn.args.posonlyargs + fn.args.args + fn.args.kwonlyargs] + names = [n for n in names if n not in ("self", "cls")] + if not names: + continue + read = {n.id for n in ast.walk(fn) + if isinstance(n, ast.Name) and isinstance(n.ctx, ast.Load)} + for n in names: + if n not in read: + out.append((f"{filename}:{fn.lineno}", fn.name, n)) + return out + + +def test_the_unread_parameter_scan_finds_one(expect): + """The positive control, on the exact shape `_sh` had.""" + found = _unread_parameters( + "def _helper(srcdir, expr):\n" + " return run(GLOBAL, expr)\n", "probe.py") + expect.num(len(found), 1, "a helper ignoring its first argument is found") + expect.text(found[0][2], "srcdir", "and the parameter is named") + + +def test_the_unread_parameter_scan_spares_fixtures_and_hooks(expect): + """The false-positive budget, which is what makes the zero below mean something.""" + for label, src in ( + ("a test function's unread fixture", + "def test_x(tmp_path, expect):\n expect.num(1, 1, 'n')\n"), + ("a pytest hook's unread argument", + "def pytest_sessionfinish(session, exitstatus):\n do(session)\n"), + ("a parameter read only inside an f-string", + "def _h(tree):\n return f'{tree}/x'\n"), + ("a parameter read only in a nested function", + "def _h(tree):\n def inner():\n return tree\n return inner\n"), + ("a parameter read only in a comprehension", + "def _h(items):\n return [i for i in items]\n"), + ): + expect.num(len(_unread_parameters(src)), 0, f"not flagged: {label}") + + +def test_no_helper_in_this_corpus_takes_a_parameter_it_never_reads(expect): + """The population. One before this change -- `_sh(srcdir, expr)` -- and none now. + + Corpus-wide rather than this file only: the defect was here, but the class is not. + """ + import pathlib + + here = pathlib.Path(__file__).parent + files = sorted(here.glob("*.py")) + expect.at_least(len(files), 10, "premise: the scan has a corpus to read") + found = [] + for f in files: + found += _unread_parameters(f.read_text(encoding="utf-8"), f.name) + expect.text(repr(found), "[]", "no helper ignores an argument it was handed")