diff --git a/CHANGELOG.md b/CHANGELOG.md index 327e0219..ed18bdbf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -425,6 +425,15 @@ true until the next version shipped. ### Fixed +- The sentinel sweep no longer excludes an assertion by accident of naming (#938). + + `_comparisons()` selected on the first two parameter names, so `wrote(cur, want, + name)` sat outside because its first parameter is not called `got`. That happens + to be the right answer for a cursor, and it would also have been the answer for a + future comparison whose first parameter was `left`. Exclusion is now a positive + match on the kind of the left operand, and `inputs == selected + excluded` fails + when a method matches neither rule. A list of method names is not the fix. + - A loop that never ran asserted nothing, and half of that was already refused by a mechanism nobody had recorded covered it (#432). diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index d555a60e..f7574646 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -1864,10 +1864,23 @@ 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. +**The exclusion is derived from the signature too (#938).** Selection was the only +rule, so `wrote(cur, want, name)` sat outside because its first parameter is not +called `got`. That is the right answer for a cursor, and it would also have been the +answer for a comparison whose first parameter was `left`. Exclusion is now a +positive match on the kind of the left operand (`cur`, `result`, `plan`, `exc`, +`reason`), and `inputs == selected + excluded` fails when a method matches neither +rule. A declared list of method names is not the fix: a new method whose first +parameter is `cur` is excluded for the same reason `wrote` is. + + | 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_public_assertion_is_selected_or_excluded` | every public Expect method is selected or excluded, with no residue | a method whose first parameter is not `got` lands in neither bucket | +| `test_wrote_is_excluded_because_its_left_operand_is_a_cursor` | `wrote` is out because the left operand is a cursor | a name-list would exclude it for being called `wrote` | +| `test_a_caller_supplied_value_not_named_got_fails_the_partition` | a value comparison named `left` is residue, not silently excluded | the hole #938 names: a future assertion not called `got` | | `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 | diff --git a/test/pytest/test_failed_query_sentinel.py b/test/pytest/test_failed_query_sentinel.py index 366cbc3b..a8fac3dd 100644 --- a/test/pytest/test_failed_query_sentinel.py +++ b/test/pytest/test_failed_query_sentinel.py @@ -33,22 +33,75 @@ # 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 = [] +# +# THE EXCLUSION IS DERIVED TOO (#938). Selection used to be the only rule, so +# "not selected" was a residue: `wrote(cur, want, name)` sat outside because its +# first parameter is not called `got`, which happens to be the right answer and +# would also be the answer for a future comparison whose first parameter was +# `left`. A method matching neither rule must fail an arm, not land in a third +# bucket. The kind of the left operand is a property of the signature; this layer +# has no annotations, so the first parameter name is how the signature records that +# kind. +def _params(fn): + return list(inspect.signature(fn).parameters)[1:] + + +def _is_comparison(fn): + params = _params(fn) + # (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. + return len(params) >= 2 and ( + (params[0] == "got" and params[1] in ("want", "floor")) + or (params[0], params[1]) == ("forward", "reverse")) + + +# First-parameter names that mean the left operand is not a caller-supplied value. +# The map is keyed on a property of the signature, not on the method name: a new +# method whose first parameter is `cur` is excluded for the same reason `wrote` +# is, and a method whose first parameter is `left` matches neither rule. +_LEFT_OPERAND_KIND = { + "cur": "cursor: the count comes from cur.rowcount, a sentinel cannot arrive", + "result": "inner pytest run result, not a query value", + "plan": "EXPLAIN plan, not a query value", + "exc": "exception object, not a query value", + "reason": "unrunnable declaration, not a comparison", +} + + +def _exclusion_reason(fn): + params = _params(fn) + if not params: + return None + return _LEFT_OPERAND_KIND.get(params[0]) + + +def _partition(cls): + selected, excluded, residue = [], [], [] 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) + sel = _is_comparison(fn) + reason = _exclusion_reason(fn) + if sel and reason is not None: + residue.append((name, "matches both rules")) + elif sel: + selected.append((name, _params(fn)[1])) + elif reason is not None: + excluded.append((name, reason)) + else: + residue.append((name, "matches neither rule")) + return ( + sorted(selected), + sorted(excluded), + sorted(residue), + ) + + +def _comparisons(): + selected, _, _ = _partition(pgc_vacuity.Expect) + return selected def test_the_comparison_surface_is_what_this_file_thinks_it_is(expect): @@ -90,14 +143,9 @@ def test_the_comparison_surface_is_what_this_file_thinks_it_is(expect): "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. +# NOT EVERY ASSERTION IS IN THIS SWEEP. `wrote` is outside it because its left +# operand is a cursor, not because its first parameter fails to be called `got`. +# The partition below is what makes that a decision rather than a residue. # 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 @@ -114,6 +162,57 @@ def test_the_shape_table_covers_every_comparison_the_layer_offers(expect): expect.num(len(missing), 0, f"every comparison has a declared valid pair; missing: {missing}") +def test_every_public_assertion_is_selected_or_excluded(expect): + """#938. No third state: a method matching neither rule is the silent hole. + + Selection remains the derivation this file already had. Exclusion is a + POSITIVE match on the kind of the left operand, so "not selected" is no + longer a bucket. `inputs == selected + excluded` fails when a method matches + neither rule, which is what happens today when a parameter is not called `got`. + """ + selected, excluded, residue = _partition(pgc_vacuity.Expect) + public = [n for n, fn in inspect.getmembers(pgc_vacuity.Expect, inspect.isfunction) + if not n.startswith("_")] + expect.num(len(selected) + len(excluded), len(public), + f"inputs {len(public)} == selected {len(selected)} + excluded {len(excluded)}") + expect.num(len(residue), 0, + f"no public method matches neither rule; residue: {residue}") + for name, reason in excluded: + expect.at_least(len(reason.split()), 3, + f"{name} is excluded with a stated reason, not a missing entry") + + +def test_wrote_is_excluded_because_its_left_operand_is_a_cursor(expect): + """The case that showed the residue was an accident of naming.""" + excluded = dict(_partition(pgc_vacuity.Expect)[1]) + expect.num(1 if "wrote" in excluded else 0, 1, "wrote is in the excluded bucket") + expect.num(1 if "cursor" in excluded["wrote"] else 0, 1, + "and the reason is the cursor, not the method name") + + +def test_a_caller_supplied_value_not_named_got_fails_the_partition(expect): + """Acceptance: a value comparison whose first parameter is not `got` is residue. + + A declared name-list of exclusions would put this method in the same silent + bucket `wrote` used to occupy. The partition must go red instead. + """ + class Probe: + def num(self, got, want, name): + return (got, want, name) + def wrote(self, cur, want, name): + return (cur, want, name) + def eq(self, left, want, name): + return (left, want, name) + + selected, excluded, residue = _partition(Probe) + residue_names = [n for n, _ in residue] + expect.num([n for n, _ in selected].count("num"), 1, "num(got, want) is selected") + expect.num([n for n, _ in excluded].count("wrote"), 1, + "wrote(cur, want) is excluded once the left operand is a cursor") + expect.num(residue_names.count("eq"), 1, + "eq(left, want) matches neither rule, so it is residue rather than excluded") + + 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.