diff --git a/CHANGELOG.md b/CHANGELOG.md index 50adede9..25cadeb0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -194,6 +194,87 @@ 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: + 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 + 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 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. + + **`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 + 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. + + **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 + 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` + 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. diff --git a/design/ISSUE_432_PYTEST_HARNESS.md b/design/ISSUE_432_PYTEST_HARNESS.md new file mode 100644 index 00000000..ffb08c67 --- /dev/null +++ b/design/ISSUE_432_PYTEST_HARNESS.md @@ -0,0 +1,456 @@ +# 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 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 + +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 + +`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: + +``` +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 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. + +`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 + +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/ 25 tests serial: 25 passed xdist -n 4: 25 passed +``` + +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 +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. + +### 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 +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. + +## 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 | +|---|---| +| 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/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/README.md b/test/pytest/README.md new file mode 100644 index 00000000..b5c13307 --- /dev/null +++ b/test/pytest/README.md @@ -0,0 +1,59 @@ +# Running the pytest harness + +This is the issue #432 pilot. It runs beside `test/*.sh`, and replaces nothing. + +- `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 + +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. + +## 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 +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/TESTS.md b/test/pytest/TESTS.md new file mode 100644 index 00000000..b0660e67 --- /dev/null +++ b/test/pytest/TESTS.md @@ -0,0 +1,565 @@ +# 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. + +**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. + +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. + +## 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_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 + +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 | +| `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 +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 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. + +| 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 | +| `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 | — | + +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 +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. + +### 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 +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_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" +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 | +| --- | --- | +| `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. + +## 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. + +| 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. + +## 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. +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. +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. + +## 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. + +**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/compare_to_bash.py b/test/pytest/compare_to_bash.py new file mode 100755 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..987fafda --- /dev/null +++ b/test/pytest/conftest.py @@ -0,0 +1,109 @@ +"""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 re +import pathlib +import shutil + +import psycopg +import pytest + +from pgc_cluster import _pg_config, build_once, 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)", + ) + + +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.""" + 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"-- cluster: worker={worker_id} port={cluster.port} " + f"{cluster.version} .so={cluster.so_md5()}") + # 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") + 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..604bc725 --- /dev/null +++ b/test/pytest/pgc_cluster.py @@ -0,0 +1,479 @@ +"""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 fcntl +import hashlib +import os +import re +import pathlib +import shutil +import socket +import subprocess +import tempfile + +# 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: + """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.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): + 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}" + + @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() + + +_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() + + +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 = pick_port(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..af398d4e --- /dev/null +++ b/test/pytest/pgc_vacuity.py @@ -0,0 +1,501 @@ +"""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 = {} + +# 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.""" + + +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) + + +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.""" + + 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, 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( + "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}" + + seen_types, seen_providers = [], [] + for node in _plan_nodes(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 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. + + 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." + ) + + 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}" + 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 nodes: + 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. + + 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}" + ) + 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 + + +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", + 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_build_refusal.py b/test/pytest/test_build_refusal.py new file mode 100644 index 00000000..7f1df25f --- /dev/null +++ b/test/pytest/test_build_refusal.py @@ -0,0 +1,243 @@ +"""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") + + +# --------------------------------------------------------------------------- +# 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") diff --git a/test/pytest/test_connection.py b/test/pytest/test_connection.py new file mode 100644 index 00000000..2d5a3dd5 --- /dev/null +++ b/test/pytest/test_connection.py @@ -0,0 +1,186 @@ +"""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 +import pathlib + + +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 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 [], [want], + f"the loaded extension is this tree's {want}") + + +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 _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") + 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_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): + """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 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 aux_band, read_ephemeral_floor + + floor = read_ephemeral_floor() + lo, hi = aux_band(floor) + wid = pgc_cluster.worker_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") + + +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_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/pytest/test_guards_pinned.py b/test/pytest/test_guards_pinned.py new file mode 100644 index 00000000..981d498d --- /dev/null +++ b/test/pytest/test_guards_pinned.py @@ -0,0 +1,263 @@ +"""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") + + +# --------------------------------------------------------------------------- +# 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/pytest/test_layer.py b/test/pytest/test_layer.py new file mode 100644 index 00000000..831bd15c --- /dev/null +++ b/test/pytest/test_layer.py @@ -0,0 +1,256 @@ +"""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") + + +# --------------------------------------------------------------------------- +# 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/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") 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 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..81effe10 --- /dev/null +++ b/test/selftest/360-an-unrunnable-pytest-test-must.sh @@ -0,0 +1,151 @@ +# ---- 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. +# +# 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" \ + "$([ "$_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 +# 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" + +# 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 _ts_assigns 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