Repository navigation
Test integrity after #130: a test may not replace another test's method (rules@5) - #158
Conversation
The runner records every discovered test's method (instance attribute, owning class, what that class holds) as each module is loaded, before the next test module is imported, and fails the shard (harness_changed) when a test starts or stops and one of them is no longer what unittest would call. Before, a test that replaced another test's method in its body, at import from a module loaded later, or in a class fixture made the replaced test pass, and serial unittest, run_shards and the rules@4 critic all said OK. The critic gains test_replaced (a harness family, so it applies to every changed Python file): an assignment, deletion or setattr of a test... attribute on anything other than self, or a mock.patch / patch.object / patch.multiple of one. Reviewer bumped to rules@5. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Actions runs at |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f93e29bf64
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if len(args) >= 2 and isinstance(args[1], ast.Constant) and isinstance(args[1].value, str) \ | ||
| and not self._on_self(args[0]): |
There was a problem hiding this comment.
When changed code uses the valid patch.object(target=Cls, attribute="test_a", new=...) form, this branch only reads positional args[1], so _patched_attributes returns no attribute and the high test_replaced finding is skipped; the positional form is detected. Read attribute (and target) from keyword arguments too so this equivalent patch form is covered by the critic's blocking integrity rule.
Useful? React with 👍 / 👎.
Summary
Stacked on #130 (base:
feat/integrity-next-after-123, headc0e58bc). This takes one leftover from #130's NOTES, "Another test's method replaced at run time" (GAPS 58 (b)). On #130's head, restacked ontomain, each row below gotrun_shards.py --shards 1to exit 0, serialunittest discoverto sayOK, and the critic (rules@4) to report no finding:c0e58bc)Victim.test_1_must_fail = lambda self: Nonebefore that test runstest code changed test_replace.Victim.test_1_must_fail while test_replace.Victim.test_0_replace rantest_replaced, rules@5)ran test_r_victim.Victim.test_must_fail after test code changed test_r_victim.Victim.test_must_failtest_replaced, rules@5)setUpClassusessetattr(Victim, "test_must_fail", …)to replace another class's test methodharness_changednaming the replaced testtest_replaced, rules@5)Changes
scripts/run_shards.py_Loadersubclass ofTestLoaderrecords, for every discovered test, where its test method comes from as each module is loaded (_test_method: the instance attribute, the class that owns it in the MRO, and what that class holds, all by identity). The record is taken per module, before the next test module is imported. So a module imported later that replaces an earlier module's test method is seen as a change, not as part of what was loaded._check_harness, at each test's start and stop, also compares every discovered test against its record. It compares all of them, not only the shard's own tests, so the result does not depend on which shard a test was packed into. A mismatch is reported asharness_changedand names the replaced test id. A test that replaces its own method on its instance is refused as well, conservatively.--shards 1and well under 1 s per shard with 8 shards.examples/critic_rules.py: a new familytest_replaced. It flags an assignment,delorsetattrof an attribute namedtest…on anything other thanself, and a patch of one throughmock.patch("pkg.Cls.test_x"),patch.object(Cls, "test_x")orpatch.multiple(Cls, test_x=…).patch.dictis not flagged. The family is inHARNESS_RULES, so it applies to every changed Python file. The verdicts change, so the reviewer is nowrules@5, andtests/test_publish.pyandtests/test_critic.pypin it.tests/test_run_shards.py, four newIntegritytests:--shards 1and with--shards 2, each also asserting that serialunittestreportsOK;unittest: three tests generated withsetattr(Generated, f"test_generated_{n}", …)before the module is loaded, an instance attributeself.test_dir, and a test that builds its own innerTestCaseand replaces that case's method;tests/test_critic.py: 9TRIPSentries fortest_replaced(class-level and fixturecls.test_a = …, at import,setattr,del type(t).test_a, tuple target,patch.object,patch.multiple,patch(...).start()) and 3CLEANentries (self.test_dir,setattr(self, "testing", …),patch.dictwith atest_…key,obj.tests_dir.path, computed-namesetattrgeneration).docs/TESTING.md(the "While running" guarantee, three table rows and the vacuous-test bullet),docs/CRITIC.md(the rule table, theHARNESS_RULESlist and the revision paragraph), andspec/GAPS.md58.Restack (done in this lane, before this PR)
origin/main(after Integrate honest two-arm GitHits accounting and dry-run exit status #145–Delete dead code across the repository #156,dc69499) intofeat/integrity-next-after-119, pushed as a fast-forward to0e6daacwith no rewrite and no force. The only conflict wasspec/GAPS.md:mainappended items 59–64 and left 58 unchanged, and Test integrity after #119: a test may not stop without an outcome, or report its own unexpected success as a pass #123 changed 58. Both sides were kept. Nothing Delete dead code across the repository #156 deleted was brought back: Delete dead code across the repository #156's narrowing ofintegrity_findings(path, source, added)and its CRITIC.md rewording are kept asmainhas them.feat/integrity-next-after-123, pushed as a fast-forward toc0e58bc. The only conflict wasspec/GAPS.mditem 58, resolved to Test integrity after #123: a test may not swallow its own failures through its cleanup or subtest methods #130's text.*-restack-1031branches were not pushed.Evidence (local: Linux, CPython 3.13.5, 8 CPUs, venv with
hypothesis6.170.14 sotests/test_properties.pyruns)PYTHONDONTWRITEBYTECODE=1 PYTHONPATH=. python scripts/run_shards.py --floor c0e58bcatf93e29b: exit 0,Ran 1276 tests,OK (skipped=23, expected failures=5). Counted as active + xfail, that is 1248 active + 5 xfail. The 23 skips are not counted. Floor: all 1272 tests discovered on the base are here or excused by the diff.0e6daac,--floor dc69499): 1262 active + 5 xfail with 23 skipped (Ran 1267, OK). Test integrity after #123: a test may not swallow its own failures through its cleanup or subtest methods #130 after the merge (c0e58bc,--floor 0e6daac): 1267 active + 5 xfail with 23 skipped (Ran 1272, OK).python -m conformance.run: 23 golden traces matched.benchmarks/run_scenarios.py: exit 0.scripts/run_shards.pyswapped back toc0e58bc's, the new row test fails on all three rows (0 != 1). The negative control passes under both runners, as a control should.c0e58bc..f93e29b, throughcritic.request→examples/critic_rules.py):rules@5, 0 findings.rules@5's integrity rule, run over every Python file as if each were newly added, gives 0 findings (and 0test_replaced) onorigin/mainand on the heads of all 15 open PRs at the time of the sweep (Bridge red-team floors: PI, AB, DS, SIM, SCH, COST fixture cases #96, Console: trust several certificate keys for a rotation, and refuse a key anyone could sign under #122–Console: a validity window for certificates, measured from issued_at (stacked on #125) #128, Test integrity after #123: a test may not swallow its own failures through its cleanup or subtest methods #130–Ed25519: the library verifiers refuse a key no seed derives themselves (stacked on #126) #133, Add scripts/pilot_dry_run.py: the GitHits pilot's dry run as one command (stacked on #141) #143, Add the factory: Space-Time Parsing of a repository on one ledger (v0 slice) #154, Bridge: a blank or invisible-character destination record ID settles nothing (stacked on #131) #157). Files were only read; none of those PRs were touched.NOTES: claim ceiling
setUp), or an assertion method (assertEqual,fail,failureException). The runner compares only the method named by_testMethodName.try/exceptaround the test's own assertions, or an assertion removed in the diff.__getattr__or a metaclass. The runner readsvars()along the MRO.setattr(Cls, computed_name, …)is deliberately not matched, because that is how tests are generated at import. The runner still sees the replacement if it happens after loading.TestCaseand replaces that case'stest…method, which the runner allows (the negative control asserts this one finding). No such pattern exists onmainor on any open PR head.test…attribute on a discovered test class after loading now fails the shard. None exists in this suite..github, Attest, RISE, required checks, the evaluation record, schemas and golden traces.🤖 Generated with Claude Code