Repository navigation
test(e2e): report batch cleanup leftovers as a plain UserWarning on rc/1.104.0 - #43406
Conversation
The leftover warning used a class defined in a test-directory module. The xdist controller cannot import it, so an uncaught leftover warning crashed the whole e2e run. Same change as #43391 on rc/1.103.0
|
| @@ -141,7 +140,7 @@ def test_delete_refused_because_a_batch_still_references_the_file_is_left_and_re | |||
| calls=ExpectedCalls((f"delete None {MANAGED_FILE_ID}",)), | |||
There was a problem hiding this comment.
pytest.warns(UserWarning) also accepts custom subclasses, so this test and the batch-leftover test would still pass if cleanup brought back a warning type that the xdist controller cannot import. Check that each captured warning has the exact type UserWarning. The repository requires changes to existing tests not to weaken regression coverage; that requirement should be met before merging.
Rule Used: What: Flag any modifications to existing tests and verify they don't weaken test coverage or mask regressions. Why: Developers may alter tests to make failing code pass rather than fix the actual bug, hiding regressions. Good: ``` // Test updated t... (source)
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!
TLDR
Problem this solves:
How it solves it:
UserWarninginstead ofBatchCleanupLeftoverUser Flow
Before: a release qualifier running the litellm-e2e Buildkite pipeline gets a crashed run instead of test results
INTERNALERRORandModuleNotFoundError: No module named 'batch_cleanup', and the retry step then fails with "no artifacts found"After: the same leftover shows up as a warning and the run finishes with real results
UserWarningnaming the file id and keeps going to a pass/fail verdictRelevant issues
Same change as #43391, which already fixed this on rc/1.103.0. The main copy is its own PR
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
uv run pytest tests/unit/<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)The existing
tests/e2e/batches/test_batch_cleanup.pycovers both leftover paths and passes 50/50 on this branchDelays 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
Before (e0cbb0d)
6d672483c9, which has the samebatch_cleanup.pyas this base: https://buildkite.com/berriai-1/litellm-e2e/builds/313After (2263772)
pytest tests/e2e/batches/test_batch_cleanup.py -q50 passed, including the two leftover cases that now assert aUserWarningwhose message names the left file and batchbuiltins, which the xdist controller can always importType
✅ Test
Caveats (if any)
Low
Final Attestation