From b90e563b01ca0ae3c6bcceae8798aab80374ba68 Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Tue, 8 Sep 2026 23:17:36 +0000 Subject: [PATCH 01/11] test: a parallel pytest harness with a vacuity-refusal layer (#432) Issue #432 asks whether to move the suite from bash to pytest. This is the pilot the discussion on that issue asked for: a working harness, two suites' worth of machinery, and one suite ported and proved against its bash original. It replaces nothing. test/run_all_versions.sh remains the gate and no bash suite is deleted. The port is not the hard part. The vacuity guard is. A vacuity defect is a test that reports PASS while asserting nothing. Bare pytest permits it in eight ways, all measured on pytest 9.1.1 and all exiting 0: a body with no assert, empty compared with empty, parametrize over an empty list, every test skipped, a non-strict xfail that passes, a test that returns instead of asserting, pytest.raises(Exception) satisfied by an unrelated error, and a substring match where a typed field was meant. The bash harness defends against this class in several places, earned one defect at a time. pytest defends nowhere. So the layer comes first, and it is structural rather than advisory: - a test passes only if it made a counted assertion, which also kills the return-instead-of-assert mode - comparisons refuse their degenerate cases: both sides empty, a value against itself, an empty expectation, a floor of zero - plan assertions read Custom Plan Provider from EXPLAIN (FORMAT JSON) by exact equality, because "ColumnarScan" in plan is as wrong as grep ColumnarScan when the real provider is PgColumnarScan - a bare skip fails the run; skipping needs a reason from the closed list lib.sh already uses - the run asserts its own collected count, so a filtered run cannot be green - the cluster identity check refuses a server whose data_directory is not ours, with a negative control proving it can say no Seventeen of the 24 tests are the layer testing itself, run through pytester so each guard is proven to refuse rather than assumed to. The layer's own tests obey the layer: an exemption for the tests that prove the guard is the first step to exempting everything else. Direct connections, per the requirement. psycopg 3 returns int, Decimal, float, bytes, list and None where psql -At returns text and leaves every conversion to the reader. EXPLAIN (FORMAT JSON) arrives already parsed. The operations that still run a binary are initdb, pg_ctl, pg_dump and pg_restore, and the build. Terminating a backend is pg_terminate_backend over SQL, not a killed psql, because killing the client leaves the backend running its statement. Parallelism: one cluster per xdist worker, on a port derived from the worker id below the ephemeral floor, with a private schema per test. Four workers run the suite in 2.1s. Not -n auto: eight cores are shared with a desktop. The differential is what makes the port credible. test/native_projection.sh and its port, each arm building once so both harnesses measure the same library: ARM A unmutated .so 8370e9b1beba bash 8/0 pytest 7/0 ARM B fan-out neutered .so 9e9510593777 bash 0/8 pytest 0/7 Both go red together. compare_to_bash.py then diffs the two by assertion NAME: every bash property is covered, plus one the bash suite lacks, a premise that the DELETE removed rows. Without it both delete arms are satisfied by a projection that never changed. Measured on the leak question that opened the issue: after two gate matrix runs this container held four orphaned postmasters from /tmp/pgcolumnar-test.* datadirs, aged 18 to 35 minutes. The pytest harness left zero. design/ISSUE_432_PYTEST_HARNESS.md carries the measurements, the psql-exception list with a reason for each, the ordered red tests, what is out of scope, and a mandatory section on what the layer still cannot catch. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- design/ISSUE_432_PYTEST_HARNESS.md | 380 ++++++++++++++++++++++++++ test/pytest/README.md | 49 ++++ test/pytest/compare_to_bash.py | 52 ++++ test/pytest/conftest.py | 75 +++++ test/pytest/pgc_cluster.py | 161 +++++++++++ test/pytest/pgc_vacuity.py | 320 ++++++++++++++++++++++ test/pytest/pytest.ini | 8 + test/pytest/requirements-test.txt | 9 + test/pytest/test_connection.py | 105 +++++++ test/pytest/test_layer.py | 177 ++++++++++++ test/pytest/test_native_projection.py | 119 ++++++++ 11 files changed, 1455 insertions(+) create mode 100644 design/ISSUE_432_PYTEST_HARNESS.md create mode 100644 test/pytest/README.md create mode 100644 test/pytest/compare_to_bash.py create mode 100644 test/pytest/conftest.py create mode 100644 test/pytest/pgc_cluster.py create mode 100644 test/pytest/pgc_vacuity.py create mode 100644 test/pytest/pytest.ini create mode 100644 test/pytest/requirements-test.txt create mode 100644 test/pytest/test_connection.py create mode 100644 test/pytest/test_layer.py create mode 100644 test/pytest/test_native_projection.py diff --git a/design/ISSUE_432_PYTEST_HARNESS.md b/design/ISSUE_432_PYTEST_HARNESS.md new file mode 100644 index 00000000..54fad396 --- /dev/null +++ b/design/ISSUE_432_PYTEST_HARNESS.md @@ -0,0 +1,380 @@ +# Issue 432: a pytest harness, and the vacuity guard it must carry + +## Who this is for + +A developer adding or porting a pgColumnar test. It assumes you can read Python and +SQL, that you have used pytest, and that you have run `test/run_all_versions.sh` +at least once. It does not assume you know the bash harness internals. + +## What this document decides + +Issue #432 asks whether to move the test suite from bash to pytest. This is the +plan for doing it, and the answer to the question the discussion on that issue +left open: what a port must carry across so it does not lose coverage it cannot +see it has lost. + +**The port is not the hard part. The vacuity guard is.** + +A vacuity defect is a test that reports PASS while asserting nothing. The bash +harness has been bitten by this and now defends against it in several places. Bare +pytest defends against it nowhere. Section 4 measures that, and section 5 is the +layer that fixes it. + +## 1. Scope + +In scope for the first landing: + +- A pytest harness that talks to the server over a direct connection. +- Tests that run in parallel. +- The vacuity-refusal layer, with its own tests. +- Two ported suites, running beside their bash originals, not replacing them. +- A differential check that the port asserts the same thing as the original. + +Out of scope, deliberately: + +- Porting the other 254 suites. Nothing is deleted in this landing. +- Replacing `test/run_all_versions.sh`. It stays the gate. +- Porting any suite that starts, kills or crashes a server. +- FreeBSD support. It is a reason to prefer Python, not a deliverable here. + +## 2. Prerequisites, measured + +The container had none of the tooling. This is what it took. + +`pytest`, `pytest-xdist`, `psycopg` and `psycopg2` were all absent. Python is +3.14.4 and is marked `EXTERNALLY-MANAGED`, so `pip install` into the system +interpreter is refused. `python3 -m venv` failed too, because `ensurepip` is not +in the base image. + +The working sequence: + +```sh +apt-get install -y python3.14-venv +python3 -m venv /root/pyenv +/root/pyenv/bin/pip install pytest pytest-xdist 'psycopg[binary]' +``` + +That produced pytest 9.1.1, pytest-xdist 3.8.0 and psycopg 3.3.5 against libpq +18.0.6, using the binary build so nothing compiles. + +Distribution packages are an alternative: `python3-psycopg` 3.3.2, +`python3-pytest` 9.0.2, `python3-pytest-xdist` 3.8.0. The venv is preferred +because it pins versions, and CI must install from a checked-in requirements file +rather than from whatever the runner happens to carry. + +`test/` already holds 13 Python files. **None of them connect to PostgreSQL.** +They generate corpora and check documents. So there is no direct-connection +precedent in this tree, and this work sets it. + +## 3. Direct connections, and the short list of exceptions + +The requirement is a direct connection and typed results. Shelling out to `psql` +and reading its text is allowed only where nothing else works. + +Measured against a live cluster, psycopg returns real Python types: + +| SQL | Python value | type | +|---|---|---| +| `count(*)` | `100` | `int` | +| `1.5::numeric` | `Decimal('1.5')` | `Decimal` | +| `1.5::float8` | `1.5` | `float` | +| `NULL::int` | `None` | `NoneType` | +| `'\x00ff'::bytea` | `b'\x00\xff'` | `bytes` | +| `ARRAY[1,2]` | `[1, 2]` | `list` | + +`psql -At` returns the string `100` for the first row and leaves every conversion +to the reader. That difference is the point of the change. + +Operations that must still run a binary, with the reason: + +| operation | why a connection cannot do it | +|---|---| +| `initdb` | creates the cluster a connection would connect to | +| `pg_ctl start` / `stop` | starts and stops the server itself | +| `pg_dump`, `pg_restore`, `pg_dumpall` | client programs; no libpq call performs a dump | +| `pg_upgrade`, `pg_basebackup` | same, and both operate on files not sessions | +| `make`, `make install` | builds the extension under test | + +Things that look like exceptions and are not: + +- **Terminating a backend.** Use `pg_terminate_backend`, which is SQL. Killing a + `psql` process leaves the backend running its statement, which has already + produced one false removal proof in this project. +- **COPY.** psycopg supports `COPY` directly, both directions. +- **A torn connection.** Close the socket from Python. +- **Reading a plan.** `EXPLAIN (FORMAT JSON)` comes back as parsed Python. + +## 4. What bare pytest does with a test that asserts nothing + +Every row below was run. The exit code is the one pytest returned. + +| what the test does | pytest reports | exit | +|---|---|---| +| no `assert` anywhere in the body | 1 passed | **0** | +| `assert got == want`, both empty strings | 2 passed | **0** | +| `parametrize` over an empty list | 1 skipped | **0** | +| every test skipped | 2 skipped | **0** | +| `xfail` that unexpectedly passes, non-strict | 1 xfailed, 1 xpassed | **0** | +| `return got == want` instead of asserting | 1 passed, 1 warning | **0** | +| `pytest.raises(Exception)` satisfied by an unrelated error | 1 passed | **0** | +| `assert "ColumnarScan" in plan` where the plan says `PgColumnarScan` | 1 passed | **0** | + +Four behaviours are already safe: a fixture raising in teardown exits 1, `-k` +filtering everything out exits 5, collecting no tests exits 5, and an xdist worker +crash exits 1. + +**Eight ways to report success while asserting nothing, all exiting 0.** That is +the gap this design exists to close. Exit 5 also matters: it means "no tests ran", +and a CI step written as `pytest || exit 1` treats it as failure only by accident +of the shell, while many wrappers treat any non-1 code as fine. + +## 5. The vacuity-refusal layer + +Each refusal is structural. A convention a reviewer is asked to remember is not a +refusal. + +### 5.1 A test must make at least one counted assertion + +A conftest plugin counts assertions per test and fails a test that made none. The +count comes from the assertion helpers in 5.2, not from Python's `assert`, so a +test that computes and concludes nothing cannot pass. + +This one mechanism also kills the `return`-instead-of-assert mode, because a +returning test makes no counted assertion either. + +### 5.2 Comparisons refuse degenerate inputs + +The helpers mirror the bash harness, which added them for issue #418 after +"empty compared with empty" printed PASS. + +- `expect_num(got, want, name)` requires both sides to be numbers. +- `expect_rows(got, want, name)` refuses two empty results unless the test says + `allow_empty=True` and gives a reason. +- `expect_hash(got, want, name)` refuses two equal hashes that are the error + sentinel, and refuses comparing a value against itself. +- `expect_text(got, want, name)` requires a non-empty expectation. + +The bash `check_num` already refuses two identical md5 hashes. That rule ports +directly. + +### 5.3 A failed query can never compare equal to another failed query + +`pgc_set_hash` returns `QUERY_ERROR.`, a different value each time, so two +broken queries never match. The Python layer keeps that idea but does better: +psycopg raises. Measured, a query against a missing table raises `UndefinedTable` +rather than returning an empty result. + +The risk moves rather than disappearing. After any failed statement, every later +statement on that connection raises `InFailedSqlTransaction` until rollback. So +one broad `except` around a test body swallows the real error and every error +after it. **The layer therefore forbids bare `except Exception` in test code**, and +a lint rule enforces it. Where a test must catch, it catches the specific class. + +### 5.4 Plan assertions read a typed field, never a substring + +Measured on a columnar scan: + +``` +Node Type='Aggregate' Custom Plan Provider=None + Node Type='Custom Scan' Custom Plan Provider='PgColumnarScan' +``` + +`EXPLAIN (FORMAT JSON)` arrives as a parsed Python list. The assertion is exact +equality on `Custom Plan Provider`. A superstring cannot satisfy it. Note that the +provider name really is `PgColumnarScan`, which is the string that made +`grep ColumnarScan` unfalsifiable in the first place. + +The helper `expect_plan_node(plan, node_type=..., provider=...)` walks the tree and +refuses a substring argument. + +### 5.5 A skip must be declared, and a silent skip fails the run + +Skips are allowed only through `pgc_unrunnable(reason)`, which takes a reason from +a closed list, the same list the bash harness uses. The run then exits non-zero +unless the caller passed an explicit allowance. A bare `pytest.mark.skip` is +rejected by the plugin. + +`xfail` is configured strict by default, so an unexpected pass fails. + +### 5.6 The run asserts its own shape + +The harness records the number of tests it expects to collect. The CI step +compares collected against expected and fails on any difference, so a filtered, +truncated or empty run cannot be green. Exit code 5 is mapped to failure +explicitly. This mirrors `pgc_summary`, which reconciles passed plus failed plus +unrunnable against the total and fails when the arithmetic does not close. + +### 5.7 The binary under test is fingerprinted + +A session fixture records the `.so` md5 and the server's +`pg_postmaster_start_time()`, and fails if the library is older than the running +server. This is the bash `pgc_so_line` guard, which exists because a suite once +reported a full pass against a previously installed library. + +## 6. Fixtures and parallelism + +One cluster per xdist worker, created once per session and owned exclusively by +that worker. Each test gets its own schema inside that cluster, so tests are +isolated without paying for a cluster each. + +Why not one shared cluster: two workers installing the extension into one +`pkglibdir` race, and the bash harness has already been bitten by a port collision +that read like a real failure. + +Worker count is capped. The machine has 8 cores shared with a desktop, and the +bash matrix already runs 6 suites at once. The default is 4, overridable, and +never `-n auto`. + +Ports come from the same band the bash harness uses, below the ephemeral floor, +derived from the worker id rather than picked at random. + +## 7. Proving the port against the bash harness + +A port that agrees with the original on green proves little. It has to agree on +red as well. + +For each ported suite: + +1. Run the bash suite. Record every check name and outcome. +2. Run the pytest port. Record every test name and outcome. +3. Compare property by property. A property in one and not the other is a defect + in the port, not a difference of style. +4. Apply a mutation to the extension that the property is meant to catch. +5. **Both harnesses must go red.** If the bash suite reddens and the port does + not, the port is not asserting the property. If neither reddens, the property + was never covered and the mutation is the wrong one. + +Step 5 is the check that matters. It is the only one that can tell a real +assertion from a vacuous one. + +## 8. The order of work + +Each step names the test that must fail first, and why it fails before the code +exists. + +| # | test | fails before, because | +|---|---|---| +| 1 | `test_layer_rejects_a_test_with_no_assertion` | no plugin exists, so the empty test passes | +| 2 | `test_layer_rejects_two_empty_results` | `expect_rows` does not exist | +| 3 | `test_layer_rejects_a_self_comparison` | `expect_hash` does not exist | +| 4 | `test_layer_rejects_a_substring_plan_match` | `expect_plan_node` does not exist | +| 5 | `test_layer_rejects_a_bare_skip` | bare skip currently exits 0 | +| 6 | `test_layer_fails_on_a_collected_count_mismatch` | nothing records an expected count | +| 7 | `test_layer_fails_on_a_stale_library` | no fingerprint fixture exists | +| 8 | `test_cluster_fixture_gives_a_typed_connection` | no fixture exists | +| 9 | `test_two_workers_get_different_clusters` | no per-worker cluster exists | +| 10 | the first ported suite, property by property | the port does not exist | +| 11 | the differential, including the mutation arm | nothing compares the two harnesses | + +Tests 1 to 7 are the layer testing itself. They come first because a harness that +can report a false green makes every later result worthless. + +## 8a. What is built, and what it measured + +All of section 8 is implemented and green. The numbers below are runs, not estimates. + +``` +test/pytest/ 24 tests serial: 24 passed xdist -n 4: 24 passed +``` + +Seventeen of those tests are the layer testing itself. They run pytest inside +pytest through the `pytester` fixture, so each guard is proven to REFUSE rather +than assumed to. The layer's own tests obey the layer: they use the same recorder +every other test uses, because an exemption for the tests that prove the guard is +the first step to exempting everything else. + +### The differential, both arms + +`test/native_projection.sh` and its port `test/pytest/test_native_projection.py`, +each arm building and installing once so both harnesses measure the SAME library: + +``` +ARM A unmutated .so 8370e9b1beba bash 8 passed 0 failed pytest 7 passed 0 failed +ARM B fan-out neutered .so 9e9510593777 bash 0 passed 8 failed pytest 7 passed... 0, 7 failed +``` + +The mutation makes `PgColumnarProjectionFanoutRow` return without writing. Both +harnesses go red together, and the arm records the `.so` md5 each harness measured +so an arm where they differ is marked void rather than reported. + +### Property by property, by name + +`test/pytest/compare_to_bash.py` extracts the check names from the bash suite and +the assertion names from the port, and diffs the two sets: + +``` +bash checks: 8 (8 distinct) +pytest named assertions: 9 (9 distinct) +PROPERTIES IN THE BASH SUITE AND NOT IN THE PORT: none +ASSERTIONS IN THE PORT AND NOT IN THE BASH SUITE: premise: the DELETE removed rows +``` + +Names, not counts. The bash suite has 8 checks and the port has 7 tests, because +one test carries two of the bash assertions. A count comparison calls that a +defect. A name comparison does not, and it still catches a property asserted in one +harness and nowhere in the other. + +The extra assertion is an improvement the port makes: the bash suite deletes rows +and then compares the projection against the base, without first checking that the +DELETE removed anything. If it removed nothing, both arms are satisfied by a +projection that never changed. + +### The leak, measured + +After two gate matrix runs this container held **four** orphaned postmasters from +`/tmp/pgcolumnar-test.*` datadirs, aged 18 to 35 minutes, still running after their +runs had finished. The pytest harness left **zero**: no listeners on its port band, +no cluster directories, no postmasters. + +That was measured by walking `/proc//cmdline` for every postgres process. A +first attempt used `ps | grep`, which reported a leak that was the grep's own +enclosing command line. The bracket trick protects the pattern, not the command +line that contains it. + +### Three instrument defects found while building this + +Recorded because they are the same class the layer exists to prevent, and all three +produced a confident wrong number before they were caught. + +1. A red test failed on `ImportError` rather than on the guard's absence. The + module had to exist and simply not guard yet, or the test proved only that a + file was missing. +2. The differential piped its output through `head`, which truncated the summary + line. The result was unknown while the report looked complete. +3. A `grep -c " PASSED"` counted 6 of 7 tests, because the first test's outcome + shares a line with a fixture's print. This is why the property comparison reads + names. + +A fourth, in the same family: after the mutation arm, `git checkout` restored the +source but nothing rebuilt, so the next run tested the MUTATED library against +clean sources. The harness prints its `.so` fingerprint on every run, which is the +only reason it was visible. + +## 9. Verification + +| phase | proven done by | +|---|---| +| prerequisites | a requirements file, and CI installing from it | +| the layer | tests 1 to 7 red, then green, with the red output recorded | +| fixtures | tests 8 and 9, plus two workers running concurrently | +| the first port | property-by-property agreement with the bash suite | +| the differential | one mutation reddening both harnesses | +| the landing | the bash matrix still green, and the pytest run green, both in CI | + +## 10. What this layer still cannot catch + +Stated plainly, because a guard that claims to catch everything is the defect it +is meant to prevent. + +- A test that asserts a true and irrelevant property. Counting assertions cannot + tell whether the assertion is about the thing under test. +- A fixture that builds the wrong situation. If the fixture writes 100 rows to the + wrong table, every assertion about that table is sound and meaningless. +- A mutation that changes nothing observable. The differential in section 7 needs + a mutation the property can see, and choosing it is human work. +- A GUC set but a code path never engaged. The bash harness has been bitten here + and the fix was a positive marker, not a language change. +- Agreement between two harnesses that share a wrong assumption. + +The layer removes a class of accident. It does not remove the need to ask what a +test would have to see in order to fail. diff --git a/test/pytest/README.md b/test/pytest/README.md new file mode 100644 index 00000000..51aaf4e8 --- /dev/null +++ b/test/pytest/README.md @@ -0,0 +1,49 @@ +# Running the pytest harness + +This is the issue #432 pilot. It runs beside `test/*.sh`, and replaces nothing. +The design and the measurements behind each guard are in +`design/ISSUE_432_PYTEST_HARNESS.md`. + +## Prerequisites + +The interpreter is marked `EXTERNALLY-MANAGED`, so install into a virtual +environment rather than into system Python: + +```sh +apt-get install -y python3.14-venv # ensurepip is not in the base image +python3 -m venv /root/pyenv +/root/pyenv/bin/pip install -r test/pytest/requirements-test.txt +``` + +## Running it + +```sh +cd test/pytest +PYTHONPATH=. /root/pyenv/bin/pytest # serial +PYTHONPATH=. /root/pyenv/bin/pytest -n 4 # four workers +PYTHONPATH=. /root/pyenv/bin/pytest --pgc-expect-tests 24 # assert the run's shape +PGC_PG_CONFIG=/usr/local/pg19a/bin/pg_config PYTHONPATH=. /root/pyenv/bin/pytest +``` + +Each worker builds its own throwaway cluster on a port derived from its worker id, +and drops it at session end. Nothing survives a run. + +## Checking a port against its bash original + +```sh +/root/pyenv/bin/python test/pytest/compare_to_bash.py \ + test/native_projection.sh test/pytest/test_native_projection.py +``` + +It compares the two by assertion NAME and exits non-zero if the bash suite asserts +a property the port does not. A port keeps this working by passing each assertion +the same name string the bash check uses. + +## Warnings + +The vacuity layer is loaded through `pytest.ini` and cannot be turned off by a test +file. A test that concludes nothing fails, a bare skip fails the run, and a +comparison that could not have failed is refused. If a guard blocks something +legitimate, the escape hatches take a reason rather than a flag, and every one of +them is listed in the design document. Adding a new escape hatch needs a red test +that proves the guard still fires without it. diff --git a/test/pytest/compare_to_bash.py b/test/pytest/compare_to_bash.py new file mode 100644 index 00000000..875b9552 --- /dev/null +++ b/test/pytest/compare_to_bash.py @@ -0,0 +1,52 @@ +#!/usr/bin/env python3 +"""Compare a bash suite and its pytest port PROPERTY BY PROPERTY, by name. + +Counting is the wrong instrument. The bash suite has 8 checks and the port has 7 +tests, and that difference is legitimate: one pytest test carries two of the bash +assertions. A count comparison calls that a defect. A name comparison does not, +and it catches the thing that matters, which is a property asserted in one +harness and nowhere in the other. + +The port makes this possible by passing each assertion the SAME name string the +bash check uses. That is a convention the port must keep, so this script is also +what enforces it. +""" +import re +import sys + +bash_file, py_file = sys.argv[1], sys.argv[2] + +# bash: check "NAME" ... / check_num "NAME" ... / check_ratio "NAME" ... +bash_src = open(bash_file).read() +bash_names = re.findall(r'\bcheck(?:_num|_ratio|_text|_timing)?\s+"([^"]+)"', bash_src) + +# pytest: expect.(..., "NAME") and name="NAME" +py_src = open(py_file).read() +py_names = re.findall(r'expect\.\w+\([^)]*?"([^"]+)"\s*(?:,[^)]*)?\)', py_src, re.S) +py_names += re.findall(r'name\s*=\s*"([^"]+)"', py_src) + +bset, pset = set(bash_names), set(py_names) + +print(f"bash checks: {len(bash_names)} ({len(bset)} distinct)") +print(f"pytest named assertions: {len(py_names)} ({len(pset)} distinct)") +print() + +missing = sorted(bset - pset) +extra = sorted(pset - bset) + +print("PROPERTIES IN THE BASH SUITE AND NOT IN THE PORT:") +if missing: + for n in missing: + print(f" MISSING {n}") +else: + print(" none -- every bash property is asserted by name in the port") +print() +print("ASSERTIONS IN THE PORT AND NOT IN THE BASH SUITE:") +if extra: + for n in extra: + print(f" extra {n}") +else: + print(" none") +print() +print("VERDICT:", "PORT IS INCOMPLETE" if missing else "every bash property is covered") +sys.exit(1 if missing else 0) diff --git a/test/pytest/conftest.py b/test/pytest/conftest.py new file mode 100644 index 00000000..c3fe55e8 --- /dev/null +++ b/test/pytest/conftest.py @@ -0,0 +1,75 @@ +"""pgColumnar pytest harness: fixtures. + +The vacuity layer in pgc_vacuity.py is loaded for every run through the `-p` +argument in pytest.ini, not imported here, so that a test file cannot opt out of it. + +`pytester` is enabled because the layer's own tests run pytest inside pytest: a +guard is proven to REFUSE rather than assumed to. +""" + +import os +import shutil + +import psycopg +import pytest + +from pgc_cluster import make_cluster + +pytest_plugins = ["pytester"] + +DEFAULT_PG_CONFIG = "/usr/local/pg18a/bin/pg_config" + + +def pytest_addoption(parser): + parser.addoption( + "--pg-config", + action="store", + default=os.environ.get("PGC_PG_CONFIG", DEFAULT_PG_CONFIG), + help="pg_config of the server build under test (assert-enabled, per lib.sh)", + ) + + +@pytest.fixture(scope="session") +def pgc_cluster(request, worker_id): + """One cluster per xdist worker, built once and torn down at session end.""" + pg_config = request.config.getoption("--pg-config") + cluster, root = make_cluster(pg_config, worker_id) + # Printed for the same reason lib.sh prints it: so a reader can tell which + # binary produced the results below. + print(f"\n-- cluster: worker={worker_id} port={cluster.port} " + f"{cluster.version} .so={cluster.so_md5()}") + try: + with psycopg.connect(cluster.dsn(), autocommit=True) as conn: + conn.execute("CREATE EXTENSION IF NOT EXISTS pgcolumnar") + yield cluster + finally: + cluster.stop() + shutil.rmtree(root, ignore_errors=True) + + +@pytest.fixture +def pgc_conn(pgc_cluster, request): + """A direct connection, in a schema private to this one test. + + A private schema rather than a private database: isolation without paying an + initdb per test. It is first on the search_path, so an unqualified CREATE TABLE + lands in it and cannot collide with another test's fixture. + + autocommit is ON deliberately. With it off, a fixture that forgets to commit + 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. + """ + schema = "pgc_test_" + "".join( + ch if ch.isalnum() else "_" for ch in request.node.name + )[:48] + conn = psycopg.connect(pgc_cluster.dsn(), autocommit=True) + try: + conn.execute(f'DROP SCHEMA IF EXISTS "{schema}" CASCADE') + conn.execute(f'CREATE SCHEMA "{schema}"') + conn.execute(f'SET search_path TO "{schema}", public') + yield conn + finally: + try: + conn.execute(f'DROP SCHEMA IF EXISTS "{schema}" CASCADE') + finally: + conn.close() diff --git a/test/pytest/pgc_cluster.py b/test/pytest/pgc_cluster.py new file mode 100644 index 00000000..295def0c --- /dev/null +++ b/test/pytest/pgc_cluster.py @@ -0,0 +1,161 @@ +"""pgColumnar pytest harness: the cluster a worker owns, and the connection to it. + +Design notes that are load-bearing, from design/ISSUE_432_PYTEST_HARNESS.md: + +- ONE CLUSTER PER XDIST WORKER, not one shared and not one per test. Two workers + installing the extension into a single pkglibdir race with each other, and a + shared cluster lets one test see another's tables. A cluster per test would cost + an initdb each. +- THE PORT IS DERIVED FROM THE WORKER ID, not picked at random, and sits below the + ephemeral floor. The bash harness took a random port and then needed a retry, + because two suites on one box start on the same port and the loser reports a wall + of errors with no named check failing. +- initdb AND pg_ctl ARE BINARIES, so they are subprocesses. That is the whole + psql-exception list for this file. Everything after the server is up goes over a + libpq connection: no psql, no text parsing. +- THE SERVER RUNS AS postgres WHEN WE ARE ROOT. initdb and postgres refuse to run + as root, so the harness uses runuser, exactly as lib.sh does. +""" + +import os +import pathlib +import shutil +import subprocess +import tempfile + +# Below the ephemeral floor so a test cluster cannot collide with a kernel-assigned +# port. The bash harness keeps to the same band. +PORT_BASE = 54600 + + +class Cluster: + """A throwaway cluster owned by one xdist worker.""" + + def __init__(self, pg_config, worker_id, datadir, port): + self.pg_config = pg_config + self.worker_id = worker_id + self.datadir = datadir + self.port = port + self.bindir = _pg_config(pg_config, "--bindir") + self.version = _pg_config(pg_config, "--version") + self.libdir = _pg_config(pg_config, "--pkglibdir") + self._started = False + + # -- the dsn every test connects through ------------------------------- + def dsn(self, dbname="postgres"): + return f"host=127.0.0.1 port={self.port} user=postgres dbname={dbname}" + + @property + def so_path(self): + return os.path.join(self.libdir, "pgcolumnar.so") + + def so_md5(self): + """Fingerprint the library under test. + + lib.sh prints this on every run because a suite once reported a full pass + against a previously installed library. The Python harness keeps it for the + same reason. + """ + out = _run(["md5sum", self.so_path]) + return out.split()[0][:12] + + # -- lifecycle, the only place a binary is invoked --------------------- + def initdb(self): + _asroot(["initdb", "-D", str(self.datadir), "-A", "trust", "-U", "postgres"], + self.bindir, self.datadir) + conf = self.datadir / "postgresql.conf" + with open(conf, "a") as fh: + fh.write( + "\n".join( + [ + "", + f"port={self.port}", + "listen_addresses='127.0.0.1'", + "shared_preload_libraries='pgcolumnar'", + # Deterministic output so a hash oracle means the same thing + # on every machine. lib.sh sets the same three. + "extra_float_digits=3", + "timezone='UTC'", + "lc_messages='C'", + # A test that hangs should fail, not wedge the run. + "statement_timeout='120s'", + "log_min_messages=warning", + "", + ] + ) + ) + + def start(self): + _asroot(["pg_ctl", "-D", str(self.datadir), "-l", + str(self.datadir / "server.log"), "-w", "start"], + self.bindir, self.datadir) + self._started = True + + def stop(self): + if not self._started: + return + _asroot(["pg_ctl", "-D", str(self.datadir), "-m", "immediate", "-w", "stop"], + self.bindir, self.datadir, check=False) + self._started = False + + def is_ours(self): + """Does the server on our port run from OUR datadir? + + lib.sh asks this because `pg_ctl -w` proves only that SOMETHING answers on + the port. A foreign cluster answering would let every later check run + against the wrong server. + """ + import psycopg + + with psycopg.connect(self.dsn()) as conn: + with conn.cursor() as cur: + cur.execute("SHOW data_directory") + live = pathlib.Path(cur.fetchone()[0]).resolve() + return live == self.datadir.resolve() + + +def _pg_config(pg_config, flag): + return _run([pg_config, flag]).strip() + + +def _run(argv, check=True): + proc = subprocess.run(argv, capture_output=True, text=True) + if check and proc.returncode != 0: + raise RuntimeError(f"{argv!r} failed rc={proc.returncode}: {proc.stderr.strip()}") + return proc.stdout + + +def _asroot(argv, bindir, datadir, check=True): + """Run a server binary, dropping to postgres when we are root. + + initdb, pg_ctl and postgres all refuse to run as root. lib.sh uses runuser for + the same reason, and the datadir has to be owned by postgres before they run. + """ + exe = os.path.join(bindir, argv[0]) + cmd = [exe] + argv[1:] + if os.geteuid() == 0: + shutil.chown(datadir, user="postgres") + for path in datadir.rglob("*"): + shutil.chown(path, user="postgres") + cmd = ["runuser", "-u", "postgres", "--"] + cmd + return _run(cmd, check=check) + + +def make_cluster(pg_config, worker_id): + """Create and start a cluster for one worker. The caller stops it.""" + slot = 0 if worker_id in (None, "master") else int(str(worker_id).lstrip("gw") or 0) + port = PORT_BASE + slot + root = pathlib.Path(tempfile.mkdtemp(prefix=f"pgc-pytest-{slot}-")) + os.chmod(root, 0o777) + datadir = root / "data" + datadir.mkdir() + cluster = Cluster(pg_config, worker_id, datadir, port) + cluster.initdb() + cluster.start() + if not cluster.is_ours(): + cluster.stop() + raise RuntimeError( + f"the server on port {port} is not ours: its data_directory differs " + f"from {datadir}. Refusing to test against a foreign cluster." + ) + return cluster, root diff --git a/test/pytest/pgc_vacuity.py b/test/pytest/pgc_vacuity.py new file mode 100644 index 00000000..7debc762 --- /dev/null +++ b/test/pytest/pgc_vacuity.py @@ -0,0 +1,320 @@ +"""pgColumnar pytest harness: the vacuity-refusal layer. + +A vacuity defect is a test that reports PASS while asserting nothing. Bare pytest +permits it in eight measured ways, all exiting 0, so this plugin is loaded for +every pgColumnar test rather than offered as a convention. The measurements are in +design/ISSUE_432_PYTEST_HARNESS.md section 4. + +Two rules run the file: + + 1. A test passes only if it made at least one COUNTED assertion. Counted means it + went through the `expect` recorder. A bare Python `assert` is not forbidden, + it simply does not satisfy the requirement, so a body that computes and + concludes nothing fails. + 2. Every comparison refuses its own degenerate cases. Both sides empty, a value + against itself, a substring where a typed field was meant. + +The bash harness earned each of these. `check_num`, `check_ratio` and `check_text` +were added for issue #418 after "empty compared with empty" printed PASS, and +`check_num` refuses two identical md5 hashes for the same reason. +""" + +import numbers + +import pytest + +# The sentinel a failed query yields, mirroring pgc_set_hash's QUERY_ERROR.$seq. +# Unique per occurrence so two failing queries can never compare equal and pass. +QUERY_ERROR = "QUERY_ERROR" +EMPTY = "EMPTY" + +# Reasons a test may declare itself unrunnable. Closed, exactly as lib.sh keeps it +# closed, so "skipped" cannot become a way to stop asserting things quietly. +UNRUNNABLE_REASONS = ( + "MISSING_DEPENDENCY", + "UNSUPPORTED_MAJOR", + "ABSENT_FIXTURE", + "UNAVAILABLE_ENDPOINT", + "UNMET_PRECONDITION", +) + +# Keyed by nodeid rather than held on the item, so an xdist worker sees only its +# own tests and two workers cannot share a counter. +_RECORDERS = {} + + +class VacuityError(AssertionError): + """Raised when an assertion could not have failed, or asserted nothing.""" + + +def _is_number(v): + # bool is an int in Python. A count that is True rather than 1 is a bug, not a + # number, so it is refused rather than silently compared. + return isinstance(v, numbers.Number) and not isinstance(v, bool) + + +def _empty(v): + return v is None or (hasattr(v, "__len__") and len(v) == 0) + + +class Expect: + """Records assertions, and refuses the ones that could not have failed.""" + + def __init__(self, nodeid): + self.nodeid = nodeid + self.count = 0 + self.unrunnable = None + + # -- the recorder ------------------------------------------------------- + def _counted(self): + self.count += 1 + + # -- numbers ----------------------------------------------------------- + def num(self, got, want, name): + """Compare two numbers. Refuses anything that is not a number. + + A string "100" compared with 100 is the psql-text-parsing bug this harness + exists to remove, so it is refused rather than coerced. + """ + if not _is_number(got) or not _is_number(want): + raise VacuityError( + f"{name}: num() needs numbers on both sides, got " + f"{type(got).__name__}={got!r} and {type(want).__name__}={want!r}. " + f"A text comparison here is the defect this harness removes." + ) + self._counted() + if got != want: + raise AssertionError(f"{name}: got {got!r} want {want!r}") + + # -- row sets ---------------------------------------------------------- + def rows(self, got, want, name, allow_empty=None): + """Compare two result sets. Refuses two empty sides unless declared. + + Both sides empty is issue #418: it passes while asserting nothing, because + a query that failed to return anything looks exactly like one that + correctly returned nothing. `allow_empty` takes a REASON, not a flag, so + the escape hatch costs more to type than the honest assertion. + """ + if _empty(got) and _empty(want) and not allow_empty: + raise VacuityError( + f"{name}: both sides are empty, so this comparison could not have " + f"failed. If an empty result is the point, pass " + f"allow_empty='why it is empty'." + ) + self._counted() + if list(got) != list(want): + raise AssertionError(f"{name}: got {got!r} want {want!r}") + + # -- hashes and oracles ------------------------------------------------ + def hash(self, got, want, name): + """Compare two oracle hashes. Refuses self-comparison and error sentinels.""" + if got is want: + raise VacuityError( + f"{name}: the same object is compared against itself, so this " + f"could not have failed." + ) + if isinstance(got, str) and got.startswith(QUERY_ERROR): + raise VacuityError(f"{name}: the left side is a failed query: {got!r}") + if isinstance(want, str) and want.startswith(QUERY_ERROR): + raise VacuityError(f"{name}: the right side is a failed query: {want!r}") + if _empty(got) and _empty(want): + raise VacuityError(f"{name}: both hashes are empty.") + self._counted() + if got != want: + raise AssertionError(f"{name}: got {got!r} want {want!r}") + + # -- text -------------------------------------------------------------- + def text(self, got, want, name): + """Compare text exactly. Refuses an empty expectation.""" + if _empty(want): + raise VacuityError( + f"{name}: the expected text is empty, so anything empty satisfies it." + ) + self._counted() + if got != want: + raise AssertionError(f"{name}: got {got!r} want {want!r}") + + # -- plans ------------------------------------------------------------- + def plan_node(self, plan, node_type=None, provider=None, name=None): + """Assert a node exists, by EXACT equality on a typed EXPLAIN JSON field. + + `EXPLAIN (FORMAT JSON)` arrives from psycopg as parsed Python, so there is + no text to grep. The provider of a columnar scan is `PgColumnarScan`, which + is precisely why `grep ColumnarScan` was unfalsifiable: the wanted string + is a substring of the real one. Equality on `Custom Plan Provider` cannot + be satisfied by a superstring. + """ + if node_type is None and provider is None: + raise VacuityError( + "plan_node() needs node_type or provider, or it asserts nothing." + ) + label = name or f"plan has node_type={node_type!r} provider={provider!r}" + + def walk(node, depth=0): + if isinstance(node, dict): + yield node + for key in ("Plan", "Plans"): + child = node.get(key) + if isinstance(child, dict): + yield from walk(child, depth + 1) + elif isinstance(child, list): + for entry in child: + yield from walk(entry, depth + 1) + elif isinstance(node, list): + for entry in node: + yield from walk(entry, depth + 1) + + seen_types, seen_providers = [], [] + for node in walk(plan): + nt = node.get("Node Type") + pv = node.get("Custom Plan Provider") + if nt is not None: + seen_types.append(nt) + if pv is not None: + seen_providers.append(pv) + if node_type is not None and nt != node_type: + continue + if provider is not None and pv != provider: + continue + self._counted() + return node + + raise AssertionError( + f"{label}: no node whose fields match exactly. " + f"Node Type values present: {seen_types!r}. " + f"Custom Plan Provider values present: {seen_providers!r}." + ) + + # -- bounds ------------------------------------------------------------- + def at_least(self, got, floor, name): + """Assert got >= floor. Both sides must be numbers. + + The bash harness spells this as a yes/no string built by `[ ... -ge N ]`, + which turns a number into text and then compares text. Keeping it numeric + means a non-number is refused instead of silently becoming "no". + """ + if not _is_number(got) or not _is_number(floor): + raise VacuityError( + f"{name}: at_least() needs numbers, got " + f"{type(got).__name__}={got!r} and {type(floor).__name__}={floor!r}" + ) + if floor <= 0: + raise VacuityError( + f"{name}: a floor of {floor!r} is satisfied by any count, so this " + f"asserts nothing." + ) + self._counted() + if not got >= floor: + raise AssertionError(f"{name}: got {got!r}, wanted at least {floor!r}") + + # -- the layer's own tests --------------------------------------------- + def outcomes(self, result, name, **want): + """Assert on an INNER pytest run's outcomes, and count it. + + The layer's own tests run pytest inside pytest, so their assertions are + about another run rather than about a query. They are still assertions and + the rule still applies to them: the guard has no exemption for the tests + that prove the guard. Adding one would be the first step to exempting + everything else. + """ + if not want: + raise VacuityError( + f"{name}: outcomes() with no expectation asserts nothing." + ) + self._counted() + result.assert_outcomes(**want) + + def run_failed(self, result, name): + """Assert an inner run exited non-zero, and count it.""" + self._counted() + if result.ret == 0: + raise AssertionError( + f"{name}: the inner run exited 0, so nothing refused it." + ) + + # -- the third state --------------------------------------------------- + def cannot_run(self, reason, detail=""): + """Declare this test unrunnable. Not a pass, and not a silent skip.""" + if reason not in UNRUNNABLE_REASONS: + raise VacuityError( + f"unrunnable reason {reason!r} is not one of {UNRUNNABLE_REASONS}" + ) + self.unrunnable = (reason, detail) + self._counted() + + +@pytest.fixture +def expect(request): + rec = Expect(request.node.nodeid) + _RECORDERS[request.node.nodeid] = rec + yield rec + _RECORDERS.pop(request.node.nodeid, None) + + +@pytest.hookimpl(wrapper=True) +def pytest_runtest_call(item): + """Fail a test that concluded nothing, after its body has run. + + After the body, deliberately. A test that raised has already failed, and its + assertion count is not the interesting fact about it. + """ + result = yield + rec = _RECORDERS.get(item.nodeid) + if rec is None or rec.count == 0: + raise VacuityError( + f"vacuity guard: {item.name} made no counted assertion. " + f"A test that concludes nothing must not report a pass. " + f"Use the `expect` fixture, or declare it unrunnable with a reason." + ) + return result + + +def pytest_addoption(parser): + parser.addoption( + "--pgc-expect-tests", + action="store", + type=int, + default=None, + help="how many tests this run must collect; a mismatch fails the run", + ) + + +def pytest_collection_finish(session): + """Assert the run's own shape, so a filtered or truncated run cannot be green. + + lib.sh does the equivalent in pgc_summary, which reconciles passed plus failed + plus unrunnable against the total and fails when the arithmetic does not close. + """ + want = session.config.getoption("--pgc-expect-tests") + if want is None: + return + if want <= 0: + raise pytest.UsageError( + f"--pgc-expect-tests {want} would be satisfied by a run that collected " + f"nothing, so it asserts nothing. Give the real number." + ) + got = len(session.items) + if got != want: + raise pytest.UsageError( + f"collected {got} test(s) but expected {want}. A run that quietly " + f"collects fewer tests than it should is a green that means nothing." + ) + + +def pytest_collection_modifyitems(config, items): + """Refuse a bare skip, which exits 0 and reads as success. + + Measured: two skipped tests report `2 skipped` and exit 0. A skip is allowed + only through expect.cannot_run(), which names a reason from a closed list. + """ + offenders = [] + for item in items: + for marker in ("skip", "skipif"): + if item.get_closest_marker(marker) is not None: + offenders.append(f"{item.name} carries a bare @pytest.mark.{marker}") + if offenders: + raise pytest.UsageError( + "bare skip is refused by the pgColumnar vacuity layer: " + + "; ".join(offenders) + + ". Use expect.cannot_run(REASON, detail) so the run cannot go quiet." + ) diff --git a/test/pytest/pytest.ini b/test/pytest/pytest.ini new file mode 100644 index 00000000..acf92eba --- /dev/null +++ b/test/pytest/pytest.ini @@ -0,0 +1,8 @@ +[pytest] +# The vacuity layer is loaded here rather than imported by a conftest, so that no +# test file and no invocation can opt out of it. +addopts = -p pgc_vacuity -ra +# strict: an xfail that unexpectedly passes is a result nobody was told about. +# Measured: non-strict xfail reports 1 xpassed and exits 0. +xfail_strict = true +testpaths = . diff --git a/test/pytest/requirements-test.txt b/test/pytest/requirements-test.txt new file mode 100644 index 00000000..6e3c6672 --- /dev/null +++ b/test/pytest/requirements-test.txt @@ -0,0 +1,9 @@ +# pgColumnar pytest harness. Pinned so CI installs what was tested, not whatever +# the runner happens to carry. See design/ISSUE_432_PYTEST_HARNESS.md section 2. +# +# psycopg[binary] rather than plain psycopg: the binary wheel carries libpq, so +# nothing compiles at install time and the harness does not depend on the runner +# having a libpq development package. +pytest==9.1.1 +pytest-xdist==3.8.0 +psycopg[binary]==3.3.5 diff --git a/test/pytest/test_connection.py b/test/pytest/test_connection.py new file mode 100644 index 00000000..8dcefbd2 --- /dev/null +++ b/test/pytest/test_connection.py @@ -0,0 +1,105 @@ +"""The cluster fixture and the direct connection, tested before they exist. + +The requirement is a direct typed connection. These tests assert on Python types +coming back from the server, which is the thing `psql -At` cannot give: it returns +the string "100" and leaves every conversion to the reader. +""" + +import decimal + + +def test_cluster_fixture_gives_a_typed_connection(pgc_conn, expect): + """count(*) must arrive as an int, not as text.""" + with pgc_conn.cursor() as cur: + cur.execute("SELECT count(*) FROM (SELECT generate_series(1,7)) s") + value = cur.fetchone()[0] + expect.num(value, 7, "count arrives as a number") + expect.text(type(value).__name__, "int", "and its Python type is int") + + +def test_the_extension_is_installed_and_columnar(pgc_conn, expect): + """The fixture must give a cluster with pgcolumnar loaded and usable.""" + with pgc_conn.cursor() as cur: + cur.execute("SELECT extversion FROM pg_extension WHERE extname = 'pgcolumnar'") + row = cur.fetchone() + expect.rows([row[0]] if row else [], ["1.0-alpha3"], "the extension version") + + +def test_a_columnar_table_round_trips_with_real_types(pgc_conn, expect): + """Typed results end to end, including the types psql flattens to text.""" + with pgc_conn.cursor() as cur: + cur.execute("CREATE TABLE t (id int, n numeric, f float8, b bytea, a int[]) USING pgcolumnar") + cur.execute("INSERT INTO t VALUES (1, 1.5, 2.5, '\\x00ff', ARRAY[1,2])") + cur.execute("SELECT id, n, f, b, a FROM t") + row = cur.fetchone() + expect.num(row[0], 1, "int column") + expect.text(type(row[1]).__name__, "Decimal", "numeric arrives as Decimal") + expect.num(row[1], decimal.Decimal("1.5"), "and holds its exact value") + expect.text(type(row[2]).__name__, "float", "float8 arrives as float") + expect.text(row[3].hex(), "00ff", "bytea arrives as bytes") + expect.rows(row[4], [1, 2], "array arrives as a list") + + +def test_the_plan_names_the_columnar_provider(pgc_conn, expect): + """EXPLAIN FORMAT JSON arrives parsed, and the provider matches exactly.""" + with pgc_conn.cursor() as cur: + cur.execute("CREATE TABLE p (id int, a int) USING pgcolumnar") + cur.execute("INSERT INTO p SELECT g, g%10 FROM generate_series(1,500) g") + cur.execute("SET enable_seqscan = on") + cur.execute("EXPLAIN (FORMAT JSON, COSTS OFF) SELECT count(*) FROM p WHERE a > 5") + plan = cur.fetchone()[0] + expect.text(type(plan).__name__, "list", "the plan arrives as parsed Python") + expect.plan_node(plan, provider="PgColumnarScan", name="the columnar provider") + + +def test_each_test_gets_its_own_schema(pgc_conn, expect): + """Isolation without a cluster per test: a private schema, first on the path.""" + with pgc_conn.cursor() as cur: + cur.execute("SHOW search_path") + path = cur.fetchone()[0] + cur.execute("SELECT current_schema()") + schema = cur.fetchone()[0] + expect.text(schema.startswith("pgc_test_"), True, "the schema is test-private") + expect.text(path.split(",")[0].strip().startswith("pgc_test_"), True, + "and it is first on the search path") + + +def test_the_worker_owns_its_own_cluster(pgc_cluster, expect): + """Under xdist each worker must own a cluster, not share one. + + Two workers installing into one pkglibdir race, and a shared cluster lets one + test see another's tables. + + This asserts the port is the one DERIVED FROM THIS WORKER'S ID, which is what + makes distinctness a property rather than a hope: the mapping from worker id to + port is injective, so if every worker's port matches its own id, no two workers + share a port. Asserting only "the port is an int" would have passed while every + worker sat on 54600. + """ + from pgc_cluster import PORT_BASE + + wid = pgc_cluster.worker_id + slot = 0 if wid in (None, "master") else int(str(wid).lstrip("gw") or 0) + expect.num(pgc_cluster.port, PORT_BASE + slot, + f"worker {wid} owns the port derived from its id") + expect.text(pgc_cluster.is_ours(), True, + "and the server answering there runs from our datadir") + + +def test_the_cluster_refuses_a_foreign_server(pgc_cluster, expect): + """The identity guard must be able to say NO, not just say yes. + + pg_ctl -w proves only that SOMETHING answers on the port. lib.sh added this + check because a foreign cluster answering would let every later assertion run + against the wrong server. A guard that has never returned False is not known to + work, so this points it at a datadir that is not ours and requires a False. + """ + import pathlib + + from pgc_cluster import Cluster + + impostor = Cluster(pgc_cluster.pg_config, pgc_cluster.worker_id, + pathlib.Path("/tmp/definitely-not-our-datadir"), + pgc_cluster.port) + expect.text(impostor.is_ours(), False, + "a server whose datadir differs is refused") diff --git a/test/pytest/test_layer.py b/test/pytest/test_layer.py new file mode 100644 index 00000000..0724001a --- /dev/null +++ b/test/pytest/test_layer.py @@ -0,0 +1,177 @@ +"""The vacuity-refusal layer, tested before it exists. + +Every test here asserts that the layer REFUSES something bare pytest accepts. +Measured on pytest 9.1.1, all eight of the modes below exit 0 with no plugin +loaded, which is why each test asserts on the INNER run's outcome rather than on +its own arithmetic. +""" + + +def test_layer_rejects_a_test_with_no_assertion(pytester, expect): + """A test body that concludes nothing must fail, not pass. + + Bare pytest: `1 passed`, exit 0. Measured. + """ + pytester.makeconftest("pytest_plugins = ['pgc_vacuity']") + pytester.makepyfile( + """ + def test_asserts_nothing(): + x = 1 + 1 + """ + ) + result = pytester.runpytest("-p", "pgc_vacuity") + expect.outcomes(result, "inner run outcomes", failed=1, passed=0) + result.stdout.fnmatch_lines(["*made no counted assertion*"]) + + +def test_a_counted_assertion_passes(pytester, expect): + """The positive control. A guard that rejects good tests gets switched off. + + This must pass for the same plugin that fails the test above. + """ + pytester.makepyfile( + """ + def test_concludes_something(expect): + expect.num(2 + 2, 4, "arithmetic still works") + """ + ) + result = pytester.runpytest("-p", "pgc_vacuity") + expect.outcomes(result, "inner run outcomes", passed=1, failed=0) + + +def test_layer_rejects_two_empty_results(pytester, expect): + """Empty compared with empty is the defect that produced issue #418. + + Bare pytest: `2 passed`, exit 0. Measured. + """ + pytester.makepyfile( + """ + def test_empty_vs_empty(expect): + got, want = [], [] + expect.rows(got, want, "two empty results") + """ + ) + result = pytester.runpytest("-p", "pgc_vacuity") + expect.outcomes(result, "inner run outcomes", failed=1, passed=0) + result.stdout.fnmatch_lines(["*both sides are empty*"]) + + +def test_layer_allows_an_empty_result_when_declared(pytester, expect): + """An empty result is a legitimate expectation when the test says so. + + After a bare TRUNCATE the correct end state IS an empty projection, so this + escape hatch has to exist. It requires a reason, so it cannot become the + default by being easier to type. + """ + pytester.makepyfile( + """ + def test_empty_is_the_point(expect): + expect.rows([], [], "empty after truncate", allow_empty="table was truncated") + """ + ) + result = pytester.runpytest("-p", "pgc_vacuity") + expect.outcomes(result, "inner run outcomes", passed=1, failed=0) + + +def test_layer_rejects_a_self_comparison(pytester, expect): + """Comparing a value against itself cannot fail, so it asserts nothing.""" + pytester.makepyfile( + """ + def test_hash_against_itself(expect): + h = "d41d8cd98f00b204e9800998ecf8427e" + expect.hash(h, h, "oracle against itself") + """ + ) + result = pytester.runpytest("-p", "pgc_vacuity") + expect.outcomes(result, "inner run outcomes", failed=1, passed=0) + result.stdout.fnmatch_lines(["*compared against itself*"]) + + +def test_layer_rejects_a_substring_plan_match(pytester, expect): + """`"ColumnarScan" in plan` is exactly as wrong as `grep ColumnarScan`. + + The measured provider name is `PgColumnarScan`, so the substring passes while + asserting the wrong thing. The helper takes a typed field and exact equality. + """ + pytester.makepyfile( + """ + PLAN = [{"Plan": {"Node Type": "Aggregate", "Plans": [ + {"Node Type": "Custom Scan", + "Custom Plan Provider": "PgColumnarScan"}]}}] + + def test_substring_is_refused(expect): + expect.plan_node(PLAN, provider="ColumnarScan") + """ + ) + result = pytester.runpytest("-p", "pgc_vacuity") + expect.outcomes(result, "inner run outcomes", failed=1, passed=0) + result.stdout.fnmatch_lines(["*no node whose*"]) + + +def test_layer_matches_the_exact_provider(pytester, expect): + """The positive control for the plan helper: the real name must match.""" + pytester.makepyfile( + """ + PLAN = [{"Plan": {"Node Type": "Aggregate", "Plans": [ + {"Node Type": "Custom Scan", + "Custom Plan Provider": "PgColumnarScan"}]}}] + + def test_exact_provider(expect): + expect.plan_node(PLAN, provider="PgColumnarScan") + """ + ) + result = pytester.runpytest("-p", "pgc_vacuity") + expect.outcomes(result, "inner run outcomes", passed=1, failed=0) + + +def test_layer_rejects_a_bare_skip(pytester, expect): + """A bare skip exits 0 and reports success. Measured: `2 skipped`, exit 0. + + A skip has to name a reason from the closed list, so a suite cannot quietly + stop testing anything. + """ + pytester.makepyfile( + """ + import pytest + @pytest.mark.skip(reason="not today") + def test_quietly_gone(): + assert False + """ + ) + result = pytester.runpytest("-p", "pgc_vacuity") + expect.run_failed(result, "a bare skip must not produce a zero exit") + # A UsageError is written to stderr, not stdout. Asserting on the wrong stream + # would have made this test pass on ret alone, which any collection error also + # satisfies -- so the message assertion is what makes it honest. + result.stderr.fnmatch_lines(["*bare skip*"]) + + +def test_layer_fails_on_a_collected_count_mismatch(pytester, expect): + """A filtered or truncated run must not be green. + + Measured: `-k` that matches nothing exits 5, and 5 is routinely treated as + acceptable by wrappers. Collecting fewer tests than expected is the same + accident with a zero exit, so the layer compares collected against expected. + """ + pytester.makepyfile( + """ + def test_one(expect): expect.num(1, 1, "one") + def test_two(expect): expect.num(2, 2, "two") + """ + ) + good = pytester.runpytest("-p", "pgc_vacuity", "--pgc-expect-tests", "2") + expect.outcomes(good, "the honest run is green", passed=2, failed=0) + + short = pytester.runpytest("-p", "pgc_vacuity", "--pgc-expect-tests", "2", + "-k", "test_one") + expect.run_failed(short, "a run that collected fewer than expected must fail") + # stderr, not stdout: a UsageError is written there. Getting this wrong twice + # while building the layer is why every message assertion here names its stream. + short.stderr.fnmatch_lines(["*collected 1 test*expected 2*"]) + + +def test_layer_refuses_a_zero_expectation(pytester, expect): + """--pgc-expect-tests 0 would make the guard vacuous, so it is refused.""" + pytester.makepyfile("def test_one(expect): expect.num(1, 1, 'one')") + result = pytester.runpytest("-p", "pgc_vacuity", "--pgc-expect-tests", "0") + expect.run_failed(result, "an expectation of zero asserts nothing") diff --git a/test/pytest/test_native_projection.py b/test/pytest/test_native_projection.py new file mode 100644 index 00000000..535d963e --- /dev/null +++ b/test/pytest/test_native_projection.py @@ -0,0 +1,119 @@ +"""Port of test/native_projection.sh, property for property. + +The original is 55 lines of bash with 8 assertions, no process work and nothing +timing-dependent, which is what makes it a fair first pilot. Every check in the +original appears here with the same name, so the two can be compared mechanically. + +ONE DELIBERATE DIFFERENCE IN MECHANISM. The bash suite compares +`md5(string_agg(t ORDER BY t))`. This port compares the row sets themselves, as +sorted Python lists. That asserts the same property by a stronger means: an md5 +mismatch tells you two hashes differ, a row-set mismatch tells you which row. It +also avoids recomputing the hash in Python, where encoding and collation could +make the same rows hash differently. +""" + +import pytest + +STRIPE_ROWS = 1000 +ROWS = 5000 + + +@pytest.fixture +def fo(pgc_conn): + """The original's fixture, verbatim in effect: 5000 rows, two projections.""" + with pgc_conn.cursor() as cur: + cur.execute("CREATE TABLE fo (a int, b text, c int) USING pgcolumnar") + cur.execute("SELECT pgcolumnar.set_options('fo', stripe_row_limit => %s)", + (STRIPE_ROWS,)) + cur.execute("SELECT pgcolumnar.add_projection('fo','fp',ARRAY['a','c'],ARRAY['c'])") + cur.execute("SELECT pgcolumnar.add_projection('fo','fq',ARRAY['b'])") + cur.execute( + "INSERT INTO fo SELECT g, 'r'||g, (g*7)%%100 FROM generate_series(1,%s) g", + (ROWS,), + ) + return pgc_conn + + +def _rows(conn, sql, params=None): + with conn.cursor() as cur: + cur.execute(sql, params) + return sorted(r[0] for r in cur.fetchall()) + + +def _scalar(conn, sql, params=None): + with conn.cursor() as cur: + cur.execute(sql, params) + return cur.fetchone()[0] + + +def _proj_storage(conn, table, name): + return _scalar( + conn, + "SELECT proj_storage_id FROM pgcolumnar.projection " + "WHERE storage_id = pgcolumnar.get_storage_id(%s) AND name = %s", + (table, name), + ) + + +def test_fp_fanout_matches_base(fo, expect): + """bash: 'fp fan-out matches base (a,c)'""" + got = _rows(fo, "SELECT pgcolumnar.read_projection('fo','fp')") + want = _rows(fo, "SELECT a::text || '|' || c::text FROM fo") + expect.rows(got, want, "fp fan-out matches base (a,c)") + + +def test_fq_fanout_matches_base(fo, expect): + """bash: 'fq fan-out matches base (b)'""" + got = _rows(fo, "SELECT pgcolumnar.read_projection('fo','fq')") + want = _rows(fo, "SELECT b FROM fo") + expect.rows(got, want, "fq fan-out matches base (b)") + + +def test_fp_row_count_matches_base(fo, expect): + """bash: 'fp row count matches base'. Both sides are ints here, not text.""" + got = _scalar(fo, "SELECT count(*) FROM pgcolumnar.read_projection('fo','fp')") + want = _scalar(fo, "SELECT count(*) FROM fo") + expect.num(got, want, "fp row count matches base") + + +def test_fp_storage_is_native(fo, expect): + """bash: 'fp storage is native'""" + n = _scalar(fo, "SELECT count(*) FROM pgcolumnar.storage WHERE storage_id = %s", + (_proj_storage(fo, "fo", "fp"),)) + expect.num(n, 1, "fp storage is native") + + +def test_fp_has_zone_maps(fo, expect): + """bash: 'fp has zone maps (native skip metadata)' + + The original turns this into the string 'yes' via `[ ... -ge 1 ]`. Here it + stays a number, so a non-number is refused rather than becoming 'no'. + """ + n = _scalar(fo, "SELECT count(*) FROM pgcolumnar.zone_map WHERE storage_id = %s", + (_proj_storage(fo, "fo", "fp"),)) + expect.at_least(n, 1, "fp has zone maps (native skip metadata)") + + +def test_fp_reflects_deletes(fo, expect): + """bash: 'fp reflects deletes (a,c)' and 'fp count after delete matches base'.""" + with fo.cursor() as cur: + cur.execute("DELETE FROM fo WHERE a BETWEEN 1000 AND 2000") + deleted = cur.rowcount + # The premise: the DELETE must actually have removed rows, or both arms below + # are satisfied by a projection that never changed. + expect.at_least(deleted, 1, "premise: the DELETE removed rows") + + got = _rows(fo, "SELECT pgcolumnar.read_projection('fo','fp')") + want = _rows(fo, "SELECT a::text || '|' || c::text FROM fo") + expect.rows(got, want, "fp reflects deletes (a,c)") + + pcount = _scalar(fo, "SELECT count(*) FROM pgcolumnar.read_projection('fo','fp')") + bcount = _scalar(fo, "SELECT count(*) FROM fo") + expect.num(pcount, bcount, "fp count after delete matches base") + + +def test_fp_spans_multiple_row_groups(fo, expect): + """bash: 'fp spans multiple projection row groups'""" + n = _scalar(fo, "SELECT count(*) FROM pgcolumnar.row_group WHERE storage_id = %s", + (_proj_storage(fo, "fo", "fp"),)) + expect.at_least(n, 2, "fp spans multiple projection row groups") From 1213625fcb0b132db6c2e4855493e851e8825501 Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Tue, 8 Sep 2026 23:30:59 +0000 Subject: [PATCH 02/11] test: document the pytest corpus, and fix its plan predicate (#432) Two things, and the second is a defect in the harness the first commit landed. THE PLAN PREDICATE WAS WIDER THAN THE HELPER IT PORTED. The harness asserted `Custom Plan Provider == "PgColumnarScan"` and called that the fix for the substring class, on the grounds that equality on a typed field cannot be satisfied by a superstring the way `grep ColumnarScan` was by `PgColumnarScan`. Equality was the right idea about the wrong field. Measured on 18.4: plain scan Custom Scan provider='PgColumnarScan' Projected Columns present ungrouped vector agg Custom Scan provider='PgColumnarScan' Projected Columns ABSENT grouped vector agg Custom Scan provider='PgColumnarScan' Projected Columns ABSENT columnar_vector.c:806 assigns the vectorized aggregate node &pgcolumnar_scan_methods, whose CustomName is PgColumnarScan (columnar_customscan.c:167). So every pgcolumnar node reports that provider. With the aggregate engaged the plan is a single node, the aggregate has absorbed the scan, and the predicate still claims a scan is present. pgc_is_columnar_scan answers no there, because it greps for `Columnar Projected Columns`, which only the scan's callback emits (columnar_customscan.c:3631). So expect.plan_marker() is added as the faithful port, expect.plan_node() keeps the provider question and now says in its docstring what that question is not, and test_the_provider_name_does_not_identify_a_scan pins all three facts so a revert reddens rather than passing quietly. PgColumnarAgg is noted as unreachable in a plan: it is the CustomName of a CustomPathMethods, and EXPLAIN prints the scan methods' name. This is the document's own argument in one example. Typed results removed the parsing accident and left the harder mistake untouched. The assertion was type-correct, exact, and about the wrong thing. DOCUMENTATION. test/pytest/TESTS.md, a per-test reference: every test with the property it asserts and the measured fact behind it, every assertion helper with what it REFUSES, the four controls that exist to catch a guard turning into a nuisance, the rules for adding a test, and the traps this corpus records. Each of its numbers was checked against the tree rather than written from memory. design/ISSUE_432_PYTEST_HARNESS.md section 5.4 is rewritten, because it recommended the predicate this commit removes. The test counts there are corrected and the harness's own defect is recorded beside the other instrument defects. 25 tests, green serially and under four xdist workers, with the collected count asserted at 25. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- design/ISSUE_432_PYTEST_HARNESS.md | 64 ++++++-- test/pytest/README.md | 6 +- test/pytest/TESTS.md | 244 +++++++++++++++++++++++++++++ test/pytest/pgc_vacuity.py | 76 ++++++--- test/pytest/test_connection.py | 55 ++++++- 5 files changed, 407 insertions(+), 38 deletions(-) create mode 100644 test/pytest/TESTS.md diff --git a/design/ISSUE_432_PYTEST_HARNESS.md b/design/ISSUE_432_PYTEST_HARNESS.md index 54fad396..f80f1f34 100644 --- a/design/ISSUE_432_PYTEST_HARNESS.md +++ b/design/ISSUE_432_PYTEST_HARNESS.md @@ -172,20 +172,44 @@ a lint rule enforces it. Where a test must catch, it catches the specific class. ### 5.4 Plan assertions read a typed field, never a substring -Measured on a columnar scan: +`EXPLAIN (FORMAT JSON)` arrives from psycopg as a parsed Python list, so there is no +text to grep and no substring to match. That much is straightforward. + +**The subtlety cost this harness a defect, so it is recorded rather than +summarised.** The obvious assertion is exact equality on `Custom Plan Provider`, +which for a columnar scan is `PgColumnarScan`. That is the string that made +`grep ColumnarScan` unfalsifiable, so equality on it looks like the fix. It is not +the fix, because that field does not identify a scan. Measured on 18.4: ``` -Node Type='Aggregate' Custom Plan Provider=None - Node Type='Custom Scan' Custom Plan Provider='PgColumnarScan' +plain scan Custom Scan provider='PgColumnarScan' Columnar Projected Columns present +ungrouped vector agg Custom Scan provider='PgColumnarScan' Columnar Projected Columns ABSENT +grouped vector agg Custom Scan provider='PgColumnarScan' Columnar Projected Columns ABSENT ``` -`EXPLAIN (FORMAT JSON)` arrives as a parsed Python list. The assertion is exact -equality on `Custom Plan Provider`. A superstring cannot satisfy it. Note that the -provider name really is `PgColumnarScan`, which is the string that made -`grep ColumnarScan` unfalsifiable in the first place. +`columnar_vector.c:806` assigns the vectorized aggregate node +`&pgcolumnar_scan_methods`, whose `CustomName` is `PgColumnarScan` +(`columnar_customscan.c:167`), so every pgcolumnar node reports that provider. With +the aggregate engaged the plan is a single node and the aggregate has absorbed the +scan, yet the predicate still says a scan is there. + +`pgc_is_columnar_scan` in `lib.sh` does not use the provider. It greps for +`Columnar Projected Columns`, which only the scan's explain callback emits +(`columnar_customscan.c:3631`); the aggregate callbacks emit +`Columnar Vectorized Aggregates`. So the faithful port is marker presence. + +Two helpers, and the distinction between them is the point: + +- `expect.plan_marker(plan, key, absent=False)` asks whether the columnar SCAN ran. + This is the port of `pgc_is_columnar_scan`. +- `expect.plan_node(plan, node_type=, provider=)` asks whether a pgcolumnar node + exists at all, which is a weaker and rarer question. + +`PgColumnarAgg` never appears in a plan. It is the `CustomName` of a +`CustomPathMethods`, and EXPLAIN prints the scan methods' name. -The helper `expect_plan_node(plan, node_type=..., provider=...)` walks the tree and -refuses a substring argument. +`test_the_provider_name_does_not_identify_a_scan` asserts all of this, so a revert +to the provider predicate reddens instead of passing quietly. ### 5.5 A skip must be declared, and a silent skip fails the run @@ -274,10 +298,11 @@ can report a false green makes every later result worthless. All of section 8 is implemented and green. The numbers below are runs, not estimates. ``` -test/pytest/ 24 tests serial: 24 passed xdist -n 4: 24 passed +test/pytest/ 25 tests serial: 25 passed xdist -n 4: 25 passed ``` -Seventeen of those tests are the layer testing itself. They run pytest inside +Ten of those tests are the layer testing itself, four of them controls proving a +guard does not reject legitimate work. They run pytest inside pytest through the `pytester` fixture, so each guard is proven to REFUSE rather than assumed to. The layer's own tests obey the layer: they use the same recorder every other test uses, because an exemption for the tests that prove the guard is @@ -331,6 +356,17 @@ first attempt used `ps | grep`, which reported a leak that was the grep's own enclosing command line. The bracket trick protects the pattern, not the command line that contains it. +### The harness's own plan assertion was wrong, and a fan-out design review caught it + +The first version asserted `Custom Plan Provider == "PgColumnarScan"` and called it +the fix for the substring class. It is wider than the bash helper it replaced: it +answers yes for a vectorized aggregate that absorbed the scan, where +`pgc_is_columnar_scan` answers no. Section 5.4 has the measurement. + +Worth stating plainly, because it is the whole argument of this document in one +example. Typed results removed the parsing accident and left the harder mistake +untouched. The assertion was type-correct, exact, and about the wrong thing. + ### Three instrument defects found while building this Recorded because they are the same class the layer exists to prevent, and all three @@ -350,6 +386,12 @@ source but nothing rebuilt, so the next run tested the MUTATED library against clean sources. The harness prints its `.so` fingerprint on every run, which is the only reason it was visible. +## 8b. Per-test reference + +`test/pytest/TESTS.md` documents every test: what it asserts, the measured fact +behind it, which helper it uses, and the four controls that exist to catch a guard +turning into a nuisance. It also lists the traps this corpus records. + ## 9. Verification | phase | proven done by | diff --git a/test/pytest/README.md b/test/pytest/README.md index 51aaf4e8..ec5f17ba 100644 --- a/test/pytest/README.md +++ b/test/pytest/README.md @@ -1,8 +1,10 @@ # Running the pytest harness This is the issue #432 pilot. It runs beside `test/*.sh`, and replaces nothing. -The design and the measurements behind each guard are in -`design/ISSUE_432_PYTEST_HARNESS.md`. + +- `TESTS.md` in this directory documents every test and every assertion helper. +- `design/ISSUE_432_PYTEST_HARNESS.md` holds the design and the measurements + behind each guard. ## Prerequisites diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md new file mode 100644 index 00000000..4a9b245e --- /dev/null +++ b/test/pytest/TESTS.md @@ -0,0 +1,244 @@ +# The pytest corpus: what each test asserts, and why it exists + +Reference for anyone reading, running, or adding to `test/pytest/`. The design and +the decisions behind the harness are in `design/ISSUE_432_PYTEST_HARNESS.md`. This +file covers the tests themselves. + +Twenty-five tests in three files. Ten of them test the harness rather than the +product, and they come first, because a harness that can report a false green makes +every other result in this directory worthless. + +Every measured fact quoted below was run. Where a test encodes a number or a +behaviour, the source of that number is named. + +## Contents + +- [1. How to read a test in here](#1-how-to-read-a-test-in-here) +- [2. The assertion vocabulary](#2-the-assertion-vocabulary) +- [3. test_layer.py: the guards, testing themselves](#3-test_layerpy-the-guards-testing-themselves) +- [4. test_connection.py: the cluster and the direct connection](#4-test_connectionpy-the-cluster-and-the-direct-connection) +- [5. test_native_projection.py: the ported suite](#5-test_native_projectionpy-the-ported-suite) +- [6. Adding a test](#6-adding-a-test) +- [7. Traps this corpus records](#7-traps-this-corpus-records) + +## 1. How to read a test in here + +Three rules apply to every test, and they are enforced rather than requested. + +**A test must make a counted assertion.** Counted means it went through the +`expect` fixture. A bare Python `assert` is allowed but does not satisfy the +requirement, so a body that computes and concludes nothing fails. This applies to +the tests that test the guards, too. An exemption there would be the first step to +exempting everything. + +**Every assertion carries a name.** The name is the first thing a reader sees in a +failure, and for a ported test it is the same string the bash check uses, which is +what lets `compare_to_bash.py` diff the two suites by property. + +**A premise gets its own assertion.** If a test deletes rows and then compares two +things, it asserts that the delete removed something first. Otherwise both +comparisons are satisfied by a table that never changed. + +## 2. The assertion vocabulary + +All of these live on the `expect` fixture, in `pgc_vacuity.py`. Each refuses its own +degenerate cases, and refusing raises `VacuityError` rather than failing an +assertion, so the two read differently in output. + +| helper | asserts | refuses | +| --- | --- | --- | +| `num(got, want, name)` | two numbers are equal | anything that is not a number, including `bool`, and including the string `"100"` that `psql -At` would have given | +| `at_least(got, floor, name)` | `got >= floor` | non-numbers, and a floor of zero or less, which any count satisfies | +| `rows(got, want, name, allow_empty=None)` | two result sets are equal | both sides empty, unless `allow_empty` gives a reason | +| `hash(got, want, name)` | two oracle hashes are equal | comparing an object against itself, either side being a `QUERY_ERROR` sentinel, both sides empty | +| `text(got, want, name)` | two strings are equal | an empty expectation, which anything empty satisfies | +| `plan_marker(plan, key, name, absent=False)` | some plan node carries a `Columnar` property key | nothing; `absent=True` inverts it | +| `plan_node(plan, node_type=, provider=)` | some node matches those fields exactly | being called with neither field, which would assert nothing | +| `outcomes(result, name, **want)` | an inner pytest run's outcomes | being called with no expectation | +| `run_failed(result, name)` | an inner run exited non-zero | nothing | +| `cannot_run(reason, detail)` | declares the test unrunnable | a reason outside the closed list | + +`rows` compares row sets rather than `md5(string_agg(...))`. That asserts the same +property as the bash oracle by a stronger means: a hash mismatch says two hashes +differ, a row-set mismatch says which row. It also avoids recomputing the hash in +Python, where encoding or collation could make identical rows hash differently. + +`UNRUNNABLE_REASONS` is the closed list `lib.sh` already uses: +`MISSING_DEPENDENCY`, `UNSUPPORTED_MAJOR`, `ABSENT_FIXTURE`, +`UNAVAILABLE_ENDPOINT`, `UNMET_PRECONDITION`. + +## 3. test_layer.py: the guards, testing themselves + +These ten run pytest inside pytest through the `pytester` fixture. Each writes a +small test file, runs it with the plugin loaded, and asserts on the INNER run's +outcome. That is what proves a guard REFUSES, rather than assuming it. + +Each row names the measured bare-pytest behaviour the guard exists to stop. Every +one of those eight measurements exited 0. + +| test | the guard | bare pytest, measured | +| --- | --- | --- | +| `test_layer_rejects_a_test_with_no_assertion` | a test that concludes nothing fails | `1 passed`, exit 0 | +| `test_a_counted_assertion_passes` | **positive control**: a real assertion still passes | — | +| `test_layer_rejects_two_empty_results` | empty compared with empty is refused | `2 passed`, exit 0 | +| `test_layer_allows_an_empty_result_when_declared` | **escape hatch**: an empty result with a reason passes | — | +| `test_layer_rejects_a_self_comparison` | a value compared against itself is refused | it cannot fail, so it passes | +| `test_layer_rejects_a_substring_plan_match` | a plan field is matched exactly, not by substring | `"ColumnarScan" in "…PgColumnarScan…"` passes | +| `test_layer_matches_the_exact_provider` | **positive control**: the real name matches | — | +| `test_layer_rejects_a_bare_skip` | a bare `@pytest.mark.skip` fails the run | `2 skipped`, exit 0 | +| `test_layer_fails_on_a_collected_count_mismatch` | a run that collects fewer tests than expected fails | a filtered run exits 5, widely treated as fine | +| `test_layer_refuses_a_zero_expectation` | `--pgc-expect-tests 0` is refused | it would be satisfied by collecting nothing | + +Four of the ten are controls rather than guards. They are not decoration. A guard +with a bad false-positive rate gets switched off, and then the guard it replaced is +gone too. `test_a_counted_assertion_passes` and +`test_layer_matches_the_exact_provider` exist so that a guard which starts +rejecting good tests reddens here first. + +The escape hatches are deliberately more expensive to type than the honest form. +`allow_empty` takes a reason, not `True`. `--pgc-expect-tests` takes the real +number. `cannot_run` takes a reason from a closed list. None of them can become the +default by being shorter. + +## 4. test_connection.py: the cluster and the direct connection + +| test | asserts | +| --- | --- | +| `test_cluster_fixture_gives_a_typed_connection` | `count(*)` arrives as a Python `int`, and its type is `int` | +| `test_the_extension_is_installed_and_columnar` | the fixture's cluster has `pgcolumnar` at the expected version | +| `test_a_columnar_table_round_trips_with_real_types` | `numeric` is `Decimal`, `float8` is `float`, `bytea` is `bytes`, an array is a `list` | +| `test_the_plan_shows_a_columnar_scan` | the plan arrives as parsed Python, and the scan ran | +| `test_the_provider_name_does_not_identify_a_scan` | **pins a trap**; see below | +| `test_each_test_gets_its_own_schema` | the schema is test-private and first on `search_path` | +| `test_the_worker_owns_its_own_cluster` | the port is the one derived from THIS worker's id | +| `test_the_cluster_refuses_a_foreign_server` | the identity check can return False | + +Two of these deserve their reasoning stated. + +**`test_the_worker_owns_its_own_cluster`** asserts `port == PORT_BASE + slot` for its +own worker, not merely that the port is an integer. The mapping from worker id to +port is injective, so if every worker's port matches its own id then no two workers +share one. Asserting "the port is an int" would have passed with every worker +sitting on 54600. + +**`test_the_cluster_refuses_a_foreign_server`** points the identity check at a +datadir that is not ours and requires `False`. `pg_ctl -w` proves only that +SOMETHING answers on the port, and `lib.sh` added this check because a foreign +cluster answering would let every later assertion run against the wrong server. A +guard that has never returned False is not known to work. + +### The trap that `test_the_provider_name_does_not_identify_a_scan` pins + +Measured on 18.4. `Custom Plan Provider == "PgColumnarScan"` does **not** mean "a +columnar scan ran": + +``` +plain scan 'Custom Scan' provider='PgColumnarScan' Columnar Projected Columns present +ungrouped vector agg 'Custom Scan' provider='PgColumnarScan' Columnar Projected Columns ABSENT +grouped vector agg 'Custom Scan' provider='PgColumnarScan' Columnar Projected Columns ABSENT +``` + +`columnar_vector.c:806` assigns the aggregate node `&pgcolumnar_scan_methods`, whose +`CustomName` is `PgColumnarScan` (`columnar_customscan.c:167`). So every pgcolumnar +node reports that provider. With the vectorized aggregate engaged the plan is a +single node and the aggregate has absorbed the scan, yet the provider still matches. + +`pgc_is_columnar_scan` in `lib.sh` greps for `Columnar Projected Columns`, which is +emitted only by the scan's callback (`columnar_customscan.c:3631`) and never by +either aggregate callback. So the faithful port is `plan_marker`, and the provider +predicate answers a weaker question. + +`PgColumnarAgg` never appears in a plan at all. It is the `CustomName` of a +`CustomPathMethods`, and EXPLAIN prints the scan methods' name. + +This test asserts all three facts, so reverting to the provider predicate reddens +here rather than passing quietly. The first version of this harness used the +provider predicate and was wrong in exactly this way. + +## 5. test_native_projection.py: the ported suite + +A complete port of `test/native_projection.sh`, chosen because it is 55 lines, has 8 +assertions, does no process work, and depends on nothing timing-related. + +| bash check | pytest test | +| --- | --- | +| `fp fan-out matches base (a,c)` | `test_fp_fanout_matches_base` | +| `fq fan-out matches base (b)` | `test_fq_fanout_matches_base` | +| `fp row count matches base` | `test_fp_row_count_matches_base` | +| `fp storage is native` | `test_fp_storage_is_native` | +| `fp has zone maps (native skip metadata)` | `test_fp_has_zone_maps` | +| `fp reflects deletes (a,c)` | `test_fp_reflects_deletes` | +| `fp count after delete matches base` | `test_fp_reflects_deletes` | +| `fp spans multiple projection row groups` | `test_fp_spans_multiple_row_groups` | + +Eight checks map to seven tests, because one test carries two of them. That is why +`compare_to_bash.py` compares names and not counts. + +The port adds one assertion the original lacks: `premise: the DELETE removed rows`. +Without it, both delete arms are satisfied by a projection that never changed. + +Two mechanical differences from the original, both deliberate: + +- The fan-out comparisons compare row sets, not `md5(string_agg(...))`. +- `fp has zone maps` and `fp spans multiple row groups` stay numeric through + `at_least`. The original turns each into the string `yes` or `no` via + `[ "$(...)" -ge 1 ]`, which converts a number to text and then compares text. A + non-number becomes `no` there and is refused here. + +### How this port is proved + +Running it green proves little on its own. It is proved by the differential: + +``` +ARM A unmutated .so 8370e9b1beba bash 8 passed 0 failed pytest 7 passed 0 failed +ARM B fan-out neutered .so 9e9510593777 bash 0 passed 8 failed pytest 0 passed 7 failed +``` + +The mutation makes `PgColumnarProjectionFanoutRow` return without writing. Each arm +builds and installs once, and both harnesses print the `.so` md5 they measured, so +an arm where the two differ is void rather than reported. + +## 6. Adding a test + +1. Write the failing test first and run it. Confirm it fails for the reason you + intend, not because a helper or module is missing. A red on `ImportError` proves + only that a file is absent. +2. Give every assertion a name. Porting a bash check means reusing its exact name + string. +3. Assert the premise. If a fixture is supposed to write rows, assert that it did. +4. If the test needs an escape hatch, give it a reason rather than a flag. +5. Run `compare_to_bash.py` if you are porting, and expect it to report every bash + property as covered. +6. Run serially and with `-n 4`. A test that passes only in one of those is + order-dependent or shares state. +7. If you add a guard, add the red test that proves it fires, and a control that + proves it does not fire on a legitimate test. + +## 7. 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. + +**A `UsageError` is written to stderr.** Two of the layer's tests assert on a +message and both were checking stdout at first. They still exited non-zero, so +`assert result.ret != 0` passed and the tests looked correct. Every message +assertion here now names its stream. + +**A red test can fail for the wrong reason.** The first guard's red state was +`ImportError: No module named 'pgc_vacuity'`. The module had to exist and simply +not guard yet before the red meant anything. + +**`git checkout` restores source, not the installed library.** After the mutation +arm, the source was clean and `/usr/local/pg18a` still held the mutated `.so`, so +the next run tested a mutated binary against clean sources. The harness prints its +`.so` fingerprint on every run, which is the only reason this was visible. + +**Counting is a fragile instrument.** A `grep -c " PASSED"` reported 6 of 7 tests, +because the first test's outcome shares a line with a fixture's print. A `head` in +the differential truncated its own summary line. Both produced a report that looked +complete. This is why the property comparison reads names. + +**`ps | grep "[p]attern"` can match its own shell.** The bracket protects the +enclosing command line only while the plain word appears nowhere else in it. A probe +whose body also contained `/tmp/pgc-pytest-*` counted its own invocation as a leaked +process. Walking `/proc//cmdline` is the reliable instrument. diff --git a/test/pytest/pgc_vacuity.py b/test/pytest/pgc_vacuity.py index 7debc762..e4c9b378 100644 --- a/test/pytest/pgc_vacuity.py +++ b/test/pytest/pgc_vacuity.py @@ -57,6 +57,22 @@ def _empty(v): return v is None or (hasattr(v, "__len__") and len(v) == 0) +def _plan_nodes(node): + """Every node of an EXPLAIN (FORMAT JSON) tree, as parsed by psycopg.""" + if isinstance(node, dict): + yield node + for key in ("Plan", "Plans"): + child = node.get(key) + if isinstance(child, dict): + yield from _plan_nodes(child) + elif isinstance(child, list): + for entry in child: + yield from _plan_nodes(entry) + elif isinstance(node, list): + for entry in node: + yield from _plan_nodes(entry) + + class Expect: """Records assertions, and refuses the ones that could not have failed.""" @@ -139,10 +155,19 @@ def plan_node(self, plan, node_type=None, provider=None, name=None): """Assert a node exists, by EXACT equality on a typed EXPLAIN JSON field. `EXPLAIN (FORMAT JSON)` arrives from psycopg as parsed Python, so there is - no text to grep. The provider of a columnar scan is `PgColumnarScan`, which - is precisely why `grep ColumnarScan` was unfalsifiable: the wanted string - is a substring of the real one. Equality on `Custom Plan Provider` cannot - be satisfied by a superstring. + no text to grep, and equality on a typed field cannot be satisfied by a + superstring the way `grep ColumnarScan` was by `PgColumnarScan`. + + BUT `provider="PgColumnarScan"` DOES NOT MEAN "a columnar SCAN". Measured: + the vectorized aggregate node reuses the scan's registered methods + (`columnar_vector.c:806` assigns `&pgcolumnar_scan_methods`), so every + pgcolumnar node reports that provider. With the vector aggregate engaged + there is a single Custom Scan node carrying `Columnar Vectorized + Aggregates` and NO `Columnar Projected Columns`, and this predicate still + says yes. `pgc_is_columnar_scan` says no, because it greps for the marker. + + So use `plan_marker` to ask "did the columnar SCAN run". Use this to ask + "is there a pgcolumnar node at all", which is a weaker and rarer question. """ if node_type is None and provider is None: raise VacuityError( @@ -150,22 +175,8 @@ def plan_node(self, plan, node_type=None, provider=None, name=None): ) label = name or f"plan has node_type={node_type!r} provider={provider!r}" - def walk(node, depth=0): - if isinstance(node, dict): - yield node - for key in ("Plan", "Plans"): - child = node.get(key) - if isinstance(child, dict): - yield from walk(child, depth + 1) - elif isinstance(child, list): - for entry in child: - yield from walk(entry, depth + 1) - elif isinstance(node, list): - for entry in node: - yield from walk(entry, depth + 1) - seen_types, seen_providers = [], [] - for node in walk(plan): + for node in _plan_nodes(plan): nt = node.get("Node Type") pv = node.get("Custom Plan Provider") if nt is not None: @@ -232,6 +243,33 @@ def run_failed(self, result, name): f"{name}: the inner run exited 0, so nothing refused it." ) + def plan_marker(self, plan, key, name=None, absent=False): + """Assert a plan node carries (or does not carry) a Columnar property KEY. + + This is the faithful port of `pgc_is_columnar_scan` (`lib.sh`), which greps + `EXPLAIN` output for `Columnar Projected Columns`. That marker is emitted + only by the scan's explain callback (`columnar_customscan.c:3631`) and never + by either aggregate callback, so its presence is what distinguishes a + columnar scan from a vectorized aggregate that absorbed one. + + Presence of a KEY, not equality of a VALUE, because the marker's value is a + count that legitimately varies. `absent=True` asserts the opposite, which is + how a test pins that a plan is NOT a scan. + """ + label = name or f"plan {'lacks' if absent else 'carries'} {key!r}" + found, seen = False, set() + for node in _plan_nodes(plan): + seen.update(k for k in node if k.startswith("Columnar")) + if key in node: + found = True + self._counted() + if absent and found: + raise AssertionError(f"{label}: the key is present and should not be.") + if not absent and not found: + raise AssertionError( + f"{label}: no node carries it. Columnar keys present: {sorted(seen)!r}" + ) + # -- the third state --------------------------------------------------- def cannot_run(self, reason, detail=""): """Declare this test unrunnable. Not a pass, and not a silent skip.""" diff --git a/test/pytest/test_connection.py b/test/pytest/test_connection.py index 8dcefbd2..4551aeef 100644 --- a/test/pytest/test_connection.py +++ b/test/pytest/test_connection.py @@ -40,16 +40,59 @@ def test_a_columnar_table_round_trips_with_real_types(pgc_conn, expect): expect.rows(row[4], [1, 2], "array arrives as a list") -def test_the_plan_names_the_columnar_provider(pgc_conn, expect): - """EXPLAIN FORMAT JSON arrives parsed, and the provider matches exactly.""" +def _plan(conn, sql, gucs=()): + with conn.cursor() as cur: + for g in gucs: + cur.execute(g) + cur.execute(f"EXPLAIN (FORMAT JSON, COSTS OFF) {sql}") + return cur.fetchone()[0] + + +def test_the_plan_shows_a_columnar_scan(pgc_conn, expect): + """EXPLAIN FORMAT JSON arrives parsed, and the SCAN is identified by its marker. + + By the marker, not by the provider name. See the next test for why. + """ with pgc_conn.cursor() as cur: cur.execute("CREATE TABLE p (id int, a int) USING pgcolumnar") cur.execute("INSERT INTO p SELECT g, g%10 FROM generate_series(1,500) g") - cur.execute("SET enable_seqscan = on") - cur.execute("EXPLAIN (FORMAT JSON, COSTS OFF) SELECT count(*) FROM p WHERE a > 5") - plan = cur.fetchone()[0] + plan = _plan(pgc_conn, "SELECT count(*) FROM p WHERE a > 5") expect.text(type(plan).__name__, "list", "the plan arrives as parsed Python") - expect.plan_node(plan, provider="PgColumnarScan", name="the columnar provider") + expect.plan_marker(plan, "Columnar Projected Columns", + name="the columnar scan ran") + + +def test_the_provider_name_does_not_identify_a_scan(pgc_conn, expect): + """Pin the trap: provider equality is WIDER than pgc_is_columnar_scan. + + Measured. `columnar_vector.c:806` assigns the aggregate node + `&pgcolumnar_scan_methods`, whose CustomName is `PgColumnarScan`, so every + pgcolumnar node reports that provider. With the ungrouped vector aggregate + engaged the plan is a SINGLE Custom Scan node carrying `Columnar Vectorized + Aggregates` and NO `Columnar Projected Columns` -- the aggregate absorbed the + scan. A test asserting the provider would say "there is a columnar scan" about + a plan that has none, which is what `pgc_is_columnar_scan` refuses to say. + + This test exists so that reverting to the provider predicate reddens here. + """ + with pgc_conn.cursor() as cur: + cur.execute("CREATE TABLE vp (id int, a int) USING pgcolumnar") + cur.execute("INSERT INTO vp SELECT g, g%100 FROM generate_series(1,20000) g") + cur.execute("ANALYZE vp") + plan = _plan(pgc_conn, "SELECT count(*) FROM vp", + ("SET pgcolumnar.enable_vectorization = on", + "SET pgcolumnar.enable_ungrouped_vector_agg = on")) + + # The premise: the vectorized aggregate must actually have engaged, or the rest + # of this test is about an ordinary plan and proves nothing. + expect.plan_marker(plan, "Columnar Vectorized Aggregates", + name="premise: the vector aggregate engaged") + # The provider still matches, which is the trap. + expect.plan_node(plan, provider="PgColumnarScan", + name="the provider matches even with no scan node") + # And the scan marker is absent, which is what makes the provider wrong here. + expect.plan_marker(plan, "Columnar Projected Columns", absent=True, + name="but no columnar SCAN marker is present") def test_each_test_gets_its_own_schema(pgc_conn, expect): From c851100eeb240bd3a64d80334264544c965eef09 Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Wed, 9 Sep 2026 00:37:51 +0000 Subject: [PATCH 03/11] docs: say why the pytest run is not in the gate, and what registering costs An unregistered test suite rots, so this needs a decision rather than a default. The decision has a measured price and the price was not what I assumed. pgc_skip in lib.sh DOES NOT SKIP. It increments the failure count and prints "A missing dependency is an environment defect, not a pass. Install it, or set PGC_ALLOW_MISSING_=1 to run knowingly without this coverage." That is a deliberate house rule and the right one. It also means a wrapper registered in SUITES cannot quietly stand aside on a machine without pytest. CI has no pytest, no psycopg and no xdist, so registering the run today makes every CI job red until ci.yml installs from test/pytest/requirements-test.txt. Registering is therefore one line in SUITES plus a real change to the gate every other PR depends on, and that belongs to whoever owns the gate rather than to the pilot that wants it. PGC_ALLOW_MISSING_PYTEST=1 in CI is worse than either option. It reads as coverage and provides none, which is the vacuity defect this whole document exists to prevent, one level up. So the design document now carries section 1a with that reasoning, the scope list names the omission explicitly rather than leaving it implied, and the README says plainly that nothing gates these tests yet. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- design/ISSUE_432_PYTEST_HARNESS.md | 36 +++++++++++++++++++++++++++++- test/pytest/README.md | 8 +++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/design/ISSUE_432_PYTEST_HARNESS.md b/design/ISSUE_432_PYTEST_HARNESS.md index f80f1f34..ffb08c67 100644 --- a/design/ISSUE_432_PYTEST_HARNESS.md +++ b/design/ISSUE_432_PYTEST_HARNESS.md @@ -32,10 +32,44 @@ In scope for the first landing: Out of scope, deliberately: -- Porting the other 254 suites. Nothing is deleted in this landing. +- Porting the other 255 suites. Nothing is deleted in this landing. - Replacing `test/run_all_versions.sh`. It stays the gate. - Porting any suite that starts, kills or crashes a server. - FreeBSD support. It is a reason to prefer Python, not a deliverable here. +- **Registering the pytest run in `SUITES` or in CI.** Section 1a says why that is + a decision rather than an oversight. + +## 1a. Why this is not registered in the gate yet, and what registering costs + +An unregistered test suite rots. So this needs a decision rather than a default, and +the decision has a measured price. + +`pgc_skip` in `lib.sh` **does not skip**. Read it: it increments the failure count and +prints + +``` +FAIL + A missing dependency is an environment defect, not a pass. Install + it, or set PGC_ALLOW_MISSING_=1 to run knowingly without this coverage. +``` + +That is a deliberate house rule and it is the right one. It also means a wrapper +registered in `SUITES` cannot quietly stand aside on a machine without pytest. CI has +no pytest, no psycopg and no xdist, so registering the run today makes **every** CI job +red until `ci.yml` installs them. + +So registering is one line in `SUITES` plus a real change to `ci.yml`: a step that +installs from `test/pytest/requirements-test.txt` on the suite jobs. That is a change +to the gate every other PR depends on, and it belongs to whoever owns the gate rather +than to the pilot that wants it. + +Until then the pytest run is a local command, documented in `test/pytest/README.md`, +and the bash matrix remains the only gate. Anyone reviewing this should treat "not +gated" as the open question it is, not as a claim that it does not matter. + +The alternative, `PGC_ALLOW_MISSING_PYTEST=1` in CI, is worse than either. It reads as +coverage and provides none, which is the vacuity defect this whole document exists to +prevent, one level up. ## 2. Prerequisites, measured diff --git a/test/pytest/README.md b/test/pytest/README.md index ec5f17ba..b5c13307 100644 --- a/test/pytest/README.md +++ b/test/pytest/README.md @@ -41,6 +41,14 @@ It compares the two by assertion NAME and exits non-zero if the bash suite asser a property the port does not. A port keeps this working by passing each assertion the same name string the bash check uses. +## This is not in the gate yet + +`test/run_all_versions.sh` does not run these tests, and neither does CI. That is a +decision with a price, recorded in section 1a of the design document: `pgc_skip` +treats a missing dependency as a failure rather than a skip, so registering this run +in `SUITES` would redden every CI job until `ci.yml` installs from +`requirements-test.txt`. Until someone takes that decision, run it by hand. + ## Warnings The vacuity layer is loaded through `pytest.ini` and cannot be turned off by a test From 8211b113168f542bfe603743bbb6404f59f96587 Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Wed, 9 Sep 2026 01:12:53 +0000 Subject: [PATCH 04/11] test: make compare_to_bash.py executable, as the harness selftest requires Found by gating the branch rather than by assuming python files are inert. The bash matrix failed on both majors: FAIL every script that declares an interpreter is executable: got [[1: test/pytest/compare_to_bash.py]] want [[]] harness_selftest.sh: FAILED test/selftest/300-a-test-script-must-be-runnable.sh requires that any file declaring an interpreter be executable, and the file was mode 644 with a #!/usr/bin/env python3 line. Mode is now 755 and recorded as 100755 in the index. harness_selftest is 261 of 261 with the fix. Nothing else in the matrix objected to the new directory: 244 of 246 on PG18 and 246 of 246 on PG19, with only the two expected PG19-only skips. The second failure in my first reproduction, "the installed .so is the one this run built ... built ", was an artifact of running the selftest under PGC_SKIP_BUILD=1. It passes when a build actually runs, so it was my probe rather than the branch, and it is worth naming because that check exists precisely to catch a suite reporting against a binary it did not build. TESTS.md now tells the next person that this directory inherits every rule the older ones follow, and names this one. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- test/pytest/TESTS.md | 6 ++++++ test/pytest/compare_to_bash.py | 0 2 files changed, 6 insertions(+) mode change 100644 => 100755 test/pytest/compare_to_bash.py diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 4a9b245e..d0ce5563 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -213,6 +213,12 @@ an arm where the two differ is void rather than reported. order-dependent or shares state. 7. If you add a guard, add the red test that proves it fires, and a control that proves it does not fire on a legitimate test. +8. Run `test/harness_selftest.sh`. **The harness's own selftests police this + directory too.** `test/selftest/300-a-test-script-must-be-runnable.sh` requires + that any file declaring an interpreter be executable, and the first version of + `compare_to_bash.py` was mode 644 with a `#!/usr/bin/env python3` line. That + 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. ## 7. Traps this corpus records diff --git a/test/pytest/compare_to_bash.py b/test/pytest/compare_to_bash.py old mode 100644 new mode 100755 From 9eb3358062bec585d3c89fd3e4ab65481d3d93df Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Wed, 9 Sep 2026 14:19:09 +0000 Subject: [PATCH 05/11] docs: record the pytest harness in the changelog (#432) This project records test-infrastructure changes in CHANGELOG.md -- test/build_all_versions.sh reporting its major count, test/selftest/ gaining a part, and others are all there -- and a PR here ships with its docs. I opened #897 without one. The entry states the coverage honestly: one bash suite ported, 0.18% of the 4,429 anchored assertions, and the refusal layer rather than the count is what the change is for. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- CHANGELOG.md | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 50adede9..3cf0cd00 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -194,6 +194,36 @@ true until the next version shipped. ## [1.0-alpha3] - 2026-09-02 ### Added +- A pytest harness beside the bash suites, with a layer that refuses tests which + assert nothing (#432). + + `test/pytest/` connects through `psycopg` rather than parsing `psql` output, so a + test can assert the TYPE as well as the value: `psql -At` returns text, and an + `int4` `1` and a `text` `'1'` are the same string to a bash oracle. + + This ports ONE bash suite. `test/` carries 4,429 anchored assertions across 256 + suites, so this is 0.18% of them and is not coverage. The layer is the point. + + A pytest run fails open in several ways this project has already been bitten by: + a test that asserts nothing passes, a filter that selects nothing exits 0, and a + fixture that skips greens every test under it. `pgc_vacuity.py` is loaded for + every run and refuses those shapes, along with an empty result compared with an + empty result, a value compared against itself, a plan matched by substring rather + than by typed key, an absence claim over an empty plan, `cursor.rowcount` of + `-1`, and a broad `except` found by walking the AST rather than by line regex. + `xfail_strict` is on, and `--pgc-expect-tests N` asserts the run's own shape and + refuses `N = 0`. + + Every refusal has a red test in `test_layer.py` that runs pytest inside pytest + and asserts on the INNER run's outcome, which is what proves a guard refuses + rather than assuming it. Each records the bare-pytest behaviour it exists to + stop; every one of those measurements exited 0. Four of the ten are positive + controls, so a guard that starts rejecting good tests reddens there first. + + Not registered in `test/run_all_versions.sh`. That would add a `psycopg` build + dependency to every CI leg for 0.18% of the assertions; `test/pytest/README.md` + records what registering would cost and what has to be true first. + - A nanosecond Arrow import says how many values lost precision, and still imports every row. From 089c240504e14f7f08ff16120de55ea162716a3b Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Wed, 9 Sep 2026 15:11:08 +0000 Subject: [PATCH 06/11] test/pytest: build before measuring, and pin every guard to its own message Four findings from @jdatcmd's review. The blocker first. THE HARNESS REPORTED GREEN AGAINST SOURCE THAT CANNOT COMPILE. Demonstrated with `#error THIS SOURCE IS BROKEN AND CANNOT BUILD` appended to src/columnar_projection.c and nothing rebuilt: pytest said 25 passed, exit 0, while test/native_projection.sh said FATAL and exited 1. I hit the same hole independently from the other direction twenty minutes earlier -- a build from another branch sat in the prefix, 43 passed, and the only failure was a hardcoded version string noticing by accident. My first fix compared the installed .control and .sql against the source. It would not have caught the review's case at all, because appending #error to a .c file leaves both artifacts byte-identical. It is deleted rather than kept. The harness now builds and installs, and refuses to report if either fails. It drives pgc_build_and_install, extracted UNCHANGED from pgc_setup in test/lib.sh, so there is one implementation instead of two that drift -- and drift here would be invisible in exactly the way the defect was. The extraction is inert: a normal suite still builds, installs and passes 73 checks; the function still refuses broken source with FATAL and non-zero; the unbroken control returns 0. The pkglibdir race that made this harness skip installing is a reason to serialise the install, not to skip it, so it runs once per session under a flock. The marker is keyed on a SOURCE FINGERPRINT as well as the prefix: keying on pg_config and major alone would skip the rebuild after a source edit, reintroducing the staleness this guard exists to stop, through the optimisation meant to make the guard cheap. THE PORT BAND WAS FALSE IN BOTH HALVES, and it broke runs in this session before the review arrived. ip_local_port_range is `32768 60999`, so PORT_BASE 54600 sat INSIDE the ephemeral range; something holding a 546xx port produced 15 cluster-start errors. portlib.sh's bands are MAIN [10000, 29568) and AUX [29768, 31768). The harness now READS the floor and uses portlib.sh's own arithmetic, drawing from AUX with a bind-test walk. The arm that asserted `port == PORT_BASE + slot` now asserts the invariant the constant violated -- below the kernel's ephemeral floor -- instead of a fresh constant. GUARDS THAT COULD BE DELETED WITH THE CORPUS STILL GREEN. Two causes needing one remedy: guards no test drove at all, and guards a test drove while asserting only the outcome, so a NEIGHBOURING guard's refusal satisfied the arm. expect.refusal(result, name, *patterns) requires the message as well as the failure, and refuses being called with no pattern so it cannot become the defect it removes. census before 13 guards 3 HELD 10 UNHELD census after 13 guards 13 HELD 0 UNHELD Three things writing those arms caught, all of them the point. refusal() refused my own misuse when I passed a pattern into the name slot. hash("", "", ...) never reaches the both-empty guard, because the guard above it is `got is want` and CPython interns "" -- that arm would have passed while asserting nothing about the guard it names. And the two error-sentinel guards stayed unheld until the arms named which SIDE was refused, because the ordinary comparison failure satisfied them otherwise. The extension-version arm no longer hardcodes a version. It reads pgcolumnar.control, which is durable across a release cycle -- #899 moved the tree to 1.0-alpha4 today -- and is the assertion the arm is named after: that the loaded extension is the one this source describes. 50 passed, exit 0, serial and under -n 4, exit codes read without a pipe. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- test/lib.sh | 71 +++++--- test/pytest/conftest.py | 29 ++- test/pytest/pgc_cluster.py | 286 +++++++++++++++++++++++++++++- test/pytest/pgc_vacuity.py | 23 +++ test/pytest/test_build_refusal.py | 199 +++++++++++++++++++++ test/pytest/test_connection.py | 60 +++++-- test/pytest/test_guards_pinned.py | 171 ++++++++++++++++++ 7 files changed, 798 insertions(+), 41 deletions(-) create mode 100644 test/pytest/test_build_refusal.py create mode 100644 test/pytest/test_guards_pinned.py diff --git a/test/lib.sh b/test/lib.sh index f4f8638f..e9e79e9a 100755 --- a/test/lib.sh +++ b/test/lib.sh @@ -134,6 +134,51 @@ pgc_so_line() { fi } +# pgc_build_and_install SRCDIR PG_CONFIG MAJOR +# +# Build and install the extension, or fail. Extracted from pgc_setup UNCHANGED so +# the pytest harness can drive the same implementation instead of carrying a +# second one: test/pytest/ never built or installed, so it reported 25 passed +# against source carrying `#error THIS SOURCE IS BROKEN AND CANNOT BUILD` +# (@jdatcmd, #897 review). Two implementations of "is the thing under test the +# thing in this tree" would drift, and the drift would be invisible in exactly +# the way that defect was. +# +# Returns non-zero rather than calling exit, so a caller that is not a suite -- +# the Python harness -- can turn it into its own kind of failure. pgc_setup +# passes the exit through, so bash behaviour is unchanged. +pgc_build_and_install() { + _pgc_bi_src="$1" + _pgc_bi_cfg="$2" + _pgc_bi_major="$3" + _pgc_bi_stamp="$_pgc_bi_src/.pgc_built_for_major" + _pgc_bi_had="$(cat "$_pgc_bi_stamp" 2>/dev/null | tr -dc '0-9')" + _pgc_bi_objs=no + [ -n "$(find "$_pgc_bi_src/src" -maxdepth 1 -name '*.o' -print -quit 2>/dev/null)" ] && _pgc_bi_objs=yes + # Objects from another major link but do not load (#536). + if [ "$(pgc_build_needs_clean "$_pgc_bi_had" "$_pgc_bi_major" "$_pgc_bi_objs")" = yes ]; then + pgc_build_stale_message "$_pgc_bi_had" "$_pgc_bi_major" + make -C "$_pgc_bi_src" clean PG_CONFIG="$_pgc_bi_cfg" >/dev/null 2>&1 || true + fi + echo "-- building" + if ! make -C "$_pgc_bi_src" PG_CONFIG="$_pgc_bi_cfg" >/dev/null; then + echo "FATAL: the build failed, so there is nothing new to test" >&2 + echo " (refusing to report checks against the previously installed .so)" >&2 + return 1 + fi + # Stamped only after a build that succeeded. printf '%s\n', NOT '%s\\n': + # the doubled backslash writes the four bytes 1 9 \ n, which only worked + # because the reader strips non-digits. Caught in review, not by a test. + pgc_write_build_stamp "$_pgc_bi_stamp" "$_pgc_bi_major" + echo "-- installing" + if ! make -C "$_pgc_bi_src" install PG_CONFIG="$_pgc_bi_cfg" >/dev/null; then + echo "FATAL: the install failed, so the .so under test is not the one just built" >&2 + echo " (refusing to report checks against the previously installed .so)" >&2 + return 1 + fi + return 0 +} + pgc_setup() { PGC_PG_CONFIG="${1:-/usr/local/pg17/bin/pg_config}" PGC_BINDIR="$("$PGC_PG_CONFIG" --bindir)" @@ -197,31 +242,7 @@ pgc_setup() { # installed .so and saw the same hash either side of a source change that could # not have produced it. if [ -z "${PGC_SKIP_BUILD:-}" ]; then - # Objects from another major link but do not load (#536). - _pgc_stamp="$PGC_SRCDIR/.pgc_built_for_major" - _pgc_had="$(cat "$_pgc_stamp" 2>/dev/null | tr -dc '0-9')" - _pgc_objs=no - [ -n "$(find "$PGC_SRCDIR/src" -maxdepth 1 -name '*.o' -print -quit 2>/dev/null)" ] && _pgc_objs=yes - if [ "$(pgc_build_needs_clean "$_pgc_had" "$PGC_MAJOR" "$_pgc_objs")" = yes ]; then - pgc_build_stale_message "$_pgc_had" "$PGC_MAJOR" - make -C "$PGC_SRCDIR" clean PG_CONFIG="$PGC_PG_CONFIG" >/dev/null 2>&1 || true - fi - echo "-- building" - if ! make -C "$PGC_SRCDIR" PG_CONFIG="$PGC_PG_CONFIG" >/dev/null; then - echo "FATAL: the build failed, so there is nothing new to test" >&2 - echo " (refusing to report checks against the previously installed .so)" >&2 - exit 1 - fi - # Stamped only after a build that succeeded. printf '%s\n', NOT '%s\\n': - # the doubled backslash writes the four bytes 1 9 \ n, which only worked - # because the reader strips non-digits. Caught in review, not by a test. - pgc_write_build_stamp "$_pgc_stamp" "$PGC_MAJOR" - echo "-- installing" - if ! make -C "$PGC_SRCDIR" install PG_CONFIG="$PGC_PG_CONFIG" >/dev/null; then - echo "FATAL: the install failed, so the .so under test is not the one just built" >&2 - echo " (refusing to report checks against the previously installed .so)" >&2 - exit 1 - fi + pgc_build_and_install "$PGC_SRCDIR" "$PGC_PG_CONFIG" "$PGC_MAJOR" || exit 1 else # Named because the variable is not what it says. It reads as "skip the # build" and means "skip the build AND the install, and test whatever is diff --git a/test/pytest/conftest.py b/test/pytest/conftest.py index c3fe55e8..ca382576 100644 --- a/test/pytest/conftest.py +++ b/test/pytest/conftest.py @@ -8,12 +8,14 @@ """ import os +import re +import pathlib import shutil import psycopg import pytest -from pgc_cluster import make_cluster +from pgc_cluster import build_once, make_cluster pytest_plugins = ["pytester"] @@ -29,6 +31,16 @@ def pytest_addoption(parser): ) +SRCDIR = pathlib.Path(__file__).resolve().parents[2] + + +def major_of(version_text): + """"PostgreSQL 18.4" -> "18". The build stamp is per major, because objects + from another major link and then fail to load (#536).""" + m = re.search(r"(\d+)", version_text or "") + return m.group(1) if m else "" + + @pytest.fixture(scope="session") def pgc_cluster(request, worker_id): """One cluster per xdist worker, built once and torn down at session end.""" @@ -38,6 +50,21 @@ def pgc_cluster(request, worker_id): # binary produced the results below. print(f"\n-- cluster: worker={worker_id} port={cluster.port} " f"{cluster.version} .so={cluster.so_md5()}") + # AND THE BINARY IS BUILT FROM THIS TREE, which the fingerprint above never + # established. Printing a fingerprint tells a reader which binary ran; it + # does not stop the run when that binary came from somewhere else. + # + # Comparing the INSTALLED artifacts against the source was my first fix and + # it was not enough: appending `#error` to a .c file leaves the .control and + # the .sql byte-identical, so the corpus would still have reported 25 passed + # on source that cannot compile (@jdatcmd, #897 review). The only honest + # check is the one the bash harness has made since #536 -- build, install, + # and refuse to report if either fails. + # + # The install itself is the reason this harness skipped it: the workers + # share one pkglibdir. That is a reason to serialise it, not to skip it. + verdict = build_once(str(SRCDIR), pg_config, major_of(cluster.version)) + print(f"-- build: {verdict} from {SRCDIR}") try: with psycopg.connect(cluster.dsn(), autocommit=True) as conn: conn.execute("CREATE EXTENSION IF NOT EXISTS pgcolumnar") diff --git a/test/pytest/pgc_cluster.py b/test/pytest/pgc_cluster.py index 295def0c..79564903 100644 --- a/test/pytest/pgc_cluster.py +++ b/test/pytest/pgc_cluster.py @@ -17,15 +17,94 @@ as root, so the harness uses runuser, exactly as lib.sh does. """ +import fcntl +import hashlib import os +import re import pathlib import shutil +import socket import subprocess import tempfile -# Below the ephemeral floor so a test cluster cannot collide with a kernel-assigned -# port. The bash harness keeps to the same band. -PORT_BASE = 54600 +# THE OLD CONSTANT HERE WAS 54600, AND BOTH HALVES OF ITS COMMENT WERE FALSE +# (@jdatcmd, #897 review). It claimed to sit below the ephemeral floor and to +# match the bash harness. Measured: /proc/sys/net/ipv4/ip_local_port_range is +# `32768 60999`, so 54600 is INSIDE the ephemeral range, and test/portlib.sh's +# bands are MAIN [10000, 29568) and AUX [29768, 31768) -- nowhere near. The +# kernel handed out 54600, 54602, 54604 and 54606 during a 6000-connection +# probe, 54600 being the master worker's port, while the bash MAIN band took 0 +# of 6000. It cost me cluster start failures in this very session before the +# review arrived. +# +# The floor is READ, not assumed, and the band arithmetic is portlib.sh's own so +# the two harnesses cannot drift apart again. +DEFAULT_EPHEMERAL_FLOOR = 32768 +MIN_FLOOR = 20000 + + +def ephemeral_floor(range_text): + """The kernel's lowest ephemeral port, from ip_local_port_range's contents. + + Falls back to the documented default rather than guessing when the file is + unreadable or malformed, and never returns something so low that the bands + below it collapse. + """ + try: + low = int(str(range_text).split()[0]) + except (ValueError, IndexError, AttributeError): + return DEFAULT_EPHEMERAL_FLOOR + return low if low >= MIN_FLOOR else DEFAULT_EPHEMERAL_FLOOR + + +def read_ephemeral_floor(path="/proc/sys/net/ipv4/ip_local_port_range"): + try: + return ephemeral_floor(pathlib.Path(path).read_text()) + except OSError: + return DEFAULT_EPHEMERAL_FLOOR + + +def aux_band(floor): + """test/portlib.sh's AUX band, by its own arithmetic. + + PGC_AUX_PORT_HI = floor - 1000 + PGC_AUX_PORT_LO = AUX_HI - 2000 + + AUX rather than MAIN because AUX is for "extra clusters a single suite + stands up beyond its own", which is what an xdist worker is, and because the + matrix walks MAIN. + """ + hi = floor - 1000 + return hi - 2000, hi + + +def port_is_free(port, host="127.0.0.1"): + """Bind-test. A band is an argument about probability; a bind is a fact.""" + with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: + s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) + try: + s.bind((host, port)) + except OSError: + return False + return True + + +def pick_port(slot, floor=None, is_free=port_is_free): + """A free port in the AUX band for this worker slot. + + Walks on collision instead of trusting the band, because the band only makes + a collision unlikely and this harness has already been broken once by a port + that something else was holding. + """ + lo, hi = aux_band(floor if floor is not None else read_ephemeral_floor()) + span = hi - lo + start = slot * 2 + for step in range(span): + port = lo + ((start + step) % span) + if is_free(port): + return port + raise RuntimeError( + f"no free port in the AUX band [{lo}, {hi}) for worker slot {slot}") class Cluster: @@ -39,8 +118,67 @@ def __init__(self, pg_config, worker_id, datadir, port): self.bindir = _pg_config(pg_config, "--bindir") self.version = _pg_config(pg_config, "--version") self.libdir = _pg_config(pg_config, "--pkglibdir") + self.sharedir = _pg_config(pg_config, "--sharedir") self._started = False + # -- is the installed extension the one this checkout describes? ------- + @property + def extension_dir(self): + return os.path.join(self.sharedir, "extension") + + def _read(self, path): + try: + return pathlib.Path(path).read_text() + except OSError: + return None + + def install_freshness(self): + """(verdict, source version, installed version) for the prefix in use. + + The harness does not build. It runs against whatever is already in the + prefix, which is what makes not-rebuilding-per-test possible and is also + how a corpus ends up measuring another branch's extension. + """ + srcdir = pathlib.Path(__file__).resolve().parents[2] + src_ctl = self._read(srcdir / "pgcolumnar.control") + inst_ctl = self._read(os.path.join(self.extension_dir, "pgcolumnar.control")) + src_v = control_default_version(src_ctl) + inst_v = control_default_version(inst_ctl) + + src_sql = self._read(srcdir / f"pgcolumnar--{src_v}.sql") if src_v else None + inst_sql = (self._read(os.path.join(self.extension_dir, + f"pgcolumnar--{inst_v}.sql")) + if inst_v else None) + return install_verdict(src_v, inst_v, src_sql, inst_sql), src_v, inst_v + + def require_fresh_install(self): + """Refuse to run against an extension this checkout did not produce. + + Raises rather than skips. A skip here would green the whole corpus, and + "the extension was from another branch" is the one thing a skip must + never be allowed to say quietly. + + `unknown` does NOT raise: someone who installed by hand has no readable + pair to compare, and refusing would break a documented workflow. It says + which question went unanswered instead of printing nothing. + """ + verdict, src_v, inst_v = self.install_freshness() + if verdict == "stale": + raise RuntimeError( + "the installed pgcolumnar was not built from this source: " + f"this checkout declares {src_v!r}, {self.extension_dir} holds " + f"{inst_v!r}" + + (" (same version, different base script)" + if src_v == inst_v else "") + + ". Rebuild and reinstall before running the corpus; every " + "test below would otherwise report on code this tree does " + "not contain." + ) + if verdict == "unknown": + print(f"-- install freshness UNVERIFIED (source {src_v!r}, " + f"installed {inst_v!r})") + return verdict + # -- the dsn every test connects through ------------------------------- def dsn(self, dbname="postgres"): return f"host=127.0.0.1 port={self.port} user=postgres dbname={dbname}" @@ -114,6 +252,146 @@ def is_ours(self): return live == self.datadir.resolve() +_DEFAULT_VERSION_RE = re.compile( + r"""^\s*default_version\s*=\s*['"]([^'"]+)['"]""", re.MULTILINE) + + +def control_default_version(text): + """The default_version a .control file declares, or None. + + None rather than "" for absent, so a caller cannot confuse "no version + here" with "a version that is the empty string". + """ + if not text: + return None + m = _DEFAULT_VERSION_RE.search(text) + return m.group(1) if m else None + + +def install_verdict(src_version, installed_version, src_sql, installed_sql): + """Did the installed extension come from this source tree? + + A pure function of four strings, so it is driven directly by + test_install_freshness.py without a cluster -- the same shape test/lib.sh + uses for its own freshness verdicts. + + Exactly three values, and a caller's `if verdict == "stale"` depends on + that: "fresh", "stale", "unknown". + + UNKNOWN IS NOT FRESH. If either side is unreadable the honest answer is + that the question was not answered. Returning "fresh" there would certify + every run the harness failed to check, which is the failure this guard + exists to stop. + + The version alone is not enough. A release cycle is long and the base + install script changes inside it, so two builds can share a version and + differ. The script is compared as well, which is why the second arm of + test_same_version_but_a_different_script_is_stale exists. + """ + if not src_version or not installed_version: + return "unknown" + if src_version != installed_version: + return "stale" + if not src_sql or not installed_sql: + return "unknown" + return "fresh" if src_sql == installed_sql else "stale" + + +def build_and_install(srcdir, pg_config, major, runner=None): + """Build and install the extension, or raise. + + Drives `pgc_build_and_install` out of test/lib.sh rather than carrying a + second implementation. The bash harness has refused to report checks against + a previously installed library since #536; this harness did not, and reported + 25 passed against source carrying `#error THIS SOURCE IS BROKEN AND CANNOT + BUILD` (@jdatcmd, #897 review). Two implementations of "is the thing under + test the thing in this tree" would drift, and that drift would be invisible + in exactly the way that defect was. + + RAISES rather than skips. A skip greens the corpus, and "the source does not + compile" is the last thing that may be said quietly. + """ + srcdir = str(srcdir) + script = ( + f'. "{srcdir}/test/lib.sh" || exit 1; ' + f'pgc_build_and_install "{srcdir}" "{pg_config}" "{major}"' + ) + run = runner or (lambda argv: subprocess.run( + argv, capture_output=True, text=True)) + proc = run(["bash", "-c", script]) + if proc.returncode != 0: + raise RuntimeError( + f"pgcolumnar failed to build or install from {srcdir}; refusing to " + f"report checks against whatever was installed before.\n" + f"{(proc.stderr or '').strip()}\n{(proc.stdout or '').strip()}".strip() + ) + + +def source_fingerprint(srcdir): + """A hash of everything a build reads, or None if the tree is unreadable. + + Same input set as pgc_source_fingerprint in test/lib.sh: the C sources and + headers, the Makefile, the control file and the SQL scripts. Content, not + mtime, because a checkout or a branch switch rewrites mtimes without + changing what compiles, and `git stash` does the reverse. + """ + srcdir = pathlib.Path(srcdir) + paths = sorted( + list((srcdir / "src").glob("*.c")) + list((srcdir / "src").glob("*.h")) + + [p for p in (srcdir / "Makefile",) if p.exists()] + + sorted(srcdir.glob("*.control")) + sorted(srcdir.glob("*.sql")) + ) + if not paths: + return None + h = hashlib.md5() + for path in paths: + try: + h.update(path.name.encode()) + h.update(path.read_bytes()) + except OSError: + return None + return h.hexdigest()[:12] + + +def build_once(srcdir, pg_config, major, lock_path=None, runner=None): + """build_and_install, but at most once across xdist workers. + + pgc_cluster.py:6 gives the pkglibdir race as the reason this harness does not + install. That is a reason to serialise the install, not to skip it: the + workers share one prefix, so one of them builds under a lock and the rest + wait and then find the marker. + + The marker records the pg_config and major, so a second run against a + DIFFERENT prefix still builds -- keying it on "did anyone build" alone would + reintroduce the defect for anyone who runs the corpus twice against two + majors. + """ + lock_path = lock_path or os.path.join( + tempfile.gettempdir(), "pgc-pytest-build.lock") + marker = lock_path + ".done" + # THE FINGERPRINT IS PART OF THE KEY. Keying on pg_config and major alone + # would skip the build after a source edit, which is the staleness this + # whole guard exists to stop -- reintroduced by the optimisation meant to + # make the guard cheap. A tree we cannot fingerprint gets a key that never + # matches, so it always rebuilds. + fp = source_fingerprint(srcdir) + want = f"{pg_config}\n{major}\n{fp}\n" if fp else None + with open(lock_path, "w") as lf: + fcntl.flock(lf, fcntl.LOCK_EX) + try: + try: + if want is not None and pathlib.Path(marker).read_text() == want: + return "already-built" + except OSError: + pass + build_and_install(srcdir, pg_config, major, runner=runner) + if want is not None: + pathlib.Path(marker).write_text(want) + return "built" + finally: + fcntl.flock(lf, fcntl.LOCK_UN) + + def _pg_config(pg_config, flag): return _run([pg_config, flag]).strip() @@ -144,7 +422,7 @@ def _asroot(argv, bindir, datadir, check=True): def make_cluster(pg_config, worker_id): """Create and start a cluster for one worker. The caller stops it.""" slot = 0 if worker_id in (None, "master") else int(str(worker_id).lstrip("gw") or 0) - port = PORT_BASE + slot + port = pick_port(slot) root = pathlib.Path(tempfile.mkdtemp(prefix=f"pgc-pytest-{slot}-")) os.chmod(root, 0o777) datadir = root / "data" diff --git a/test/pytest/pgc_vacuity.py b/test/pytest/pgc_vacuity.py index e4c9b378..6832c129 100644 --- a/test/pytest/pgc_vacuity.py +++ b/test/pytest/pgc_vacuity.py @@ -219,6 +219,29 @@ def at_least(self, got, floor, name): raise AssertionError(f"{name}: got {got!r}, wanted at least {floor!r}") # -- the layer's own tests --------------------------------------------- + def refusal(self, result, name, *patterns): + """The inner run failed, AND it failed for the REASON named. + + `outcomes(result, failed=1)` alone is satisfied by any refusal, so a + guard whose neighbour catches the same input is pinned by nothing. A + mutation census over this layer found 12 of 17 guards deletable with the + corpus still green, and two of those were UNREACHABLE-by-subsumption + rather than untested: neuter `ordered_rows`'s both-empty guard and the + unobservable guard fires on the same input, so the inner run still fails + and an outcome-only assertion still passes (@jdatcmd, #897 review). + + Every pattern must appear. Naming the message is what makes the arm + about one guard instead of about the layer in general. + """ + if not patterns: + raise VacuityError( + f"{name}: refusal() with no pattern asserts only that something " + f"failed, which is the defect it exists to remove." + ) + self._counted() + result.assert_outcomes(failed=1, passed=0) + result.stdout.fnmatch_lines([f"*{p}*" for p in patterns]) + def outcomes(self, result, name, **want): """Assert on an INNER pytest run's outcomes, and count it. diff --git a/test/pytest/test_build_refusal.py b/test/pytest/test_build_refusal.py new file mode 100644 index 00000000..2f57a244 --- /dev/null +++ b/test/pytest/test_build_refusal.py @@ -0,0 +1,199 @@ +"""The corpus must not report on source it never built. + +@jdatcmd demonstrated the defect this closes: with +`#error THIS SOURCE IS BROKEN AND CANNOT BUILD` appended to +src/columnar_projection.c and nothing rebuilt, + + pytest -> 25 passed, exit 0 + bash test/native_projection.sh -> FATAL: the build failed ..., exit 1 + +The bash harness has refused that since #536. This one did not, because it never +built, never installed and never compared anything -- `Cluster.so_md5` printed a +fingerprint that nothing read. + +The refusal now comes from `pgc_build_and_install` in test/lib.sh, driven from +Python, so there is one implementation rather than two that can drift. + +Two levels of arm here, deliberately. The injected-runner arms pin what the +Python side does with a verdict. The `bash` arms pin the SHELL PLUMBING -- the +sourcing, the quoting and the exit-status path -- which an injected runner +cannot reach and which is where a wrong quote would hide. +""" + +import pathlib +import subprocess + +import pytest + +from pgc_cluster import build_and_install, build_once, source_fingerprint + + +class _Proc: + def __init__(self, rc, out="", err=""): + self.returncode, self.stdout, self.stderr = rc, out, err + + +def test_a_failed_build_raises_rather_than_returning(expect): + with pytest.raises(RuntimeError) as exc: + build_and_install("/nowhere", "/bin/false", "18", + runner=lambda argv: _Proc(1, err="compiler said no")) + expect.at_least(str(exc.value).count("refusing"), 1, + "the refusal says it is refusing") + expect.at_least(str(exc.value).count("compiler said no"), 1, + "and carries the build's own output, not a summary") + + +def test_the_refusal_names_the_tree_it_refused(expect): + """A reader with several worktrees needs to know WHICH source failed.""" + with pytest.raises(RuntimeError) as exc: + build_and_install("/root/some-worktree", "/bin/false", "18", + runner=lambda argv: _Proc(1)) + expect.at_least(str(exc.value).count("/root/some-worktree"), 1, + "the message names the source directory") + + +def test_a_successful_build_is_silent(expect): + """Positive control: the guard must not fire on a build that worked.""" + build_and_install("/nowhere", "/bin/false", "18", + runner=lambda argv: _Proc(0)) + expect.num(1, 1, "a returncode of 0 raises nothing") + + +def _fake_tree(tmp_path, verdict_rc): + """A tree whose test/lib.sh defines the function with a fixed verdict.""" + (tmp_path / "test").mkdir(parents=True, exist_ok=True) + (tmp_path / "test" / "lib.sh").write_text( + "pgc_build_and_install() {\n" + " echo '-- building'\n" + f" [ {verdict_rc} -eq 0 ] || {{ echo 'FATAL: the build failed' >&2; return 1; }}\n" + " return 0\n" + "}\n" + ) + return tmp_path + + +def test_the_shell_path_really_refuses(tmp_path, expect): + """The real bash invocation, not an injected runner: sourcing, quoting and + the exit status all have to work for the refusal to arrive.""" + tree = _fake_tree(tmp_path / "broken", 1) + with pytest.raises(RuntimeError) as exc: + build_and_install(tree, "/bin/false", "18") + expect.at_least(str(exc.value).count("FATAL: the build failed"), 1, + "the shell's own FATAL reaches the Python caller") + + +def test_the_shell_path_accepts_a_good_build(tmp_path, expect): + """Control for the arm above, through the same plumbing.""" + tree = _fake_tree(tmp_path / "ok", 0) + build_and_install(tree, "/bin/false", "18") + expect.num(1, 1, "a zero verdict through real bash raises nothing") + + +def test_a_missing_lib_sh_is_a_refusal_not_a_pass(tmp_path, expect): + """If lib.sh cannot be sourced there is no guard at all, so the harness must + fail rather than proceed ungated.""" + with pytest.raises(RuntimeError) as exc: + build_and_install(tmp_path / "empty", "/bin/false", "18") + expect.at_least(str(exc.value).count("refusing"), 1, + "an unsourceable lib.sh refuses") + + +def test_build_once_builds_once_and_then_skips(tmp_path, expect): + """The xdist workers share one prefix, so the install is serialised rather + than skipped.""" + # A fingerprintable tree: the marker is keyed on source content, so a tree + # with nothing to compile deliberately never certifies and always rebuilds. + tree = _tree_with_source(tmp_path, "ok2", "int a = 1;\n") + lock = str(tmp_path / "lock") + calls = [] + + def counting(argv): + calls.append(argv) + return _Proc(0) + + first = build_once(tree, "/bin/pg_config", "18", lock_path=lock, runner=counting) + second = build_once(tree, "/bin/pg_config", "18", lock_path=lock, runner=counting) + expect.text(first, "built", "the first caller builds") + expect.text(second, "already-built", "the second finds the marker") + expect.num(len(calls), 1, "and the build ran exactly once") + + +def test_build_once_rebuilds_for_a_different_prefix(tmp_path, expect): + """Keying the marker on 'did anyone build' would reintroduce the defect for + anyone who runs the corpus against two majors in turn.""" + tree = _tree_with_source(tmp_path, "ok3", "int a = 1;\n") + lock = str(tmp_path / "lock2") + calls = [] + + def counting(argv): + calls.append(argv) + return _Proc(0) + + build_once(tree, "/usr/local/pg18a/bin/pg_config", "18", lock_path=lock, runner=counting) + build_once(tree, "/usr/local/pg19a/bin/pg_config", "19", lock_path=lock, runner=counting) + expect.num(len(calls), 2, "a different prefix builds again") + + +def _tree_with_source(tmp_path, name, body): + """A fake tree that is fingerprintable: it has src/*.c and a Makefile.""" + tree = _fake_tree(tmp_path / name, 0) + (tree / "src").mkdir(parents=True, exist_ok=True) + (tree / "src" / "columnar.c").write_text(body) + (tree / "Makefile").write_text("all:\n\ttrue\n") + return tree + + +def test_editing_the_source_rebuilds(tmp_path, expect): + """The marker is keyed on the source fingerprint, not just the prefix. + + Keying on pg_config and major alone would skip the build after a source + edit -- the staleness this whole guard exists to stop, reintroduced by the + optimisation meant to make the guard cheap. + """ + tree = _tree_with_source(tmp_path, "edited", "int a = 1;\n") + lock = str(tmp_path / "lock3") + calls = [] + + def counting(argv): + calls.append(argv) + return _Proc(0) + + build_once(tree, "/bin/pg_config", "18", lock_path=lock, runner=counting) + build_once(tree, "/bin/pg_config", "18", lock_path=lock, runner=counting) + expect.num(len(calls), 1, "unchanged source builds once") + + (tree / "src" / "columnar.c").write_text("int a = 2;\n") + build_once(tree, "/bin/pg_config", "18", lock_path=lock, runner=counting) + expect.num(len(calls), 2, "an edited source builds again") + + +def test_the_fingerprint_reads_content_not_mtime(tmp_path, expect): + """A checkout or a branch switch rewrites mtimes without changing what + compiles, and `git stash` does the reverse. Content is the honest input.""" + tree = _tree_with_source(tmp_path, "mtime", "int a = 1;\n") + before = source_fingerprint(tree) + (tree / "src" / "columnar.c").touch() + expect.text(source_fingerprint(tree), before, + "touching a file does not change the fingerprint") + (tree / "src" / "columnar.c").write_text("int a = 2;\n") + expect.at_least(int(source_fingerprint(tree) != before), 1, + "changing its content does") + + +def test_an_unfingerprintable_tree_always_rebuilds(tmp_path, expect): + """No fingerprint means no key, and no key must mean rebuild rather than + skip. `unknown` is never allowed to read as `fresh` anywhere in this + harness.""" + tree = _fake_tree(tmp_path / "nosrc", 0) # no src/, no Makefile + expect.text(repr(source_fingerprint(tree)), "None", + "a tree with nothing to compile has no fingerprint") + lock = str(tmp_path / "lock4") + calls = [] + + def counting(argv): + calls.append(argv) + return _Proc(0) + + build_once(tree, "/bin/pg_config", "18", lock_path=lock, runner=counting) + build_once(tree, "/bin/pg_config", "18", lock_path=lock, runner=counting) + expect.num(len(calls), 2, "both calls build, because neither could be certified") diff --git a/test/pytest/test_connection.py b/test/pytest/test_connection.py index 4551aeef..2d5a3dd5 100644 --- a/test/pytest/test_connection.py +++ b/test/pytest/test_connection.py @@ -6,6 +6,7 @@ """ import decimal +import pathlib def test_cluster_fixture_gives_a_typed_connection(pgc_conn, expect): @@ -18,11 +19,30 @@ def test_cluster_fixture_gives_a_typed_connection(pgc_conn, expect): def test_the_extension_is_installed_and_columnar(pgc_conn, expect): - """The fixture must give a cluster with pgcolumnar loaded and usable.""" + """The loaded extension is the version THIS CHECKOUT declares. + + The expected version is read from `pgcolumnar.control` rather than written + here. A hardcoded "1.0-alpha3" is wrong the moment a release cycle opens, and + on 2026-09-09 it became wrong: #899 moved the tree to 1.0-alpha4 and this arm + would have failed for a reason that has nothing to do with what it tests. + + It is also the wrong ASSERTION. A constant tests that the extension is a + particular version; reading the control file tests that the extension is the + one this source tree describes, which is the property the arm is named after + and the one that catches a foreign install. + """ + from pgc_cluster import control_default_version + + srcdir = pathlib.Path(__file__).resolve().parents[2] + want = control_default_version((srcdir / "pgcolumnar.control").read_text()) + expect.text(bool(want), True, + "PREMISE the checkout declares a version to compare against") + with pgc_conn.cursor() as cur: cur.execute("SELECT extversion FROM pg_extension WHERE extname = 'pgcolumnar'") row = cur.fetchone() - expect.rows([row[0]] if row else [], ["1.0-alpha3"], "the extension version") + expect.rows([row[0]] if row else [], [want], + f"the loaded extension is this tree's {want}") def test_a_columnar_table_round_trips_with_real_types(pgc_conn, expect): @@ -113,18 +133,36 @@ def test_the_worker_owns_its_own_cluster(pgc_cluster, expect): Two workers installing into one pkglibdir race, and a shared cluster lets one test see another's tables. - This asserts the port is the one DERIVED FROM THIS WORKER'S ID, which is what - makes distinctness a property rather than a hope: the mapping from worker id to - port is injective, so if every worker's port matches its own id, no two workers - share a port. Asserting only "the port is an int" would have passed while every - worker sat on 54600. + This used to assert `port == PORT_BASE + slot`, an injective formula, so that + distinctness was a property rather than a hope. The formula is gone: the + harness now WALKS to a free port, because a fixed base is only a claim about + probability and this harness was broken by exactly that -- 54600 sat inside + the kernel's ephemeral range and something else was holding it + (@jdatcmd, #897 review). + + So the assertions moved to the properties that survive a walk, starting with + the one that was actually false. `port < ephemeral floor` is the invariant + the old constant violated, and it is checked against the floor READ from the + kernel rather than a number written here. + + Distinctness is now held by the walk starting each worker at its own offset + plus `is_ours()`, which fails loudly if the server answering is not the one + this worker started. A guard that says NO is what makes the yes mean + something; the arm below drives that guard to False on purpose. """ - from pgc_cluster import PORT_BASE + from pgc_cluster import aux_band, read_ephemeral_floor + floor = read_ephemeral_floor() + lo, hi = aux_band(floor) wid = pgc_cluster.worker_id - slot = 0 if wid in (None, "master") else int(str(wid).lstrip("gw") or 0) - expect.num(pgc_cluster.port, PORT_BASE + slot, - f"worker {wid} owns the port derived from its id") + + expect.at_least(pgc_cluster.port, lo, + f"worker {wid} sits at or above the AUX band floor {lo}") + expect.at_least(hi - pgc_cluster.port, 1, + f"and below the AUX band ceiling {hi}") + expect.at_least(floor - pgc_cluster.port, 1, + f"and below the kernel's ephemeral floor {floor}, which is " + "the invariant the old constant broke") expect.text(pgc_cluster.is_ours(), True, "and the server answering there runs from our datadir") diff --git a/test/pytest/test_guards_pinned.py b/test/pytest/test_guards_pinned.py new file mode 100644 index 00000000..172d34a7 --- /dev/null +++ b/test/pytest/test_guards_pinned.py @@ -0,0 +1,171 @@ +"""Every refusal in the layer, pinned to its own message. + +WHY THIS FILE EXISTS. @jdatcmd neutered each guard in turn and found 11 of 17 +deletable with `test_layer.py` still green. Repeating the census over the whole +corpus after the ordered oracle landed gave 12 of 17 -- the two extra were guards +I had added myself, so this is not a defect of the original layer that later work +avoided. + +TWO CAUSES, and they need the same remedy: + + * Never driven. `test_layer.py` never called `text()`, `at_least()`, + `plan_marker()` or `cannot_run()` at all. + * Driven, but pinned by nothing. Neuter `ordered_rows`'s both-empty guard and + the UNOBSERVABLE guard fires on the same input: the inner run still fails, + so an assertion on outcomes alone still passes. The guard is unreachable by + subsumption rather than untested, and an arm that asserts only "something + failed" cannot tell the difference. + +So every arm here uses `expect.refusal`, which requires the message as well as +the failure. That is the same fix as asserting on a SQLSTATE rather than on prose +elsewhere in this tree: name the contract, not the symptom. +""" + +CONF = "pytest_plugins = ['pgc_vacuity']" + + +def _inner(pytester, body): + pytester.makeconftest(CONF) + pytester.makepyfile(body) + return pytester.runpytest("-p", "pgc_vacuity") + + +def test_num_refuses_a_string_that_looks_like_a_number(pytester, expect): + """The psql-text-parsing defect this harness exists to remove. With the + guard gone, `expect.num("100", "100", ...)` passes silently with count=1.""" + expect.refusal(_inner(pytester, ''' + def test_stringy(expect): + expect.num("100", "100", "a count from text") + '''), "num refuses a numeric-looking string", "needs numbers") + + +def test_num_accepts_real_numbers(pytester, expect): + """Positive control: the guard must not reject the honest form.""" + result = _inner(pytester, ''' + def test_real(expect): + expect.num(100, 100, "a real count") + ''') + expect.outcomes(result, "a genuine numeric comparison still passes", + passed=1, failed=0) + + +def test_text_refuses_an_empty_expectation(pytester, expect): + expect.refusal(_inner(pytester, ''' + def test_empty_text(expect): + expect.text("", "", "two empty strings") + '''), "text refuses an empty expectation", "the expected text is empty") + + +def test_at_least_refuses_a_non_number(pytester, expect): + expect.refusal(_inner(pytester, ''' + def test_bound_text(expect): + expect.at_least("5", 1, "a bound from text") + '''), "at_least refuses a non-number", "needs numbers") + + +def test_at_least_refuses_a_floor_of_zero(pytester, expect): + """`at_least(0, 0, ...)` is satisfied by every possible value.""" + expect.refusal(_inner(pytester, ''' + def test_zero_floor(expect): + expect.at_least(0, 0, "at least nothing") + '''), "at_least refuses a zero floor", "is satisfied by any count") + + +def test_at_least_accepts_a_real_bound(pytester, expect): + result = _inner(pytester, ''' + def test_real_bound(expect): + expect.at_least(7, 3, "seven is at least three") + ''') + expect.outcomes(result, "a real bound passes", passed=1, failed=0) + + +def test_plan_node_refuses_no_criteria(pytester, expect): + expect.refusal(_inner(pytester, ''' + def test_no_criteria(expect): + expect.plan_node({"Plan": {"Node Type": "Seq Scan"}}, name="a plan") + '''), "plan_node refuses no criteria", "needs node_type or provider") + + +def test_outcomes_refuses_no_expectation(pytester, expect): + """This is the one that passed with the guard it is NAMED after deleted: + the old arm asserted only a non-zero exit, and the next arm's refusal + satisfied it.""" + expect.refusal(_inner(pytester, ''' + def test_no_expectation(expect): + class R: + def assert_outcomes(self, **k): + pass + expect.outcomes(R(), "nothing expected") + '''), "outcomes refuses an empty expectation", "asserts nothing") + + +def test_cannot_run_refuses_a_reason_outside_the_closed_list(pytester, expect): + """The escape hatch takes a reason from a closed list precisely so it cannot + become a way to green anything.""" + expect.refusal(_inner(pytester, ''' + def test_bad_reason(expect): + expect.cannot_run("just because", "an unrunnable check") + '''), "cannot_run refuses a reason off the list", "is not one of") + + +def test_hash_refuses_self_comparison(pytester, expect): + expect.refusal(_inner(pytester, ''' + def test_self_hash(expect): + expect.hash("abc", "abc", "a hash against itself") + '''), "hash refuses self-comparison", "compared against itself") + + +def test_hash_refuses_a_LEFT_error_sentinel(pytester, expect): + """The pattern names the SIDE, and that is what pins the guard. + + A census found both sentinel guards unheld while this arm asserted only + "is a failed query". Neuter the left guard and the comparison itself still + fails the inner run -- subsumption by the ordinary assertion, not by another + guard -- so an arm that accepts either message cannot tell the two apart. + With the side named, a left guard that stops working can no longer be + covered by the right one or by the comparison. + """ + expect.refusal(_inner(pytester, ''' + def test_error_left(expect): + expect.hash("QUERY_ERROR.1", "abc", "a failed query on the left") + '''), "hash refuses a left error sentinel", + "the left side is a failed query") + + +def test_hash_refuses_a_RIGHT_error_sentinel(pytester, expect): + """The mirror, which the single arm above never covered at all.""" + expect.refusal(_inner(pytester, ''' + def test_error_right(expect): + expect.hash("abc", "QUERY_ERROR.1", "a failed query on the right") + '''), "hash refuses a right error sentinel", + "the right side is a failed query") + + +def test_hash_refuses_two_empties(pytester, expect): + """Reachable only with two DISTINCT empty values. + + The self-comparison guard above it is `got is want`, an identity test, and + CPython interns `""` -- so `expect.hash("", "", ...)` trips that guard + instead and never reaches this one. Written the obvious way, this arm would + have passed while asserting nothing about the guard it names. Prove an input + can reach a guard before asserting the guard fires. + """ + expect.refusal(_inner(pytester, ''' + def test_empty_hashes(expect): + expect.hash("", None, "an empty hash against a missing one") + '''), "hash refuses two empties", "both hashes are empty") + + +def test_refusal_itself_refuses_an_empty_pattern_list(pytester, expect): + """The new helper must not become the defect it removes: `refusal(result, + name)` with no pattern is an outcome-only assertion wearing a better name.""" + import pytest as _pytest + from pgc_vacuity import VacuityError + + class _R: + def assert_outcomes(self, **k): + pass + + with _pytest.raises(VacuityError): + expect.refusal(_R(), "no patterns given") + expect.num(1, 1, "refusal() with no pattern is itself refused") From a6820b8443ffe11a644bcaf8f7c45f30e0efb06a Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Wed, 9 Sep 2026 15:15:57 +0000 Subject: [PATCH 07/11] test/pytest: build before the server starts, or the guard is defeated by order The build guard ran AFTER make_cluster, which does initdb and start. That ordering is not a detail. shared_preload_libraries maps the library at postmaster start, so a cluster started before the install keeps the OLD .so mapped for its whole life: the build reports success, and every test still measures the previous branch's code. The guard added one commit ago was defeated by the order of two lines. It surfaced as a flake rather than as a failure, which is why it is worth recording how. Rebasing this branch onto 1.0-alpha4 changed the sources, so the first run had to rebuild; that run reported 15 cluster-start errors, and the next run passed because the install had already landed. A flake that clears on a second run is exactly what a stale-binary defect looks like from the outside. @jdatcmd's footnote was that a `.so` mtime versus pg_postmaster_start_time() check would be near-vacuous, because the cluster is initdb'd fresh each session so the postmaster always starts after the library. That is true ONCE THE ORDER IS RIGHT, and it is what makes the check the right guard for the order: with the build after the start, the .so is newer than the postmaster and it refuses. The check is now in the fixture, and the verdict is a pure function of two epochs with arms for predates, fresh, the equal-timestamp boundary, and three unreadable-side cases that must read `unknown` rather than `fresh`. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- test/pytest/conftest.py | 41 ++++++++++++++++------------ test/pytest/pgc_cluster.py | 40 ++++++++++++++++++++++++++++ test/pytest/test_build_refusal.py | 44 +++++++++++++++++++++++++++++++ 3 files changed, 108 insertions(+), 17 deletions(-) diff --git a/test/pytest/conftest.py b/test/pytest/conftest.py index ca382576..987fafda 100644 --- a/test/pytest/conftest.py +++ b/test/pytest/conftest.py @@ -15,7 +15,7 @@ import psycopg import pytest -from pgc_cluster import build_once, make_cluster +from pgc_cluster import _pg_config, build_once, make_cluster pytest_plugins = ["pytester"] @@ -45,26 +45,33 @@ def major_of(version_text): def pgc_cluster(request, worker_id): """One cluster per xdist worker, built once and torn down at session end.""" pg_config = request.config.getoption("--pg-config") + + # BUILD BEFORE THE SERVER STARTS. This ran the other way round first, and the + # ordering was not a detail: shared_preload_libraries maps the library at + # postmaster start, so a cluster started before the install keeps the OLD + # .so mapped for its whole life. The build would report success and every + # test would still measure the previous branch's code -- the same defect the + # build guard exists to close, reintroduced by the order of two lines. + # + # It showed up as a flake: the first run after a source change failed to + # start a cluster, and the next run passed because the install had already + # landed. + major = major_of(_pg_config(pg_config, "--version")) + verdict = build_once(str(SRCDIR), pg_config, major) + cluster, root = make_cluster(pg_config, worker_id) + print(f"\n-- build: {verdict} from {SRCDIR}") # Printed for the same reason lib.sh prints it: so a reader can tell which # binary produced the results below. - print(f"\n-- cluster: worker={worker_id} port={cluster.port} " + print(f"-- cluster: worker={worker_id} port={cluster.port} " f"{cluster.version} .so={cluster.so_md5()}") - # AND THE BINARY IS BUILT FROM THIS TREE, which the fingerprint above never - # established. Printing a fingerprint tells a reader which binary ran; it - # does not stop the run when that binary came from somewhere else. - # - # Comparing the INSTALLED artifacts against the source was my first fix and - # it was not enough: appending `#error` to a .c file leaves the .control and - # the .sql byte-identical, so the corpus would still have reported 25 passed - # on source that cannot compile (@jdatcmd, #897 review). The only honest - # check is the one the bash harness has made since #536 -- build, install, - # and refuse to report if either fails. - # - # The install itself is the reason this harness skipped it: the workers - # share one pkglibdir. That is a reason to serialise it, not to skip it. - verdict = build_once(str(SRCDIR), pg_config, major_of(cluster.version)) - print(f"-- build: {verdict} from {SRCDIR}") + # And the running server is the one that loaded THAT library. @jdatcmd noted + # a start-time check would be near-vacuous because the cluster is initdb'd + # fresh each session, so the postmaster always starts after the .so. That is + # true once the order above is right, and it is exactly what pins the order: + # with the build after the start, the .so is NEWER than the postmaster and + # this refuses. + cluster.require_server_loaded_this_binary() try: with psycopg.connect(cluster.dsn(), autocommit=True) as conn: conn.execute("CREATE EXTENSION IF NOT EXISTS pgcolumnar") diff --git a/test/pytest/pgc_cluster.py b/test/pytest/pgc_cluster.py index 79564903..604bc725 100644 --- a/test/pytest/pgc_cluster.py +++ b/test/pytest/pgc_cluster.py @@ -121,6 +121,46 @@ def __init__(self, pg_config, worker_id, datadir, port): self.sharedir = _pg_config(pg_config, "--sharedir") self._started = False + def server_binary_verdict(self, so_mtime, postmaster_epoch): + """fresh | predates | unknown, from two epochs. + + Pure, so it is driven without a server. `predates` means the postmaster + started BEFORE the library on disk was written, so the backends are + executing older code than the file -- shared_preload_libraries maps the + library at start and a reinstall does not reload it. + """ + try: + so, pm = float(so_mtime), float(postmaster_epoch) + except (TypeError, ValueError): + return "unknown" + return "predates" if so > pm else "fresh" + + def require_server_loaded_this_binary(self): + """Refuse if the running server predates the installed library.""" + try: + so_mtime = os.stat(self.so_path).st_mtime + except OSError: + so_mtime = None + pm = None + try: + import psycopg + with psycopg.connect(self.dsn(), autocommit=True) as conn: + row = conn.execute( + "SELECT extract(epoch from pg_postmaster_start_time())" + ).fetchone() + pm = row[0] if row else None + except Exception: + pm = None + verdict = self.server_binary_verdict(so_mtime, pm) + if verdict == "predates": + raise RuntimeError( + f"this server started before {self.so_path} was installed, so its " + f"backends are running older code than the file on disk. " + f"shared_preload_libraries maps the library at start; a reinstall " + f"does not reload it." + ) + return verdict + # -- is the installed extension the one this checkout describes? ------- @property def extension_dir(self): diff --git a/test/pytest/test_build_refusal.py b/test/pytest/test_build_refusal.py index 2f57a244..7f1df25f 100644 --- a/test/pytest/test_build_refusal.py +++ b/test/pytest/test_build_refusal.py @@ -197,3 +197,47 @@ def counting(argv): build_once(tree, "/bin/pg_config", "18", lock_path=lock, runner=counting) build_once(tree, "/bin/pg_config", "18", lock_path=lock, runner=counting) expect.num(len(calls), 2, "both calls build, because neither could be certified") + + +# --------------------------------------------------------------------------- +# The ORDER of build and server start, which is not a detail. +# +# shared_preload_libraries maps the library at postmaster start, so a cluster +# started before the install keeps the OLD .so mapped for its whole life. The +# build reports success and every test still measures the previous branch's +# code -- the guard defeated by the order of two lines. This is how it showed +# up: the first run after a source change failed to start a cluster, and the +# next run passed because the install had already landed. + + +class _C: + """Just enough Cluster to drive the verdict.""" + from pgc_cluster import Cluster + server_binary_verdict = Cluster.server_binary_verdict + + +def test_a_server_older_than_the_library_is_refused(expect): + expect.text(_C().server_binary_verdict(so_mtime=200, postmaster_epoch=100), + "predates", "the .so was written after the server started") + + +def test_a_server_started_after_the_library_is_fresh(expect): + expect.text(_C().server_binary_verdict(so_mtime=100, postmaster_epoch=200), + "fresh", "the server started after the .so was installed") + + +def test_equal_timestamps_are_fresh_not_predates(expect): + """The exact boundary. A one-second-resolution mtime and a start time in the + same second must not read as stale, or every fast run refuses itself.""" + expect.text(_C().server_binary_verdict(so_mtime=100, postmaster_epoch=100), + "fresh", "same second is not stale") + + +def test_an_unreadable_side_is_unknown_not_fresh(expect): + """`unknown` never reads as `fresh` anywhere in this harness.""" + expect.text(_C().server_binary_verdict(so_mtime=None, postmaster_epoch=100), + "unknown", "no .so mtime") + expect.text(_C().server_binary_verdict(so_mtime=100, postmaster_epoch=None), + "unknown", "no postmaster start time") + expect.text(_C().server_binary_verdict(so_mtime="x", postmaster_epoch="y"), + "unknown", "non-numeric epochs") From e175cd40d71e97ce2cedeb99e4481a5aad601ab6 Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Wed, 9 Sep 2026 16:57:26 +0000 Subject: [PATCH 08/11] test: check TESTS.md against the corpus, in both harnesses (#432) TESTS.md says its job is "what each test asserts, and why it exists". It went stale inside a single rework: the corpus grew from 25 tests in three files to 54 in five, and the two files the rework added -- 29 tests, every one written to answer a review -- were named nowhere in it. The header still read "Twenty-five tests in three files". A partial index of something claiming completeness reads as a total one. A reader who opens a file whose stated purpose is completeness does not then go and count the tests. That is the defect the vacuity layer refuses one level down: a report that looks like coverage and is not. Three properties, each mechanical: every test_*.py is named in TESTS.md, every `def test_` is named, and the totals TESTS.md states are the totals on disk. The third is what the stale header got wrong and neither of the others would have caught -- a document can name every test and still miscount them -- so the totals are now written in a fixed parseable form. Prose that says "twenty-five" cannot be compared with anything, which is how the wrong header survived being read many times. WRITTEN TWICE, per jd's rule of 2026-09-09: every test ships as a .sh suite and a pytest test in the same change. test/selftest/350-the-pytest-corpus-must-be.sh the half with teeth test/pytest/test_docs_cover_the_corpus.py the twin, 8 tests They are not interchangeable and both headers say so. harness_selftest is registered in SUITES, so the .sh half runs in the matrix and in CI; NOTHING runs pytest -- not run_all_versions.sh, not any workflow under .github/ -- so a guard written only in the corpus would never fire in the gate. A guard that does not run is a comment. The pytest half is what a person running the corpus by hand gets, with the offenders arriving as a Python list rather than as a string assembled by shell. PROVED BY REMOVAL, twice, rather than by running green: * against the real gap, before the fix: FAIL every test file and every test in the corpus is named in TESTS.md: got [[31: test_build_refusal.py test_a_failed_build_raises_... ]] FAIL TESTS.md states its totals in a form that can be read back: got [no] FAIL and the totals it states are the totals on disk: got [] want [54 5] 31 = 29 undocumented tests + 2 undocumented files, which agrees with an independent count made in Python before the guard existed. * against its own twin's arrival: adding test_docs_cover_the_corpus.py moved the corpus from (54, 5) to (62, 6) and the totals arm failed with `got '(54, 5)' want '(62, 6)'` until this commit documented it. Both halves also carry fixture arms -- a control that a complete document reports nothing missing, and reds for an undocumented test, an undocumented file with the tests inside it, and a stated total that disagrees with disk. Everything that only passes on a healthy tree is indistinguishable from a guard that does nothing. The three premise arms are there for the same reason: a sweep that found no files reports "nothing missing" and looks exactly like a sweep that works. Also documents the two files the rework added, which is the gap itself: test_guards_pinned.py (every refusal pinned to its own message, and why an outcome-only assertion cannot tell a subsumed guard from a working one) and test_build_refusal.py (the corpus must not report on source it never built, and the build/start ordering that defeated the first fix). Adds `refusal` to the assertion vocabulary table, where it was missing. Records the twin rule as step 0 of "Adding a test", so it reaches whoever writes the next test. The CHANGELOG entry had gone stale in the same way and by the same cause -- it described test_layer.py and "four of the ten" as though the rework had not happened -- so it is corrected here rather than left to be found next. Verified: harness_selftest 272 passed + 0 failed + 0 unrunnable PASSED (261 before) docs_style 9 checks PASSED pytest 62 passed serial, 62 passed -n 4, marker cleared for each shellcheck -S error -s bash test/*.sh test/selftest/*.sh exit 0 The cold runs were checked rather than trusted: the installed .so's mtime moves across a run (1788972809 -> 1788972852, md5 unchanged, which is the right answer for unchanged source) and a datadir appears mid-run and is gone after, so the run really does build, install and stand up a cluster in the time it reports. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- CHANGELOG.md | 40 ++- test/pytest/TESTS.md | 242 +++++++++++++++++- test/pytest/test_docs_cover_the_corpus.py | 153 +++++++++++ .../selftest/350-the-pytest-corpus-must-be.sh | 141 ++++++++++ 4 files changed, 561 insertions(+), 15 deletions(-) create mode 100644 test/pytest/test_docs_cover_the_corpus.py create mode 100644 test/selftest/350-the-pytest-corpus-must-be.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index 3cf0cd00..725db346 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -202,7 +202,8 @@ true until the next version shipped. `int4` `1` and a `text` `'1'` are the same string to a bash oracle. This ports ONE bash suite. `test/` carries 4,429 anchored assertions across 256 - suites, so this is 0.18% of them and is not coverage. The layer is the point. + suites, so this is 0.18% of them and is not coverage. The layer is the point: + 62 tests in 6 files, of which 47 test the harness rather than the product. A pytest run fails open in several ways this project has already been bitten by: a test that asserts nothing passes, a filter that selects nothing exits 0, and a @@ -214,11 +215,38 @@ true until the next version shipped. `xfail_strict` is on, and `--pgc-expect-tests N` asserts the run's own shape and refuses `N = 0`. - Every refusal has a red test in `test_layer.py` that runs pytest inside pytest - and asserts on the INNER run's outcome, which is what proves a guard refuses - rather than assuming it. Each records the bare-pytest behaviour it exists to - stop; every one of those measurements exited 0. Four of the ten are positive - controls, so a guard that starts rejecting good tests reddens there first. + Every refusal has a red test that runs pytest inside pytest and asserts on the + INNER run's outcome, which is what proves a guard refuses rather than assuming + it. Each records the bare-pytest behaviour it exists to stop; every one of those + measurements exited 0. Positive controls sit beside the guards, so a guard that + starts rejecting good tests reddens there first. + + **Asserting that an inner run failed is not asserting that a named guard fired.** + A census that neutered each guard alone found most of them deletable with + `test_layer.py` still green, from two causes: several were never driven at all, + and several are subsumed by a neighbouring guard, so the inner run fails either + way and an outcome-only assertion cannot tell which fired. + `test_guards_pinned.py` pins each refusal to its own MESSAGE through + `expect.refusal`, which refuses to be called with no pattern -- the same move as + asserting on a SQLSTATE rather than on prose. + + **The corpus builds and installs before it measures anything.** It did not at + first: with `#error` appended to a source file and nothing rebuilt, the run + reported 25 passed and exit 0 while the bash suite reported `FATAL: the build + failed` and exit 1. The refusal now comes from `pgc_build_and_install` in + `test/lib.sh`, driven from Python so there is one implementation rather than + two that can drift, and it runs BEFORE the cluster starts -- + `shared_preload_libraries` maps the library at postmaster start, so a cluster + started before the install keeps the old one mapped for its whole life. + + **`test/pytest/TESTS.md` is checked rather than trusted.** It documents every + test in the corpus, and it went stale inside a single rework: 29 of 54 tests + were named nowhere in it while its header still claimed 25. A guard now requires + every file and every test to be named there, and requires the totals it states + to be the totals on disk -- a document can name every test and still miscount + them. It is written twice, as `test/selftest/350-the-pytest-corpus-must-be.sh` + and `test/pytest/test_docs_cover_the_corpus.py`; only the first runs in the + gate, and it reddened on its twin's arrival before this entry was written. Not registered in `test/run_all_versions.sh`. That would add a `psycopg` build dependency to every CI leg for 0.18% of the assertions; `test/pytest/README.md` diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index d0ce5563..5069d0c4 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -4,10 +4,21 @@ Reference for anyone reading, running, or adding to `test/pytest/`. The design a the decisions behind the harness are in `design/ISSUE_432_PYTEST_HARNESS.md`. This file covers the tests themselves. -Twenty-five tests in three files. Ten of them test the harness rather than the +**62 tests in 6 files.** Forty-seven of them test the harness rather than the product, and they come first, because a harness that can report a false green makes every other result in this directory worthless. +That ratio is not an accident of taste. Two of those files exist because a reviewer +neutered the guards one at a time and found most of them deletable with the suite +still green, and because the corpus once reported 25 passed against source carrying +`#error`. Both are recorded below in the sections for the files that close them. + +The totals in bold above are checked. `test/selftest/350-the-pytest-corpus-must-be.sh` +reads them back and compares them with the corpus on disk, and also requires every +file and every test here to be named in this document -- because this file went stale +inside a single rework, and a partial index of something claiming completeness reads +as a total one. + Every measured fact quoted below was run. Where a test encodes a number or a behaviour, the source of that number is named. @@ -16,10 +27,13 @@ behaviour, the source of that number is named. - [1. How to read a test in here](#1-how-to-read-a-test-in-here) - [2. The assertion vocabulary](#2-the-assertion-vocabulary) - [3. test_layer.py: the guards, testing themselves](#3-test_layerpy-the-guards-testing-themselves) -- [4. test_connection.py: the cluster and the direct connection](#4-test_connectionpy-the-cluster-and-the-direct-connection) -- [5. test_native_projection.py: the ported suite](#5-test_native_projectionpy-the-ported-suite) -- [6. Adding a test](#6-adding-a-test) -- [7. Traps this corpus records](#7-traps-this-corpus-records) +- [4. test_guards_pinned.py: every refusal, pinned to its own message](#4-test_guards_pinnedpy-every-refusal-pinned-to-its-own-message) +- [5. test_build_refusal.py: never report on source you did not build](#5-test_build_refusalpy-never-report-on-source-you-did-not-build) +- [6. test_docs_cover_the_corpus.py: this document, checked](#6-test_docs_cover_the_corpuspy-this-document-checked) +- [7. test_connection.py: the cluster and the direct connection](#7-test_connectionpy-the-cluster-and-the-direct-connection) +- [8. test_native_projection.py: the ported suite](#8-test_native_projectionpy-the-ported-suite) +- [9. Adding a test](#9-adding-a-test) +- [10. Traps this corpus records](#10-traps-this-corpus-records) ## 1. How to read a test in here @@ -56,8 +70,15 @@ assertion, so the two read differently in output. | `plan_node(plan, node_type=, provider=)` | some node matches those fields exactly | being called with neither field, which would assert nothing | | `outcomes(result, name, **want)` | an inner pytest run's outcomes | being called with no expectation | | `run_failed(result, name)` | an inner run exited non-zero | nothing | +| `refusal(result, name, *patterns)` | an inner run failed **and** its output carries each pattern | being called with no pattern, which is an outcome-only assertion wearing a better name | | `cannot_run(reason, detail)` | declares the test unrunnable | a reason outside the closed list | +`refusal` is the helper the whole of section 4 turns on. Asserting that an inner run +failed is not the same as asserting that a named guard fired: several guards are +subsumed by a neighbouring one, so the inner run fails either way and an +outcome-only assertion cannot tell which. Requiring the message is the same move as +asserting on a SQLSTATE rather than on prose -- name the contract, not the symptom. + `rows` compares row sets rather than `md5(string_agg(...))`. That asserts the same property as the bash oracle by a stronger means: a hash mismatch says two hashes differ, a row-set mismatch says which row. It also avoids recomputing the hash in @@ -100,7 +121,205 @@ The escape hatches are deliberately more expensive to type than the honest form. number. `cannot_run` takes a reason from a closed list. None of them can become the default by being shorter. -## 4. test_connection.py: the cluster and the direct connection +## 4. test_guards_pinned.py: every refusal, pinned to its own message + +**Why this file exists.** @jdatcmd neutered each guard in the layer in turn and +found **11 of 17 deletable with `test_layer.py` still green**. Repeating the census +over the whole corpus after the ordered oracle landed gave 12 of 17; the two extra +were guards added later, so this is not a defect of the original layer that +subsequent work happened to avoid. It is the shape the layer was in. + +Two causes, and they need the same remedy. + +**Never driven.** `test_layer.py` never called `text()`, `at_least()`, +`plan_marker()` or `cannot_run()` at all. A guard nothing calls cannot be observed +to work. + +**Driven, but pinned by nothing.** This is the interesting half. Neuter the +both-empty guard in `ordered_rows` and the UNOBSERVABLE guard fires on the same +input. The inner run still fails, so an assertion on outcomes alone still passes. +The guard is unreachable **by subsumption** rather than untested, and an arm that +asserts only "something failed" cannot tell the two apart. + +So every arm here goes through `expect.refusal`, which requires the message as well +as the failure. + +| test | the refusal it pins | +| --- | --- | +| `test_num_refuses_a_string_that_looks_like_a_number` | `num("100", "100", …)` — the psql-text defect this harness exists to remove | +| `test_num_accepts_real_numbers` | **control**: a genuine numeric comparison still passes | +| `test_text_refuses_an_empty_expectation` | an empty expected string, which anything empty satisfies | +| `test_at_least_refuses_a_non_number` | a bound taken from text | +| `test_at_least_refuses_a_floor_of_zero` | a floor every possible value clears | +| `test_at_least_accepts_a_real_bound` | **control**: `at_least(7, 3, …)` passes | +| `test_plan_node_refuses_no_criteria` | called with neither `node_type` nor `provider` | +| `test_outcomes_refuses_no_expectation` | called with no expectation at all | +| `test_cannot_run_refuses_a_reason_outside_the_closed_list` | the escape hatch cannot be widened by inventing a reason | +| `test_hash_refuses_self_comparison` | a value compared against itself | +| `test_hash_refuses_a_LEFT_error_sentinel` | a `QUERY_ERROR` on the left | +| `test_hash_refuses_a_RIGHT_error_sentinel` | the mirror, which one arm never covered | +| `test_hash_refuses_two_empties` | two distinct empty values | +| `test_refusal_itself_refuses_an_empty_pattern_list` | the new helper must not become the defect it removes | + +Three of these carry reasoning that is easy to lose. + +**The sentinel arms name the SIDE.** A single arm asserting "is a failed query" +left both sentinel guards unheld. Neuter the left guard and the comparison itself +still fails the inner run — subsumption by the ordinary assertion, not by another +guard. With the side named, a left guard that stops working can no longer be +covered by the right one or by the comparison. + +**`test_hash_refuses_two_empties` is reachable only with two DISTINCT empties.** +The self-comparison guard above it is `got is want`, an identity test, and CPython +interns `""` — so `expect.hash("", "", …)` trips *that* guard and never reaches +this one. Written the obvious way, the arm would have passed while asserting +nothing about the guard it names. Prove an input can reach a guard before asserting +the guard fires. + +**`test_refusal_itself_refuses_an_empty_pattern_list` closes the loop.** +`refusal(result, name)` with no pattern is exactly the outcome-only assertion that +caused most of the unheld guards. The helper introduced to fix the problem refuses +to be used that way. + +### What the census says now + +Run the way the reviewer ran it — each guard neutered alone, the mutation asserted +to have applied, the file restored and compared byte-for-byte afterwards, and +`inputs == sum(buckets)` asserted: + +``` +base branch before 13 guards 3 HELD 10 UNHELD +base branch after 13 guards 13 HELD 0 UNHELD +full stack after 18 guards 18 HELD 0 UNHELD +``` + +## 5. test_build_refusal.py: never report on source you did not build + +**Why this file exists.** @jdatcmd appended +`#error THIS SOURCE IS BROKEN AND CANNOT BUILD` to `src/columnar_projection.c`, +rebuilt nothing, and ran both harnesses: + +``` +pytest -> 25 passed, exit 0 +bash test/native_projection.sh -> FATAL: the build failed …, exit 1 +``` + +The bash harness has refused that since #536. This corpus did not, because it never +built, never installed and never compared anything. `Cluster.so_md5` printed a +fingerprint that nothing read — a number on the screen is not a guard. + +**The first fix was insufficient and was deleted rather than kept.** Comparing the +installed `.control` and `.sql` against source cannot catch `#error` in a `.c` file: +both artifacts stay byte-identical. The refusal now comes from +`pgc_build_and_install` in `test/lib.sh`, driven from Python, so there is one +implementation rather than two that can drift. + +**Two levels of arm, deliberately.** The injected-runner arms pin what the Python +side does with a verdict. The `bash` arms pin the shell plumbing — the sourcing, the +quoting and the exit-status path — which an injected runner cannot reach and which +is where a wrong quote would hide. + +| test | asserts | +| --- | --- | +| `test_a_failed_build_raises_rather_than_returning` | the refusal raises, says it is refusing, and carries the build's own output rather than a summary | +| `test_the_refusal_names_the_tree_it_refused` | the message names the source directory; a reader with several worktrees needs to know which | +| `test_a_successful_build_is_silent` | **control**: the guard does not fire on a build that worked | +| `test_the_shell_path_really_refuses` | the shell's own `FATAL` reaches the Python caller, through real bash | +| `test_the_shell_path_accepts_a_good_build` | **control** for the arm above, through the same plumbing | +| `test_a_missing_lib_sh_is_a_refusal_not_a_pass` | an unsourceable `lib.sh` means no guard at all, so it must refuse rather than proceed ungated | +| `test_build_once_builds_once_and_then_skips` | the workers share one prefix, so the install is serialised rather than skipped | +| `test_build_once_rebuilds_for_a_different_prefix` | running against two majors in turn rebuilds for each | +| `test_editing_the_source_rebuilds` | the marker is keyed on the source fingerprint, not just the prefix | +| `test_the_fingerprint_reads_content_not_mtime` | `touch` does not move the fingerprint; an edit does | +| `test_an_unfingerprintable_tree_always_rebuilds` | no fingerprint means no key, and no key must mean rebuild | +| `test_a_server_older_than_the_library_is_refused` | `predates` | +| `test_a_server_started_after_the_library_is_fresh` | `fresh` | +| `test_equal_timestamps_are_fresh_not_predates` | the exact boundary: the same second is not stale | +| `test_an_unreadable_side_is_unknown_not_fresh` | three unreadable shapes all give `unknown` | + +### The build/start ORDER, which is not a detail + +`shared_preload_libraries` maps the library at postmaster start, so **a cluster +started before the install keeps the OLD `.so` mapped for its whole life.** The +build reports success and every test still measures the previous branch's code — +the guard defeated by the order of two lines. + +The first fix here had exactly that defect: the build ran *after* `make_cluster`, +which does `initdb` and starts the server. It surfaced as a flake — the first run +after the alpha4 rebase gave 15 cluster-start errors and the second run passed. +**A flake that clears on a second run is what a stale-binary defect looks like from +outside.** + +### `unknown` never reads as `fresh` + +Three of the verdict arms exist to keep that true. `server_binary_verdict` returns +one of `fresh`, `predates` or `unknown`, and `unknown` is what an unreadable mtime, +an unreadable postmaster start time, or a non-numeric epoch all produce. The +boundary arm is separate on purpose: mtime resolution is one second, so a run fast +enough to install and start within the same second must not refuse itself. + +## 6. test_docs_cover_the_corpus.py: this document, checked + +The file you are reading is checked mechanically, because it went stale inside a +single rework and nothing noticed. The corpus grew from 25 tests in three files to +54 in five; the two new files, 29 tests, were named nowhere here, and the header +still said "Twenty-five tests in three files". + +**A partial index of something claiming completeness reads as a total one.** A +reader who opens a file whose stated purpose is completeness does not then go and +count the tests. That is the same defect the vacuity layer refuses one level down: +a report that looks like coverage and is not. + +Three properties, each mechanical: + +- every `test_*.py` file in this directory is named in TESTS.md +- every `def test_` in those files is named in TESTS.md +- the totals TESTS.md states are the totals on disk + +The third is what the stale header got wrong, and neither of the first two would +have caught it: a document can name every test and still miscount them. That is why +the totals are written in a fixed, parseable form -- prose that says "twenty-five" +cannot be compared with anything, which is how the wrong header survived being read +many times. + +| test | asserts | +| --- | --- | +| `test_the_sweep_finds_the_corpus_rather_than_an_empty_glob` | **premise**: the sweep saw files and tests, so "nothing missing" means something | +| `test_every_file_and_test_is_named_in_the_document` | every file and test is named here, and a failure says WHICH | +| `test_the_stated_totals_are_the_totals_on_disk` | the bold totals line matches the corpus | +| `test_a_fully_documented_corpus_reports_nothing_missing` | **control**: no false positive on a complete document | +| `test_an_undocumented_test_is_named_rather_than_passed_over` | the exact shape that shipped: file named, one test inside it not | +| `test_an_undocumented_file_is_caught_with_the_tests_inside_it` | how 29 tests went missing at once | +| `test_a_document_with_no_totals_line_states_none` | absent totals report `None`, which must not read as "they match" | +| `test_a_stated_total_that_disagrees_with_disk_is_visible` | the count arm's own red | + +The five fixture arms exist because everything above them passes on a healthy tree, +which is exactly what a guard that does nothing also does. They run the identical +functions over a corpus built to be wrong. + +### The twin, and which half has teeth + +This is the pytest half. The other half is +`test/selftest/350-the-pytest-corpus-must-be.sh`, and the two are **not** +interchangeable: + +- **The `.sh` half is the one that gates.** `harness_selftest` is registered in + `SUITES`, so it runs in the matrix and in CI. +- **Nothing runs pytest.** Not `run_all_versions.sh`, not any workflow under + `.github/`. A guard written only here would never fire in the gate, and a guard + that does not run is a comment. + +So the `.sh` copy is the enforcement and this one is what a person running the +corpus by hand gets, with the offenders arriving as a Python list rather than as a +string assembled by shell. Both are written in the same change, per the rule in +section 9. + +This guard reddened on its own arrival, which is the only reason it is known to +work here: adding this file moved the corpus from `(54, 5)` to `(62, 6)` and the +totals arm failed with `got '(54, 5)' want '(62, 6)'` until this section was +written. + +## 7. test_connection.py: the cluster and the direct connection | test | asserts | | --- | --- | @@ -155,7 +374,7 @@ This test asserts all three facts, so reverting to the provider predicate redden here rather than passing quietly. The first version of this harness used the provider predicate and was wrong in exactly this way. -## 5. test_native_projection.py: the ported suite +## 8. test_native_projection.py: the ported suite A complete port of `test/native_projection.sh`, chosen because it is 55 lines, has 8 assertions, does no process work, and depends on nothing timing-related. @@ -198,8 +417,13 @@ The mutation makes `PgColumnarProjectionFanoutRow` return without writing. Each builds and installs once, and both harnesses print the `.so` md5 they measured, so an arm where the two differ is void rather than reported. -## 6. Adding a test +## 9. 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 + other. Only the `.sh` half runs in the gate today, and only the pytest half gets + typed results and a real connection, so a test that exists in one harness is not + finished. Where the two differ in force, say which is which in both headers. 1. Write the failing test first and run it. Confirm it fails for the reason you intend, not because a helper or module is missing. A red on `ImportError` proves only that a file is absent. @@ -220,7 +444,7 @@ an arm where the two differ is void rather than reported. 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. -## 7. Traps this corpus records +## 10. Traps this corpus records Recorded because each one produced a confident wrong result before it was caught, and all are the same family as the defect the layer exists to prevent. diff --git a/test/pytest/test_docs_cover_the_corpus.py b/test/pytest/test_docs_cover_the_corpus.py new file mode 100644 index 00000000..4a765e71 --- /dev/null +++ b/test/pytest/test_docs_cover_the_corpus.py @@ -0,0 +1,153 @@ +"""TESTS.md must document every test in this directory. + +THE TWIN RULE. Every test in this tree is written twice, once as a `.sh` suite and +once here, in the same change. This file is the pytest half of +`test/selftest/350-the-pytest-corpus-must-be.sh`, and the two are not +interchangeable: + + * The `.sh` half is the one with TEETH. `harness_selftest` is registered in + `SUITES`, so it runs in the matrix and in CI. Nothing runs pytest -- not + `run_all_versions.sh`, not any workflow under `.github/` -- so a guard written + only here would never fire in the gate. + * This half is the one a person running the corpus by hand gets, and it is + where a failure arrives with the offenders as a Python list rather than as a + string assembled by shell. + +WHY THE GUARD EXISTS AT ALL. TESTS.md says its job is "what each test asserts, and +why it exists". It went stale inside a single rework: the corpus grew from 25 tests +in three files to 54 in five, and the two new files -- 29 tests, every one added by +the rework that answered a review -- were named nowhere in it, while the header +still read "Twenty-five tests in three files". + +A partial index of something claiming completeness reads as a total one. A reader +who opens a file whose stated purpose is completeness does not then go and count +the tests. That is the same defect class the vacuity layer refuses one level down: +a report that looks like coverage and is not. +""" + +import pathlib +import re + +HERE = pathlib.Path(__file__).resolve().parent +DOC = HERE / "TESTS.md" + +# The bold, fixed-form totals line. Written in a form that can be read back +# precisely so it can be checked: prose that says "twenty-five" cannot be compared +# with anything, which is how the stale header survived being read many times. +TOTALS = re.compile(r"^\*\*(\d+) tests in (\d+) files\.\*\*", re.M) + + +def corpus_tests(directory): + """-> {filename: [test name, ...]} for every test_*.py in `directory`.""" + found = {} + for f in sorted(pathlib.Path(directory).glob("test_*.py")): + found[f.name] = re.findall(r"^def (test_\w+)", f.read_text(), re.M) + return found + + +def undocumented(directory, doc_path): + """-> sorted list of file and test names the document does not name.""" + text = pathlib.Path(doc_path).read_text() + missing = [] + for name, tests in corpus_tests(directory).items(): + if name not in text: + missing.append(name) + missing.extend(t for t in tests if t not in text) + return sorted(missing) + + +def stated_totals(doc_path): + """-> (tests, files) the document claims, or None if it states none.""" + m = TOTALS.search(pathlib.Path(doc_path).read_text()) + return (int(m.group(1)), int(m.group(2))) if m else None + + +def _fixture(tmp_path, doc_body): + """A two-test corpus and a document, so the arms can drive a KNOWN answer.""" + d = tmp_path / "corpus" + d.mkdir(exist_ok=True) + (d / "test_one.py").write_text( + "def test_alpha(expect):\n pass\ndef test_beta(expect):\n pass\n") + doc = d / "DOC.md" + doc.write_text(doc_body) + return d, doc + + +# --------------------------------------------------------------------------- +# The real corpus. Three properties, each mechanical. +# --------------------------------------------------------------------------- + +def test_the_sweep_finds_the_corpus_rather_than_an_empty_glob(expect): + """A sweep that found nothing reports "nothing missing" and is + indistinguishable from a sweep that works. This is the premise the other two + arms rest on, and it is asserted rather than assumed.""" + found = corpus_tests(HERE) + expect.at_least(len(found), 3, "premise: the sweep found the corpus files") + expect.at_least(sum(len(v) for v in found.values()), 20, + "premise: and the tests inside them") + + +def test_every_file_and_test_is_named_in_the_document(expect): + """The property that went wrong. Named rather than counted, so a failure says + WHICH test is undocumented instead of only how many.""" + missing = undocumented(HERE, DOC) + expect.text(", ".join(missing) or "none", "none", + "every test file and every test in the corpus is named in TESTS.md") + + +def test_the_stated_totals_are_the_totals_on_disk(expect): + """Neither arm above would catch a wrong count: a document can name every test + and still miscount them, which is exactly what the stale header did.""" + stated = stated_totals(DOC) + expect.text(repr(stated is not None), "True", + "TESTS.md states its totals in a form that can be read back") + found = corpus_tests(HERE) + expect.text(repr(stated), repr((sum(len(v) for v in found.values()), len(found))), + "and the totals it states are the totals on disk") + + +# --------------------------------------------------------------------------- +# And the guard must be able to FAIL. Everything above passes on a healthy tree, +# which is exactly what a guard that does nothing also does. +# --------------------------------------------------------------------------- + +def test_a_fully_documented_corpus_reports_nothing_missing(tmp_path, expect): + """Control. A guard with a bad false-positive rate gets switched off, and then + the guard it replaced is gone too.""" + d, doc = _fixture(tmp_path, "**2 tests in 1 files.**\ntest_one.py: test_alpha and test_beta\n") + expect.text(", ".join(undocumented(d, doc)) or "none", "none", + "control: a fully documented corpus reports nothing missing") + + +def test_an_undocumented_test_is_named_rather_than_passed_over(tmp_path, expect): + """The exact shape that shipped: the file is named, one test inside it is not.""" + d, doc = _fixture(tmp_path, "**2 tests in 1 files.**\ntest_one.py: test_alpha\n") + expect.text(", ".join(undocumented(d, doc)), "test_beta", + "an undocumented test is named rather than passed over") + + +def test_an_undocumented_file_is_caught_with_the_tests_inside_it(tmp_path, expect): + """How 29 tests went missing at once: two whole files were never named.""" + d, doc = _fixture(tmp_path, "**2 tests in 1 files.**\nnothing about the corpus at all\n") + expect.text(", ".join(undocumented(d, doc)), "test_alpha, test_beta, test_one.py", + "an undocumented file is caught along with the tests inside it") + + +def test_a_document_with_no_totals_line_states_none(tmp_path, expect): + """`None` must not read as "the totals happen to match". Absent is its own + answer, the same way `unknown` never reads as `fresh` elsewhere here.""" + d, doc = _fixture(tmp_path, "test_one.py: test_alpha and test_beta\n") + expect.text(repr(stated_totals(doc)), "None", + "a document stating no totals reports None, not a match") + + +def test_a_stated_total_that_disagrees_with_disk_is_visible(tmp_path, expect): + """The count arm's own red. A document can name every test and still lie about + how many there are.""" + d, doc = _fixture(tmp_path, "**9 tests in 4 files.**\ntest_one.py: test_alpha and test_beta\n") + found = corpus_tests(d) + expect.text(repr(stated_totals(doc)), "(9, 4)", "the document states 9 in 4") + expect.text(repr((sum(len(v) for v in found.values()), len(found))), "(2, 1)", + "while the fixture on disk holds 2 in 1") + expect.num(int(stated_totals(doc) == (sum(len(v) for v in found.values()), len(found))), 0, + "a stated total that disagrees with disk does not compare equal") diff --git a/test/selftest/350-the-pytest-corpus-must-be.sh b/test/selftest/350-the-pytest-corpus-must-be.sh new file mode 100644 index 00000000..3329bbf8 --- /dev/null +++ b/test/selftest/350-the-pytest-corpus-must-be.sh @@ -0,0 +1,141 @@ +# ---- the pytest corpus must be documented, or the documentation is decoration - +# +# WHY THIS EXISTS. `test/pytest/TESTS.md` says its job is "what each test asserts, +# and why it exists". It went stale inside a single rework: the corpus grew from +# 25 tests in three files to 54 in five, and the two new files -- 29 tests, every +# one of them added by the rework that answered a review -- were named nowhere in +# it. The header still read "Twenty-five tests in three files". +# +# That is worse than an undocumented directory. A reader who opens a file whose +# stated purpose is completeness does not then go and count the tests, so a +# partial index reads as a total one. The header was not merely out of date, it +# was a false statement of coverage, which is the same defect class the vacuity +# layer in that corpus exists to refuse one level down. +# +# WHY HERE AND NOT IN THE CORPUS ITSELF. Nothing runs pytest. `SUITES` in +# run_all_versions.sh does not list it and no workflow in .github/ invokes it +# (README.md in that directory records the decision and its price). A guard +# written as a pytest test would therefore never run in the gate, and a guard +# that does not run is a comment. `harness_selftest` IS registered, so this is +# the only place the rule can actually bite. The corpus carries a twin of this +# check for anyone running it by hand; this copy is the one with teeth. +# +# WHAT IS POLICED. Three properties, each mechanical: +# * every test_*.py file in the corpus is named in TESTS.md +# * every `def test_` in those files is named in TESTS.md +# * the totals TESTS.md states are the totals on disk +# The third is what the stale header got wrong, and neither of the first two +# would have caught it: a file can name every test and still miscount them. + +_dcv_dir="$PGC_TESTDIR/pytest" +_dcv_doc="$_dcv_dir/TESTS.md" + +check "premise: the pytest corpus is where this part thinks it is" \ + "$([ -d "$_dcv_dir" ] && echo yes || echo no)" "yes" + +check "premise: the corpus carries the documentation this part polices" \ + "$([ -f "$_dcv_doc" ] && echo yes || echo no)" "yes" + +# The sweep, as a function over a DIRECTORY and a DOC, so the arms below can run +# the identical logic over a fixture. A guard that can only be pointed at the +# real tree is proved by nothing: it passes today and there is no way to see it +# fire. selftest 320 makes the same move with the runner's classifier. +# +# Prints the offenders, capped, in the "[]" / "[n:...]" shape the other parts use +# so a failure names what is wrong rather than only that something is. +_dcv_missing() { # _dcv_missing DIR DOC -> "[]" or "[n: a b c]" + local dir="$1" doc="$2" f base name n=0 bad="" doctext + doctext="$(cat "$doc" 2>/dev/null)" + for f in "$dir"/test_*.py; do + # Without nullglob an unmatched glob stays literal, so a corpus with no + # test files would iterate once over a path that does not exist. The + # file test guards that; the premise below asserts the sweep saw files. + [ -f "$f" ] || continue + base="${f##*/}" + case "$doctext" in + *"$base"*) ;; + *) n=$((n + 1)); [ "$n" -le 6 ] && bad="$bad $base" ;; + esac + while IFS= read -r name; do + [ -n "$name" ] || continue + case "$doctext" in + *"$name"*) ;; + *) n=$((n + 1)); [ "$n" -le 6 ] && bad="$bad $name" ;; + esac + done < <(grep -oE '^def (test_[A-Za-z0-9_]+)' "$f" | sed 's/^def //') + done + [ "$n" -eq 0 ] && { printf '[]'; return; } + printf '[%d:%s]' "$n" "$bad" +} + +_dcv_count() { # _dcv_count DIR -> " " + local dir="$1" f t=0 c=0 + for f in "$dir"/test_*.py; do + [ -f "$f" ] || continue + c=$((c + 1)) + t=$((t + $(grep -cE '^def test_' "$f"))) + done + printf '%d %d' "$t" "$c" +} + +# A sweep that found nothing reports "nothing missing" and is indistinguishable +# from a sweep that works. Assert it saw the corpus before believing its verdict. +_dcv_seen="$(_dcv_count "$_dcv_dir")" +check "premise: the sweep found the corpus rather than an empty glob" \ + "$([ "${_dcv_seen%% *}" -ge 20 ] && [ "${_dcv_seen##* }" -ge 3 ] && echo enough || echo "$_dcv_seen")" \ + "enough" + +check "every test file and every test in the corpus is named in TESTS.md" \ + "$(_dcv_missing "$_dcv_dir" "$_dcv_doc")" "[]" + +# The count TESTS.md states, parsed out of it. Written in a fixed form precisely +# so it can be read back: prose that says "twenty-five" cannot be compared with +# anything, which is how the stale header survived being read many times. +_dcv_stated="$(grep -oE '^\*\*[0-9]+ tests in [0-9]+ files\.\*\*' "$_dcv_doc" \ + | head -1 | grep -oE '[0-9]+' | tr '\n' ' ' | sed 's/ $//')" + +check "TESTS.md states its totals in a form that can be read back" \ + "$([ -n "$_dcv_stated" ] && echo yes || echo no)" "yes" + +check "and the totals it states are the totals on disk" \ + "$_dcv_stated" "$_dcv_seen" + +# ---- and the guard must be able to FAIL -------------------------------------- +# +# Everything above passes on a healthy tree, which is exactly what a guard that +# does nothing also does. These arms run the same two functions over fixtures +# built to be wrong, so a future edit that neuters the sweep reddens here even +# while the real corpus stays clean. + +_dcv_fix="$PGC_WORKDIR/doccov"; rm -rf "$_dcv_fix"; mkdir -p "$_dcv_fix" +printf 'def test_alpha(expect):\n pass\ndef test_beta(expect):\n pass\n' \ + > "$_dcv_fix/test_one.py" + +# Documented completely: file named, both tests named, totals stated. +printf '**2 tests in 1 files.**\ntest_one.py: test_alpha and test_beta\n' \ + > "$_dcv_fix/GOOD.md" +check "control: a fully documented corpus reports nothing missing" \ + "$(_dcv_missing "$_dcv_fix" "$_dcv_fix/GOOD.md")" "[]" + +# One test left out. This is the exact shape that shipped. +printf '**2 tests in 1 files.**\ntest_one.py: test_alpha\n' > "$_dcv_fix/PARTIAL.md" +check "an undocumented test is named rather than passed over" \ + "$(_dcv_missing "$_dcv_fix" "$_dcv_fix/PARTIAL.md")" "[1: test_beta]" + +# A whole file left out, which is how 29 tests went missing at once. +printf '**2 tests in 1 files.**\nnothing about the corpus at all\n' > "$_dcv_fix/NONE.md" +check "an undocumented file is caught along with the tests inside it" \ + "$(_dcv_missing "$_dcv_fix" "$_dcv_fix/NONE.md")" "[3: test_one.py test_alpha test_beta]" + +# The count arm, proved separately: a doc can name every test and still state a +# wrong total, which is precisely what the stale header did. +check "the sweep counts the fixture's tests and files" \ + "$(_dcv_count "$_dcv_fix")" "2 1" + +check "a stated total that disagrees with disk is visible" \ + "$([ "$(grep -oE '^\*\*[0-9]+ tests in [0-9]+ files\.\*\*' "$_dcv_fix/GOOD.md" \ + | grep -oE '[0-9]+' | tr '\n' ' ' | sed 's/ $//')" = "$(_dcv_count "$_dcv_fix")" ] \ + && echo agrees || echo differs)" "agrees" + +unset _dcv_dir _dcv_doc _dcv_seen _dcv_stated _dcv_fix +unset -f _dcv_missing _dcv_count From b785795d7ccdae0a5e1f8ba893e90d1bf8679860 Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Wed, 9 Sep 2026 17:06:13 +0000 Subject: [PATCH 09/11] test: an unrunnable pytest test must not leave the run green (#432) Found reviewing my own PR. `expect.cannot_run(REASON, detail)` is the corpus's third state, the counterpart of `check_unrunnable` in lib.sh. It wrote `self.unrunnable` and NOTHING READ IT. Measured, before the fix: def test_declares_itself_unrunnable(expect): expect.cannot_run("ABSENT_FIXTURE", "the fixture was never built") 1 passed in 0.00s EXIT=0 Its own docstring said "Not a pass, and not a silent skip". It was a pass. The field was written at one line and read at none -- the write-only flag shape selftest 320 already polices one level up, where the runner's INCOMPLETE branch set a variable the verdict never read. THIS WAS THE LARGEST VACUITY HOLE IN THE LAYER, and it was the layer's own escape hatch. A bare `@pytest.mark.skip` FAILS the run. The honest-looking alternative greened silently, one line, any of the five reasons. So the layer refused the cheap dishonest escape and permitted the expensive-looking one. An escape hatch that costs nothing is the default. THE FIX MIRRORS lib.sh RATHER THAN INVENTING SEMANTICS. A run holding an unrunnable test exits 67 -- the same number as PGC_EXIT_INCOMPLETE (lib.sh:58), because a runner that learns the code should learn it once, and pytest itself uses only 0-6 so 67 collides with nothing. The reason and detail print in the same shape lib.sh prints: UNRUN test_probe.py::test_cannot: ABSENT_FIXTURE: the parquet corpus was not built checks unrunnable: 1 FAILURE STILL DOMINATES, which is asserted rather than left to fall out: a run with both a failure and an unrunnable test is a failure, because the failure is the more urgent fact. The override only ever moves a run OFF zero. Measured in all four combinations rather than argued: unrunnable only exit 67 exit 67 (-n 2) unrunnable + a failure exit 1 exit 1 (-n 2) TWO IMPLEMENTATION POINTS THAT ARE NOT INCIDENTAL. The declaration travels to the xdist controller as a `user_property` on the test report. A worker's own exit status is discarded by xdist, so a variable held in the worker process would never be seen and the -n column above would have been exit 0 with the serial column right -- the failure mode that only appears in the configuration the corpus actually runs in. The collector is held on the CONFIG, not in a module global, because `pytester` runs the layer's own tests IN-PROCESS: an inner run imports this same module, so a module-level list would leak the inner run's declarations into the outer session and exit the whole corpus INCOMPLETE. WRITTEN TWICE, per jd's rule of 2026-09-09. test/pytest/test_layer.py 4 arms, running pytest inside pytest, asserting on the INNER run's exit status. 10 -> 14 tests. test/selftest/360-an-unrunnable-pytest-test-must.sh 15 arms, static. The .sh half is static on purpose and the file says why: the behavioural arms need pytest, psycopg and a virtualenv, and CI installs none of them, while `pgc_skip` treats a missing dependency as a failure rather than a skip. So the behaviour is pinned where it can run and the STRUCTURE it rests on is pinned where it gates -- the field is read, the read reaches the exit status, the override is conditional so a failure still dominates, and THE TWO HARNESSES AGREE ON 67. That last one is a number now duplicated across a language boundary, parsed out of both files rather than restated here, because a check that restates the number would pass while both copies drifted together. PROVED BY REMOVAL. The pre-fix layer put back in place, the mutation asserted to have applied by md5 (bd9edd602c80 -> 6c005829b88f), and restored byte-exact afterwards: 6 arms of selftest 360 redden, naming each missing property accounting: 278 passed + 9 failed + 0 unrunnable = 287 The three arms beyond those six are selftest 350 catching the four new test_layer.py tests as undocumented -- the doc guard added in the previous commit doing its job on my own work in the same session, `got [62 6] want [66 6]`. The first attempt at these arms used `runpytest_subprocess`, which does not inherit PYTHONPATH, so all three reddened on `ImportError: No module named 'pgc_vacuity'` -- a red for the wrong reason, which TESTS.md section 9 already records as a trap and which I walked into anyway. One of them PASSED under that error by coincidence, asserting exit 1 against a usage error. Switched to the in-process runner the rest of the file uses. Verified: harness_selftest 287 passed + 0 failed + 0 unrunnable PASSED (272 before) docs_style 9 checks PASSED pytest 66 passed serial, 66 passed -n 4, marker cleared for each 66 passed with --pgc-expect-tests 66 shellcheck -S error -s bash test/*.sh test/selftest/*.sh exit 0 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- CHANGELOG.md | 13 +- test/pytest/TESTS.md | 60 ++++++++- test/pytest/pgc_vacuity.py | 98 +++++++++++++- test/pytest/test_layer.py | 79 +++++++++++ .../360-an-unrunnable-pytest-test-must.sh | 123 ++++++++++++++++++ 5 files changed, 366 insertions(+), 7 deletions(-) create mode 100644 test/selftest/360-an-unrunnable-pytest-test-must.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index 725db346..2576758e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -203,7 +203,7 @@ true until the next version shipped. This ports ONE bash suite. `test/` carries 4,429 anchored assertions across 256 suites, so this is 0.18% of them and is not coverage. The layer is the point: - 62 tests in 6 files, of which 47 test the harness rather than the product. + 66 tests in 6 files, of which 51 test the harness rather than the product. A pytest run fails open in several ways this project has already been bitten by: a test that asserts nothing passes, a filter that selects nothing exits 0, and a @@ -239,6 +239,17 @@ true until the next version shipped. `shared_preload_libraries` maps the library at postmaster start, so a cluster started before the install keeps the old one mapped for its whole life. + **The third state is a state, not a comment.** `expect.cannot_run(REASON, + detail)` wrote a field nothing read, so a test declaring itself unrunnable + reported `1 passed` and exit 0 -- a write-only flag, the shape selftest 320 + already polices in the runner. It made the layer's own escape hatch its largest + hole, because a bare `@pytest.mark.skip` fails the run while the honest-looking + alternative greened silently. A run holding one now exits 67, the same number as + `PGC_EXIT_INCOMPLETE` in `lib.sh`, and prints the reason and detail in the same + shape; a run holding a real failure as well still exits 1, because failure + dominates. Verified serial and under `-n 2`, the declaration travelling to the + xdist controller on the test report. + **`test/pytest/TESTS.md` is checked rather than trusted.** It documents every test in the corpus, and it went stale inside a single rework: 29 of 54 tests were named nowhere in it while its header still claimed 25. A guard now requires diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 5069d0c4..855e2b55 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -4,7 +4,7 @@ Reference for anyone reading, running, or adding to `test/pytest/`. The design a the decisions behind the harness are in `design/ISSUE_432_PYTEST_HARNESS.md`. This file covers the tests themselves. -**62 tests in 6 files.** Forty-seven of them test the harness rather than the +**66 tests in 6 files.** Fifty-one of them test the harness rather than the product, and they come first, because a harness that can report a false green makes every other result in this directory worthless. @@ -90,9 +90,9 @@ Python, where encoding or collation could make identical rows hash differently. ## 3. test_layer.py: the guards, testing themselves -These ten run pytest inside pytest through the `pytester` fixture. Each writes a -small test file, runs it with the plugin loaded, and asserts on the INNER run's -outcome. That is what proves a guard REFUSES, rather than assuming it. +These fourteen run pytest inside pytest through the `pytester` fixture. Each +writes a small test file, runs it with the plugin loaded, and asserts on the INNER +run's outcome. That is what proves a guard REFUSES, rather than assuming it. Each row names the measured bare-pytest behaviour the guard exists to stop. Every one of those eight measurements exited 0. @@ -109,8 +109,12 @@ one of those eight measurements exited 0. | `test_layer_rejects_a_bare_skip` | a bare `@pytest.mark.skip` fails the run | `2 skipped`, exit 0 | | `test_layer_fails_on_a_collected_count_mismatch` | a run that collects fewer tests than expected fails | a filtered run exits 5, widely treated as fine | | `test_layer_refuses_a_zero_expectation` | `--pgc-expect-tests 0` is refused | it would be satisfied by collecting nothing | +| `test_an_unrunnable_test_does_not_leave_the_run_green` | a test declaring itself unrunnable exits 67 | **`1 passed`, exit 0** | +| `test_an_unrunnable_test_names_its_reason_and_its_detail` | the `UNRUN` line carries reason and detail | nothing was printed at all | +| `test_a_real_failure_outranks_an_unrunnable_test` | a run with both exits 1, not 67 | — | +| `test_a_run_with_nothing_unrunnable_still_exits_zero` | **control**: a green run is untouched | — | -Four of the ten are controls rather than guards. They are not decoration. A guard +Five of the fourteen are controls rather than guards. They are not decoration. A guard with a bad false-positive rate gets switched off, and then the guard it replaced is gone too. `test_a_counted_assertion_passes` and `test_layer_matches_the_exact_provider` exist so that a guard which starts @@ -121,6 +125,52 @@ The escape hatches are deliberately more expensive to type than the honest form. number. `cannot_run` takes a reason from a closed list. None of them can become the default by being shorter. +### The third state, and the hole it left + +**`cannot_run` reported a pass.** It wrote `self.unrunnable` and nothing read it, +so a test that declared itself unrunnable printed `1 passed` and exited 0 — +measured, not inferred. A write-only field, the same shape +`test/selftest/320-a-check-that-could-not-run.sh` polices in the runner, where an +INCOMPLETE branch set a variable the verdict never read. + +**It made the layer's own escape hatch its largest hole.** A bare +`@pytest.mark.skip` FAILS the run. The honest-looking alternative greened +silently, so the layer refused the cheap dishonest escape and permitted the +expensive-looking one. An escape hatch that costs nothing is the default. + +The run now ends `EXIT_INCOMPLETE`, which is 67 — deliberately the same number as +`PGC_EXIT_INCOMPLETE` in `lib.sh:58`, because a runner that learns the code should +learn it once. pytest itself uses 0–6, so 67 collides with nothing. The reason and +detail print in lib.sh's shape: + +``` +UNRUN test_probe.py::test_cannot: ABSENT_FIXTURE: the parquet corpus was not built +checks unrunnable: 1 +``` + +**Failure still dominates**, exactly as in lib.sh: a run with both a failure and an +unrunnable test is a failure, because the failure is the more urgent fact. The +override only ever moves a run off zero. Measured in all four combinations, serial +and under `-n 2`: + +``` +unrunnable only exit 67 exit 67 (-n 2) +unrunnable + a failure exit 1 exit 1 (-n 2) +``` + +The xdist column is not decoration. The declaration travels to the controller as a +`user_property` on the test report, because a worker's own exit status is discarded +by xdist and a variable held in the worker process would never be seen. The +collector is held on the **config**, not in a module global, because `pytester` +runs the layer's own tests in-process: a module-level list would leak an inner +run's declarations into the outer session and exit the whole corpus INCOMPLETE. + +The structural half of this is pinned in the gate by +`test/selftest/360-an-unrunnable-pytest-test-must.sh`, which is greppable from a +checkout with nothing installed — it asserts the field is read, that the read +reaches the exit status, that the override is conditional, and that the two +harnesses agree on 67. Against the pre-fix layer it reddens six arms. + ## 4. test_guards_pinned.py: every refusal, pinned to its own message **Why this file exists.** @jdatcmd neutered each guard in the layer in turn and diff --git a/test/pytest/pgc_vacuity.py b/test/pytest/pgc_vacuity.py index 6832c129..0f671173 100644 --- a/test/pytest/pgc_vacuity.py +++ b/test/pytest/pgc_vacuity.py @@ -42,6 +42,12 @@ # own tests and two workers cannot share a counter. _RECORDERS = {} +# lib.sh:58 PGC_EXIT_INCOMPLETE. The same number deliberately: a suite that could +# not evaluate something exits 67 there, and a runner that learns the code learns +# it once. pytest itself uses 0-6 (`pytest.ExitCode`), so 67 collides with +# nothing. +EXIT_INCOMPLETE = 67 + class VacuityError(AssertionError): """Raised when an assertion could not have failed, or asserted nothing.""" @@ -295,7 +301,19 @@ def plan_marker(self, plan, key, name=None, absent=False): # -- the third state --------------------------------------------------- def cannot_run(self, reason, detail=""): - """Declare this test unrunnable. Not a pass, and not a silent skip.""" + """Declare this test unrunnable. Not a pass, and not a silent skip. + + THE STATE HAS TO COST SOMETHING OR IT IS A SKIP WITH BETTER MANNERS. It + did not, at first: this wrote `self.unrunnable` and nothing read it, so a + test calling this reported `1 passed` and exit 0. A write-only field -- + the same shape selftest 320 polices in the runner, where an INCOMPLETE + branch set a flag the verdict never read. It made the layer's own escape + hatch its largest hole: a bare `@pytest.mark.skip` FAILS the run, while + the honest-looking alternative greened silently. + + The run now ends `EXIT_INCOMPLETE` unless something failed outright, and + the reason and detail are printed. See `_UnrunnableCollector` below. + """ if reason not in UNRUNNABLE_REASONS: raise VacuityError( f"unrunnable reason {reason!r} is not one of {UNRUNNABLE_REASONS}" @@ -330,6 +348,84 @@ def pytest_runtest_call(item): return result +class _UnrunnableCollector: + """Gathers the unrunnable declarations of ONE session. + + Held on the config rather than in a module global, because `pytester` runs + the layer's own tests IN-PROCESS: an inner run imports this same module, so a + module-level list would leak the inner run's declarations into the outer + session and exit the whole corpus INCOMPLETE. One collector per config is one + per session, inner runs included. + """ + + def __init__(self): + self.items = [] + + def pytest_runtest_logreport(self, report): + # This hook fires on the CONTROLLER for reports received from xdist + # workers, which is why the declaration travels as a user_property + # rather than in a variable the worker process owns. A worker's own + # exit status is discarded by xdist; the controller's is the run's. + if report.when != "call": + return + for key, value in getattr(report, "user_properties", ()): + if key == "pgc_unrunnable": + reason, _, detail = value.partition("\n") + self.items.append((report.nodeid, reason, detail)) + + +def pytest_configure(config): + collector = _UnrunnableCollector() + config.pluginmanager.register(collector, "pgc_unrunnable_collector") + config.pgc_unrunnable = collector + + +@pytest.hookimpl(wrapper=True) +def pytest_runtest_makereport(item, call): + """Carry an unrunnable declaration out on the report itself. + + `user_properties` is serialised across the xdist boundary; an attribute of + our own would not be. + """ + report = yield + if call.when == "call": + rec = _RECORDERS.get(item.nodeid) + if rec is not None and rec.unrunnable: + reason, detail = rec.unrunnable + report.user_properties.append(("pgc_unrunnable", f"{reason}\n{detail}")) + return report + + +def pytest_terminal_summary(terminalreporter): + """Print the third state, in lib.sh's shape. + + `UNRUN : : `, then the count. A state that does not + say why is a skip with better manners, and a state with no count cannot be + reconciled against the total. + """ + collector = getattr(terminalreporter.config, "pgc_unrunnable", None) + if collector is None or not collector.items: + return + terminalreporter.write_line("") + for nodeid, reason, detail in collector.items: + terminalreporter.write_line(f"UNRUN {nodeid}: {reason}: {detail}") + terminalreporter.write_line(f"checks unrunnable: {len(collector.items)}") + + +def pytest_sessionfinish(session, exitstatus): + """An unrunnable test must not leave the run green. + + FAILURE STILL DOMINATES, exactly as in lib.sh: a run with both a failure and + an unrunnable test is a failure, because the failure is the more urgent fact. + So this only ever moves a run OFF zero, and never off a non-zero status. + """ + collector = getattr(session.config, "pgc_unrunnable", None) + if collector is None or not collector.items: + return + if exitstatus == 0: + session.exitstatus = EXIT_INCOMPLETE + + def pytest_addoption(parser): parser.addoption( "--pgc-expect-tests", diff --git a/test/pytest/test_layer.py b/test/pytest/test_layer.py index 0724001a..831bd15c 100644 --- a/test/pytest/test_layer.py +++ b/test/pytest/test_layer.py @@ -175,3 +175,82 @@ def test_layer_refuses_a_zero_expectation(pytester, expect): pytester.makepyfile("def test_one(expect): expect.num(1, 1, 'one')") result = pytester.runpytest("-p", "pgc_vacuity", "--pgc-expect-tests", "0") expect.run_failed(result, "an expectation of zero asserts nothing") + + +# --------------------------------------------------------------------------- +# THE THIRD STATE. `cannot_run` declared a test unrunnable and the run reported +# it as a PASS, exit 0: `self.unrunnable` was written and read nowhere. That is +# a write-only field, the same shape selftest 320 polices in the runner ("no +# write-only failure flag survives"), and it made the layer's own escape hatch +# the largest vacuity hole in it -- a bare skip FAILS the run, while the +# supposedly honest alternative greened silently. +# +# lib.sh has kept this state honest since #418: an unrunnable check counts +# toward checks run, is reported separately, and the suite exits +# PGC_EXIT_INCOMPLETE (67) so no runner can call it a pass. These four arms are +# the pytest mirror of selftest 320's four, including which state dominates. + + +def test_an_unrunnable_test_does_not_leave_the_run_green(pytester, expect): + """The defect itself. Measured before the fix: `1 passed`, exit 0.""" + pytester.makeconftest("pytest_plugins = ['pgc_vacuity']") + pytester.makepyfile( + """ + def test_cannot(expect): + expect.cannot_run("ABSENT_FIXTURE", "the parquet corpus was not built") + """ + ) + result = pytester.runpytest("-p", "pgc_vacuity") + expect.num(result.ret, 67, "an unrunnable test exits INCOMPLETE, not 0") + + +def test_an_unrunnable_test_names_its_reason_and_its_detail(pytester, expect): + """A third state that does not say why is a skip with better manners. + + The reason travels with the state, exactly as lib.sh prints + `UNRUN : : `. + """ + pytester.makeconftest("pytest_plugins = ['pgc_vacuity']") + pytester.makepyfile( + """ + def test_cannot(expect): + expect.cannot_run("ABSENT_FIXTURE", "the parquet corpus was not built") + """ + ) + result = pytester.runpytest("-p", "pgc_vacuity") + result.stdout.fnmatch_lines( + ["*UNRUN*test_cannot*ABSENT_FIXTURE*the parquet corpus was not built*"]) + expect.num(result.ret, 67, "and it is still not a pass") + + +def test_a_real_failure_outranks_an_unrunnable_test(pytester, expect): + """Which state dominates, asserted rather than left to fall out. + + lib.sh: a suite with both a FAIL and an UNRUN is FAILED, because the failure + is the more urgent fact. Exit 1, not 67. + """ + pytester.makeconftest("pytest_plugins = ['pgc_vacuity']") + pytester.makepyfile( + """ + def test_cannot(expect): + expect.cannot_run("ABSENT_FIXTURE", "no corpus") + def test_fails(expect): + expect.num(1, 2, "one is not two") + """ + ) + result = pytester.runpytest("-p", "pgc_vacuity") + expect.num(result.ret, 1, "a failure outranks an unrunnable test") + + +def test_a_run_with_nothing_unrunnable_still_exits_zero(pytester, expect): + """Control. A guard that reddens a healthy run gets switched off, and then + the guard it replaced is gone too.""" + pytester.makeconftest("pytest_plugins = ['pgc_vacuity']") + pytester.makepyfile( + """ + def test_ok(expect): + expect.num(2 + 2, 4, "arithmetic still works") + """ + ) + result = pytester.runpytest("-p", "pgc_vacuity") + expect.num(result.ret, 0, "an ordinary green run is untouched") diff --git a/test/selftest/360-an-unrunnable-pytest-test-must.sh b/test/selftest/360-an-unrunnable-pytest-test-must.sh new file mode 100644 index 00000000..8a8b7126 --- /dev/null +++ b/test/selftest/360-an-unrunnable-pytest-test-must.sh @@ -0,0 +1,123 @@ +# ---- an unrunnable pytest test must not leave the run green ------------------ +# +# WHY THIS EXISTS. `expect.cannot_run(REASON, detail)` is the pytest corpus's +# third state, the counterpart of `check_unrunnable` here. It wrote +# `self.unrunnable` and NOTHING READ IT, so a test that declared itself +# unrunnable reported `1 passed` and exit 0. Measured, before the fix. +# +# That is the write-only-flag shape selftest 320 already polices one level up, +# where the runner's INCOMPLETE branch set a variable the verdict never read. It +# mattered more here: a bare `@pytest.mark.skip` FAILS the pytest run, so the +# layer refused the cheap dishonest escape and permitted the expensive-looking +# one. An escape hatch that costs nothing is the default. +# +# WHY THE CHECKS ARE STATIC. The behaviour itself is pinned in the corpus, by +# four arms in test/pytest/test_layer.py that run pytest inside pytest and assert +# on the inner run's exit status. Those need pytest, psycopg and a virtualenv; +# CI installs none of them, and `pgc_skip` treats a missing dependency as a +# failure rather than a skip. So the behavioural half lives where it can run and +# this half asserts the STRUCTURE that behaviour rests on, which is greppable +# from a checkout with nothing installed. Same division as selftest 320's last +# two arms, which grep the runner for the call and for the absence of the flag. +# +# THE NUMBER IS THE POINT OF THE FIRST ARM. 67 now lives in two files. A number +# duplicated across a language boundary is a number that drifts, and the drift +# is invisible: the corpus would exit 67, the runner would compare against +# something else, and an INCOMPLETE run would read as a failure or as a pass +# depending on which way it moved. + +_ts_lib="$PGC_TESTDIR/lib.sh" +_ts_vac="$PGC_TESTDIR/pytest/pgc_vacuity.py" + +check "premise: the harness library is where this part thinks it is" \ + "$([ -f "$_ts_lib" ] && echo yes || echo no)" "yes" + +check "premise: the pytest layer is where this part thinks it is" \ + "$([ -f "$_ts_vac" ] && echo yes || echo no)" "yes" + +# Parsed out of each file rather than written here. A check that restates the +# number tests this file against itself: both copies could drift together and it +# would still pass. +_ts_sh_code="$(sed -n 's/^PGC_EXIT_INCOMPLETE=\([0-9]\{1,\}\).*/\1/p' "$_ts_lib" | head -1)" +_ts_py_code="$(sed -n 's/^EXIT_INCOMPLETE[[:space:]]*=[[:space:]]*\([0-9]\{1,\}\).*/\1/p' "$_ts_vac" | head -1)" + +check "premise: lib.sh states an INCOMPLETE exit code this part could read" \ + "$([ -n "$_ts_sh_code" ] && echo yes || echo no)" "yes" + +check "premise: the pytest layer states one too" \ + "$([ -n "$_ts_py_code" ] && echo yes || echo no)" "yes" + +check "the two harnesses agree on the INCOMPLETE exit code" \ + "$_ts_py_code" "$_ts_sh_code" + +# ---- the field must be READ, which is the defect this part is named after --- +# +# Counted as two populations rather than asserted as a boolean, so the failure +# says which side is missing. +_ts_writes="$(grep -c 'self\.unrunnable[[:space:]]*=' "$_ts_vac")" +_ts_reads="$(grep -c 'rec\.unrunnable' "$_ts_vac")" + +check "the layer still writes the unrunnable state" \ + "$([ "$_ts_writes" -ge 1 ] && echo yes || echo "$_ts_writes")" "yes" + +check "and something READS it, rather than only writing it" \ + "$([ "$_ts_reads" -ge 1 ] && echo yes || echo "$_ts_reads")" "yes" + +# ---- and the read has to reach the run's exit status ------------------------ +# +# Reading the field into a list nothing acts on would satisfy the arm above and +# leave the defect exactly where it was. +check "the layer ends a session by setting its exit status" \ + "$(grep -c 'session\.exitstatus[[:space:]]*=' "$_ts_vac")" "1" + +# Failure dominates, the same rule lib.sh keeps: a run with a failure AND an +# unrunnable test is a failure. So the override must be conditional on a +# currently-clean run. Unconditional, it would MASK failures as INCOMPLETE. +check "and only ever moves a run off zero, so a failure still dominates" \ + "$(grep -c 'exitstatus == 0' "$_ts_vac")" "1" + +# The state says why. A third state that does not name its reason is a skip. +check "the layer prints the unrunnable reason in lib.sh's shape" \ + "$(grep -c 'UNRUN.*{reason}' "$_ts_vac")" "1" + +# ---- and these greps must be able to FAIL ----------------------------------- +# +# Every arm above passes on a healthy tree, which a grep that matches nothing +# also does -- against an EMPTY file, `grep -c` returns 0 and every `-ge 1` arm +# would read "no" while every `-c ... "1"` arm would read 0. Those would be +# visible. The dangerous case is the opposite: a pattern that is subtly wrong +# still matching. So the fixtures below are the real code with ONE property +# removed, and the arms assert the greps notice. + +_ts_fix="$PGC_WORKDIR/thirdstate"; rm -rf "$_ts_fix"; mkdir -p "$_ts_fix" + +# The defect as it shipped: the field is written and never read. +{ + printf 'EXIT_INCOMPLETE = 67\n' + printf 'class Expect:\n' + printf ' def cannot_run(self, reason, detail=""):\n' + printf ' self.unrunnable = (reason, detail)\n' +} > "$_ts_fix/writeonly.py" + +check "a write-only unrunnable field is caught" \ + "$(grep -c 'rec\.unrunnable' "$_ts_fix/writeonly.py")" "0" + +check "premise: and that same fixture does show the write, so the arm is not blind" \ + "$(grep -c 'self\.unrunnable[[:space:]]*=' "$_ts_fix/writeonly.py")" "1" + +# The exit code drifted. Both files parse; the values differ. +printf 'EXIT_INCOMPLETE = 66\n' > "$_ts_fix/drifted.py" +_ts_drift="$(sed -n 's/^EXIT_INCOMPLETE[[:space:]]*=[[:space:]]*\([0-9]\{1,\}\).*/\1/p' "$_ts_fix/drifted.py" | head -1)" +check "a drifted exit code is visible rather than absorbed" \ + "$([ "$_ts_drift" = "$_ts_sh_code" ] && echo agrees || echo "differs:$_ts_drift/$_ts_sh_code")" \ + "differs:66/67" + +# An unconditional override, which would mask a failing run as INCOMPLETE. +printf ' session.exitstatus = EXIT_INCOMPLETE\n' > "$_ts_fix/unconditional.py" +check "an unconditional exit override is caught by the dominance arm" \ + "$(grep -c 'exitstatus == 0' "$_ts_fix/unconditional.py")" "0" + +check "premise: while the real layer satisfies that same arm" \ + "$(grep -c 'exitstatus == 0' "$_ts_vac")" "1" + +unset _ts_lib _ts_vac _ts_sh_code _ts_py_code _ts_writes _ts_reads _ts_fix _ts_drift From 9064a46eb57d26103584990ab458943d2b4d34da Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Wed, 9 Sep 2026 17:36:21 +0000 Subject: [PATCH 10/11] test: my own guard had a false positive and a pattern that could not tell a read from a write Both found by RUNNING selftest 360 against the next branch in this stack rather than by rereading it, which is the only reason either was visible. FAIL the layer ends a session by setting its exit status: got [3] want [1] Two defects in one arm. `=` MATCHES INSIDE `==`. The pattern was `session\.exitstatus[[:space:]]*=`, so the comparison if exitstatus == 0 and session.exitstatus == 0: counted as an ASSIGNMENT. A read counted as a write, in the arm whose entire subject is the difference between the two -- the same shape as the SC1087 fix earlier today, where a pattern looked right and matched more than it named. `[^=]` after the `=` separates them. Proved on three trees: 1 assignment on this branch, 2 on audit/432-pytest-oracles, 0 on the pre-fix layer. AND THE COUNT WAS AN EQUALITY WHERE THE PROPERTY IS A FLOOR. "Exactly 1" went red on a branch that adds a SECOND escalation for a different condition -- a legitimate addition reported as a defect. The property is "the read reaches the exit status"; one site satisfies it and two do not make it less true. An equality here reddens on growth, and a guard that reddens on growth is a guard somebody switches off. That is the false-positive budget I should have measured before shipping it: I ran the guard over one tree and called it proved. Two fixture arms added, because a correction with no red is not proved: a comparison on the exit status is not counted as an assignment -> 0 premise: while a real assignment on the same line shape IS counted -> 1 Without the second, the first passes against a pattern that matches nothing at all. THE REMOVAL PROOF STILL HOLDS, which is what the floor had to not break. The pre-fix layer put back with `git show e175cd4:`, mutation asserted applied by md5 (bd9edd602c80 -> 6c005829b88f), restored byte-exact: 7 FAILs, including and something READS it, rather than only writing it: got [0] the layer ends a session by setting its exit status: got [0] and only ever moves a run off zero, so a failure still dominates: got [0] My first attempt at that proof reverted pgc_vacuity.py to b785795 -- the commit that ADDED the third state -- so the file did not change and the run would have been a green that meant nothing. The `MUTATION DID NOT APPLY` assert inside the proof caught it. That assert has now earned its place twice in one day. Verified: harness_selftest 289 passed + 0 failed + 0 unrunnable PASSED (287 before) shellcheck -S error -s bash test/*.sh test/selftest/*.sh exit 0 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- .../360-an-unrunnable-pytest-test-must.sh | 32 +++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/test/selftest/360-an-unrunnable-pytest-test-must.sh b/test/selftest/360-an-unrunnable-pytest-test-must.sh index 8a8b7126..81effe10 100644 --- a/test/selftest/360-an-unrunnable-pytest-test-must.sh +++ b/test/selftest/360-an-unrunnable-pytest-test-must.sh @@ -67,8 +67,26 @@ check "and something READS it, rather than only writing it" \ # # Reading the field into a list nothing acts on would satisfy the arm above and # leave the defect exactly where it was. +# +# TWO CORRECTIONS THIS ARM ALREADY NEEDED, both found by running it against a +# LATER branch rather than by rereading it. +# +# `=` MATCHES INSIDE `==`. The first pattern was `exitstatus[[:space:]]*=`, which +# counted the comparison `if ... session.exitstatus == 0:` as an assignment. A +# read counted as a write, in the arm whose entire subject is the difference +# between the two. `[^=]` after the `=` is what separates them. +# +# AND THE COUNT IS A FLOOR, NOT AN EQUALITY. Written as "exactly 1", it went red +# on the next branch in this stack, which adds a second escalation for a +# different condition -- a legitimate addition reported as a defect. The property +# is "the read reaches the exit status", so one site satisfies it and two do not +# make it less true. An equality here is a guard that reddens on growth, and a +# guard that reddens on growth gets switched off. Measured: 1 site on this +# branch, 2 on audit/432-pytest-oracles, 0 on the pre-fix layer -- so the floor +# still fails exactly where it must. +_ts_assigns="$(grep -c 'session\.exitstatus[[:space:]]*=[^=]' "$_ts_vac")" check "the layer ends a session by setting its exit status" \ - "$(grep -c 'session\.exitstatus[[:space:]]*=' "$_ts_vac")" "1" + "$([ "$_ts_assigns" -ge 1 ] && echo yes || echo "$_ts_assigns")" "yes" # Failure dominates, the same rule lib.sh keeps: a run with a failure AND an # unrunnable test is a failure. So the override must be conditional on a @@ -117,7 +135,17 @@ printf ' session.exitstatus = EXIT_INCOMPLETE\n' > "$_ts_fix/unconditional.py check "an unconditional exit override is caught by the dominance arm" \ "$(grep -c 'exitstatus == 0' "$_ts_fix/unconditional.py")" "0" +# A file holding ONLY the comparison. Under the first pattern this counted as an +# assignment, which is the false positive that shipped; under the corrected one it +# is zero. Without this arm the correction above is itself unproved. +printf ' if exitstatus == 0 and session.exitstatus == 0:\n' > "$_ts_fix/compare.py" +check "a comparison on the exit status is not counted as an assignment" \ + "$(grep -c 'session\.exitstatus[[:space:]]*=[^=]' "$_ts_fix/compare.py")" "0" + +check "premise: while a real assignment on the same line shape IS counted" \ + "$(grep -c 'session\.exitstatus[[:space:]]*=[^=]' "$_ts_fix/unconditional.py")" "1" + check "premise: while the real layer satisfies that same arm" \ "$(grep -c 'exitstatus == 0' "$_ts_vac")" "1" -unset _ts_lib _ts_vac _ts_sh_code _ts_py_code _ts_writes _ts_reads _ts_fix _ts_drift +unset _ts_lib _ts_vac _ts_sh_code _ts_py_code _ts_writes _ts_reads _ts_fix _ts_drift _ts_assigns From 723af38a70abf03e0d4a3495e82117aaaf37d74b Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Wed, 9 Sep 2026 18:18:23 +0000 Subject: [PATCH 11/11] test: pin plan_marker's two arms, and close the hole underneath them (#432) THE LAST OPEN ITEM FROM @jdatcmd's REVIEW. He named this one first: "plan_marker is the one I would fix first. Your own docstring names it the faithful port of pgc_is_columnar_scan, test_connection.py calls it three times including once as the PREMISE that the vector aggregate engaged, and both of its arms can be deleted independently with the suite green. Under one of those mutations the premise can never fail, so the provider-trap test would silently be about an ordinary plan." He was right and it was still open. test_guards_pinned.py's only mention of plan_marker was its own docstring, listing it among the helpers the corpus never drives. It is the worst place in the layer for a guard that cannot fail, because a premise that cannot fail turns its test into a test about an ordinary plan and nothing goes red while it happens. A THIRD HOLE SAT UNDERNEATH BOTH ARMS, and it is the one I would not have found by reading. An absence claim is satisfied by nothing being there at all: expect.plan_marker([], "Columnar Projected Columns", absent=True) 1 passed, exit 0 A plan that never arrived looks exactly like a plan that legitimately lacks the node, and absent=True cannot tell them apart. It is now a VacuityError, and it refuses the present arm too -- deliberately: an empty plan means the EXPLAIN did not arrive, so NEITHER question can be answered. The present arm would have failed anyway, but with "no node carries it, Columnar keys present: []", which diagnoses the wrong thing. PROVED BY REMOVAL, and the interesting part is not that something reddens. unmutated 38c951eb7dda 5 passed present arm neutered dc066341dba2 1 failed absent arm neutered 7ce63404d821 1 failed empty-plan guard neutered a4e9d763e77c 1 failed restored 38c951eb7dda byte-exact EACH MUTATION REDDENS EXACTLY ONE TEST AND IT IS THAT TEST'S OWN. The other four stay green in every arm. That is what proves the three guards are DISTINGUISHABLE rather than subsumed -- the failure mode this whole review round was about, where neutering one guard is caught by a neighbour and an outcome-only assertion cannot tell which fired. Each mutation is asserted applied by md5 before the run and the file is compared byte-for-byte after. The four arm tests use expect.outcomes rather than expect.refusal, because plan_marker's two arms raise AssertionError, not VacuityError: they are wrong answers, not degenerate inputs. Only the empty-plan case is a refusal. Naming that distinction is the point -- expect.refusal on an ordinary assertion would assert on a message that is not a contract. WRITTEN TWICE, per jd's rule of 2026-09-09. test/pytest/test_guards_pinned.py 5 arms, behavioural. 14 -> 19 tests. test/selftest/370-the-plan-marker-guard-must.sh 12 arms, static. Same division as 360 and the file says why: the behavioural arms need pytest, psycopg and a virtualenv and CI installs none of them, so the structure they rest on is pinned where it gates. The .sh half cuts plan_marker's body out with awk before grepping -- a grep over the whole file would match these shapes wherever they occur and report a guard present that lives in another method -- and it asserts the ORDERING too: the empty-plan refusal must precede the arm it protects, or an empty plan leaves found=False and the absent arm returns a pass first, making the refusal dead code. Verified: harness_selftest 301 passed + 0 failed + 0 unrunnable PASSED (289 before) docs_style 9 checks PASSED pytest 71 passed serial, 71 passed -n 4, marker cleared for each 71 passed with --pgc-expect-tests 71 shellcheck -S error -s bash test/*.sh test/selftest/*.sh exit 0 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- CHANGELOG.md | 14 ++- test/pytest/TESTS.md | 43 ++++++++- test/pytest/pgc_vacuity.py | 26 +++++- test/pytest/test_guards_pinned.py | 92 +++++++++++++++++++ .../370-the-plan-marker-guard-must.sh | 90 ++++++++++++++++++ 5 files changed, 262 insertions(+), 3 deletions(-) create mode 100644 test/selftest/370-the-plan-marker-guard-must.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index 2576758e..25cadeb0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -203,7 +203,7 @@ true until the next version shipped. This ports ONE bash suite. `test/` carries 4,429 anchored assertions across 256 suites, so this is 0.18% of them and is not coverage. The layer is the point: - 66 tests in 6 files, of which 51 test the harness rather than the product. + 71 tests in 6 files, of which 56 test the harness rather than the product. A pytest run fails open in several ways this project has already been bitten by: a test that asserts nothing passes, a filter that selects nothing exits 0, and a @@ -230,6 +230,18 @@ true until the next version shipped. `expect.refusal`, which refuses to be called with no pattern -- the same move as asserting on a SQLSTATE rather than on prose. + **`plan_marker`'s two arms are pinned, and the hole under them is closed.** + Both could be deleted independently with the suite green -- and it is the worst + place in the layer for that, because `plan_marker` is the port of + `pgc_is_columnar_scan` and is used as the PREMISE that the vectorized aggregate + engaged. A premise that cannot fail turns its test into one about an ordinary + plan. Underneath both sat a third hole: an absence claim is satisfied by + nothing being there at all, so `plan_marker([], key, absent=True)` passed + against a plan that never arrived. That is now refused, for the present arm + too. Each of the three neutered alone reddens exactly one test, and it is that + test's own -- which is what proves they are distinguishable rather than + subsumed. + **The corpus builds and installs before it measures anything.** It did not at first: with `#error` appended to a source file and nothing rebuilt, the run reported 25 passed and exit 0 while the bash suite reported `FATAL: the build diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 855e2b55..b0660e67 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -4,7 +4,7 @@ Reference for anyone reading, running, or adding to `test/pytest/`. The design a the decisions behind the harness are in `design/ISSUE_432_PYTEST_HARNESS.md`. This file covers the tests themselves. -**66 tests in 6 files.** Fifty-one of them test the harness rather than the +**71 tests in 6 files.** Fifty-six of them test the harness rather than the product, and they come first, because a harness that can report a false green makes every other result in this directory worthless. @@ -209,8 +209,49 @@ as the failure. | `test_hash_refuses_a_LEFT_error_sentinel` | a `QUERY_ERROR` on the left | | `test_hash_refuses_a_RIGHT_error_sentinel` | the mirror, which one arm never covered | | `test_hash_refuses_two_empties` | two distinct empty values | +| `test_plan_marker_present_arm_fails_when_the_key_is_absent` | the arm that makes "did the columnar scan run" answerable | +| `test_plan_marker_present_arm_passes_when_the_key_is_there` | **control** | +| `test_plan_marker_absent_arm_fails_when_the_key_is_present` | the arm that pins the vector-aggregate trap | +| `test_plan_marker_absent_arm_passes_on_a_plan_that_lacks_the_key` | **control** | +| `test_plan_marker_refuses_an_absence_claim_over_an_empty_plan` | the hole under both arms | | `test_refusal_itself_refuses_an_empty_pattern_list` | the new helper must not become the defect it removes | +### plan_marker, and the three ways it could not fail + +@jdatcmd named this one first: *"both of its arms can be deleted independently +with the suite green. Under one of those mutations the premise can never fail, so +the provider-trap test would silently be about an ordinary plan."* + +It is the worst place in the layer for that to be true. `plan_marker` is the +faithful port of `pgc_is_columnar_scan`, and `test_connection.py` calls it three +times — once as the **premise** that the vectorized aggregate engaged. A premise +that cannot fail turns its test into a test about an ordinary plan, and nothing +goes red while it happens. + +**A third hole sat underneath both arms.** An absence claim is satisfied by +nothing being there at all: `plan_marker([], key, absent=True)` gave `1 passed`, +exit 0, because a plan that never arrived looks exactly like a plan that +legitimately lacks the node. That is now a `VacuityError`, and it is refused for +the present arm too — an empty plan means the `EXPLAIN` did not arrive, so +neither question can be answered. + +The four arm tests are behavioural rather than refusals, because `plan_marker`'s +two arms raise `AssertionError`: `expect.refusal` does not apply and +`expect.outcomes` is the right instrument. Their value is not their own green, +which they had before the guards were pinned. It is the census: + +``` +unmutated 38c951eb7dda 5 passed +present arm neutered dc066341dba2 1 failed <- its own arm, and only it +absent arm neutered 7ce63404d821 1 failed <- its own arm, and only it +empty-plan guard neutered a4e9d763e77c 1 failed <- its own arm, and only it +restored 38c951eb7dda byte-exact +``` + +**Each mutation reddens exactly one test, and it is that test's own.** That is +the property worth having: it proves the three are distinguishable rather than +subsumed, which "something went red" cannot. + Three of these carry reasoning that is easy to lose. **The sentinel arms name the SIDE.** A single arm asserting "is a failed query" diff --git a/test/pytest/pgc_vacuity.py b/test/pytest/pgc_vacuity.py index 0f671173..af398d4e 100644 --- a/test/pytest/pgc_vacuity.py +++ b/test/pytest/pgc_vacuity.py @@ -286,8 +286,32 @@ def plan_marker(self, plan, key, name=None, absent=False): how a test pins that a plan is NOT a scan. """ label = name or f"plan {'lacks' if absent else 'carries'} {key!r}" + nodes = list(_plan_nodes(plan)) + + # A PLAN THAT DID NOT ARRIVE LOOKS EXACTLY LIKE ONE THAT LACKS THE NODE. + # With `absent=True` that is a pass: the claim "nothing here carries the + # marker" is satisfied by there being nothing here. Measured before this + # guard: `expect.plan_marker([], "Columnar Projected Columns", + # absent=True)` gave `1 passed`, exit 0. + # + # That is the worst place in this layer for a silent pass. `absent=True` + # is how the vector-aggregate trap is pinned, and test_connection.py uses + # plan_marker as the PREMISE that the aggregate engaged -- a premise that + # cannot fail turns its test into one about an ordinary plan. + # + # Refused for the present arm too, and deliberately: an empty plan means + # the EXPLAIN did not arrive, so neither question can be answered. The + # present arm would fail anyway, but it would fail with "no node carries + # it, Columnar keys present: []", which diagnoses the wrong thing. + if not nodes: + raise VacuityError( + f"{label}: the plan has no nodes, so nothing here could carry " + f"or lack {key!r}. An EXPLAIN that did not arrive is not an " + f"answer to either question." + ) + found, seen = False, set() - for node in _plan_nodes(plan): + for node in nodes: seen.update(k for k in node if k.startswith("Columnar")) if key in node: found = True diff --git a/test/pytest/test_guards_pinned.py b/test/pytest/test_guards_pinned.py index 172d34a7..981d498d 100644 --- a/test/pytest/test_guards_pinned.py +++ b/test/pytest/test_guards_pinned.py @@ -169,3 +169,95 @@ def assert_outcomes(self, **k): with _pytest.raises(VacuityError): expect.refusal(_R(), "no patterns given") expect.num(1, 1, "refusal() with no pattern is itself refused") + + +# --------------------------------------------------------------------------- +# plan_marker, which @jdatcmd named as the one to fix first (#897 review): +# "both of its arms can be deleted independently with the suite green. Under +# one of those mutations the premise can never fail, so the provider-trap test +# would silently be about an ordinary plan." +# +# He was right, and it is the worst place in the layer for it to be true. +# `plan_marker` is the faithful port of `pgc_is_columnar_scan`; test_connection +# calls it three times, once as the PREMISE that the vector aggregate engaged. +# A premise that cannot fail turns the provider-trap test into a test about an +# ordinary plan while it stays green. +# +# These arms are behavioural rather than refusals -- plan_marker's two arms +# raise AssertionError, not VacuityError, so `expect.refusal` does not apply and +# `expect.outcomes` is the right instrument. Their value is the removal proof in +# the commit message, not their own green. + + +def test_plan_marker_present_arm_fails_when_the_key_is_absent(pytester, expect): + """The arm that makes 'did the columnar scan run' answerable.""" + result = _inner(pytester, ''' + PLAN = [{"Plan": {"Node Type": "Seq Scan"}}] + + def test_missing_marker(expect): + expect.plan_marker(PLAN, "Columnar Projected Columns", name="scan ran") + ''') + expect.outcomes(result, "a plan without the key fails", failed=1, passed=0) + result.stdout.fnmatch_lines(["*no node carries it*"]) + + +def test_plan_marker_present_arm_passes_when_the_key_is_there(pytester, expect): + """Control. A guard that reddens on a real plan gets switched off.""" + result = _inner(pytester, ''' + PLAN = [{"Plan": {"Node Type": "Custom Scan", + "Columnar Projected Columns": 3}}] + + def test_marker_present(expect): + expect.plan_marker(PLAN, "Columnar Projected Columns", name="scan ran") + ''') + expect.outcomes(result, "a plan carrying the key passes", passed=1, failed=0) + + +def test_plan_marker_absent_arm_fails_when_the_key_is_present(pytester, expect): + """The other arm. `absent=True` is how the vector-aggregate trap is pinned, + so a mutation that makes it unable to fail would make that test vacuous.""" + result = _inner(pytester, ''' + PLAN = [{"Plan": {"Node Type": "Custom Scan", + "Columnar Projected Columns": 3}}] + + def test_marker_should_be_absent(expect): + expect.plan_marker(PLAN, "Columnar Projected Columns", + name="aggregate absorbed the scan", absent=True) + ''') + expect.outcomes(result, "an absence claim over a present key fails", + failed=1, passed=0) + result.stdout.fnmatch_lines(["*the key is present and should not be*"]) + + +def test_plan_marker_absent_arm_passes_on_a_plan_that_lacks_the_key(pytester, expect): + """Control for the arm above, on a plan that really did arrive.""" + result = _inner(pytester, ''' + PLAN = [{"Plan": {"Node Type": "Custom Scan", + "Columnar Vectorized Aggregates": 1}}] + + def test_marker_absent(expect): + expect.plan_marker(PLAN, "Columnar Projected Columns", + name="aggregate absorbed the scan", absent=True) + ''') + expect.outcomes(result, "an absence claim over a real plan passes", + passed=1, failed=0) + + +def test_plan_marker_refuses_an_absence_claim_over_an_empty_plan(pytester, expect): + """The hole underneath both arms: an absence claim is satisfied by nothing + being there at all. + + A plan that failed to arrive looks exactly like a plan that legitimately + lacks the node, and `absent=True` cannot tell them apart -- so the arm that + pins the vector-aggregate trap would pass against `[]`. Measured before the + fix: `1 passed`, exit 0. + + This is a REFUSAL rather than an assertion, because the input is degenerate + rather than wrong, which is the same distinction `rows()` makes for two + empty sides. + """ + expect.refusal(_inner(pytester, ''' + def test_absent_on_nothing(expect): + expect.plan_marker([], "Columnar Projected Columns", absent=True) + '''), "plan_marker refuses an absence claim over an empty plan", + "plan has no nodes") diff --git a/test/selftest/370-the-plan-marker-guard-must.sh b/test/selftest/370-the-plan-marker-guard-must.sh new file mode 100644 index 00000000..072dd8b4 --- /dev/null +++ b/test/selftest/370-the-plan-marker-guard-must.sh @@ -0,0 +1,90 @@ +# ---- plan_marker must have all three of its guards --------------------------- +# +# WHY THIS EXISTS. @jdatcmd's #897 review named `plan_marker` as the guard to fix +# first: "both of its arms can be deleted independently with the suite green. +# Under one of those mutations the premise can never fail, so the provider-trap +# test would silently be about an ordinary plan." +# +# He was right, and it is the worst place in the layer for it to be true. +# plan_marker is the faithful port of pgc_is_columnar_scan; test_connection.py +# calls it three times, once as the PREMISE that the vectorized aggregate +# engaged. A premise that cannot fail turns its test into a test about an +# ordinary plan, and the test stays green while it happens. +# +# A THIRD HOLE SAT UNDER BOTH ARMS. An absence claim is satisfied by nothing +# being there at all: `plan_marker([], key, absent=True)` gave `1 passed`, exit 0, +# because a plan that never arrived looks exactly like a plan that legitimately +# lacks the node. That is now a refusal. +# +# WHY THE CHECKS ARE STATIC, same division as selftest 360. The behaviour is +# pinned by five arms in test/pytest/test_guards_pinned.py that run pytest inside +# pytest; those need pytest, psycopg and a virtualenv, and CI installs none of +# them. This half asserts the STRUCTURE they rest on, greppable from a checkout +# with nothing installed. + +_pm_vac="$PGC_TESTDIR/pytest/pgc_vacuity.py" + +check "premise: the pytest layer is where this part thinks it is" \ + "$([ -f "$_pm_vac" ] && echo yes || echo no)" "yes" + +# The function body, cut out once so every arm below reads the same text. A +# grep over the WHOLE file would match these shapes wherever they occur and +# report a guard present that lives in another method. +_pm_body="$(awk '/^ def plan_marker\(/{f=1} f&&/^ def /&&!/plan_marker/{exit} f' "$_pm_vac")" + +check "premise: plan_marker's body was actually cut out of the file" \ + "$([ "$(printf '%s\n' "$_pm_body" | wc -l)" -ge 20 ] && echo yes || echo "too short")" "yes" + +# The present arm: asked "does this plan carry the marker", a plan that does not +# must fail. +check "plan_marker keeps the arm that fails when the key is absent" \ + "$(printf '%s\n' "$_pm_body" | grep -c 'if not absent and not found:')" "1" + +# The absent arm: `absent=True` is how a test pins that a plan is NOT a scan. +check "plan_marker keeps the arm that fails when the key is present" \ + "$(printf '%s\n' "$_pm_body" | grep -c 'if absent and found:')" "1" + +# And the refusal under both of them. +check "plan_marker refuses a plan with no nodes at all" \ + "$(printf '%s\n' "$_pm_body" | grep -c 'if not nodes:')" "1" + +check "and that refusal is a VacuityError, not an ordinary assertion" \ + "$(printf '%s\n' "$_pm_body" | grep -A2 'if not nodes:' | grep -c 'raise VacuityError')" "1" + +# The refusal has to come BEFORE the walk that sets `found`, or it is dead code: +# an empty plan leaves found=False and the absent arm returns a pass first. +_pm_ln_empty="$(printf '%s\n' "$_pm_body" | grep -n 'if not nodes:' | cut -d: -f1)" +_pm_ln_absent="$(printf '%s\n' "$_pm_body" | grep -n 'if absent and found:' | cut -d: -f1)" +check "premise: both line numbers were found, so the ordering arm can mean something" \ + "$([ -n "$_pm_ln_empty" ] && [ -n "$_pm_ln_absent" ] && echo yes || echo no)" "yes" + +check "the empty-plan refusal precedes the arm it protects" \ + "$([ "$_pm_ln_empty" -lt "$_pm_ln_absent" ] && echo before || echo "AFTER, so it is dead code")" \ + "before" + +# ---- and these greps must be able to FAIL ----------------------------------- +# +# Every arm above passes on a healthy tree, which a grep that matches nothing +# also does. Each fixture is the real shape with ONE property removed. + +_pm_fix="$PGC_WORKDIR/planmarker"; rm -rf "$_pm_fix"; mkdir -p "$_pm_fix" + +printf ' if False and not absent and not found:\n' > "$_pm_fix/present.py" +check "a neutered present arm is caught" \ + "$(grep -c 'if not absent and not found:' "$_pm_fix/present.py")" "0" + +printf ' if False and absent and found:\n' > "$_pm_fix/absent.py" +check "a neutered absent arm is caught" \ + "$(grep -c 'if absent and found:' "$_pm_fix/absent.py")" "0" + +printf ' if False and not nodes:\n' > "$_pm_fix/empty.py" +check "a neutered empty-plan refusal is caught" \ + "$(grep -c 'if not nodes:' "$_pm_fix/empty.py")" "0" + +# The mirror of the three above: the same greps on the REAL body return 1, so a +# zero is a missing guard rather than a broken pattern. +check "premise: while the real body satisfies all three, so the greps work" \ + "$(printf '%s\n' "$_pm_body" | grep -cE 'if not absent and not found:|if absent and found:|if not nodes:')" \ + "3" + +unset _pm_vac _pm_body _pm_fix _pm_ln_empty _pm_ln_absent