test(lint): ban blind pytest.raises(Exception) with ruff B017 - #37731
Conversation
A bare pytest.raises(Exception) accepts whatever the body throws. The TypeError a refactor introduces satisfies it exactly as well as the rejection the test was written for, so the crash reads as a pass and the test never goes red. All 111 existing sites are narrowed here. A runtime probe recorded the concrete exception each one actually catches, and each site now names that type. Where the code under test genuinely raises a bare Exception, the site pins a stable slice of the message with match= instead. Two sites tell on themselves. The shared responses-API cancel test raises "custom_llm_provider is required but passed as None" rather than talking to a provider at all, because cancel_responses takes a provider, not a model. And test_bedrock_guardrails_with_streaming was the only test in its file still passing without AWS credentials, because the NoCredentialsError boto3 raised long before the guardrail ran satisfied the blind raises.
Greptile SummaryThis PR enables Ruff B017 for tests and narrows broad
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| ruff-tests.toml | Enables B017 in the existing test lint configuration to reject blind broad-exception assertions. |
| tests/test_litellm/caching/test_redis_cache.py | Narrows Redis failure assertions and separately validates the transition to an open circuit breaker. |
| tests/proxy_unit_tests/test_proxy_utils.py | Narrows validation assertions and records the currently observed mock-induced TypeError behavior. |
| tests/guardrails_tests/test_eu_ai_act_article5.py | Narrows guardrail rejection assertions to HTTPException. |
| tests/guardrails_tests/test_semantic_guard.py | Narrows semantic guard rejection assertions to HTTPException. |
Reviews (2): Last reviewed commit: "test(lint): ban blind pytest.raises(Exce..." | Re-trigger Greptile
| from litellm.types.proxy.guardrails.guardrail_hooks.litellm_content_filter import ( | ||
| ContentFilterCategoryConfig, | ||
| ) | ||
| from fastapi import HTTPException |
There was a problem hiding this comment.
Proxy dependency imported outside proxy
These guardrail tests now import FastAPI directly outside proxy/, coupling SDK test collection to a proxy-only dependency. The pattern also occurs in three sibling files.
Rule Used: What: Do not allow fastapi imports on files outsid... (source)
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
The fastapi import isn't a boundary violation: test_lakera_v2.py, test_zscaler_ai_guard.py and test_lasso_guardrails.py already import HTTPException the same way, and no lint gate forbids it. |
|
@greptile re review |
The narrowed NotFoundError only holds where OPENAI_API_KEY is set. Without one the SDK raises OpenAIError while building the client, long before any 404, so CI went red. OpenAIError covers both and still rejects a TypeError from a refactor.
Two whole-tree test lints are red on litellm_internal_staging, which blocks the lint job on every PR into it. test_user_api_key_auth.py used pytest.raises(Exception) with no match=. B017 forbids that (enforced since #37731): any Exception subtype, including one from an unrelated regression, satisfies the assert and reads as a pass. Narrowed with match=r"(?i)budget", which preserves the original `assert "budget" in str(exc.value).lower()` it replaces. test_unit_test_max_model_budget_limiter.py wrapped an if/else with two different awaited calls inside pytest.raises(). PT012 forbids that (enforced since #37748): the block must hold a single simple statement, so a coroutine built in the wrong branch cannot silently never run. The coroutine is now built outside the block and awaited inside it. Both violations landed in #37736, one day before ruff-tests.toml began enforcing these rules whole-tree, so no delta-vs-base gate caught them. Verified: `ruff check --config ruff-tests.toml tests` is clean, both tests pass, and each still fails under an injected regression.
TLDR
Problem this solves:
pytest.raises(Exception)passes on any crash, not the rejectionTypeErrorsatisfies it as well as the real errortests/, several never reaching the code they nameHow it solves it:
B017inruff-tests.toml, already wired to CImatch=where the code genuinely raises a bareExceptionUser Flow
Before: a developer cancels a response id the gateway never issued, and nothing checks what comes back
After: the same change turns CI red before a developer ever sees it
Relevant issues
Linear ticket
Pre-Submission checklist
Screenshots / Proof of Fix
This PR changes lint config and tests, so there is no proxy route to curl. The proof is three 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 three mutations, applied one at a time and reverted after each run:
Before (21e9632)
A key-update validator starts failing with the wrong error class
validator_wrong_error, then runA malformed team entry crashes instead of being rejected
team_guard_dropped, then runThe Redis breaker hides which failure the caller saw
breaker_rewraps, then runAfter (ef07d8c)
A key-update validator starts failing with the wrong error class
validator_wrong_error, then runA malformed team entry crashes instead of being rejected
team_guard_dropped, then runThe Redis breaker hides which failure the caller saw
breaker_rewraps, then runRule and regression numbers
ruff check --config ruff-tests.toml testswent from 111B017findings to zero. Ruff has no autofix for this rule, so every site was edited by hand or by a script driven off the probe.The 55 touched test files were run in full at both hashes. 34 tests fail identically on each side, all of them live-provider tests with no Gemini, Bedrock, Azure or Databricks credentials on this machine. Exactly one test differs:
That is the finding, not a regression. 14 of the 15 tests in that file already fail here without AWS credentials; this one passed only because the
NoCredentialsErrorboto3 raised, long before the guardrail ran, satisfiedpytest.raises(Exception). With credentials present it exercises the guardrail block it was written for.Type
🧹 Refactoring
🚄 Infrastructure
✅ Test
Caveats (if any)
Exceptionunder anoqa: the code raisesException()with no messageTypeErrorfrom their mocks, never the DB error they nameFinal Attestation