From 668756a1e7320a251d8f15d27839dcb713a4bdff Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Thu, 10 Sep 2026 01:57:28 +0000 Subject: [PATCH 1/6] test: the harness guards run in the gate, without a database (#432) 152 tests in test/pytest/ and NOT ONE of them ran in CI. selftest 350 says it plainly about its own subject: a guard that does not run is a comment. This makes the guard-testing half of the corpus run in the gate, and the change that allows it is one import. ONE EAGER IMPORT COUPLED THE WHOLE CORPUS TO A DATABASE DRIVER --------------------------------------------------------------- `conftest.py` imported psycopg at module scope. conftest is imported before every run, so a DATABASE DRIVER was a hard requirement of every test -- including the 61 that never open a connection. With psycopg absent the run did not fail a test, it failed to COLLECT: ImportError while loading conftest '.../conftest.py' conftest.py:15: in import psycopg E ModuleNotFoundError: No module named 'psycopg' Measured, both directions, on the corpus as it stands: import at module scope 0 of 142 tests run with no driver installed deferred into fixtures 61 of 142 run and pass The layer itself never needed it: pgc_vacuity.py imports ast, numbers, pathlib and pytest. pgc_cluster.py imports no driver either. It was conftest alone. README.md records why the corpus is not in `SUITES`: pgc_skip treats a missing dependency as a failure rather than a skip, so registering it would redden every job until the driver is installed everywhere. THAT ARGUMENT IS ABOUT THE CLUSTER TESTS. It never applied to the guard tests -- and until this import moved there was no way to separate them, because importing conftest imported the driver. THE JOB -------- `.github/workflows/ci.yml` gains `pytest-guards`: ubuntu-latest, no database, no build, an interpreter and two pinned packages. Measured: 61 tests, ~3 seconds. THE FILE LIST AND THE PINS ARE BOTH DERIVED. The list comes from `NO_CLUSTER` in test_harness_deps.py and the pins from requirements-test.txt, because a second copy of either is a hand-maintained value that goes stale silently -- which this repository has spent a day proving, on TESTS.md's totals line (#908). Two arms hold the job to that: it must derive the list, and it must name no corpus file literally. The job also asserts psycopg is ABSENT before running. Without that, the tests would pass for the ordinary reason and prove nothing about the coupling. A SHIM RATHER THAN AN UNINSTALL -------------------------------- test_harness_deps.py proves the property behaviourally: it writes a `psycopg.py` that raises on import, puts it FIRST on the path, and requires the no-cluster files to collect and pass anyway. Uninstalling the driver would test the machine rather than the harness, could not run beside anything else, and would leave the environment broken if the test died. Both behavioural arms assert the shim actually bites before believing anything it produces. THE CONTROL IS THE HALF THAT MATTERS. Deferring must make the IMPORT lazy, not the database optional, so a cluster test must STILL fail with the driver gone, and fail naming the shim rather than by being quietly skipped. the guard half, driver shimmed out 61 passed, exit 0 a cluster test, driver shimmed out fails, naming the shim the CI job simulated end to end 61 passed, exit 0, psycopg absent BOTH HARNESSES, per jd's rule. selftest 350 carries the static half -- conftest imports no driver at module scope, the driver IS still imported inside the fixtures that connect (or the first arm is satisfied by a harness that talks to no database at all), and the job derives rather than repeats. harness_selftest 450 passed + 0 failed + 0 unrunnable, rc=0 pytest corpus 154 passed, serial and under -n 4 docs_style 9 checks PASSED shellcheck -S error clean Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- .github/workflows/ci.yml | 45 +++++ test/pytest/TESTS.md | 51 +++++- test/pytest/conftest.py | 17 +- test/pytest/test_harness_deps.py | 170 ++++++++++++++++++ .../selftest/350-the-pytest-corpus-must-be.sh | 56 ++++++ 5 files changed, 332 insertions(+), 7 deletions(-) create mode 100644 test/pytest/test_harness_deps.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f2676c4f..8d91a3da 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -63,6 +63,51 @@ jobs: - name: shellcheck -S error over the harness run: shellcheck -S error -s bash test/*.sh test/selftest/*.sh + # The pytest harness's own guards, where they can actually run. + # + # THIS RUNS NO DATABASE AND BUILDS NOTHING. test/pytest/ is 152 tests, and the + # majority of them test the HARNESS rather than the product -- they drive pytest + # inside pytest to prove a guard refuses what it claims to refuse. Those need an + # interpreter and nothing else. + # + # README.md records why the corpus is not in `SUITES`: pgc_skip treats a missing + # dependency as a failure rather than a skip, so registering it there would + # redden every job until the driver is installed everywhere. That argument is + # about the CLUSTER tests. It never applied to the guard tests, and until + # conftest deferred its psycopg import there was no way to separate them -- + # importing conftest imported the driver, so a run with no driver failed to + # COLLECT rather than failing a test. + # + # THE FILE LIST AND THE PINS ARE BOTH DERIVED, NOT WRITTEN HERE. The list comes + # from NO_CLUSTER in test_harness_deps.py and the pins from + # requirements-test.txt, because a second copy of either is a thing that goes + # stale silently -- which this repository has spent a lot of time proving. + pytest-guards: + name: pytest (harness guards, no database) + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - uses: actions/checkout@v4 + - name: install pytest, pinned from requirements-test.txt, WITHOUT the driver + run: | + set -euo pipefail + python3 -m venv /tmp/pgcvenv + # Only the two runner packages. psycopg is deliberately absent: these + # tests must not need it, and this job is what proves that. + PINS="$(grep -E '^(pytest|pytest-xdist)==' test/pytest/requirements-test.txt)" + test -n "$PINS" + echo "$PINS" + /tmp/pgcvenv/bin/pip install --quiet $PINS + ! /tmp/pgcvenv/bin/pip show psycopg >/dev/null 2>&1 + - name: run the guard tests that need no cluster + run: | + set -euo pipefail + cd test/pytest + FILES="$(python3 -c 'import sys; sys.path.insert(0, "."); from test_harness_deps import NO_CLUSTER; print(" ".join(NO_CLUSTER))')" + test -n "$FILES" + echo "running: $FILES" + PYTHONPATH=. /tmp/pgcvenv/bin/pytest -q $FILES + # Build against every supported major. Fast, and it is what an API change # between majors trips first. # Both architectures, because a build preflight is cheap and the project has diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index f759f5b3..f2ab4563 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -59,9 +59,10 @@ behaviour, the source of that number is named. - [11. test_zonemap_boundaries.py: exact boundaries](#11-test_zonemap_boundariespy-exact-boundaries) - [12. test_saop_element_pushdown.py: scattered set pruning](#12-test_saop_element_pushdownpy-scattered-set-pruning) - [13. test_hilbert_locality.py: what the Hilbert curve buys](#13-test_hilbert_localitypy-what-the-hilbert-curve-buys) -- [14. Adding a test](#14-adding-a-test) -- [15. What this corpus does NOT yet refuse](#15-what-this-corpus-does-not-yet-refuse) -- [16. Traps this corpus records](#16-traps-this-corpus-records) +- [14. test_harness_deps.py: the harness must self-test without a database](#14-testharnessdepspy-the-harness-must-self-test-without-a-database) +- [15. Adding a test](#15-adding-a-test) +- [16. What this corpus does NOT yet refuse](#16-what-this-corpus-does-not-yet-refuse) +- [17. Traps this corpus records](#17-traps-this-corpus-records) ## 1. How to read a test in here @@ -1012,7 +1013,45 @@ pins the eight counts exactly. Its header records why -- for a CURVE change the digest pins upstream catch it first and the integers add nothing, so their real domain is a changed READER at an unchanged layout. -## 14. Adding a test +## 14. test_harness_deps.py: the harness must self-test without a database + +`conftest.py` imported psycopg at module scope, and conftest is imported before +every run, so a **database driver was a hard requirement of the whole corpus** -- +including the 61 tests that never open a connection. With psycopg absent the run +did not fail a test, it failed to COLLECT: + + ImportError while loading conftest '.../conftest.py' + conftest.py:15: in + import psycopg + E ModuleNotFoundError: No module named 'psycopg' + +Measured: with the import deferred into the two fixtures that connect, **61 of +142 tests run and pass with no driver installed**; with it at module scope, zero +do. That coupling is half of why the guard-testing part of this corpus cannot run +where the gate runs (README.md, "This is not in the gate yet"). + +| test | asserts | +| --- | --- | +| `test_the_guard_half_of_the_corpus_runs_without_a_database_driver` | the no-cluster files collect and pass with `import psycopg` shimmed to raise | +| `test_a_cluster_test_still_needs_the_driver` | **control**: deferring made the IMPORT lazy, not the database optional | +| `test_conftest_imports_no_database_driver_at_module_scope` | the regression named in one line, for whoever edits conftest next | +| `test_the_no_cluster_list_still_names_files_that_exist` | the list is a hazard; it fails loudly rather than covering fewer files after a rename | +| `test_ci_derives_the_file_list_rather_than_repeating_it` | the CI job asks this module for `NO_CLUSTER` and names no file literally | +| `test_the_job_installs_no_database_driver` | the job asserts psycopg is absent rather than assuming it | + +**THIS IS NOW IN THE GATE.** `.github/workflows/ci.yml` runs a `pytest-guards` +job: no database, no build, an interpreter and the two pinned runner packages. +The file list is derived from `NO_CLUSTER` in this module and the pins from +`requirements-test.txt`, so neither is a second copy that can go stale -- and two +arms above hold it to that. Measured in the job: 61 tests, ~3 seconds. + +A SHIM RATHER THAN AN UNINSTALL. Uninstalling psycopg would test the machine +rather than the harness, could not run beside anything else, and would leave the +environment broken if the test died. A module that raises on import, first on the +path, is the same observation and reversible by construction. Both behavioural +arms assert the shim actually bites before believing anything it produces. + +## 15. Adding a test 0. **Write it twice.** Every test in this tree ships as a `.sh` suite and a pytest test **in the same change** (jd, 2026-09-09). Not ported later, not one or the @@ -1039,7 +1078,7 @@ domain is a changed READER at an unchanged layout. failed the selftest on both majors of the matrix, which is how it was found. A new directory under `test/` inherits every rule the old ones follow. -## 15. What this corpus does NOT yet refuse +## 16. What this corpus does NOT yet refuse `VACUITY_MODES.md` is the inventory: 79 ways a pytest harness can report a pass while asserting nothing, 73 of them demonstrated by an actual run. **This layer refuses 25 @@ -1051,7 +1090,7 @@ Read it before adding a test. The gaps most likely to affect a new test are that same family satisfies it, and that a write is not required to have written anything. Both are named there with the refusal each needs. -## 16. Traps this corpus records +## 17. Traps this corpus records Recorded because each one produced a confident wrong result before it was caught, and all are the same family as the defect the layer exists to prevent. diff --git a/test/pytest/conftest.py b/test/pytest/conftest.py index 987fafda..ef8e671b 100644 --- a/test/pytest/conftest.py +++ b/test/pytest/conftest.py @@ -5,6 +5,19 @@ `pytester` is enabled because the layer's own tests run pytest inside pytest: a guard is proven to REFUSE rather than assumed to. + +PSYCOPG IS IMPORTED INSIDE THE FIXTURES THAT USE IT, NOT HERE, AND THAT IS +LOAD-BEARING RATHER THAN TIDINESS. conftest is imported before every run, so a +module-scope `import psycopg` made a DATABASE DRIVER a hard requirement of the +whole corpus -- including the tests that never open a connection. Measured on the +corpus as it stands: with psycopg absent, 61 of 142 tests run and pass; with the +import at module scope, zero do, and the failure is a conftest ImportError before +collection. + +That is the difference between "this harness needs Postgres" and "the tests that +talk to Postgres need Postgres", and it is what lets the guard-testing half of +this corpus run somewhere that has no database at all -- which is where the gate +is (README.md, "This is not in the gate yet"). """ import os @@ -12,7 +25,6 @@ import pathlib import shutil -import psycopg import pytest from pgc_cluster import _pg_config, build_once, make_cluster @@ -72,6 +84,7 @@ def pgc_cluster(request, worker_id): # with the build after the start, the .so is NEWER than the postmaster and # this refuses. cluster.require_server_loaded_this_binary() + import psycopg # deferred: see the module docstring try: with psycopg.connect(cluster.dsn(), autocommit=True) as conn: conn.execute("CREATE EXTENSION IF NOT EXISTS pgcolumnar") @@ -93,6 +106,8 @@ def pgc_conn(pgc_cluster, request): leaves the next connection looking at an absent table, which is measured in the design document as a way to make a test assert nothing. """ + import psycopg # deferred: see the module docstring + schema = "pgc_test_" + "".join( ch if ch.isalnum() else "_" for ch in request.node.name )[:48] diff --git a/test/pytest/test_harness_deps.py b/test/pytest/test_harness_deps.py new file mode 100644 index 00000000..87fb0a43 --- /dev/null +++ b/test/pytest/test_harness_deps.py @@ -0,0 +1,170 @@ +"""The harness must be able to test ITSELF without a database. + +WHY THIS EXISTS. `conftest.py` imported psycopg at module scope, and conftest is +imported before every run, so a DATABASE DRIVER was a hard requirement of the +whole corpus -- including the 61 tests that never open a connection. With psycopg +absent the run did not fail a test, it failed to COLLECT: + + ImportError while loading conftest '.../conftest.py' + conftest.py:15: in + import psycopg + E ModuleNotFoundError: No module named 'psycopg' + +That coupling is why the guard-testing half of this corpus cannot run where the +gate runs. README.md records the decision not to register the corpus in `SUITES` +and names the price; this removes one of the two things making that price real. + +These arms keep it removed. A module-scope import reads like an ordinary tidy-up +when someone adds a fixture, and nothing else here would notice. +""" + +import os +import pathlib +import subprocess +import sys +import tempfile + +HERE = pathlib.Path(__file__).resolve().parent + +# The files that never open a connection. Derived rather than listed would be +# better, but "imports psycopg" is not the property -- the property is "uses a +# cluster fixture", and a file can name one in a docstring. Listed, with an arm +# below that fails if one of them starts needing a database. +NO_CLUSTER = [ + "test_docs_cover_the_corpus.py", + "test_guards_pinned.py", + "test_ordered.py", + "test_runshape.py", +] + + +def _run_without_psycopg(args, expect): + """Run pytest with `import psycopg` forced to fail, and return the result. + + A SHIM RATHER THAN AN UNINSTALL. Uninstalling psycopg would test the machine + rather than the harness, cannot run concurrently with anything else, and + leaves the environment broken if the test dies. A module that raises on + import, placed FIRST on the path, is the same observation and is reversible + by construction. + """ + shim = tempfile.mkdtemp(prefix="pgc-nopsy-") + pathlib.Path(shim, "psycopg.py").write_text( + 'raise ImportError("psycopg is shimmed out by ' + 'test_harness_deps: the harness must self-test without a database")\n' + ) + env = dict(os.environ) + env["PYTHONPATH"] = shim + os.pathsep + str(HERE) + proc = subprocess.run( + [sys.executable, "-m", "pytest", "-q", "-p", "no:cacheprovider", *args], + cwd=str(HERE), env=env, capture_output=True, text=True, + ) + # PREMISE: the shim must actually bite, or this arm proves nothing at all. + probe = subprocess.run( + [sys.executable, "-c", "import psycopg"], + cwd=str(HERE), env=env, capture_output=True, text=True, + ) + expect.at_least(int("ImportError" in probe.stderr), 1, + "premise: the shim really does make `import psycopg` fail") + return proc + + +def test_the_guard_half_of_the_corpus_runs_without_a_database_driver(expect): + """61 of 142 tests need no database. They must not need its driver either.""" + proc = _run_without_psycopg(NO_CLUSTER, expect) + out = proc.stdout + proc.stderr + # Collection surviving is the first thing to establish: without it, a green + # exit code would only mean pytest never got as far as running anything. + defeated = "ImportError while loading conftest" in out or "ModuleNotFoundError" in out + expect.text(repr(defeated), "False", + "collection is not defeated by the absent driver") + expect.num(proc.returncode, 0, + f"the no-cluster files pass with psycopg absent: {out[-400:]}") + + +def test_a_cluster_test_still_needs_the_driver(expect): + """THE CONTROL, and without it the arm above is satisfied by a corpus that + connects to nothing at all. + + Deferring the import must not have made the database optional -- only its + IMPORT lazy. A test that actually wants a connection must still fail when the + driver is gone, and it must fail for that reason rather than by being skipped. + """ + proc = _run_without_psycopg(["test_connection.py"], expect) + expect.at_least(proc.returncode, 1, + "a cluster test cannot pass without the driver") + expect.at_least( + int("psycopg is shimmed out" in (proc.stdout + proc.stderr)), 1, + "and it fails BECAUSE the driver is gone, naming the shim") + + +def test_conftest_imports_no_database_driver_at_module_scope(expect): + """The regression named directly, because the behavioural arm above is slow + and a reader changing conftest deserves to be told in one line.""" + src = (HERE / "conftest.py").read_text() + module_scope = [ + ln for ln in src.splitlines() + if ln.startswith("import psycopg") or ln.startswith("from psycopg") + ] + expect.text(", ".join(module_scope) or "none", "none", + "conftest.py imports no database driver at module scope") + # PREMISE: the check can see an import at all -- otherwise "none" is what a + # broken reader says too. + fake = "import os\nimport psycopg\n" + seen = [ln for ln in fake.splitlines() if ln.startswith("import psycopg")] + expect.num(len(seen), 1, "premise: the reader recognises a module-scope import") + + +def test_the_no_cluster_list_still_names_files_that_exist(expect): + """A list is a hazard. This one is small and pinned, so it fails loudly rather + than silently covering fewer files after a rename.""" + missing = [n for n in NO_CLUSTER if not (HERE / n).is_file()] + expect.text(", ".join(missing) or "none", "none", + "every file in NO_CLUSTER exists") + expect.at_least(len(NO_CLUSTER), 4, "premise: the list is not empty") + + +def test_ci_derives_the_file_list_rather_than_repeating_it(expect): + """CI must ask this module for NO_CLUSTER, not carry its own copy. + + A second copy of a list is the defect this repository spent a day removing + from TESTS.md: a hand-maintained value whose correct content is a function of + the tree, going stale silently because nothing compares the two. The job runs + `from test_harness_deps import NO_CLUSTER`, so adding a file here changes what + CI runs with no second edit. + + The pins are single-sourced the same way, out of requirements-test.txt, so the + version CI installs cannot drift from the version the corpus was tested with. + """ + ci = (HERE.parent.parent / ".github" / "workflows" / "ci.yml") + expect.text(repr(ci.is_file()), "True", "premise: ci.yml is where this expects") + text = ci.read_text() + + expect.at_least(text.count("from test_harness_deps import NO_CLUSTER"), 1, + "the job derives the file list from this module") + expect.at_least(text.count("requirements-test.txt"), 1, + "and the pins from requirements-test.txt") + + # AND IT MUST NOT ALSO HARDCODE THEM. Deriving plus a stale literal copy is + # worse than either alone, because the copy looks authoritative. + job = text[text.index("pytest-guards:"):] + job = job[:job.index("\n build:")] if "\n build:" in job else job + hardcoded = [n for n in NO_CLUSTER if n in job] + expect.text(", ".join(hardcoded) or "none", "none", + "the job names no corpus file literally") + + +def test_the_job_installs_no_database_driver(expect): + """The job's value is that it runs where there is no database. + + If it installed psycopg the tests would pass for the ordinary reason and prove + nothing about the coupling this file exists to keep removed. + """ + ci = (HERE.parent.parent / ".github" / "workflows" / "ci.yml") + job = ci.read_text() + job = job[job.index("pytest-guards:"):] + job = job[:job.index("\n build:")] if "\n build:" in job else job + installs_driver = "psycopg" in job and "pip install" in job and "pip show psycopg" not in job + expect.text(repr(installs_driver), "False", + "the job installs no database driver") + expect.at_least(job.count("pip show psycopg"), 1, + "and it asserts the driver is absent rather than assuming it") diff --git a/test/selftest/350-the-pytest-corpus-must-be.sh b/test/selftest/350-the-pytest-corpus-must-be.sh index 6835fe29..40b3d775 100644 --- a/test/selftest/350-the-pytest-corpus-must-be.sh +++ b/test/selftest/350-the-pytest-corpus-must-be.sh @@ -267,6 +267,62 @@ check "control: distinct names in the same corpus report no duplicate" \ unset -f _dcv_dupes + +# ---- the harness must self-test without a database, and the gate must run it -- +# +# conftest.py imported psycopg AT MODULE SCOPE, and conftest is imported before +# every run, so a DATABASE DRIVER was a hard requirement of the whole corpus -- +# including the 61 tests that never open a connection. With psycopg absent the run +# did not fail a test, it failed to COLLECT. +# +# That coupling was half of why README.md says the corpus is "not in the gate +# yet": pgc_skip treats a missing dependency as a failure rather than a skip, so +# registering the corpus in SUITES would redden every job. That argument is about +# the CLUSTER tests, and until the import moved there was no way to separate them. +# +# The behavioural proof lives in test/pytest/test_harness_deps.py, which shims +# `import psycopg` to raise and requires the no-cluster files to pass anyway. +# THIS half is static and it is the half that runs in the matrix. + +_hd_conf="$PGC_TESTDIR/pytest/conftest.py" +_hd_ci="$PGC_SRCDIR/.github/workflows/ci.yml" + +check "premise: the pytest conftest is where this part thinks it is" \ + "$([ -f "$_hd_conf" ] && echo yes || echo no)" "yes" + +check "conftest imports no database driver at module scope" \ + "$(grep -cE '^(import psycopg|from psycopg)' "$_hd_conf")" "0" + +# PREMISE: the pattern can see such an import at all, or "0" is what a broken +# grep says too. +printf 'import os\nimport psycopg\n' > "$PGC_WORKDIR/hd-fixture.py" +check "premise: the pattern recognises a module-scope driver import" \ + "$(grep -cE '^(import psycopg|from psycopg)' "$PGC_WORKDIR/hd-fixture.py")" "1" + +# And it must still be imported SOMEWHERE, or the fixtures cannot connect and the +# arm above is satisfied by a harness that talks to no database at all. +check "and the driver is still imported inside the fixtures that connect" \ + "$([ "$(grep -cE '^\s+import psycopg' "$_hd_conf")" -ge 1 ] && echo yes || echo no)" "yes" + +check "premise: the CI workflow is where this part thinks it is" \ + "$([ -f "$_hd_ci" ] && echo yes || echo no)" "yes" + +check "the gate runs the harness guards" \ + "$([ "$(grep -c 'pytest-guards:' "$_hd_ci")" -ge 1 ] && echo yes || echo no)" "yes" + +# DERIVED, NOT REPEATED. A second copy of the file list is the defect this repo +# spent a day removing from TESTS.md. +check "and it derives the file list rather than repeating it" \ + "$([ "$(grep -c 'from test_harness_deps import NO_CLUSTER' "$_hd_ci")" -ge 1 ] \ + && echo yes || echo no)" "yes" + +# Presence, not a count: the comment block above the job names this file too, +# and an exact count would be an assertion about the prose as much as the code. +check "and derives the pins from requirements-test.txt" \ + "$([ "$(grep -c 'requirements-test.txt' "$_hd_ci")" -ge 1 ] && echo yes || echo no)" "yes" + +unset _hd_conf _hd_ci + unset _dcv_dir _dcv_doc _dcv_seen _dcv_stated _dcv_fix _dcv_ht unset -f _dcv_missing _dcv_count From 2ad25372b26f8c159e6ec4a5abea8fe92960bfd2 Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Thu, 10 Sep 2026 12:46:51 +0000 Subject: [PATCH 2/6] test/pytest: decide the database-free half, and stop stating counts The gate job's file list came from NO_CLUSTER, a hand-written list, and nothing decided whether it was RIGHT. The only arm checked that the names it held exist, and at_least(len(NO_CLUSTER), 4) is satisfied by any list of five. So a new database-free test file was silently skipped by the job and nothing went red -- a coverage hole in the mechanism that exists to give those tests coverage. Membership is now DECIDED, from a property of each file, and reconciled against the declaration in BOTH directions. The property is read with ast, not a line regex: this corpus builds tests as strings for pytester, so a file that merely MENTIONS the driver in prose must not count, and one that reaches a cluster only through a fixture must. The cluster fixtures are read off conftest.py rather than named here, so adding one does not need a second edit. NO_CLUSTER missing a database-free file [1: undeclared:test_ordered.py] NO_CLUSTER claiming a cluster test [1: needs-a-cluster:test_connection.py] Both reddened; mutations count-asserted and restored byte-exact. The message names the offender and which way the disagreement goes, because "the lists differ" is not something a reader can act on. AND NO COUNT IS WRITTEN ANYWHERE. Six sentences stated "61 of 142"; the corpus is now 168 tests, and the ci.yml comment said 152 against a corpus of 154, so it was wrong the day it was written. The gate's step prints how many files it ran and pytest prints how many tests passed, which is #908's rule: a derived value that a human maintains is a defect, and the corpus gate provably cannot police prose -- its totals guard matches only the bold fixed-form line #908 removed. Three smaller things, each found by reading this change rather than the tree: A table-of-contents link must RESOLVE, not merely name a file. TESTS.md gained an entry whose anchor stripped the underscores out of test_harness_deps.py, so the link went nowhere while both existing arms passed -- they sweep for NAMES, and a broken link still contains the name it points at. The rule is GitHub's and mechanical, it has ONE definition in this file, and the sweep carries a coverage premise because without nullglob an unmatched glob stays literal and a loop that runs no checks reports every check it did run as passing. Two `printf ... | grep -qxF` pipelines are gone, one of them written by this change. Under this suite's pipefail grep -q exits on the first match, printf takes EPIPE, and the pipeline reports failure though the pattern WAS present -- so a name that matched is counted absent. Measured at 10 spurious absences in 40 runs under load; 40 runs of the rewritten sweep over the largest document give one distinct answer. Selftest 080 states this rule and its sweep has never entered test/selftest/, which #486 is fixing separately. harness_selftest 465 passed + 0 failed + 0 unrunnable = 465 PASSED pytest corpus 168 passed shellcheck -S error -s bash clean One consequence to merge in order: with this landing first, #922 adding a database-free test file will REDDEN this arm until that file is declared. That is the hole closing, not a regression -- before this change the file would have been skipped in silence. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- .github/workflows/ci.yml | 17 +- CHANGELOG.md | 75 +++ test/pytest/TESTS.md | 68 +- test/pytest/conftest.py | 11 +- test/pytest/test_harness_deps.py | 631 +++++++++++++++++- .../selftest/350-the-pytest-corpus-must-be.sh | 162 ++++- 6 files changed, 931 insertions(+), 33 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8d91a3da..b86bbf42 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -65,10 +65,15 @@ jobs: # The pytest harness's own guards, where they can actually run. # - # THIS RUNS NO DATABASE AND BUILDS NOTHING. test/pytest/ is 152 tests, and the - # majority of them test the HARNESS rather than the product -- they drive pytest - # inside pytest to prove a guard refuses what it claims to refuse. Those need an - # interpreter and nothing else. + # THIS RUNS NO DATABASE AND BUILDS NOTHING. Most of test/pytest/ tests the + # HARNESS rather than the product -- it drives pytest inside pytest to prove a + # guard refuses what it claims to refuse. Those tests need an interpreter and + # nothing else. + # + # NO COUNT IS WRITTEN HERE. One was, and it was wrong the day it was written: + # the comment said 152 against a corpus of 154. The step below prints how many + # files it ran and pytest prints how many tests passed, so the numbers reach a + # reader from the run instead of from a sentence (#908). # # README.md records why the corpus is not in `SUITES`: pgc_skip treats a missing # dependency as a failure rather than a skip, so registering it there would @@ -105,7 +110,9 @@ jobs: cd test/pytest FILES="$(python3 -c 'import sys; sys.path.insert(0, "."); from test_harness_deps import NO_CLUSTER; print(" ".join(NO_CLUSTER))')" test -n "$FILES" - echo "running: $FILES" + # Printed, not stated: the count is a fact about the tree at this + # commit, so it belongs in the output rather than in a comment. + echo "running $(set -- $FILES; echo $#) database-free file(s): $FILES" PYTHONPATH=. /tmp/pgcvenv/bin/pytest -q $FILES # Build against every supported major. Fast, and it is what an API change diff --git a/CHANGELOG.md b/CHANGELOG.md index ee62d1c1..c6cbb2a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,54 @@ true until the next version shipped. ### Added +- The harness guards run in the gate, without a database, and which files that is + gets DECIDED rather than listed (#432). + + `conftest.py` imported psycopg at module scope, and conftest is imported before + every run, so a database driver was a hard requirement of COLLECTING the whole + pytest corpus -- including every test that never opens a connection. Deferring + that one import into the two fixtures that connect lets the guard-testing half + run where the gate runs: `.github/workflows/ci.yml` gains a `pytest-guards` job + with no database, no build, an interpreter and two pinned packages. The job + derives its file list from `NO_CLUSTER` in `test/pytest/test_harness_deps.py` + and its pins from `requirements-test.txt`, so neither is a second copy, and it + asserts psycopg is ABSENT before running -- otherwise the tests would pass for + the ordinary reason and prove nothing about the coupling. + + `NO_CLUSTER` is now decided, not declaimed. The only arm over it asked whether + the files it named EXIST, which is one direction of a membership claim, and the + missing direction is the one that loses coverage: a database-free test file that + nobody adds to the list is simply absent from the job, every arm stays green and + nothing says so. It had already happened twice -- `test_build_refusal.py` and + `test_layer.py` both need no database and neither was listed. The property is + now computed from the corpus by an ast walk and required to equal the list in + both directions, so the job runs every database-free file rather than the four + somebody remembered. + + An ast walk rather than a line regex, because three shapes here defeat a grep: a + file may name the driver in a docstring, discuss a cluster fixture in prose, or + build another test as a string for `pytester`. And needing a database is not + importing the driver -- a test reaches a cluster through a FIXTURE and may import + nothing -- so a file is cluster-bound if it imports the driver at module scope, + if any test or fixture in it requests a fixture that reaches a cluster, or if it + drives a cluster-bound file as a subprocess. The connecting fixtures are read off + `conftest.py` rather than named in the classifier. + + Every rule was proved by removal: eight mutations, each asserted to have applied + and restored byte-exact, each reddening a named arm. Two of them survived the + first pass and found real gaps -- a closure over each file's own fixture graph + that changed no classification, removed as dead, and a prose filter with no + killing arm, which now has one. The classification of all twelve corpus files was + exercised against reality: each of the six called database-free passes with + `import psycopg` shimmed to raise and no usable `pg_config`, and each of the six + called cluster-bound fails, for the driver or for `pg_config` and nothing else. + + Because the corpus is not in `SUITES` and the job runs only the database-free + files, none of those arms runs in the gate, so + `test/selftest/350-the-pytest-corpus-must-be.sh` runs the membership decision + through the module's command line, with its own fixture corpus to fail against. + A guard that does not run is a comment. + - Exact zone-map boundary coverage now lives in matching shell and pytest tests (#831). @@ -160,6 +208,33 @@ true until the next version shipped. ### Fixed +- TESTS.md's contents list no longer carries a link that goes nowhere, and the + corpus gate now checks every one of them. + + The entry added for `test_harness_deps.py` stripped the underscores out of the + file name -- `#14-testharnessdepspy-...` against a heading GitHub renders as + `#14-test_harness_depspy-...` -- so the link silently resolved to nothing. The + eleven entries above it keep the underscores, so the document already stated the + convention. Neither existing arm could see it: both sweep for NAMES, and a broken + anchor is still a string containing the name it points at. Selftest 350 now + derives each heading's anchor by GitHub's rule and requires every in-document + link to reach one. Measured over the three documents in that directory it reports + nothing, and over the document as it shipped it reported exactly the one entry. + +- No sentence in the pytest harness states how many tests the corpus holds (#908). + + Eight places said "61 of 142" or "152 tests": `test_harness_deps.py` twice, + `conftest.py`, `TESTS.md` three times, the `pytest-guards` job's comment in + `ci.yml`, and selftest 350's own comment. The corpus held 154 on the day they + were written, so the job's comment was already wrong, and a concurrent branch + adds a thirteenth test file, which would have made every one of them wrong + again. These are the hand-maintained derived values #908 spent a day removing + from TESTS.md's totals line, reintroduced as prose, where that line's guard + cannot see them: it matches only the bold fixed-form line. The numbers are gone. + The job prints how many files it ran and pytest prints how many tests passed, the + membership arm prints the partition, and an arm over the job and its comment + block refuses a written count there. + - `ALTER TABLE ... RENAME COLUMN` now carries the new name into `pgcolumnar.projection_declaration`, for the named relation and for every inheritance descendant, including a `PARTITION OF` child (#888). diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index f2ab4563..5f828644 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -59,7 +59,7 @@ behaviour, the source of that number is named. - [11. test_zonemap_boundaries.py: exact boundaries](#11-test_zonemap_boundariespy-exact-boundaries) - [12. test_saop_element_pushdown.py: scattered set pruning](#12-test_saop_element_pushdownpy-scattered-set-pruning) - [13. test_hilbert_locality.py: what the Hilbert curve buys](#13-test_hilbert_localitypy-what-the-hilbert-curve-buys) -- [14. test_harness_deps.py: the harness must self-test without a database](#14-testharnessdepspy-the-harness-must-self-test-without-a-database) +- [14. test_harness_deps.py: the harness must self-test without a database](#14-test_harness_depspy-the-harness-must-self-test-without-a-database) - [15. Adding a test](#15-adding-a-test) - [16. What this corpus does NOT yet refuse](#16-what-this-corpus-does-not-yet-refuse) - [17. Traps this corpus records](#17-traps-this-corpus-records) @@ -1017,7 +1017,7 @@ domain is a changed READER at an unchanged layout. `conftest.py` imported psycopg at module scope, and conftest is imported before every run, so a **database driver was a hard requirement of the whole corpus** -- -including the 61 tests that never open a connection. With psycopg absent the run +including every test that never opens a connection. With psycopg absent the run did not fail a test, it failed to COLLECT: ImportError while loading conftest '.../conftest.py' @@ -1025,25 +1025,77 @@ did not fail a test, it failed to COLLECT: import psycopg E ModuleNotFoundError: No module named 'psycopg' -Measured: with the import deferred into the two fixtures that connect, **61 of -142 tests run and pass with no driver installed**; with it at module scope, zero +With the import deferred into the two fixtures that connect, the database-free +files run and pass with no driver installed; with it at module scope, none of them do. That coupling is half of why the guard-testing part of this corpus cannot run where the gate runs (README.md, "This is not in the gate yet"). +**WHICH FILES NEED NO DATABASE IS DECIDED, NOT DECLAIMED.** `NO_CLUSTER` in that +module is the declaration, and the gate's job runs exactly it. The property is +computed from the corpus by an ast walk, and the two must agree in BOTH +directions. + +The missing direction was the one that loses coverage. The only arm over the list +asked whether the files it names EXIST, so a database-free file nobody added was +simply absent from the job: every arm stayed green and nothing said so. It had +already happened twice -- `test_build_refusal.py` and `test_layer.py` both need no +database and neither was listed -- and a concurrent branch adds a third. A floor of four on the list length did not help: the list had four +entries, so the floor was satisfied by the state it was meant to police. + +AN AST WALK RATHER THAN A LINE REGEX, because three shapes here defeat a grep: a +file may name the driver in a docstring, discuss a cluster fixture in prose, or +BUILD another test as a string for `pytester`. This layer has already paid for that +lesson once -- the broad-except refusal was first written as a line regex and +rejected its own tests, because the forbidden shape appears inside a +`makepyfile` string. + +AND NEEDING A DATABASE IS NOT IMPORTING THE DRIVER. A test reaches a cluster +through a FIXTURE and may import nothing, so a file is cluster-bound if it imports +the driver at module scope (which kills collection outright), if any test or +fixture in it requests -- directly or transitively -- a fixture that reaches a +cluster, or if it DRIVES a cluster-bound file as a subprocess. The connecting +fixtures are read off `conftest.py` rather than named in the classifier. + | test | asserts | | --- | --- | | `test_the_guard_half_of_the_corpus_runs_without_a_database_driver` | the no-cluster files collect and pass with `import psycopg` shimmed to raise | | `test_a_cluster_test_still_needs_the_driver` | **control**: deferring made the IMPORT lazy, not the database optional | | `test_conftest_imports_no_database_driver_at_module_scope` | the regression named in one line, for whoever edits conftest next | -| `test_the_no_cluster_list_still_names_files_that_exist` | the list is a hazard; it fails loudly rather than covering fewer files after a rename | -| `test_ci_derives_the_file_list_rather_than_repeating_it` | the CI job asks this module for `NO_CLUSTER` and names no file literally | +| `test_the_declaration_is_exactly_the_database_free_half` | `NO_CLUSTER` equals the property, both ways, so an undeclared database-free file is named | +| `test_the_partition_accounts_for_every_file_in_the_corpus` | **premise**: every file lands in exactly one bucket, and neither bucket is the whole corpus | +| `test_the_cluster_fixtures_are_read_off_conftest_rather_than_named_here` | the roots of the property are derived from `conftest.py`, not typed | +| `test_the_classifier_tells_a_plain_file_from_one_that_requests_a_cluster` | the base case and its control, over a fixture corpus | +| `test_the_classifier_follows_a_cluster_fixture_through_a_local_wrapper` | a module-local fixture wrapping `pgc_cluster` is followed | +| `test_the_classifier_catches_a_module_scope_driver_import` | an eager import kills collection, so the file cannot run in the job | +| `test_the_classifier_is_not_fooled_by_prose_that_names_the_driver` | a docstring, a block-comment string, a generated test, and a file merely discussed | +| `test_the_classifier_takes_a_fixture_that_provisions_without_connecting` | the second signal, isolated: a fixture that starts a cluster and imports no driver | +| `test_the_classifier_follows_a_conftest_fixture_that_connects_indirectly` | a conftest fixture reaching a cluster through a sibling, importing nothing itself | +| `test_the_classifier_does_not_read_a_helpers_parameter_as_a_fixture` | pytest resolves names for tests and fixtures, not for helpers | +| `test_the_classifier_follows_a_file_that_drives_a_cluster_bound_file` | this file's own shape: driving a cluster-bound file inherits what it needs | +| `test_the_membership_report_names_a_database_free_file_left_undeclared` | the hole itself, on a fixture, with the control beside it | +| `test_the_membership_report_names_a_declared_file_that_needs_a_cluster` | the other direction: a listed file that starts using a cluster fixture | +| `test_the_membership_report_names_a_declared_file_that_is_gone` | a rename is still caught, and as its own kind rather than as a cluster need | +| `test_the_gate_runs_the_membership_decision_rather_than_only_this_file` | selftest 350 runs the decision, and the command line it uses works | +| `test_ci_derives_the_file_list_rather_than_repeating_it` | the CI job asks this module for `NO_CLUSTER`, names no file literally, and states no count | | `test_the_job_installs_no_database_driver` | the job asserts psycopg is absent rather than assuming it | **THIS IS NOW IN THE GATE.** `.github/workflows/ci.yml` runs a `pytest-guards` job: no database, no build, an interpreter and the two pinned runner packages. The file list is derived from `NO_CLUSTER` in this module and the pins from -`requirements-test.txt`, so neither is a second copy that can go stale -- and two -arms above hold it to that. Measured in the job: 61 tests, ~3 seconds. +`requirements-test.txt`, so neither is a second copy that can go stale -- and +three arms above hold it to that. **The job states no count and neither does this +section**: it prints how many files it ran and pytest prints how many tests +passed, so the numbers reach a reader from the run. A written count is a +hand-maintained derived value, and the one in the job's comment was wrong the day +it was written (#908). + +BUT THE ARMS IN THIS FILE DO NOT RUN IN THE GATE, and that is why +`test/selftest/350-the-pytest-corpus-must-be.sh` runs the membership decision +through this module's command line. The corpus is not in `SUITES` (README.md), and +the `pytest-guards` job runs the database-free files -- which this file is not, +because its control arm needs a real cluster. A guard that does not run is a +comment, so the decision has a copy with teeth, exactly as the documentation +sweep in that part does. A SHIM RATHER THAN AN UNINSTALL. Uninstalling psycopg would test the machine rather than the harness, could not run beside anything else, and would leave the diff --git a/test/pytest/conftest.py b/test/pytest/conftest.py index ef8e671b..c03425cd 100644 --- a/test/pytest/conftest.py +++ b/test/pytest/conftest.py @@ -9,10 +9,13 @@ PSYCOPG IS IMPORTED INSIDE THE FIXTURES THAT USE IT, NOT HERE, AND THAT IS LOAD-BEARING RATHER THAN TIDINESS. conftest is imported before every run, so a module-scope `import psycopg` made a DATABASE DRIVER a hard requirement of the -whole corpus -- including the tests that never open a connection. Measured on the -corpus as it stands: with psycopg absent, 61 of 142 tests run and pass; with the -import at module scope, zero do, and the failure is a conftest ImportError before -collection. +whole corpus -- including every test that never opens a connection. With the +import at module scope NO test runs without the driver, and the failure is a +conftest ImportError before collection rather than a failed test. + +No count is written here. Which files need no database is decided in +test_harness_deps.py, from the corpus, and the gate's job prints what it ran: +a number in this docstring would be a hand-maintained derived value (#908). That is the difference between "this harness needs Postgres" and "the tests that talk to Postgres need Postgres", and it is what lets the guard-testing half of diff --git a/test/pytest/test_harness_deps.py b/test/pytest/test_harness_deps.py index 87fb0a43..323e0f1d 100644 --- a/test/pytest/test_harness_deps.py +++ b/test/pytest/test_harness_deps.py @@ -2,7 +2,7 @@ WHY THIS EXISTS. `conftest.py` imported psycopg at module scope, and conftest is imported before every run, so a DATABASE DRIVER was a hard requirement of the -whole corpus -- including the 61 tests that never open a connection. With psycopg +whole corpus -- including every test that never opens a connection. With psycopg absent the run did not fail a test, it failed to COLLECT: ImportError while loading conftest '.../conftest.py' @@ -16,28 +16,311 @@ These arms keep it removed. A module-scope import reads like an ordinary tidy-up when someone adds a fixture, and nothing else here would notice. + +AND THE MEMBERSHIP IS DECIDED RATHER THAN DECLAIMED. `NO_CLUSTER` below says +which files need no database, and the gate runs exactly those. A list that is +only ASSERTED is a silent coverage hole: a new database-free test file is simply +absent from the job, every arm stays green, and the tests never run anywhere. So +the property is computed from the corpus and the two are required to AGREE, in +both directions. """ +import ast import os import pathlib +import re import subprocess import sys import tempfile HERE = pathlib.Path(__file__).resolve().parent -# The files that never open a connection. Derived rather than listed would be -# better, but "imports psycopg" is not the property -- the property is "uses a -# cluster fixture", and a file can name one in a docstring. Listed, with an arm -# below that fails if one of them starts needing a database. +# The database driver, and the module that provisions a cluster. Named once: the +# classifier below asks conftest.py which fixtures reach a cluster rather than +# being told their names, so these two are the only hard-coded identifiers. +DRIVER = "psycopg" +CLUSTER_MODULE = "pgc_cluster" + +# Where the ci.yml region the count arm reads begins. The job's own comment, not +# the job key, because a prose count lives in the comment. +JOB_COMMENT = "# The pytest harness's own guards" + +# THE FILES THAT REACH NO DATABASE. This is what the `pytest-guards` job in +# .github/workflows/ci.yml runs, derived from here rather than copied into the +# workflow. +# +# DECLARED HERE, DECIDED BELOW. `test_the_declaration_is_exactly_the_database_free_half` +# computes the property over the corpus and requires SET EQUALITY with this list, +# so a database-free file nobody adds here is NAMED rather than silently skipped, +# and a file listed here that starts using a cluster fixture is named too. +# +# A LIST *AND* A PROPERTY, RATHER THAN THE PROPERTY ALONE, DELIBERATELY. Deriving +# the membership outright and keeping no list would remove the hand-maintained +# value, but it would also leave the classifier with nothing to be checked +# against: a bug that dropped a file would quietly shrink what the gate runs and +# nothing would go red -- the same silent hole, one level down. Two declarations +# that must agree fail loudly whichever of them is wrong. It is the shape +# selftest 350 already uses for TESTS.md, for the same reason. NO_CLUSTER = [ + "test_build_refusal.py", "test_docs_cover_the_corpus.py", "test_guards_pinned.py", + "test_layer.py", "test_ordered.py", "test_runshape.py", ] +# --------------------------------------------------------------------------- +# Deciding "this file needs no database" +# +# AN AST WALK RATHER THAN A LINE REGEX, and this layer has already paid for that +# lesson once: the broad-except refusal was first written as a line regex and +# immediately rejected its own tests, because the forbidden shape appears inside +# a `pytester.makepyfile` STRING. The same trap is here in three forms -- a file +# may name the driver in a docstring, build another test as a string, or discuss +# a cluster fixture in prose -- and a grep cannot tell any of those from code. +# +# NEEDING A DATABASE IS NOT THE SAME AS IMPORTING THE DRIVER. A test reaches a +# cluster through a FIXTURE and may import nothing at all, so the property is: +# +# a file is cluster-bound if it imports the driver at module scope (which kills +# collection outright), or if any test or fixture in it requests -- directly or +# transitively -- a fixture that reaches a cluster, or if it DRIVES a +# cluster-bound file as a subprocess. +# +# The cluster fixtures themselves are derived from conftest.py rather than typed +# here, so a new one is covered without a second edit. +# --------------------------------------------------------------------------- + + +def _parse(path): + return ast.parse(pathlib.Path(path).read_text(), filename=pathlib.Path(path).name) + + +def _prose(tree): + """id() of every string used as a STATEMENT: module, class and function + docstrings, and the bare strings this corpus uses as block comments. + + These are the strings a reader writes ABOUT code, so nothing in them counts + as a reference to anything.""" + out = set() + for node in ast.walk(tree): + if (isinstance(node, ast.Expr) and isinstance(node.value, ast.Constant) + and isinstance(node.value.value, str)): + out.add(id(node.value)) + return out + + +def _own_body(node): + """Every node inside NODE, not descending into a nested function. + + A nested function's body runs when IT is called, not when NODE is, so an + import inside one is not an import by NODE.""" + out, stack = [], list(ast.iter_child_nodes(node)) + while stack: + n = stack.pop() + out.append(n) + if isinstance(n, (ast.FunctionDef, ast.AsyncFunctionDef, ast.Lambda)): + continue + stack.extend(ast.iter_child_nodes(n)) + return out + + +def _module_scope(tree): + """Every node that executes at import time: the module body and anything + nested in its `if`/`try`/`with`, but nothing inside a def or a class.""" + out, stack = [], list(tree.body) + while stack: + n = stack.pop() + out.append(n) + if isinstance(n, (ast.FunctionDef, ast.AsyncFunctionDef, + ast.Lambda, ast.ClassDef)): + continue + stack.extend(ast.iter_child_nodes(n)) + return out + + +def _imports_driver(nodes): + for n in nodes: + if isinstance(n, ast.Import): + if any(a.name.split(".")[0] == DRIVER for a in n.names): + return True + elif isinstance(n, ast.ImportFrom): + if (n.module or "").split(".")[0] == DRIVER: + return True + return False + + +def _is_fixture(fn): + """@pytest.fixture, @pytest.fixture(...), @fixture or @fixture(...).""" + for dec in fn.decorator_list: + f = dec.func if isinstance(dec, ast.Call) else dec + if isinstance(f, ast.Attribute) and f.attr == "fixture": + return True + if isinstance(f, ast.Name) and f.id == "fixture": + return True + return False + + +def _defs(tree): + """{name: (kind, params, body)} for every module-level def. + + kind is "fixture", "test" or "helper". Only the first two can pull a fixture + in: pytest resolves parameter names for those, and a helper's parameter is + just a parameter -- so a helper taking `conn` named after a fixture must not + make its file cluster-bound.""" + out = {} + for n in tree.body: + if not isinstance(n, (ast.FunctionDef, ast.AsyncFunctionDef)): + continue + if _is_fixture(n): + kind = "fixture" + elif n.name.startswith("test_"): + kind = "test" + else: + kind = "helper" + out[n.name] = (kind, [a.arg for a in n.args.args], _own_body(n)) + return out + + +def _imported_from(tree, module): + names = set() + for n in ast.walk(tree): + if isinstance(n, ast.ImportFrom) and (n.module or "") == module: + names |= {a.asname or a.name for a in n.names} + return names + + +def _close_over_fixtures(defs, reached): + """Add every fixture that requests something already reached, until stable.""" + changed = True + while changed: + changed = False + for name, (kind, params, _body) in defs.items(): + if kind == "fixture" and name not in reached and set(params) & reached: + reached.add(name) + changed = True + return reached + + +def cluster_fixtures(conftest): + """The fixture names that reach a cluster, READ OFF conftest.py. + + A fixture reaches a cluster if it imports the driver, or calls something + imported from pgc_cluster (which is what provisions a server), or requests a + fixture that does. Derived rather than typed, so adding a third connecting + fixture to conftest does not need an edit here.""" + tree = _parse(conftest) + provisioners = _imported_from(tree, CLUSTER_MODULE) + defs = _defs(tree) + roots = set() + for name, (kind, _params, body) in defs.items(): + if kind != "fixture": + continue + called = {n.func.id for n in body + if isinstance(n, ast.Call) and isinstance(n.func, ast.Name)} + if _imports_driver(body) or (called & provisioners): + roots.add(name) + return _close_over_fixtures(defs, roots) + + +def _mentioned_files(tree, others): + """Corpus file names this module names OUTSIDE its prose -- that is, in code. + + A file that hands another file's name to pytest is driving it, and inherits + what that file needs.""" + prose = _prose(tree) + found = set() + for node in ast.walk(tree): + if (isinstance(node, ast.Constant) and isinstance(node.value, str) + and id(node) not in prose): + found |= {o for o in others if o in node.value} + return found + + +def partition(directory=None): + """(database-free, cluster-bound) over every test_*.py in DIRECTORY, sorted. + + Every file lands in exactly one bucket, so the two lengths sum to the number + of files the glob saw.""" + directory = HERE if directory is None else pathlib.Path(directory) + names = sorted(p.name for p in directory.glob("test_*.py")) + roots = cluster_fixtures(directory / "conftest.py") + + bound, mentions = {}, {} + for name in names: + tree = _parse(directory / name) + defs = _defs(tree) + # NO CLOSURE OVER THE FILE'S OWN FIXTURE GRAPH, and that is not an + # omission. A chain of local fixtures can only reach a cluster if some + # fixture IN the chain names a conftest root as its own parameter -- and + # that fixture is itself in this loop, so walking the chain finds nothing + # the direct check does not. Measured: neutering a closure here changed + # no file's classification, which is what dead code does. + # + # The closure IS load-bearing inside conftest, where a fixture can reach + # a cluster through a sibling without importing anything of its own, and + # `test_the_classifier_follows_a_conftest_fixture_that_connects_indirectly` + # kills it there. + uses = any(kind in ("fixture", "test") and set(params) & roots + for kind, params, _body in defs.values()) + bound[name] = _imports_driver(_module_scope(tree)) or uses + mentions[name] = _mentioned_files(tree, set(names) - {name}) + + # A file that drives a cluster-bound file needs whatever that file needs. + changed = True + while changed: + changed = False + for name in names: + if not bound[name] and any(bound[o] for o in mentions[name]): + bound[name] = True + changed = True + + return ([n for n in names if not bound[n]], [n for n in names if bound[n]]) + + +def membership_report(directory=None, declared=None): + """"[]" when the declaration is exactly the database-free half, else + "[n: kind:file ...]" naming every disagreement and which way it goes. + + The same "[]" / "[n: ...]" shape the selftest parts use, so a failure names + what is wrong rather than only that something is.""" + directory = HERE if directory is None else pathlib.Path(directory) + declared = NO_CLUSTER if declared is None else declared + free, _bound = partition(directory) + free = set(free) + present = {p.name for p in directory.glob("test_*.py")} + bad = ["absent:" + n for n in sorted(set(declared) - present)] + bad += ["needs-a-cluster:" + n + for n in sorted((set(declared) & present) - free)] + bad += ["undeclared:" + n for n in sorted(free - set(declared))] + return "[]" if not bad else "[%d: %s]" % (len(bad), " ".join(bad)) + + +def _main(argv): + """The gate's entry point. The pytest corpus is not in `SUITES`, so the arms + below run nowhere the gate can see them; selftest 350 runs this instead.""" + if len(argv) < 2 or argv[0] not in ("--disagree", "--partition"): + # A mode with no DIRECTORY has to be a usage error rather than an + # IndexError: the caller is a shell arm, and a traceback on stderr with an + # empty stdout is what a passing "[]" comparison looks like from bash. + sys.stderr.write( + "usage: test_harness_deps.py --disagree DIR [FILE...]\n" + " test_harness_deps.py --partition DIR\n") + return 2 + mode, directory, rest = argv[0], argv[1], argv[2:] + if mode == "--partition": + free, bound = partition(directory) + print("free: %s | bound: %s" % (" ".join(free) or "-", + " ".join(bound) or "-")) + return 0 + # An EMPTY declaration is a real question ("nothing is declared"), so the + # default only applies when no FILE argument was given at all. + print(membership_report(directory, rest if len(argv) > 2 else None)) + return 0 + + def _run_without_psycopg(args, expect): """Run pytest with `import psycopg` forced to fail, and return the result. @@ -69,7 +352,8 @@ def _run_without_psycopg(args, expect): def test_the_guard_half_of_the_corpus_runs_without_a_database_driver(expect): - """61 of 142 tests need no database. They must not need its driver either.""" + """The database-free files need no database, so they must not need its driver + either. The count is not written down here: the run prints it.""" proc = _run_without_psycopg(NO_CLUSTER, expect) out = proc.stdout + proc.stderr # Collection surviving is the first thing to establish: without it, a green @@ -79,6 +363,8 @@ def test_the_guard_half_of_the_corpus_runs_without_a_database_driver(expect): "collection is not defeated by the absent driver") expect.num(proc.returncode, 0, f"the no-cluster files pass with psycopg absent: {out[-400:]}") + print("\n-- no-cluster files: %d, driver shimmed out: %s" + % (len(NO_CLUSTER), out.strip().splitlines()[-1])) def test_a_cluster_test_still_needs_the_driver(expect): @@ -114,13 +400,312 @@ def test_conftest_imports_no_database_driver_at_module_scope(expect): expect.num(len(seen), 1, "premise: the reader recognises a module-scope import") -def test_the_no_cluster_list_still_names_files_that_exist(expect): - """A list is a hazard. This one is small and pinned, so it fails loudly rather - than silently covering fewer files after a rename.""" - missing = [n for n in NO_CLUSTER if not (HERE / n).is_file()] - expect.text(", ".join(missing) or "none", "none", - "every file in NO_CLUSTER exists") - expect.at_least(len(NO_CLUSTER), 4, "premise: the list is not empty") +# ---- the declaration must be DECIDED, in both directions --------------------- +# +# WHAT WAS WRONG. The only arm over NO_CLUSTER asked whether the files it names +# EXIST. That is one direction of a membership claim, and the missing direction +# is the one that loses coverage: a database-free file nobody adds to the list is +# absent from the gate's job, every arm stays green, and nothing says so. It had +# already happened twice on this branch -- test_build_refusal.py and +# test_layer.py both need no database and neither was listed -- and a concurrent +# branch adds a third. +# +# A floor of 4 on the list length did not help either: the list had four entries, +# so the floor was satisfied by the very state it was meant to police. + + +def test_the_declaration_is_exactly_the_database_free_half(expect): + """NO_CLUSTER == the files the property says need no database, both ways. + + This is the arm that closes the hole. It names the offender and which way the + disagreement goes, because "the lists differ" is not something a reader can + act on.""" + expect.text(membership_report(), "[]", + "NO_CLUSTER is exactly the database-free half of the corpus") + + +def test_the_partition_accounts_for_every_file_in_the_corpus(expect): + """PREMISE for the arm above: the classifier saw the corpus, and split it. + + A classifier that parsed nothing reports an empty database-free set, which + agrees with an empty declaration and looks exactly like success. And one that + called everything database-free would also pass a one-directional check.""" + free, bound = partition() + files = sorted(p.name for p in HERE.glob("test_*.py")) + expect.num(len(free) + len(bound), len(files), + f"every file lands in exactly one bucket: {len(free)}+{len(bound)}") + expect.at_least(len(files), 10, "premise: the glob found the corpus") + expect.at_least(len(free), 1, "premise: the partition is not all cluster-bound") + expect.at_least(len(bound), 1, "premise: the partition is not all database-free") + print("\n-- partition: %d database-free, %d cluster-bound, %d files" + % (len(free), len(bound), len(files))) + + +def test_the_cluster_fixtures_are_read_off_conftest_rather_than_named_here(expect): + """The roots of the property are DERIVED. The only identifiers this module + spells out are the driver and the module that provisions a server, so a third + connecting fixture in conftest is covered without an edit here.""" + roots = cluster_fixtures(HERE / "conftest.py") + expect.at_least(len(roots), 2, + f"conftest names fixtures that reach a cluster: {sorted(roots)}") + print("\n-- cluster fixtures derived from conftest.py: %s" % sorted(roots)) + + +# ---- and the classifier must be able to get it WRONG ------------------------- +# +# Everything above passes on a healthy tree, which is what a classifier that +# returns a constant also does. These arms run it over fixtures built to be each +# shape it has to tell apart, so a future edit that neuters it reddens here while +# the real corpus stays clean. + +_FAKE_CONFTEST = '''\ +"""A conftest shaped like the real one: one fixture imports the driver, one +depends on that fixture and imports it too.""" +import pytest +from pgc_cluster import make_cluster + +@pytest.fixture(scope="session") +def pgc_cluster(): + cluster = make_cluster() + import psycopg + yield cluster + +@pytest.fixture +def pgc_conn(pgc_cluster): + import psycopg + yield psycopg.connect("") +''' + + +def _fake_corpus(tmp_path, files, conftest=None): + d = tmp_path / "corpus" + d.mkdir(exist_ok=True) + (d / "conftest.py").write_text(_FAKE_CONFTEST if conftest is None else conftest) + for name, body in files.items(): + (d / name).write_text(body) + return d + + +def test_the_classifier_tells_a_plain_file_from_one_that_requests_a_cluster(tmp_path, expect): + """The base case and its control, over a conftest this test wrote: the + derivation of the cluster fixtures runs here too, on a fixture rather than on + the real corpus.""" + d = _fake_corpus(tmp_path, { + "test_plain.py": "def test_nothing(expect):\n expect.num(1, 1, 'x')\n", + "test_direct.py": "def test_it(pgc_conn, expect):\n pass\n", + }) + expect.text(" ".join(sorted(cluster_fixtures(d / "conftest.py"))), + "pgc_cluster pgc_conn", + "both connecting fixtures are derived, including the indirect one") + free, bound = partition(d) + expect.text(" ".join(free) + " / " + " ".join(bound), + "test_plain.py / test_direct.py", + "a test requesting a cluster fixture is bound; one requesting none is free") + + +def test_the_classifier_follows_a_cluster_fixture_through_a_local_wrapper(tmp_path, expect): + """THE CASE A GREP CANNOT SEE. The file names no fixture of conftest's in any + test; a module-local fixture does, and the tests request that.""" + d = _fake_corpus(tmp_path, { + "test_wrapped.py": ( + "import pytest\n\n" + "@pytest.fixture\n" + "def loaded(pgc_cluster):\n" + " yield pgc_cluster\n\n" + "def test_it(loaded, expect):\n" + " pass\n" + ), + }) + free, bound = partition(d) + expect.text(" ".join(free) + "/" + " ".join(bound), "/test_wrapped.py", + "a cluster fixture reached through a local fixture is followed") + + +def test_the_classifier_catches_a_module_scope_driver_import(tmp_path, expect): + """An import at module scope kills COLLECTION, so the file cannot run in the + job even though no test of its own asks for a connection.""" + d = _fake_corpus(tmp_path, { + "test_eager.py": "import psycopg\n\ndef test_it(expect):\n pass\n", + }) + free, bound = partition(d) + expect.text(" ".join(free) + "/" + " ".join(bound), "/test_eager.py", + "a module-scope driver import makes the file cluster-bound") + + +def test_the_classifier_is_not_fooled_by_prose_that_names_the_driver(tmp_path, expect): + """THE REGEX TRAP, and this layer has paid for it once already: the + broad-except refusal was first written as a line regex and rejected its own + tests, because the forbidden shape appears inside a `makepyfile` string. + + A docstring naming the driver, a block-comment string naming a cluster + fixture, and a test BUILT as a string are all prose about code.""" + d = _fake_corpus(tmp_path, { + "test_needsdb.py": "def test_it(pgc_conn, expect):\n pass\n", + "test_prose.py": ( + '"""This file explains `import psycopg` and the pgc_conn fixture,\n' + 'and it discusses test_needsdb.py, which does need a cluster."""\n' + "\n" + "def test_it(pytester, expect):\n" + ' """It uses pgc_conn nowhere; it writes a test that would."""\n' + ' "a block comment mentioning import psycopg and pgc_cluster"\n' + " pytester.makepyfile(\n" + ' "import psycopg\\n"\n' + ' "def test_inner(pgc_conn):\\n pass\\n"\n' + " )\n" + ), + }) + free, bound = partition(d) + expect.text(" ".join(free) + " / " + " ".join(bound), + "test_prose.py / test_needsdb.py", + "prose, a generated test, and a file merely DISCUSSED are not requirements") + + +def test_the_classifier_takes_a_fixture_that_provisions_without_connecting(tmp_path, expect): + """THE SECOND SIGNAL, isolated. A fixture can start a cluster and hand back the + object without ever connecting, so it imports no driver: the only thing that + marks it is the call into the module that provisions a server. + + Its own arm rather than a clause in the one below, because a fixture that + neither imports nor calls would leave two rules untested at once.""" + d = _fake_corpus(tmp_path, { + "test_raw.py": "def test_it(pgc_raw, expect):\n pass\n", + }, conftest=( + "import pytest\n" + "from pgc_cluster import make_cluster\n\n" + "@pytest.fixture(scope='session')\n" + "def pgc_raw():\n" + " yield make_cluster()\n" + )) + expect.text(" ".join(sorted(cluster_fixtures(d / "conftest.py"))), "pgc_raw", + "a fixture that provisions a cluster is a root without importing a driver") + free, bound = partition(d) + expect.text(" ".join(free) + " / " + " ".join(bound), " / test_raw.py", + "and the file requesting it is cluster-bound") + + +def test_the_classifier_follows_a_conftest_fixture_that_connects_indirectly(tmp_path, expect): + """THE CASE THE CLOSURE OVER CONFTEST EXISTS FOR. A conftest fixture can reach + a cluster through a SIBLING and import nothing of its own, so a test requesting + only that fixture names no root and no driver anywhere. + + Without the closure the file reads as database-free and the gate would run it + against a database that is not there.""" + d = _fake_corpus(tmp_path, { + "test_indirect.py": "def test_it(pgc_readonly, expect):\n pass\n", + }, conftest=( + "import pytest\n" + "from pgc_cluster import make_cluster\n\n" + "@pytest.fixture(scope='session')\n" + "def pgc_cluster():\n" + " import psycopg\n" + " yield make_cluster()\n\n" + "@pytest.fixture\n" + "def pgc_readonly(pgc_cluster):\n" + " yield pgc_cluster\n" + )) + expect.text(" ".join(sorted(cluster_fixtures(d / "conftest.py"))), + "pgc_cluster pgc_readonly", + "a fixture reaching a cluster only through a sibling is a root too") + free, bound = partition(d) + expect.text(" ".join(free) + " / " + " ".join(bound), " / test_indirect.py", + "and the file requesting it is cluster-bound") + + +def test_the_classifier_does_not_read_a_helpers_parameter_as_a_fixture(tmp_path, expect): + """pytest resolves parameter names for tests and fixtures, not for helpers, so + a helper taking `pgc_conn` pulls in nothing.""" + d = _fake_corpus(tmp_path, { + "test_helper.py": ( + "def _plan(pgc_conn, sql):\n return sql\n\n" + "def test_it(expect):\n expect.text(_plan(None, 'x'), 'x', 'n')\n" + ), + }) + free, bound = partition(d) + expect.text(" ".join(free) + "/" + " ".join(bound), "test_helper.py/", + "a helper's parameter name is just a parameter name") + + +def test_the_classifier_follows_a_file_that_drives_a_cluster_bound_file(tmp_path, expect): + """This file's own shape. It requests no fixture, and it hands a cluster-bound + file to pytest as a subprocess, so it needs whatever that file needs.""" + d = _fake_corpus(tmp_path, { + "test_needy.py": "def test_it(pgc_conn, expect):\n pass\n", + "test_driver.py": ( + "import subprocess, sys\n\n" + "def test_it(expect):\n" + " subprocess.run([sys.executable, '-m', 'pytest', 'test_needy.py'])\n" + ), + }) + free, bound = partition(d) + expect.text(" ".join(free) + "/" + " ".join(bound), "/test_driver.py test_needy.py", + "driving a cluster-bound file is inherited") + + +def test_the_membership_report_names_a_database_free_file_left_undeclared(tmp_path, expect): + """THE HOLE, on a fixture. This is the exact shape that shipped: a file that + needs no database and is in nobody's list.""" + d = _fake_corpus(tmp_path, { + "test_free.py": "def test_it(expect):\n pass\n", + "test_bound.py": "def test_it(pgc_conn, expect):\n pass\n", + }) + expect.text(membership_report(d, []), "[1: undeclared:test_free.py]", + "an undeclared database-free file is named") + expect.text(membership_report(d, ["test_free.py"]), "[]", + "control: the same corpus with it declared is clean") + + +def test_the_membership_report_names_a_declared_file_that_needs_a_cluster(tmp_path, expect): + """The other direction: a listed file that starts using a cluster fixture + would make the gate's job fail for a reason nobody declared.""" + d = _fake_corpus(tmp_path, { + "test_free.py": "def test_it(expect):\n pass\n", + "test_bound.py": "def test_it(pgc_conn, expect):\n pass\n", + }) + expect.text(membership_report(d, ["test_free.py", "test_bound.py"]), + "[1: needs-a-cluster:test_bound.py]", + "a declared file that reaches a cluster is named") + + +def test_the_membership_report_names_a_declared_file_that_is_gone(tmp_path, expect): + """A rename used to be the only thing the old arm caught. It still is caught, + and as its own kind rather than as "needs a cluster".""" + d = _fake_corpus(tmp_path, { + "test_free.py": "def test_it(expect):\n pass\n", + }) + expect.text(membership_report(d, ["test_free.py", "test_renamed_away.py"]), + "[1: absent:test_renamed_away.py]", + "a declared file that does not exist is named") + + +def test_the_gate_runs_the_membership_decision_rather_than_only_this_file(expect): + """A guard that does not run is a comment, and nothing in the gate runs + pytest over this file: the corpus is not in `SUITES` (README.md), and the + `pytest-guards` job runs the database-free files, which this is not. + + So selftest 350 runs the decision through this module's command line. This + arm is what keeps that true.""" + part = (HERE.parent / "selftest" / "350-the-pytest-corpus-must-be.sh") + expect.text(repr(part.is_file()), "True", "premise: selftest 350 is where this expects") + text = part.read_text() + expect.at_least(text.count("--disagree"), 1, + "selftest 350 decides the membership in the gate") + # And the command line it uses must work, rather than being a string nobody + # ran: the same call, made here. + proc = subprocess.run([sys.executable, str(HERE / "test_harness_deps.py"), + "--disagree", str(HERE)], + capture_output=True, text=True) + expect.num(proc.returncode, 0, f"the command line runs: {proc.stderr[-300:]}") + expect.text(proc.stdout.strip(), "[]", + "and gives the same verdict as the arm above") + + # AND A MISUSE MUST BE A USAGE ERROR, not a traceback. An empty stdout is + # exactly what bash compares as a passing "[]", so a crash here would read as + # a clean corpus: the arm in selftest 350 would pass while deciding nothing. + bad = subprocess.run([sys.executable, str(HERE / "test_harness_deps.py"), + "--partition"], capture_output=True, text=True) + expect.num(bad.returncode, 2, f"a mode with no directory is a usage error: {bad.stderr[-200:]}") + expect.text(repr(bad.stdout), "''", "and it writes nothing to stdout") + expect.at_least(bad.stderr.count("usage:"), 1, "and says how to call it instead") def test_ci_derives_the_file_list_rather_than_repeating_it(expect): @@ -152,6 +737,22 @@ def test_ci_derives_the_file_list_rather_than_repeating_it(expect): expect.text(", ".join(hardcoded) or "none", "none", "the job names no corpus file literally") + # AND IT MUST STATE NO COUNT. A number in a comment is the same + # hand-maintained derived value as a number in a list, and it went stale the + # day it was written: the job's own comment said 152 while the corpus held + # 154. The job prints what it ran instead. + # + # OVER THE COMMENT BLOCK AS WELL AS THE JOB BODY. The slice above starts at + # the job key, and the comment explaining the job sits ABOVE that -- which is + # exactly where the stale count was, so a region ending at the job key could + # not see it. + region = text[text.index(JOB_COMMENT):] + region = region[:region.index("\n build:")] if "\n build:" in region else region + expect.at_least(len(region), len(job), "premise: the region includes the comment block") + counts = re.findall(r"\b\d+ (?:tests|files|of \d+)\b", region) + expect.text(", ".join(counts) or "none", "none", + "the job and its comment state no corpus count") + def test_the_job_installs_no_database_driver(expect): """The job's value is that it runs where there is no database. @@ -168,3 +769,7 @@ def test_the_job_installs_no_database_driver(expect): "the job installs no database driver") expect.at_least(job.count("pip show psycopg"), 1, "and it asserts the driver is absent rather than assuming it") + + +if __name__ == "__main__": + sys.exit(_main(sys.argv[1:])) diff --git a/test/selftest/350-the-pytest-corpus-must-be.sh b/test/selftest/350-the-pytest-corpus-must-be.sh index 40b3d775..429aceb8 100644 --- a/test/selftest/350-the-pytest-corpus-must-be.sh +++ b/test/selftest/350-the-pytest-corpus-must-be.sh @@ -196,7 +196,11 @@ _dcv_absent() { # _dcv_absent DIR DOC -> "[]" or "[n: a b c]" } | sort -u )" while IFS= read -r name; do [ -n "$name" ] || continue - printf '%s\n' "$ondisk" | grep -qxF "$name" && continue + # grep -cxF on a here-string, NOT `printf ... | grep -qxF`: grep -q exits on + # the first match, printf takes EPIPE, and under this suite's pipefail the + # pipeline reports failure though the name WAS present. Selftest 080 states + # the rule; measured here at 10 spurious absences in 40 runs under load. + [ "$(grep -cxF "$name" <<<"$ondisk" || true)" != 0 ] && continue n=$((n + 1)); [ "$n" -le 6 ] && bad="$bad $name" done < <(grep -oE '`test_[A-Za-z0-9_]*(\.py)?`' "$doc" 2>/dev/null \ | tr -d '`' | sort -u) @@ -268,11 +272,100 @@ check "control: distinct names in the same corpus report no duplicate" \ unset -f _dcv_dupes +# ---- and every table-of-contents link must RESOLVE --------------------------- +# +# WHY THIS EXISTS. TESTS.md's contents list gained an entry whose anchor stripped +# the underscores out of the file name -- "#14-testharnessdepspy-..." against a +# heading GitHub renders as "#14-test_harness_depspy-..." -- so the link silently +# went nowhere. Eleven entries above it keep the underscores, so the document +# already stated the convention and the new entry simply disagreed with it. +# +# NEITHER EXISTING ARM COULD SEE IT. Both sweep for NAMES; an anchor is not a +# name, and a broken link is still a string containing the file name it points at. +# A reader finds out by clicking. +# +# THE RULE IS GITHUB'S, and it is mechanical: lowercase the heading text, drop +# every character that is not a letter, digit, space, hyphen or underscore, then +# turn spaces into hyphens. So the dot in ".py" and the colon after it disappear +# and the underscores stay. Over the three documents in this directory the sweep +# reports nothing, and over the document as it shipped it reported exactly the one +# entry -- which is the whole false-positive budget, measured rather than assumed. +# +# ONE DIRECTION ON PURPOSE: every link must reach a heading. The reverse, every +# heading must be linked, is a different property, and "## Contents" is itself a +# heading that no entry links to -- so the reverse needs an exemption list, which +# is the hand-maintained value this file keeps removing. + +_toc_anchor() { # _toc_anchor TEXT -> the anchor GitHub derives from it + printf '%s' "$1" | tr 'A-Z' 'a-z' | sed -e 's/[^a-z0-9 _-]//g' -e 's/ /-/g' +} + +_toc_unresolved() { # _toc_unresolved DOC -> "[]" or "[n: a b c]" + local doc="$1" anchor n=0 bad="" anchors + # Every anchor the document's own headings produce, one per line. + # Through _toc_anchor, so GitHub's rule has ONE definition here. The stream + # form was a second copy of the same sed program, and two copies drift. + anchors="$(while IFS= read -r _h; do [ -n "$_h" ] && _toc_anchor "$_h" && echo; done \ + < <(grep -E '^#{2,} ' "$doc" 2>/dev/null | sed -e 's/^#* //'))" + while IFS= read -r anchor; do + [ -n "$anchor" ] || continue + # grep -cxF on a here-string, for the reason given in _dcv_absent above. + [ "$(grep -cxF "$anchor" <<<"$anchors" || true)" != 0 ] && continue + n=$((n + 1)); [ "$n" -le 6 ] && bad="$bad $anchor" + done < <(grep -oE '\]\(#[A-Za-z0-9_-]+\)' "$doc" 2>/dev/null \ + | sed -e 's/^](#//' -e 's/)$//' | sort -u) + [ "$n" -eq 0 ] && { printf '[]'; return; } + printf '[%d:%s]' "$n" "$bad" +} + +# PREMISE: the sweep found links at all. A document whose links it cannot parse +# reports "nothing broken", which is what a correct document reports too. +check "premise: the sweep reads the contents list's links" \ + "$([ "$(grep -coE '\]\(#[A-Za-z0-9_-]+\)' "$_dcv_doc")" -ge 15 ] \ + && echo enough || echo too-few)" "enough" + +# AND THE SWEEP MUST HAVE SWEPT. Without nullglob an unmatched glob stays literal, +# the [ -f ] skips it, and the loop below runs NO checks while the part still +# reports every check it did run as passing. A clean sweep needs a coverage premise. +_toc_n=0 +for _toc_f in "$_dcv_dir"/*.md; do + [ -f "$_toc_f" ] || continue + _toc_n=$((_toc_n + 1)) + check "every in-document link in ${_toc_f##*/} reaches a heading" \ + "$(_toc_unresolved "$_toc_f")" "[]" +done +check "premise: the link sweep saw the directory's documents" \ + "$([ "$_toc_n" -ge 3 ] && echo enough || echo "$_toc_n")" "enough" +unset _toc_f _toc_n + +# ---- and it must be able to FAIL, on a fixture rather than on the tree ------- + +printf '## 14. test_harness_deps.py: the harness\n- [14. x](#14-test_harness_depspy-the-harness)\n' \ + > "$_dcv_fix/ANCHOR_GOOD.md" +check "control: an anchor that keeps the underscores resolves" \ + "$(_toc_unresolved "$_dcv_fix/ANCHOR_GOOD.md")" "[]" + +# The exact shape that shipped: the underscores stripped out of the file name. +printf '## 14. test_harness_deps.py: the harness\n- [14. x](#14-testharnessdepspy-the-harness)\n' \ + > "$_dcv_fix/ANCHOR_BAD.md" +check "an anchor that strips the underscores is named, not passed over" \ + "$(_toc_unresolved "$_dcv_fix/ANCHOR_BAD.md")" \ + "[1: 14-testharnessdepspy-the-harness]" + +# And the derivation itself, on the heading this defect was found in: the dot and +# the colon go, the underscores stay. +check "the anchor rule drops punctuation and keeps underscores" \ + "$(_toc_anchor '14. test_harness_deps.py: the harness must self-test')" \ + "14-test_harness_depspy-the-harness-must-self-test" + +unset -f _toc_anchor _toc_unresolved + + # ---- the harness must self-test without a database, and the gate must run it -- # # conftest.py imported psycopg AT MODULE SCOPE, and conftest is imported before # every run, so a DATABASE DRIVER was a hard requirement of the whole corpus -- -# including the 61 tests that never open a connection. With psycopg absent the run +# including every test that never opens a connection. With psycopg absent the run # did not fail a test, it failed to COLLECT. # # That coupling was half of why README.md says the corpus is "not in the gate @@ -321,7 +414,70 @@ check "and it derives the file list rather than repeating it" \ check "and derives the pins from requirements-test.txt" \ "$([ "$(grep -c 'requirements-test.txt' "$_hd_ci")" -ge 1 ] && echo yes || echo no)" "yes" -unset _hd_conf _hd_ci +# ---- and the DECLARATION must be decided, not declaimed ---------------------- +# +# WHAT WAS WRONG. `NO_CLUSTER` in test_harness_deps.py says which files need no +# database, and the job runs exactly those. The only arm over it asked whether the +# files it names EXIST. That is one direction, and the missing direction is the one +# that loses coverage: a database-free file nobody adds to the list is absent from +# the job, every arm stays green, and NOTHING says so. It had already happened +# twice -- test_build_refusal.py and test_layer.py both need no database and +# neither was listed. +# +# The module decides the property from the corpus with an ast walk and requires set +# equality with the list. WHY HERE: nothing in the gate runs pytest over that file. +# The corpus is not in SUITES, and the pytest-guards job runs the database-free +# files, which test_harness_deps.py is not -- its control arm needs a real cluster. +# So the module exposes the decision on its command line and this part runs it. +# Same move as the documentation sweep above: the corpus carries a twin, this copy +# has the teeth. +_hd_decide="$PGC_TESTDIR/pytest/test_harness_deps.py" + +check "premise: the membership decider is where this part thinks it is" \ + "$([ -f "$_hd_decide" ] && echo yes || echo no)" "yes" + +check "every database-free file in the corpus is declared in NO_CLUSTER" \ + "$(python3 "$_hd_decide" --disagree "$PGC_TESTDIR/pytest" 2>&1)" "[]" + +# PREMISE: the decider SPLIT the corpus. One that parsed nothing reports an empty +# database-free set, which agrees with an empty list and looks like success; one +# that called everything database-free would pass a one-directional check. +_hd_part="$(python3 "$_hd_decide" --partition "$PGC_TESTDIR/pytest" 2>&1)" +check "premise: the decider partitions the corpus into two non-empty halves" \ + "$(printf '%s' "$_hd_part" | grep -cE 'free: test_[^|]+ \| bound: test_')" "1" + +echo " CORPUS: $_hd_part" + +# ---- and that arm must be able to FAIL, on a fixture rather than on the tree -- +# +# Everything above passes on a healthy tree, which is what a decider returning a +# constant also does. The fixture is a two-file corpus with a conftest shaped like +# the real one: one fixture imports the driver, one depends on it. +_hd_fix="$PGC_WORKDIR/nocluster"; rm -rf "$_hd_fix"; mkdir -p "$_hd_fix" +{ + printf 'import pytest\n' + printf 'from pgc_cluster import make_cluster\n\n' + printf '@pytest.fixture\ndef pgc_conn():\n import psycopg\n yield make_cluster()\n' +} > "$_hd_fix/conftest.py" +printf 'def test_it(expect):\n pass\n' > "$_hd_fix/test_free.py" +printf 'def test_it(pgc_conn, expect):\n pass\n' > "$_hd_fix/test_bound.py" + +check "the fixture corpus splits the way the property says" \ + "$(python3 "$_hd_decide" --partition "$_hd_fix")" \ + "free: test_free.py | bound: test_bound.py" + +check "a database-free file declared nowhere is named, not passed over" \ + "$(python3 "$_hd_decide" --disagree "$_hd_fix" test_bound.py)" \ + "[2: needs-a-cluster:test_bound.py undeclared:test_free.py]" + +check "control: the same corpus with the right declaration is clean" \ + "$(python3 "$_hd_decide" --disagree "$_hd_fix" test_free.py)" "[]" + +check "a declared file that no longer exists is named" \ + "$(python3 "$_hd_decide" --disagree "$_hd_fix" test_free.py test_renamed.py)" \ + "[1: absent:test_renamed.py]" + +unset _hd_conf _hd_ci _hd_decide _hd_part _hd_fix unset _dcv_dir _dcv_doc _dcv_seen _dcv_stated _dcv_fix _dcv_ht unset -f _dcv_missing _dcv_count From b10e3f8a583dbe9293d98f312088d6f1dc53b688 Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Thu, 10 Sep 2026 17:15:31 +0000 Subject: [PATCH 3/6] test/pytest: the classifier sees five more dependency forms, and its own controls now run in the gate (#432) @linuxhikerpm blocked this with three items. All three were real and all three are fixed. The first two I reproduced before fixing; the third I verified is mine. 1. THE CLASSIFIER'S OWN CONTROLS DID NOT RUN IN THE GATE THEY GOVERN. test_harness_deps.py defines the classifier that computes NO_CLUSTER, and the `pytest (harness guards, no database)` job runs exactly NO_CLUSTER -- which does not contain that file, and correctly so: it hands real cluster-bound file names to pytest in a subprocess, so it needs whatever they need. The code deciding the job's contents was the one thing the job could not check. The eight synthetic classifier arms are now in test_harness_deps_classifier.py, which drives a corpus it writes in tmp_path, reads nothing from the real tree, is declared in NO_CLUSTER, and imports the classifier as a LIBRARY -- which does not make it cluster-bound, because the propagation rule reads string constants naming corpus files, not imports. PROVED, which is what the review asked for. Neutering the transitive conftest-fixture closure: before this change the gated job stayed GREEN, selftest 350 stayed green, and only the excluded targeted test failed after this change the gated job goes RED, naming four arms in test_harness_deps_classifier.py 2. FIVE ORDINARY PYTEST DEPENDENCY FORMS WERE CLASSIFIED DATABASE-FREE. `_defs` walked `tree.body` and `args.args` only. Measured against the classifier as it was, each a DIRECT dependency on a cluster root: a test method inside a class free -> bound @pytest.mark.usefixtures free -> bound a keyword-only fixture parameter free -> bound request.getfixturevalue free -> bound an ALIASED cluster root in conftest free -> bound module-level positional bound (the shape it did see) a helper's fixture-named parameter free (and must stay free) a plain test, and `self` free (and must stay free) THE FIFTH NEEDED A SHAPE THE REVIEW DID NOT GIVE, and this matters because I nearly recorded it as not reproducing. An alias on the TEST side is caught anyway: the underlying fixture still takes the root positionally. It is an alias on the ROOT, in conftest, that hides it -- the root was recorded under the def's name while a test requests it under the alias. Measured: roots=['_mk'] before, roots=['conn'] after, and the test requesting `conn` goes free -> bound. `request.getfixturevalue` is NOT supported and NOT banned. The name is computed at run time, so no AST can resolve it, and a file using it is classified CLUSTER-BOUND: wrong in the direction that costs CI time, rather than the direction that greens a gate over tests nothing ran. `self` and `cls` are dropped, because they are bound by Python and no fixture can be requested under either name. Every class is walked rather than only `Test*` ones: a classifier that guesses the collection convention is one convention change from being wrong, and counting a non-collected method errs safely. 3. .shim/psycopg.py WAS AN AUDIT ARTIFACT AND IS GONE. It holds `raise ImportError("psycopg shimmed out by the gate probe")`, it is present at 7d51838 and absent in BOTH parents, so the merge commit added it. Mine, from probing the no-driver path by hand. Removed. MEASURED full pytest corpus 188 passed the gated set exactly as CI runs it 8 files, 141 passed, psycopg absent harness_selftest 561 checks, 561 passed + 0 failed, rc 0 partition / NO_CLUSTER agree in both directions, disagreements [] the real corpus's classification unchanged by the widening Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- .shim/psycopg.py | 1 - test/pytest/TESTS.md | 78 +++++- test/pytest/test_harness_deps.py | 282 ++++++++------------ test/pytest/test_harness_deps_classifier.py | 275 +++++++++++++++++++ 4 files changed, 464 insertions(+), 172 deletions(-) delete mode 100644 .shim/psycopg.py create mode 100644 test/pytest/test_harness_deps_classifier.py diff --git a/.shim/psycopg.py b/.shim/psycopg.py deleted file mode 100644 index c5477549..00000000 --- a/.shim/psycopg.py +++ /dev/null @@ -1 +0,0 @@ -raise ImportError("psycopg shimmed out by the gate probe") diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 2f3890e4..dc32768b 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -61,9 +61,10 @@ behaviour, the source of that number is named. - [13. test_hilbert_locality.py: what the Hilbert curve buys](#13-test_hilbert_localitypy-what-the-hilbert-curve-buys) - [14. test_suite_accounting.py: the matrix accounting for its own suites](#14-test_suite_accountingpy-the-matrix-accounting-for-its-own-suites) - [15. test_harness_deps.py: the harness must self-test without a database](#15-test_harness_depspy-the-harness-must-self-test-without-a-database) -- [16. Adding a test](#16-adding-a-test) -- [17. What this corpus does NOT yet refuse](#17-what-this-corpus-does-not-yet-refuse) -- [18. Traps this corpus records](#18-traps-this-corpus-records) +- [16. test_harness_deps_classifier.py: the classifier, in the file the gate runs](#16-test_harness_deps_classifierpy-the-classifier-in-the-file-the-gate-runs) +- [17. Adding a test](#17-adding-a-test) +- [18. What this corpus does NOT yet refuse](#18-what-this-corpus-does-not-yet-refuse) +- [19. Traps this corpus records](#19-traps-this-corpus-records) ## 1. How to read a test in here @@ -1239,7 +1240,72 @@ environment broken if the test died. A module that raises on import, first on th path, is the same observation and reversible by construction. Both behavioural arms assert the shim actually bites before believing anything it produces. -## 16. Adding a test +## 16. test_harness_deps_classifier.py: the classifier, in the file the gate runs + +`test_harness_deps.py` defines the classifier that decides which files the +`pytest (harness guards, no database)` job runs — and that file is itself classified +cluster-bound, correctly, because it hands real cluster-bound file names to pytest in +a subprocess. So the job's file list is `NO_CLUSTER`, and the code that computes +`NO_CLUSTER` was the one thing the job never ran. + +@linuxhikerpm measured the consequence: disabling transitive conftest-fixture closure +left shell selftest 350 green and every CI-selected test passing, and only the excluded +targeted test failed. A load-bearing branch could break with both real gates green. + +These arms drive a corpus they write in `tmp_path` and read nothing from the real tree, +so this file needs no database and is declared in `NO_CLUSTER`. It imports the +classifier as a **library**, which does not make it cluster-bound: the propagation rule +reads string constants naming corpus files, not imports. + +### The classifier's branches + +| test | what it asserts | +| --- | --- | +| `test_the_classifier_tells_a_plain_file_from_one_that_requests_a_cluster` | the base case and its control | +| `test_the_classifier_follows_a_cluster_fixture_through_a_local_wrapper` | a local fixture wrapping a conftest root | +| `test_the_classifier_catches_a_module_scope_driver_import` | importing the driver at module scope is enough | +| `test_the_classifier_is_not_fooled_by_prose_that_names_the_driver` | a docstring naming psycopg is not an import | +| `test_the_classifier_takes_a_fixture_that_provisions_without_connecting` | a fixture that provisions but never connects is still a root | +| `test_the_classifier_follows_a_conftest_fixture_that_connects_indirectly` | the transitive closure inside conftest, which is the branch that was ungated | +| `test_the_classifier_does_not_read_a_helpers_parameter_as_a_fixture` | a helper's parameter is not a request | +| `test_the_classifier_follows_a_file_that_drives_a_cluster_bound_file` | driving another file inherits what it needs | + +### Ordinary pytest dependency forms + +The classifier read module-level `def`s and positional parameters. pytest resolves a +fixture through five more shapes, and @linuxhikerpm built a direct fixture in each: +every one was classified database-free and then failed under the no-driver shim. +Measured against the classifier as it was: + +| form | before | after | +| --- | --- | --- | +| a test method inside a class | **free** | bound | +| `@pytest.mark.usefixtures` | **free** | bound | +| a keyword-only fixture parameter | **free** | bound | +| `request.getfixturevalue` | **free** | bound | +| an aliased cluster root in `conftest.py` | **free** | bound | +| module-level positional | bound | bound | + +The fifth needed a shape the review did not give. An alias on the *test* side is caught +anyway, because the underlying fixture still takes the root positionally; it is an alias +on the **root**, in conftest, that hides it — the root was recorded under the def's name +while a test requests it under the alias. Measured: `roots=['_mk']` before, `roots=['conn']` after. + +`request.getfixturevalue` is not supported and not banned. The name is computed at run +time, so no AST can resolve it, and such a file is classified **cluster-bound** — +wrong in the direction that costs CI time rather than the direction that greens a gate +over tests nothing ran. + +| test | what it asserts | +| --- | --- | +| `test_a_test_method_inside_a_class_is_a_fixture_request` | pytest collects `test_*` methods of a class | +| `test_usefixtures_is_a_fixture_request_without_a_parameter` | a dependency with no parameter | +| `test_a_keyword_only_parameter_is_a_fixture_request` | `def test_x(*, pgc_conn)` | +| `test_a_dynamic_request_is_treated_as_cluster_bound` | unresolvable means conservative, not free | +| `test_an_aliased_cluster_root_is_found_under_the_name_tests_request` | the root is read under its requestable name | +| `test_a_plain_test_and_a_helpers_parameter_stay_database_free` | the cost side: a rule that calls everything bound would empty the gate | + +## 17. Adding a test 0. **Write it twice.** Every test in this tree ships as a `.sh` suite and a pytest test **in the same change** (jd, 2026-09-09). Not ported later, not one or the @@ -1266,7 +1332,7 @@ arms assert the shim actually bites before believing anything it produces. failed the selftest on both majors of the matrix, which is how it was found. A new directory under `test/` inherits every rule the old ones follow. -## 17. What this corpus does NOT yet refuse +## 18. What this corpus does NOT yet refuse `VACUITY_MODES.md` is the inventory: 79 ways a pytest harness can report a pass while asserting nothing, 73 of them demonstrated by an actual run. **This layer refuses 25 @@ -1278,7 +1344,7 @@ Read it before adding a test. The gaps most likely to affect a new test are that same family satisfies it, and that a write is not required to have written anything. Both are named there with the refusal each needs. -## 18. Traps this corpus records +## 19. Traps this corpus records Recorded because each one produced a confident wrong result before it was caught, and all are the same family as the defect the layer exists to prevent. diff --git a/test/pytest/test_harness_deps.py b/test/pytest/test_harness_deps.py index 906f5396..dce36ea9 100644 --- a/test/pytest/test_harness_deps.py +++ b/test/pytest/test_harness_deps.py @@ -71,6 +71,10 @@ # Landed on main in #922 after this list was written, and the arm above caught # it: the property says it needs no database, so the declaration must say so too. "test_suite_accounting.py", + # The classifier's own controls, split out of this file so the job whose file + # list IS this list actually runs them. This file cannot be in the list: it + # hands cluster-bound file names to pytest, so it needs what they need. + "test_harness_deps_classifier.py", ] @@ -166,24 +170,122 @@ def _is_fixture(fn): return False +def _fixture_name(fn): + """The name a test REQUESTS this fixture by: the alias when it has one. + + `@pytest.fixture(name="conn")` makes the function requestable as `conn` and NOT + as its own name. Recording the def's name therefore did two wrong things at + once: it missed the dependency a test declares, and it invented a fixture name + nothing can request. Reported by @linuxhikerpm. + """ + for dec in fn.decorator_list: + if not isinstance(dec, ast.Call): + continue + f = dec.func + if (isinstance(f, ast.Attribute) and f.attr == "fixture") or \ + (isinstance(f, ast.Name) and f.id == "fixture"): + for kw in dec.keywords: + if kw.arg == "name" and isinstance(kw.value, ast.Constant) \ + and isinstance(kw.value.value, str): + return kw.value.value + return fn.name + + +def _usefixtures(fn): + """Fixture names pulled in by `@pytest.mark.usefixtures(...)`. + + A dependency with NO PARAMETER, so a walk over the signature cannot see it. + This is the form a test uses precisely when it wants the fixture's effect and + not its value -- which is exactly when it is a cluster it wants. + """ + out = [] + for dec in fn.decorator_list: + if not isinstance(dec, ast.Call): + continue + f = dec.func + if (isinstance(f, ast.Attribute) and f.attr == "usefixtures") or \ + (isinstance(f, ast.Name) and f.id == "usefixtures"): + out += [a.value for a in dec.args + if isinstance(a, ast.Constant) and isinstance(a.value, str)] + return out + + +def _params(fn): + """Every parameter pytest will try to resolve as a fixture. + + POSITIONAL-ONLY, POSITIONAL, AND KEYWORD-ONLY. pytest resolves a keyword-only + parameter as a fixture exactly as it resolves a positional one; reading only + `args.args` classified `def test_x(*, pgc_conn)` as needing nothing at all. + + `self` and `cls` are dropped: they are bound by Python, not by pytest, and a + fixture cannot be requested under either name. + """ + a = fn.args + names = ([q.arg for q in getattr(a, "posonlyargs", [])] + + [q.arg for q in a.args] + + [q.arg for q in a.kwonlyargs]) + return [n for n in names if n not in ("self", "cls")] + + +def _collectable(tree): + """(qualifier, def) for every def pytest can collect or resolve. + + MODULE LEVEL AND CLASS BODIES. pytest collects `test_*` methods of a class and + resolves their fixtures identically, so a walk over `tree.body` alone + classified `class TestX: def test_y(self, pgc_conn)` as needing no database. + Every class is walked rather than only `Test*`-named ones: a classifier that + guesses the collection convention is one convention change from being wrong, + and counting a non-collected method is conservative in the safe direction. + """ + def walk(node, prefix): + for n in node.body: + if isinstance(n, (ast.FunctionDef, ast.AsyncFunctionDef)): + yield prefix, n + elif isinstance(n, ast.ClassDef): + yield from walk(n, prefix + n.name + ".") + return list(walk(tree, "")) + + def _defs(tree): - """{name: (kind, params, body)} for every module-level def. + """{key: (kind, params, body)} for every def pytest can collect or resolve. kind is "fixture", "test" or "helper". Only the first two can pull a fixture in: pytest resolves parameter names for those, and a helper's parameter is just a parameter -- so a helper taking `conn` named after a fixture must not - make its file cluster-bound.""" + make its file cluster-bound. + + A FIXTURE IS KEYED BY ITS REQUESTABLE NAME, because that is the name another + def names to depend on it, and the closure below matches keys against + parameters. Tests and helpers are keyed by their qualified name instead: those + names are never requested, and two classes may both define `test_x`, which a + bare-name key would collapse into one -- silently dropping a def from the walk. + """ out = {} - for n in tree.body: - if not isinstance(n, (ast.FunctionDef, ast.AsyncFunctionDef)): - continue + for prefix, n in _collectable(tree): if _is_fixture(n): - kind = "fixture" + kind, key = "fixture", _fixture_name(n) elif n.name.startswith("test_"): - kind = "test" + kind, key = "test", prefix + n.name else: - kind = "helper" - out[n.name] = (kind, [a.arg for a in n.args.args], _own_body(n)) + kind, key = "helper", prefix + n.name + out[key] = (kind, _params(n) + _usefixtures(n), _own_body(n)) + return out + + +def dynamic_requests(tree): + """Call sites of `request.getfixturevalue(...)`, which no AST can resolve. + + The name is computed at run time, so a static classifier cannot know which + fixture is pulled -- and the honest answer is not to ban the form but to stop + claiming a file that uses it needs no database. `partition` treats such a file + as cluster-bound, which is wrong only in the direction that costs a little CI + time rather than the direction that reports a green gate for tests nothing ran. + """ + out = [] + for n in ast.walk(tree): + if isinstance(n, ast.Call) and isinstance(n.func, ast.Attribute) \ + and n.func.attr == "getfixturevalue": + out.append(n.lineno) return out @@ -268,7 +370,12 @@ def partition(directory=None): # kills it there. uses = any(kind in ("fixture", "test") and set(params) & roots for kind, params, _body in defs.values()) - bound[name] = _imports_driver(_module_scope(tree)) or uses + # A dynamic request is unresolvable, so the file is treated as + # cluster-bound rather than assumed free. Wrong in the direction that + # costs CI time, not in the direction that greens a gate over tests + # nothing ran. + dynamic = bool(dynamic_requests(tree)) + bound[name] = _imports_driver(_module_scope(tree)) or uses or dynamic mentions[name] = _mentioned_files(tree, set(names) - {name}) # A file that drives a cluster-bound file needs whatever that file needs. @@ -489,161 +596,6 @@ def _fake_corpus(tmp_path, files, conftest=None): return d -def test_the_classifier_tells_a_plain_file_from_one_that_requests_a_cluster(tmp_path, expect): - """The base case and its control, over a conftest this test wrote: the - derivation of the cluster fixtures runs here too, on a fixture rather than on - the real corpus.""" - d = _fake_corpus(tmp_path, { - "test_plain.py": "def test_nothing(expect):\n expect.num(1, 1, 'x')\n", - "test_direct.py": "def test_it(pgc_conn, expect):\n pass\n", - }) - expect.text(" ".join(sorted(cluster_fixtures(d / "conftest.py"))), - "pgc_cluster pgc_conn", - "both connecting fixtures are derived, including the indirect one") - free, bound = partition(d) - expect.text(" ".join(free) + " / " + " ".join(bound), - "test_plain.py / test_direct.py", - "a test requesting a cluster fixture is bound; one requesting none is free") - - -def test_the_classifier_follows_a_cluster_fixture_through_a_local_wrapper(tmp_path, expect): - """THE CASE A GREP CANNOT SEE. The file names no fixture of conftest's in any - test; a module-local fixture does, and the tests request that.""" - d = _fake_corpus(tmp_path, { - "test_wrapped.py": ( - "import pytest\n\n" - "@pytest.fixture\n" - "def loaded(pgc_cluster):\n" - " yield pgc_cluster\n\n" - "def test_it(loaded, expect):\n" - " pass\n" - ), - }) - free, bound = partition(d) - expect.text(" ".join(free) + "/" + " ".join(bound), "/test_wrapped.py", - "a cluster fixture reached through a local fixture is followed") - - -def test_the_classifier_catches_a_module_scope_driver_import(tmp_path, expect): - """An import at module scope kills COLLECTION, so the file cannot run in the - job even though no test of its own asks for a connection.""" - d = _fake_corpus(tmp_path, { - "test_eager.py": "import psycopg\n\ndef test_it(expect):\n pass\n", - }) - free, bound = partition(d) - expect.text(" ".join(free) + "/" + " ".join(bound), "/test_eager.py", - "a module-scope driver import makes the file cluster-bound") - - -def test_the_classifier_is_not_fooled_by_prose_that_names_the_driver(tmp_path, expect): - """THE REGEX TRAP, and this layer has paid for it once already: the - broad-except refusal was first written as a line regex and rejected its own - tests, because the forbidden shape appears inside a `makepyfile` string. - - A docstring naming the driver, a block-comment string naming a cluster - fixture, and a test BUILT as a string are all prose about code.""" - d = _fake_corpus(tmp_path, { - "test_needsdb.py": "def test_it(pgc_conn, expect):\n pass\n", - "test_prose.py": ( - '"""This file explains `import psycopg` and the pgc_conn fixture,\n' - 'and it discusses test_needsdb.py, which does need a cluster."""\n' - "\n" - "def test_it(pytester, expect):\n" - ' """It uses pgc_conn nowhere; it writes a test that would."""\n' - ' "a block comment mentioning import psycopg and pgc_cluster"\n' - " pytester.makepyfile(\n" - ' "import psycopg\\n"\n' - ' "def test_inner(pgc_conn):\\n pass\\n"\n' - " )\n" - ), - }) - free, bound = partition(d) - expect.text(" ".join(free) + " / " + " ".join(bound), - "test_prose.py / test_needsdb.py", - "prose, a generated test, and a file merely DISCUSSED are not requirements") - - -def test_the_classifier_takes_a_fixture_that_provisions_without_connecting(tmp_path, expect): - """THE SECOND SIGNAL, isolated. A fixture can start a cluster and hand back the - object without ever connecting, so it imports no driver: the only thing that - marks it is the call into the module that provisions a server. - - Its own arm rather than a clause in the one below, because a fixture that - neither imports nor calls would leave two rules untested at once.""" - d = _fake_corpus(tmp_path, { - "test_raw.py": "def test_it(pgc_raw, expect):\n pass\n", - }, conftest=( - "import pytest\n" - "from pgc_cluster import make_cluster\n\n" - "@pytest.fixture(scope='session')\n" - "def pgc_raw():\n" - " yield make_cluster()\n" - )) - expect.text(" ".join(sorted(cluster_fixtures(d / "conftest.py"))), "pgc_raw", - "a fixture that provisions a cluster is a root without importing a driver") - free, bound = partition(d) - expect.text(" ".join(free) + " / " + " ".join(bound), " / test_raw.py", - "and the file requesting it is cluster-bound") - - -def test_the_classifier_follows_a_conftest_fixture_that_connects_indirectly(tmp_path, expect): - """THE CASE THE CLOSURE OVER CONFTEST EXISTS FOR. A conftest fixture can reach - a cluster through a SIBLING and import nothing of its own, so a test requesting - only that fixture names no root and no driver anywhere. - - Without the closure the file reads as database-free and the gate would run it - against a database that is not there.""" - d = _fake_corpus(tmp_path, { - "test_indirect.py": "def test_it(pgc_readonly, expect):\n pass\n", - }, conftest=( - "import pytest\n" - "from pgc_cluster import make_cluster\n\n" - "@pytest.fixture(scope='session')\n" - "def pgc_cluster():\n" - " import psycopg\n" - " yield make_cluster()\n\n" - "@pytest.fixture\n" - "def pgc_readonly(pgc_cluster):\n" - " yield pgc_cluster\n" - )) - expect.text(" ".join(sorted(cluster_fixtures(d / "conftest.py"))), - "pgc_cluster pgc_readonly", - "a fixture reaching a cluster only through a sibling is a root too") - free, bound = partition(d) - expect.text(" ".join(free) + " / " + " ".join(bound), " / test_indirect.py", - "and the file requesting it is cluster-bound") - - -def test_the_classifier_does_not_read_a_helpers_parameter_as_a_fixture(tmp_path, expect): - """pytest resolves parameter names for tests and fixtures, not for helpers, so - a helper taking `pgc_conn` pulls in nothing.""" - d = _fake_corpus(tmp_path, { - "test_helper.py": ( - "def _plan(pgc_conn, sql):\n return sql\n\n" - "def test_it(expect):\n expect.text(_plan(None, 'x'), 'x', 'n')\n" - ), - }) - free, bound = partition(d) - expect.text(" ".join(free) + "/" + " ".join(bound), "test_helper.py/", - "a helper's parameter name is just a parameter name") - - -def test_the_classifier_follows_a_file_that_drives_a_cluster_bound_file(tmp_path, expect): - """This file's own shape. It requests no fixture, and it hands a cluster-bound - file to pytest as a subprocess, so it needs whatever that file needs.""" - d = _fake_corpus(tmp_path, { - "test_needy.py": "def test_it(pgc_conn, expect):\n pass\n", - "test_driver.py": ( - "import subprocess, sys\n\n" - "def test_it(expect):\n" - " subprocess.run([sys.executable, '-m', 'pytest', 'test_needy.py'])\n" - ), - }) - free, bound = partition(d) - expect.text(" ".join(free) + "/" + " ".join(bound), "/test_driver.py test_needy.py", - "driving a cluster-bound file is inherited") - - def test_the_membership_report_names_a_database_free_file_left_undeclared(tmp_path, expect): """THE HOLE, on a fixture. This is the exact shape that shipped: a file that needs no database and is in nobody's list.""" diff --git a/test/pytest/test_harness_deps_classifier.py b/test/pytest/test_harness_deps_classifier.py new file mode 100644 index 00000000..eb8b246d --- /dev/null +++ b/test/pytest/test_harness_deps_classifier.py @@ -0,0 +1,275 @@ +"""The classifier's own controls, in the file the gate actually runs (#432). + +WHY THIS FILE IS SEPARATE FROM `test_harness_deps.py`, which defines the classifier +it tests. That file is classified cluster-bound -- correctly, because it hands real +cluster-bound file names to pytest in a subprocess -- so the `pytest (harness guards, +no database)` job, whose file list IS `NO_CLUSTER`, never ran it. The classifier that +decides the job's contents was therefore the one thing the job could not check. + +@linuxhikerpm measured the consequence: disabling transitive conftest-fixture closure +left shell selftest 350 green and every CI-selected test passing, and only the +excluded targeted test failed. A load-bearing branch could break with both real gates +green. + +These arms drive a corpus they write in `tmp_path` and read nothing from the real +tree, so they need no database and this file is declared in `NO_CLUSTER`. It imports +the classifier as a LIBRARY, which does not make it cluster-bound: the propagation +rule reads string constants naming corpus files, not imports. +""" + +from test_harness_deps import _fake_corpus, cluster_fixtures, partition + +def test_the_classifier_tells_a_plain_file_from_one_that_requests_a_cluster(tmp_path, expect): + """The base case and its control, over a conftest this test wrote: the + derivation of the cluster fixtures runs here too, on a fixture rather than on + the real corpus.""" + d = _fake_corpus(tmp_path, { + "test_plain.py": "def test_nothing(expect):\n expect.num(1, 1, 'x')\n", + "test_direct.py": "def test_it(pgc_conn, expect):\n pass\n", + }) + expect.text(" ".join(sorted(cluster_fixtures(d / "conftest.py"))), + "pgc_cluster pgc_conn", + "both connecting fixtures are derived, including the indirect one") + free, bound = partition(d) + expect.text(" ".join(free) + " / " + " ".join(bound), + "test_plain.py / test_direct.py", + "a test requesting a cluster fixture is bound; one requesting none is free") + +def test_the_classifier_follows_a_cluster_fixture_through_a_local_wrapper(tmp_path, expect): + """THE CASE A GREP CANNOT SEE. The file names no fixture of conftest's in any + test; a module-local fixture does, and the tests request that.""" + d = _fake_corpus(tmp_path, { + "test_wrapped.py": ( + "import pytest\n\n" + "@pytest.fixture\n" + "def loaded(pgc_cluster):\n" + " yield pgc_cluster\n\n" + "def test_it(loaded, expect):\n" + " pass\n" + ), + }) + free, bound = partition(d) + expect.text(" ".join(free) + "/" + " ".join(bound), "/test_wrapped.py", + "a cluster fixture reached through a local fixture is followed") + +def test_the_classifier_catches_a_module_scope_driver_import(tmp_path, expect): + """An import at module scope kills COLLECTION, so the file cannot run in the + job even though no test of its own asks for a connection.""" + d = _fake_corpus(tmp_path, { + "test_eager.py": "import psycopg\n\ndef test_it(expect):\n pass\n", + }) + free, bound = partition(d) + expect.text(" ".join(free) + "/" + " ".join(bound), "/test_eager.py", + "a module-scope driver import makes the file cluster-bound") + +def test_the_classifier_is_not_fooled_by_prose_that_names_the_driver(tmp_path, expect): + """THE REGEX TRAP, and this layer has paid for it once already: the + broad-except refusal was first written as a line regex and rejected its own + tests, because the forbidden shape appears inside a `makepyfile` string. + + A docstring naming the driver, a block-comment string naming a cluster + fixture, and a test BUILT as a string are all prose about code.""" + d = _fake_corpus(tmp_path, { + "test_needsdb.py": "def test_it(pgc_conn, expect):\n pass\n", + "test_prose.py": ( + '"""This file explains `import psycopg` and the pgc_conn fixture,\n' + 'and it discusses test_needsdb.py, which does need a cluster."""\n' + "\n" + "def test_it(pytester, expect):\n" + ' """It uses pgc_conn nowhere; it writes a test that would."""\n' + ' "a block comment mentioning import psycopg and pgc_cluster"\n' + " pytester.makepyfile(\n" + ' "import psycopg\\n"\n' + ' "def test_inner(pgc_conn):\\n pass\\n"\n' + " )\n" + ), + }) + free, bound = partition(d) + expect.text(" ".join(free) + " / " + " ".join(bound), + "test_prose.py / test_needsdb.py", + "prose, a generated test, and a file merely DISCUSSED are not requirements") + +def test_the_classifier_takes_a_fixture_that_provisions_without_connecting(tmp_path, expect): + """THE SECOND SIGNAL, isolated. A fixture can start a cluster and hand back the + object without ever connecting, so it imports no driver: the only thing that + marks it is the call into the module that provisions a server. + + Its own arm rather than a clause in the one below, because a fixture that + neither imports nor calls would leave two rules untested at once.""" + d = _fake_corpus(tmp_path, { + "test_raw.py": "def test_it(pgc_raw, expect):\n pass\n", + }, conftest=( + "import pytest\n" + "from pgc_cluster import make_cluster\n\n" + "@pytest.fixture(scope='session')\n" + "def pgc_raw():\n" + " yield make_cluster()\n" + )) + expect.text(" ".join(sorted(cluster_fixtures(d / "conftest.py"))), "pgc_raw", + "a fixture that provisions a cluster is a root without importing a driver") + free, bound = partition(d) + expect.text(" ".join(free) + " / " + " ".join(bound), " / test_raw.py", + "and the file requesting it is cluster-bound") + +def test_the_classifier_follows_a_conftest_fixture_that_connects_indirectly(tmp_path, expect): + """THE CASE THE CLOSURE OVER CONFTEST EXISTS FOR. A conftest fixture can reach + a cluster through a SIBLING and import nothing of its own, so a test requesting + only that fixture names no root and no driver anywhere. + + Without the closure the file reads as database-free and the gate would run it + against a database that is not there.""" + d = _fake_corpus(tmp_path, { + "test_indirect.py": "def test_it(pgc_readonly, expect):\n pass\n", + }, conftest=( + "import pytest\n" + "from pgc_cluster import make_cluster\n\n" + "@pytest.fixture(scope='session')\n" + "def pgc_cluster():\n" + " import psycopg\n" + " yield make_cluster()\n\n" + "@pytest.fixture\n" + "def pgc_readonly(pgc_cluster):\n" + " yield pgc_cluster\n" + )) + expect.text(" ".join(sorted(cluster_fixtures(d / "conftest.py"))), + "pgc_cluster pgc_readonly", + "a fixture reaching a cluster only through a sibling is a root too") + free, bound = partition(d) + expect.text(" ".join(free) + " / " + " ".join(bound), " / test_indirect.py", + "and the file requesting it is cluster-bound") + +def test_the_classifier_does_not_read_a_helpers_parameter_as_a_fixture(tmp_path, expect): + """pytest resolves parameter names for tests and fixtures, not for helpers, so + a helper taking `pgc_conn` pulls in nothing.""" + d = _fake_corpus(tmp_path, { + "test_helper.py": ( + "def _plan(pgc_conn, sql):\n return sql\n\n" + "def test_it(expect):\n expect.text(_plan(None, 'x'), 'x', 'n')\n" + ), + }) + free, bound = partition(d) + expect.text(" ".join(free) + "/" + " ".join(bound), "test_helper.py/", + "a helper's parameter name is just a parameter name") + +def test_the_classifier_follows_a_file_that_drives_a_cluster_bound_file(tmp_path, expect): + """This file's own shape. It requests no fixture, and it hands a cluster-bound + file to pytest as a subprocess, so it needs whatever that file needs.""" + d = _fake_corpus(tmp_path, { + "test_needy.py": "def test_it(pgc_conn, expect):\n pass\n", + "test_driver.py": ( + "import subprocess, sys\n\n" + "def test_it(expect):\n" + " subprocess.run([sys.executable, '-m', 'pytest', 'test_needy.py'])\n" + ), + }) + free, bound = partition(d) + expect.text(" ".join(free) + "/" + " ".join(bound), "/test_driver.py test_needy.py", + "driving a cluster-bound file is inherited") + + +# --------------------------------------------------------------------------- +# ORDINARY PYTEST DEPENDENCY FORMS +# +# The classifier read module-level `def`s and positional parameters, and pytest +# resolves a fixture through five more shapes than that. @linuxhikerpm built a +# direct fixture in each and found them classified database-free and then failing +# under the no-driver shim. Measured against the classifier as it was: +# +# a test method inside a class free <- wrong +# @pytest.mark.usefixtures free <- wrong +# a keyword-only fixture parameter free <- wrong +# request.getfixturevalue free <- wrong +# an ALIASED cluster root in conftest free <- wrong +# module-level positional bound (the one shape it did see) +# +# The fifth needed a shape the review did not give: an alias on the TEST side is +# caught anyway, because the underlying fixture still takes the root positionally. +# It is an alias on the ROOT, in conftest, that hides it -- the root was recorded +# under the def's name while a test requests it under the alias. +# +# One arm per form, and each is a DIRECT dependency, so nothing here relies on the +# closure arms above. + + +def _one_file(tmp_path, body, conftest=None): + return _fake_corpus(tmp_path, {"test_case.py": body}, conftest=conftest) + + +def _verdict(tmp_path, body, conftest=None): + free, _bound = partition(_one_file(tmp_path, body, conftest)) + return "free" if "test_case.py" in free else "bound" + + +def test_a_test_method_inside_a_class_is_a_fixture_request(tmp_path, expect): + """pytest collects `test_*` methods of a class and resolves their fixtures the + same way. A walk over `tree.body` alone sees no def at all.""" + expect.text(_verdict(tmp_path, "class TestThing:\n" + " def test_a(self, pgc_conn):\n" + " assert pgc_conn\n"), + "bound", "a class method requesting a cluster fixture is cluster-bound") + + +def test_usefixtures_is_a_fixture_request_without_a_parameter(tmp_path, expect): + """The form a test uses when it wants the fixture's effect rather than its + value -- which is exactly when the thing it wants is a cluster.""" + expect.text(_verdict(tmp_path, "import pytest\n" + "@pytest.mark.usefixtures('pgc_conn')\n" + "def test_a():\n" + " assert True\n"), + "bound", "@pytest.mark.usefixtures names a dependency") + + +def test_a_keyword_only_parameter_is_a_fixture_request(tmp_path, expect): + """pytest resolves a keyword-only parameter exactly as a positional one.""" + expect.text(_verdict(tmp_path, "def test_a(*, pgc_conn):\n assert pgc_conn\n"), + "bound", "a keyword-only parameter is resolved as a fixture") + + +def test_a_dynamic_request_is_treated_as_cluster_bound(tmp_path, expect): + """`request.getfixturevalue(name)` computes the name at run time, so no AST can + resolve it. The honest answer is not to ban the form but to stop claiming the + file needs no database: wrong in the direction that costs CI time, not in the + direction that greens a gate over tests nothing ran.""" + expect.text(_verdict(tmp_path, "def test_a(request):\n" + " c = request.getfixturevalue('pgc_conn')\n" + " assert c\n"), + "bound", "an unresolvable request is cluster-bound, not free") + + +def test_an_aliased_cluster_root_is_found_under_the_name_tests_request(tmp_path, expect): + """`@pytest.fixture(name="conn")` makes the function requestable as `conn` and + NOT under its own name. Recording the def's name did two wrong things at once: + it missed the dependency a test declares, and it invented a root nothing can + request.""" + aliased = ( + "import pytest\n" + "from pgc_cluster import make_cluster\n\n" + "@pytest.fixture(name='conn')\n" + "def _mk():\n" + " cluster = make_cluster()\n" + " import psycopg\n" + " yield psycopg.connect('')\n" + ) + d = _one_file(tmp_path, "def test_a(conn):\n assert conn\n", conftest=aliased) + expect.row_set([(n,) for n in sorted(cluster_fixtures(d / "conftest.py"))], + [("conn",)], + "the root is read under the name a test requests, not the def's") + free, bound = partition(d) + expect.text(" ".join(free) + "/" + " ".join(bound), "/test_case.py", + "so a test requesting the alias is cluster-bound") + + +def test_a_plain_test_and_a_helpers_parameter_stay_database_free(tmp_path, expect): + """The cost side, and it is the half a widened classifier gets wrong: a rule + that calls everything cluster-bound would pass every arm above and empty the + gate. `self` and `cls` are dropped for the same reason -- they are bound by + Python, and no fixture can be requested under either name.""" + expect.text(_verdict(tmp_path, "def test_a():\n assert True\n"), + "free", "a test requesting nothing needs no database") + expect.text(_verdict(tmp_path, "def helper(pgc_conn):\n return pgc_conn\n" + "def test_a():\n assert True\n"), + "free", "a helper's parameter is not a fixture request") + expect.text(_verdict(tmp_path, "class TestThing:\n" + " def test_a(self):\n" + " assert True\n"), + "free", "and `self` is not a fixture request") From 6cdea5bd6ee7b7c7abc88483606c06840d20863f Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Thu, 10 Sep 2026 18:59:26 +0000 Subject: [PATCH 4/6] test/pytest: read usefixtures on a class and at module level, fix an arm that could not fail, and stop the shell part driving the corpus (#432) Three changes. The first is jd's rule, the other two are @jdatcmd's findings. 1. THE SHELL PART NO LONGER DRIVES THE PYTEST CORPUS. jd's rule, set while this branch was in review: the shell tests and the pytest corpus are PARALLEL IN FUNCTIONALITY and must not call, import or reference each other outside docs. Each asserts against the product, in its own terms, never against the other harness's implementation. This branch's additions to test/selftest/350 broke that in the strongest form -- they INVOKED the decider: python3 "$_hd_decide" --disagree "$PGC_TESTDIR/pytest" python3 "$_hd_decide" --partition "$PGC_TESTDIR/pytest" plus arms reading test/pytest/conftest.py as text. A shell arm driving the python decider is not a second measurement of the property: it agrees by construction and can never report the decider wrong. Both blocks are gone. NO COVERAGE IS LOST, and that is checked rather than asserted -- every property they tested is already in the corpus, where it is native: conftest imports no driver at module scope test_harness_deps.py::test_conftest_imports_no_database_driver_at_module_scope the declaration is exactly the database-free half, both directions test_harness_deps.py::test_the_declaration_is_exactly_the_database_free_half the partition accounts for every file, and the three report cases test_harness_deps.py and test_harness_deps_classifier.py What stays in the shell part is the CI WORKFLOW, which belongs to neither harness. Net new cross-harness references in 350 against main: the ci.yml job-name grep, and three synthetic strings in the anchor fixtures -- a string that resembles a filename is a fixture, not a reference. 2. `usefixtures` ON A CLASS AND AT MODULE LEVEL were classified database-free, and the class one is pointed: class-method descent exists to serve exactly that shape, so the two belonged in one change and only one was there. Measured against the previous head: @pytest.mark.usefixtures on a def bound (the only form it saw) @pytest.mark.usefixtures on a class free -> bound pytestmark = pytest.mark.usefixtures free -> bound pytestmark = [ ... ] free -> bound a method taking the fixture bound (unchanged) a plain test free (unchanged) pytest applies a class decorator to every method and a module-level `pytestmark` to every test. Four arms, including the cost side: only `usefixtures` is a dependency, or every parametrised class would be cluster-bound. Removing the class descent reddens its arm alone; removing the module read reddens the two module arms. 3. `test_the_job_installs_no_database_driver`'s FIRST ASSERTION COULD NOT FAIL. It needed `"pip show psycopg" not in job` while the second assertion required that exact string, so it was pinned to False whatever the install line installed. @jdatcmd added `psycopg[binary]==3.3.5` to the install line and both assertions passed. The product was never at risk -- `! pip show psycopg` runs under `set -euo pipefail` -- so it was a dead arm rather than a hole. It now reads the install LINES and asks whether any names the driver, with a control in the same arm that appends such a line to a copy and requires the same expression to see it. MEASURED harness_selftest 550 checks, 550 passed + 0 failed + 0 unrunnable, rc 0 pytest corpus 192 passed the gated set as CI runs it 8 files, 145 passed, psycopg asserted absent the classifier file 18 arms, and each new capability reddens its own arm Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- test/pytest/TESTS.md | 4 + test/pytest/test_harness_deps.py | 93 +++++++++++---- test/pytest/test_harness_deps_classifier.py | 63 +++++++++++ .../selftest/350-the-pytest-corpus-must-be.sh | 106 +++--------------- 4 files changed, 157 insertions(+), 109 deletions(-) diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index dc32768b..7f44cc78 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -1304,6 +1304,10 @@ over tests nothing ran. | `test_a_dynamic_request_is_treated_as_cluster_bound` | unresolvable means conservative, not free | | `test_an_aliased_cluster_root_is_found_under_the_name_tests_request` | the root is read under its requestable name | | `test_a_plain_test_and_a_helpers_parameter_stay_database_free` | the cost side: a rule that calls everything bound would empty the gate | +| `test_usefixtures_on_a_class_reaches_its_methods` | pytest applies a class decorator to every method, which is the form class-method descent exists to serve | +| `test_a_module_level_pytestmark_reaches_every_test` | `pytestmark = pytest.mark.usefixtures(...)` is a dependency of every test in the file and of no signature | +| `test_a_pytestmark_written_as_a_list_reaches_every_test_too` | the list form is what a file uses once it has two marks | +| `test_an_unrelated_class_decorator_does_not_bind_anything` | the cost side: only `usefixtures` is a dependency, or every parametrised class would be cluster-bound | ## 17. Adding a test diff --git a/test/pytest/test_harness_deps.py b/test/pytest/test_harness_deps.py index dce36ea9..410827b2 100644 --- a/test/pytest/test_harness_deps.py +++ b/test/pytest/test_harness_deps.py @@ -191,8 +191,20 @@ def _fixture_name(fn): return fn.name +def _usefixtures_in(node): + """Fixture names a single `usefixtures(...)` call names, or [] if it is not one.""" + if not isinstance(node, ast.Call): + return [] + f = node.func + if (isinstance(f, ast.Attribute) and f.attr == "usefixtures") or \ + (isinstance(f, ast.Name) and f.id == "usefixtures"): + return [a.value for a in node.args + if isinstance(a, ast.Constant) and isinstance(a.value, str)] + return [] + + def _usefixtures(fn): - """Fixture names pulled in by `@pytest.mark.usefixtures(...)`. + """Fixture names pulled in by `@pytest.mark.usefixtures(...)` on this def. A dependency with NO PARAMETER, so a walk over the signature cannot see it. This is the form a test uses precisely when it wants the fixture's effect and @@ -200,13 +212,27 @@ def _usefixtures(fn): """ out = [] for dec in fn.decorator_list: - if not isinstance(dec, ast.Call): + out += _usefixtures_in(dec) + return out + + +def _module_usefixtures(tree): + """Fixture names a module-level `pytestmark` pulls in for EVERY test in the file. + + `pytestmark = pytest.mark.usefixtures("pgc_conn")`, and the list form. pytest + applies it to every test in the module, so it is a dependency of all of them and + of none of their signatures. Reported by @jdatcmd, who built it alongside the + class form and measured both classified database-free. + """ + out = [] + for n in tree.body: + if not isinstance(n, ast.Assign): continue - f = dec.func - if (isinstance(f, ast.Attribute) and f.attr == "usefixtures") or \ - (isinstance(f, ast.Name) and f.id == "usefixtures"): - out += [a.value for a in dec.args - if isinstance(a, ast.Constant) and isinstance(a.value, str)] + if not any(isinstance(t, ast.Name) and t.id == "pytestmark" for t in n.targets): + continue + vals = n.value.elts if isinstance(n.value, (ast.List, ast.Tuple)) else [n.value] + for v in vals: + out += _usefixtures_in(v) return out @@ -228,7 +254,14 @@ def _params(fn): def _collectable(tree): - """(qualifier, def) for every def pytest can collect or resolve. + """(qualifier, def, inherited) for every def pytest can collect or resolve. + + `inherited` is the fixture names an enclosing CLASS or the MODULE pulls in with + `usefixtures`. pytest applies a class decorator to every method and a module-level + `pytestmark` to every test, so those are dependencies of defs whose own decorator + list and signature say nothing. Without them the class form was classified + database-free -- which is the form the class-method descent below exists to serve, + so the two belonged in one change and only one of them was there. MODULE LEVEL AND CLASS BODIES. pytest collects `test_*` methods of a class and resolves their fixtures identically, so a walk over `tree.body` alone @@ -237,13 +270,16 @@ def _collectable(tree): guesses the collection convention is one convention change from being wrong, and counting a non-collected method is conservative in the safe direction. """ - def walk(node, prefix): + def walk(node, prefix, inherited): for n in node.body: if isinstance(n, (ast.FunctionDef, ast.AsyncFunctionDef)): - yield prefix, n + yield prefix, n, list(inherited) elif isinstance(n, ast.ClassDef): - yield from walk(n, prefix + n.name + ".") - return list(walk(tree, "")) + cls_marks = [] + for dec in n.decorator_list: + cls_marks += _usefixtures_in(dec) + yield from walk(n, prefix + n.name + ".", list(inherited) + cls_marks) + return list(walk(tree, "", _module_usefixtures(tree))) def _defs(tree): @@ -261,14 +297,14 @@ def names to depend on it, and the closure below matches keys against bare-name key would collapse into one -- silently dropping a def from the walk. """ out = {} - for prefix, n in _collectable(tree): + for prefix, n, inherited in _collectable(tree): if _is_fixture(n): kind, key = "fixture", _fixture_name(n) elif n.name.startswith("test_"): kind, key = "test", prefix + n.name else: kind, key = "helper", prefix + n.name - out[key] = (kind, _params(n) + _usefixtures(n), _own_body(n)) + out[key] = (kind, _params(n) + _usefixtures(n) + inherited, _own_body(n)) return out @@ -719,11 +755,30 @@ def test_the_job_installs_no_database_driver(expect): job = ci.read_text() job = job[job.index("pytest-guards:"):] job = job[:job.index("\n build:")] if "\n build:" in job else job - installs_driver = "psycopg" in job and "pip install" in job and "pip show psycopg" not in job - expect.text(repr(installs_driver), "False", - "the job installs no database driver") - expect.at_least(job.count("pip show psycopg"), 1, - "and it asserts the driver is absent rather than assuming it") + # WHAT THIS USED TO ASK, AND WHY IT COULD NOT FAIL. The first assertion was + # + # installs_driver = "psycopg" in job and "pip install" in job \ + # and "pip show psycopg" not in job + # + # and the second required `pip show psycopg` to be present. So whenever the second + # passed, the third conjunct of the first was False and `installs_driver` was pinned + # to False whatever the install line installed. @jdatcmd changed the job to + # `pip install --quiet $PINS psycopg[binary]==3.3.5` and BOTH assertions still + # passed. Two assertions guaranteeing each other's vacuity. + # + # The form below can fail: it looks at the install LINES and asks whether any of + # them names the driver, and the premise after it is the control -- it appends such + # a line to a copy and requires the same expression to see it. + driver_installs = [l for l in job.splitlines() + if "pip install" in l and DRIVER in l] + expect.num(len(driver_installs), 0, + "no pip install line in the job names the database driver") + planted = job + "\n pip install --quiet $PINS " + DRIVER + "[binary]==3.3.5\n" + expect.at_least(len([l for l in planted.splitlines() + if "pip install" in l and DRIVER in l]), 1, + "premise: and that test sees such a line when one is there") + expect.at_least(job.count("pip show " + DRIVER), 1, + "and the job asserts the driver is absent rather than assuming it") if __name__ == "__main__": diff --git a/test/pytest/test_harness_deps_classifier.py b/test/pytest/test_harness_deps_classifier.py index eb8b246d..7c3193f6 100644 --- a/test/pytest/test_harness_deps_classifier.py +++ b/test/pytest/test_harness_deps_classifier.py @@ -273,3 +273,66 @@ def test_a_plain_test_and_a_helpers_parameter_stay_database_free(tmp_path, expec " def test_a(self):\n" " assert True\n"), "free", "and `self` is not a fixture request") + +# --------------------------------------------------------------------------- +# `usefixtures` ON A CLASS AND AT MODULE LEVEL +# +# The first version read `usefixtures` on a FUNCTION only. @jdatcmd found the other two +# places pytest reads it, and the class one is pointed: class-method descent exists to +# serve exactly that shape, so the two belonged in one change and only one was there. +# Measured against that version, four files through `partition()`: +# +# @pytest.mark.usefixtures on a def bound (the only form it saw) +# @pytest.mark.usefixtures on a class free <- wrong +# pytestmark = pytest.mark.usefixtures free <- wrong +# pytestmark = [ ... ] free <- wrong +# +# pytest applies a class decorator to every method and a module-level `pytestmark` to +# every test, so each is a dependency of defs whose own decorator list and signature say +# nothing about it. + + +def test_usefixtures_on_a_class_reaches_its_methods(tmp_path, expect): + """The form class-method descent exists to serve.""" + expect.text(_verdict(tmp_path, "import pytest\n" + "@pytest.mark.usefixtures('pgc_conn')\n" + "class TestThing:\n" + " def test_a(self):\n" + " assert True\n"), + "bound", "a class-level usefixtures binds its methods") + + +def test_a_module_level_pytestmark_reaches_every_test(tmp_path, expect): + """`pytestmark` is how a file says "all of these need it" with no decorator in sight.""" + expect.text(_verdict(tmp_path, "import pytest\n" + "pytestmark = pytest.mark.usefixtures('pgc_conn')\n" + "def test_a():\n" + " assert True\n"), + "bound", "a module-level pytestmark binds every test in the file") + + +def test_a_pytestmark_written_as_a_list_reaches_every_test_too(tmp_path, expect): + """The list form is the common one once a file has two marks, and reading only the + bare form would have covered the arm above while missing the shape people write.""" + expect.text(_verdict(tmp_path, "import pytest\n" + "pytestmark = [pytest.mark.usefixtures('pgc_conn')]\n" + "def test_a():\n" + " assert True\n"), + "bound", "a pytestmark list binds every test in the file") + + +def test_an_unrelated_class_decorator_does_not_bind_anything(tmp_path, expect): + """The cost side, and the one a widened reader gets wrong: only `usefixtures` is a + dependency. A rule that treated any class decorator as one would call every + parametrised class cluster-bound and empty the gate.""" + expect.text(_verdict(tmp_path, "import pytest\n" + "@pytest.mark.slow\n" + "class TestThing:\n" + " def test_a(self):\n" + " assert True\n"), + "free", "an unrelated mark on a class is not a fixture request") + expect.text(_verdict(tmp_path, "import pytest\n" + "pytestmark = pytest.mark.slow\n" + "def test_a():\n" + " assert True\n"), + "free", "nor is an unrelated module-level pytestmark") diff --git a/test/selftest/350-the-pytest-corpus-must-be.sh b/test/selftest/350-the-pytest-corpus-must-be.sh index caa6dff3..6c6f9e28 100644 --- a/test/selftest/350-the-pytest-corpus-must-be.sh +++ b/test/selftest/350-the-pytest-corpus-must-be.sh @@ -372,39 +372,26 @@ unset -f _toc_anchor _toc_unresolved # ---- the harness must self-test without a database, and the gate must run it -- # -# conftest.py imported psycopg AT MODULE SCOPE, and conftest is imported before -# every run, so a DATABASE DRIVER was a hard requirement of the whole corpus -- -# including every test that never opens a connection. With psycopg absent the run -# did not fail a test, it failed to COLLECT. +# THE MEMBERSHIP DECISION AND THE CONFTEST IMPORT ARE NOT CHECKED HERE, and that is +# jd's rule rather than an omission: the shell harness and the pytest corpus are +# PARALLEL IN FUNCTIONALITY and must not call, import or reference each other. An +# earlier version of this part read test/pytest/conftest.py and INVOKED +# test_harness_deps.py with `python3 ... --disagree`, which is the coupling, not a +# second measurement: a shell arm driving the python decider agrees with it by +# construction and can never report it wrong. # -# That coupling was half of why README.md says the corpus is "not in the gate -# yet": pgc_skip treats a missing dependency as a failure rather than a skip, so -# registering the corpus in SUITES would redden every job. That argument is about -# the CLUSTER tests, and until the import moved there was no way to separate them. +# Both properties are asserted in the corpus, where they are native: +# conftest imports no driver at module scope +# test_harness_deps.py::test_conftest_imports_no_database_driver_at_module_scope +# the declaration is exactly the database-free half, both directions +# test_harness_deps.py::test_the_declaration_is_exactly_the_database_free_half +# the partition accounts for every file, and the three report cases +# test_harness_deps.py and test_harness_deps_classifier.py # -# The behavioural proof lives in test/pytest/test_harness_deps.py, which shims -# `import psycopg` to raise and requires the no-cluster files to pass anyway. -# THIS half is static and it is the half that runs in the matrix. +# What stays here is the CI WORKFLOW, which belongs to neither harness. -_hd_conf="$PGC_TESTDIR/pytest/conftest.py" _hd_ci="$PGC_SRCDIR/.github/workflows/ci.yml" -check "premise: the pytest conftest is where this part thinks it is" \ - "$([ -f "$_hd_conf" ] && echo yes || echo no)" "yes" - -check "conftest imports no database driver at module scope" \ - "$(grep -cE '^(import psycopg|from psycopg)' "$_hd_conf")" "0" - -# PREMISE: the pattern can see such an import at all, or "0" is what a broken -# grep says too. -printf 'import os\nimport psycopg\n' > "$PGC_WORKDIR/hd-fixture.py" -check "premise: the pattern recognises a module-scope driver import" \ - "$(grep -cE '^(import psycopg|from psycopg)' "$PGC_WORKDIR/hd-fixture.py")" "1" - -# And it must still be imported SOMEWHERE, or the fixtures cannot connect and the -# arm above is satisfied by a harness that talks to no database at all. -check "and the driver is still imported inside the fixtures that connect" \ - "$([ "$(grep -cE '^\s+import psycopg' "$_hd_conf")" -ge 1 ] && echo yes || echo no)" "yes" check "premise: the CI workflow is where this part thinks it is" \ "$([ -f "$_hd_ci" ] && echo yes || echo no)" "yes" @@ -425,68 +412,7 @@ check "and derives the pins from requirements-test.txt" \ # ---- and the DECLARATION must be decided, not declaimed ---------------------- # -# WHAT WAS WRONG. `NO_CLUSTER` in test_harness_deps.py says which files need no -# database, and the job runs exactly those. The only arm over it asked whether the -# files it names EXIST. That is one direction, and the missing direction is the one -# that loses coverage: a database-free file nobody adds to the list is absent from -# the job, every arm stays green, and NOTHING says so. It had already happened -# twice -- test_build_refusal.py and test_layer.py both need no database and -# neither was listed. -# -# The module decides the property from the corpus with an ast walk and requires set -# equality with the list. WHY HERE: nothing in the gate runs pytest over that file. -# The corpus is not in SUITES, and the pytest-guards job runs the database-free -# files, which test_harness_deps.py is not -- its control arm needs a real cluster. -# So the module exposes the decision on its command line and this part runs it. -# Same move as the documentation sweep above: the corpus carries a twin, this copy -# has the teeth. -_hd_decide="$PGC_TESTDIR/pytest/test_harness_deps.py" - -check "premise: the membership decider is where this part thinks it is" \ - "$([ -f "$_hd_decide" ] && echo yes || echo no)" "yes" - -check "every database-free file in the corpus is declared in NO_CLUSTER" \ - "$(python3 "$_hd_decide" --disagree "$PGC_TESTDIR/pytest" 2>&1)" "[]" - -# PREMISE: the decider SPLIT the corpus. One that parsed nothing reports an empty -# database-free set, which agrees with an empty list and looks like success; one -# that called everything database-free would pass a one-directional check. -_hd_part="$(python3 "$_hd_decide" --partition "$PGC_TESTDIR/pytest" 2>&1)" -check "premise: the decider partitions the corpus into two non-empty halves" \ - "$(printf '%s' "$_hd_part" | grep -cE 'free: test_[^|]+ \| bound: test_')" "1" - -echo " CORPUS: $_hd_part" - -# ---- and that arm must be able to FAIL, on a fixture rather than on the tree -- -# -# Everything above passes on a healthy tree, which is what a decider returning a -# constant also does. The fixture is a two-file corpus with a conftest shaped like -# the real one: one fixture imports the driver, one depends on it. -_hd_fix="$PGC_WORKDIR/nocluster"; rm -rf "$_hd_fix"; mkdir -p "$_hd_fix" -{ - printf 'import pytest\n' - printf 'from pgc_cluster import make_cluster\n\n' - printf '@pytest.fixture\ndef pgc_conn():\n import psycopg\n yield make_cluster()\n' -} > "$_hd_fix/conftest.py" -printf 'def test_it(expect):\n pass\n' > "$_hd_fix/test_free.py" -printf 'def test_it(pgc_conn, expect):\n pass\n' > "$_hd_fix/test_bound.py" - -check "the fixture corpus splits the way the property says" \ - "$(python3 "$_hd_decide" --partition "$_hd_fix")" \ - "free: test_free.py | bound: test_bound.py" - -check "a database-free file declared nowhere is named, not passed over" \ - "$(python3 "$_hd_decide" --disagree "$_hd_fix" test_bound.py)" \ - "[2: needs-a-cluster:test_bound.py undeclared:test_free.py]" - -check "control: the same corpus with the right declaration is clean" \ - "$(python3 "$_hd_decide" --disagree "$_hd_fix" test_free.py)" "[]" - -check "a declared file that no longer exists is named" \ - "$(python3 "$_hd_decide" --disagree "$_hd_fix" test_free.py test_renamed.py)" \ - "[1: absent:test_renamed.py]" - -unset _hd_conf _hd_ci _hd_decide _hd_part _hd_fix +unset _hd_ci unset _dcv_dir _dcv_doc _dcv_seen _dcv_stated _dcv_fix _dcv_ht unset -f _dcv_missing _dcv_count From 11c315ce2141981370ba2ecfafdf333754b16f6c Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Thu, 10 Sep 2026 19:11:38 +0000 Subject: [PATCH 5/6] test/pytest: needing a cluster and needing the driver are two properties, and the job's list is the intersection (#432) #927 landing made this branch's own arm fire, by name, which is what it exists to do: disagreements: [1: undeclared:test_raises_sqlstate.py] THE OBVIOUS FIX WAS WRONG, and the arm that caught it was right. Declaring the file turned the driver-free job red: four of its arms fail with psycopg shimmed out, because the modules it hands to `pytester` import the driver. It requests no cluster fixture and it still cannot run where there is no driver. MY SECOND ATTEMPT WAS ALSO WRONG, and an existing arm refused it. Folding the driver condition into `partition()` contradicted `test_the_classifier_is_not_fooled_by_prose_that_names_the_driver`, which asserts that a generated inner test requesting a cluster fixture is the INNER run's requirement and not this file's. That arm is correct, and breaking it was the signal that I was overloading one property with two meanings. SO THERE ARE TWO PROPERTIES, derived separately: partition() -> does this file request a cluster? driver_dependent() -> does it need psycopg IMPORTABLE, even with no cluster? job_runnable() -> the intersection, which is what the job can run and `membership_report` compares the declaration against the intersection, with `needs-the-driver:` as a kind of its own -- `needs-a-cluster:` would be a wrong diagnosis and the reader's next action differs. TWO CONDITIONS FOR THE DRIVER PROPERTY, because a driver import in a string is not enough on its own. `test_harness_deps_classifier.py` writes fixture corpora containing `import psycopg` and only ever PARSES them -- nothing imports those files. Reading the string alone would have thrown that file out of the gate it exists to be in. The difference is whether the file drives `pytester`. cluster-free 9 files, including test_raises_sqlstate.py driver-dependent test_raises_sqlstate.py job runnable 8, which is the declaration disagreements [] Three arms pin it, including both controls: a file that only parses a driver import stays job-runnable, and prose naming the driver is not a dependency on it. MEASURED harness_selftest 550 checks, 550 passed + 0 failed + 0 unrunnable, rc 0 pytest corpus 227 passed the gated set as CI runs it 8 files, 150 passed, psycopg asserted absent Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- test/pytest/TESTS.md | 3 + test/pytest/test_harness_deps.py | 83 ++++++++++++++++++++- test/pytest/test_harness_deps_classifier.py | 76 ++++++++++++++++++- 3 files changed, 157 insertions(+), 5 deletions(-) diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index fdc1d384..61df728b 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -1339,6 +1339,9 @@ over tests nothing ran. | `test_a_module_level_pytestmark_reaches_every_test` | `pytestmark = pytest.mark.usefixtures(...)` is a dependency of every test in the file and of no signature | | `test_a_pytestmark_written_as_a_list_reaches_every_test_too` | the list form is what a file uses once it has two marks | | `test_an_unrelated_class_decorator_does_not_bind_anything` | the cost side: only `usefixtures` is a dependency, or every parametrised class would be cluster-bound | +| `test_a_file_whose_generated_tests_import_the_driver_is_driver_dependent` | cluster-free and still unrunnable where there is no driver, so the job's list is the intersection of two properties | +| `test_a_file_that_only_PARSES_a_driver_import_is_job_runnable` | the control: a driver import in a string nothing runs is not a dependency, and reading only the string would exclude this very file | +| `test_prose_naming_the_driver_is_not_a_driver_dependency` | a docstring naming psycopg is a sentence about code | ## 17. Adding a test diff --git a/test/pytest/test_harness_deps.py b/test/pytest/test_harness_deps.py index 410827b2..1f3c2758 100644 --- a/test/pytest/test_harness_deps.py +++ b/test/pytest/test_harness_deps.py @@ -148,6 +148,51 @@ def _module_scope(tree): return out +def _needs_the_driver_installed(tree, prose): + """Does this file need psycopg IMPORTABLE, even though it requests no cluster? + + The job this declaration feeds installs no driver as well as running no cluster, so + the property it needs is wider than "requests no cluster fixture". A file that builds + a source with `import psycopg` in it and gives that to `pytester` needs the driver: + the inner run imports the generated module, and with the driver absent it fails at + import rather than reaching whatever the arm was about. + + Measured: `test_raises_sqlstate.py` requests no cluster fixture and is correctly + cluster-free, and four of its arms FAIL with psycopg shimmed out, because the files + it generates import it. + + THIS IS NOT THE CLUSTER PROPERTY AND MUST NOT BE FOLDED INTO IT. `partition()` asks + whether a file requests a cluster; a generated inner test requesting `pgc_conn` is + the INNER run's requirement, not this file's, and + `test_the_classifier_is_not_fooled_by_prose_that_names_the_driver` asserts exactly + that. Wiring this into `partition()` contradicted that arm, correctly, on the first + attempt. The job needs BOTH properties, so the job's list is the intersection and the + two are derived separately. + + TWO CONDITIONS, because a driver import in a string is not enough on its own. + `test_harness_deps_classifier.py` writes fixture corpora containing `import psycopg` + and only ever PARSES them -- nothing runs them, so it needs no driver and belongs in + the job. The difference is whether the file drives `pytester`, and reading only the + string would have thrown that file out of the gate it exists to be in. + + Prose is excluded for the reason it is excluded everywhere here: a docstring naming + the driver is a sentence, not an import. + """ + drives_inner_run = False + for node in ast.walk(tree): + if isinstance(node, ast.Name) and node.id == "pytester": + drives_inner_run = True + break + if not drives_inner_run: + return False + for node in ast.walk(tree): + if (isinstance(node, ast.Constant) and isinstance(node.value, str) + and id(node) not in prose + and ("import psycopg" in node.value or "from psycopg" in node.value)): + return True + return False + + def _imports_driver(nodes): for n in nodes: if isinstance(n, ast.Import): @@ -426,6 +471,31 @@ def partition(directory=None): return ([n for n in names if not bound[n]], [n for n in names if bound[n]]) +def driver_dependent(directory=None): + """Cluster-free files that still need psycopg importable, so the job cannot run them. + + The job this declaration feeds installs no driver as well as running no cluster, so + its file list is the INTERSECTION of the two properties rather than either one. + `test_raises_sqlstate.py` is the case that showed the difference: it requests no + cluster fixture, and four of its arms fail with psycopg shimmed out, because the + modules it hands to `pytester` import it. + """ + directory = HERE if directory is None else pathlib.Path(directory) + out = [] + for path in sorted(directory.glob("test_*.py")): + tree = _parse(path) + if _needs_the_driver_installed(tree, _prose(tree)): + out.append(path.name) + return out + + +def job_runnable(directory=None): + """The files the driver-free job can run: cluster-free AND not driver-dependent.""" + directory = HERE if directory is None else pathlib.Path(directory) + free, _bound = partition(directory) + return sorted(set(free) - set(driver_dependent(directory))) + + def membership_report(directory=None, declared=None): """"[]" when the declaration is exactly the database-free half, else "[n: kind:file ...]" naming every disagreement and which way it goes. @@ -434,13 +504,18 @@ def membership_report(directory=None, declared=None): what is wrong rather than only that something is.""" directory = HERE if directory is None else pathlib.Path(directory) declared = NO_CLUSTER if declared is None else declared - free, _bound = partition(directory) - free = set(free) + runnable = set(job_runnable(directory)) + cluster_free = set(partition(directory)[0]) + driver_bound = set(driver_dependent(directory)) present = {p.name for p in directory.glob("test_*.py")} bad = ["absent:" + n for n in sorted(set(declared) - present)] bad += ["needs-a-cluster:" + n - for n in sorted((set(declared) & present) - free)] - bad += ["undeclared:" + n for n in sorted(free - set(declared))] + for n in sorted((set(declared) & present) - cluster_free)] + # A kind of its own, because "needs-a-cluster" would be a wrong diagnosis and the + # reader's next action differs: this file needs the DRIVER, not a server. + bad += ["needs-the-driver:" + n + for n in sorted((set(declared) & present) & driver_bound)] + bad += ["undeclared:" + n for n in sorted(runnable - set(declared))] return "[]" if not bad else "[%d: %s]" % (len(bad), " ".join(bad)) diff --git a/test/pytest/test_harness_deps_classifier.py b/test/pytest/test_harness_deps_classifier.py index 7c3193f6..336f6ba2 100644 --- a/test/pytest/test_harness_deps_classifier.py +++ b/test/pytest/test_harness_deps_classifier.py @@ -17,7 +17,13 @@ rule reads string constants naming corpus files, not imports. """ -from test_harness_deps import _fake_corpus, cluster_fixtures, partition +from test_harness_deps import ( + _fake_corpus, + cluster_fixtures, + driver_dependent, + job_runnable, + partition, +) def test_the_classifier_tells_a_plain_file_from_one_that_requests_a_cluster(tmp_path, expect): """The base case and its control, over a conftest this test wrote: the @@ -336,3 +342,71 @@ def test_an_unrelated_class_decorator_does_not_bind_anything(tmp_path, expect): "def test_a():\n" " assert True\n"), "free", "nor is an unrelated module-level pytestmark") + +# --------------------------------------------------------------------------- +# NEEDING A CLUSTER AND NEEDING THE DRIVER ARE TWO PROPERTIES +# +# The job this declaration feeds installs no driver as well as running no cluster, so its +# file list is the INTERSECTION. `test_raises_sqlstate.py` is the case that showed the +# difference: it requests no cluster fixture -- correctly cluster-free -- and four of its +# arms fail with psycopg shimmed out, because the modules it hands to `pytester` import it. +# +# Folding that into `partition()` was my first attempt and it contradicted +# `test_the_classifier_is_not_fooled_by_prose_that_names_the_driver`, which asserts that a +# generated inner test requesting a cluster fixture is the INNER run's requirement and not +# this file's. That arm is right. Two properties, derived separately, intersected for the +# job. + + +def test_a_file_whose_generated_tests_import_the_driver_is_driver_dependent(tmp_path, expect): + """Cluster-free and still unable to run where there is no driver.""" + d = _fake_corpus(tmp_path, { + "test_inner_driver.py": ( + "def test_it(pytester, expect):\n" + " pytester.makepyfile(\n" + ' "import psycopg\\n"\n' + ' "def test_inner():\\n pass\\n"\n' + " )\n" + ), + }) + free, _bound = partition(d) + expect.text(" ".join(free), "test_inner_driver.py", + "it requests no cluster, so the cluster property calls it free") + expect.row_set([(n,) for n in driver_dependent(d)], [("test_inner_driver.py",)], + "and the driver property calls it dependent") + expect.num(len(job_runnable(d)), 0, + "so the job's list, which is the intersection, excludes it") + + +def test_a_file_that_only_PARSES_a_driver_import_is_job_runnable(tmp_path, expect): + """The control, and the reason the rule needs two conditions rather than one. + + `test_harness_deps_classifier.py` writes fixture corpora containing `import psycopg` + and never runs them -- nothing imports those files, they are parsed. Reading only the + string would throw this very file out of the gate it exists to be in. + """ + d = _fake_corpus(tmp_path, { + "test_parses_only.py": ( + "def test_it(tmp_path, expect):\n" + ' (tmp_path / "fixture.py").write_text("import psycopg\\n")\n' + ' expect.num(1, 1, "parsed, never run")\n' + ), + }) + expect.num(len(driver_dependent(d)), 0, + "a driver import in a string nothing runs is not a dependency") + expect.text(" ".join(job_runnable(d)), "test_parses_only.py", + "so the job can run it") + + +def test_prose_naming_the_driver_is_not_a_driver_dependency(tmp_path, expect): + """The same exclusion the cluster property makes, for the same reason: a docstring + naming psycopg is a sentence about code.""" + d = _fake_corpus(tmp_path, { + "test_prose_only.py": ( + '"""This file explains `import psycopg` and says nothing else."""\n' + "def test_it(pytester, expect):\n" + ' pytester.makepyfile("def test_inner():\\n pass\\n")\n' + ), + }) + expect.num(len(driver_dependent(d)), 0, + "prose naming the driver is not a dependency on it") From 1ce1147d70cb87a5a8bcb27d5522892f483ab320 Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Thu, 10 Sep 2026 19:25:00 +0000 Subject: [PATCH 6/6] test/pytest: declare test_failed_query_sentinel.py, which #930 landed (#432) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The arm named it rather than leaving a hole: cluster-free, not driver-dependent, so the driver-free job can run it and the declaration has to say so. Third time this arm has caught a merge-order consequence rather than a mistake -- #922 brought test_suite_accounting.py, #927 brought test_raises_sqlstate.py (which turned out to need the DRIVER and so is correctly excluded), and #930 brings this one. declared 9 · cluster-free 10 · driver-dependent 1 · disagreements [] corpus 238 passed · the gated set 9 files, 161 passed with psycopg absent Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- test/pytest/test_harness_deps.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/pytest/test_harness_deps.py b/test/pytest/test_harness_deps.py index 1f3c2758..26eebaf4 100644 --- a/test/pytest/test_harness_deps.py +++ b/test/pytest/test_harness_deps.py @@ -75,6 +75,11 @@ # list IS this list actually runs them. This file cannot be in the list: it # hands cluster-bound file names to pytest, so it needs what they need. "test_harness_deps_classifier.py", + # Landed on main in #930 while this branch was in review, and the arm below named + # it: cluster-free, not driver-dependent, so the job can run it and the + # declaration has to say so. The third time this arm has caught a merge-order + # consequence rather than a mistake. + "test_failed_query_sentinel.py", ]