Skip to content

test(cost-calc): stop 182 global writes leaking out of the cost-calc suites - #37815

Merged
yuneng-berri merged 3 commits into
litellm_internal_stagingfrom
litellm_cost_calc_global_writes
Aug 22, 2026
Merged

test(cost-calc): stop 182 global writes leaking out of the cost-calc suites#37815
yuneng-berri merged 3 commits into
litellm_internal_stagingfrom
litellm_cost_calc_global_writes

Conversation

@yuneng-berri

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

Copy link
Copy Markdown
Contributor

TLDR

Problem this solves:

  • 58 tests set an env var and never put it back
  • 18 more hand-roll copy-then-restore around two globals
  • A hand-rolled restore is skipped when the test fails

How it solves it:

  • One monkeypatch fixture the env-var tests ask for
  • monkeypatch.setattr for the two config globals

User Flow

No end-user behavior changes. Both suites report the same counts, 89 and 174,
and a test that runs after them in the same worker now starts from the
environment it would have had on its own.

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: run a suite, then read the process back in the same interpreter.
What is left over is what the next test in that worker inherits.

uv run python -c '
import os, litellm, pytest
before = {k: getattr(litellm, k) for k in ("cost_margin_config", "cost_discount_config")}
pytest.main(["tests/test_litellm/test_cost_calculator.py", "-q", "-p", "no:randomly"])
print("LITELLM_LOCAL_MODEL_COST_MAP after the run:", os.environ.get("LITELLM_LOCAL_MODEL_COST_MAP", "<unset>"))
for k, v in before.items():
    print(f"litellm.{k} after the run:", "restored" if getattr(litellm, k) == v else "LEAKED")
'

Before (ff02d5c)

the leak the suites leave behind

  1. The command above
89 passed, 356 warnings in 0.47s
LITELLM_LOCAL_MODEL_COST_MAP after the run: True
litellm.cost_margin_config after the run: restored
litellm.cost_discount_config after the run: restored
  1. The same command against tests/test_litellm/litellm_core_utils/llm_cost_calc/test_llm_cost_calc_utils.py
174 passed, 2 warnings in 0.96s
LITELLM_LOCAL_MODEL_COST_MAP after the run: True

what the budget rules see

  1. uv run python scripts/check_test_quality.py tests/test_litellm/test_cost_calculator.py tests/test_litellm/litellm_core_utils/llm_cost_calc/test_llm_cost_calc_utils.py | grep -o 'TQ00[0-9]' | sort | uniq -c
   2 TQ003
  75 TQ004
 105 TQ005

After (eee8cb9)

the leak the suites leave behind

  1. The same command
89 passed, 356 warnings in 0.44s
LITELLM_LOCAL_MODEL_COST_MAP after the run: <unset>
litellm.cost_margin_config after the run: restored
litellm.cost_discount_config after the run: restored
  1. The same command against the other file
174 passed, 2 warnings in 0.96s
LITELLM_LOCAL_MODEL_COST_MAP after the run: <unset>

what the budget rules see

  1. The same count
(no output: neither file has a TQ violation left)
  1. uv run python scripts/test_quality_gate.py
OK: every TQ rule is within its test-suite ceiling (base origin/litellm_internal_staging)

Type

✅ Test

🧹 Refactoring

Caveats (if any)

  • The two config globals already came back, on both sides
  • The 117-entry conftest snapshot is what was catching them
  • So that half is about not needing the snapshot, not a live leak
  • Neutering the cost-map fixture leaves all 174 green either way
  • TQ003 drops 3, one of which was stale before this branch
  • TQ005's ceiling stays 4 under its count, exactly as on staging

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

@greptile-apps

greptile-apps Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR replaces hand-written environment and global-state restoration in the cost-calculation suites with pytest monkeypatch fixtures.

  • Restores the local model-cost-map environment and module global after each affected test.
  • Uses monkeypatch for cost margin and discount configuration changes.
  • Tightens the test-quality budgets to reflect the removed direct mutations.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
test-quality-budget.json Lowers the TQ003, TQ004, and TQ005 ceilings to match the removed test anti-patterns.
tests/test_litellm/litellm_core_utils/llm_cost_calc/test_llm_cost_calc_utils.py Centralizes local model-cost-map setup and cleanup in a monkeypatch-backed fixture.
tests/test_litellm/test_cost_calculator.py Replaces direct environment and LiteLLM configuration mutations with automatically restored monkeypatch operations.

Reviews (2): Last reviewed commit: "test(cost-calc): stop 182 global writes ..." | Re-trigger Greptile

Comment thread tests/test_litellm/litellm_core_utils/llm_cost_calc/test_llm_cost_calc_utils.py Outdated
@yuneng-berri
yuneng-berri force-pushed the litellm_cost_calc_global_writes branch from 247b09d to eee8cb9 Compare August 21, 2026 10:34
@yuneng-berri yuneng-berri changed the title test(cost-calc): stop 96 global writes leaking out of the cost-calc suite test(cost-calc): stop 182 global writes leaking out of the cost-calc suites Aug 21, 2026
@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 added a commit that referenced this pull request Aug 21, 2026
…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.
…suites

Across test_cost_calculator.py and llm_cost_calc/test_llm_cost_calc_utils.py,
58 tests opened by setting LITELLM_LOCAL_MODEL_COST_MAP in os.environ and
replacing litellm.model_cost, and none of them put the env var back. The
second file already had a _local_model_cost_map fixture doing it by hand with
a try/finally, so both idioms sat in the same file.

Keep that fixture, give it monkeypatch, and have every one of those tests ask
for it. The margin and discount tests drop their hand-rolled
copy-then-restore in favour of monkeypatch.setattr, which also puts the
global back when an assertion fails part way through.

Both files also drop a sys.path.insert whose argument resolves outside the
repo, so it was never what made the imports work.

TQ003 1077 -> 1075, TQ004 768 -> 693, TQ005 2836 -> 2731, and the budget
ceilings come down with them.
@yuneng-berri
yuneng-berri force-pushed the litellm_cost_calc_global_writes branch from eee8cb9 to 2c4038a Compare August 21, 2026 10:59
@yuneng-berri

Copy link
Copy Markdown
Contributor Author

@greptileai

yuneng-berri added a commit that referenced this pull request Aug 22, 2026
#37806)

* test: use monkeypatch.setenv for env writes in tests/test_litellm

`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.

* fix(test): delete the key through monkeypatch instead of popping it first

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.

* chore(test): leave the two cost-calc files to the PR that rewrites them 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.
…itellm_cost_calc_global_writes

# Conflicts:
#	test-quality-budget.json
The local_cost_map fixture set LITELLM_LOCAL_MODEL_COST_MAP but never reloaded
litellm.model_cost, and reading the variable is not what loads the map. So the
three streaming-cost tests billed against whatever map the process happened to
be holding, and their hardcoded prices only held when something else had
already swapped in the checked-in one. This branch stops the cost-calc tests
leaking that map, which left test_main billing at the ambient prices instead.

The fixture now loads the map it names, so the prices these tests assert hold
on their own.
@yuneng-berri
yuneng-berri merged commit 0c97eea into litellm_internal_staging Aug 22, 2026
67 checks passed
@yuneng-berri
yuneng-berri deleted the litellm_cost_calc_global_writes branch August 22, 2026 04:00
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