Skip to content

test(unit): run tests/unit with only an allowlisted host environment - #43299

Closed
yuneng-berri wants to merge 1 commit into
mainfrom
litellm_unit_tests_hermetic_env
Closed

yuneng-berri wants to merge 1 commit into
mainfrom
litellm_unit_tests_hermetic_env

Conversation

@yuneng-berri

@yuneng-berri yuneng-berri commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

TLDR

Problem this solves:

  • CircleCI unit fails on main from CircleCI's own project env vars
  • Real xAI, Redis, AWS and Azure values reach tests that assume none
  • Locally, a .env anywhere up the tree leaks in the same way

How it solves it:

  • tests/unit/conftest.py drops every env var outside a small allowlist
  • It also sets PYTHON_DOTENV_DISABLED before litellm is imported

Pre-Submission checklist

  • I have added meaningful tests
  • The handful of test files covering my change pass locally, e.g. uv run pytest tests/unit/<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)

Screenshots / Proof of Fix

Shared setup: every one of the 95 CircleCI project env var names set to a fake value (REDIS_PORT numeric, DATABASE_URL a URL), plus a .env at the repo root with XAI_API_KEY, REDIS_HOST and REDIS_PORT, then the modules that fail on main run the way the unit job runs them

env -i HOME=$HOME PATH=$PATH PYTHONPATH=$PWD $(cat ci_fake_env.txt) python -m pytest \
  tests/unit/llms/xai/test_xai_oauth.py tests/unit/test_redis.py tests/unit/integrations/test_s3_v2.py \
  tests/unit/test_utils.py tests/unit/test_logging.py tests/unit/test_video_generation.py \
  tests/unit/caching/test_redis_cache.py tests/unit/integrations/websearch_interception/test_websearch_chat_completion.py \
  tests/unit/proxy/test_proxy_server.py -q -n 4 --dist=loadscope

Before (1e6c983)

  1. 14 failed, 1162 passed: the xAI OAuth tests pick up the real key, the Redis sentinel tests pick up REDIS_HOST, the S3 and Azure token tests pick up cloud credentials
  2. main's scheduled CircleCI unit job fails the same tests (job 2218776 and the 06:09 UTC run at 99655b6)

After (1b3fab9)

  1. 1175 passed, 0 failed under the same env and .env
  2. The CircleCI unit job on this PR is the full-suite check

Type

Test
Infrastructure

Caveats (if any)

Medium

  • Not every main unit red is env leakage
    • test_proxy_server auth, uvicorn access log and video logging tests pass alone even with the CI env, so they look like cross-module state leaks under xdist; this PR's CI run shows what remains

Low

  • A unit test that genuinely reads a host variable outside the allowlist now sees it unset; set it with monkeypatch inside the test instead

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

CircleCI's unit job inherits every project env var, so real provider keys,
REDIS_HOST, DATABASE_URL and AWS or Azure credentials reached tests that
assume none are set. Locally, litellm's import-time load_dotenv did the same
from any .env up the tree. The unit conftest now drops every variable outside
a small allowlist and disables dotenv before litellm is imported.
@yuneng-berri
yuneng-berri requested a review from a team September 26, 2026 08:36
@yuneng-berri

Copy link
Copy Markdown
Contributor Author

Folded into #43294, which now carries every stale-test and CI-hygiene fix in one place, so closing this duplicate

@greptile-apps

greptile-apps Bot commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 4/5

[Low risk] Test setup filters environment variables during unit tests.

The PR should not merge until the MCP shard retains its explicitly configured legacy-peer interpreter.

Findings

  1. P1 Legacy MCP peer setting removed ▶
  2. P2 Audio test opt-out removed ▶

Summary

The PR clears non-allowlisted host environment variables and disables dotenv before importing LiteLLM for unit tests.

  • This isolates unit tests from ambient credentials, but also removes environment controls used by existing test paths.

Reviews (1) · Last reviewed commit: "test(unit): run tests/unit with only an ..."

@codecov

codecov Bot commented Sep 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Comment thread tests/unit/conftest.py
Comment on lines +20 to +22
for _name in tuple(os.environ):
if _name not in HOST_ENVIRONMENT_ALLOWLIST and not _name.startswith(HOST_ENVIRONMENT_ALLOWED_PREFIXES):
del os.environ[_name]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Legacy MCP peer setting removed

The mcp-integration CI shard supplies MCP_TEST_PEER_PYTHON to select its separately installed MCP 1.28.1 interpreter. This loop deletes that setting, so the MCP e2e test falls back to sys.executable and starts the peer with the suite’s MCP 2.2.0 environment. The shard no longer tests the intended legacy-peer compatibility and may fail against the wrong SDK.

Comment thread tests/unit/conftest.py
Comment on lines +20 to +22
for _name in tuple(os.environ):
if _name not in HOST_ENVIRONMENT_ALLOWLIST and not _name.startswith(HOST_ENVIRONMENT_ALLOWED_PREFIXES):
del os.environ[_name]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Audio test opt-out removed

If a runner sets SKIP_AUDIO_TESTS=true because soundfile is unavailable, this loop removes the setting before the audio test’s skip condition is evaluated. The test then runs instead of being skipped and can fail because the dependency is missing.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant