Skip to content

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

Open
yuneng-berri wants to merge 3 commits into
litellm_internal_stagingfrom
litellm_env_writes_to_monkeypatch
Open

test: unwind environment writes in tests/test_litellm with monkeypatch#37806
yuneng-berri wants to merge 3 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
Collaborator

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 direct test environment writes with pytest monkeypatch operations so mutations unwind automatically, and lowers the TQ004 quality budget accordingly.

  • Converts 262 direct environment writes to monkeypatch.setenv or monkeypatch.delenv.
  • Preserves inherited values at all five sites implicated by the previous review thread.
  • Reduces the TQ004 ceiling from 768 to 506.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains; the previously reported environment-restoration issue is fixed because all five affected tests now let monkeypatch record and restore inherited values.

Important Files Changed

Filename Overview
tests/test_litellm/enterprise/enterprise_callbacks/send_emails/test_resend_email.py Uses monkeypatch.delenv before testing missing-key behavior, allowing pytest teardown to restore an inherited RESEND_API_KEY.
tests/test_litellm/enterprise/enterprise_callbacks/send_emails/test_sendgrid_email.py Replaces pop-and-manual-restore logic with monkeypatch.delenv, preserving inherited SENDGRID_API_KEY state.
tests/test_litellm/proxy/auth/test_login_utils.py Deletes UI_PASSWORD through monkeypatch so the original worker environment is restored after the test.
tests/test_litellm/test_add_deployment_no_master_key.py Uses monkeypatch-managed deletion for LITELLM_SALT_KEY, avoiding loss of inherited state.
tests/test_litellm/test_count_tokens_public_api.py Uses monkeypatch-managed deletion for OPENAI_API_KEY, preserving its pre-test value at teardown.
test-quality-budget.json Ratchets the TQ004 ceiling down by the same 262 violations removed by the test updates.

Reviews (2): Last reviewed commit: "fix(test): delete the key through monkey..." | 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
Collaborator 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.
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.

1 participant