test: a pytest harness with a vacuity-refusal layer, 25 tests (#432) - #897
Open
OffgridwithJD wants to merge 4 commits into
Open
test: a pytest harness with a vacuity-refusal layer, 25 tests (#432)#897OffgridwithJD wants to merge 4 commits into
OffgridwithJD wants to merge 4 commits into
Conversation
…prompt#432) Issue commandprompt#432 asks whether to move the suite from bash to pytest. This is the pilot the discussion on that issue asked for: a working harness, two suites' worth of machinery, and one suite ported and proved against its bash original. It replaces nothing. test/run_all_versions.sh remains the gate and no bash suite is deleted. The port is not the hard part. The vacuity guard is. A vacuity defect is a test that reports PASS while asserting nothing. Bare pytest permits it in eight ways, all measured on pytest 9.1.1 and all exiting 0: a body with no assert, empty compared with empty, parametrize over an empty list, every test skipped, a non-strict xfail that passes, a test that returns instead of asserting, pytest.raises(Exception) satisfied by an unrelated error, and a substring match where a typed field was meant. The bash harness defends against this class in several places, earned one defect at a time. pytest defends nowhere. So the layer comes first, and it is structural rather than advisory: - a test passes only if it made a counted assertion, which also kills the return-instead-of-assert mode - comparisons refuse their degenerate cases: both sides empty, a value against itself, an empty expectation, a floor of zero - plan assertions read Custom Plan Provider from EXPLAIN (FORMAT JSON) by exact equality, because "ColumnarScan" in plan is as wrong as grep ColumnarScan when the real provider is PgColumnarScan - a bare skip fails the run; skipping needs a reason from the closed list lib.sh already uses - the run asserts its own collected count, so a filtered run cannot be green - the cluster identity check refuses a server whose data_directory is not ours, with a negative control proving it can say no Seventeen of the 24 tests are the layer testing itself, run through pytester so each guard is proven to refuse rather than assumed to. The layer's own tests obey the layer: an exemption for the tests that prove the guard is the first step to exempting everything else. Direct connections, per the requirement. psycopg 3 returns int, Decimal, float, bytes, list and None where psql -At returns text and leaves every conversion to the reader. EXPLAIN (FORMAT JSON) arrives already parsed. The operations that still run a binary are initdb, pg_ctl, pg_dump and pg_restore, and the build. Terminating a backend is pg_terminate_backend over SQL, not a killed psql, because killing the client leaves the backend running its statement. Parallelism: one cluster per xdist worker, on a port derived from the worker id below the ephemeral floor, with a private schema per test. Four workers run the suite in 2.1s. Not -n auto: eight cores are shared with a desktop. The differential is what makes the port credible. test/native_projection.sh and its port, each arm building once so both harnesses measure the same library: ARM A unmutated .so 8370e9b1beba bash 8/0 pytest 7/0 ARM B fan-out neutered .so 9e9510593777 bash 0/8 pytest 0/7 Both go red together. compare_to_bash.py then diffs the two by assertion NAME: every bash property is covered, plus one the bash suite lacks, a premise that the DELETE removed rows. Without it both delete arms are satisfied by a projection that never changed. Measured on the leak question that opened the issue: after two gate matrix runs this container held four orphaned postmasters from /tmp/pgcolumnar-test.* datadirs, aged 18 to 35 minutes. The pytest harness left zero. design/ISSUE_432_PYTEST_HARNESS.md carries the measurements, the psql-exception list with a reason for each, the ordered red tests, what is out of scope, and a mandatory section on what the layer still cannot catch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
…prompt#432) Two things, and the second is a defect in the harness the first commit landed. THE PLAN PREDICATE WAS WIDER THAN THE HELPER IT PORTED. The harness asserted `Custom Plan Provider == "PgColumnarScan"` and called that the fix for the substring class, on the grounds that equality on a typed field cannot be satisfied by a superstring the way `grep ColumnarScan` was by `PgColumnarScan`. Equality was the right idea about the wrong field. Measured on 18.4: plain scan Custom Scan provider='PgColumnarScan' Projected Columns present ungrouped vector agg Custom Scan provider='PgColumnarScan' Projected Columns ABSENT grouped vector agg Custom Scan provider='PgColumnarScan' Projected Columns ABSENT columnar_vector.c:806 assigns the vectorized aggregate node &pgcolumnar_scan_methods, whose CustomName is PgColumnarScan (columnar_customscan.c:167). So every pgcolumnar node reports that provider. With the aggregate engaged the plan is a single node, the aggregate has absorbed the scan, and the predicate still claims a scan is present. pgc_is_columnar_scan answers no there, because it greps for `Columnar Projected Columns`, which only the scan's callback emits (columnar_customscan.c:3631). So expect.plan_marker() is added as the faithful port, expect.plan_node() keeps the provider question and now says in its docstring what that question is not, and test_the_provider_name_does_not_identify_a_scan pins all three facts so a revert reddens rather than passing quietly. PgColumnarAgg is noted as unreachable in a plan: it is the CustomName of a CustomPathMethods, and EXPLAIN prints the scan methods' name. This is the document's own argument in one example. Typed results removed the parsing accident and left the harder mistake untouched. The assertion was type-correct, exact, and about the wrong thing. DOCUMENTATION. test/pytest/TESTS.md, a per-test reference: every test with the property it asserts and the measured fact behind it, every assertion helper with what it REFUSES, the four controls that exist to catch a guard turning into a nuisance, the rules for adding a test, and the traps this corpus records. Each of its numbers was checked against the tree rather than written from memory. design/ISSUE_432_PYTEST_HARNESS.md section 5.4 is rewritten, because it recommended the predicate this commit removes. The test counts there are corrected and the harness's own defect is recorded beside the other instrument defects. 25 tests, green serially and under four xdist workers, with the collected count asserted at 25. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
… costs An unregistered test suite rots, so this needs a decision rather than a default. The decision has a measured price and the price was not what I assumed. pgc_skip in lib.sh DOES NOT SKIP. It increments the failure count and prints "A missing dependency is an environment defect, not a pass. Install it, or set PGC_ALLOW_MISSING_<CAP>=1 to run knowingly without this coverage." That is a deliberate house rule and the right one. It also means a wrapper registered in SUITES cannot quietly stand aside on a machine without pytest. CI has no pytest, no psycopg and no xdist, so registering the run today makes every CI job red until ci.yml installs from test/pytest/requirements-test.txt. Registering is therefore one line in SUITES plus a real change to the gate every other PR depends on, and that belongs to whoever owns the gate rather than to the pilot that wants it. PGC_ALLOW_MISSING_PYTEST=1 in CI is worse than either option. It reads as coverage and provides none, which is the vacuity defect this whole document exists to prevent, one level up. So the design document now carries section 1a with that reasoning, the scope list names the omission explicitly rather than leaving it implied, and the README says plainly that nothing gates these tests yet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
…uires
Found by gating the branch rather than by assuming python files are inert. The bash
matrix failed on both majors:
FAIL every script that declares an interpreter is executable:
got [[1: test/pytest/compare_to_bash.py]] want [[]]
harness_selftest.sh: FAILED
test/selftest/300-a-test-script-must-be-runnable.sh requires that any file declaring
an interpreter be executable, and the file was mode 644 with a
#!/usr/bin/env python3 line. Mode is now 755 and recorded as 100755 in the index.
harness_selftest is 261 of 261 with the fix. Nothing else in the matrix objected to
the new directory: 244 of 246 on PG18 and 246 of 246 on PG19, with only the two
expected PG19-only skips.
The second failure in my first reproduction, "the installed .so is the one this run
built ... built <none>", was an artifact of running the selftest under
PGC_SKIP_BUILD=1. It passes when a build actually runs, so it was my probe rather
than the branch, and it is worth naming because that check exists precisely to catch
a suite reporting against a binary it did not build.
TESTS.md now tells the next person that this directory inherits every rule the older
ones follow, and names this one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
Collaborator
Author
|
🤖 Generated with Claude Code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First of three for #432. This is the foundation: a cluster fixture, a direct
psycopgconnection, the vacuity-refusal layer that every later test depends on, and 25 tests.The two follow-ups are ready and held back deliberately, because each is only reviewable once this one is agreed:
Why a second harness at all, stated honestly
The bash suites are not being replaced and this does not try to.
test/carries 4,429 anchored assertions across 256 suites; this PR ports one of them. That is 0.18%. Nobody should read this as coverage.What it buys is the thing bash cannot do cheaply: a typed result.
psql -Atreturns text, so a bash oracle compares strings and anint41and atext'1'are the same value to it.psycopgreturnsint,Decimal,float,bytes,list,None, and a test can assert the type as well as the value. Per #432 this uses a direct connection everywhere and shells out topsqlonly where there is no alternative — currently nowhere in these 25 tests.The layer is the point, not the tests
A pytest suite fails open. A test that asserts nothing passes; a filter that selects nothing exits 0; a fixture that skips greens every test under it. This project has already shipped a vacuity defect, so the harness refuses those shapes rather than documenting them.
pgc_vacuity.pyis loaded for every run viapytest.ini, and it refuses:cursor.rowcountof-1, which is a number and truthyexcept, found by walking the AST rather than by line regexskip, andxfail_strict = trueso an xpass is not silently green--pgc-expect-tests N, which asserts the run's own shape, and refusesN = 0Every one of those has a red test in
test_layer.pythat runs pytest inside pytest through thepytesterfixture and asserts on the inner run's outcome. That is what proves a guard refuses rather than assuming it. Each row of the table inTESTS.mdalso records the bare-pytest behaviour it exists to stop, measured: every one of those measurements exited 0.Four of the ten layer tests are positive controls, deliberately. A guard with a bad false-positive rate gets switched off, and then whatever it replaced is gone too.
The escape hatches are all more expensive to type than the honest form:
allow_emptytakes a reason, notTrue;--pgc-expect-teststakes the real number;cannot_runtakes a reason from a closed list. None can become the default by being shorter.The port is proved against the bash suite it came from
test_native_projection.pyportstest/native_projection.sh.compare_to_bash.pyruns both and compares the property names each asserts, not the counts — agrep -c " PASSED"reported 6 of 7 because the first test's outcome shares a line with a fixture's print, and a count that is wrong for that reason looks exactly like a count that is right.Not in the gate, and why
test/run_all_versions.shdoes not run this. Registering it would add apsycopgbuild dependency to every CI leg for 0.18% of the assertions, and the point of these 25 tests today is the layer, not the coverage.README.mdsays what registering would cost and what has to be true before it is worth it.Verification
At
c8b2a9e, rebased ontoedd729e:Exit codes read without a pipe, because a pipeline reports the exit status of its last stage and that has already produced one wrong verdict in this project.
I also derived which existing suites this branch can affect rather than guessing. Nothing in
test/namestest/pytest/or the design document, so the reachable surface is the harness's own accounting and the docs checks:harness_selftestis the one that matters here:compare_to_bash.pyis recorded100755, and it failed that suite on both majors as100644with a shebang.The rebase onto
edd729ewas verified not to have touched anything in this PR —git diff c02cfc4 HEAD -- test/pytest designis empty — and the 25 were re-run at the rebased tip rather than inherited from the pre-rebase run.Reviewing this
The highest-value thing to attack is
test_layer.py. If any guard there can be made to pass with the guard removed, the layer is decoration. I have run each one that way; a second pair of hands is worth more than my own repetition.