Skip to content

test: add six ruff rules that catch tests which cannot fail - #37709

Merged
ryan-crabbe-berri merged 1 commit into
litellm_internal_stagingfrom
litellm_ruff_vacuous_assert_rules
Aug 20, 2026
Merged

test: add six ruff rules that catch tests which cannot fail#37709
ryan-crabbe-berri merged 1 commit into
litellm_internal_stagingfrom
litellm_ruff_vacuous_assert_rules

Conversation

@ryan-crabbe-berri

Copy link
Copy Markdown
Contributor

TLDR

Problem this solves:

  • assert False in a try: is caught by its own except
  • Bare a == b statements evaluate and throw the result away
  • 50 such sites in tests/, several unable to ever fail

How it solves it:

  • Selects 6 more ruff rules in ruff-tests.toml, already wired to CI
  • Rewrites every assert False as pytest.fail, which escapes except Exception
  • Restores the missing assert on 9 dead comparisons
  • One test asserted True == True; now it tests its module

User Flow

Before: a proxy admin scopes MCP servers to a team, the gateway stops honoring that scope, and no test objects

  1. They POST /team/new, then attach two MCP servers to the team through the Admin UI
  2. A member of that team lists tools and sees zero servers instead of the two granted
  3. A separate developer disables OpenAI container file errors so a missing file quietly returns nothing
  4. Their app calls the container file content route for a deleted file, gets an empty body back instead of a 404, and writes it to disk as if it were real content
  5. Both regressions land green, because the tests covering them either compare True == True or hide their failure inside their own except Exception

After: both regressions are caught before merge

  1. The same two changes are made
  2. CI runs the same test files and both go red, naming the team whose servers vanished and the container file that should have errored
  3. Any new test written with either shape is rejected at lint time, so the next one cannot reach the proxy either

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 proof is four mutations: break a behavior a test exists to police, and show the old test does not notice while the new one does. Every command is identical on both sides; only the tests/ tree differs.

The four mutations, applied one at a time and reverted after each run:

mcp_team_servers        litellm/proxy/_experimental/mcp_server/db.py
                        `return mcp_servers or []` -> `return []`
custom_llm_api_key      litellm/images/main.py
                        custom_handler.aimage_generation(api_key=api_key) -> (api_key=None)
pattern_wildcard        litellm/router_utils/pattern_match_deployments.py
                        `if "*" not in litellm_deployment_litellm_model:` -> `if True:`
container_file_content  litellm/containers/endpoint_factory.py
                        retrieve_container_file_content returns None instead of raising

container_file_content hits the real OpenAI containers API with a live key: it creates a container, asks for a file that does not exist, and deletes the container.

Before (4af59d7)

Team MCP server grants stop working, test still green

  1. Apply mcp_team_servers, then run
pytest tests/test_litellm/proxy/db/mcp_server/test_db.py -q
  1. Output:
1 passed, 2 warnings in 1.64s

Custom provider stops receiving the caller's api_key, test still green

  1. Apply custom_llm_api_key, then run
pytest tests/local_testing/test_custom_llm.py::test_image_generation_async_additional_params -q
  1. Output:
1 passed, 3 warnings in 0.04s

Wildcard routing stops substituting the model, test still green

  1. Apply pattern_wildcard, then run
pytest tests/local_testing/test_router_pattern_matching.py::test_router_pattern_match_e2e -q
  1. Output:
1 passed, 1 warning in 0.08s

Container file errors get swallowed, test still green

  1. Apply container_file_content, then run against the live OpenAI API
pytest tests/llm_translation/test_containers_api.py::test_container_files_api -q
  1. Output:
1 passed, 1 warning in 1.53s

After (634131b)

Team MCP server grants stop working, test now fails

  1. Apply mcp_team_servers and run the same command:
pytest tests/test_litellm/proxy/db/mcp_server/test_db.py -q
  1. Output:
____________ test_fetch_mcp_servers_by_team[team_record4-expected4] ____________
1 failed, 4 passed, 2 warnings in 1.96s

Custom provider stops receiving the caller's api_key, test now fails

  1. Apply custom_llm_api_key and run the same command:
pytest tests/local_testing/test_custom_llm.py::test_image_generation_async_additional_params -q
  1. Output:
>           assert mock_client.call_args.kwargs["api_key"] == "my-api-key"
E           AssertionError: assert None == 'my-api-key'
1 failed, 3 warnings in 0.10s

Wildcard routing stops substituting the model, test now fails

  1. Apply pattern_wildcard and run the same command:
pytest tests/local_testing/test_router_pattern_matching.py::test_router_pattern_match_e2e -q
  1. Output:
>           assert request_body["model"] == "my-custom-model"
E           AssertionError: assert '*' == 'my-custom-model'
1 failed, 1 warning in 0.24s

Container file errors get swallowed, test now fails

  1. Apply container_file_content and run the same command against the live OpenAI API:
pytest tests/llm_translation/test_containers_api.py::test_container_files_api -q
  1. Output:
3. Testing retrieve_container_file (expect error)...
   Got expected error ✓
3b. Testing retrieve_container_file_content (expect error)...
>               pytest.fail("Should have raised error for non-existent file content")
E               Failed: Should have raised error for non-existent file content
1 failed, 1 warning in 2.42s

The gate is clean and would catch the next one

  1. Run the CI step's exact command:
ruff check --config ruff-tests.toml tests
All checks passed!
  1. Same rules on the merge base, for the count this PR started from:
ruff check --isolated --select B011,B015,B018,PT015,PLR0133,PLW0127 tests --statistics
27	B011   	assert-false
27	PT015  	pytest-assert-always-false
 9	B015   	useless-comparison
 5	PLW0127	self-assigning-variable
 3	B018   	useless-expression
 1	PLR0133	comparison-of-constant
Found 72 errors.

Touched files are unchanged in pass/fail

  1. Run all 23 edited test files on this branch and on the merge base:
pytest $(cat touched.txt) -q
66 failed, 1551 passed, 14 skipped, 22 rerun in 296.25s
  1. The same 66 node ids fail identically on 4af59d7c6e, on absent Gemini, Bedrock, Azure and Databricks credentials. comm over the two FAILED lists reports no id that fails only after this PR.

Type

🧹 Refactoring
🚄 Infrastructure
✅ Test

Caveats (if any)

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

`assert False` inside a `try:` raises AssertionError, which the `except
Exception` right below it catches, so several tests reported green no matter
what the code did. `pytest.fail` raises Failed, a BaseException, and escapes.

A bare `a == b` statement is evaluated and discarded. Nine of those sat in
tests, and one was comparing against a model name the router never produces.

Selects B011, B015, B018, PT015, PLR0133 and PLW0127 in ruff-tests.toml
alongside F821, with all 50 existing violations fixed, so no budget file or
ratchet is needed. CI already runs this config over tests/.
@greptile-apps

greptile-apps Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR strengthens test reliability by enabling six Ruff checks for ineffective test statements and repairing the existing violations.

  • Replaces failure assertions that broad exception handlers could swallow with pytest.fail.
  • Restores assertions to previously discarded comparisons and removes dead expressions and self-assignments.
  • Replaces the placeholder MCP database test with parametrized coverage of nullable and populated team permissions.
  • Adds the new checks to the test-specific Ruff configuration already used by CI.

Confidence Score: 5/5

The PR appears safe to merge, with the changed tests becoming stricter and no concrete regressions identified.

The lint configuration is wired to the existing test-lint command, swallowed failure paths now escape correctly, and the newly active assertions match the exercised production behavior.

Important Files Changed

Filename Overview
ruff-tests.toml Enables six additional rules targeting ineffective assertions, comparisons, expressions, and self-assignments in tests.
tests/llm_translation/test_containers_api.py Uses pytest.fail so missing expected API errors can no longer be swallowed by the surrounding broad exception handlers.
tests/local_testing/test_router_pattern_matching.py Parses the outgoing request and adds active assertions for the routed model and transformed message payload.
tests/test_litellm/proxy/db/mcp_server/test_db.py Replaces a constant placeholder assertion with parametrized coverage of team MCP-server lookup behavior and database-call arguments.
tests/test_team.py Restores an active assertion for the updated team membership count using the post-update team snapshot.
tests/local_testing/test_custom_llm.py Restores assertions that verify custom image-generation credentials and optional parameters are forwarded.
tests/test_litellm/passthrough/test_passthrough_main.py Restores validation of the URL supplied to the passthrough HTTP request.
tests/local_testing/test_secret_detect_hook.py Restores validation that detected secrets are redacted from callback data.

Reviews (1): Last reviewed commit: "test: add six ruff rules that catch test..." | Re-trigger Greptile

@codecov

codecov Bot commented Aug 20, 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 enabled auto-merge (squash) August 20, 2026 21:18

@tin-berri tin-berri left a comment

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.

Adds 6 ruff rules catching tests that structurally cannot fail (assert False inside try/except swallowing AssertionError, bare == comparisons with no assert) and fixes all 50 existing violations by hand. The proof here is unusually strong — 4 real mutations against actual production bugs (MCP team-server-scope grant broken, custom LLM api_key silently dropped, wildcard routing not substituting, container-file-not-found error swallowed hitting the live OpenAI API) each show the OLD test staying green and the NEW test catching it. Confirms the 66 pre-existing failures across touched files are identical before/after (env-credential-gated, not introduced by this PR). This is exactly the kind of test-quality PR that's worth taking seriously since it found real latent bugs while fixing itself. CI green.

@ryan-crabbe-berri
ryan-crabbe-berri merged commit 21e9632 into litellm_internal_staging Aug 20, 2026
67 checks passed
@ryan-crabbe-berri
ryan-crabbe-berri deleted the litellm_ruff_vacuous_assert_rules branch August 20, 2026 21:21
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