Skip to content

test(logging): pick this test's own records out of the shared log batch - #41487

Merged
yuneng-berri merged 3 commits into
mainfrom
litellm_isolate_generic_api_ndjson_test
Sep 16, 2026
Merged

yuneng-berri merged 3 commits into
mainfrom
litellm_isolate_generic_api_ndjson_test

Conversation

@yuneng-berri

Copy link
Copy Markdown
Contributor

TLDR

Problem this solves:

  • Two logging tests fail depending on which file ran first
  • They assume the flushed batch holds only their own records

How it solves it:

  • Select each test's records by the messages it sent
  • Format and payload assertions stay, order stops mattering

User Flow

This is a test-only change. No runtime behavior changes, so there is no end user flow to describe

Relevant issues

Affected release

Linear ticket

Pre-Submission checklist

  • I have added meaningful tests
  • The handful of test files covering my change pass locally
  • My PR passes all required CI/CD checks
  • My PR's scope is as isolated as possible; it only solves 1 specific problem
  • I have received a Greptile Confidence Score of at least 4/5 before requesting a maintainer review

Screenshots / Proof of Fix

This PR only touches a test file, so there is nothing to exercise against a live proxy: runtime behavior is identical before and after. The bug is ordering inside one pytest process, so the proof is the contaminated order going red to green, plus a mutation check that the relaxed assertions still bite

The logging_testing job runs every file under tests/logging_callback_tests in one process, so a file that logs before these tests leaves records in the generic logger's queue. test_bedrock_knowledgebase_hook.py is one such file

Before (4bf04b2)

contaminated order: an extra failure appears in the generic file

  1. uv run pytest tests/logging_callback_tests/test_bedrock_knowledgebase_hook.py tests/logging_callback_tests/test_generic_api_callback.py -q
  2. Output:
FAILED tests/logging_callback_tests/test_generic_api_callback.py::test_generic_api_callback
8 failed, 15 passed
  1. The 7 bedrock failures are pre-existing locally (no AWS credentials) and are the same 7 that file produces on its own, so the generic file contributes the 8th

on CI the leak landed on the sibling test instead

AssertionError: Expected 2 lines of NDJSON, got 4
 +  where 4 = len(['{"id": "chatcmpl-...", "model": "claude-3.5-sonnet", ...

The test only ever called gpt-5.5, so the claude-3.5-sonnet records came from another file

After (baca62d)

contaminated order: the generic file no longer contributes a failure

  1. uv run pytest tests/logging_callback_tests/test_bedrock_knowledgebase_hook.py tests/logging_callback_tests/test_generic_api_callback.py -q
  2. Output:
7 failed, 16 passed

the generic file on its own

  1. uv run pytest tests/logging_callback_tests/test_generic_api_callback.py -q
  2. Output:
10 passed in 21.49s

the relaxed assertions still catch a dropped record

  1. Temporarily change data = "\n".join(safe_dumps(log) for log in self.log_queue) to self.log_queue[:1] in litellm/integrations/generic_api/generic_api_callback.py
  2. uv run pytest tests/logging_callback_tests/test_generic_api_callback.py -q
  3. Output:
FAILED tests/logging_callback_tests/test_generic_api_callback.py::test_generic_api_callback_ndjson_format
FAILED tests/logging_callback_tests/test_generic_api_callback.py::test_generic_api_callback_sumologic_uses_ndjson
2 failed, 8 passed
  1. Revert the edit

Type

✅ Test

Caveats (if any)

Low

  • This makes the two tests order-proof, it does not stop the leak
    • The generic logger's queue is still shared across files in the job
    • Another test that reads the batch positionally could hit the same thing

Final Attestation

  • The tests check the right things, including the edge cases, and regressions in the respective real-world customer use-cases are not possible after this PR

The generic API logger batches whatever is queued when it flushes, so records
from tests in other files in the same job land in the same request. Two tests
assumed otherwise: one read actual_request[0], the other counted NDJSON lines,
and both broke whenever another file logged first. Select by the messages each
test sent instead, which keeps the format assertions and stops the order from
deciding the outcome.
@yuneng-berri
yuneng-berri requested a review from a team September 16, 2026 20:54
@codecov

codecov Bot commented Sep 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@greptile-apps

greptile-apps Bot commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR updates two logging tests to select their own records from shared batches by matching request messages

  • Retains payload validation and NDJSON parsing while allowing unrelated records in the batch
  • The latest revision adds the required Final annotations and import, fully addressing the previous finding without changing test behavior

Confidence Score: 5/5

The PR appears safe to merge, with the previous annotation finding fully addressed and no new blocking issues

The latest changes only add a supported typing import, local annotations, and formatting. The previous finding is fully fixed, and no additional repository-rule violations were identified

Important Files Changed
Filename Overview
tests/logging_callback_tests/test_generic_api_callback.py Filters shared batches to each test’s records and adds the required Final annotations; no new actionable issues found

Reviews (2): Last reviewed commit: "style(test): annotate the new locals as ..." | Re-trigger Greptile

Comment thread tests/logging_callback_tests/test_generic_api_callback.py Outdated
@yuneng-berri

Copy link
Copy Markdown
Contributor Author

@greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants