diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 52c3178b..d59b7f0d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -63,6 +63,58 @@ 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. 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 + # 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" + # 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 # between majors trips first. # Both architectures, because a build preflight is cheap and the project has diff --git a/CHANGELOG.md b/CHANGELOG.md index 84ea3081..b245614c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -96,6 +96,54 @@ true until the next version shipped. which made the statement rule silently conditional on the class being positional while the documentation stated it unconditionally. +- 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). @@ -335,6 +383,33 @@ true until the next version shipped. because a map that names the wrong gap is worse than one that admits it does not know. +- 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 c9877221..aeee6636 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -60,11 +60,13 @@ behaviour, the source of that number is named. - [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_suite_accounting.py: the matrix accounting for its own suites](#14-test_suite_accountingpy-the-matrix-accounting-for-its-own-suites) -- [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) -- [18. test_raises_sqlstate.py: which error, and which statement](#18-test_raises_sqlstatepy-which-error-and-which-statement) -- [19. test_failed_query_sentinel.py: a failed query is not a comparison](#19-test_failed_query_sentinelpy-a-failed-query-is-not-a-comparison) +- [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. 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) +- [20. test_raises_sqlstate.py: which error, and which statement](#20-test_raises_sqlstatepy-which-error-and-which-statement) +- [21. test_failed_query_sentinel.py: a failed query is not a comparison](#21-test_failed_query_sentinelpy-a-failed-query-is-not-a-comparison) ## 1. How to read a test in here @@ -1181,7 +1183,169 @@ reproduces on long files and not short ones -- it passed every fixture and faile on the real population, naming two of the longest suites. Selftest 040 carries the same story from #473 and #476. -## 15. Adding a test +## 15. 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 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' + conftest.py:15: in + import psycopg + E ModuleNotFoundError: No module named 'psycopg' + +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_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 +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 +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. + +## 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 | +| `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 | +| `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 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 @@ -1208,7 +1372,7 @@ story from #473 and #476. 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. -## 16. 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 27 @@ -1229,7 +1393,7 @@ setup, and a compound statement such as a `for` holding the setup and the statem under test. Both are pinned by arms that assert the scan reports nothing on them, and `VACUITY_MODES.md` section 3.4 says what would close the mode. -## 17. 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. @@ -1261,7 +1425,7 @@ process. Walking `/proc//cmdline` is the reliable instrument. `test_one_tree_hashes_one_way_however_the_locale_is_set` requires one tree to give one fingerprint across every installed locale. -## 18. test_raises_sqlstate.py: which error, and which statement +## 20. test_raises_sqlstate.py: which error, and which statement Numbered 17 rather than inserted after section 4, where a reader looking for a per-file section would expect it. Renumbering twelve headings and their Contents @@ -1388,7 +1552,7 @@ one condition faithfully, and require the copy to go blind. | `test_disabling_the_sqlstate_rule_makes_the_scan_blind` | the neutering proof: a copy of the layer with `False and` prefixed, nothing renamed, goes blind while still containing the pinned text | | `test_disabling_the_statement_rule_makes_the_scan_blind` | the same for the second condition, so neither rule rests on the other's arm | | `test_the_mode_this_layer_only_narrows_is_still_listed_as_open` | `raises-catches-setup` must stay in section 3 of the mode inventory | -## 19. test_failed_query_sentinel.py: a failed query is not a comparison +## 21. test_failed_query_sentinel.py: a failed query is not a comparison `error-swallowed-to-empty`: two queries raise, a helper turns each into the same value, and they compare equal. The test is green and has asserted nothing about diff --git a/test/pytest/conftest.py b/test/pytest/conftest.py index 987fafda..c03425cd 100644 --- a/test/pytest/conftest.py +++ b/test/pytest/conftest.py @@ -5,6 +5,22 @@ `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 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 +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 +28,6 @@ import pathlib import shutil -import psycopg import pytest from pgc_cluster import _pg_config, build_once, make_cluster @@ -72,6 +87,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 +109,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..26eebaf4 --- /dev/null +++ b/test/pytest/test_harness_deps.py @@ -0,0 +1,865 @@ +"""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 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' + 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. + +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 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", + # 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", + # 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", +] + + +# --------------------------------------------------------------------------- +# 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 _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): + 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 _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_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(...)` 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 + not its value -- which is exactly when it is a cluster it wants. + """ + out = [] + for dec in fn.decorator_list: + 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 + 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 + + +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, 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 + 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, inherited): + for n in node.body: + if isinstance(n, (ast.FunctionDef, ast.AsyncFunctionDef)): + yield prefix, n, list(inherited) + elif isinstance(n, ast.ClassDef): + 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): + """{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. + + 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 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) + inherited, _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 + + +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()) + # 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. + 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 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. + + 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 + 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) - 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)) + + +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. + + 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): + """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 + # 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:]}") + 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): + """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") + + +# ---- 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_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): + """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") + + # 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. + + 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 + # 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__": + sys.exit(_main(sys.argv[1:])) diff --git a/test/pytest/test_harness_deps_classifier.py b/test/pytest/test_harness_deps_classifier.py new file mode 100644 index 00000000..336f6ba2 --- /dev/null +++ b/test/pytest/test_harness_deps_classifier.py @@ -0,0 +1,412 @@ +"""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, + 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 + 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") + +# --------------------------------------------------------------------------- +# `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") + +# --------------------------------------------------------------------------- +# 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") diff --git a/test/selftest/350-the-pytest-corpus-must-be.sh b/test/selftest/350-the-pytest-corpus-must-be.sh index 088f1bb5..6c6f9e28 100644 --- a/test/selftest/350-the-pytest-corpus-must-be.sh +++ b/test/selftest/350-the-pytest-corpus-must-be.sh @@ -206,7 +206,9 @@ _dcv_absent() { # _dcv_absent DIR DOC -> "[]" or "[n: a b c]" # It is not latent. It reddened #923's `suites (PG 17)` on a name that # exists, while PG 18 passed, and reproduces at this corpus size only # under load: 170 names, 400 trials on a busy machine, 6 false absences - # piped and 0 on a here-string. + # piped and 0 on a here-string. An independent run of the same shape in + # isolation gave 10 in 40, so the rate is load- and size-dependent rather + # than fixed -- the two measurements bracket it. [ "$(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 \ @@ -278,6 +280,140 @@ 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 -- +# +# 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. +# +# 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 +# +# What stays here is the CI WORKFLOW, which belongs to neither harness. + +_hd_ci="$PGC_SRCDIR/.github/workflows/ci.yml" + + +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" + +# ---- and the DECLARATION must be decided, not declaimed ---------------------- +# +unset _hd_ci + unset _dcv_dir _dcv_doc _dcv_seen _dcv_stated _dcv_fix _dcv_ht unset -f _dcv_missing _dcv_count