test: remove tests that mutation analysis proved assert nothing - #34475
Conversation
25 test functions across three files pass unchanged when every function they execute is mutated; the owning file killed zero of their scored mutants. Four zero-kill tests tied to the fix in #31288 are kept for rewrite instead of removal.
…itellm_/test-coverage-mutation-analysis-e42223
Greptile SummaryRemoves mutation-ineffective tests from four test modules.
Confidence Score: 5/5The PR appears safe to merge with no new blocking failures eligible for this follow-up review. No blocking failure remains within the scope of the previous review thread.
|
| Filename | Overview |
|---|---|
| tests/test_litellm/caching/test_redis_cache.py | Removes twelve mutation-ineffective tests around Redis increments, batch reads, and pipelines while retaining targeted regression tests. |
| tests/test_litellm/llms/custom_httpx/test_http_handler.py | Removes five mutation-ineffective aiohttp transport and session-validation tests. |
| tests/test_litellm/llms/pass_through/guardrail_translation/test_handler.py | Removes mocked handler-behavior tests and their unused imports and helper, leaving registry coverage. |
| tests/test_litellm/proxy/test_proxy_server.py | Removes the mutation-ineffective embedding token-array proxy test. |
Reviews (2): Last reviewed commit: "test: remove five more zero-kill tests f..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -22,78 +22,6 @@ def redis_no_ping(): | |||
| yield | |||
There was a problem hiding this comment.
These deletions leave async_increment TTL semantics, Redis-version parsing, batch decoding, pipeline regrouping, and per-command error propagation without direct coverage. Please replace the ineffective tests with mutation-effective assertions so regressions affecting spend counters, rate-limit windows, and Redis update buffering remain detectable.
Rule Used: What: Flag any modifications to existing tests and... (source)
Knowledge Base Used: Response Caching
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
The removed tests executed those paths but could not detect breakage in them: across the functions each test touches, the mutation run scored their covered sets and the whole file killed zero of the mutants, so the coverage being removed was line execution without behavioral assertion. The evidence table in the description lists this per test Replacing them is deliberately sequenced as a follow-up rather than bundled here, so the new tests get written against the recorded surviving mutants (the concrete uncaught behavior changes) instead of re-mocking the same seams, and so this PR stays removal-only with its evidence reviewable in isolation. The four namespace regression tests tied to #31288 were kept for exactly the concern you raise |
|
The mutation evidence is rigorous and the sequencing argument is sound. Writing replacement tests against the recorded surviving mutants rather than re-mocking the same seams is exactly the right approach — it forces the new tests to assert real behavioral outcomes instead of reproducing the same structural flaw. The four kept namespace regression tests confirm you're applying the distinction correctly: retained because they cover known regressions, removed because zero-kill on scored mutants is an objective verdict regardless of line coverage. This is acceptable as a removal-only PR given the evidence provided. The key ask is that the follow-up actually lands — the surviving mutant list should drive the new test authorship directly. |
The http_handler pair only received its full mutation verdict after the first removal batch landed; these five tests pass unchanged when every function they execute is mutated and the owning file killed none of their scored mutants. The ssl tests excluded from mutation scoring are untouched.
|
@greptile review again |
7047a37
into
litellm_internal_staging
2 participants
This PR removes 30 test functions that a mutation testing sweep proved cannot fail when the code they exercise breaks. For each removed test, every source function it executes had mutants generated and scored, and the owning test file killed zero of them, meaning these tests pass unchanged against arbitrarily broken implementations of what they appear to cover
The sweep scored each mirror test file by mutating its source module (mutmut 3.5.0 in a Linux py3.12 container, with mutation restricted to lines the file actually covers) and running only that file against the mutants. A test was only removed when the verdict had full power: the pair's mutation run completed with no unchecked mutants, the test's covered functions had scored mutants, and none were killed. Four additional zero-kill tests in test_redis_cache.py were kept because git blame ties them to the regression fix in #31288; they need a rewrite rather than removal. A follow-up commit removes five more tests in llms/custom_httpx/test_http_handler.py, whose verdict reached full power only after the initial batch landed, once its mutation run completed with the schemata-incompatible ssl tests excluded from scoring
Relevant issues
Linear ticket
Pre-Submission checklist
@greptileaito re-request a review after pushing changes)The first item is intentionally unchecked; this PR removes tests that provably assert nothing rather than adding new ones
Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
Per-test evidence from the mutation sweep, captured at commit 9b0a424 (the tree the sweep scored). Each row says: the removed test executes N source functions that had scored mutants, those functions had M scored mutants total, and the entire test file killed 0 of them
The three edited files pass standalone before the change at 9b0a424 (each pair's mutation run requires a green baseline before scoring) and after the change at HEAD:
Type
✅ Test
Changes
tests/test_litellm/caching/test_redis_cache.py loses 12 zero-kill tests around async_increment, batch_get_cache, and the rpush and lpop pipelines; their assertions only echoed their own mocks. The four namespace regression tests from #31288 stay
tests/test_litellm/llms/pass_through/guardrail_translation/test_handler.py loses its four handler-behavior classes, whose tests asserted mock delegation rather than outcomes, plus the helper and imports only they used. The registry tests remain
tests/test_litellm/proxy/test_proxy_server.py loses test_embedding_input_array_of_tokens
Final Attestation