diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 6fd59ece..84e5ac21 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -239,6 +239,46 @@ checkout with nothing installed — it asserts the field is read, that the read reaches the exit status, that the override is conditional, and that the two harnesses agree on 67. Against the pre-fix layer it reddens six arms. + +### An A/B whose arms agree measures nothing + +`expect.differ(a, b, name)` is the assertion `mutation-arm-unobservable` says nobody +writes. Before it the layer had **eight** helpers asserting equality and **one** +asserting inequality — `ordering_observable`, specific to a forward/reverse pair — so +the general case was hand-rolled. + +| test | asserts | +| --- | --- | +| `test_layer_requires_ab_arms_to_differ` | two identical arms fail, naming the mode | +| `test_differ_names_both_arms_when_they_agree` | the refusal carries the value both arms held | +| `test_differ_passes_when_the_arms_differ` | the positive control | +| `test_differ_counts_as_an_assertion` | `differ` alone is a concluded test | +| `test_differ_refuses_a_failed_query_on_either_side` | a failed arm is refused, left and right | +| `test_differ_refuses_two_failed_queries` | **the inverse of #930's trap**; see below | +| `test_the_inequality_scan_finds_a_planted_offence` | the AST scan fires on both spellings | +| `test_the_inequality_scan_does_not_flag_honest_code` | five shapes it must not flag | +| `test_no_test_in_this_corpus_hand_rolls_an_inequality` | the population is zero, across 17 files | + +**Two failed queries are not two observable arms.** `query_error()` produces a value +unique per occurrence precisely so two failures cannot compare **equal** and pass an +equality assertion. That uniqueness makes them compare **unequal**, so an arms-differ +assertion passes on a pair of statements that both blew up — the defect arriving +through the fix for it. Measured: two calls give `QUERY_ERROR.1.` and +`QUERY_ERROR.2.`. + +**The hand-rolled idiom threw both values away.** `expect.num(int(after != before), 1, +...)` reports `got 0 want 1` when it fails, and a reader cannot tell arms that were +both empty from arms that were both wrong from arms correctly identical. Three defects, +one message. + +**The scan is AST rather than a line regex**, for the reason the `pytest.raises` scan +records: the two paragraphs in this tree that describe the old idiom quote it verbatim, +so a text sweep flags its own documentation. + +**And the scan found a site the manual count missed.** Grepping for before/after naming +found two. The scan found three — the third spelled `int(stated == disk) == 0`, the same +assertion with the comparison inverted, which no search for `!=` would reach. + ## 4. test_guards_pinned.py: every refusal, pinned to its own message **Why this file exists.** @jdatcmd neutered each guard in the layer in turn and diff --git a/test/pytest/VACUITY_MODES.md b/test/pytest/VACUITY_MODES.md index 52696ac8..65ae6248 100644 --- a/test/pytest/VACUITY_MODES.md +++ b/test/pytest/VACUITY_MODES.md @@ -233,9 +233,27 @@ binds the exception and the body pins its SQLSTATE. See section 2. 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. +- `mutation-arm-unobservable` — **narrowed, not closed.** The assertion now exists and + the old spelling of it is unavailable. `expect.differ(a, b, name)` refuses two arms + that agree and names the value they shared; `expect.num(int(after != before), 1, ...)` + and its `int(a == b) == 0` twin are refused by an AST scan over the corpus, so the + class is closed rather than the three instances. It also refuses a failed query on + either arm, which is the INVERSE of the trap #930 closed: `query_error()` makes each + failure unique so two failures cannot compare equal, and that uniqueness makes them + compare UNEQUAL — so an arms-differ assertion passed on a pair of statements that + both blew up. Measured: two calls give `QUERY_ERROR.1.` and + `QUERY_ERROR.2.`. + + WHAT IS NOT CLOSED is the omission. The layer cannot know which two values in a test + are arms, so a test that runs an A/B and asserts nothing about the pair is still + vacuous and nothing refuses it. What has gone is writing the assertion wrongly; + what remains is not writing it at all. See TESTS.md section 3. + + THE SCAN FOUND A SITE THE MANUAL COUNT MISSED, which is the argument for it. I + measured the population by grepping for before/after naming and found two. The AST + scan found three: the third was in `test_docs_cover_the_corpus.py` and spelled + `int(stated == disk) == 0`, the same assertion with the comparison inverted, which no + search for `!=` would reach. **`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. @@ -333,7 +351,12 @@ Each entry names the red test to write first. 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`. +3. ~~`test_layer_requires_ab_arms_to_differ` — closes `mutation-arm-unobservable`.~~ + **Done, and it NARROWS rather than closes** — 3.5 says what remains. The assertion + exists, refuses a failed arm on either side, and the hand-rolled spelling is refused + by an AST scan so the class cannot come back. What no mechanism can do is notice a + test that runs an A/B and asserts nothing about the pair, because nothing tells the + layer which two values are arms. 4. ~~`test_raises_requires_a_sqlstate` — closes `raises-too-broad`.~~ **Done.** It closes `raises-too-broad` and narrows `raises-catches-setup`, which stays open in 3.4 with the two shapes it cannot see named there. What would close the diff --git a/test/pytest/pgc_vacuity.py b/test/pytest/pgc_vacuity.py index 012b10ac..c03b79c9 100644 --- a/test/pytest/pgc_vacuity.py +++ b/test/pytest/pgc_vacuity.py @@ -366,6 +366,47 @@ def ordering_observable(self, forward, reverse, name): f"order is observable before asserting order." ) + # -- inequality ---------------------------------------------------------- + def differ(self, got, want, name): + """Assert that two arms of an A/B are observably different. + + `mutation-arm-unobservable`: both arms produce the identical answer and both + are green, because the assertion that would catch it is the one nobody writes. + Before this the layer had eight helpers asserting equality and one asserting + inequality -- `ordering_observable`, specific to a forward/reverse pair -- so + the general case was hand-rolled as `expect.num(int(after != before), 1, ...)` + at two sites. That idiom throws BOTH VALUES AWAY: when it fails it says + `got 0 want 1`, and a reader cannot tell arms that were both empty from arms + that were both wrong from arms correctly identical. Three defects, one message. + + TWO FAILED QUERIES ARE NOT TWO ARMS, and that refusal is the inverse of the + one #930 added. `query_error()` makes each failure UNIQUE precisely so two + failures cannot compare EQUAL and pass an equality assertion -- which makes + them compare UNEQUAL, so an arms-differ assertion passes on a pair of + statements that both blew up. Measured: two calls give + `QUERY_ERROR.1.` and `QUERY_ERROR.2.`, which are `!=`. The fix + for one direction opened the other, which is why this is checked rather than + inherited from the equality helpers' refusal. + """ + for side, v in (("left", got), ("right", want)): + if _failed_query(v): + raise VacuityError( + # ONE UNBREAKABLE TOKEN FIRST: pytest word-wraps a long traceback + # line, so an arm matching a multi-word phrase against a single + # `E` line can miss a message that contains it. + f"{name}: failed-query-is-not-an-arm: the {side} arm is a failed " + f"query: {v!r}. query_error() makes each failure unique, so two " + f"failures do not compare equal -- which means they DIFFER, and " + f"this assertion would report the mutation as observable. Assert " + f"the failure you expect instead of differencing two of them." + ) + self._counted() + if got == want: + raise AssertionError( + f"{name}: arms-do-not-differ: {got!r} on both arms. An A/B whose arms " + f"agree cannot show that the thing between them did anything." + ) + # -- row counts --------------------------------------------------------- def rowcount(self, got, want, name): """Compare a row count, refusing psycopg's "no count available" sentinel. diff --git a/test/pytest/test_build_refusal.py b/test/pytest/test_build_refusal.py index 42523ecf..30dec399 100644 --- a/test/pytest/test_build_refusal.py +++ b/test/pytest/test_build_refusal.py @@ -182,8 +182,8 @@ def test_the_fingerprint_reads_content_not_mtime(tmp_path, expect): expect.text(source_fingerprint(tree), before, "touching a file does not change the fingerprint") (tree / "src" / "columnar.c").write_text("int a = 2;\n") - expect.at_least(int(source_fingerprint(tree) != before), 1, - "changing its content does") + expect.differ(source_fingerprint(tree), before, + "changing its content does") def test_an_unfingerprintable_tree_always_rebuilds(tmp_path, expect): @@ -294,8 +294,8 @@ def test_the_fingerprint_covers_a_separately_built_module(tmp_path, expect): (tree / "objstore" / "module.c").write_text("int b = 2;\n") after = source_fingerprint(tree) - expect.num(int(after != before), 1, - "editing a separately built module moves the fingerprint") + expect.differ(after, before, + "editing a separately built module moves the fingerprint") (tree / "objstore" / "module.c").write_text("int b = 1;\n") expect.text(source_fingerprint(tree), before, diff --git a/test/pytest/test_docs_cover_the_corpus.py b/test/pytest/test_docs_cover_the_corpus.py index 3743ae27..5268f084 100644 --- a/test/pytest/test_docs_cover_the_corpus.py +++ b/test/pytest/test_docs_cover_the_corpus.py @@ -317,8 +317,9 @@ def test_a_stated_total_that_disagrees_with_disk_is_visible(tmp_path, expect): expect.text(repr(stated_totals(doc)), "(9, 4)", "the document states 9 in 4") expect.text(repr((sum(len(v) for v in found.values()), len(found))), "(2, 1)", "while the fixture on disk holds 2 in 1") - expect.num(int(stated_totals(doc) == (sum(len(v) for v in found.values()), len(found))), 0, - "a stated total that disagrees with disk does not compare equal") + expect.differ(stated_totals(doc), + (sum(len(v) for v in found.values()), len(found)), + "a stated total that disagrees with disk does not compare equal") # --------------------------------------------------------------------------- diff --git a/test/pytest/test_failed_query_sentinel.py b/test/pytest/test_failed_query_sentinel.py index 430cf3d6..366cbc3b 100644 --- a/test/pytest/test_failed_query_sentinel.py +++ b/test/pytest/test_failed_query_sentinel.py @@ -56,7 +56,7 @@ def test_the_comparison_surface_is_what_this_file_thinks_it_is(expect): 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"): + "ordering_observable", "differ"): expect.num(names.count(required), 1, f"{required} is one of them") @@ -82,8 +82,23 @@ def test_the_comparison_surface_is_what_this_file_thinks_it_is(expect): # 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,)]), + # The two arms must DIFFER for the same reason, and for `differ` the sentinel is + # the INVERSE trap: `query_error()` makes each failure unique so two failures + # cannot compare equal, which makes them compare UNEQUAL -- so without its + # refusal this assertion reports two blown-up statements as an observable + # difference. The fix for one direction opened the other. + "differ": ("abc", "xyz"), } +# NOT EVERY ASSERTION IS IN THIS SWEEP, and the reason is worth stating because the +# exclusion is currently an accident of naming rather than a judgement. The +# derivation keys on the first two parameter names, so `wrote(cur, want, name)` is +# outside it: its left side is a CURSOR rather than a value a query returned, and a +# sentinel cannot arrive there -- the count comes from `cur.rowcount`. That happens to +# be the right answer, but a future assertion whose first parameter is not called +# `got` would be excluded just as silently and for no good reason. A declared +# exclusion list with a reason per entry is the fix; it is not in this commit. + # 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. diff --git a/test/pytest/test_layer.py b/test/pytest/test_layer.py index 8f1ae4b9..3e6b7772 100644 --- a/test/pytest/test_layer.py +++ b/test/pytest/test_layer.py @@ -290,3 +290,253 @@ def test_swallows(expect): result = pytester.runpytest("-p", "pgc_vacuity") expect.run_failed(result, "a broad except must not be collectable") result.stderr.fnmatch_lines(["*catches Exception broadly*"]) + +# ---- an A/B whose arms do not differ measures nothing ------------------------- +# +# `mutation-arm-unobservable` in VACUITY_MODES.md section 3.5: both arms of an A/B +# produce the identical answer and both are green, because the assertion that would +# catch it -- that the arms must DIFFER -- is the one nobody writes. +# +# Measured before building: the layer had EIGHT helpers asserting equality and ONE +# asserting inequality, `ordering_observable`, which is specific to a forward/reverse +# pair. Two sites in the corpus hand-rolled the general case as +# `expect.num(int(after != before), 1, ...)`, which throws both values away: when it +# fails it says `got 0 want 1` and the reader cannot see what the two arms were. + + +def test_layer_requires_ab_arms_to_differ(pytester, expect): + """Two arms that agree cannot show that the thing between them did anything. + + Bare pytest: the test passes, because nothing was asserted about the pair. + """ + pytester.makeconftest("pytest_plugins = ['pgc_vacuity']") + pytester.makepyfile( + """ + def test_the_mutation_changed_nothing(expect): + baseline = [(1, 'a'), (2, 'b')] + mutated = [(1, 'a'), (2, 'b')] + expect.differ(baseline, mutated, "the mutation moved the result") + """ + ) + result = pytester.runpytest("-p", "pgc_vacuity") + # `expect.refusal` rather than `fnmatch_lines`, and one token per call. The raw + # matcher searches the inner run's WHOLE stdout, and pytest prints the failing + # function's SOURCE in the traceback -- so a pattern naming a value in the test + # matches the source line rather than anything the guard produced. The arm below + # passed that way before this change, against a layer with no `differ` at all. + expect.refusal(result, "two identical arms do not pass", r"arms-do-not-differ") + + +def test_differ_names_both_arms_when_they_agree(pytester, expect): + """The hand-rolled idiom this replaces printed `got 0 want 1`. + + A reader of that cannot tell whether the arms were both empty, both wrong, or + correctly identical -- which is three different defects with one message. + """ + pytester.makepyfile( + """ + def test_identical_arms(expect): + expect.differ("PLAN-A", "PLAN-A", "the GUC changed the plan") + """ + ) + result = pytester.runpytest("-p", "pgc_vacuity") + # ANCHORED, because the value is written in the test body: with `fnmatch_lines` + # this arm passed against an AttributeError from a `differ` that did not exist, + # satisfied by its own source printed in the traceback. Measured, not reasoned. + expect.refusal(result, "the refusal names the value both arms carried", r"PLAN-A") + expect.refusal(result, "and names the mode", r"arms-do-not-differ") + + +def test_differ_passes_when_the_arms_differ(pytester, expect): + """The positive control. A guard that rejects real A/B tests gets switched off.""" + pytester.makepyfile( + """ + def test_arms_differ(expect): + expect.differ([(1,), (2,)], [(2,), (1,)], "reversing changes the order") + """ + ) + result = pytester.runpytest("-p", "pgc_vacuity") + expect.outcomes(result, "differing arms pass", passed=1, failed=0) + + +def test_differ_counts_as_an_assertion(pytester, expect): + """A test whose only assertion is `differ` has concluded something. + + Without this the no-assertion guard fires instead, and the arm above would pass + for the wrong reason -- a failure, but not the one it names. + """ + pytester.makepyfile( + """ + def test_only_a_differ(expect): + expect.differ(1, 2, "one is not two") + """ + ) + result = pytester.runpytest("-p", "pgc_vacuity") + expect.outcomes(result, "differ alone is a counted assertion", passed=1, failed=0) + + +def test_differ_refuses_a_failed_query_on_either_side(pytester, expect): + """TWO FAILED QUERIES ARE NOT TWO OBSERVABLE ARMS, and this is the inverse of the + trap #930 closed. + + `query_error()` produces a value unique per occurrence, precisely so that two + failures cannot compare EQUAL and pass an equality assertion. That uniqueness + makes them compare UNEQUAL, so an arms-differ assertion passes on a pair of + statements that both blew up -- the same defect arriving through the fix for it. + + Measured: `query_error()` twice gives `QUERY_ERROR.1.` and + `QUERY_ERROR.2.`, which are `!=`. + """ + for side in ("left", "right"): + pytester.makeconftest("pytest_plugins = ['pgc_vacuity']") + args = ('pgc_vacuity.query_error("a"), "PLAN-B"' if side == "left" + else '"PLAN-A", pgc_vacuity.query_error("b")') + pytester.makepyfile( + f""" + import pgc_vacuity + + def test_one_arm_failed(expect): + expect.differ({args}, "the arms differ") + """ + ) + result = pytester.runpytest("-p", "pgc_vacuity") + expect.refusal(result, f"a failed query on the {side} is not an arm", + r"failed query") + + +def test_differ_refuses_two_failed_queries(pytester, expect): + """The shape the uniqueness fix created, written out. + + Both arms raised, both sentinels are distinct, and without the refusal the + assertion reports that the mutation was observable. + """ + pytester.makeconftest("pytest_plugins = ['pgc_vacuity']") + pytester.makepyfile( + """ + import pgc_vacuity + + def test_both_arms_failed(expect): + a = pgc_vacuity.query_error("baseline blew up") + b = pgc_vacuity.query_error("mutated blew up") + expect.differ(a, b, "the mutation moved the result") + """ + ) + result = pytester.runpytest("-p", "pgc_vacuity") + expect.refusal(result, "two failures are not two arms", r"failed query") + +# ---- and the hand-rolled idiom cannot come back -------------------------------- +# +# A helper nobody is required to use is a convention, not a mechanism, and this +# directory has a standing rule that a mode counts as refused only when a mechanism +# refuses it. `differ` on its own leaves the old spelling available, so the scan below +# makes the class unavailable rather than fixing the two instances -- which is the same +# argument as "fix the class, not the instance". +# +# AST, NOT A LINE REGEX. The two paragraphs in this tree that DESCRIBE the old idiom +# quote it verbatim, so a text sweep flags its own documentation; `ast` sees code only. +# That is the same trap the `pytest.raises` scan records, where a regex version +# refused the layer's own test suite with 22 invented offences. + +_INEQ_MSG = "int() of a comparison, passed to an expect call" + + +def _hand_rolled_inequalities(source, filename=""): + """-> ["file:line", ...] for `expect.X(int(a != b), ...)` and friends. + + The shape is `int()` appearing as an ARGUMENT to a call on `expect`. A + bare `int(a != b)` assigned to a name is not flagged: it asserts nothing by + itself, and flagging it would be a claim about arithmetic rather than about an + assertion. + """ + import ast + + found = [] + + class V(ast.NodeVisitor): + def visit_Call(self, node): + target = node.func + is_expect = (isinstance(target, ast.Attribute) + and isinstance(target.value, ast.Name) + and target.value.id in ("expect", "e")) + if is_expect: + for arg in node.args: + if (isinstance(arg, ast.Call) + and isinstance(arg.func, ast.Name) + and arg.func.id == "int" + and len(arg.args) == 1 + and isinstance(arg.args[0], ast.Compare) + and any(isinstance(op, (ast.NotEq, ast.Eq)) + for op in arg.args[0].ops)): + found.append(f"{filename}:{arg.lineno}") + self.generic_visit(node) + + V().visit(ast.parse(source)) + return found + + +def test_the_inequality_scan_finds_a_planted_offence(expect): + """The scan must fire on the exact shape the two converted sites used.""" + planted = _hand_rolled_inequalities( + "def test_x(expect):\n" + " expect.num(int(after != before), 1, 'moved')\n", + "planted.py") + expect.num(len(planted), 1, "the scan finds a hand-rolled inequality") + expect.text(planted[0], "planted.py:2", "and names where it is") + # at_least as well as num, because the other converted site used that one. + expect.num(len(_hand_rolled_inequalities( + "def test_y(expect):\n" + " expect.at_least(int(a != b), 1, 'moved')\n")), 1, + "and finds it through at_least too") + # THE INVERTED SPELLING, which is the one the manual count missed. The third site + # wrote `int(stated == disk) == 0` -- the same assertion with the comparison + # flipped -- so a scan that looked only for `!=` would have left it in place and + # reported a clean corpus. Without this arm, dropping `ast.Eq` from the scan is + # invisible. + expect.num(len(_hand_rolled_inequalities( + "def test_z(expect):\n" + " expect.num(int(stated == disk), 0, 'disagrees')\n")), 1, + "and finds the int(a == b) spelling, not only int(a != b)") + + +def test_the_inequality_scan_does_not_flag_honest_code(expect): + """The false-positive budget, which a static guard needs before it ships.""" + for label, src in ( + ("a bare int()", "def t(expect):\n expect.num(int(x), 1, 'n')\n"), + ("a comparison not wrapped in int()", + "def t(expect):\n expect.differ(a, b, 'arms')\n"), + ("an int(compare) bound to a name first", + "def t(expect):\n flag = int(a != b)\n expect.num(flag, 1, 'n')\n"), + # PASSED TO A CALL THAT IS NOT AN ASSERTION, which is what makes the + # expect-context condition load-bearing. Without this shape, dropping that + # condition changed nothing and the arm stayed green -- found by mutating it. + # The scan's subject is assertions, not arithmetic: `print(int(a != b))` + # asserts nothing and flagging it would be a false red. + ("an int(compare) passed to a call that is not an expect", + "def t(expect):\n print(int(a != b))\n expect.num(1, 1, 'n')\n"), + ("the idiom inside a comment", + "def t(expect):\n # expect.num(int(a != b), 1, 'moved')\n" + " expect.num(1, 1, 'n')\n"), + ("the idiom inside a string", + "def t(expect):\n s = \"expect.num(int(a != b), 1, 'x')\"\n" + " expect.text(s, s, 'n')\n"), + ): + expect.num(len(_hand_rolled_inequalities(src)), 0, + f"not flagged: {label}") + + +def test_no_test_in_this_corpus_hand_rolls_an_inequality(expect): + """The population, which is what makes the two arms above worth having. + + Two sites used the idiom before `differ` existed. Both are converted, so this is + zero -- and the arms above are why a zero here means the scan looked rather than + that it cannot see. + """ + import pathlib + + here = pathlib.Path(__file__).parent + files = sorted(here.glob("test_*.py")) + expect.at_least(len(files), 10, "premise: the scan has a corpus to read") + offences = [] + for f in files: + offences += _hand_rolled_inequalities(f.read_text(encoding="utf-8"), f.name) + expect.text(repr(offences), "[]", "no test hand-rolls an inequality")