Skip to content

test: enforce F811 so a duplicate definition cannot silently replace the first - #37878

Merged
ryan-crabbe-berri merged 1 commit into
litellm_internal_stagingfrom
litellm_ruff_no_duplicate_definitions
Aug 21, 2026
Merged

test: enforce F811 so a duplicate definition cannot silently replace the first#37878
ryan-crabbe-berri merged 1 commit into
litellm_internal_stagingfrom
litellm_ruff_no_duplicate_definitions

Conversation

@ryan-crabbe-berri

Copy link
Copy Markdown
Contributor

TLDR

Problem this solves:

  • A name bound twice keeps only the second binding
  • A local can shadow an import the module still calls
  • 344 such sites, and nothing stops the next one

How it solves it:

  • Selects ruff F811 in ruff-tests.toml, already wired to CI
  • 311 were repeated imports and came out with ruff's own fix
  • 33 needed a decision: shadowed imports, rebound closures, fixtures
  • Load-test fixtures move to a conftest, which is how pytest shares them

User Flow

Before: a developer edits the watsonx credential lookup and the test that checks which secrets it reads can stop compiling its own assertion

  1. They set WATSONX_API_KEY and send POST https://litellm-domain/v1/chat/completions for a watsonx model
  2. The call succeeds, because the gateway read the key from the environment
  3. Someone changes which environment names that lookup tries, and the test guarding it builds its expected list with a variable named call that has quietly replaced the call helper the same file imports
  4. The next person to use that helper in the file gets a confusing failure that has nothing to do with their change, because the name no longer means what the import says it means

After: the shadowing is rejected before it lands

  1. The same edit is made
  2. CI flags the local that took over an imported name, so the file keeps one meaning per name
  3. The same check refuses a second def test_x that would silently replace the first, so a test cannot disappear by being renamed onto its neighbour

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

This PR changes lint config and tests, so there is no proxy route to curl. The check that matters for a change made mostly of deletions is that no test went missing and no module stopped importing, so both sides collect the whole suite and the counts are compared.

Before (4b29702)

  1. Run
pytest tests --collect-only -q --continue-on-collection-errors
  1. Output:
46989/46991 tests collected (2 deselected), 70 errors in 20.12s

After (e9d40a8)

  1. Run
pytest tests --collect-only -q --continue-on-collection-errors
  1. Output:
46989/46991 tests collected (2 deselected), 70 errors in 41.76s
  1. Run
ruff check --config ruff-tests.toml tests
  1. Output:
All checks passed!

Rule and regression numbers

ruff check --config ruff-tests.toml tests went from 344 F811 findings to zero. Identical collection on both sides: the same 46989 tests, and the same 70 pre-existing collection errors, all of them modules that fail to import for missing credentials or optional dependencies. Nothing new and nothing gone.

Type

🧹 Refactoring
🚄 Infrastructure
✅ Test

Caveats (if any)

  • Nine prisma_client parameters keep a narrow noqa with the reason
  • tests/load_tests/conftest.py is new, so pytest shares those fixtures
  • 311 of 344 sites are ruff's own safe fix, so the diff is mostly deletions

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

…the first

A name bound twice keeps only the second binding. In `tests/` that is nearly
always a repeated import, harmless but misleading, and the same rule is what
catches the cases that are not harmless: a local that shadows an import the
module still calls, and a second `def test_x` that quietly replaces the first.

311 of the 344 sites were repeated imports and came out with ruff's own fix.
The remaining 33 needed a decision. Four modules imported a name they never
used because a local definition below already shadowed it. Two comprehensions
bound `call` over `unittest.mock.call`, which those modules import and use.
One test rebound the two module handles its nested reload closure had captured.
One class attribute shadowed an unused `status` import.

The load-test fixtures move to a conftest, which is how pytest is meant to share
them, so the test module no longer imports three fixture names it never calls.
The nine `prisma_client` parameters keep a narrow `noqa`: pytest resolves that
fixture by name before the body runs, so the parameter never shadows anything.
@greptile-apps

greptile-apps Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Too many files changed for review (172 files, 100 file limit).

Bypass the limit by tagging @greptile-apps to review.

@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!

@ryan-crabbe-berri
ryan-crabbe-berri merged commit ed02a12 into litellm_internal_staging Aug 21, 2026
67 checks passed
@ryan-crabbe-berri
ryan-crabbe-berri deleted the litellm_ruff_no_duplicate_definitions branch August 21, 2026 19:49
yuneng-berri added a commit that referenced this pull request Aug 22, 2026
* test: unshadow the module handles the F811 sweep left behind, and pin the two live tests that went red with it

The F811 sweep in #37878 removed the fixture-local `import litellm` from four
conftests, but the bare `import litellm.proxy.proxy_server` a few lines below
still binds `litellm` as a function local, so `importlib.reload(litellm)` runs
before the name is assigned and every test in those directories errors at
setup. The `hasattr` guard on the line above already proves the module is
loaded, so the import only ever bound the name. Drop it, and enable F823 in
ruff-tests.toml, which flags all four sites at the failing line and would have
blocked the sweep

The same sweep renamed the `check_non_streaming_response` parameter but left
one read of `completion`, which now resolves to `litellm.completion`, and
removed an import whose side effect was the only thing making
`litellm.proxy.proxy_server` reachable in the moderation hook test. That test
already takes `monkeypatch`, so patch the router through it and stop leaking
the router into later tests

`test_content_policy_exception_openai` passed vacuously until #37887 turned it
into a real `pytest.raises`, and OpenAI no longer rejects a lyrics prompt with
a content policy error. Inject an AsyncOpenAI client whose transport answers
with OpenAI's own `content_policy_violation` rejection so the mapping to
ContentPolicyViolationError is exercised every run

`test_async_create_batch` hit a 409 cancelling a batch OpenAI had already
marked failed. The cancel step tolerated a completed batch but not a failed
one. Fold both guards into one helper that tolerates a failed batch only when
OpenAI's recorded error is the org's enqueued token limit, and prints the
batch's errors so the reason is in the log either way

* test: close the injected AsyncOpenAI client after the content policy test

* chore(lint): ratchet TQ005 down by the global mutation this branch cleared

* chore(lint): ratchet TQ005 to 2660 on the merged tree

* chore(lint): ratchet TQ005 to 2561 on the merged tree

* chore(lint): ratchet TQ005 to 2548 on the merged tree
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.

2 participants