diff --git a/CHANGELOG.md b/CHANGELOG.md index 4554ab0f..84ea3081 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -262,6 +262,79 @@ true until the next version shipped. placement. The half still open is the general shape: a guard anyone adds later in a teardown still cannot fail its test, and nothing refuses that. +- A failed query is no longer comparable with another failed query (#432). + + `error-swallowed-to-empty`: two queries raise, a helper turns each into the same + value, and they compare equal. The test is green and has asserted nothing about + either query. `test/lib.sh` closed this by PRODUCING the sentinel with a sequence + number per failure -- `res="QUERY_ERROR.$seq"` -- so two failures can never compare + equal. + + The pytest port had the constant `QUERY_ERROR = "QUERY_ERROR"`, a comment claiming it + was "unique per occurrence" -- which is false of a constant -- and a refusal in + exactly one assertion. Measured before the fix, with a sentinel on both sides: + `expect.hash` refused; `expect.text`, `expect.rows`, `expect.row_set` and + `expect.ordered_rows` all PASSED. Four of the five comparisons accepted two failed + queries as agreement. `expect.num`, `expect.at_least` and `expect.rowcount` refused + already, by their type guards rather than by anything about sentinels. + + There is now one refusal, called by every comparison, so an assertion added later + inherits it instead of being the next hole -- which is how this survived: `hash` had + a refusal and the four written after it did not. It matches the prefix at any depth, + because a sentinel arrives as a CELL inside a row as often as it arrives as a whole + side. `row_set` refuses BEFORE it maps its rows through `repr`, since + `repr(("QUERY_ERROR.1",))` does not start with the prefix: delegating an assertion + does not delegate its refusals when the delegation transforms the data. + + `query_error()` produces a value unique per occurrence, for the paths that compare + without the layer -- a helper comparing by hand, which is what the two existing + hand-rolled sentinels in `test_hilbert_locality.py` do. The refusal is the mechanism; + the producer is the second line. One of those two sentinels is produced by + `coalesce(...)` inside SQL and cannot use the Python producer, which is why the + refusal has to be the mechanism rather than the other way round. + + Each refusal is proved load-bearing: removing it from one assertion at a time makes + the arm name that assertion and no other, five times out of five. The mutations are + only distinguishable with `__pycache__` cleared between runs -- the five deleted + lines are byte-identical, so three of the five leave the file the same size and + Python reuses the stale bytecode, which made two of the refusals look like they did + not bite. + + THREE DEFECTS @jdatcmd FOUND IN THE FIRST VERSION, each reproduced before it was fixed. + + The HATCH WAS OPEN and the arm that said otherwise was tautological: it rewrote + `pgc_vacuity.QUERY_ERROR` and THEN minted its sentinels with `query_error()`, which + read that same global -- producer and matcher moved together, so the refusal matched + whatever the prefix had just been set to. The faithful hatch mints while armed and + rewrites afterwards, which is what a corpus file does, and against the first version + that COMPARED two sentinels instead of refusing them. End to end it reported + `1 passed` over two failed queries. The prefix is now bound in a DEFAULT ARGUMENT, + evaluated once when the function is defined and never read from the module namespace + again, so rewriting the global changes neither what is minted nor what is refused. + `ZZZ_NOT_A_PREFIX` is in the arm's spellings deliberately: `'Q'` cannot disarm a + prefix-reading matcher, because `'QUERY_ERROR.1'.startswith('Q')` is true. + + `ordering_observable` WAS MADE WEAKER BY THE PRODUCER, and this is the one place the + change regressed the layer. It takes `(forward, reverse)` rather than `(got, want)`, + so it sat outside the refusal and outside the derivation that finds comparisons. With + the old shared constant two failed readings were IDENTICAL and it went red -- loudly. + With unique sentinels they differ, so it passed and greenlit every ordered assertion + resting on the premise. The refusal is now the first thing it does, the derivation + recognises `(forward, reverse)`, and an arm pins both. + + And `TESTS.md` stated the split twice in one sentence while only the first half was + gated: 26 refused and "the other 47" sums to 73 against the 72 the inventory names. + `selftest/350`'s regex matches the half a change naturally updates. Both halves are + now read by an arm in the corpus's own docs guard, against the inventory's count of + the ids it names rather than a number typed twice. + + `VACUITY_MODES.md` said "the port has the sentinel constant but nothing produces it", + and that was wrong in both halves: two sites did produce sentinels by hand, and the + thing actually missing was the refusal in four of the five comparisons. The + correction is recorded in the document rather than quietly replacing the sentence, + because a map that names the wrong gap is worse than one that admits it does not + know. + - `ALTER TABLE ... RENAME COLUMN` now carries the new name into `pgcolumnar.projection_declaration`, for the named relation and for every inheritance descendant, including a `PARTITION OF` child (#888). diff --git a/test/pytest/README.md b/test/pytest/README.md index 0a5745f2..05db5b6d 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 26 refused by this layer today. + file, 73 demonstrated by a run, and 27 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 9d52c567..c9877221 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -64,6 +64,7 @@ behaviour, the source of that number is named. - [16. What this corpus does NOT yet refuse](#16-what-this-corpus-does-not-yet-refuse) - [17. Traps this corpus records](#17-traps-this-corpus-records) - [18. test_raises_sqlstate.py: which error, and which statement](#18-test_raises_sqlstatepy-which-error-and-which-statement) +- [19. test_failed_query_sentinel.py: a failed query is not a comparison](#19-test_failed_query_sentinelpy-a-failed-query-is-not-a-comparison) ## 1. How to read a test in here @@ -733,6 +734,7 @@ many times. | `test_the_readme_and_the_inventory_agree_on_what_is_refused` | README.md quotes the inventory's number, so the two cannot drift apart again | | `test_the_inventory_accounts_for_every_mode_the_run_found` | the admitted gap row is the run's total minus what is written down | | `test_the_prose_totals_match_the_counted_modes` | every sentence stating what the layer refuses today carries the counted number, not just the table | +| `test_the_two_halves_of_the_refused_sentence_sum_to_the_named_total` | TESTS.md states the split twice in one sentence, and BOTH halves are checked against the inventory's own count — the gated half alone let 26 + 47 = 73 past a named total of 72 | | `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 | @@ -1209,8 +1211,8 @@ story from #473 and #476. ## 16. 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 26 -of them.** The other 46, of which 45 were demonstrated, are listed there with the +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 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. @@ -1386,3 +1388,62 @@ one condition faithfully, and require the copy to go blind. | `test_disabling_the_sqlstate_rule_makes_the_scan_blind` | the neutering proof: a copy of the layer with `False and` prefixed, nothing renamed, goes blind while still containing the pinned text | | `test_disabling_the_statement_rule_makes_the_scan_blind` | the same for the second condition, so neither rule rests on the other's arm | | `test_the_mode_this_layer_only_narrows_is_still_listed_as_open` | `raises-catches-setup` must stay in section 3 of the mode inventory | +## 19. test_failed_query_sentinel.py: a failed query is not a comparison + +`error-swallowed-to-empty`: two queries raise, a helper turns each into the same +value, and they compare equal. The test is green and has asserted nothing about +either query. + +`lib.sh` closed this by PRODUCING the sentinel with a sequence number per failure, +`res="QUERY_ERROR.$seq"`, so two failures can never compare equal. The port had the +constant `QUERY_ERROR = "QUERY_ERROR"`, a comment claiming it was "unique per +occurrence" — which is false of a constant — and a refusal in exactly one assertion. + +Measured before this file existed, with a sentinel on both sides: + +| assertion | before | after | +| --- | --- | --- | +| `expect.hash` | refused | refused | +| `expect.text` | **passed** | refused | +| `expect.rows` | **passed** | refused | +| `expect.row_set` | **passed** | refused | +| `expect.ordered_rows` | **passed** | refused | +| `expect.num`, `at_least`, `rowcount` | refused, by their type guards | unchanged | + +So four of the five comparisons accepted two failed queries as agreement. + +**The mechanism is the refusal; `query_error()` is the second line.** A non-unique +sentinel is safe against the layer, because no comparison accepts one at all. It is +not safe against a helper that compares by hand, which is why the producer exists and +why new code should use it. The SQL-side sentinel in `test_hilbert_locality.py` +cannot use it — it is produced by `coalesce(...)` inside the query — and does not need +to, for the same reason. + +| test | what it asserts | how it could fail | +| --- | --- | --- | +| `test_the_comparison_surface_is_what_this_file_thinks_it_is` | the derivation finds the layer's `(got, want)` assertions | a renamed or removed assertion makes the arm below vacuous | +| `test_the_shape_table_covers_every_comparison_the_layer_offers` | every derived comparison has a declared valid pair | an assertion added to the layer is silently outside the arm below | +| `test_every_comparison_refuses_a_failed_query_on_either_side` | each comparison refuses a sentinel on the left and on the right | a comparison that compares instead of refusing; the arm distinguishes "refused" from "failed" | +| `test_row_set_refuses_before_it_maps_rather_than_after` | `row_set` refuses a sentinel that arrived as a cell | `row_set` reprs its rows before delegating, so a refusal only in `rows` cannot see it | +| `test_the_producer_is_unique_per_occurrence` | fifty calls are fifty distinct values, all carrying the prefix | a producer that returns a constant, which is what the comment used to claim | +| `test_the_constant_alone_is_not_unique_which_is_why_the_producer_exists` | the control: the bare constant equals itself | compared in plain Python, because the layer now refuses to compare two sentinels | +| `test_the_refusal_cannot_be_switched_off_from_the_corpus_it_polices` | sentinels minted WHILE ARMED are still refused after the global is rewritten | every conftest is imported before collection, so a module global is writable by the corpus the rule polices | +| `test_a_hardcoded_sentinel_survives_the_same_rewrite` | a sentinel no producer minted — the corpus writes three, one from inside SQL — is still refused after a rewrite | a matcher reading a rewritable global would stop seeing them | +| `test_the_ordering_premise_refuses_a_failed_reading` | `ordering_observable` refuses a failed reading on either side | with the old shared constant two failed readings were identical and it went RED; unique sentinels differ, so it passed and greenlit every ordered assertion resting on it | +| `test_a_legitimate_comparison_is_untouched` | equal text, rows, sets and numbers still pass | a refusal that also refuses real data is not a refusal | + +**Each refusal is proved load-bearing.** Removing the one call from each assertion, +one at a time, with `__pycache__` cleared between runs: + +| refusal removed from | the arm names | arms reddened | +| --- | --- | --- | +| `row_set` | `row_set COMPARED a failed query` | 2 (also the delegation arm) | +| `ordered_rows` | `ordered_rows COMPARED a failed query` | 1 | +| `rows` | `rows COMPARED a failed query` | 1 | +| `hash` | `hash COMPARED a failed query` | 1 | +| `text` | `text COMPARED a failed query` | 2 (also the hatch arm, which is phrased over `text`) | + +The cache matters: the five deleted lines are byte-identical, so three of the five +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. diff --git a/test/pytest/VACUITY_MODES.md b/test/pytest/VACUITY_MODES.md index 1317984a..fbc98802 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 | 26 | -| named in section 3, not refused | 46 | +| named in section 2, refused today | 27 | +| named in section 3, not refused | 45 | | **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 -26 of the 79, counted by section 1a's rule. Each is enforced by a mechanism, not a convention, and each has a red +27 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 | @@ -87,6 +87,7 @@ test in `test_layer.py` that fails without it. | an empty parameter set fails the run, with its own message | `empty-parametrize-is-a-silent-skip` | | 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` | Three of those were added after checking this layer against the inventory rather than reasoning about it, and all three had passed silently before: @@ -119,7 +120,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, **46 of them named below**, **49 demonstrated by a run**. 51 have a refusal already designed. +55 modes by the run's count, **45 of them named below**, **49 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 @@ -223,10 +224,21 @@ binds the exception and the body pins its SQLSTATE. See section 2. - `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. -- `error-swallowed-to-empty` — two queries raise, a helper turns each into the same - falsy value, and they compare equal. `lib.sh` closed this deliberately with a - unique `QUERY_ERROR.$seq` per failure. **The port has the sentinel constant but - nothing produces it.** +**`error-swallowed-to-empty` is now closed.** Every comparison in the layer refuses a + value carrying the `QUERY_ERROR` prefix, on either side, at any depth — so two + queries that both failed cannot compare equal, whatever a helper turned them into. + `query_error()` produces a value unique per occurrence, as `lib.sh`'s + `QUERY_ERROR.$seq` does, for the paths that compare without the layer. See + section 2 and TESTS.md section 18. + + THE EARLIER VERSION OF THIS PARAGRAPH SAID "the port has the sentinel constant but + nothing produces it", AND THAT WAS WRONG IN BOTH HALVES. Two sites did produce + sentinels by hand (`test_hilbert_locality.py`, one of them from inside SQL), and the + thing actually missing was not a producer: it was the refusal in four of the five + comparisons. `expect.text`, `expect.rows`, `expect.row_set` and `expect.ordered_rows` + each passed with a sentinel on both sides; only `expect.hash` refused. A map that + names the wrong gap is worse than one that admits it does not know, so the + measurement is recorded here rather than quietly replaced. - `guc-set-but-path-never-engaged`, `aggregate-masks-empty-fixture`, `db-derived-empty-parametrize`, `null-filter-matches-nothing`, `loop-over-zero-rows`, `assert-inside-a-loop-over-zero-rows` @@ -320,6 +332,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 26 demonstrated modes -- the ids named in section 2, +adversary. The layer is known to refuse 27 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/pgc_vacuity.py b/test/pytest/pgc_vacuity.py index 24dca22d..02dd9836 100644 --- a/test/pytest/pgc_vacuity.py +++ b/test/pytest/pgc_vacuity.py @@ -23,13 +23,82 @@ import numbers import pathlib +import itertools + 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. +# The sentinel a failed query yields, mirroring lib.sh's `res="QUERY_ERROR.$seq"`. +# +# THE COMMENT HERE USED TO CLAIM "unique per occurrence so two failing queries can +# never compare equal", WHICH WAS FALSE OF A CONSTANT. `QUERY_ERROR == QUERY_ERROR`, +# so two failures that both assigned it compared EQUAL, and a test comparing one +# failed query against another passed. Measured before the fix: `expect.text`, +# `expect.rows`, `expect.row_set` and `expect.ordered_rows` all passed with the +# sentinel on both sides. +# +# lib.sh does not have this problem because its sentinel is PRODUCED, with a +# sequence number, by the one helper every suite calls. The port had the constant +# and no producer, so uniqueness was a sentence rather than a mechanism. +# +# QUERY_ERROR stays as the PREFIX every refusal matches on. `query_error()` is the +# producer, and it is what a caller should use. QUERY_ERROR = "QUERY_ERROR" EMPTY = "EMPTY" +_query_error_seq = itertools.count(1) + + +# THE PREFIX IS BOUND AT DEFINITION TIME, in a default argument, and that is the whole +# mechanism rather than a style choice. +# +# The first version read the module global `QUERY_ERROR` in both the producer and the +# refusal. Every `conftest.py` under test/pytest/ is imported before collection, so a +# corpus file can rewrite that global -- and because BOTH sides read it, they moved +# together and the refusal matched whatever the prefix had just been set to. The arm +# that was supposed to catch this minted its sentinels AFTER rewriting, so it could not +# fail for the property it named: tautological, and @jdatcmd measured it. +# +# The faithful hatch mints while armed and rewrites afterwards, which is what a corpus +# file actually does. Measured against the first version: with the rewrite, two minted +# sentinels were COMPARED instead of refused, and a corpus file doing it end to end +# reported `1 passed` over two failed queries -- `error-swallowed-to-empty` +# reintroduced through the hatch the change claimed to close. +# +# A default argument is evaluated once, when the function is defined, and is not read +# from the module namespace afterwards. So rewriting `pgc_vacuity.QUERY_ERROR` changes +# neither what is minted nor what is refused. `QUERY_ERROR` stays exported because the +# corpus names it in literals, and it is no longer what the mechanism reads. +def query_error(detail="", _prefix=QUERY_ERROR): + """The value a failed query yields: unique per occurrence, by construction. + + Two failures can never compare equal, which is the whole mechanism -- a helper + that turns every failure into one falsy value makes "both queries failed" look + exactly like "both queries agreed". lib.sh closed this with a sequence number + per failure and this is the port of that, not of the constant. + + The sequence is per process. Under xdist each worker is its own process, so two + workers can mint the same number -- which is harmless, because a comparison only + ever happens inside one test, and the refusals below match the PREFIX rather + than any particular number. + """ + n = next(_query_error_seq) + return f"{_prefix}.{n}.{detail}" if detail else f"{_prefix}.{n}" + + +def _failed_query(v, _prefix=QUERY_ERROR): + """Is this value a failed query's sentinel? Matches the prefix, at any depth. + + A sentinel arrives as a CELL inside a row as often as it arrives as a whole + side -- `[(QUERY_ERROR,)]` is what a one-column query that failed looks like + after a helper swallowed the error -- so the walk is the point rather than a + convenience. Strings only: a tuple is walked, not tested. + """ + if isinstance(v, str): + return v.startswith(_prefix) + if isinstance(v, (list, tuple, set, frozenset)): + return any(_failed_query(x, _prefix) for x in v) + return False + # 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 = ( @@ -97,6 +166,25 @@ def __init__(self, nodeid): self.unrunnable = None # -- the recorder ------------------------------------------------------- + def _refuse_failed_query(self, name, got, want): + """Refuse a comparison where either side is a failed query. + + ONE definition, called by every comparison, so an assertion added later + inherits it instead of being the next hole. `hash` had its own copy of this + and four other assertions had none: `text`, `rows`, `row_set` (which + delegates to `rows`) and `ordered_rows` each passed with the sentinel on + both sides, which is `error-swallowed-to-empty` exactly -- two queries + raise, a helper turns each into the same value, and they compare equal. + """ + for side, v in (("left", got), ("right", want)): + if _failed_query(v): + raise VacuityError( + f"{name}: the {side} side is a failed query: {v!r}. Two failures " + f"compare equal, so this assertion cannot fail. Use " + f"query_error() so each failure is distinct, and assert the " + f"failure you expect rather than comparing two of them." + ) + def _counted(self): self.count += 1 @@ -130,6 +218,12 @@ def row_set(self, got, want, name, allow_empty=None): instruments differ, an ordered oracle could quietly be implemented as a set one and every ordering test in the tree would go silent. """ + # BEFORE the repr mapping, not after. row_set hands `rows` a list of repr + # STRINGS, and `repr(("QUERY_ERROR.1",))` is `"('QUERY_ERROR.1',)"` -- which + # does not start with the prefix, so the refusal inside `rows` cannot see a + # sentinel that arrived as a cell. Delegating an assertion does not delegate + # its refusals when the delegation transforms the data. + self._refuse_failed_query(name, got, want) self.rows(sorted(map(repr, got)), sorted(map(repr, want)), name, allow_empty=allow_empty) @@ -155,6 +249,7 @@ def ordered_rows(self, got, want, name): f"{name}: both sequences are empty, so this comparison could not " f"have failed. Use rows(..., allow_empty='why') if empty is the point." ) + self._refuse_failed_query(name, g, w) if len(set(map(repr, g))) < 2 and len(set(map(repr, w))) < 2: raise VacuityError( f"{name}: order cannot be observed in these sequences. Every element " @@ -180,6 +275,14 @@ def ordering_observable(self, forward, reverse, name): supports no ordering claim, and every ordered assertion over it is vacuous however carefully it is written. """ + # A FAILED QUERY ON EITHER SIDE, BEFORE ANYTHING ELSE. This assertion takes + # (forward, reverse) rather than (got, want), so it sat outside the refusal -- + # and making the sentinel UNIQUE turned a loud red into a silent pass here. + # Measured by @jdatcmd: with the old constant, two failed readings were + # identical and this arm went RED; with two minted sentinels they differ, so it + # went GREEN and greenlit every ordered assertion resting on the premise. That + # was the one place the producer made the layer strictly weaker than before. + self._refuse_failed_query(name, forward, reverse) f, r = list(forward), list(reverse) if not f and not r: raise VacuityError(f"{name}: both directions are empty.") @@ -218,6 +321,7 @@ def rows(self, got, want, name, allow_empty=None): correctly returned nothing. `allow_empty` takes a REASON, not a flag, so the escape hatch costs more to type than the honest assertion. """ + self._refuse_failed_query(name, got, want) if _empty(got) and _empty(want) and not allow_empty: raise VacuityError( f"{name}: both sides are empty, so this comparison could not have " @@ -236,10 +340,9 @@ def hash(self, got, want, name): 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}") + # The same definition the others use. This was the only assertion that + # refused a sentinel, and it did so with its own copy of the test. + self._refuse_failed_query(name, got, want) if _empty(got) and _empty(want): raise VacuityError(f"{name}: both hashes are empty.") self._counted() @@ -248,7 +351,8 @@ def hash(self, got, want, name): # -- text -------------------------------------------------------------- def text(self, got, want, name): - """Compare text exactly. Refuses an empty expectation.""" + """Compare text exactly. Refuses an empty expectation and a failed query.""" + self._refuse_failed_query(name, got, want) if _empty(want): raise VacuityError( f"{name}: the expected text is empty, so anything empty satisfies it." diff --git a/test/pytest/test_docs_cover_the_corpus.py b/test/pytest/test_docs_cover_the_corpus.py index c4ab613f..3743ae27 100644 --- a/test/pytest/test_docs_cover_the_corpus.py +++ b/test/pytest/test_docs_cover_the_corpus.py @@ -429,3 +429,27 @@ def test_the_prose_totals_match_the_counted_modes(expect): f"{path.name} states a refused total matching {pattern!r}") expect.num(int(m.group(1)), n, f"{path.name}: the prose total is the number of ids named") + +def test_the_two_halves_of_the_refused_sentence_sum_to_the_named_total(expect): + """TESTS.md states the split twice in one sentence: how many modes the layer + refuses, and how many it does not. Only the first half was gated. + + `selftest/350`'s TESTS.md total arm matches `This layer refuses [0-9]+`, which is the + half a change to the layer naturally updates. Closing a mode and updating that number + left "The other 47" behind, and 26 + 47 = 73 against the 72 the inventory names -- + a contradiction introduced by the very change that fixed the other half, and caught + by nothing. Reported by @jdatcmd. + + So both halves are read here, and checked against the inventory's own count of the + ids it names rather than against a number typed twice. + """ + doc = (HERE / "TESTS.md").read_text() + m = re.search(r"This layer refuses (\d+)\s*\n?of them\.\*\*\s*The other (\d+)", doc) + expect.text("found" if m else "missing", "found", + "premise: the sentence states both halves in a form this arm can read") + refused, other = int(m.group(1)), int(m.group(2)) + named = len(_named_modes()[0]) + len(_named_modes()[1]) + expect.num(refused + other, named, + "the two halves sum to the number of modes the inventory names") + expect.num(refused, len(_named_modes()[0]), + "and the refused half is the count of ids section 2 claims") diff --git a/test/pytest/test_failed_query_sentinel.py b/test/pytest/test_failed_query_sentinel.py new file mode 100644 index 00000000..430cf3d6 --- /dev/null +++ b/test/pytest/test_failed_query_sentinel.py @@ -0,0 +1,281 @@ +"""A failed query must not be comparable with another failed query (#432). + +`error-swallowed-to-empty`: two queries raise, a helper turns each into the same +value, and they compare equal. The test is green and has asserted nothing about +either query. + +`lib.sh` closed this by PRODUCING a sentinel with a sequence number per failure -- +`res="QUERY_ERROR.$seq"` -- so two failures can never compare equal. The pytest port +had the constant `QUERY_ERROR = "QUERY_ERROR"` and a comment claiming it was "unique +per occurrence", which was false of a constant, and a refusal in exactly one +assertion. Measured before this file existed, with the sentinel on both sides: + + expect.hash REFUSED + expect.text PASSED + expect.rows PASSED + expect.row_set PASSED + expect.ordered_rows PASSED + +So four of the five comparisons accepted two failed queries as agreement. + +THE MECHANISM IS THE REFUSAL, and `query_error()` is the second line rather than the +first. A non-unique sentinel is safe against the layer, because no comparison +accepts one at all; it is NOT safe against a helper that compares by hand, which is +why the producer exists and why new code should use it. +""" +import inspect + +import pgc_vacuity +from pgc_vacuity import QUERY_ERROR, VacuityError, query_error + + +# Every public assertion that compares two caller-supplied values. DERIVED from the +# signature rather than listed, so an assertion added later is covered by the arm +# below instead of being the next hole -- which is how this mode survived: `hash` +# had a refusal and the four written after it did not. +def _comparisons(): + cls = pgc_vacuity.Expect + out = [] + for name, fn in inspect.getmembers(cls, inspect.isfunction): + if name.startswith("_"): + continue + params = list(inspect.signature(fn).parameters)[1:] + # (forward, reverse) AS WELL AS (got, want). `ordering_observable` compares two + # caller-supplied readings under different parameter names, so a derivation + # keyed on "got" missed it -- and it was the one assertion the unique producer + # made WEAKER, turning a loud red into a silent pass. Reported by @jdatcmd. + if len(params) >= 2 and ( + (params[0] == "got" and params[1] in ("want", "floor")) + or (params[0], params[1]) == ("forward", "reverse")): + out.append((name, params[1])) + return sorted(out) + + +def test_the_comparison_surface_is_what_this_file_thinks_it_is(expect): + """premise: the derivation finds the assertions, so the arm below is not vacuous.""" + names = [n for n, _ in _comparisons()] + expect.at_least(len(names), 5, "the layer offers at least five (got, want) comparisons") + for required in ("hash", "text", "rows", "row_set", "ordered_rows", + "ordering_observable"): + expect.num(names.count(required), 1, f"{required} is one of them") + + +# What a VALID pair looks like for each comparison, so the sentinel can be put in +# one side and the other side stays something the assertion would otherwise accept. +# +# DECLARED, and the premise below requires it to cover every comparison the +# derivation finds. That is the anti-drift part: an assertion added to the layer +# without an entry here fails the premise, rather than being quietly skipped by an +# arm that looked like it covered everything. The first version of this arm fudged +# the shapes in a loop and never placed a sentinel in `at_least`'s floor at all -- +# it reported `at_least ACCEPTED a failed query`, which was my fixture's fault and +# not the layer's. +VALID = { + "hash": ("abc", "abc"), + "text": ("abc", "abc"), + "num": (7, 7), + "rowcount": (7, 7), + "at_least": (7, 1), + "rows": ([(1,), (2,)], [(1,), (2,)]), + "row_set": ([(1,), (2,)], [(2,), (1,)]), + "ordered_rows": ([(1,), (2,)], [(1,), (2,)]), + # Forward and reverse must DIFFER, or the assertion refuses the fixture for a + # different reason and the arm would pass without testing the sentinel. + "ordering_observable": ([(1,), (2,)], [(2,), (1,)]), +} + +# How a sentinel arrives for each: bare for a scalar comparison, and as a CELL for a +# row comparison, because that is what a one-column query that failed looks like +# after a helper swallowed the error. +def _as_side(name, sentinel): + if name in ("rows", "row_set", "ordered_rows"): + return [(sentinel,), (2,)] + return sentinel + + +def test_the_shape_table_covers_every_comparison_the_layer_offers(expect): + """premise: no comparison is silently outside the arm below.""" + missing = sorted(n for n, _ in _comparisons() if n not in VALID) + expect.num(len(missing), 0, f"every comparison has a declared valid pair; missing: {missing}") + + +def test_every_comparison_refuses_a_failed_query_on_either_side(expect): + """The arm that would have caught this mode, phrased over the whole surface. + + Two DISTINCT sentinel values, because the interned constant makes `got is want` + true and `hash` would then refuse for THAT reason -- a refusal that says nothing + about sentinels. This is the control the original `hash` arm did not have. + """ + e = pgc_vacuity.Expect("sentinel::derivation") + checked = 0 + for name, _second in _comparisons(): + fn = getattr(e, name) + good_got, good_want = VALID[name] + for side in ("left", "right"): + s = query_error(side) + got = _as_side(name, s) if side == "left" else good_got + want = _as_side(name, s) if side == "right" else good_want + try: + fn(got, want, "an arm over a failed query") + except VacuityError: + checked += 1 + except AssertionError as exc: + raise AssertionError( + f"{name} COMPARED a failed query on the {side} instead of refusing " + f"it, so a pair of failures would have compared equal: {exc}" + ) + else: + raise AssertionError( + f"{name} ACCEPTED a failed query on the {side} and passed" + ) + expect.num(checked, 2 * len(_comparisons()), + "every comparison refused the sentinel on both sides") + + +def test_row_set_refuses_before_it_maps_rather_than_after(expect): + """The delegation case, which the first version of the fix got wrong. + + `row_set` hands `rows` a list of repr STRINGS, and `repr(("QUERY_ERROR.1",))` is + `"('QUERY_ERROR.1',)"` -- it does not start with the prefix. So a refusal living + only in `rows` cannot see a sentinel that arrived as a cell. Delegating an + assertion does not delegate its refusals when the delegation transforms the data. + """ + e = pgc_vacuity.Expect("sentinel::rowset") + try: + e.row_set([(query_error(),)], [(query_error(),)], "two failed one-column queries") + except VacuityError: + expect.num(1, 1, "row_set refuses a sentinel that arrived as a cell") + else: + raise AssertionError("row_set accepted a sentinel cell") + + +def test_the_producer_is_unique_per_occurrence(expect): + """The property the old comment claimed and the constant did not have.""" + values = [query_error() for _ in range(50)] + expect.num(len(set(values)), 50, "fifty occurrences are fifty distinct values") + expect.num(sum(1 for v in values if v.startswith(QUERY_ERROR)), 50, + "and every one of them still carries the prefix the refusals match") + expect.text(query_error("no-partition").split(".")[-1], "no-partition", + "a detail survives into the value, so a log line says which query failed") + + +def test_the_constant_alone_is_not_unique_which_is_why_the_producer_exists(expect): + """The control for the arm above: the thing that was wrong, still measurable. + + Compared in plain Python rather than through `expect`, because the layer now + REFUSES to compare two sentinels -- which is the whole point, and which means an + arm about sentinel equality cannot use the assertion it is describing. The first + version of this arm did, and was refused by the guard it exists to document. + """ + expect.num(1 if QUERY_ERROR == QUERY_ERROR else 0, 1, + "the bare constant equals itself, in plain Python") + expect.num(len({QUERY_ERROR, QUERY_ERROR}), 1, + "so two failures that both assigned it would have compared equal") + expect.num(len({query_error(), query_error()}), 2, + "where two calls to the producer are two values") + + +def test_the_refusal_cannot_be_switched_off_from_the_corpus_it_polices(expect): + """Every conftest under test/pytest/ is imported before collection, so a corpus file + can rewrite this module's globals. The refusal must survive that. + + THE FIRST VERSION OF THIS ARM WAS TAUTOLOGICAL and @jdatcmd measured it. It rewrote + `pgc_vacuity.QUERY_ERROR` and THEN minted its sentinels with `query_error()`, which + read that same global -- so producer and matcher moved together and the refusal + matched whatever the prefix had just been set to. It could not fail for the property + it names. + + The faithful hatch is this one: mint while the layer is armed, rewrite afterwards, + which is what a corpus file actually does. Against the old code that COMPARED two + sentinels instead of refusing them, and an end-to-end corpus file reported + `1 passed` over two failed queries. + + `ZZZ_NOT_A_PREFIX` is in the list deliberately. `'Q'` alone cannot disarm a + prefix-reading matcher, because `'QUERY_ERROR.1'.startswith('Q')` is true -- so a + spelling that is not a prefix of the real one is the case that would have found the + hole, and it is the case the old arm never tried. + """ + e = pgc_vacuity.Expect("sentinel::hatch") + original = pgc_vacuity.QUERY_ERROR + for spelling in ("", "NOTHING_MATCHES_THIS", "Q", "ZZZ_NOT_A_PREFIX"): + a, b = query_error("a"), query_error("b") # minted WHILE ARMED + # Compared in plain Python: `expect.text` now refuses a value carrying the + # prefix, which is the point, and an arm ABOUT sentinels therefore cannot use + # the assertion it is describing on one. + expect.num(1 if a.split(".")[0] == original else 0, 1, + f"premise: minted under the real prefix ({spelling!r} case)") + pgc_vacuity.QUERY_ERROR = spelling # the corpus rewrites it after + try: + e.text(a, b, "a rewritten prefix") + except VacuityError: + expect.num(1, 1, f"the refusal survives the prefix being set to {spelling!r}") + except AssertionError: + raise AssertionError( + f"rewriting QUERY_ERROR to {spelling!r} disarmed the refusal: the two " + f"sentinels were COMPARED instead of refused" + ) + else: + raise AssertionError(f"rewriting QUERY_ERROR to {spelling!r} disarmed the refusal") + finally: + pgc_vacuity.QUERY_ERROR = original + expect.num(1 if pgc_vacuity.QUERY_ERROR == original else 0, 1, + "and the module is left as it was found") + + +def test_a_hardcoded_sentinel_survives_the_same_rewrite(expect): + """The corpus hardcodes sentinel text that no producer minted -- + `'QUERY_ERROR.empty-relation'` and `f"QUERY_ERROR.no-partition-for-{table}"` in + `test_hilbert_locality.py`, and `"QUERY_ERROR.1"` in `test_guards_pinned.py`. One of + those is built inside SQL by `coalesce(...)`, so it can never come from + `query_error()`. A matcher that read a rewritable global would stop seeing them. + """ + e = pgc_vacuity.Expect("sentinel::hardcoded") + original = pgc_vacuity.QUERY_ERROR + left = "QUERY_ERROR.empty-relation" + right = "".join(["QUERY_ERROR", ".empty-relation"]) # equal, distinct objects + expect.num(1 if left is not right else 0, 1, + "premise: the two are distinct objects, so identity guards cannot fire") + pgc_vacuity.QUERY_ERROR = "NOTHING_MATCHES_THIS" + try: + e.text(left, right, "two hardcoded sentinels after a rewrite") + except VacuityError: + expect.num(1, 1, "a hardcoded sentinel is still refused") + except AssertionError: + raise AssertionError("the hardcoded sentinel was COMPARED after the rewrite") + else: + raise AssertionError("the hardcoded sentinel was accepted after the rewrite") + finally: + pgc_vacuity.QUERY_ERROR = original + + +def test_the_ordering_premise_refuses_a_failed_reading(expect): + """The one assertion the unique producer made WEAKER before this arm existed. + + `ordering_observable` requires forward and reverse to differ. With the old shared + constant two failed readings were IDENTICAL, so it went red -- loudly, for the wrong + reason but in the right direction. With unique sentinels they differ, so it passed + and greenlit every ordered assertion resting on the premise. Measured by @jdatcmd, + and it is why the refusal is now the first thing that assertion does. + """ + e = pgc_vacuity.Expect("sentinel::ordering") + for label, fwd, rev in ( + ("both readings failed", [(query_error("f"),)], [(query_error("r"),)]), + ("one reading failed", [(query_error("f"),), (1,)], [(1,), (2,)])): + try: + e.ordering_observable(fwd, rev, "the ordering premise") + except VacuityError: + expect.num(1, 1, f"the ordering premise refuses when {label}") + except AssertionError: + raise AssertionError(f"{label}: compared instead of refused") + else: + raise AssertionError(f"{label}: the ordering premise PASSED on a failed query") + + +def test_a_legitimate_comparison_is_untouched(expect): + """The cost side. A refusal that also refuses real data is not a refusal.""" + e = pgc_vacuity.Expect("sentinel::cost") + e.text("abc", "abc", "equal text still passes") + e.rows([(1, "a")], [(1, "a")], "equal rows still pass") + e.row_set([(1,), (2,)], [(2,), (1,)], "a set comparison still ignores order") + e.num(7, 7, "equal numbers still pass") + expect.num(1, 1, "four honest comparisons passed through the new refusal")