[antonwolfy]

Summary

Follow-up to the review of #103 (thread), which #103 merged without a reply. The test-quality gate flags a test that checks data as mock_echo when a name in it only shares a name with a mock held outside the test body. The gate then fails on that test, and the only way out is the exemption marker. On main each of these gives ('mock_echo', 'trivial_size'):

class T:
    def setUp(self):
        m = MagicMock()
        self.x = m
    def test_a(self):
        with patch("x.y"):
            m = run()
        assert m.call_count == 2

@pytest.fixture
def stats():
    m = MagicMock()
    return run(m)                     # returns data
def test_a(stats):
    with patch("x.y"):
        run2()
    assert stats.call_count == 2

The same happens with self.result = MagicMock() in setUp next to a test-local result = run(), and with a fixture of one class next to a same-named parameter in another class.

Changes

scripts/audit_mocks.py:

• A mock held on self is tracked as self.m, not as a bare m: where it is assigned (_propagate_mocks), read off (_is_mock_source) and checked (_on_mock). Only the self.* names of setUp and the other non-test methods reach the tests of the class. A local of setUp is gone when the test runs.
• A fixture counts as a mock when it returns or yields a mock or a patch (_returns_mock), alone or among the items of a tuple, list, set or dict, or of SimpleNamespace(...) or dict(...), not when its body merely calls a mock factory.
• Fixtures follow pytest's scope. A fixture of a class is seen by the tests of that class and of the classes inside it, and a module-level fixture by every test. The nearest fixture of a name wins. A scope's functions and classes are read through if, try/except/finally, with and loops, as the auditor finds tests at any depth, and so are match cases; _add_held_by_class reads a class's methods the same way.
• A parameter that parametrize fills is not the fixture's value, and neither is a parameter that the test rebinds. That covers parametrize by position or by argnames=, on the test or on its class; a name indirect hands to its fixture keeps the fixture's value. A rebind is a store of the name, outside comprehensions, that a later read of the name follows; a store of a mock read off the same name (llm = llm.return_value) is none.
• _mock_names takes the mocks a test gets from outside its body as a seed, and audit_test_quality.py passes them, so a mock read off one (m = self.m) is a mock too.

tests/test_audit_test_quality.py:

• 13 cases that stay data, one per shape;
• 18 more that stay mock_echo: a setUp that puts a local mock on self, a fixture of the test's own class, a fixture that yields a patch's mock, a fixture that returns its mock in a dict or a tuple, a test, a fixture or a setUp under if, try or except, a comprehension's own variable or a store after the last read, indirect parametrize, a test under match, a fixture's mock in SimpleNamespace(...) or dict(...), and a mock read off the fixture's mock or off one setUp holds.

Risk

The gate stops flagging only names that never held a mock, with one exception: a fixture that wraps its mock in a dataclass or a class other than SimpleNamespace counts as data. A call cannot be told from a constructor by its name, and taking any call that holds a mock would make return run(m) a mock again.

The audit of this repo's own tests is byte-identical before and after at 54ce188: 4,802 test functions, the same flags.

No changelog fragment: nothing under dokimi/dokimi/ changes.

Testing

On a Linux review host, at 9405920 (main 21558de merged in, after #232):

• make ci-local: 29 passed, 0 failed, 7 skipped (the Docker catalog smoke, test-latest, the cli-surface packaging step, conda-forge-deps without micromamba, docs deploy, the pip-audit report, semgrep). The test job needed one rerun: test_unbalanced_quote_in_hook_completes_quickly in tests/test_repo_environment.py, a one-second timing check this PR does not touch, took 1.002 s while a second ci-local ran on the host; it passed on the rerun and three times alone. 8,546 tests passed, 2 skipped. Changed-line coverage has no lines to measure: only scripts/ and tests/ change.
• The 9 new data cases fail with main's audit_mocks.py (9 failed) and pass here.
• The cases from the reviews of e86129d (m = self.m, llm = llm.return_value, a test under match, SimpleNamespace(llm=m), dict(llm=...)) fail at e86129d (5 failed) and pass here. 8 mutants of that fix each fail a test: no match cases, no wrappers, a wrapper's keywords ignored, any call holding a mock taken as one, no seed, the seed not passed, the mock-read store taken as a rebind, and that exception widened.
• tests/test_audit_test_quality.py after merging main with test(audit): pin the duplicate identity, nested test files, the exempt suffix and raises #232: 172 passed.
• The cases from exai-bot's re-review (a comprehension variable, a store after the last read, indirect=True and indirect=[...]) fail at 867ba75 (5 failed) and pass here. 8 mutants of that fix each fail a test.
• The cases from exai-bot's first review (a test or fixture under if/try/except, a mock in a dict or a tuple) fail at a86b9eb (5 failed) and pass here. 6 mutants of that fix each fail a test: no descent, no except blocks, a class's direct body only for setUp, no containers, a dict's keys for its values, every container a mock.
• 12 mutants of the first commit's code each fail a test: bare locals of setUp kept, self.m stored as m (in _self_attr and in _on_mock), _returns_mock always true or blind to yield, the outer fixture winning over the nearer one, the class fixtures not walked, and each of the parametrize skips (none, no argnames=, not on the class, no comma split) and the rebind skip dropped.
• python scripts/audit_test_quality.py on this repo's tests: the same report as on main, at both commits.
• At a86b9eb, with the four follow-up branches merged together on main 54ce188 (0a5717c): the full suite (8,232 passed, 2 skipped), the growth check, mypy and ruff pass.

Checklist

• Tests added/updated and pytest passes
• ruff, mypy, and schema checks pass
• Docs updated (or N/A) — see the docs-drift checklist in CONTRIBUTING.md
• No ad-hoc run artifacts committed
• New files carry the SPDX license header (or are exempt in .github/.licenserc.yaml)

[antonwolfy]

@exai-bot /review

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.95%. Comparing base (21558de) to head (9405920).
⚠️ Report is 1 commits behind head on main.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #231      +/-   ##
==========================================
+ Coverage   92.93%   92.95%   +0.02%     
==========================================
  Files         241      241              
  Lines       30582    30647      +65     
  Branches     5043     5058      +15     
==========================================
+ Hits        28421    28489      +68     
+ Misses       1459     1457       -2     
+ Partials      702      701       -1     

see 7 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[vchamarthi]

@exai-bot /review

[exai-bot]

Note

AI-generated review of a86b9ebe54cc, requested by @vchamarthi, using the default review skill. It can be wrong: verify a finding before acting on it. React with a thumbs up or down on a finding to tell the bot whether it helped.
Model us.anthropic.claude-sonnet-5-5, effort default, 12 of 94 turns, 1.0 of 48 minutes, 248k in / 6k out tokens.

The change narrows the mock-echo audit so that a name no longer counts as a mock just because a same-named mock lives elsewhere. Mocks held on self are tracked as self.m, fixtures count only when they return or yield a mock, and fixtures follow pytest scope, with parametrize and rebinding respected. The logic holds up, and I reproduced the targeted cases. The one real gap is that the new scope walk reaches fewer tests than the auditor does, so some tests silently lose the checks they had before. The tests are well aimed.

2 findings are on the changed lines, as comments below.

For a human reviewer

• Whether silently dropping detection for fixtures that return containers of mocks is acceptable, given that the gate's purpose is to catch mock echoes.
• A fixture that takes another mock fixture as a parameter and returns it (def llm2(llm): return llm) is not tracked. That was already so on main, so it is not a regression.

[antonwolfy]

@exai-bot /re-review

[exai-bot]

Note

AI-generated review of 867ba75f4e17, requested by @antonwolfy, using the default review skill. It can be wrong: verify a finding before acting on it. React with a thumbs up or down on a finding to tell the bot whether it helped.
Model us.anthropic.claude-sonnet-5-5, effort default, 10 of 94 turns, 1.1 of 48 minutes, 189k in / 6k out tokens.

Re-review of 867ba75f4e17: 2 new findings.

Findings

1. Minor (confirmed, correctness): _rebound drops a mock fixture for any Store of the name, including comprehension variables

dokimi/scripts/audit_mocks.py:188

_rebound collects every ast.Name with a Store context in the test's own nodes. That includes comprehension and for targets, and the position of the store is ignored. I probed a mock-only test with an unrelated [llm for llm in run()] in an assert. llm is a mock fixture and the test asserts only llm.called. It is no longer flagged mock_echo (flags are ()). The comprehension variable has its own scope, so the fixture was not rebound. A store that comes after the last read of the parameter also counts, though the checked value is still the fixture's mock. This weakens the gate for those tests.

Suggested fix:

Skip comprehension targets, for example by walking only statement-level stores. Or count a rebind only when it comes before the first use of the name.

2. Minor (confirmed, correctness): parametrize(..., indirect=True) is treated as filling the parameter with data

dokimi/scripts/audit_mocks.py:170

With indirect=True (or indirect=['stats']) the parametrized value goes to the fixture of that name, and the test still receives the fixture's return value. _parametrized ignores indirect. A mock-returning fixture used that way is now dropped from the test's mocks. A test that asserts only llm.called is then not flagged. I probed @​pytest.mark.parametrize('llm', [1], indirect=True) over a fixture returning MagicMock(), and it gives no flags. This is rare, but it is a new false negative that the PR does not mention under Risk.

Suggested fix:

Skip names covered by indirect (a truthy constant, or a list naming them) when building the parametrize set.

[antonwolfy]

Both findings of the re-review of 867ba75 are fixed in e86129d:

1. _rebound: a rebind is now a store of the name, outside comprehensions, that a later read of the name follows. names = [llm for llm in run()] before assert llm.called, and llm = None after the last read, keep the fixture's mock, so both are mock_echo again. stats = run() or for stats in run(): before the check still make the parameter data.
2. indirect: _parametrized leaves out the names indirect hands to their fixtures: all of them for a true constant, by keyword or as the third positional argument, and the listed ones for a list or tuple. parametrize('llm', [1], indirect=True) and indirect=['llm'] are mock_echo again; with indirect=['n'], llm stays parametrize's value.

Each new mock case fails on 867ba75 (5 failed), and each of 8 mutants of the fix fails a test: comprehensions walked, any store counted, a store after the read counted, indirect ignored altogether, for a true constant only, for a list only, or as a positional argument, and argnames not read positionally. The audit of this repo's own tests is unchanged.

[napetrov]

Review of e86129d.

Summary: the scope fix for self.* mocks and fixtures that return mocks looks correct, and both re-review findings on 867ba75 (comprehension stores in _rebound, indirect parametrize) are fixed in e86129d with tests. I found nothing new to raise and pushed no fixes.

Test evidence (local, on e86129d):

• uv run pytest dokimi/tests/test_audit_test_quality.py: 158 passed.
• ruff check on the two touched files: clean. (ruff format --check would reformat them, but CI gates lint only, per ci.yml.)
• scripts/audit_test_quality.py over the repo's tests: only the existing exempt test is reported, same as main.
• Extra probes with a mock-returning fixture llm and a patched test body: an assert of llm.call_count alone is mock_echo. A walrus store of llm after the last read still gives mock_echo. A store in a nested def does not rebind the parameter (still mock_echo). with open(...) as llm before the read correctly makes it data.

Open items: none.

Verdict: ready from my side.

[vchamarthi]

No blockers. 1 non-blocking comment.

No blockers. One open question on the rebound-name rule; the rest of the scoping change reads correct at e86129d.

• Question: _rebound treats llm = llm.return_value as ending the fixture mock, so assert llm.called is no longer flagged; should a store whose value is a mock source keep the name?

  audit_test_quality.py:463 builds mocks = _mock_names(fn, t.nodes) | outer, so _propagate_mocks never sees the outer name llm and cannot mark the new llm as a mock. With @pytest.fixture def llm(): return MagicMock() and def test_x(llm): with patch('m.y'): run(llm); llm = llm.return_value; assert llm.called, llm is stored (line 3) before its last read (line 4), so _rebound drops it from outer (audit_mocks.py:165) and nothing flags the assert. Before this PR the name stayed in outer and was flagged. Not run: I read this from the code only. (dokimi/scripts/audit_mocks.py:165)

CI at send time: 0 failing, 0 pending.

[napetrov]

Approving at e86129d: reviewed, CI green, no blocking items open.

[antonwolfy]

@napetrov #232 landed first, so I merged main into this branch (9405920). The merge is clean, and tests/test_audit_test_quality.py passes with both: 172 tests. The review points from @vchamarthi and @vlad-perevezentsev are fixed in f24709a; see the three threads.