You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
test: gate tests that cannot fail for a wrong behaviour - #103
Coverage counts a line as tested once a test runs it, whatever the test checks afterwards. This adds a static auditor for tests that would still pass if the code returned the wrong value, and a pytest gate whose baseline only shrinks. The baseline starts empty.
dokimi/
├── scripts/
+│ └── audit_test_quality.py # ast scan of every test_*; report, --json
└── tests/
+ ├── test_audit_test_quality.py # ratchet (BASELINE = {}) + fixtures per flag
└── 21 test files # the 30 findings on main, fixed
Flag
Shape
Gated
type_only
every assert is is not None / isinstance(x, dict) / hasattr / len(x) > 0
yes
mock_echo
patches something, only checks that a mock was called
yes
set_then_read
x = C(a=1) then assert x.a == 1, nothing else
yes
constant_mirror
pins a dokimi constant or enum .value to a literal
yes
no_assert
no assert, pytest.raises, assert* call or asserting helper
yes
duplicate
same body, parameters and decorators as an earlier test in the same class/module
yes
trivial_size
≤ 3 statements and one of the first four
report only
A test shaped like this on purpose carries # test-quality: ok <reason> on its def or decorator line. A marker with no reason exempts nothing.
How the 30 findings were resolved:
27 fixed. Each test now checks the value, call argument or output it exists for. Some examples: the anthropic/openai init tests check which chat client was built and with which model/api_key. test_dry_run_skips_the_preflight checks that no process was spawned and that the cell is a dry run. test_generate_personas_clamps_count checks for the clamped 5 and 8 in what is logged for clamps_count.
2 deleted. Both duplicated existing coverage. test_every_eval_score_judge_provider_is_accepted_by_judge: the flag-equality test already pins eval score choices to JUDGE_PROVIDERS, which the remaining parametrized test builds. TestPersonaAnalyzer.test_init_with_github_token: test_github_client_lazy_load already checks that the token reaches Github(...).
1 exempted.test_every_listed_judge_provider_is_accepted_by_judge: its whole contract is that it must not raise, since Judge raises ValueError for a provider it does not route.
The original abicheck auditor also flagged test_full_reply_recovers_after_a_truncated_one. That was a false positive: in assert m.call_count == 2 and vote.aggregate == 70.0, the and also checks a value. The auditor now judges each operand of an and separately, and a fixture covers this case.
Evidence
Before (on main: the 21 test files at intel/main, plus the new gate):
The gate fails on a violation. test_each_gated_flag_fails_the_ratchet[<flag>] runs a small fixture for each gated flag through ratchet_errors and expects "<flag>: new: t.py::test_x". test_a_check_of_behaviour_is_not_flagged[...] covers 15 shapes that must not be flagged: is None, not offenders, not hasattr, isinstance(x, OpenAIJudge), assert_called_with, call_args, a value after the call count in an and, an echo through a transform, an asserting helper, and more.
make ci-local: every row passes except ci.yml:test / Run tests, which failed once on test_repo_reach.py::test_reach_timeout_kills_the_harness_process_tree[orphan-exit 0 (no answer)]. That is a timing test this PR does not touch, and it passes when run on its own (2 passed). The touched test files pass (pytest on all 22 of them).
Merge Danger
Door: two-way. The change touches only tests and a script. Reverting it removes the gate, and nothing persisted depends on it.
Blast Radius: contributors
A new test that only checks types or existence, echoes its input, only checks that a mock was called, asserts nothing, or repeats another test's body now fails pytest. Fix the test, delete it, or add # test-quality: ok <reason>.
The package, the CLI, schemas and scores do not change, so this PR has no changelog fragment.
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)
Add scripts/audit_test_quality.py, a static scan of every test_* function
that flags type_only, mock_echo, set_then_read, constant_mirror, no_assert
and duplicate tests (trivial_size is reported, not gated), and
tests/test_audit_test_quality.py, which holds the findings to a baseline
that only shrinks. The baseline starts empty.
The 30 tests flagged on main are fixed: 27 now assert the value, call
argument or output they exist for, two are deleted as covered elsewhere,
and one must-not-raise test is exempted with its reason.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.67%. Comparing base (5064dc2) to head (25870ca). ⚠️ Report is 3 commits behind head on main.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Address four precision gaps in the test auditor:
1. Constructor tracking now invalidates after object mutations (cfg.retries=9)
so set_then_read tests detect broken reset implementations.
2. Tuple comparisons like assert (mock.call_count, out) == (1, 42) are
recognized as behavioral (not mock-only) since they check values too.
3. Weak direct asserts plus asserting helpers no longer flag as type_only,
since the helper detects behavior.
4. Duplicate detection now includes parameter defaults so test_a(x=1) and
test_b(x=2) with the same body are not flagged as duplicates.
Each fix includes a regression test that fails on the old logic.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The callable form pytest.raises(ValueError, make, "bad") checks behavior, but only context-manager blocks set raises. Adding assert out is not None to a test containing this call makes the gate reject it as type_only, unlike the equivalent with pytest.raises(...) test. Recognize calls that supply the callable as well, and add a regression fixture. Merely constructing an unentered pytest.raises(ValueError) context should not count.
Count pytest.fail as a behavioral assertion
dokimi/scripts/audit_test_quality.py:366
A test with assert out is not None followed by if out != 42: pytest.fail(...) is rejected as type_only, although the guard rejects an incorrect value. Explicit failure calls are recognized for no_assert but omitted from the checks used by the shape predicates. Count pytest.fail as an assertion call and add a regression fixture. Avoid exempting arbitrary calls solely because their names contain words such as check or validate.
Assert clamped persona counts reach generation prompts
dokimi/tests/test_personas_generator.py:182
These assertions check the log, not the generation request described in the PR. dokimi/personas/generator.py:142-169 uses the clamped target_count only in the log message; the prompt always asks for 5–8 personas, so both calls send identical requests while this test passes. Assert that mock_llm.invoke.call_args_list contains requests for 5 and 8 personas respectively, and propagate the clamped count into the prompt so the test checks the behavior its name promises.
The reason will be displayed to describe this comment to others. Learn more.
Review at 0e3cf28. The gate and the 27 test fixes hold up, and the ratchet fails on a violation. The auditor itself is where the problems are: it lets through some of the weak tests it exists to catch, and it still rejects some tests that check behaviour. The first four inline comments are worth fixing before merge.
make ci-local: 19 passed, 0 failed, 4 skipped (docker, semgrep, test-latest, docs deploy). 5,214 tests passed, 1 skipped. The changed-line coverage step measured 0 lines, because scripts/ is not in the coverage source. Run by hand, audit_test_quality.py is at 99% (only the reversed-len branch at line 199 and the __main__ line are missed).
Reproductions for the auditor findings, calling audit_source on small sources.
A mutation run on the auditor: 71 mutants, 36 of which pass tests/test_audit_test_quality.py.
Mutations of the production code behind the strengthened tests (judge, harness runner, provider choices).
I agree with @copilot-pull-request-reviewer's three earlier points (asserting helper and type_only, pytest.raises(E, fn) / pytest.fail, the persona clamp test); all still hold at this head.
Other things
The PR description is stale. It says 3338 test functions audited and 38 passed in the gate file. At this head the script reports 3356 functions (flagged: 1 (1 exempt) is unchanged) and the gate file runs 42 tests.
scripts/ is outside the mypy gate (files = ["dokimi"]). ruff check covers the script, and a manual mypy run on it is clean, so this is only a note.
Checked, no issues
Ratchet:BASELINE = {}, new findings and stale baseline entries both fail, exempt tests are ignored. Mutants that drop the new-finding or fixed-finding path are killed.
Deleted tests:test_every_eval_score_judge_provider_is_accepted_by_judge is covered by the flag-equality test. Adding openai-codex to the eval score choices makes it fail.
Exemption:Judge.__init__ raises ValueError for an unrouted provider, so "must not raise" is the whole contract of test_every_listed_judge_provider_is_accepted_by_judge.
Dry run: forcing the preflight on in a dry run fails test_dry_run_skips_the_preflight with "a dry run ran ('bench', '--version')".
Repo rules: SPDX header present, AGENTS.md names match the script and test, test_docs_code_refs.py passes, no schema or package change, no generated files, tests run offline.
… pin more auditor branches
A helper counts as a behavioral check only if it has a non-shape assert. Adds
fixtures for helper shape/value, every patching spelling and the len() spellings;
the anthropic judge init test now passes temperature and seed; the persona clamp
test says it observes the log line.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Verified classification and exemption bugs can reject behavioral tests or let weak tests bypass the gate.
No open issues 🎉
4 previously missed issues
In code that hasn't changed since last review
Inline pytest.fail checks are misclassified as type-only
dokimi/scripts/audit_test_quality.py:371
A test using assert isinstance(out, dict) followed by if out.get('value') != 42: pytest.fail(...) is rejected as type_only, although it detects an incorrect value. Moving the conditional into a helper makes it pass the gate. Count explicit pytest.fail calls as checks here and add a regression fixture so inlining this behavioral check does not fail CI.
Behavioral helpers should suppress weak-only classification
dokimi/scripts/audit_test_quality.py:389
Behavioral helpers suppress only type_only. A patched test calling _value(out) (which asserts out == 42) and send.assert_called_once() still fails the gate as mock_echo. Adding the same behavioral helper alongside constructor readbacks or constant checks also leaves set_then_read or constant_mirror active. These tests can detect incorrect values. Skip the weak-only shape classification when a behavioral helper is called, while preserving duplicate detection, and add regression fixtures for these combinations.
Weak-only shape checks bypass the test gate
dokimi/scripts/audit_test_quality.py:391
The type_only predicate lets two weak-only tests bypass the gate: assert isinstance(out, dict) combined with send.assert_called_once() on a patched mock, and _shape(build()) where _shape only asserts isinstance(out, dict). Both produce no flags despite accepting an arbitrary dictionary. Count shape-only helper assertions here, and do not let call-only mock assertions suppress the flag. Add regression fixtures for both combinations.
Exemption markers in string literals bypass the test gate
dokimi/scripts/audit_test_quality.py:444
The substring search recognizes exemption markers inside string literals. For example, @pytest.mark.parametrize("text", ["test-quality: ok"]) exempts an assertion-free test because the closing quote and brackets count as its reason. This silently bypasses the gate without an exemption comment. Extract comment tokens and require a nonempty reason within the comment; add a regression fixture containing the bare marker as test data.
… and bare vs method calls
Implement the three deferred fixes:
1. Mutation tracking now recognizes AugAssign (c.n += 1), AnnAssign with value
(c.n: int = 5), tuple targets (c.n, y = 5, 6), and setattr() calls. Each pattern
invalidates the constructor echo check (set_then_read flag).
2. Mock echo detection now handles and/or expressions by judging each operand
separately, and correctly rejects tuple comparisons with any non-mock elements
(e.g., (send.call_count, out) == (1, 42) is not mock-only).
3. The no_assert suppression now only recognizes bare function calls with hint
words (verify_result, check_*), not method calls (v.validate).
Added regression tests for all three patterns.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Unlike type_only, mock_echo ignores behavioral helper checks. A test that patches send, checks send.assert_called_once(), and calls a same-file _value(out) helper containing assert out == 42 still fails the gate. The helper detects an incorrect result, so honor helper_checks here too and add a regression fixture for this combination.
Ignore exemption markers inside source strings
dokimi/scripts/audit_test_quality.py:480
Scanning raw source lets decorator data accidentally exempt a test. For example, @pytest.mark.parametrize("text", ["test-quality: ok example"]) makes an assertion-free test exempt without any exemption comment. Recognize the marker only in comment tokens on the permitted lines, and add a regression fixture showing that marker text inside a string does not bypass the gate.
The reason will be displayed to describe this comment to others. Learn more.
Review at cfe920e. Threads 1, 4 and 6 are fixed, and the strengthened test_init_anthropic now kills the hardcoded temperature=None, seed=None client. Two of the auditor threads are still open, because the cases they named still reproduce. I'd fix those before merge.
Reproductions for the auditor findings, calling audit_source on small sources.
A mutation run on the auditor and on judge.py: 11 auditor mutants, 8 killed, 2 survive (inline), 1 equivalent. The ChatAnthropic(..., temperature=None, seed=None) mutant fails test_init_anthropic (1 failed, 58 passed).
The real-tree scan: 3,364 test functions, 1 flagged (the exempt one).
@copilot-pull-request-reviewer's four auditor points still hold at this head. New evidence on the exemption marker in a string literal: @pytest.mark.parametrize('text', ['test-quality: ok example']) on def test_a(text): run(text) gives (('no_assert',), exempt=True), because _is_exempt (audit_test_quality.py:478-479) partitions the raw source line.
Still open from my earlier review:
audit_test_quality.py:231: the mutation tracking still misses the call spellings the thread named. After c = C(n=1), each of r = c.bump(), x = [c.bump()], mutate(c), c.items.append(3) and (c := C(n=2)) followed by assert c.n == 1 still gives ('set_then_read', 'trivial_size'). c.n += 1, c.n: int = 5, c.n, y = 5, 6 and setattr are fixed. Any call with c as receiver or argument needs to count as a mutation.
audit_test_quality.py:316-328: mock_echo is still decided by attribute shape. In a patched test, assert (m.call_count, m.called) == (1, True) gives no flag (the literal (1, True) fails the all-mock-attribute check, so the docstring's own example is not flagged). assert out is not None then assert m.called gives no flag. assert stats.call_count == 3, where stats is a real result, is still flagged mock_echo.
test_audit_test_quality.py: the non-blocking fixture gaps you declined (kw_defaults/vararg identity, rglob, report suffix, single-flag mutants) are still unpinned. Fine as a follow-up.
…teral tuple expectations, callable raises and self helpers
Address review on the test-quality gate: mutations via any call on the object or
as an argument, walrus rebinds, mock-only tuple comparisons with literal
expectations, callable-form pytest.raises, and self.<helper>() calls.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Passing the object to hasattr, isinstance, callable or print marked it
mutated, so one extra weak assert hid set_then_read on a constructor echo.
Calls to read-only builtins no longer count as mutations.
Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
An or passes as soon as one operand does, so assert out == 42 or send.called
holds for a wrong out. Both _is_weak and _is_mock_verify required every
operand to be weak, so neither type_only nor mock_echo fired.
Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
assert True, assert out == 42 or True and assert m.assert_called_once_with
(an uncalled method, always truthy) counted as value checks, so a test made
only of them passed the gate. They are now ignored, which leaves no_assert.
Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
isinstance plus send.assert_called_once() (or assert send.called) checked no
outcome yet cleared type_only. Two such tests now pin the retrieved docs and
the get_library_docs arguments; before, a wrong _retrieve_docs result passed.
Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
pytest collects test_*.py and *_test.py by default and pyproject does not
narrow python_files, so a test in tests/foo_test.py ran in CI but was never
audited. audit_tree now scans both patterns.
Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
Unflagged shapes (assert_not_called in a skip test, bare ensure_* call, call_count on a real object): kept as intentional, with a comment next to _CALL_ONLY_METHODS in scripts/audit_test_quality.py saying so.
@xaleryb: which sentence does "'request' -> 'logged'" refer to? The PR body only has "checks for the clamped 5 and 8 in the generator's request", and I can't find a sentence where "logged" fits. I'll edit it once you point at it.
…heck
assert [out == 5] is truthy whatever out is, like the tuple case, so
the gate let it pass as a value check. (*xs,) and {**d} are empty when
xs or d is, so they can fail and are no longer treated as always true.
Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
Re-ran every earlier point's original input with audit_source at a743d62, plus the full suite, ruff, mypy, bandit and the gate on the merged tree.
PR body, "How the 30 findings were resolved": "test_generate_personas_clamps_count checks for the clamped 5 and 8 in the generator's request". generate_personas(target_count=2) and (target_count=20) send identical invoke() requests, and the test reads the log line. It should say "logged".
assert [out == 5], assert {out == 5} and assert {"k": out == 5} -> no flag -> no_assert, the same as the tuple case. assert (*xs,) -> no_assert, though it fails for an empty xs.
Pushed:
bb05698: non-empty list, set and dict displays are treated as always true. A starred-only display or a **-only dict is not.
Left for the author: the PR body sentence above. Static checks are red because of a license-header failure inherited from main (repos/errors.py), not from this PR.
The reason will be displayed to describe this comment to others. Learn more.
Review at bb05698. The fixes since ac76f2f hold: dict arguments now count as a mutation, and the always-true rule covers lists, sets and dicts. Two of the three inline comments are test gaps I'd fix before merge, and one is a nit. All three earlier threads are resolved; the duplicate-identity gaps go to a follow-up, as you said.
make ci-local: 24 passed, 2 failed, 4 skipped. 6,449 tests passed, 1 skipped; the changed-line coverage step has nothing to measure, because scripts/ is outside the coverage source. Both failures are the gitleaks job, which could not download its binary (HTTP 504). gitleaks detect on origin/main..HEAD by hand: 17 commits, no leaks.
Mutation runs on the always-true and dict-argument branches of the auditor, and on the production code behind the strengthened tests (budget, repo environment, chunking, refiner, panel, model compare, product registry, plugin delta).
audit_source probes for the new display and dict rules.
Checked, no issues
Always-true rule:assert [out == 5], {out == 5}, {'ok': out == 5}, [*xs, 1] and {1: 2, **d} give no_assert. assert (), [*xs], {**d} and (*xs,) do not. Mutants that drop a display type, make the starred test bool(node.elts) or flip a branch are all killed.
Dict arguments:mutate({'k': c}), mutate(**{'k': c}) and mutate({'k': c.x}) no longer give set_then_read.
Strengthened tests:test_chunk_text_basic, test_gap_filler_skips_zero_gaps, the Gemini panel routing, the t_stat sign and length, the onetbb registry entry and the budget table row each fail under a plausible mutation of the code they cover.
Repo rules: SPDX headers present, AGENTS.md names resolve (test_docs_code_refs.py passes), no generated files, no changelog fragment needed (only scripts/, tests/ and AGENTS.md change), no schema change.
Re @xaleryb's PR-body point: the sentence now reads that test_generate_personas_clamps_count checks the clamped 5 and 8 in what is logged, not in the request. Antonwolfy's three inline points from bb05698 are fixed in 0aef179 and f26ba24 (answered on their threads); the duplicate-identity gaps stay a follow-up as agreed.
Re-ran every earlier point's original input with audit_source at 0aef179, plus the full suite, ruff, mypy, bandit and the gate on the head merged with main. All of them pass except one:
scripts/audit_test_quality.py:90-91 says assert_not_called in a skip test and call_count on a real object are "Deliberately not flagged". @patch('m.llm') def test_skip(llm): run(skip=True); llm.assert_not_called() -> mock_echo (the same with assert_not_awaited). @patch('m.clock') def test_x(clock): stats = run(); assert stats.call_count == 3 -> mock_echo. Expected: the comment and the behaviour agree.
No commits pushed this round.
Left for the author: either change the comment to say these shapes are flagged mock_echo on purpose, or stop mock_echo from flagging a test whose only checks are assert_not_called/assert_not_awaited.
main carries an older copy of scripts/audit_test_quality.py and its test from intel#102;
the branch's reviewed version is kept (main had nothing the branch lacks).
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…mock echo
The comment said assert_not_called in a skip test and call_count on a real
object are deliberately not flagged; mock_echo flagged both. Now:
- assert_not_called / assert_not_awaited, `not m.called` and
`m.call_count == 0` are an absence the code under test chose, so they
do not make a test mock_echo.
- .called / .call_count / .await_count count only when read off a name
bound to a patch or a mock: an injected @patch argument, `with patch()
as m`, `m = MagicMock()` / `mocker.patch()` / `p.start()`, a patch's
replacement argument, a name read off one of those, or a mock_* fixture.
`stats = run(); assert stats.call_count == 3` is data.
The near-miss (only `llm.called` / `llm.call_count == 1` on the patched
mock) is still flagged.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
@xaleryb Fixed in 7e46537: the auditor now does what the comment says. I updated the comment and the module docstring to match.
assert_not_called / assert_not_awaited no longer make a test mock_echo. A skip test that checks the patched dependency was not called is checking an outcome. The attribute spellings of the same absence (assert not llm.called, assert llm.call_count == 0) are treated the same way. A test that mixes cache.assert_called_once() with llm.assert_not_called() is also not flagged.
.called / .call_count / .await_count only count when they are read off a name bound to a patch or a mock. That means an injected @patch argument (decorators passed new= inject nothing), with patch(...) as m, m = MagicMock() / mocker.patch(...) / p.start(), the replacement handed to a patch (monkeypatch.setattr(mod, "f", fake)), a name read off one of those (chat = llm.return_value.chat), or a mock_* fixture. stats = run(); assert stats.call_count == 3 is data and is no longer flagged.
The near-miss is still flagged: @patch('m.llm') def test_x(llm): run(); assert llm.called (and llm.call_count == 1, aliases, stacked decorators, p.start(), mocker.patch).
New tests: test_a_not_called_check_or_a_count_on_data_is_not_a_mock_echo (8 shapes, your two included) and test_a_called_check_on_a_patched_mock_is_still_a_mock_echo (7 shapes). I checked them by mutation. Each of these reverts makes at least one new case fail: counting assert_not_called as an echo again, dropping the absence check, accepting any receiver, ignoring injected @patch args, dropping alias tracking, and dropping the mock_* fixture rule.
I also merged intel/main (dddcd42). The copy of the auditor and its test that came to main with #102 is an older snapshot of this branch: it lacks bb05698 (list/set/dict displays) and a743d62. I kept the branch version. Main's new tests pass the gate with no new findings.
The reason will be displayed to describe this comment to others. Learn more.
Review at 7e46537. The three earlier threads are fixed, and the new not called / data rules in mock_echo do what the commit says. Two things are worth fixing before merge, both about the new mock tracking: it lets a real mock_echo through that the base script flagged, and one of its branches has no test.
make ci-local: 6,903 tests passed, 21 skipped; 20 jobs passed, 7 skipped. The 8 failures are ruff, zizmor, pre-commit, bandit, mypy, deptry, mkdocs and pip-audit, missing from my venv (exit 127): make dev stops at the tools install on this base, which #179 fixed on main. Static checks, mypy and the strict docs build pass on GitHub's CI at this head.
tests/test_audit_test_quality.py: 122 passed. Against the base script, 14 of them fail, so the new fixtures pin the new code.
audit_source probes for the new tracking, compared with the base script, and a mutation run on _mock_names / _on_mock / _is_absence: 36 mutants, 11 of which pass the whole gate file.
Mutations of compare_plugin_runs (swapped averages, flipped delta sign): the plugin-delta test fails on each.
Checked, no issues
Plugin delta: swapping plugin_avg and base_avg, or flipping the sign, fails both stamp parameters. zip(..., strict=True) holds: two evaluations, each with a skill key.
Starred dict:assert {**d, 'k': 1} and assert {1: 2, **d} are no_assert; the all(...) mutant fails them.
Budget and environment tests: a gate that always exits 1, or exits with no warning, fails test_gate_passes_a_clean_tree. The two crash tests assert the AGENTS.md instruction and no longer depend on the integer limit.
Real tree:flagged: 1 (1 exempt), no mock_echo; no crash on async, nested, lambda, starred or patch.dict / patch.multiple sources.
Repo rules: SPDX headers present, no schema or package change, no generated files, tests run offline.
Review at 7e46537: both items are in 6ea4615 (replied on the threads). The ruff/zizmor/mypy "exit 127" failures in that run come from the missing venv tools, not from this PR.
The reason will be displayed to describe this comment to others. Learn more.
Review at 3f69451. The refactor into audit_mocks.py changes no verdict, and both earlier threads are fixed. One thing is worth fixing before merge: the new outer-mock tracking is keyed by bare name, so it flags tests that check data. One non-blocking test gap is inline.
make ci-local: 28 passed, 1 failed, 7 skipped. 7,237 tests passed, 1 skipped. The failure is pip-audit: gitpython 3.2.0 (CVE-2026-114145, fixed in 3.2.1) from the lock files, which this PR does not touch. The changed-line coverage step has nothing to measure, because scripts/ is outside the coverage source.
audit_source from 7e46537 and from the head on all 190 test files and about 35 probe sources: no verdict differs between the old module and the split one.
python scripts/audit_test_quality.py: 4,309 test functions, flagged: 1 (1 exempt).
A mutation run on the new tracking: 31 mutants, 18 of which pass tests/test_audit_test_quality.py.
Mutations of the code behind the plugin-delta and environment test edits: the tests fail on each.
Checked, no issues
Refactor:audit_test_quality.py and audit_mocks.py give the same flags as the single-file version; no unused imports, the sibling import works when the script is run by path and when the test loads it.
Growth ratchet and deptry:check_growth.py and deptry pass; the DEP001 entry for audit_mocks covers the sibling import.
Repo rules: SPDX header on audit_mocks.py, no schema or package change, no generated files, tests run offline.
The reason will be displayed to describe this comment to others. Learn more.
A mock held by setUp, a helper method or a same-file fixture makes every same-named local or parameter in the class/file a mock, so tests that check data are flagged mock_echo.
_add_held_by_class collects the bare names bound by any non-test method (self. stripped, plain locals included) and adds them to every test of the class. _outer_mocks does the same for a fixture name across the whole file (audit_mocks.py:234-235), when the fixture body merely contains a mock factory or a patch call. A test that rebinds the name, or gets it from another class or from conftest.py, is still treated as holding the mock. At 7e46537 all three gave no flag.
The same happens with setUp: self.result = MagicMock() and a test-local result = run(), and with a fixture in class A and a same-named parameter in class B. The gate rejects these with no way out except the exemption marker. The mutants that keep the self. stripping ([1] vs [-1], every method vs setUp only) pass the whole gate file, so no test pins this.
Suggestion: hold self.<name> attributes only as self.<name>, so a bare local never matches; count a fixture as a mock only when it returns or yields the mock (or the patch), and skip a parameter the test rebinds or parametrizes. Add one "stays data" fixture for each of the shapes above.
The reason will be displayed to describe this comment to others. Learn more.
I pushed a commit per nit, up to f1801ab, which fix the nit from my review:
f1801ab: dokimi/tests/test_audit_test_quality.py:520: added flagged cases for @pytest.fixture(scope=...), def test_x(*, llm) and def test_x(llm, /), and a not-flagged case for a plain helper named like the parameter.
make ci-local at f1801ab: 28 passed, 1 failed, 7 skipped.
Not approving yet. Still open: the risk from my review.
…pped
Merging main kept the branch's line and lost main's check of the summary
line: a summary that prints the lazy total as the on-demand total passed.
Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
Re-ran the original input of every earlier point (ours and the open threads) with audit_source at 3f69451. Also ran mutants of each auditor rule from our earlier commits, and the full suite, ruff, mypy, bandit and deptry on the head merged with main. All of them pass, with one exception:
Merging main (dddcd42) kept the branch's test_gate_passes_a_clean_tree and dropped main's "start 40 chars, lazy 0, on demand 13, off 0" in out check. Input: repo budget printing the lazy total where the on-demand total belongs. Observed: every budget test passes. Expected: fail, as on main.
Pushed:
25870catest(repo budget): restores main's summary-line assert next to the table-row assert. It fails on that mutant without the change.
pip-audit (locks) is red because of the gitpython 3.2.0 advisory in the locks. The scheduled run on main fails the same way, so it does not come from this PR.
The reason will be displayed to describe this comment to others. Learn more.
Approved at 25870ca. Every earlier point re-run at this head passes and no must-fix or mid item is open; full suite, ruff and mypy clean on Linux; CI green.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
antonwolfy
left a comment
Summary
Coverage counts a line as tested once a test runs it, whatever the test checks afterwards. This adds a static auditor for tests that would still pass if the code returned the wrong value, and a pytest gate whose baseline only shrinks. The baseline starts empty.
type_onlyis not None/isinstance(x, dict)/hasattr/len(x) > 0mock_echoset_then_readx = C(a=1)thenassert x.a == 1, nothing elseconstant_mirrordokimiconstant or enum.valueto a literalno_assertpytest.raises,assert*call or asserting helperduplicatetrivial_sizeA test shaped like this on purpose carries
# test-quality: ok <reason>on itsdefor decorator line. A marker with no reason exempts nothing.How the 30 findings were resolved:
model/api_key.test_dry_run_skips_the_preflightchecks that no process was spawned and that the cell is a dry run.test_generate_personas_clamps_countchecks for the clamped 5 and 8 in what is logged for clamps_count.test_every_eval_score_judge_provider_is_accepted_by_judge: the flag-equality test already pinseval scorechoices toJUDGE_PROVIDERS, which the remaining parametrized test builds.TestPersonaAnalyzer.test_init_with_github_token:test_github_client_lazy_loadalready checks that the token reachesGithub(...).test_every_listed_judge_provider_is_accepted_by_judge: its whole contract is that it must not raise, sinceJudgeraisesValueErrorfor a provider it does not route.The original abicheck auditor also flagged
test_full_reply_recovers_after_a_truncated_one. That was a false positive: inassert m.call_count == 2 and vote.aggregate == 70.0, theandalso checks a value. The auditor now judges each operand of anandseparately, and a fixture covers this case.Evidence
intel/main, plus the new gate):test_each_gated_flag_fails_the_ratchet[<flag>]runs a small fixture for each gated flag throughratchet_errorsand expects"<flag>: new: t.py::test_x".test_a_check_of_behaviour_is_not_flagged[...]covers 15 shapes that must not be flagged:is None,not offenders,not hasattr,isinstance(x, OpenAIJudge),assert_called_with,call_args, a value after the call count in anand, an echo through a transform, an asserting helper, and more.make ci-local: every row passes exceptci.yml:test / Run tests, which failed once ontest_repo_reach.py::test_reach_timeout_kills_the_harness_process_tree[orphan-exit 0 (no answer)]. That is a timing test this PR does not touch, and it passes when run on its own (2 passed). The touched test files pass (pyteston all 22 of them).Merge Danger
Door: two-way. The change touches only tests and a script. Reverting it removes the gate, and nothing persisted depends on it.
Blast Radius: contributors
pytest. Fix the test, delete it, or add# test-quality: ok <reason>.Checklist
pytestpassesruff,mypy, and schema checks pass.github/.licenserc.yaml)🤖 Generated with Claude Code