Skip to content

test(audit-logs): let monkeypatch own the audit log and s3 callback globals - #37842

Merged
yuneng-berri merged 1 commit into
litellm_internal_stagingfrom
litellm_audit_log_callbacks_test_globals
Aug 22, 2026
Merged

test(audit-logs): let monkeypatch own the audit log and s3 callback globals#37842
yuneng-berri merged 1 commit into
litellm_internal_stagingfrom
litellm_audit_log_callbacks_test_globals

Conversation

@yuneng-berri

Copy link
Copy Markdown
Contributor

TLDR

Problem this solves:

  • Fifteen tests assign three litellm callback globals directly
  • Two autouse fixtures exist largely to put those globals back
  • What a test sets and what undoes it live in different places

How it solves it:

  • monkeypatch.setattr sets and restores at the point of use
  • The fixtures keep only the cache clears monkeypatch cannot do
  • 22 TQ005 violations in the file drop to zero

User 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 27 pass either way

Relevant issues

Linear ticket

Pre-Submission checklist

  • I have added meaningful tests
  • The handful of test files covering my change pass locally, e.g. 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
  • My PR passes all required CI/CD checks (e.g., lint, schema.d.ts sync check, etc.)
  • 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 (Greptile reviews automatically once the PR is opened; only comment @greptileai to 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 autouse fixtures already restored these globals, so the run reads "restored" on both sides. That is the point. The bookkeeping was correct and sitting far from the code it covered, and monkeypatch does the same job where the value is set. What changes is the count the quality rules see.

Shared setup: run the file, read the globals back in the same interpreter, then count what the quality rules see in it.

uv run python -c '
import litellm, pytest
ATTRS = ("audit_log_callbacks", "s3_callback_params", "s3_audit_callback_params")
before = {a: getattr(litellm, a, None) for a in ATTRS}
pytest.main(["tests/test_litellm/proxy/management_helpers/test_audit_log_callbacks.py", "-q", "-p", "no:randomly"])
after = {a: getattr(litellm, a, None) for a in ATTRS}
print("globals after the run:", "restored" if after == before else "LEAKED")
'
uv run python scripts/check_test_quality.py tests/test_litellm/proxy/management_helpers/test_audit_log_callbacks.py | grep -oE "TQ00[0-9]" | sort | uniq -c

Before (ff02d5c)

  1. The first command
27 passed, 4 warnings in 2.70s
globals after the run: restored
  1. The second command
   3 TQ001
   9 TQ002
  22 TQ005

After (52832d9)

  1. The first command
27 passed, 4 warnings in 2.85s
globals after the run: restored
  1. The second command
   3 TQ001
   9 TQ002
  1. uv run python scripts/test_quality_gate.py --base origin/litellm_internal_staging
OK: every TQ rule is within its test-suite ceiling (base origin/litellm_internal_staging)

Type

🧹 Refactoring
✅ Test

Caveats (if any)

  • The file's TQ001 and TQ002 counts are untouched, left for their own change

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

…lobals

Fifteen tests assigned litellm.audit_log_callbacks, s3_callback_params or
s3_audit_callback_params directly and leaned on two autouse fixtures to put
them back. monkeypatch.setattr does that at the point of use, so each test now
says what it sets, including the one that swaps the value mid-test to prove the
cache does not serve the stale params.

The fixtures keep only the work monkeypatch cannot do: the per-test empty
callback list, and clearing the logger and audit caches around each test.
@greptile-apps

greptile-apps Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This test-only refactor delegates restoration of audit-log and S3 callback globals to pytest's monkeypatch fixture while retaining explicit cache cleanup

  • Replaces direct global assignments with monkeypatch.setattr
  • Removes redundant manual restoration logic from autouse fixtures
  • Ratchets the TQ005 quality budget down by the 22 removed violations

Confidence Score: 5/5

The PR appears safe to merge because the test state remains isolated and the quality-budget reduction matches the removed violations

Monkeypatch restores each modified global after dependent fixtures finish, while explicit teardown leaves both logger caches empty and preserves the previous isolation behavior

Important Files Changed

Filename Overview
tests/test_litellm/proxy/management_helpers/test_audit_log_callbacks.py Replaces direct callback-global mutation with monkeypatch-managed state without changing test assertions or leaving stale caches
test-quality-budget.json Lowers the TQ005 ceiling by 22 to match the direct global assignments removed from the test file

Reviews (1): Last reviewed commit: "test(audit-logs): let monkeypatch own th..." | Re-trigger Greptile

@codecov

codecov Bot commented Aug 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@yuneng-berri
yuneng-berri merged commit 49da936 into litellm_internal_staging Aug 22, 2026
67 checks passed
@yuneng-berri
yuneng-berri deleted the litellm_audit_log_callbacks_test_globals branch August 22, 2026 03:16
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.

3 participants