test(interactions): drop the save/restore scaffolding around the legacy flag - #37841
Conversation
…cy flag Seven tests captured litellm.use_legacy_interactions_schema, wrapped their body in a try, and put it back in a finally. monkeypatch.setattr does that, so the capture, the try and the finally go and the bodies lose an indentation level. The remaining hand-rolled restores stay. They hold the flag only across the iterator's constructor and put it back before the test iterates, so handing them to monkeypatch would widen that window to the whole test and change what the streaming assertions run against.
Greptile SummaryThis test-only refactor replaces manual save/restore blocks for the legacy interactions-schema flag with pytest’s function-scoped monkeypatch fixture while preserving the existing assertions and cleanup behavior.
Confidence Score: 5/5The PR appears safe to merge because the fixture teardown preserves test isolation and the quality-budget adjustment matches the removed violations. The changed tests retain the same flag values and assertions while pytest restores the previous module attribute after each test, and the fourteen-point TQ005 reduction corresponds exactly to the removed qualifying assignments.
|
| Filename | Overview |
|---|---|
| tests/test_litellm/interactions/test_gemini_interactions_transformation.py | Replaces seven manual global-flag restoration blocks with equivalent pytest monkeypatch lifecycle management without changing tested behavior. |
| test-quality-budget.json | Ratchets the TQ005 ceiling down by fourteen, exactly matching the fourteen assignments removed from the test file. |
Reviews (1): Last reviewed commit: "test(interactions): drop the save/restor..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…itellm_gemini_interactions_test_globals # Conflicts: # test-quality-budget.json
TLDR
Problem this solves:
How it solves it:
monkeypatch.setattrreplaces the capture and the finallyUser Flow
This one is test-only scaffolding with no user-visible behavior, so there is no before and after flow to walk. The tests assert the same things on the same code, and the same 36 pass either way
Relevant issues
Linear ticket
Pre-Submission checklist
uv run pytest tests/test_litellm/<your_test_file>.py -v. Leave the suites (make test-unit-*,make test-unit) to CI: it finishes in ~15 minutes where a laptop takes an hour or more@greptileaito re-request a review after pushing changes)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
To be plain about what this does and does not prove: the try/finally it removes already restored the global, so the run reads "restored" on both sides. That is the point. The bookkeeping was correct and hand-written around every body, and
monkeypatchdoes the same job where the value is set. What changes is the count the quality rules see.Shared setup: run the file, read the global back in the same interpreter, then count what the quality rules see in it.
Before (ff02d5c)
After (83743ab)
uv run python scripts/test_quality_gate.py --base origin/litellm_internal_stagingType
🧹 Refactoring
✅ Test
Caveats (if any)
Final Attestation