Skip to content

test: unwind environment writes in tests/test_litellm with monkeypatch - #37806

Merged
yuneng-berri merged 4 commits into
litellm_internal_stagingfrom
litellm_env_writes_to_monkeypatch
Aug 22, 2026
Merged

test: unwind environment writes in tests/test_litellm with monkeypatch#37806
yuneng-berri merged 4 commits into
litellm_internal_stagingfrom
litellm_env_writes_to_monkeypatch

Conversation

@yuneng-berri

@yuneng-berri yuneng-berri commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

TLDR

Problem this solves:

  • 768 tests write straight into os.environ
  • The value outlives the test, in that worker
  • So a later test reads a key it never set
  • TQ004 has sat at its seed since the rule shipped

How it solves it:

  • 262 of those writes become monkeypatch.setenv, which unwinds at teardown
  • Ratchet the TQ004 ceiling from 768 to 568

User Flow

No end-user behavior changes and no test behavior changes. The same 1,732 tests
pass, and one fewer environment variable survives the session.

Relevant issues

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 (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

Screenshots / Proof of Fix

Shared setup: a leak counter, run as a pytest plugin, so the two sides are
comparable rather than asserted.

import os

_before = {}


def pytest_sessionstart(session):
    _before.update(os.environ)


def pytest_sessionfinish(session, exitstatus):
    added = sorted(set(os.environ) - set(_before))
    changed = sorted(k for k in _before if k in os.environ and os.environ[k] != _before[k])
    print("\nENV LEAKED AFTER SESSION: %d added, %d changed" % (len(added), len(changed)))
    for k in added:
        print("  + %s" % k)
    for k in changed:
        print("  ~ %s" % k)

Both sides run the same 38 files, in the same order, with the same command:

python -m pytest $(cat tq004_files.txt | tr '\n' ' ') -p no:randomly -q -p envleak

Before (ff02d5c)

The rule's count

  1. python scripts/check_test_quality.py tests | grep -c ' TQ004 '
768

What the session leaves behind

  1. The 38 files, run in order:
1469 passed, 15 warnings in 336.70s (0:05:36)

ENV LEAKED AFTER SESSION: 2 added, 1 changed
  + APISERPENT_API_KEY
  + LITELLM_LOCAL_MODEL_COST_MAP
  ~ LITELLM_LICENSE
  1. LITELLM_LICENSE was already set on the machine, and a test overwrote it for
    every test that ran after it. APISERPENT_API_KEY was invented by a test and
    left behind

After (7219e7a)

The rule's count

  1. python scripts/check_test_quality.py tests | grep -c ' TQ004 '
568

What the session leaves behind

  1. The same 38 files, same order:
1469 passed, 15 warnings in 388.93s (0:06:28)

ENV LEAKED AFTER SESSION: 0 added, 0 changed
  1. Nothing is left behind. An earlier revision of this PR read the remaining
    LITELLM_LOCAL_MODEL_COST_MAP as conftest-owned; it was not, it came from
    the two cost-calc files that now belong to test(cost-calc): stop 182 global writes leaking out of the cost-calc suites #37815

The ratchet

  1. python scripts/test_quality_gate.py --update
Ratcheted TQ-rule limits down by 262 violations this branch fixed
  1. 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

✅ Test

Caveats (if any)

  • Four shapes are skipped, each because the fixture would not bind
  • A mock.patch family decorator wraps the test, so pytest may not inject
  • A defaulted positional would shift, so the value lands wrong
  • A nested def can shadow the name, so the inner one wins
  • A test called directly by another test gets no fixture at all
  • 506 writes remain, mostly inside those four shapes
  • Five pop-then-restore tests now use delenv, so nothing is dropped

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

`os.environ["X"] = v` inside a test leaks the value into every test that runs
after it in the same worker, so ordering decides the result. 262 of those
writes across 40 files now go through pytest's `monkeypatch` fixture, which
restores the previous value at teardown.

The rewrite skips any test that a mock.patch-family decorator wraps, any test
with defaulted positional parameters, any test whose own name is called
directly elsewhere, and rebinds nothing inside nested defs, because in each of
those cases appending a fixture parameter changes what pytest or mock binds.

Ratchets the TQ004 ceiling from 768 to 506.
@greptile-apps

greptile-apps Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR replaces persistent test environment writes with pytest monkeypatch operations and lowers the corresponding test-quality budget.

  • Converts direct environment assignments and deletions across the LiteLLM test suite to teardown-safe monkeypatch operations.
  • Correctly fixes restoration of pre-existing RESEND_API_KEY, SENDGRID_API_KEY, and UI_PASSWORD values.
  • Reduces the TQ004 ceiling from 768 to 568.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains; the previously reported environment-restoration issue is fixed at the current head.

Important Files Changed

Filename Overview
test-quality-budget.json Lowers the TQ004 ceiling to reflect the direct environment writes removed by this PR.
tests/test_litellm/enterprise/enterprise_callbacks/send_emails/test_resend_email.py Uses monkeypatch.delenv so RESEND_API_KEY is absent during the test and its inherited state is restored afterward.
tests/test_litellm/enterprise/enterprise_callbacks/send_emails/test_sendgrid_email.py Uses monkeypatch.delenv to test missing-key behavior without losing a pre-existing SENDGRID_API_KEY.
tests/test_litellm/proxy/auth/test_login_utils.py Removes UI_PASSWORD through monkeypatch inside the patched environment, preserving fallback coverage and restoring the worker’s original state.

Reviews (3): Last reviewed commit: "chore(test): leave the two cost-calc fil..." | 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!

…irst

Five tests popped a key straight out of `os.environ`, ran, then restored it with
`monkeypatch.setenv`. By the time monkeypatch saw the name it was already gone,
so it recorded "absent" as the value to go back to and deleted the key at
teardown. On a worker that inherited a real `RESEND_API_KEY`, `SENDGRID_API_KEY`,
`UI_PASSWORD`, `LITELLM_SALT_KEY` or `OPENAI_API_KEY`, every test after the first
one ran without it.

`monkeypatch.delenv(..., raising=False)` removes the key and restores whatever
was there, so the try/finally the manual restore needed goes with it.
@yuneng-berri

Copy link
Copy Markdown
Contributor Author

Fixed: all five pop-then-restore sites now use monkeypatch.delenv, so the inherited value survives teardown. @greptileai

…em fully

Both files are also in #37815, which converts the module-global writes as well
as the env writes and folds them into one fixture. Two PRs rewriting the same
lines differently is a conflict nobody benefits from resolving, so this one
drops back to staging on those two and keeps the other 39.

TQ004 clears 200 here instead of 275; the rest moves with #37815.
@yuneng-berri

Copy link
Copy Markdown
Contributor Author

@greptileai

@yuneng-berri

Copy link
Copy Markdown
Contributor Author

@greptileai please review the current head, no review has landed on it yet

…itellm_env_writes_to_monkeypatch

# Conflicts:
#	test-quality-budget.json
@yuneng-berri
yuneng-berri merged commit 6937974 into litellm_internal_staging Aug 22, 2026
68 checks passed
@yuneng-berri
yuneng-berri deleted the litellm_env_writes_to_monkeypatch branch August 22, 2026 03:28
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