diff --git a/CHANGELOG.md b/CHANGELOG.md index 38cedf69..149eb93c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2241,6 +2241,27 @@ true until the next version shipped. either a new ledger row or a rename that would orphan an existing one, and `main` has no tool to remove an orphan until #983 lands -- so the sequencing is written into the arm's comment rather than quietly skipped. +- A `conftest.py` can no longer stub an `Expect` method on the class so a false + claim reports as a pass (#967). + + #964 snapshots the module's bindings. `Expect.num = a stub that still counts` + is not a rebind of `Expect` -- the name still points at the same class -- so a + test asserting `1 == 2` printed `1 passed` and exited 0. That is strictly worse + than switching off a meta-rule: the comparison never happens, the count still + rises, and every guard downstream is satisfied by a test that concluded nothing. + + Public methods of `Expect` are now snapshotted by identity, the same way the + module bindings are. The refusal names `Expect.num` and restores the method + before raising, so an in-process `pytester` inner session cannot poison the + rest of the file. Names that start with `_` are excluded: stubbing `_record` + still leaves the count at 0 and is refused by `pytest_runtest_call`, which is a + different mechanism and the control this issue asked to keep. + + What this does not close: `expect.num = stub` on the instance, or a subclass + yielded by an overridden `expect` fixture. Both still keep the count and drop + the comparison. A snapshot of `Expect.__dict__` cannot see either. #967 stays + open for those two routes. + - A collection-time vacuity refusal keeps its reason under pytest-xdist (#963). `pytest_collection_modifyitems` raises `UsageError`. Serial, that is rc 4 and diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 5ff7fcea..5e955aa8 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -296,6 +296,8 @@ the general case was hand-rolled. | `test_a_conftest_cannot_switch_off_the_broad_except_scan` | the same hatch, a second scan | | `test_a_conftest_cannot_switch_off_the_raises_scan` | and a third | | `test_the_refusal_names_the_binding_that_changed` | an honest run, refused, naming the conftest's name | +| `test_a_conftest_cannot_stub_an_expect_method_so_a_false_claim_passes` | a conftest replacing `Expect.num` **on the class** with a stub that still counts is refused at collection, naming `Expect.num` | +| `test_stubbing_the_recorder_still_fails_closed_by_count` | **control**: stubbing `_record` still fails via count 0, so the public-method snapshot did not swallow the recorder's own protection | | `test_a_new_attribute_on_the_layer_is_not_a_rebind` | the control: a check that fires on anything is not a check | | `test_the_rebind_does_not_leak_into_this_session` | the binding is restored, so `pytester` does not poison the outer run | @@ -318,6 +320,12 @@ a lock: the criterion #924 set is that silencing a rule must cost more than stat reason. **The shell harness needs no equivalent** — `selftest/260` greps `lib.sh` from a separate process and never sources the file it judges. +**#967 closes the class, not the instance.** `pgc_vacuity.Expect.num = stub` is +refused at collection. `expect.num = stub` on the instance, or a subclass yielded +by an overridden `expect` fixture, still keep the count and drop the comparison: +`Expect.__dict__` is unchanged, so the snapshot cannot see them. A stub that still +records satisfies the zero-assertion backstop. Those two routes stay #967. + **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 diff --git a/test/pytest/pgc_vacuity.py b/test/pytest/pgc_vacuity.py index b5ec8158..5ff35510 100644 --- a/test/pytest/pgc_vacuity.py +++ b/test/pytest/pgc_vacuity.py @@ -2050,6 +2050,13 @@ def _raises_sites(path): # assignment are not. It also checks at COLLECTION only, so a test that rebinds a # name inside its own body and restores it is untouched; two arms in # test_failed_query_sentinel.py do exactly that, legitimately. +# +# Module bindings are not the only writable surface. `Expect.num = a stub` leaves +# the name `Expect` pointing at the same class (#967), so the snapshot below cannot +# see it. Public methods of Expect are snapshotted separately, by identity, the +# same way. Leading-underscore names including `_record` are excluded: stubbing +# the recorder leaves the count at 0 and `pytest_runtest_call` refuses the test, +# which is a different mechanism and must stay the one that fires. def _binding_guard(): snapshot = {} missing = object() @@ -2071,12 +2078,48 @@ def restore(ns): return arm, changed, restore +def _public_attr_guard(cls): + """Snapshot the public attributes of a class, by identity. + + #964's module snapshot cannot see `Expect.num = a stub`: the binding + `Expect` is unchanged. This is the next frame (#967), and it is the + CLASS dictionary. An instance attribute or a subclass yielded by an + overridden fixture is a different object: `Expect.__dict__` is untouched, + so this snapshot cannot see it. Names that start with `_` are excluded, + so `Expect._record` stays the control: stubbing it leaves the count at 0 + and is refused by `pytest_runtest_call`, not here. + """ + snapshot = {} + missing = object() + + def arm(): + snapshot.update( + {k: v for k, v in cls.__dict__.items() if not k.startswith("_")} + ) + + def changed(): + return sorted( + f"{cls.__name__}.{k}" + for k, v in snapshot.items() + if cls.__dict__.get(k, missing) is not v + ) + + def restore(): + for k, v in snapshot.items(): + setattr(cls, k, v) + + return arm, changed, restore + + _arm_bindings, _changed_bindings, _restore_bindings = _binding_guard() +_arm_expect, _changed_expect, _restore_expect = _public_attr_guard(Expect) def pytest_collection_modifyitems(session, config, items, _changed=_changed_bindings, - _restore=_restore_bindings): + _restore=_restore_bindings, + _changed_methods=_changed_expect, + _restore_methods=_restore_expect): """Refuse a bare skip, which exits 0 and reads as success. Measured: two skipped tests report `2 skipped` and exit 0. A skip is allowed @@ -2086,14 +2129,22 @@ def pytest_collection_modifyitems(session, config, items, # Captured in this signature at DEFINITION time, so rebinding `_changed_bindings` # or `_restore_bindings` on the module does not reach what runs here. _rebound = _changed(globals()) - if _rebound: - _restore(globals()) + _methods = _changed_methods() + if _rebound or _methods: + if _rebound: + _restore(globals()) + if _methods: + _restore_methods() + names = _rebound + _methods + # #963's reporter rather than a bare `raise`: a UsageError raised in a + # WORKER never reaches the controller, so the refusal arrived as a bare + # exit code. Both surfaces this hook now guards report through it. _collection_usage_error( session, config, items, "the pgColumnar vacuity layer refuses this run: a conftest or plugin " "rebound the layer's own " - + ("names " if len(_rebound) > 1 else "name ") - + ", ".join(_rebound) + + ("names " if len(names) > 1 else "name ") + + ", ".join(names) + " -- a rule this layer enforces is read through that binding, so the " "run would have reported on rules that were switched off. The bindings " "have been restored. If a check is wrong for your case, say so where the " @@ -2210,3 +2261,4 @@ def pytest_collection_modifyitems(session, config, items, # module is fully executed before pytest imports any conftest, so nothing the policed # tree writes can be in the snapshot. _arm_bindings(globals()) +_arm_expect() diff --git a/test/pytest/test_layer.py b/test/pytest/test_layer.py index 9a580cf2..1fb23c39 100644 --- a/test/pytest/test_layer.py +++ b/test/pytest/test_layer.py @@ -729,6 +729,85 @@ def test_raises_compound(expect): hatched.stderr.fnmatch_lines(["*_raises_sites*"]) +@pytest.mark.parametrize("mode", ["serial", "xdist"]) +def test_a_conftest_cannot_stub_an_expect_method_so_a_false_claim_passes(pytester, expect, mode): + """#967, the class-attribute frame. #964 snapshots module bindings. + `Expect.num = a stub` is not a rebind of `Expect` -- the name still points + at the same class -- so a false claim reports as a pass if the stub still + increments the count. An instance attribute or a subclass fixture is a + different object and is not this test. + + The three rows the issue named, and a fix must keep 1 and 3 while turning 2 + into a refusal: + + no conftest failed, correctly + Expect.num stubbed, still counting PASSED -- the hatch + Expect._record stubbed failed, count 0 (separate control) + """ + pytester.makepyfile( + """ + def test_one_equals_two(expect): + expect.num(1, 2, "one equals two, which it does not") + """ + ) + plain = pytester.runpytest("-p", "pgc_vacuity") + expect.outcomes(plain, "premise: a false claim fails when nobody stubs", + failed=1, passed=0) + + import pgc_vacuity + original = pgc_vacuity.Expect.num + pytester.makeconftest( + "import pgc_vacuity\n" + "pgc_vacuity.Expect.num = " + "lambda self, got, want, name: self._record(name)\n" + ) + # BOTH MODES, because this surface reaches the reporter only after the merge + # that composed #963 and #967. Before it, the method branch raised + # `pytest.UsageError` directly -- and a UsageError raised in an xdist WORKER + # never reaches the controller, so `-n 2` gave a bare exit code instead of the + # sentence. Serial alone cannot see that: it is the same green either way. + extra = ("-n", "2") if mode == "xdist" else () + try: + hatched = pytester.runpytest("-p", "pgc_vacuity", *extra) + finally: + pgc_vacuity.Expect.num = original + _collection_refusal_row( + hatched, expect, rc=4, reason_glob="*Expect.num*", + name=f"stubbed Expect.num {mode}", + ) + + +def test_stubbing_the_recorder_still_fails_closed_by_count(pytester, expect): + """#967 row 3. Stubbing `_record` leaves the count at 0, so the test is + refused for making no counted assertion. That is a different mechanism from + the public-method snapshot, and it must stay the one that fires -- a snapshot + of `_record` would swallow this into a collection-time refusal and the + control would no longer mean what it says. + """ + pytester.makepyfile( + """ + def test_one_is_one(expect): + expect.num(1, 1, "one is one") + """ + ) + import pgc_vacuity + original = pgc_vacuity.Expect._record + pytester.makeconftest( + "import pgc_vacuity\n" + "pgc_vacuity.Expect._record = lambda self, name: None\n" + ) + try: + result = pytester.runpytest("-p", "pgc_vacuity") + finally: + # pytester is in-process: the inner conftest writes the shared class. + # This arm deliberately does not snapshot `_record`, so nothing restores + # it for us -- and the outer expect.outcomes would then count nothing. + pgc_vacuity.Expect._record = original + expect.outcomes(result, "stubbing _record is refused by count 0, not by snapshot", + failed=1, passed=0) + result.stdout.fnmatch_lines(["*no counted assertion*"]) + + def test_the_refusal_names_the_binding_that_changed(pytester, expect): """A refusal that does not say what was rebound sends the reader to the wrong file. The offending test here is HONEST -- the only thing wrong with