diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index bf41b3fd..87b46565 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -2432,5 +2432,42 @@ these records into a line, the shell's tab-and-newline defect returns — measur there as a record of four fields for a tabbed name and two lines for a newline — and without this arm nothing would say so. -Still to come in #937: the verdict resolved from the outcome, and a session -reconciliation that can fail. +### Phase 2: the verdict is resolved where the outcome is known + +| test | what it pins | +|---|---| +| `test_a_failed_assertion_records_fail_even_when_the_test_catches_it` | the refutation's arm | +| `test_the_failure_reason_is_the_assertions_own_message` | one message per failure, not two that can drift | +| `test_the_assertions_before_a_failure_keep_their_verdicts` | verdicts are per assertion, not per test | +| `test_a_delegated_assertion_records_fail_too` | a failure raised by pytest's own `assert_outcomes` | +| `test_a_refusal_still_leaves_no_record` | the boundary a refusal must stay outside of | +| `test_every_refusal_precedes_its_record` | the static half, so that boundary cannot drift | +| `test_every_recording_method_resolves_its_verdict` | all 15, derived from the module rather than listed | +| `test_a_recording_method_takes_exactly_one_record_per_call` | the invariant the resolution rests on, pinned after a mutation showed it was assumed | +| `test_wrapping_a_method_twice_changes_nothing` | what the drift arm cannot see, measured and deliberately not guarded | + +**The first design was wrong and the corpus is what refuted it.** Resolving each +verdict from the exception in `pytest_runtest_call` assumed a raise ends the test. +Proving a guard refuses means *catching* the `AssertionError`, which five tests +here do. Measured before the fix: + +``` +count before/mid/after: 0 / 1 / 2 + record 0 'this comparison must fail' verdict PASS <- this one RAISED + record 1 'and the test continues' verdict PASS +1 passed +``` + +A genuinely failed assertion stayed `PASS`, in a passing test, with nothing +reaching the hook to correct it. The resolution now happens **inside** the +assertion call, before any `except` in the test body can see the error. + +**It is a wrapper rather than a verdict passed at the call site** because +`outcomes` and `refusal` delegate to pytest's `assert_outcomes`, which raises a +message this layer never composes — there is no verdict for the call site to pass. + +**A refusal marks nothing, with no special case** for `VacuityError` being an +`AssertionError` subclass: every `VacuityError` is raised *before* its record is +taken, so no record exists to mark. That is scanned rather than trusted. + +Still to come in #937: a session reconciliation that can fail. diff --git a/test/pytest/pgc_vacuity.py b/test/pytest/pgc_vacuity.py index d4038679..f5613ceb 100644 --- a/test/pytest/pgc_vacuity.py +++ b/test/pytest/pgc_vacuity.py @@ -23,6 +23,7 @@ import numbers import pathlib +import functools import itertools import pytest @@ -259,6 +260,74 @@ def _plan_nodes(node): yield from _plan_nodes(entry) +def _resolving(method): + """Set the verdict of the record this call took, from the outcome. #937 phase 2. + + WHY NOT FROM THE EXCEPTION IN `pytest_runtest_call`. That was the first + design, and @OffgridwithJD refuted it: proving a guard REFUSES means catching + the AssertionError, which five tests in this corpus do (test_ordered.py:243, + test_failed_query_sentinel.py:236, :326, :357, :382). Measured before this: + + count before/mid/after: 0 / 1 / 2 + record 0 'this comparison must fail' verdict PASS <- this RAISED + record 1 'and the test continues' verdict PASS + 1 passed + + A genuinely failed assertion stayed PASS, in a passing test, with no exception + reaching the hook. Here the resolution is INSIDE the assertion call, so it + happens before any `except` in the test body can see the error. + + WHY A WRAPPER RATHER THAN A VERDICT PASSED AT THE CALL SITE. `outcomes` and + `refusal` delegate to pytest's own `assert_outcomes`, which raises a message + this layer never composes -- there is no verdict for the call site to pass. A + wrapper covers those without the assertion methods knowing they are wrapped. + + IT MARKS THE RECORD THIS CALL TOOK, BY INDEX -- AND TODAY THAT IS THE SAME + RECORD AS THE LAST ONE. The first version of this comment claimed the index + form was needed to survive nesting, and a mutation refuted it: replacing + `self._records[taken]` with `self._records[-1]` left all 235 tests green, + because nothing distinguishes them. `row_set` delegates to `rows`, but + `row_set` takes no record of its own, so one call appends at most one record + and the two expressions always name it. + + The index form is kept because it stays correct if that stops being true, and + the invariant it depends on is now PINNED rather than assumed: + test_check_records.py asserts every recording method takes exactly one record + per call. If someone writes one that records twice, that arm reddens and this + comment is still true -- which is the opposite of how the first version of it + would have aged. + + WRAPPING TWICE CHANGES NOTHING, and the drift arm deliberately does not look + for it. The marker sits on the outer wrapper, so a doubly-wrapped method is + indistinguishable from a singly-wrapped one -- @OffgridwithJD named that as the + gap most likely to be reached. Measured: both wrappers compute the same + `taken` and write the same verdict and reason, because the inner call appends + nothing before the outer one measures. An arm against a change that alters no + behaviour would be a false red waiting to happen. + + A REFUSAL MARKS NOTHING, and that needs no special case for VacuityError being + an AssertionError subclass: every VacuityError in the recording methods is + raised BEFORE the record is taken, so no record exists to mark. That is not an + accident to rely on -- test_check_records.py scans the module and fails if + anyone adds one after. + """ + + @functools.wraps(method) + def wrapper(self, *args, **kwargs): + taken = len(self._records) + try: + return method(self, *args, **kwargs) + except AssertionError as exc: + if len(self._records) > taken: + rec = self._records[taken] + rec.verdict = "FAIL" + rec.reason = str(exc) + raise + + wrapper._pgc_resolves_verdict = True + return wrapper + + class Expect: """Records assertions, and refuses the ones that could not have failed.""" @@ -342,6 +411,7 @@ def _record(self, name, verdict="PASS", reason=""): self._records.append(_Record(name, verdict, reason)) # -- numbers ----------------------------------------------------------- + @_resolving def num(self, got, want, name): """Compare two numbers. Refuses anything that is not a number. @@ -381,6 +451,7 @@ def row_set(self, got, want, name, allow_empty=None): allow_empty=allow_empty) # -- ordered sequences --------------------------------------------------- + @_resolving def ordered_rows(self, got, want, name): """Compare two sequences IN ORDER, refusing the cases where order says nothing. @@ -420,6 +491,7 @@ def ordered_rows(self, got, want, name): f"{name}: same prefix, different length: got {len(g)} rows want {len(w)}" ) + @_resolving def ordering_observable(self, forward, reverse, name): """Assert this fixture can distinguish order at all, before relying on it. @@ -448,6 +520,7 @@ def ordering_observable(self, forward, reverse, name): ) # -- inequality ---------------------------------------------------------- + @_resolving def differ(self, got, want, name): """Assert that two arms of an A/B are observably different. @@ -561,6 +634,7 @@ def wrote(self, cur, want, name): self.num(count, want, name) # -- row sets ---------------------------------------------------------- + @_resolving def rows(self, got, want, name, allow_empty=None): """Compare two result sets. Refuses two empty sides unless declared. @@ -581,6 +655,7 @@ def rows(self, got, want, name, allow_empty=None): raise AssertionError(f"{name}: got {got!r} want {want!r}") # -- hashes and oracles ------------------------------------------------ + @_resolving def hash(self, got, want, name): """Compare two oracle hashes. Refuses self-comparison and error sentinels.""" if got is want: @@ -598,6 +673,7 @@ def hash(self, got, want, name): raise AssertionError(f"{name}: got {got!r} want {want!r}") # -- text -------------------------------------------------------------- + @_resolving def text(self, got, want, name): """Compare text exactly. Refuses an empty expectation and a failed query.""" self._refuse_failed_query(name, got, want) @@ -610,6 +686,7 @@ def text(self, got, want, name): raise AssertionError(f"{name}: got {got!r} want {want!r}") # -- SQLSTATE ---------------------------------------------------------- + @_resolving def sqlstate(self, exc, want, name): """Assert a raised database error carries EXACTLY this SQLSTATE. @@ -672,6 +749,7 @@ def sqlstate(self, exc, want, name): raise AssertionError(f"{name}: got SQLSTATE {got!r} want {want!r}") # -- plans ------------------------------------------------------------- + @_resolving def plan_node(self, plan, node_type=None, provider=None, name=None): """Assert a node exists, by EXACT equality on a typed EXPLAIN JSON field. @@ -718,6 +796,7 @@ def plan_node(self, plan, node_type=None, provider=None, name=None): ) # -- bounds ------------------------------------------------------------- + @_resolving def at_least(self, got, floor, name): """Assert got >= floor. Both sides must be numbers. @@ -740,6 +819,7 @@ def at_least(self, got, floor, name): raise AssertionError(f"{name}: got {got!r}, wanted at least {floor!r}") # -- the layer's own tests --------------------------------------------- + @_resolving def refusal(self, result, name, *patterns): """The inner run failed, AND it failed for the REASON named. @@ -787,6 +867,7 @@ def refusal(self, result, name, *patterns): # anywhere in the file. result.stdout.fnmatch_lines([f"E*{p}*" for p in patterns]) + @_resolving def outcomes(self, result, name, **want): """Assert on an INNER pytest run's outcomes, and count it. @@ -803,6 +884,7 @@ def outcomes(self, result, name, **want): self._record(name) result.assert_outcomes(**want) + @_resolving def run_failed(self, result, name): """Assert an inner run exited non-zero, and count it.""" self._record(name) @@ -811,6 +893,7 @@ def run_failed(self, result, name): f"{name}: the inner run exited 0, so nothing refused it." ) + @_resolving def plan_marker(self, plan, key, name=None, absent=False): """Assert a plan node carries (or does not carry) a Columnar property KEY. @@ -864,6 +947,7 @@ def plan_marker(self, plan, key, name=None, absent=False): ) # -- the third state --------------------------------------------------- + @_resolving def cannot_run(self, reason, detail=""): """Declare this test unrunnable. Not a pass, and not a silent skip. diff --git a/test/pytest/test_check_records.py b/test/pytest/test_check_records.py index 1c74beec..770e2527 100644 --- a/test/pytest/test_check_records.py +++ b/test/pytest/test_check_records.py @@ -120,3 +120,293 @@ def test_a_name_carrying_a_separator_survives_the_record(expect): e.num(1, 1, nasty) expect.num(len(e.records), 1, "a name with separators made exactly one record") expect.text(e.records[0].name, nasty, "and the name came back byte-identical") + + +# ---- phase 2: the verdict is resolved where the outcome is known ------------- +# +# The first version of this file's comment argued the verdict could be resolved +# from the exception in `pytest_runtest_call`, because assertions are sequential +# and a raise ends the test. @OffgridwithJD refuted it, and this corpus is what +# refutes it: proving a guard REFUSES means catching the AssertionError, which +# five tests do. Measured before the fix: +# +# count before/mid/after: 0 / 1 / 2 +# record 0 'this comparison must fail' verdict PASS <- this one RAISED +# record 1 'and the test continues' verdict PASS +# 1 passed +# +# A genuinely failed assertion stayed PASS, in a passing test, with nothing +# reaching the hook to correct it. So the verdict is set on the COMPARISON's own +# path instead, where the outcome is known and no propagation is needed. + + +def test_a_failed_assertion_records_fail_even_when_the_test_catches_it(expect): + """THE ARM FOR THE REFUTATION, and the shape five tests in this corpus use. + + Catching the AssertionError is how a test proves a guard refuses. If catching + it also erased the verdict, every one of those tests would be reporting on a + record stream that says its deliberate failure passed. + """ + e = pgc_vacuity.Expect("verdict::caught") + try: + e.num(1, 2, "this comparison must fail") + except AssertionError: + pass + expect.num(len(e.records), 1, "premise: the failed assertion was still counted") + expect.text(e.records[0].verdict, "FAIL", + "and it is recorded as FAIL, not as a pass the catcher hid") + + +def test_the_failure_reason_is_the_assertions_own_message(expect): + """A verdict with no reason sends the reader back to the source to find out + what happened. The message is the one the assertion already produces, not a + second one written for the record -- two messages for one failure is how they + drift.""" + e = pgc_vacuity.Expect("verdict::reason") + try: + e.num(1, 2, "one equals two") + except AssertionError as exc: + raised = str(exc) + expect.text(e.records[0].reason, raised, + "the recorded reason is the message the assertion raised") + + +def test_the_assertions_before_a_failure_keep_their_verdicts(expect): + """The verdicts are per assertion, not per test. A test that fails its third + assertion made two real claims first, and a stream that marked the whole test + would lose them.""" + e = pgc_vacuity.Expect("verdict::ordering") + e.num(1, 1, "first, true") + e.num(2, 2, "second, true") + try: + e.num(3, 4, "third, false") + except AssertionError: + pass + e.num(5, 5, "fourth, after the catch") + expect.ordered_rows([r.verdict for r in e.records], + ["PASS", "PASS", "FAIL", "PASS"], + "each assertion carries its own verdict, in order") + + +def test_a_delegated_assertion_records_fail_too(expect): + """`outcomes` and `refusal` hand the comparison to pytest's own + `assert_outcomes`, so the AssertionError is raised by code this layer does not + write and carries a message it did not compose. + + That is the case a verdict passed in at the call site could not cover, and it + is why the resolution wraps the comparison rather than describing it. + """ + e = pgc_vacuity.Expect("verdict::delegated") + + class _FakeResult: + ret = 0 + + def assert_outcomes(self, **want): + raise AssertionError("Outcomes do not match: expected passed=1") + + try: + e.outcomes(_FakeResult(), "a delegated comparison", passed=1) + except AssertionError: + pass + expect.num(len(e.records), 1, "premise: the delegated assertion was counted") + expect.text(e.records[0].verdict, "FAIL", + "and a failure raised by pytest's own code is still recorded") + + +def test_a_refusal_still_leaves_no_record(expect): + """The boundary, restated for phase 2 because the resolution wraps a region + that a refusal must stay outside of. + + Verified statically as well as here: in every recording method, each + `VacuityError` is raised BEFORE the record is taken, so no refusal is ever + inside the wrapped region. That is what keeps a refusal out of the stream + without a special case for `VacuityError` being a subclass of AssertionError. + """ + e = pgc_vacuity.Expect("verdict::refusal") + try: + e.num("100", 100, "a text comparison") + except pgc_vacuity.VacuityError: + expect.num(len(e.records), 0, "a refused assertion still records nothing") + else: + raise AssertionError("num() accepted a string, so this arm tested nothing") + + +def test_every_refusal_precedes_its_record(expect): + """The static half of the arm above, so the invariant cannot drift silently. + + If somebody adds a `VacuityError` after the record is taken, the refusal lands + inside the wrapped region and starts being recorded as a FAILED ASSERTION. + Nothing else in the corpus would notice, and the stream would lie in the most + misleading direction available -- a refusal reported as a real failure. + + THIS SCAN FOLLOWS CALLS, and the first version did not. It looked for a + literal `raise VacuityError(...)` inside each method body, which would have + missed a refusal raised through a helper -- and `_refuse_failed_query` is + exactly such a helper, called by most of these methods. Measured while + attacking this arm: 17 methods raise it directly, 18 can raise it once calls + are followed, so the lexical scan was one method short of the real population. + + It happens that no method calls a refusing helper after its own record, so + both scans agree today. The arm is the transitive one anyway, because the + reason to write a guard is the case that does not exist yet. + """ + import ast + import inspect + + tree = ast.parse(inspect.getsource(pgc_vacuity)) + methods = {} + for cls in ast.walk(tree): + if isinstance(cls, ast.ClassDef) and cls.name == "Expect": + for fn in cls.body: + if isinstance(fn, ast.FunctionDef): + methods[fn.name] = fn + + def _raises_here(fn): + return any(isinstance(n, ast.Raise) and isinstance(n.exc, ast.Call) + and isinstance(n.exc.func, ast.Name) + and n.exc.func.id == "VacuityError" + for n in ast.walk(fn)) + + def _self_calls(fn): + return [(n.func.attr, n.lineno) for n in ast.walk(fn) + if isinstance(n, ast.Call) and isinstance(n.func, ast.Attribute) + and isinstance(n.func.value, ast.Name) and n.func.value.id == "self"] + + # Close over calls, so a method that refuses only through a helper is in the + # population too. + refusing = {nm for nm, fn in methods.items() if _raises_here(fn)} + growing = True + while growing: + growing = False + for nm, fn in methods.items(): + if nm in refusing: + continue + if any(attr in refusing for attr, _ in _self_calls(fn)): + refusing.add(nm) + growing = True + + scanned = [] + offenders = [] + for nm, fn in methods.items(): + recs = [n.lineno for n in ast.walk(fn) + if isinstance(n, ast.Call) and isinstance(n.func, ast.Attribute) + and n.func.attr == "_record"] + if not recs: + continue + scanned.append(nm) + first = min(recs) + for n in ast.walk(fn): + if (isinstance(n, ast.Raise) and isinstance(n.exc, ast.Call) + and isinstance(n.exc.func, ast.Name) + and n.exc.func.id == "VacuityError" and n.lineno > first): + offenders.append(f"{nm}:{n.lineno} raises it directly after the record") + for attr, lineno in _self_calls(fn): + if attr in refusing and lineno > first: + offenders.append(f"{nm}:{lineno} calls self.{attr}() after the record") + # THE PREMISE IS THE POPULATION, and it is the difference between "no method + # offends" and "the scan matched no methods". An empty offender list is the + # answer to both, and only one of them is good news. + expect.at_least(len(scanned), 15, "premise: the scan found the recording methods") + expect.rows(offenders, [], "no refusal is raised after its record is taken", + allow_empty=True) + + +def test_every_recording_method_resolves_its_verdict(expect): + """THE LIST CANNOT DRIFT. A method that takes a record and is not wrapped + records a PASS it never revisits, so its failures are invisible in the stream + while the test still fails normally -- nothing else in the corpus would + notice. + + Derived from the module, not from a list written here: the population is every + method that calls `_record`, and the claim is that all of them are wrapped. + A list would have to be updated by whoever adds the sixteenth, which is + exactly the person who would forget. + """ + import ast + import inspect + + tree = ast.parse(inspect.getsource(pgc_vacuity)) + recording = [] + for cls in ast.walk(tree): + if not (isinstance(cls, ast.ClassDef) and cls.name == "Expect"): + continue + for fn in cls.body: + if not isinstance(fn, ast.FunctionDef): + continue + if any(isinstance(n, ast.Call) and isinstance(n.func, ast.Attribute) + and n.func.attr == "_record" for n in ast.walk(fn)): + recording.append(fn.name) + + expect.at_least(len(recording), 15, + "premise: the scan found the recording methods") + unwrapped = sorted( + nm for nm in recording + if not getattr(getattr(pgc_vacuity.Expect, nm), "_pgc_resolves_verdict", False) + ) + expect.rows(unwrapped, [], "every method that takes a record resolves its verdict", + allow_empty=True) + + +def test_wrapping_a_method_twice_changes_nothing(expect): + """WHAT THE ARM ABOVE CANNOT SEE, measured rather than left as a worry. + + @OffgridwithJD attacked the drift arm and named double-wrapping as the most + reachable thing it would miss: the marker is on the outer wrapper, so a method + wrapped twice looks exactly like one wrapped once. + + That is true, and it does not matter -- which is the answer, not an excuse. + Both wrappers compute the same `taken` and resolve the same record to the same + verdict and reason, because the inner call appends nothing before the outer one + measures. Measured here rather than argued, because "I think it is harmless" is + the sentence that has been wrong three times today. + + So the arm is not extended to catch it. A guard against a change that alters + nothing is a false red waiting to happen, and this layer's budget forbids those + more strictly than it forbids a gap. + """ + e = pgc_vacuity.Expect("double::wrapped") + original = pgc_vacuity.Expect.num + pgc_vacuity.Expect.num = pgc_vacuity._resolving(original) + try: + try: + e.num(1, 2, "a claim that is false") + except AssertionError: + pass + e.num(3, 3, "a claim that is true") + finally: + pgc_vacuity.Expect.num = original + expect.ordered_rows([r.verdict for r in e.records], ["FAIL", "PASS"], + "a doubly-wrapped method resolves exactly as a single one does") + expect.num(e.count, 2, "and still takes one record per call") + + +def test_a_recording_method_takes_exactly_one_record_per_call(expect): + """THE INVARIANT THE RESOLUTION RESTS ON, pinned because a mutation showed it + was assumed. + + `_resolving` marks `self._records[taken]`. Replacing that with + `self._records[-1]` left the whole corpus green, because one call appends at + most one record and the two always name it. That is a property of the methods, + not of the wrapper, and nothing was asserting it. + + If a method ever records twice, `[taken]` and `[-1]` stop agreeing, the + wrapper marks the first and the second keeps a verdict nobody set. This is the + arm that says so, rather than the comment. + """ + e = pgc_vacuity.Expect("records::one-per-call") + calls = [ + lambda: e.num(1, 1, "num"), + lambda: e.text("a", "a", "text"), + lambda: e.rows(["a"], ["a"], "rows"), + lambda: e.ordered_rows(["a", "b"], ["a", "b"], "ordered_rows"), + lambda: e.at_least(5, 1, "at_least"), + lambda: e.differ("x", "y", "differ"), + lambda: e.row_set(["a"], ["a"], "row_set -- delegates to rows"), + ] + deltas = [] + for call in calls: + before = e.count + call() + deltas.append(e.count - before) + expect.rows([str(d) for d in deltas], ["1"] * len(calls), + "every call, including the delegating one, took exactly one record")