diff --git a/CHANGELOG.md b/CHANGELOG.md index 2779b8c5..e73f0194 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,35 @@ true until the next version shipped. ### Added +- `allow_empty` documented a rule the code did not enforce (#1031). + + `Expect.rows` documents the argument as taking *"a REASON, not a flag"*. The sentence even + gives the rationale: the hatch should cost more to type than the honest assertion. One line + below it sits a truthiness test: + + if _empty(got) and _empty(want) and not allow_empty: + + So `allow_empty=True` satisfied it and carried nothing, and the hatch cost LESS than the + assertion rather than more. Measured before the refusal: `allow_empty=True` and + `allow_empty=1` both passed, 3 passed. `row_set` forwards the argument, so it inherited the + hole and now has its own arm asserting it does not route around the refusal. + + THE CHECK FIRES WHENEVER THE ARGUMENT IS GIVEN, not only when both sides turn out to be + empty. Otherwise a flag form in a test whose sides happen to be non-empty passes today and + refuses on the day the data changes. That is the worst moment to learn it. + + TWO LIVE SITES USED THE FLAG FORM AND BOTH WERE SUBSTANTIVELY CORRECT. Each had its + population premise on the line above. `test_check_records.py` even stated the argument in a + comment: *"an empty offender list is the answer to both, and only one of them is good news."* + + So this is not a defect hiding behind the hatch. The cost fell on the next reader. The hatch + exists so every empty-on-both-sides comparison carries its justification where an audit of + `allow_empty=` can read it, and half of them carried none. Both now do. + + Pinned in `test_guards_pinned.py`, which exists so a refusal is asserted by its message rather + than by "something failed". That file was built after a census found 12 of 17 guards deletable + with the suite still green. + - The gate refuses two checks that share one ledger key, instead of printing a note about them (#982). diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 93bf7fa4..2501c763 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -404,6 +404,9 @@ as the failure. | `test_num_accepts_real_numbers` | **control**: a genuine numeric comparison still passes | | `test_text_refuses_an_empty_expectation` | an empty expected string, which anything empty satisfies | | `test_at_least_refuses_a_non_number` | a bound taken from text | +| `test_rows_refuses_a_flag_where_it_documents_a_reason` | `allow_empty=True` satisfied a truthiness test and carried nothing, so the escape hatch cost LESS to type than the honest assertion | +| `test_rows_accepts_a_reason` | **control**: the documented form still works, or the refusal above is a wall | +| `test_row_set_inherits_the_reason_requirement` | `row_set` delegates to `rows`, so it inherits the refusal rather than routing around it | | `test_at_least_refuses_a_floor_of_zero` | a floor every possible value clears | | `test_at_least_accepts_a_real_bound` | **control**: `at_least(7, 3, …)` passes | | `test_plan_node_refuses_no_criteria` | called with neither `node_type` nor `provider` | @@ -421,6 +424,22 @@ as the failure. | `test_refusal_itself_refuses_an_empty_pattern_list` | the new helper must not become the defect it removes | | `test_the_empty_plan_refusal_precedes_the_arms_it_protects` | the refusal's **position**: no arm may answer ahead of it | +**`allow_empty` documented a rule the code did not enforce (#1031).** `rows` documents the +argument as taking *"a REASON, not a flag"*. The sentence gives the rationale too: the hatch +should cost more to type than the honest assertion. One line below sits a truthiness test. So +`allow_empty=True` passed and carried nothing, and the hatch cost less rather than more. + +Measured before the refusal: `allow_empty=True` and `allow_empty=1` both passed, 3 passed. +`row_set` forwards the argument, so it inherited the hole, which is why it has its own arm. + +The check fires whenever the argument is given, not only when both sides turn out empty. +Otherwise a flag form in a test whose sides happen to be non-empty passes today and refuses on +the day the data changes. That is the worst moment to learn it. + +Two live sites used the flag form and **both were substantively correct**. Each had its +population premise on the line above, and one stated the argument in a comment. They now carry +that reason in the argument, where an audit of `allow_empty=` can read it. + ### plan_marker, and the three ways it could not fail @jdatcmd named this one first: *"both of its arms can be deleted independently diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index 92192835..cfeb422e 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -56,7 +56,15 @@ # distinction, the false-positive budget, and two for the same class one level down, where a # duplicated row in the tracked ledger file collapsed silently and let the last line win. # Re-derived by collection on the merged tree, per the recipe above: `304 tests collected`. -guard_tests 304 +# 304 -> 307: three arms in test_guards_pinned.py pinning that `allow_empty` takes a +# REASON and not a flag (#1031), with the accepted form as the control and `row_set` +# asserted to inherit the refusal rather than route around it. +# +# RE-DERIVE THIS AFTER EVERY REBASE, not once. Two branches can each derive a number +# correctly against a tree holding only their own arms and both be wrong for the merge -- +# measured by @jdatcmd on #1022, where two independent derivations of 280 had 283 as the +# merged truth. Derived here by collection: `307 tests collected`. +guard_tests 307 # The complement: tests that need the driver and a throwaway cluster. Until #1016 these ran # in no CI job at all -- a quarter of the corpus, green when somebody ran them by hand and diff --git a/test/pytest/pgc_vacuity.py b/test/pytest/pgc_vacuity.py index 063b0d40..8fbf5136 100644 --- a/test/pytest/pgc_vacuity.py +++ b/test/pytest/pgc_vacuity.py @@ -654,6 +654,27 @@ def rows(self, got, want, name, allow_empty=None): the escape hatch costs more to type than the honest assertion. """ self._refuse_failed_query(name, got, want) + # THE REASON IS ENFORCED, not merely documented (#1031). This read + # `not allow_empty`, a truthiness test, so `allow_empty=True` satisfied it and + # carried nothing -- which made the escape hatch cost LESS to type than the honest + # assertion, the opposite of what the docstring above argues for. Measured before + # this check: `allow_empty=True` and `allow_empty=1` both passed. + # + # CHECKED WHENEVER IT IS GIVEN, not only when both sides turn out to be empty. A + # flag form in a test whose sides happen to be non-empty would otherwise pass + # today and refuse on the day the data changes, which is the worst moment to + # learn it. + # + # `row_set` forwards this argument, so it inherits the refusal rather than + # offering a way around it. + if allow_empty is not None and not (isinstance(allow_empty, str) + and allow_empty.strip()): + raise VacuityError( + f"{name}: allow_empty takes a REASON, not a flag, and got " + f"{allow_empty!r}. The hatch exists so an empty-on-both-sides " + f"comparison carries its justification where someone auditing " + f"allow_empty= can read it. Pass allow_empty='why it is empty'." + ) if _empty(got) and _empty(want) and not allow_empty: raise VacuityError( f"{name}: both sides are empty, so this comparison could not have " diff --git a/test/pytest/test_check_records.py b/test/pytest/test_check_records.py index 26f72654..681ec9b3 100644 --- a/test/pytest/test_check_records.py +++ b/test/pytest/test_check_records.py @@ -308,7 +308,8 @@ def _self_calls(fn): # 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) + allow_empty="an empty offender list is the pass, and the at_least above " + "is the population premise that makes it mean something") def test_every_recording_method_resolves_its_verdict(expect): @@ -344,7 +345,8 @@ def test_every_recording_method_resolves_its_verdict(expect): 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) + allow_empty="an empty unwrapped list is the pass, and the at_least above " + "is the population premise that makes it mean something") def test_wrapping_a_method_twice_changes_nothing(expect): diff --git a/test/pytest/test_guards_pinned.py b/test/pytest/test_guards_pinned.py index 4840670f..38327dcf 100644 --- a/test/pytest/test_guards_pinned.py +++ b/test/pytest/test_guards_pinned.py @@ -56,6 +56,43 @@ def test_empty_text(expect): '''), "text refuses an empty expectation", "the expected text is empty") +def test_rows_refuses_a_flag_where_it_documents_a_reason(pytester, expect): + """`rows` says `allow_empty` takes a REASON, not a flag, and accepted a flag (#1031). + + The escape hatch exists so a both-sides-empty comparison carries its justification + where someone auditing `allow_empty=` can read it. A bare `True` satisfies the + truthiness test and carries nothing, so the hatch cost less to type than the honest + assertion -- the opposite of what the docstring argues for. + + Measured before this refusal: `allow_empty=True` and `allow_empty=1` both passed, 3 + passed. `row_set` forwards the argument, so it inherited the hole. + """ + expect.refusal(_inner(pytester, """ + def test_flag_not_reason(expect): + expect.rows([], [], "both sides empty, declared with a flag", allow_empty=True) + """), "rows refuses a flag where it documents a reason", "not a flag") + + +def test_rows_accepts_a_reason(pytester, expect): + """**control**: the documented form still works, or the refusal above is a wall.""" + r = _inner(pytester, """ + def test_reason(expect): + expect.rows([], [], "empty after truncate", allow_empty="the table was truncated") + """) + expect.outcomes(r, "a reason is accepted", passed=1, failed=0) + + +def test_row_set_inherits_the_reason_requirement(pytester, expect): + """`row_set` delegates to `rows`, so it must inherit the refusal rather than be a way + around it. Delegating an assertion does not delegate its refusals when the delegation + transforms the data, which `row_set` already records as a trap for the sentinel case. + """ + expect.refusal(_inner(pytester, """ + def test_set_flag(expect): + expect.row_set([], [], "two empty sets, declared with a flag", allow_empty=True) + """), "row_set refuses a flag too", "not a flag") + + def test_at_least_refuses_a_non_number(pytester, expect): expect.refusal(_inner(pytester, ''' def test_bound_text(expect):