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")