From 5467382ad88a620ff207297d16e4532a28965858 Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Thu, 10 Sep 2026 20:39:31 +0000 Subject: [PATCH 1/3] test/pytest: a write that wrote no rows is not a fixture (#432) `INSERT ... SELECT ... WHERE false` writes nothing and raises nothing. psycopg reports `INSERT 0 0` with a `rowcount` of 0, and nothing in the corpus read either field -- so the fixture a test meant to build did not exist, and every assertion below it compared two empty things. That is `insert-wrote-no-rows`, entry 2 on VACUITY_MODES.md's own list of what to add next. Measured before building: 12 write statements across 5 files, and `rowcount` read at exactly one site, which does not assert on it. THE COMMAND TAG DECIDES, NOT THE ROW COUNT. `SELECT 0` and `INSERT 0 0` both carry `rowcount == 0`, so a guard keyed on the count alone would refuse every test whose last statement was a SELECT over an empty result -- a legitimate and common assertion. `statusmessage` is the server's own command tag, so this guard never parses SQL. Measured on PG 18 against a pgcolumnar table: statusmessage rowcount statement CREATE TABLE -1 CREATE TABLE t (i int) USING pgcolumnar INSERT 0 5 5 INSERT INTO t SELECT g FROM generate_series(1,5) g INSERT 0 0 0 INSERT ... WHERE false UPDATE 0 0 UPDATE t SET i = i WHERE i > 100 DELETE 0 0 DELETE FROM t WHERE i > 100 SELECT 0 0 SELECT * FROM t WHERE false SET -1 SET search_path TO public TRUNCATE TABLE -1 TRUNCATE t A DELIBERATE ZERO STAYS WRITABLE. A DELETE that must match nothing is a real negative control, so `expect.wrote(cur, 0, name)` both compares the count and marks the write as named. An unnamed zero fails the test; naming a count does not excuse a wrong one; and a `rowcount` of -1 is refused rather than compared, for the reason `expect.rowcount` already records. IN THE CALL PHASE, NOT A TEARDOWN. #931 measured that a guard run as a teardown fixture reports the test it guards as PASSED and fails separately, so a reader sees a green test beside an error. TWO PROPERTIES, TWO FILES, AND THE SPLIT IS MEASURED RATHER THAN ASSERTED. The classifier and the refusal live in test_writes_wrote_rows.py, which needs no database: a stub cursor carrying the two measured fields exercises them exactly. Whether the connection the tests actually use is watched is a different claim that no driver-free arm can make, and test_the_connection_the_tests_use_is_watched makes it through a real `INSERT ... WHERE false`, on both `conn.execute` and a cursor the connection handed out -- 24 and 42 sites in the corpus, so a proxy watching only the connection would leave most of it unwatched. Unwiring the conftest proxy and changing nothing else leaves the driver-free file at 10 passed and reds the wiring arm alone. That is #917's defect in miniature: its pytest twin tested the reconciler's body and left the runner's CALL to it uncovered, so removing the call kept the pytest half green at 9 passed while the shell half went red by one. PROVE BY REMOVAL, five mutations, each applied by exact string match with the file asserted to still parse: control free 10 passed wiring 9 passed no refusal free 2 failed wiring 9 passed command tag ignored free 2 failed wiring 1 failed every statement is a write free 4 failed wiring 1 failed connection not wrapped free 10 PASSED wiring 1 failed acknowledgement not recorded free 1 failed wiring 1 failed FALSE-POSITIVE BUDGET FIRST, because a guard that reddens a legitimate write is worse than the mode: the full corpus with a cluster is 249 passed, and none of the 12 write sites already in the corpus reddened. TWO OF MY OWN ARMS WERE WRONG, recorded because each produced a green that meant nothing: * An arm asserting only `failed=1` passed before the feature existed. The inner test called a function not yet written, got an AttributeError, and the outer arm read that as the comparison failing. Naming the numbers is what makes the red the right red. * A multi-word pattern can straddle pytest's word wrap. `expect.refusal` anchors each pattern to one `E` line and pytest wraps a long traceback line, so matching "wrote no rows" failed against a message containing it -- which reads exactly like "the guard did not fire". The refusal now leads with the mode's own kebab-case id, which is one token and cannot be split. Docs carried in the same commit, per jd's rule. VACUITY_MODES.md moves the mode to section 2 and keeps a back-reference where it was, and its stated totals go 27 -> 28 refused and 45 -> 44 not refused -- checked by the corpus's own arms, which is how I learned the numbers rather than deciding them. TESTS.md gains section 22, and the paragraph in section 18 that listed this gap is rewritten rather than deleted, because a reader who knew the gap needs to find out where it went. Gate on this tree: harness_selftest 588 checks, 588 passed + 0 failed + 0 unrunnable, PASSED driver-free job 10 files, 171 passed, psycopg absent from the venv full corpus 249 passed with a cluster on pg18a Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- CHANGELOG.md | 49 +++++++ test/pytest/README.md | 2 +- test/pytest/TESTS.md | 100 ++++++++++++- test/pytest/VACUITY_MODES.md | 37 ++++- test/pytest/conftest.py | 8 +- test/pytest/pgc_vacuity.py | 174 ++++++++++++++++++++++ test/pytest/test_connection.py | 42 ++++++ test/pytest/test_harness_deps.py | 1 + test/pytest/test_writes_wrote_rows.py | 201 ++++++++++++++++++++++++++ 9 files changed, 598 insertions(+), 16 deletions(-) create mode 100644 test/pytest/test_writes_wrote_rows.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 21623a3f..8a4ab100 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,55 @@ true until the next version shipped. ### Added +- A write that wrote no rows no longer passes as a fixture that built something + (#432). + + `INSERT ... SELECT ... WHERE false` writes nothing and raises nothing. psycopg + reports `INSERT 0 0` with a `rowcount` of 0, and nobody in the pytest corpus read + either field -- so the fixture a test meant to build did not exist, and every + assertion below it compared two empty things. That is `insert-wrote-no-rows` in + `test/pytest/VACUITY_MODES.md`, which now lists it as refused rather than as a gap. + + THE COMMAND TAG DECIDES, NOT THE ROW COUNT. `SELECT 0` and `INSERT 0 0` both carry + `rowcount == 0`, so a guard keyed on the count alone would refuse every test whose + last statement was a SELECT over an empty result. `statusmessage` is the server's + own tag, so this guard never parses SQL. Measured on PG 18 against a pgcolumnar + table: DDL reports `CREATE TABLE`, `SET` or `TRUNCATE TABLE` with `rowcount` -1, an + `INSERT ... WHERE false` reports `INSERT 0 0` with 0, and `UPDATE 0` and `DELETE 0` + likewise. + + A deliberate zero stays writable. A DELETE that must match nothing is a real + negative control, and `expect.wrote(cur, 0, name)` says so: it compares the count + and marks the write as named. An unnamed zero fails the test, and naming a count + does not excuse a wrong one. The refusal runs in the CALL phase rather than a + teardown fixture, because #931 measured that a teardown guard reports the test it + guards as PASSED and fails separately. + + TWO PROPERTIES, TWO FILES, PROVEN SEPARATELY. The classifier and the refusal live + in `test/pytest/test_writes_wrote_rows.py`, which needs no database: a stub cursor + carrying the two measured fields exercises them exactly. Whether the connection the + tests actually use is watched is a different claim that no driver-free arm can make, + and `test_the_connection_the_tests_use_is_watched` makes it through a real + `INSERT ... WHERE false`, on both `conn.execute` and a cursor the connection handed + out -- 24 sites and 42 sites in the corpus respectively, so a proxy watching only + the connection would leave most of it unwatched. + + The split is load-bearing, measured rather than asserted. Unwiring the connection + proxy in `conftest.py` and changing nothing else leaves the driver-free file at 10 + passed and reds the wiring arm alone. That is #917's defect in miniature -- its + pytest twin tested a function's body and left the runner's CALL to it uncovered, so + removing the call kept that half green -- and it is why these arms are in two files. + + Prove-by-removal, five mutations, each applied by exact string match and the file + asserted to still parse: no refusal -> 2 arms red; the command tag ignored -> 2 red + and the wiring arm red; every statement treated as a write -> 4 red; the connection + not wrapped -> 0 driver-free red and the wiring arm red; the acknowledgement not + recorded -> 1 red. Control: 10 passed driver-free, 9 passed with a cluster. + + Full corpus with a cluster: 246 passed, and none of the 12 write sites already in + the corpus reddened -- the false-positive budget this guard needed before it could + ship. + - A broad `pytest.raises` must name a SQLSTATE, and the block must hold one statement (#432). diff --git a/test/pytest/README.md b/test/pytest/README.md index 05db5b6d..27657f52 100644 --- a/test/pytest/README.md +++ b/test/pytest/README.md @@ -5,7 +5,7 @@ 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. - `VACUITY_MODES.md` is the inventory of ways a pytest harness can report a false pass: 79 modes produced by the enumeration, 72 of them named in that - file, 73 demonstrated by a run, and 27 refused by this layer today. + file, 73 demonstrated by a run, and 28 refused by this layer today. VACUITY_MODES.md section 1a gives the counting rule and reconciles the run's totals against what is actually written down. - `design/ISSUE_432_PYTEST_HARNESS.md` holds the design and the measurements diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index aeee6636..340ac5e8 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -67,6 +67,7 @@ behaviour, the source of that number is named. - [19. Traps this corpus records](#19-traps-this-corpus-records) - [20. test_raises_sqlstate.py: which error, and which statement](#20-test_raises_sqlstatepy-which-error-and-which-statement) - [21. test_failed_query_sentinel.py: a failed query is not a comparison](#21-test_failed_query_sentinelpy-a-failed-query-is-not-a-comparison) +- [22. test_writes_wrote_rows.py: a write that wrote nothing](#22-test_writes_wrote_rowspy-a-write-that-wrote-nothing) ## 1. How to read a test in here @@ -779,6 +780,7 @@ written. | `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 | +| `test_the_connection_the_tests_use_is_watched` | writes through `pgc_conn` reach the zero-row guard, on both the connection and a handed-out cursor | Two of these deserve their reasoning stated. @@ -1375,15 +1377,19 @@ over tests nothing ran. ## 18. What this corpus does NOT yet refuse `VACUITY_MODES.md` is the inventory: 79 ways a pytest harness can report a pass while -asserting nothing, 73 of them demonstrated by an actual run. **This layer refuses 27 -of them.** The other 45, of which 44 were demonstrated, are listed there with the +asserting nothing, 73 of them demonstrated by an actual run. **This layer refuses 28 +of them.** The other 44, of which 43 were demonstrated, are listed there with the refusal design each would need and the order worth building them in. -Read it before adding a test. Two gaps are most likely to affect a new test now. +Read it before adding a test. One gap is most likely to affect a new test now. -**A write is not required to have written anything.** `INSERT ... SELECT ... WHERE -false` writes nothing, raises nothing, and leaves a `rowcount` of 0 that nobody -reads. +**`insert-wrote-no-rows` closed, and this paragraph used to be the gap.** It said a +write was not required to have written anything, which stopped being true when +section 22 landed: every write the test connection runs is recorded from the server's +command tag, and a test that ran one reporting 0 rows fails unless it named the zero. +The sentence is rewritten rather than deleted because a reader who knew the gap needs +to find out where it went -- the same reason `VACUITY_MODES.md` keeps a +back-reference for every mode that moves. **A `pytest.raises` block can still catch a failure from its own setup.** Section 17 closed `raises-too-broad` — a broad family with no SQLSTATE pinned does not collect — @@ -1611,3 +1617,85 @@ The cache matters: the five deleted lines are byte-identical, so three of the fi mutations leave the file the same size and Python reuses the stale bytecode. Without `rm -rf __pycache__` between runs, mutations 3, 4 and 5 report the same failure and the table reads as though two refusals did not bite. + +## 22. test_writes_wrote_rows.py: a write that wrote nothing + +`INSERT ... SELECT ... WHERE false` writes no rows and raises nothing. The fixture +it was supposed to build does not exist, and every assertion below it then compares +two empty things. That is `insert-wrote-no-rows` in `VACUITY_MODES.md` section 3.5, +and before this guard nothing in the corpus read either the count or the command. + +**The tag decides, not the row count.** `SELECT 0` and `INSERT 0 0` both carry +`rowcount == 0`, so a guard keyed on the count alone would refuse every test whose +last statement was a SELECT over an empty result — a legitimate and common +assertion. `statusmessage` is the server's own command tag, so this guard never +parses SQL. Measured on PG 18 against a pgcolumnar table: + +``` +statusmessage rowcount statement +CREATE TABLE -1 CREATE TABLE t (i int) USING pgcolumnar +INSERT 0 5 5 INSERT INTO t SELECT g FROM generate_series(1,5) g +INSERT 0 0 0 INSERT ... WHERE false +UPDATE 0 0 UPDATE t SET i = i WHERE i > 100 +DELETE 0 0 DELETE FROM t WHERE i > 100 +SELECT 0 0 SELECT * FROM t WHERE false +SET -1 SET search_path TO public +TRUNCATE TABLE -1 TRUNCATE t +``` + +**A deliberate zero stays writable.** A DELETE that must match nothing is a real +negative control, and `expect.wrote(cur, 0, name)` is how a test says so: it +compares the count and marks the write as named. An unnamed zero fails the test. +The acknowledgement is not a waiver — a wrong count still fails. + +**The refusal runs in the CALL phase, not a teardown.** #931 measured that a guard +run as a teardown fixture reports the test it guards as PASSED and fails +separately, so a reader sees a green test beside an error. + +| test | asserts | +| --- | --- | +| `test_a_write_that_wrote_nothing_is_recorded` | an `INSERT 0 0` is recorded, with its count and its tag | +| `test_update_and_delete_are_writes_too` | `UPDATE 0` and `DELETE 0` are writes, not only INSERT | +| `test_a_select_matching_nothing_is_not_a_write` | `SELECT 0` is not a write; **the arm a count-only guard fails** | +| `test_ddl_is_not_a_write` | `CREATE TABLE`, `SET`, `TRUNCATE TABLE`, `DROP SCHEMA` are not writes | +| `test_a_write_that_wrote_rows_needs_no_acknowledgement` | a write that moved rows is recorded and needs no naming | +| `test_an_unacknowledged_zero_row_write_fails_the_test` | the inner run fails, and the message names the mode and the command | +| `test_expect_wrote_acknowledges_the_zero` | naming the zero lets a negative control pass | +| `test_expect_wrote_refuses_a_count_that_is_not_a_count` | `rowcount == -1` is refused rather than compared | +| `test_expect_wrote_still_compares` | acknowledging a count does not excuse a wrong one | +| `test_several_writes_and_only_the_empty_one_is_named` | with three writes and one empty, the refusal names the empty one | + +### Two properties, two files, on purpose + +Every arm above runs with **no database**. A stub cursor carrying the two measured +fields exercises the classifier and the refusal exactly, which is the whole of what +those arms claim. + +It is not the whole of the guard. Whether the connection the tests actually use is +wrapped at all is a different claim, and no driver-free arm can make it: +`test_the_connection_the_tests_use_is_watched` in section 7 does, through a real +`INSERT ... WHERE false`, and through both `conn.execute` and a cursor the +connection handed out — because the corpus uses both, 24 sites and 42 sites, and a +proxy watching only the connection would leave most of the corpus unwatched. + +Splitting them is not tidiness. #917's pytest twin tested the reconciler's body and +left the runner's CALL to it uncovered: removing the call kept the pytest half at +9 passed while the shell half went red by one. Proving a function and proving its +call site are two proofs, and the second is the one that goes missing. + +### What made the arms themselves wrong twice + +Recorded because both produced a green that meant nothing. + +**An arm asserting only `failed=1` passed before the feature existed.** +`test_expect_wrote_still_compares` ran an inner test that called a function not yet +written, got an `AttributeError`, and reported a pass — satisfied by a failure that +had nothing to do with the comparison. Naming the numbers in the message is what +makes the red the right red. + +**A multi-word pattern can straddle pytest's word wrap.** `expect.refusal` anchors +each pattern to one `E` line, and pytest wraps a long traceback line. Matching +`wrote no rows` failed against a message that contained it, which reads exactly +like "the guard did not fire". The refusal now leads with the mode's own +kebab-case id, which is one token and cannot be split, and each arm matches one +token per call. diff --git a/test/pytest/VACUITY_MODES.md b/test/pytest/VACUITY_MODES.md index fbc98802..52696ac8 100644 --- a/test/pytest/VACUITY_MODES.md +++ b/test/pytest/VACUITY_MODES.md @@ -47,8 +47,8 @@ recollection of the run: | | modes | | --- | ---: | -| named in section 2, refused today | 27 | -| named in section 3, not refused | 45 | +| named in section 2, refused today | 28 | +| named in section 3, not refused | 44 | | **named in this document** | **72** | | produced by the enumeration run | 79 | | **named nowhere here** | **7** | @@ -65,7 +65,7 @@ an id can be read, argued with and turned into a test, and a number cannot. ## 2. What the layer refuses today -27 of the 79, counted by section 1a's rule. Each is enforced by a mechanism, not a convention, and each has a red +28 of the 79, counted by section 1a's rule. Each is enforced by a mechanism, not a convention, and each has a red test in `test_layer.py` that fails without it. | mechanism | modes it closes | @@ -88,6 +88,7 @@ test in `test_layer.py` that fails without it. | a skip during fixture setup fails the run | `session-fixture-skip-greens-the-whole-suite` | | a broad `pytest.raises` must pin a SQLSTATE, found by AST | `raises-too-broad` | | every comparison refuses a value carrying the `QUERY_ERROR` prefix, on either side | `error-swallowed-to-empty` | +| a write whose command tag reports 0 rows fails the test unless the zero is named | `insert-wrote-no-rows` | Three of those were added after checking this layer against the inventory rather than reasoning about it, and all three had passed silently before: @@ -120,7 +121,7 @@ guard whose subject is false greens has no business emitting a false red. ## 3. What it does not refuse -55 modes by the run's count, **45 of them named below**, **49 demonstrated by a run**. 51 have a refusal already designed. +55 modes by the run's count, **44 of them named below**, **48 demonstrated by a run**. 51 have a refusal already designed. Grouped by what a reader needs to decide about them. ### 3.1 The run can lose tests and still exit 0 @@ -219,8 +220,19 @@ binds the exception and the body pins its SQLSTATE. See section 2. ### 3.5 The fixture built the wrong situation -- `insert-wrote-no-rows` — `INSERT ... SELECT ... WHERE false` writes nothing and - raises nothing; `rowcount` is 0 and nobody reads it. +**`insert-wrote-no-rows` is now closed.** `INSERT ... SELECT ... WHERE false` writes + nothing and raises nothing, and before this nobody read either field. Every write the + test connection runs is now recorded from the server's own command tag, and a test + that ran one reporting 0 rows fails unless it named the zero with + `expect.wrote(cur, 0, ...)` — which is how a DELETE that must match nothing stays + writable. See section 2 and TESTS.md section 22. + + THE TAG DECIDES, NOT THE COUNT, and that is the whole design. `SELECT 0` and + `INSERT 0 0` both carry `rowcount == 0`, so a guard keyed on the count would refuse + every test whose last statement was a SELECT over an empty result. Measured on PG 18 + against a pgcolumnar table: DDL reports `CREATE TABLE`/`SET`/`TRUNCATE TABLE` with + `rowcount` `-1`, an `INSERT ... WHERE false` reports `INSERT 0 0` with 0, and + `UPDATE 0` and `DELETE 0` likewise. This guard therefore never parses SQL. - `mutation-arm-unobservable` — both arms of an A/B produce the identical answer and both are green. The assertion that would catch it, that the arms must **differ**, is the one nobody writes. @@ -311,7 +323,16 @@ Each entry names the red test to write first. 1. `test_expect_query_error_sentinel_is_unique_per_failure` — make something produce `QUERY_ERROR.`; the constant exists and nothing writes it. -2. `test_layer_requires_a_write_to_have_written` — closes `insert-wrote-no-rows`. +2. ~~`test_layer_requires_a_write_to_have_written` — closes `insert-wrote-no-rows`.~~ + **Done**, and in two files rather than one, because it is two properties. The + refusal and the tag-versus-count classification live in + `test_writes_wrote_rows.py`, which needs no database: a stub cursor carrying the + two measured fields exercises them exactly. Whether the connection the tests + actually use is watched at all is a different claim, and no driver-free arm can + make it — `test_the_connection_the_tests_use_is_watched` in `test_connection.py` + does, through a real `INSERT ... WHERE false`. Splitting them was not tidiness: + #917's pytest twin tested a function's body and left its CALL SITE uncovered, so + deleting the call kept that half green while the shell half went red. 3. `test_layer_requires_ab_arms_to_differ` — closes `mutation-arm-unobservable`. 4. ~~`test_raises_requires_a_sqlstate` — closes `raises-too-broad`.~~ **Done.** It closes `raises-too-broad` and narrows `raises-catches-setup`, which stays @@ -332,6 +353,6 @@ have tried to defeat them did not run. Every design states its own residual, and those residuals are the authors' own, unchallenged. So treat §2 as measured, §3 as measured, and §5 as a plan that has not yet met an -adversary. The layer is known to refuse 27 demonstrated modes -- the ids named in section 2, +adversary. The layer is known to refuse 28 demonstrated modes -- the ids named in section 2, not the run's larger total, for the reason section 1a gives. It is not known to be undefeatable on any of them. diff --git a/test/pytest/conftest.py b/test/pytest/conftest.py index c03425cd..9cbd1a3b 100644 --- a/test/pytest/conftest.py +++ b/test/pytest/conftest.py @@ -30,6 +30,8 @@ import pytest +import pgc_vacuity + from pgc_cluster import _pg_config, build_once, make_cluster pytest_plugins = ["pytester"] @@ -119,7 +121,11 @@ def pgc_conn(pgc_cluster, request): 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 + # WATCHED, so a write that wrote nothing cannot pass unnoticed. The three + # statements above run on the raw connection deliberately: they are this + # fixture's own DDL, not the test's writes, and DDL carries no row count + # anyway. See pgc_vacuity.watch_writes and test_writes_wrote_rows.py. + yield pgc_vacuity.watch_writes(conn, request.node.nodeid) finally: try: conn.execute(f'DROP SCHEMA IF EXISTS "{schema}" CASCADE') diff --git a/test/pytest/pgc_vacuity.py b/test/pytest/pgc_vacuity.py index 02dd9836..62ffb7fe 100644 --- a/test/pytest/pgc_vacuity.py +++ b/test/pytest/pgc_vacuity.py @@ -113,6 +113,49 @@ def _failed_query(v, _prefix=QUERY_ERROR): # own tests and two workers cannot share a counter. _RECORDERS = {} +# Writes seen during one test, keyed by nodeid exactly as _RECORDERS is, and for the +# same reason: `pytester` runs this layer's own tests IN-PROCESS, so an inner run +# imports this module and a single shared list would mix the two sessions together. +_WRITES = {} + +# The command tags that mean rows were supposed to move. The server reports these, +# so this guard never parses SQL -- see test_writes_wrote_rows.py for the measured +# table of statusmessage against rowcount. MERGE is PG 15+, and is listed because a +# port that starts using it should not silently fall outside the guard. +_WRITE_TAGS = ("INSERT", "UPDATE", "DELETE", "MERGE", "COPY") + + +class _Write: + """One write statement's outcome: the tag, the count, and whether it was named.""" + + __slots__ = ("tag", "count", "acknowledged") + + def __init__(self, tag, count): + self.tag = tag + self.count = count + self.acknowledged = False + + +def note_write(nodeid, cur): + """Record a statement if it was a write, from the cursor the server answered on. + + THE TAG DECIDES, NOT THE ROW COUNT. `SELECT 0` and `INSERT 0 0` both carry + rowcount 0, so a guard keyed on the count alone would refuse every test whose + last statement was a SELECT over an empty result -- a legitimate assertion. + `statusmessage` is the server's own command tag, which separates them without + this code ever looking at the SQL. + """ + message = getattr(cur, "statusmessage", None) + if not message: + return None + tag = str(message).split(" ", 1)[0].upper() + if tag not in _WRITE_TAGS: + return None + count = getattr(cur, "rowcount", -1) + w = _Write(tag, count) + _WRITES.setdefault(nodeid, []).append(w) + return w + # 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 @@ -312,6 +355,40 @@ def rowcount(self, got, want, name): ) self.num(got, want, name) + def wrote(self, cur, want, name): + """Assert how many rows a write actually wrote, and acknowledge a zero. + + Two jobs in one call, deliberately. It compares the count, and it marks the + write as NAMED so the session guard does not refuse it. A zero-row write is + legitimate when it is the thing being asserted -- a DELETE that must match + nothing is a real negative control -- and the way to say so is to say the + number. An unnamed zero stays a failure. + + A rowcount of -1 is refused rather than compared, for the reason + `expect.rowcount` records: it is psycopg's "no count available", so DDL + reaches this with -1 and comparing it to 0 would read as a mismatch, which + is the right verdict for the wrong reason. + """ + count = getattr(cur, "rowcount", None) + if count is None: + raise VacuityError( + f"{name}: wrote() needs the cursor the statement ran on, and " + f"{type(cur).__name__} has no rowcount." + ) + if count == -1: + raise VacuityError( + # Same reason as above for the single token. + f"{name}: no-count-available: the statement reported -1, which is " + f"psycopg's \"no row count\" and not a number of rows. A statement " + f"with no count is not a write whose rows can be asserted." + ) + tag = str(getattr(cur, "statusmessage", "") or "").split(" ", 1)[0].upper() + for w in _WRITES.get(self.nodeid, ()): + if not w.acknowledged and w.count == count and (not tag or w.tag == tag): + w.acknowledged = True + break + self.num(count, want, name) + # -- row sets ---------------------------------------------------------- def rows(self, got, want, name, allow_empty=None): """Compare two result sets. Refuses two empty sides unless declared. @@ -646,6 +723,76 @@ def expect(request): _RECORDERS.pop(request.node.nodeid, None) +class _WatchedCursor: + """A psycopg cursor that reports every write it runs to the guard. + + A PROXY RATHER THAN A SUBCLASS, because psycopg builds cursors itself and the + connection is what hands them out. `__getattr__` forwards everything this class + does not name, so the cursor keeps its whole API -- iteration, context manager, + fetchall, description -- and only `execute` grows a side effect. + """ + + def __init__(self, cur, nodeid): + self._cur = cur + self._nodeid = nodeid + + def execute(self, *args, **kwargs): + result = self._cur.execute(*args, **kwargs) + note_write(self._nodeid, self._cur) + # psycopg returns the cursor itself, so hand back the WATCHED one: a caller + # writing `for row in cur.execute(...)` must not escape the proxy. + return self if result is self._cur else result + + def executemany(self, *args, **kwargs): + result = self._cur.executemany(*args, **kwargs) + note_write(self._nodeid, self._cur) + return result + + def __getattr__(self, attr): + return getattr(self._cur, attr) + + def __iter__(self): + return iter(self._cur) + + def __enter__(self): + self._cur.__enter__() + return self + + def __exit__(self, *exc): + return self._cur.__exit__(*exc) + + +class _WatchedConnection: + """A psycopg connection whose cursors are watched. Same proxy argument.""" + + def __init__(self, conn, nodeid): + self._conn = conn + self._nodeid = nodeid + + def execute(self, *args, **kwargs): + cur = self._conn.execute(*args, **kwargs) + note_write(self._nodeid, cur) + return _WatchedCursor(cur, self._nodeid) + + def cursor(self, *args, **kwargs): + return _WatchedCursor(self._conn.cursor(*args, **kwargs), self._nodeid) + + def __getattr__(self, attr): + return getattr(self._conn, attr) + + def __enter__(self): + self._conn.__enter__() + return self + + def __exit__(self, *exc): + return self._conn.__exit__(*exc) + + +def watch_writes(conn, nodeid): + """Wrap a connection so its writes reach the guard. Used by conftest.""" + return _WatchedConnection(conn, nodeid) + + @pytest.hookimpl(wrapper=True) def pytest_runtest_call(item): """Fail a test that concluded nothing, after its body has run. @@ -661,6 +808,33 @@ def pytest_runtest_call(item): f"A test that concludes nothing must not report a pass. " f"Use the `expect` fixture, or declare it unrunnable with a reason." ) + # A WRITE THAT WROTE NOTHING BUILT THE WRONG FIXTURE, and the assertions below + # it then compared two empty things. `INSERT ... SELECT ... WHERE false` raises + # nothing and reports `INSERT 0 0`; before this, nobody in the corpus read + # either field. That is `insert-wrote-no-rows` in VACUITY_MODES.md section 3.5. + # + # IN THE CALL PHASE, not a teardown fixture. #931 measured that a guard run as a + # teardown reports the test it guards as PASSED and fails separately, so a + # reader sees a green test beside an error. Raising here fails the test itself. + # + # NAMED INDIVIDUALLY, because a fixture that runs four writes and gets nothing + # from the third is the real shape, and "a write wrote no rows" sends the reader + # to the wrong statement. + empty = [w for w in _WRITES.pop(item.nodeid, ()) if w.count == 0 and not w.acknowledged] + if empty: + which = ", ".join(f"#{i + 1} {w.tag}" for i, w in enumerate(empty)) + raise VacuityError( + # ONE UNBREAKABLE TOKEN FIRST. pytest word-wraps a long traceback line, + # and an arm matching a multi-word phrase against one line then matches + # nothing -- which reads as "the guard did not fire". Measured here: + # "wrote no rows" straddled the wrap. The kebab-case id is the mode's + # own name in VACUITY_MODES.md and cannot be split. + f"vacuity guard: insert-wrote-no-rows in {item.name}: {which} moved " + f"no rows. A write that wrote nothing built the fixture the assertions " + f"above it then measured as empty. Assert the count with " + f"expect.wrote(cur, n, ...) -- naming a deliberate zero is what " + f"separates a negative control from a broken fixture." + ) return result diff --git a/test/pytest/test_connection.py b/test/pytest/test_connection.py index 2d5a3dd5..3eafba7d 100644 --- a/test/pytest/test_connection.py +++ b/test/pytest/test_connection.py @@ -8,6 +8,8 @@ import decimal import pathlib +import pgc_vacuity + def test_cluster_fixture_gives_a_typed_connection(pgc_conn, expect): """count(*) must arrive as an int, not as text.""" @@ -184,3 +186,43 @@ def test_the_cluster_refuses_a_foreign_server(pgc_cluster, expect): pgc_cluster.port) expect.text(impostor.is_ours(), False, "a server whose datadir differs is refused") + +def test_the_connection_the_tests_use_is_watched(pgc_conn, expect, request): + """Writes through this fixture reach the zero-row guard. + + THIS IS THE HALF THE DRIVER-FREE ARMS CANNOT PROVE. test_writes_wrote_rows.py + exercises the classifier and the refusal against stub cursors, which says + nothing about whether the connection the tests actually use is wrapped at all. + Proving a function and proving its call site are two proofs, and the second is + the one that goes missing: #917's pytest twin tested the reconciler's body and + left the runner's CALL to it uncovered, so deleting the call kept that half + green while the shell half went red. + + BOTH PATHS, because the corpus uses both. 24 sites call `conn.execute` and 42 + call `cur.execute` on a cursor the connection handed out, so a proxy that + watched only the connection would leave most of the corpus unwatched. + """ + writes = pgc_vacuity._WRITES.setdefault(request.node.nodeid, []) + writes.clear() + + pgc_conn.execute("CREATE TABLE watched (i int) USING pgcolumnar") + expect.num(len(writes), 0, "DDL carries no row count, so it is not a write") + + cur = pgc_conn.execute("INSERT INTO watched SELECT g FROM generate_series(1,3) g") + expect.num(len(writes), 1, "a write through conn.execute is seen") + expect.num(writes[-1].count, 3, "with the count the server reported") + expect.text(writes[-1].tag, "INSERT", "and the command tag it reported") + expect.wrote(cur, 3, "and expect.wrote reads the same count back") + + with pgc_conn.cursor() as c: + c.execute("INSERT INTO watched SELECT g FROM generate_series(1,2) g") + expect.num(len(writes), 2, "a write through a handed-out cursor is seen too") + expect.wrote(c, 2, "and its count is the one the server reported") + + # A zero-row write through the real driver, which is the mode itself. Naming the + # zero is what keeps this test passing; without the name the guard would fail it, + # and that refusal is pinned in test_writes_wrote_rows.py where it can be caught. + empty = pgc_conn.execute("INSERT INTO watched SELECT g FROM generate_series(1,3) g " + "WHERE false") + expect.num(len(writes), 3, "the empty write is recorded like any other") + expect.wrote(empty, 0, "and INSERT ... WHERE false wrote no rows, deliberately") diff --git a/test/pytest/test_harness_deps.py b/test/pytest/test_harness_deps.py index 26eebaf4..da263f08 100644 --- a/test/pytest/test_harness_deps.py +++ b/test/pytest/test_harness_deps.py @@ -75,6 +75,7 @@ # list IS this list actually runs them. This file cannot be in the list: it # hands cluster-bound file names to pytest, so it needs what they need. "test_harness_deps_classifier.py", + "test_writes_wrote_rows.py", # Landed on main in #930 while this branch was in review, and the arm below named # it: cluster-free, not driver-dependent, so the job can run it and the # declaration has to say so. The third time this arm has caught a merge-order diff --git a/test/pytest/test_writes_wrote_rows.py b/test/pytest/test_writes_wrote_rows.py new file mode 100644 index 00000000..6827b4b0 --- /dev/null +++ b/test/pytest/test_writes_wrote_rows.py @@ -0,0 +1,201 @@ +"""A write that wrote nothing must not pass as a fixture that built something. + +`INSERT ... SELECT ... WHERE false` writes no rows and raises nothing. psycopg +reports `INSERT 0 0` with `rowcount == 0`, and until this guard existed nobody in +the corpus read either: the fixture built the wrong situation, and every assertion +downstream of it compared two empty things. That is `insert-wrote-no-rows` in +VACUITY_MODES.md section 3.5. + +THE TAG IS THE SIGNAL, NOT THE SQL TEXT. A zero `rowcount` alone cannot tell a +write that wrote nothing from a SELECT that matched nothing -- both are 0. The +server's own command tag separates them, so this guard never parses SQL. Measured +on PG 18 against a pgcolumnar table, which is where these values come from: + + statusmessage rowcount statement + CREATE TABLE -1 CREATE TABLE t (i int) USING pgcolumnar + INSERT 0 5 5 INSERT INTO t SELECT g FROM generate_series(1,5) g + INSERT 0 0 0 INSERT ... WHERE false + UPDATE 0 0 UPDATE t SET i = i WHERE i > 100 + DELETE 0 0 DELETE FROM t WHERE i > 100 + SELECT 0 0 SELECT * FROM t WHERE false + SET -1 SET search_path TO public + TRUNCATE TABLE -1 TRUNCATE t + +THE ARMS HERE NEED NO DATABASE, and that is deliberate rather than convenient: the +thing under test is the classifier and the refusal, which a stub cursor carrying +those two measured fields exercises exactly. The separate question -- whether the +connection the tests actually use is watched at all -- is a different property and +cannot be proven here. It has its own arm in `test_connection.py`, for the reason +#917 had to learn twice: proving a function and proving its call site are two +proofs, and the second one is the one that goes missing. +""" + +import pgc_vacuity + + +class _Cur: + """The two fields of a psycopg cursor this guard reads, and nothing else.""" + + def __init__(self, statusmessage, rowcount): + self.statusmessage = statusmessage + self.rowcount = rowcount + + +def _writes(nodeid): + return pgc_vacuity._WRITES.get(nodeid, []) + + +def test_a_write_that_wrote_nothing_is_recorded(expect, request): + nodeid = request.node.nodeid + "::probe-1" + pgc_vacuity.note_write(nodeid, _Cur("INSERT 0 0", 0)) + recorded = _writes(nodeid) + expect.num(len(recorded), 1, "an INSERT reporting 0 rows is recorded") + expect.num(recorded[0].count, 0, "and the count it carries is the zero") + expect.text(recorded[0].tag, "INSERT", "and the tag is the command, not the SQL") + pgc_vacuity._WRITES.pop(nodeid, None) + + +def test_update_and_delete_are_writes_too(expect, request): + for tag, message in (("UPDATE", "UPDATE 0"), ("DELETE", "DELETE 0")): + nodeid = request.node.nodeid + "::" + tag + pgc_vacuity.note_write(nodeid, _Cur(message, 0)) + expect.num(len(_writes(nodeid)), 1, f"a {tag} reporting 0 rows is recorded") + pgc_vacuity._WRITES.pop(nodeid, None) + + +def test_a_select_matching_nothing_is_not_a_write(expect, request): + """The arm the rowcount-only version of this guard would fail. + + `SELECT 0` and `INSERT 0 0` both carry rowcount 0. A guard keyed on the count + alone would refuse every test whose last statement was a SELECT over an empty + result, which is a legitimate and common thing to assert. + """ + nodeid = request.node.nodeid + "::select" + pgc_vacuity.note_write(nodeid, _Cur("SELECT 0", 0)) + expect.num(len(_writes(nodeid)), 0, "a SELECT returning no rows is not a write") + pgc_vacuity._WRITES.pop(nodeid, None) + + +def test_ddl_is_not_a_write(expect, request): + for message in ("CREATE TABLE", "SET", "TRUNCATE TABLE", "DROP SCHEMA"): + nodeid = request.node.nodeid + "::" + message.replace(" ", "_") + pgc_vacuity.note_write(nodeid, _Cur(message, -1)) + expect.num(len(_writes(nodeid)), 0, f"{message} is not a write that wrote rows") + pgc_vacuity._WRITES.pop(nodeid, None) + + +def test_a_write_that_wrote_rows_needs_no_acknowledgement(expect, request): + nodeid = request.node.nodeid + "::ok" + pgc_vacuity.note_write(nodeid, _Cur("INSERT 0 5", 5)) + recorded = _writes(nodeid) + expect.num(len(recorded), 1, "a write that wrote rows is still recorded") + expect.num(recorded[0].count, 5, "with the count it reported") + expect.num(sum(1 for w in recorded if w.count == 0), 0, + "and nothing about it is a zero-row write") + pgc_vacuity._WRITES.pop(nodeid, None) + + +# ---- the guard, through a real inner run ------------------------------------- +# +# An inner run rather than a direct call on the hook, because the property is "the +# test FAILS", and only a run can report that. Same shape as test_layer.py's arms. + +_PROLOGUE = """ + import pgc_vacuity + + class _Cur: + def __init__(self, statusmessage, rowcount): + self.statusmessage = statusmessage + self.rowcount = rowcount +""" + + +def _inner(pytester, body): + pytester.makeconftest("pytest_plugins = ['pgc_vacuity']") + pytester.makepyfile(_PROLOGUE + body) + return pytester.runpytest("-p", "pgc_vacuity") + + +def test_an_unacknowledged_zero_row_write_fails_the_test(expect, pytester): + result = _inner(pytester, """ + def test_fixture_built_nothing(expect, request): + pgc_vacuity.note_write(request.node.nodeid, _Cur("INSERT 0 0", 0)) + expect.num(1, 1, "an assertion, so the no-assertion guard is not what fires") + """) + expect.outcomes(result, "a test whose write wrote nothing does not pass", + passed=0, failed=1) + # ONE PATTERN PER CALL. `expect.refusal` anchors each pattern to a single `E` + # line, and pytest WORD-WRAPS a long traceback line -- so two patterns need two + # lines and a multi-word phrase can straddle the wrap. Measured: matching + # "wrote no rows" failed against a message that contained it, which reads as + # "the guard did not fire". Single unbreakable tokens, one per call. + expect.refusal(result, "and the refusal names the mode", r"insert-wrote-no-rows") + expect.refusal(result, "and it names the command that moved nothing", r"INSERT") + + +def test_expect_wrote_acknowledges_the_zero(expect, pytester): + """A zero-row write is legitimate when it is the thing being asserted. + + A negative control -- a DELETE that must delete nothing -- is a real test, and + the guard must not make it unwritable. Naming the zero is the acknowledgement. + """ + result = _inner(pytester, """ + def test_deliberately_wrote_nothing(expect, request): + cur = _Cur("DELETE 0", 0) + pgc_vacuity.note_write(request.node.nodeid, cur) + expect.wrote(cur, 0, "the delete matched nothing, which is the point") + """) + expect.outcomes(result, "naming the zero lets the test pass", passed=1, failed=0) + + +def test_expect_wrote_refuses_a_count_that_is_not_a_count(expect, pytester): + """rowcount is -1 when the statement produced no count. + + Comparing -1 with an expected number would read as a mismatch, which is the + right verdict for the wrong reason. The same trap `expect.rowcount` records. + """ + result = _inner(pytester, """ + def test_no_count_available(expect, request): + expect.wrote(_Cur("CREATE TABLE", -1), 0, "a DDL statement has no row count") + """) + expect.outcomes(result, "a -1 is refused rather than compared", passed=0, failed=1) + expect.refusal(result, "and the refusal says the count is not one", + r"no-count-available") + + +def test_expect_wrote_still_compares(expect, pytester): + """The acknowledgement is not a waiver: a wrong count is still a failure.""" + result = _inner(pytester, """ + def test_wrote_the_wrong_number(expect, request): + cur = _Cur("INSERT 0 3", 3) + pgc_vacuity.note_write(request.node.nodeid, cur) + expect.wrote(cur, 5, "the fixture claims five rows") + """) + expect.outcomes(result, "acknowledging a count does not excuse a wrong one", + passed=0, failed=1) + # THE PATTERN IS NOT DECORATION. With only `failed=1` this arm passed before + # `expect.wrote` existed at all, satisfied by the AttributeError from the + # missing function -- a green for a reason that had nothing to do with the + # comparison. Naming the numbers is what makes the red the right red. + expect.refusal(result, "and the refusal names both counts", r"got 3 want 5") + + +def test_several_writes_and_only_the_empty_one_is_named(expect, pytester): + """The refusal must name WHICH write wrote nothing, not that one did. + + A fixture that runs four INSERTs and gets nothing from the third is the real + shape. A message saying only "a write wrote no rows" sends the reader to the + wrong statement. + """ + result = _inner(pytester, """ + def test_three_writes_one_empty(expect, request): + n = request.node.nodeid + pgc_vacuity.note_write(n, _Cur("INSERT 0 5", 5)) + pgc_vacuity.note_write(n, _Cur("UPDATE 0", 0)) + pgc_vacuity.note_write(n, _Cur("INSERT 0 7", 7)) + expect.num(1, 1, "an assertion") + """) + expect.outcomes(result, "the empty write fails the test", passed=0, failed=1) + expect.refusal(result, "and the UPDATE is the one named, not the INSERTs", + r"UPDATE") + expect.refusal(result, "and the mode is named", r"insert-wrote-no-rows") From 29834e3bfd7a1005942d4cefe3a4ba3e7010c16c Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Thu, 10 Sep 2026 20:58:32 +0000 Subject: [PATCH 2/3] test/pytest: two holes found by attacking the guard I had just shipped (#432) Both were found by asking what the new guard ACCEPTS rather than what it refuses, and the first version was green with both. `expect.wrote` ACCEPTED A SELECT. A query matching nothing reports `SELECT 0` with `rowcount == 0`, so `expect.wrote(cur, 0, name)` compared 0 with 0 and passed -- asserting "this write wrote no rows" about a statement that is not a write. It reads as a deliberate zero and pins nothing, which is this layer's own subject appearing inside the assertion written to close it. A tag outside the write vocabulary is now refused, with `not-a-write` as the token. THE REFUSAL NUMBERED THE WRONG THING. It enumerated the empty writes it was about to print, so `#1` meant "the first one I am complaining about" and identified no statement: a reader counting writes in the source went to the wrong line. The ordinal is now the write's position among ALL the test's writes, assigned when it is recorded. AND THAT ONE PRODUCED AN ARM THAT COULD NOT DISCRIMINATE, which is the part worth keeping. My first version of the arm used two writes, where the old numbering and the new one both print `#1` -- so it passed against the defect it was written for. It now uses three writes with the empty one SECOND: the filtered numbering says `#1`, the absolute one says `#2`, and only one of those sends a reader to the right statement. An acknowledgement is now matched by the CURSOR that ran the statement rather than by `(tag, count)`. Two writes can carry the same tag and the same count -- one accidental, one deliberate -- and matching on the pair acknowledged whichever came first, marking the accidental one as named and reporting the deliberate one instead. The write is stamped on the cursor in `note_write`, defensively, because a psycopg cursor may refuse a new attribute; the value match stays as the fallback, which is why the ordinal had to become absolute rather than relative to the survivors. Prove by removal, now eight mutations, each applied by exact string match with the file asserted to still parse: driver-free wiring arm control 13 passed 9 passed no refusal 3 failed 9 passed command tag ignored 2 failed 1 failed every statement is a write 3 failed 1 failed connection not wrapped 13 PASSED 1 failed acknowledgement not recorded 1 failed 9 passed non-write accepted 1 failed 9 passed acknowledge by (tag, count) only 1 failed 9 passed ordinal relative to survivors 1 failed 9 passed The fifth row is still the one that matters: unwiring the connection leaves every driver-free arm green and reds the wiring arm alone, which is why they are two files. Gate: harness_selftest 588 checks, 588 passed + 0 failed + 0 unrunnable, PASSED driver-free job 10 files, 174 passed, psycopg absent from the venv full corpus 252 passed with a cluster on pg18a Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- test/pytest/TESTS.md | 27 ++++++++++ test/pytest/pgc_vacuity.py | 60 ++++++++++++++++++---- test/pytest/test_writes_wrote_rows.py | 74 +++++++++++++++++++++++++++ 3 files changed, 151 insertions(+), 10 deletions(-) diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 340ac5e8..1177b1ce 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -1664,6 +1664,9 @@ separately, so a reader sees a green test beside an error. | `test_expect_wrote_refuses_a_count_that_is_not_a_count` | `rowcount == -1` is refused rather than compared | | `test_expect_wrote_still_compares` | acknowledging a count does not excuse a wrong one | | `test_several_writes_and_only_the_empty_one_is_named` | with three writes and one empty, the refusal names the empty one | +| `test_wrote_refuses_a_statement_that_is_not_a_write` | `expect.wrote` on a `SELECT 0` is refused, not compared | +| `test_the_acknowledgement_names_one_write_and_not_its_twin` | naming one zero does not acknowledge a different identical zero | +| `test_acknowledging_both_identical_zeros_passes` | the control for that arm: naming both is legitimate | ### Two properties, two files, on purpose @@ -1683,6 +1686,30 @@ left the runner's CALL to it uncovered: removing the call kept the pytest half a 9 passed while the shell half went red by one. Proving a function and proving its call site are two proofs, and the second is the one that goes missing. +### Two holes found by attacking this guard, after it was green + +Both were found by asking what the guard would accept rather than what it refuses, +and both are recorded because the first version shipped green with them. + +**`expect.wrote` accepted a SELECT.** A query matching nothing reports `SELECT 0` +with `rowcount == 0`, so `expect.wrote(cur, 0, name)` compared 0 with 0 and passed -- +asserting "this write wrote no rows" about a statement that is not a write. It reads +as a deliberate zero and pins nothing, which is this document's own subject appearing +inside the assertion written to close it. A non-write tag is now refused. + +**The refusal numbered the wrong thing.** It enumerated the empty writes it was about +to print, so `#1` meant "the first one I am complaining about" and identified no +statement -- a reader counting writes in the source went to the wrong line. The +ordinal is now the write's position among ALL the test's writes. + +That one also made an arm that could not discriminate. With two writes, both the old +and the new numbering print `#1`, so the arm passed either way; the arm now uses +three writes with the empty one second, where the old numbering says `#1` and the new +one says `#2`. An acknowledgement is also matched by the cursor that ran the +statement rather than by `(tag, count)`, because two writes can carry the same tag +and the same count -- one accidental, one deliberate -- and matching on the pair +marked the accidental one as named and reported the deliberate one instead. + ### What made the arms themselves wrong twice Recorded because both produced a green that meant nothing. diff --git a/test/pytest/pgc_vacuity.py b/test/pytest/pgc_vacuity.py index 62ffb7fe..343896af 100644 --- a/test/pytest/pgc_vacuity.py +++ b/test/pytest/pgc_vacuity.py @@ -126,13 +126,22 @@ def _failed_query(v, _prefix=QUERY_ERROR): class _Write: - """One write statement's outcome: the tag, the count, and whether it was named.""" + """One write statement's outcome. + + THE ORDINAL IS AMONG ALL THE TEST'S WRITES, not among the empty ones. The + refusal numbered the survivors it was about to print, so "#1" meant "the first + one I am complaining about" and identified no statement -- a reader counting + writes in the source went to the wrong line. Found by attacking this guard with + three writes where the empty one is the second: the filtered number said #1 and + the real answer was #2. + """ - __slots__ = ("tag", "count", "acknowledged") + __slots__ = ("tag", "count", "acknowledged", "ordinal") - def __init__(self, tag, count): + def __init__(self, tag, count, ordinal): self.tag = tag self.count = count + self.ordinal = ordinal self.acknowledged = False @@ -152,8 +161,19 @@ def note_write(nodeid, cur): if tag not in _WRITE_TAGS: return None count = getattr(cur, "rowcount", -1) - w = _Write(tag, count) - _WRITES.setdefault(nodeid, []).append(w) + seen = _WRITES.setdefault(nodeid, []) + w = _Write(tag, count, len(seen) + 1) + seen.append(w) + # STAMPED ON THE CURSOR THAT RAN IT, so an acknowledgement is about a statement + # rather than about a pair of numbers. Two writes can carry the same tag and the + # same count -- one accidental, one deliberate -- and matching on those + # acknowledged whichever came first, which marked the accidental one as named and + # reported the deliberate one instead. A psycopg cursor may refuse a new + # attribute, so this is defensive and `wrote` still falls back to matching. + try: + cur._pgc_write = w + except (AttributeError, TypeError): + pass return w # lib.sh:58 PGC_EXIT_INCOMPLETE. The same number deliberately: a suite that could @@ -383,10 +403,30 @@ def wrote(self, cur, want, name): f"with no count is not a write whose rows can be asserted." ) tag = str(getattr(cur, "statusmessage", "") or "").split(" ", 1)[0].upper() - for w in _WRITES.get(self.nodeid, ()): - if not w.acknowledged and w.count == count and (not tag or w.tag == tag): - w.acknowledged = True - break + if tag not in _WRITE_TAGS: + raise VacuityError( + # A SELECT matching nothing reports `SELECT 0` with rowcount 0, so + # this compared 0 with 0 and PASSED -- asserting "this write wrote no + # rows" about a statement that is not a write. It read as a deliberate + # zero and pinned nothing, which is this layer's own subject appearing + # inside the assertion meant to close it. + f"{name}: not-a-write: the statement reported tag " + f"{tag or '(none)'!s}, which is not one of {', '.join(_WRITE_TAGS)}. " + f"wrote() asserts how many rows a WRITE moved; for a query that " + f"returned no rows, assert the rows." + ) + # BY IDENTITY FIRST: the write stamped on this cursor is the statement this + # call is about. The value match is the fallback for a cursor that could not + # be stamped, and it is why the ordinal in the refusal is absolute. + w = getattr(cur, "_pgc_write", None) + if w is not None and not w.acknowledged: + w.acknowledged = True + else: + for candidate in _WRITES.get(self.nodeid, ()): + if not candidate.acknowledged and candidate.count == count \ + and candidate.tag == tag: + candidate.acknowledged = True + break self.num(count, want, name) # -- row sets ---------------------------------------------------------- @@ -822,7 +862,7 @@ def pytest_runtest_call(item): # to the wrong statement. empty = [w for w in _WRITES.pop(item.nodeid, ()) if w.count == 0 and not w.acknowledged] if empty: - which = ", ".join(f"#{i + 1} {w.tag}" for i, w in enumerate(empty)) + which = ", ".join(f"#{w.ordinal} {w.tag}" for w in empty) raise VacuityError( # ONE UNBREAKABLE TOKEN FIRST. pytest word-wraps a long traceback line, # and an arm matching a multi-word phrase against one line then matches diff --git a/test/pytest/test_writes_wrote_rows.py b/test/pytest/test_writes_wrote_rows.py index 6827b4b0..82ff35ce 100644 --- a/test/pytest/test_writes_wrote_rows.py +++ b/test/pytest/test_writes_wrote_rows.py @@ -199,3 +199,77 @@ def test_three_writes_one_empty(expect, request): expect.refusal(result, "and the UPDATE is the one named, not the INSERTs", r"UPDATE") expect.refusal(result, "and the mode is named", r"insert-wrote-no-rows") + +# ---- two holes I found by attacking my own guard ------------------------------ + + +def test_wrote_refuses_a_statement_that_is_not_a_write(expect, pytester): + """`expect.wrote` must not accept a SELECT. + + A SELECT that matched nothing reports `SELECT 0` with `rowcount == 0`, so + `expect.wrote(cur, 0, ...)` compared 0 with 0 and passed -- asserting "this + write wrote no rows" about a statement that is not a write at all. It reads as + a deliberate zero and pins nothing, which is this document's whole subject in + the assertion meant to close it. + """ + result = _inner(pytester, """ + def test_wrote_about_a_select(expect, request): + expect.wrote(_Cur("SELECT 0", 0), 0, "a select is not a write") + """) + expect.outcomes(result, "wrote() refuses a non-write statement", passed=0, failed=1) + expect.refusal(result, "and the refusal names the reason", r"not-a-write") + + +def test_the_acknowledgement_names_one_write_and_not_its_twin(expect, pytester): + """Acknowledging one zero must not acknowledge a DIFFERENT identical zero. + + Two writes can carry the same tag and the same count: one accidental, one + deliberate. Matching on (tag, count) alone acknowledged whichever came first, + so naming the deliberate one could mark the accidental one as named. The test + still failed -- one unacknowledged write remained -- but the refusal named the + wrong statement, which is the same defect as a wrong line number in a + traceback. + + The write is now carried on the cursor that ran it, so an acknowledgement is + about that statement rather than about a pair of numbers. + """ + # THREE WRITES, AND THE ORDINAL IS WHAT DISCRIMINATES. With two writes both + # spellings of this guard print "#1" and the arm passes either way -- it did, + # before I changed the shape. The accidental write is the SECOND of three here, + # so value-matching (which acknowledges the accidental one and leaves the + # deliberate one, then numbers the survivors from 1) prints "#1", and + # identity-matching with an absolute ordinal prints "#2". Only one of those is + # the statement a reader has to go and look at. + result = _inner(pytester, """ + def test_two_identical_zeros(expect, request): + n = request.node.nodeid + fine = _Cur("INSERT 0 5", 5) + accidental = _Cur("INSERT 0 0", 0) + deliberate = _Cur("INSERT 0 0", 0) + for c in (fine, accidental, deliberate): + pgc_vacuity.note_write(n, c) + expect.wrote(deliberate, 0, "the third one is deliberate") + """) + expect.outcomes(result, "the accidental zero is still unacknowledged", + passed=0, failed=1) + expect.refusal(result, "and it is named by its position among ALL the writes", + r"#2") + + +def test_acknowledging_both_identical_zeros_passes(expect, pytester): + """The control for the arm above: naming both is legitimate and must pass. + + Without this, the arm above is satisfied by a guard that can never be + satisfied, which is a refusal that has stopped being a test. + """ + result = _inner(pytester, """ + def test_two_named_zeros(expect, request): + n = request.node.nodeid + first = _Cur("DELETE 0", 0) + second = _Cur("DELETE 0", 0) + pgc_vacuity.note_write(n, first) + pgc_vacuity.note_write(n, second) + expect.wrote(first, 0, "the first delete matched nothing") + expect.wrote(second, 0, "and so did the second") + """) + expect.outcomes(result, "naming both zeros passes", passed=1, failed=0) From 7c5d85b54cc66ca181b5f46064e005cd7611884a Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Thu, 10 Sep 2026 21:14:58 +0000 Subject: [PATCH 3/3] test/pytest: the stamp is a call site too (#432) @jdatcmd found that the identity fix could not run against the real driver, and that it took the ordinal fix down with it. Both are right and the second is the part that mattered: the two rest on each other, so with the stamp swallowed the refusal named the wrong statement in exactly the case the ordinal was added for. `note_write` stamped the RAW psycopg cursor. A `psycopg.Cursor` cannot take a new attribute, so the stamp was swallowed by its own `except` on every real write, and the object the test holds is a `_WatchedCursor` whose `__getattr__` forwards the lookup to the raw cursor that never got one. Measured through the driver: connection.cursor() stampable=False (Cursor: AttributeError) conn.execute() return stampable=False (Cursor: AttributeError) ServerCursor stampable=False (ServerCursor: AttributeError) the arms' _Cur stub stampable=True THE ARMS COULD NOT SEE IT, and that is the lesson rather than the bug. They proved the identity mechanism on an object that differs from the real one in exactly the respect under test -- the hazard `_WatchedCursor`'s own docstring names, arriving in the arms written to guard it. The fix stamps the object the CALLER receives; the stamp on the raw cursor stays, because it is what the layer's own stub-driven arms use, and its comment now says what it reaches and what it does not. AND MY FIRST ARM FOR IT PINNED NEITHER SITE. There are two stamps -- one in `_WatchedCursor.execute`, one in `_WatchedConnection.execute` -- and the arm exercised only the connection path, so removing the cursor-path stamp left it green. Found by mutating the fix rather than by reading it. The arm now drives both paths with INDISTINGUISHABLE writes, which is the only case where identity and value matching can disagree: the existing wiring arm uses the cursor path with different counts, where value matching finds the right write whether or not the stamp lands. Prove by removal, ten mutations, each by exact string with a parse assertion: driver-free cluster arms control 13 passed 10 passed no refusal 3 failed 10 passed command tag ignored 2 failed 2 failed every statement is a write 3 failed 2 failed connection not wrapped 13 passed 2 failed acknowledgement not recorded 1 failed 1 failed non-write accepted 1 failed 10 passed acknowledge by (tag, count) only 1 failed 1 failed ordinal relative to survivors 1 failed 10 passed cursor-path stamp removed 13 passed 1 failed connection-path stamp removed 13 passed 1 failed Three rows now red ONLY the cluster side, which is the argument for the split stated three times over: a driver-free arm cannot see wiring, and cannot see an attribute a real object refuses. RESIDUAL RECORDED, MEASURED BY @jdatcmd ON TWO MAJORS. `statusmessage` is never absent and its wording is byte-identical on PG 15.18 and PG 17.10 across twelve shapes. Four shapes write rows while reporting a non-write tag -- a data-modifying CTE and a SELECT of an inserting function both report SELECT, a DO block reports DO, a CALL reports CALL -- and ZERO occur in the corpus, whose writes are 15 INSERT, 14 COPY, 2 DELETE and 1 UPDATE. So it is stated rather than closed: an arm for a shape nothing uses is an instrument with nothing exercising it. Gate: harness_selftest 588 checks, 588 passed + 0 failed + 0 unrunnable, PASSED driver-free job 10 files, 174 passed, psycopg absent from the venv full corpus 253 passed with a cluster on pg18a Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- test/pytest/TESTS.md | 37 +++++++++++++++++++ test/pytest/pgc_vacuity.py | 36 ++++++++++++++----- test/pytest/test_connection.py | 65 ++++++++++++++++++++++++++++++++++ 3 files changed, 130 insertions(+), 8 deletions(-) diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 1177b1ce..6fd59ece 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -781,6 +781,7 @@ written. | `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 | | `test_the_connection_the_tests_use_is_watched` | writes through `pgc_conn` reach the zero-row guard, on both the connection and a handed-out cursor | +| `test_the_acknowledgement_is_by_cursor_against_the_real_driver` | the write is stamped on the object the caller holds, which a stub cannot prove | Two of these deserve their reasoning stated. @@ -1686,6 +1687,42 @@ left the runner's CALL to it uncovered: removing the call kept the pytest half a 9 passed while the shell half went red by one. Proving a function and proving its call site are two proofs, and the second is the one that goes missing. +### What the command tag cannot see + +Measured by @jdatcmd on PG 15.18 and PG 17.10, twelve statement shapes each through +psycopg 3.3.5: `statusmessage` is never absent and its wording is byte-identical +across both majors, which is the premise this guard rests on. + +Four shapes **write rows and report a tag that is not a write**, so this guard does +not see them: a data-modifying CTE and a `SELECT` of an inserting function both report +`SELECT`, a `DO` block reports `DO`, and a `CALL` reports `CALL`. **Zero occur in the +corpus** — the writes today are 15 `INSERT`, 14 `COPY`, 2 `DELETE` and 1 `UPDATE` — so +this is a residual to state rather than a gap to close. Writing an arm for a shape +nothing uses would be an instrument with nothing exercising it. + +### The stamp is a call site too + +The acknowledgement is carried on the cursor the caller holds. The first version +stamped the **raw** psycopg cursor, which cannot take a new attribute at all, so the +stamp was swallowed by its own `except` on every real write and `wrote()` fell back to +matching by `(tag, count)` — which made the absolute ordinal name the wrong statement +in exactly the case the ordinal was added for. The two fixes rest on each other. + +Measured, against PostgreSQL through the real driver: + +``` +connection.cursor() stampable=False (Cursor: AttributeError) +conn.execute() return stampable=False (Cursor: AttributeError) +ServerCursor stampable=False (ServerCursor: AttributeError) +the arms' _Cur stub stampable=True +``` + +**The driver-free arms could not see it**, and that is the lesson rather than the bug: +they proved the identity mechanism on an object that differs from the real one in +exactly the respect under test. The arm that catches it is cluster-bound, because a +real cursor is the only thing that can show it — which is the same sentence as the +wiring arm's, one level down. Found by @jdatcmd. + ### Two holes found by attacking this guard, after it was green Both were found by asking what the guard would accept rather than what it refuses, diff --git a/test/pytest/pgc_vacuity.py b/test/pytest/pgc_vacuity.py index 343896af..012b10ac 100644 --- a/test/pytest/pgc_vacuity.py +++ b/test/pytest/pgc_vacuity.py @@ -164,12 +164,21 @@ def note_write(nodeid, cur): seen = _WRITES.setdefault(nodeid, []) w = _Write(tag, count, len(seen) + 1) seen.append(w) - # STAMPED ON THE CURSOR THAT RAN IT, so an acknowledgement is about a statement + # STAMPED ON THE OBJECT THAT RAN IT, so an acknowledgement is about a statement # rather than about a pair of numbers. Two writes can carry the same tag and the # same count -- one accidental, one deliberate -- and matching on those - # acknowledged whichever came first, which marked the accidental one as named and - # reported the deliberate one instead. A psycopg cursor may refuse a new - # attribute, so this is defensive and `wrote` still falls back to matching. + # acknowledged whichever came first, marking the accidental one as named and + # reporting the deliberate one instead. + # + # THIS STAMP REACHES A STUB AND NOT A REAL CURSOR, measured rather than assumed: a + # `psycopg.Cursor`, a `ServerCursor` and the cursor `conn.execute` returns all + # raise AttributeError here, so on every real write this `except` swallowed it and + # `wrote` fell back to matching by value -- which made the absolute ordinal name + # the wrong statement in exactly the case it was added for. The caller-facing + # object is stamped by `_WatchedCursor` instead, and that is the stamp `wrote` + # finds. This one serves the layer's own arms, which pass a stub. + # Found by @jdatcmd, whose point was that the stub is stampable and the real + # cursor is not, so the arms could not see it. try: cur._pgc_write = w except (AttributeError, TypeError): @@ -775,17 +784,25 @@ class _WatchedCursor: def __init__(self, cur, nodeid): self._cur = cur self._nodeid = nodeid + # SET IN __init__ so it is always an INSTANCE attribute. Without it the first + # lookup falls through to `__getattr__`, which forwards to the raw cursor and + # raises -- readable through `getattr(..., None)`, but it would make the + # absence of a stamp indistinguishable from a cursor that has not run yet. + self._pgc_write = None def execute(self, *args, **kwargs): result = self._cur.execute(*args, **kwargs) - note_write(self._nodeid, self._cur) + # THE PROXY IS WHAT GETS STAMPED, because the proxy is what the caller holds + # and a real psycopg cursor cannot take the attribute at all. `__getattr__` + # never intercepts this, because the instance really has it. + self._pgc_write = note_write(self._nodeid, self._cur) # psycopg returns the cursor itself, so hand back the WATCHED one: a caller # writing `for row in cur.execute(...)` must not escape the proxy. return self if result is self._cur else result def executemany(self, *args, **kwargs): result = self._cur.executemany(*args, **kwargs) - note_write(self._nodeid, self._cur) + self._pgc_write = note_write(self._nodeid, self._cur) return result def __getattr__(self, attr): @@ -811,8 +828,11 @@ def __init__(self, conn, nodeid): def execute(self, *args, **kwargs): cur = self._conn.execute(*args, **kwargs) - note_write(self._nodeid, cur) - return _WatchedCursor(cur, self._nodeid) + watched = _WatchedCursor(cur, self._nodeid) + # Stamped on the proxy handed back, for the reason _WatchedCursor.execute + # gives: this is the object the test holds and passes to `wrote`. + watched._pgc_write = note_write(self._nodeid, cur) + return watched def cursor(self, *args, **kwargs): return _WatchedCursor(self._conn.cursor(*args, **kwargs), self._nodeid) diff --git a/test/pytest/test_connection.py b/test/pytest/test_connection.py index 3eafba7d..00d8c72b 100644 --- a/test/pytest/test_connection.py +++ b/test/pytest/test_connection.py @@ -226,3 +226,68 @@ def test_the_connection_the_tests_use_is_watched(pgc_conn, expect, request): "WHERE false") expect.num(len(writes), 3, "the empty write is recorded like any other") expect.wrote(empty, 0, "and INSERT ... WHERE false wrote no rows, deliberately") + +def test_the_acknowledgement_is_by_cursor_against_the_real_driver(pgc_conn, expect, + request): + """The stamp must land on the object the CALLER holds, not the one psycopg owns. + + THE DRIVER-FREE ARMS CANNOT SEE THIS, and that is why it belongs here. They stamp + a stub, and a stub accepts a new attribute; a real `psycopg.Cursor` raises + AttributeError, so the stamp was swallowed by its own `except` on every real + write and `wrote()` silently fell back to matching on `(tag, count)`. The arms + proved the identity mechanism on an object that differs from the real one in + exactly the respect under test -- which is the hazard the `_WatchedCursor` + docstring names, arriving in the arms that were supposed to guard it. Found by + @jdatcmd on #432. + + It matters because the two fixes rest on each other: the absolute ordinal is only + trustworthy when the acknowledgement is by identity, so with the stamp swallowed + the refusal named the wrong statement in precisely the case the ordinal was added + for. + + TWO INDISTINGUISHABLE WRITES, because that is the only case where identity and + value matching can disagree. Both are named, so the test passes; the assertion is + about WHICH one each call acknowledged. + """ + writes = pgc_vacuity._WRITES.setdefault(request.node.nodeid, []) + writes.clear() + + pgc_conn.execute("CREATE TABLE twin (i int) USING pgcolumnar") + first = pgc_conn.execute("INSERT INTO twin SELECT 1 WHERE false") + second = pgc_conn.execute("INSERT INTO twin SELECT 1 WHERE false") + expect.num(len(writes), 2, "two indistinguishable zero-row writes are recorded") + expect.text(f"{writes[0].tag}/{writes[0].count} {writes[1].tag}/{writes[1].count}", + "INSERT/0 INSERT/0", "premise: the two are indistinguishable by value") + + expect.text(type(getattr(second, "_pgc_write", None)).__name__, "_Write", + "the cursor the caller received carries the write it ran") + expect.num(getattr(second, "_pgc_write").ordinal, 2, + "and it is the SECOND write, by its absolute ordinal") + + expect.wrote(second, 0, "naming the second write") + expect.num(int(writes[1].acknowledged), 1, "acknowledges the second") + expect.num(int(writes[0].acknowledged), 0, "and leaves the first unnamed") + + expect.wrote(first, 0, "naming the first as well, so nothing is left unnamed") + expect.num(int(writes[0].acknowledged), 1, "which acknowledges the first") + + # BOTH STAMP SITES, because there are two and one probe pins neither. The writes + # above went through `conn.execute`; a cursor the connection hands out is a second + # path with its own stamp, and removing that one alone left this arm green -- + # found by mutating it, not by reading it. The existing wiring arm uses the cursor + # path but with DISTINGUISHABLE counts, so value matching finds the right write + # there whether or not the stamp lands. + writes.clear() + with pgc_conn.cursor() as cur: + cur.execute("INSERT INTO twin SELECT 1 WHERE false") + expect.num(len(writes), 1, "a cursor-path write is recorded") + expect.text(type(getattr(cur, "_pgc_write", None)).__name__, "_Write", + "and the handed-out cursor carries the write it ran") + cur.execute("INSERT INTO twin SELECT 1 WHERE false") + expect.num(len(writes), 2, "and so is its indistinguishable twin") + expect.num(getattr(cur, "_pgc_write").ordinal, 2, + "the cursor now carries the SECOND write, not the first") + expect.wrote(cur, 0, "naming what the cursor last ran") + expect.num(int(writes[1].acknowledged), 1, "acknowledges the second write") + expect.num(int(writes[0].acknowledged), 0, "and leaves the first unnamed") + expect.wrote(first, 0, "so name the first explicitly too")