diff --git a/CHANGELOG.md b/CHANGELOG.md index 2e000528..ae869f3a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -407,6 +407,95 @@ true until the next version shipped. ### Fixed +- A loop that never ran asserted nothing, and half of that was already refused by a + mechanism nobody had recorded covered it (#432). + + `assert-inside-a-loop-over-zero-rows` in VACUITY_MODES.md 3.5 is two shapes. + + **Already refused.** When a loop's body holds a test's ONLY counted assertions, a + zero-trip loop leaves the count at 0 and `pytest_runtest_call` raises `VacuityError`. + Measured on a planted test rather than read off the hook: the zero-trip case fails + with "made no counted assertion" and the same loop with one row passes. + + **Was open.** When the test ALSO asserts outside the loop, the count is non-zero, the + test passes, and the loop's assertions simply never ran. That is the shape a query + returning no rows produces, and the shape a glob matching nothing produces. + + `test_loop_coverage_premise.py` requires a cardinality premise for exactly that shape. + THE POPULATION, measured over the whole corpus before the arm was written: 20 loops + non-empty by construction and so unable to be zero-trip, 0 in the already-refused + shape, and **2 at risk** -- both of which already carried a premise. The arm is green + on arrival, which is the point rather than a weakness: the property was true and + nothing was holding it there, so what this catches is the third one. + + **IT NARROWS RATHER THAN CLOSES, and 3.5 names the residual.** The honest requirement + is a premise bounding the cardinality of THIS iterable; what is enforced is a counted + assertion outside the loop taking `len(...)` of something. `test_harness_deps.py`'s + loop iterates `sorted(found)` while its premise bounds `len(files)`, because `found` is + built from `files` in a preceding loop -- so a rule demanding the names match would + reject correct code, which is how a guard gets switched off. The residual is a loop + whose premise bounds the wrong collection: a reviewer catches it, a sweep does not. + + Section 5 gains entry 6, struck and anchored to its mode id, which is the first use of + the rule the previous commit added. + +- Section 5 of VACUITY_MODES.md, the "what to add next" list, is checked (#432). + + 1a, 2 and 3 are all compared against the mode ids on disk. Section 5 was prose, and + it was **wrong**: entry 1 still said "the constant exists and nothing writes it" long + after `query_error()` existed and `test_failed_query_sentinel.py` carried ten arms + over it, including the producer's uniqueness and the constant's non-uniqueness that + is the reason the producer exists. + + **That is the most expensive place in the document for a stale sentence**, because + its only reader is someone about to build something. The near-miss one document over + is what it costs: a bad enumeration of `test/selftest/340` made an existing block + look like a coverage gap, and the duplicate was written and proven to discriminate + before the duplication was noticed. + + **What is checkable is the anchor, not the work.** Entry 1's work landed under four + test names, none of them the one the entry proposed, so asking whether the NAMED test + exists would have passed and said nothing. Two arms hold the list instead: + + - every entry names at least one mode id, so it is tied to the inventory at all. + Entry 1 named none, which is exactly how it stayed wrong. + - no UN-STRUCK entry names an id section 2 already claims as refused. An entry whose + id has reached section 2 is done by the document's own accounting, whatever the + test ended up being called. + + Entry 1 is struck and anchored to `error-swallowed-to-empty`, and it records what the + entry got wrong rather than replacing it: both halves of "the constant exists and + nothing writes it" were false -- two sites already minted sentinels by hand, and the + missing thing was the REFUSAL in four of the five comparisons. + + **Fixing it made the second arm vacuous on this document**, because with every entry + struck there is nothing left to refuse. The planted fixture beside it is therefore + the whole of its evidence, and the document says so rather than leaving it implied: a + two-entry fixture where moving the open entry's id into section 2 must be named, with + the clean control beside it. + + The totals do not move -- 28 refused, 44 not refused, 72 named -- because the mode was + already counted as refused. Only the list that tells the next person what to do was + wrong. + + **And 3.6 now records measured populations, so the next entry is chosen on evidence.** + Section 5's new rule is that an entry must name a mode id, which makes WHICH id worth + measuring. Four of 3.6's were counted by AST scan over the whole corpus: + `truthy-cursor-from-execute` has 7 sites and **all are benign** -- every one is + `x = cur.execute(...)`, idiomatic in psycopg3, and **zero** branch on it, which is the + dangerous form; `empty-query-string-succeeds` has 0; the multistatement mode has 1, a + setup that fetches nothing with a `count(*)` premise right after it; and the + server-cursor rowcount mode has 2, both legitimate. So all four are PROSPECTIVE, and a + guard for any of them would be insurance rather than a closure. Recorded because a + refusal with no population has not refused anything, and 1a counts section 2 as + "refused today". + + One instrument defect of mine, caught by that fixture: the parser matched the + SECTION HEADING as an entry. `re.split(r"^## ")` leaves a chunk beginning "5. What to + add next", which the numbered-item pattern also matches -- inventing an entry 5 that + is the title and colliding with the real entry 5. The fixture reported 3 entries in a + two-entry document, which is how it was found. + - A count `grep` never produced no longer reads as "present" (#929). #922 replaced roughly 28 `producer | grep -q PAT` tests with diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 5b187784..708a83fb 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -69,6 +69,7 @@ behaviour, the source of that number is named. - [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) - [23. test_mutation_ledger.py: which checks have ever been red](#23-test_mutation_ledgerpy-which-checks-have-ever-been-red) +- [24. test_loop_coverage_premise.py: a loop that never ran asserted nothing](#24-test_loop_coverage_premisepy-a-loop-that-never-ran-asserted-nothing) ## 1. How to read a test in here @@ -840,6 +841,27 @@ many times. | `test_the_anchor_rule_drops_punctuation_and_keeps_underscores` | GitHub's derivation, on the heading the defect was found in | | `test_an_anchor_that_strips_the_underscores_is_caught` | the exact broken link that shipped, with a control | | `test_every_in_document_link_in_this_directory_reaches_a_heading` | every contents-list link resolves, with a coverage premise | +| `test_the_next_steps_list_is_anchored_to_the_inventory` | every section 5 entry names a mode id, so the entry can be checked at all | +| `test_no_open_next_step_names_work_the_document_calls_done` | an un-struck entry whose id reached section 2 is stale work to do | +| `test_a_stale_next_step_is_caught_on_a_fixture` | **removal proof**: the shape, planted, with the control beside it | + + +**Section 5 was the last unchecked part of VACUITY_MODES.md, and it was wrong (#432).** +1a, 2 and 3 are all compared against the ids on disk; "what to add next" was prose. +Entry 1 still said *"the constant exists and nothing writes it"* long after +`query_error()` existed and `test_failed_query_sentinel.py` had ten arms over it — the +most expensive place in the document for a stale sentence, because its only reader is +someone about to build something. The near-miss one document over is the argument: a +bad enumeration of `test/selftest/340` made an existing block look like a gap, and the +duplicate was written and proven to discriminate before anyone noticed. + +**What is checkable is the anchor, not the work.** Entry 1's work landed under four +test names, none of them the one the entry proposed, so asking whether the NAMED test +exists would have passed and said nothing. So the arms check that every entry names a +mode id, and that no un-struck entry names an id section 2 already claims. + +With every entry now struck, the second arm has nothing to refuse on the real +document. That is what the fixture arm is for. The five fixture arms exist because everything above them passes on a healthy tree, which is exactly what a guard that does nothing also does. They run the identical @@ -1340,6 +1362,7 @@ fixtures are read off `conftest.py` rather than named in the classifier. | `test_a_cluster_test_still_needs_the_driver` | **control**: deferring made the IMPORT lazy, not the database optional | | `test_conftest_imports_no_database_driver_at_module_scope` | the regression named in one line, for whoever edits conftest next | | `test_the_declaration_is_exactly_the_database_free_half` | `NO_CLUSTER` equals the property, both ways, so an undeclared database-free file is named | +| `test_the_declaration_names_each_file_once` | the list's cardinality, which `membership_report`'s set comparison cannot see. Measured: pytest deduplicates the paths, so the cost is the job's own printed file count, not a double run | | `test_the_partition_accounts_for_every_file_in_the_corpus` | **premise**: every file lands in exactly one bucket, and neither bucket is the whole corpus | | `test_the_cluster_fixtures_are_read_off_conftest_rather_than_named_here` | the roots of the property are derived from `conftest.py`, not typed | | `test_the_classifier_tells_a_plain_file_from_one_that_requests_a_cluster` | the base case and its control, over a fixture corpus | @@ -2071,3 +2094,55 @@ If they disagree, one was edited by hand. `suites_not_covered` is 250 of 251, so gate cannot refuse a new check in 250 suites — a real limit, counted rather than hidden, which falls as suites are seeded. + +## 24. test_loop_coverage_premise.py: a loop that never ran asserted nothing + +**Why this file exists.** `assert-inside-a-loop-over-zero-rows` in VACUITY_MODES.md 3.5 +is two shapes, and the layer already refused one of them without anyone recording that +it did. + +**The half already refused.** When a loop's body holds the test's ONLY counted +assertions, a zero-trip loop leaves the count at 0 and `pytest_runtest_call` raises +`VacuityError`. Measured on a planted test rather than read off the hook: + +``` +only assertion inside a zero-trip loop VacuityError: made no counted assertion +the same loop with one row 1 passed +``` + +**The half that was open.** When the test *also* asserts outside the loop, the count is +non-zero, the test passes, and the loop's assertions simply never ran. Nothing noticed. +That is the shape a query returning no rows produces, and the shape a glob matching +nothing produces. + +**The population, measured before the arm was written:** + +| shape | loops | state | +| --- | ---: | --- | +| non-empty by construction (literal, `range`, local literal) | 20 | cannot be zero-trip | +| derived, loop holds the only assertions | 0 | already refused | +| derived, **with** assertions outside the loop | 2 | at risk, now guarded | + +Both at-risk loops already carried a premise, so this arm is **green on arrival**. That +is the point rather than a weakness: the property was true of the corpus and nothing was +holding it there, so what this catches is the third one. It is not insurance against an +imagined shape — it has a population of two and locks it in. + +| test | asserts | +| --- | --- | +| `test_every_at_risk_loop_carries_a_coverage_premise` | the corpus itself: every derived loop carrying an assertion has a cardinality premise | +| `test_the_sweep_finds_the_loops_it_is_meant_to_police` | **premise**: the sweep classified loops, because one that parses nothing reports no offenders either | +| `test_a_loop_with_no_premise_is_caught` | **removal proof**: the real shape with the premise removed, with the clean control beside it | +| `test_a_bounded_loop_needs_no_premise` | a literal, a `range` and a local dict's `.items()` are all exempt, because demanding a premise there would be noise a reader edits away | +| `test_the_layer_already_refuses_a_loop_holding_every_assertion` | the measured half, so this file does not claim the whole mode — and that the sweep deliberately skips that shape rather than double-reporting it | + +### Why the rule is looser than the property, and what is left + +The honest requirement is *a premise bounding the cardinality of **this** iterable*. What +is enforced is *a counted assertion outside the loop that takes `len(...)` of something*. + +The two differ, and the reason is dataflow. `test_harness_deps.py`'s loop iterates +`sorted(found)` while its premise bounds `len(files)` — `found` is built from `files` in +a preceding loop. A rule that demanded the names match would reject correct code, which +is how a guard gets switched off. **So the residual is a loop whose premise bounds the +wrong collection**, which a reviewer catches and a sweep does not. 3.5 names it. diff --git a/test/pytest/VACUITY_MODES.md b/test/pytest/VACUITY_MODES.md index eff7ef88..74b6acf1 100644 --- a/test/pytest/VACUITY_MODES.md +++ b/test/pytest/VACUITY_MODES.md @@ -288,6 +288,42 @@ binds the exception and the body pins its SQLSTATE. See section 2. `db-derived-empty-parametrize`, `null-filter-matches-nothing`, `loop-over-zero-rows`, `assert-inside-a-loop-over-zero-rows` + **`assert-inside-a-loop-over-zero-rows` is NARROWED, and half of it was already + closed by a mechanism nobody had noticed covered it (#432).** The mode is two shapes: + + *Already refused.* When a loop's body holds the test's ONLY counted assertions, a + zero-trip loop leaves the count at 0 and `pytest_runtest_call` raises `VacuityError`. + Measured on a planted test rather than read off the hook -- + + only assertion inside a zero-trip loop VacuityError: made no counted assertion + the same loop with one row 1 passed + + *Was open.* When the test ALSO asserts outside the loop the count is non-zero, the + test passes, and the loop's assertions simply never ran. `test_loop_coverage_premise.py` + now requires a cardinality premise for exactly that shape. + + THE POPULATION, measured over the whole corpus before the arm was written: + + | shape | loops | state | + | --- | ---: | --- | + | non-empty by construction (literal, range, local literal) | 20 | cannot be zero-trip | + | derived, loop holds the only assertions | 0 | already refused | + | derived, WITH assertions outside the loop | 2 | **at risk**, now guarded | + + Both at-risk loops already carried a premise, so the arm is green on arrival. That is + the point rather than a weakness: the property was true and nothing held it there. + + **WHY IT NARROWS RATHER THAN CLOSES.** The honest requirement is "a premise bounding + the cardinality of THIS iterable". What is enforced is "a counted assertion outside + the loop that takes `len(...)` of something". `test_harness_deps.py`'s loop iterates + `sorted(found)` while its premise bounds `len(files)` -- `found` is built from `files` + in a preceding loop -- so a rule demanding the names match would reject correct code. + The residual is a loop whose premise bounds the wrong collection, which a reviewer + catches and a sweep does not. + + `loop-over-zero-rows` is untouched: it is the sibling about a loop that produces no + assertion at all, which the whole-run guard covers only when the test has no others. + ### 3.6 psycopg's typed results introduce their own - `sql-null-to-python-none`, `none-conflates-null-no-row-and-missing-column` @@ -297,6 +333,27 @@ binds the exception and the body pins its SQLSTATE. See section 2. - `executemany-returning-fetchall-sees-only-the-first-batch`, `server-cursor-rowcount-is-not-a-row-count`, `empty-query-string-succeeds` +**MEASURED POPULATIONS, so the next entry is chosen on evidence (#432).** Section 5's +new rule is that an entry must name a mode id, which makes *which* id worth measuring +rather than guessing. Four of 3.6's were counted over the whole corpus by AST scan: + +| mode | sites | what they are | +| --- | ---: | --- | +| `truthy-cursor-from-execute` | 7 | **all benign.** Every one is `x = cur.execute(...)`, which is idiomatic psycopg3 — `execute` returns the cursor. **Zero** branch on it (`if`, `while`, `assert`), which is the dangerous form. | +| `empty-query-string-succeeds` | 0 | no `execute()` on an empty or whitespace literal anywhere. | +| `multistatement-execute-positions-on-the-first-result` | 1 | `test_saop_element_pushdown.py`, a three-statement SETUP that fetches nothing. The arm immediately after asserts `count(*) = 40000`, so the INSERT is proven to have run. | +| `server-cursor-rowcount-is-not-a-row-count` | 2 | **both legitimate.** A `DELETE`'s `rowcount` with an `at_least` premise on it, and a field on a stub cursor class. | + +**So all four are prospective.** A guard for any of them would be insurance against a +shape the corpus has not yet written, not a closure of one it has — and it should say +so, the way `test_the_empty_plan_refusal_precedes_the_arms_it_protects` does. + +That is not an argument against writing them. It is an argument against writing them +and calling the mode closed: the counting rule in 1a treats section 2 as "refused +today", and a refusal with no population has not refused anything yet. The cheapest of +the four is the branching form of the first, because the dangerous spelling is distinct +from the benign one and a planted fixture separates them in two lines. + The enumerating agent's own summary is worth keeping: psycopg **fixes** the half of issue #418 where an error read as empty, because a failed statement raises and `[]` can only mean zero rows. That improvement is exactly what will tempt a port to drop @@ -354,8 +411,18 @@ exit 4, and no tests run at all. 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. +1. ~~`test_expect_query_error_sentinel_is_unique_per_failure` — closes + `error-swallowed-to-empty`.~~ **Done, and this entry was wrong about the gap.** It + said "the constant exists and nothing writes it", and both halves were false: + `test_hilbert_locality.py` already minted sentinels by hand, one of them from inside + SQL, and the thing actually missing was not a producer but the REFUSAL in four of the + five comparisons — `expect.text`, `rows`, `row_set` and `ordered_rows` each passed + with a sentinel on both sides, and only `expect.hash` refused. 3.2 records that + measurement rather than quietly replacing it. `query_error()` now mints a value + unique per occurrence, as `lib.sh`'s `QUERY_ERROR.$seq` does, and + `test_failed_query_sentinel.py` holds ten arms over it — including the producer's + uniqueness and the constant's NON-uniqueness, which is the reason the producer + exists at all. 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 @@ -381,9 +448,41 @@ Each entry names the red test to write first. remains, measured: a comprehension or a tuple instead of a `for`, and a helper defined in another file. Both are ordinary Python. The refused count therefore did not move. -The three that turned a whole run green rather than one test are done. What remains -is per-assertion work, so the ordering matters less: take the sentinel first, since -the constant already exists and nothing writes it. +6. ~~`test_every_at_risk_loop_carries_a_coverage_premise` — narrows + `assert-inside-a-loop-over-zero-rows`.~~ **Done, and it NARROWS rather than closes** + — 3.5 has the measurement and names the residual. Half the mode was already refused + by `pytest_runtest_call`, which fails a test that counted nothing; the half that was + open is a loop whose assertions sit alongside others outside it. Population measured + before building: 20 loops non-empty by construction, 0 in the already-refused shape, + **2 at risk** and both already compliant. + +**Every entry on this list is now struck, and the list is checked (#432).** Section 5 +was the one part of this document with no mechanism: 1a, 2 and 3 are all compared +against the ids on disk, and this was prose. Entry 1 stayed wrong long after the work +landed, which is the most expensive place in the document for a stale sentence — its +only reader is someone about to build something. + +Two arms in `test_docs_cover_the_corpus.py` hold it now: + +- every entry names at least one mode id, so it is tied to the inventory at all. + Entry 1 named none, which is exactly how it stayed wrong: there was nothing to + check it against. +- no UN-STRUCK entry names an id section 2 already claims as refused. An entry whose + id has reached section 2 is done by this document's own accounting, whatever the + test ended up being called. + +**What is not checkable, stated rather than implied.** "Has this work been done" is not +mechanical — entry 1's work landed under four test names, none of them the one the +entry proposed, so asking whether the NAMED test exists would have passed and said +nothing. The id is the only durable anchor. + +And with every entry struck, the second arm has nothing to refuse on this document, so +it is the planted fixture beside it that keeps it honest: a two-entry document where +moving the open entry's id into section 2 must be caught. A sweep over an empty +population reports the same clean answer as a correct one. + +When the next mode is taken, add it here with its id, un-struck, and the arms will +hold the entry to the inventory from then on. ## 6. What this document cannot tell you diff --git a/test/pytest/test_docs_cover_the_corpus.py b/test/pytest/test_docs_cover_the_corpus.py index 86240705..1d8590b4 100644 --- a/test/pytest/test_docs_cover_the_corpus.py +++ b/test/pytest/test_docs_cover_the_corpus.py @@ -655,3 +655,118 @@ def test_every_in_document_link_in_this_directory_reaches_a_heading(expect): f"every in-document link in {doc.name} reaches a heading") expect.at_least(total_links, 15, "premise: and it parsed links rather than finding none") + + +# --------------------------------------------------------------------------- +# Section 5's "what to add next" list must not name work that is already done. +# +# WHY. Section 5 is the one part of this document whose only reader is someone about +# to BUILD something, and it is the only part with no mechanism. Sections 1a, 2 and 3 +# are all checked against the ids on disk; section 5 was prose. Entry 1 said "the +# constant exists and nothing writes it" long after `query_error()` existed and +# `test_failed_query_sentinel.py` had ten arms over it, so the next person to take the +# list would have built something that was already there. +# +# THAT ALMOST HAPPENED FOR REAL, one document over. A bad enumeration of +# `test/selftest/340` made an existing block look like a coverage gap, and the +# duplicate was written and proven to discriminate before the duplication was noticed +# (#432). A stale "what to add next" is the same defect with the wrong answer written +# down in advance. +# +# WHAT IS CHECKABLE, and what is not. "Has this work been done" is not mechanical: +# entry 1's work landed under four different test names, so asking whether the NAMED +# test exists would have passed and said nothing. What is mechanical is the anchor: +# +# * every entry names at least one mode id, so it is tied to the inventory at all +# * an un-struck entry's ids are in section 3 (not refused) and NOT in section 2 +# +# An entry whose id has reached section 2 is done by the document's own accounting, +# whatever it is called in the corpus. And an entry naming no id cannot be checked +# against anything, which is how entry 1 stayed wrong. +# --------------------------------------------------------------------------- + +_NEXT_ITEM = re.compile(r"^(\d+)\. (.*?)(?=^\d+\. |\Z)", re.M | re.S) + + +def _next_steps_entries(text): + """-> [(number, struck, {mode ids})] for section 5's numbered list.""" + body = "" + for chunk in re.split(r"^## ", text, flags=re.M): + if chunk.startswith("5."): + # DROP THE HEADING LINE. The section is "## 5. What to add next", and with + # the "## " split away the heading itself begins "5. " at the start of the + # string -- which the item pattern matches, inventing an entry 5 that is + # the title. It collided with the real entry 5 and reported 3 items in a + # two-item fixture, which is how it was caught. + body = chunk.split("\n", 1)[1] if "\n" in chunk else "" + break + out = [] + for m in _NEXT_ITEM.finditer(body): + item = m.group(2) + out.append((int(m.group(1)), "~~" in item, set(MODE_ID.findall(item)))) + return out + + +def test_the_next_steps_list_is_anchored_to_the_inventory(expect): + """Every entry names a mode id, so the entry can be checked at all.""" + entries = _next_steps_entries(MODES_DOC.read_text()) + expect.at_least(len(entries), 4, "premise: the list was parsed, not missed") + unanchored = sorted(n for n, _struck, ids in entries if not ids) + expect.text(", ".join(str(n) for n in unanchored) or "none", "none", + "every entry in section 5 names at least one mode id") + + +def test_no_open_next_step_names_work_the_document_calls_done(expect): + """An un-struck entry whose id has reached section 2 is stale. + + Section 2 is "refused today". An entry still listed as work to do, naming an id + the document itself has moved into section 2, sends the next reader to build + something this document says exists. + """ + refused, not_refused, _ = _named_modes_in(MODES_DOC.read_text()) + entries = _next_steps_entries(MODES_DOC.read_text()) + expect.at_least(len(refused), 20, "premise: section 2's ids were found") + + stale = sorted( + "%d:%s" % (n, i) + for n, struck, ids in entries if not struck + for i in sorted(ids) if i in refused + ) + expect.text(", ".join(stale) or "none", "none", + "no open entry names an id section 2 already claims as refused") + + +def test_a_stale_next_step_is_caught_on_a_fixture(expect): + """The removal proof, on a document built to be wrong — and its control. + + Without the control, a parser that finds no entries reports the same clean + answer as a correct list. + """ + doc = ( + "## 1a. Counting\n\n" + "## 2. Refused\n\n`one-two-three` is refused.\n\n" + "## 3. Not refused\n\n`four-five-six` is not.\n\n" + "## 5. What to add next, in order\n\n" + "1. `write_something` — closes `four-five-six`.\n" + "2. ~~`write_other` — closes `one-two-three`.~~ **Done.**\n" + ) + entries = _next_steps_entries(doc) + expect.num(len(entries), 2, "premise: both fixture entries were parsed") + expect.text(repr([(n, st) for n, st, _ in entries]), "[(1, False), (2, True)]", + "and the strike-through is what marks one done") + + refused, _, _ = _named_modes_in(doc) + stale = ["%d:%s" % (n, i) for n, st, ids in entries if not st + for i in sorted(ids) if i in refused] + expect.text(", ".join(stale) or "none", "none", + "control: the open entry names an UNrefused id, so it is not stale") + + # Now move the open entry's id into section 2, which is what "done" looks like. + moved = doc.replace("`four-five-six` is not.", "moved away.").replace( + "`one-two-three` is refused.", "`one-two-three` and `four-five-six` are refused.") + refused2, _, _ = _named_modes_in(moved) + entries2 = _next_steps_entries(moved) + stale2 = ["%d:%s" % (n, i) for n, st, ids in entries2 if not st + for i in sorted(ids) if i in refused2] + expect.text(", ".join(stale2), "1:four-five-six", + "and an open entry whose id reached section 2 is named") diff --git a/test/pytest/test_harness_deps.py b/test/pytest/test_harness_deps.py index 2efed35d..cf8d6917 100644 --- a/test/pytest/test_harness_deps.py +++ b/test/pytest/test_harness_deps.py @@ -76,6 +76,8 @@ # hands cluster-bound file names to pytest, so it needs what they need. "test_harness_deps_classifier.py", "test_writes_wrote_rows.py", + # An AST sweep plus `inspect`, so it needs neither a cluster nor the driver. + "test_loop_coverage_premise.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 @@ -532,6 +534,39 @@ def membership_report(directory=None, declared=None): return "[]" if not bad else "[%d: %s]" % (len(bad), " ".join(bad)) +def test_the_declaration_names_each_file_once(expect): + """`membership_report` compares SETS, so it cannot see a name listed twice. + + The list is the artifact, not the set: CI builds its file list with + `" ".join(NO_CLUSTER)`, and the job prints how many files it is running. A + duplicate makes that printed count one too high. + + MEASURED, because the first version of this docstring also claimed the file would + RUN TWICE and that is false: 12 entries and 13 entries both give `215 passed`, + because pytest deduplicates identical paths on its command line. So the cost is a + wrong number in the job's own output, not wasted work -- which still matters here, + since that printed count is the only place a reader learns how wide the + database-free job is. + + Found by a count that did not reconcile: the declaration said 13 and + `job_runnable` said 12 while both set differences were empty, which is only + possible if a name appears twice. + + A set comparison hiding a duplicate is the same shape as a check-name collision + folding two ledger rows into one: whenever the mechanism compares sets, the + cardinality needs its own arm. + """ + seen, dupes = set(), [] + for name in NO_CLUSTER: + if name in seen: + dupes.append(name) + seen.add(name) + expect.text(", ".join(sorted(set(dupes))) or "none", "none", + "no file is declared twice in NO_CLUSTER") + expect.num(len(NO_CLUSTER), len(seen), + "and the list's length is its number of distinct names") + + def _main(argv): """The gate's entry point. The pytest corpus is not in `SUITES`, so the arms below run nowhere the gate can see them; selftest 350 runs this instead.""" diff --git a/test/pytest/test_loop_coverage_premise.py b/test/pytest/test_loop_coverage_premise.py new file mode 100644 index 00000000..2d40b244 --- /dev/null +++ b/test/pytest/test_loop_coverage_premise.py @@ -0,0 +1,221 @@ +"""A loop that never runs asserts nothing, and the run still reports a pass. + +WHAT THIS CLOSES, AND WHAT IT DOES NOT. `assert-inside-a-loop-over-zero-rows` in +VACUITY_MODES.md 3.5 is TWO shapes, and the layer already refuses one of them. + +**Already refused.** If a loop's body holds the test's ONLY counted assertions, a +zero-trip loop leaves the count at 0 and `pytest_runtest_call` raises `VacuityError`. +Measured, on a planted test rather than by reading the hook: + + only assertion inside a zero-trip loop VacuityError: made no counted assertion + the same loop with one row 1 passed + +So that half needs nothing, and this file does not pretend to add it. + +**Still open, and what this file is for.** When the test ALSO asserts outside the +loop, the count is non-zero, the test passes, and the loop's assertions simply never +ran. Nothing notices. That is the shape a query returning no rows produces, and the +one a glob matching nothing produces. + +THE POPULATION, measured over the whole corpus before this was written: + + non-empty by construction (literal, range, local literal) 20 cannot be zero-trip + derived, loop holds the only assertions 0 already refused + derived, WITH assertions outside the loop 2 AT RISK + +Both at-risk loops already carry a premise, so this arm goes green on arrival. That is +the point rather than a weakness: the property is true today and nothing was holding it +there, so the next loop added without a premise is what this catches. It is not +insurance against an imagined shape -- it has a population of two and locks it in. + +WHY THE RULE IS LOOSER THAN THE PROPERTY, said plainly. The honest requirement is "a +premise bounding the cardinality of THIS iterable". What is enforced is "a counted +assertion outside the loop that takes `len(...)` of something". The two differ, and the +reason is dataflow: `test_harness_deps.py`'s loop iterates `sorted(found)` while its +premise bounds `len(files)` -- `found` is built FROM `files` inside the loop that +precedes it. Matching premise to iterable through that needs more than a name +comparison, and a rule that demanded the names match would reject the correct code. +So this checks that a cardinality premise EXISTS, and a reviewer checks that it is the +right one. +""" + +import ast +import pathlib + +HERE = pathlib.Path(__file__).resolve().parent + + +def _counted(node): + """An `expect.(...)` call. `cannot_run` is excluded: it declares a test + unrunnable rather than concluding anything, so it is not a conclusion to count.""" + return (isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute) + and isinstance(node.func.value, ast.Name) + and node.func.value.id == "expect" + and node.func.attr != "cannot_run") + + +def _nonempty_by_construction(fn, iterable): + """True when the iterable cannot be empty, so a premise would assert nothing. + + A literal with elements, a `range(...)`, or a name bound in this function to a + non-empty literal -- including through `.items()`, which is how the corpus spells + a table of cases. Twenty of the corpus's twenty-two such loops are this. + """ + if isinstance(iterable, (ast.List, ast.Tuple, ast.Set)) and iterable.elts: + return True + if (isinstance(iterable, ast.Call) and isinstance(iterable.func, ast.Name) + and iterable.func.id == "range"): + return True + target = None + if (isinstance(iterable, ast.Call) and isinstance(iterable.func, ast.Attribute) + and iterable.func.attr in ("items", "keys", "values")): + target = iterable.func.value + elif isinstance(iterable, ast.Name): + target = iterable + if isinstance(target, ast.Name): + for node in ast.walk(fn): + if isinstance(node, ast.Assign) and any( + isinstance(t, ast.Name) and t.id == target.id for t in node.targets): + value = node.value + if isinstance(value, ast.Dict) and value.keys: + return True + if isinstance(value, (ast.List, ast.Tuple, ast.Set)) and value.elts: + return True + return False + + +def _has_cardinality_premise(fn, loop): + """A counted assertion OUTSIDE the loop whose arguments take `len(...)`.""" + inside = {id(n) for n in ast.walk(loop)} + for node in ast.walk(fn): + if id(node) in inside or not _counted(node): + continue + for arg in node.args: + for sub in ast.walk(arg): + if (isinstance(sub, ast.Call) and isinstance(sub.func, ast.Name) + and sub.func.id == "len"): + return True + return False + + +def unpremised_loops(directory): + """-> ["file:line in test_name", ...] for every at-risk loop with no premise.""" + out = [] + for path in sorted(pathlib.Path(directory).glob("test_*.py")): + try: + tree = ast.parse(path.read_text(encoding="utf-8")) + except SyntaxError: + continue + for fn in [n for n in ast.walk(tree) if isinstance(n, ast.FunctionDef) + and n.name.startswith("test_")]: + counted_in_fn = [c for c in ast.walk(fn) if _counted(c)] + for loop in [n for n in ast.walk(fn) + if isinstance(n, (ast.For, ast.AsyncFor))]: + inner = [c for c in ast.walk(loop) if _counted(c)] + if not inner: + continue + if _nonempty_by_construction(fn, loop.iter): + continue + # The loop holding every assertion is already refused at runtime. + if len(counted_in_fn) == len(inner): + continue + if not _has_cardinality_premise(fn, loop): + out.append(f"{path.name}:{loop.lineno} in {fn.name}") + return out + + +def test_every_at_risk_loop_carries_a_coverage_premise(expect): + """The corpus itself. Green on arrival, and that is the point.""" + offenders = unpremised_loops(HERE) + expect.text(", ".join(offenders) or "none", "none", + "every derived loop carrying an assertion has a cardinality premise") + + +def test_the_sweep_finds_the_loops_it_is_meant_to_police(expect): + """The coverage premise this file's own sweep needs. + + A sweep that parses nothing reports no offenders, which is exactly what a clean + corpus reports. So count the loops it classified, not just the ones it rejected. + """ + total = 0 + for path in sorted(HERE.glob("test_*.py")): + try: + tree = ast.parse(path.read_text(encoding="utf-8")) + except SyntaxError: + continue + for fn in [n for n in ast.walk(tree) if isinstance(n, ast.FunctionDef)]: + for loop in [n for n in ast.walk(fn) + if isinstance(n, (ast.For, ast.AsyncFor))]: + if any(_counted(c) for c in ast.walk(loop)): + total += 1 + expect.at_least(total, 15, + "premise: the sweep found loops carrying assertions to classify") + + +def test_a_loop_with_no_premise_is_caught(tmp_path, expect): + """The removal proof, with the control beside it. + + The fixture is the real shape with one property removed -- the premise -- rather + than an empty file, because an empty file is caught by a sweep that does nothing. + """ + bad = tmp_path / "test_bad.py" + bad.write_text( + "def test_x(expect):\n" + " rows = query()\n" + " expect.num(1, 1, 'something outside the loop')\n" + " for r in rows:\n" + " expect.num(r, 1, 'never runs when rows is empty')\n", + encoding="utf-8") + expect.text(", ".join(unpremised_loops(tmp_path)), "test_bad.py:4 in test_x", + "a derived loop with no cardinality premise is named") + + good = tmp_path / "test_good.py" + good.write_text( + "def test_y(expect):\n" + " rows = query()\n" + " expect.at_least(len(rows), 1, 'premise: the query returned rows')\n" + " for r in rows:\n" + " expect.num(r, 1, 'runs at least once')\n", + encoding="utf-8") + bad.unlink() + expect.text(", ".join(unpremised_loops(tmp_path)) or "none", "none", + "control: the same loop with a premise is clean") + + +def test_a_bounded_loop_needs_no_premise(tmp_path, expect): + """A literal cannot be empty, so demanding a premise would be noise. + + Three spellings, because the corpus uses all three and a rule that rejected any of + them would be edited away rather than obeyed. + """ + for name, iterable in (("lit", "[1, 2]"), ("rng", "range(3)"), + ("dct", "cases.items()")): + f = tmp_path / f"test_{name}.py" + pre = " cases = {'a': 1}\n" if name == "dct" else "" + f.write_text( + f"def test_{name}(expect):\n{pre}" + " expect.num(1, 1, 'outside')\n" + f" for r in {iterable}:\n" + " expect.num(1, 1, 'inside')\n", + encoding="utf-8") + expect.text(", ".join(unpremised_loops(tmp_path)) or "none", "none", + f"a loop over {name} is non-empty by construction") + f.unlink() + + +def test_the_layer_already_refuses_a_loop_holding_every_assertion(expect): + """The measured half, so this file does not claim the whole mode. + + `pytest_runtest_call` fails a test whose counted-assertion count is 0, which is + what a zero-trip loop leaves behind when it holds them all. Read off the hook + rather than re-run inside pytest: the behaviour is pinned by `test_layer.py`, and + what matters here is that THIS file's sweep deliberately skips that shape. + """ + import inspect + import pgc_vacuity + src = inspect.getsource(pgc_vacuity.pytest_runtest_call) + expect.at_least(src.count("count == 0"), 1, + "the layer fails a test that counted nothing") + mine = inspect.getsource(unpremised_loops) + expect.at_least(mine.count("len(counted_in_fn) == len(inner)"), 1, + "and this sweep skips that shape rather than double-reporting it")