ci: gate tests/e2e on zero basedpyright errors in pre-commit and lint CI - #32918
Conversation
Greptile SummaryThis PR adds a zero-error basedpyright gate for
Confidence Score: 5/5Safe to merge — changes are scoped entirely to CI infrastructure and the e2e test harness, with no modifications to production litellm code. All changes are in CI config, Makefile, and the e2e test harness. The type fixes preserve existing behavior. The _CliArgs pydantic model is safe because --format has default='text' so model_validate will always succeed. The raise AssertionError(last) replacements are correct because Success is always returned earlier in each match block. No files require special attention.
|
| Filename | Overview |
|---|---|
| .github/workflows/test-linting.yml | Adds a new CI step that runs basedpyright over tests/e2e only when the PR touches those Python files; the diff-gating and dep-install wiring looks correct |
| Makefile | Adds lint-e2e-basedpyright target to lint-checks; LINT_E2E_DEP_INSTALL override correctly propagated through the parallel lint invocation |
| scripts/pre_commit_lint.sh | Adds e2e staged-file trigger; correctly skips when litellm_py_files is set because make lint already runs lint-e2e-basedpyright via lint-checks |
| pyproject.toml | Adds e2e-dev dependency group with playwright and websockets, formalizing previously ad-hoc pip installs |
| tests/e2e/management/management_client.py | Replaces unreachable unwrap(last) calls with explicit raise AssertionError(last); correct since Success is always returned earlier in the match |
| tests/e2e/coverage_registry/collector.py | Uses pydantic _CliArgs model to validate argparse namespace (format has default=text so validation won't fail); marker.args typing cleaned up with object-typed intermediates |
| tests/e2e/llm_translation/realtime/realtime_client.py | Renames _ws_base_url to ws_base_url to fix private-name import violations; dict[str, Any] to dict[str, object] |
| tests/e2e/models.py | Adds AnthropicMessagesResponse pydantic model to replace json.loads()['model'] access in test_budget_fallback_e2e.py |
Reviews (1): Last reviewed commit: "ci: gate tests/e2e on zero basedpyright ..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
f604034
into
litellm_internal_staging
Relevant issues
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
Before, at base
a4199d3c09, the harness claimed "fully typed" in its docs but nothing enforced it:103 of those were phantom errors from playwright and websockets not being installed in the lint env (both are real dependencies of the suite, previously installed ad hoc via
uv pip install); the remaining 24 were genuine violations (reportAny, reportUnknown*, reportPrivateUsage, reportPossiblyUnbound)After, at
55c8ca41b5, with the deps declared in the newe2e-devgroup and the violations fixed:The gate actually gates; injecting a type error and re-running fails, and reverting passes again:
make pre-commitpicks the check up when tests/e2e Python is staged, without requiring litellm/ changes:In CI the same check runs inside the existing lint job, scoped by
git diffagainst the PR base totests/e2e/**/*.py, so PRs that do not touch the harness skip it. This PR touches those files, so the step runs live hereType
🧹 Refactoring
🚄 Infrastructure
Changes
tests/e2e documents the harness as fully typed with no
Any, but basedpyright never ran over it in CI or pre-commit (pyrightconfig.json only includes litellm/), so 24 real violations had accumulated, and the two optional dependencies the suite imports (playwright for the dashboard UI tests, websockets for the realtime client) were not declared anywhere, which degraded another 103 diagnostics into unknown-type noise for anyone who ran the checker by handThis PR adds a zero-error basedpyright gate for tests/e2e. Unlike litellm/ there is no budget file; the tree is small and fully typed, so any error fails. Wiring: a new
lint-e2e-basedpyrightMakefile target (part oflint-checks, with its ownLINT_E2E_DEP_INSTALLdep so standalone runs sync the right dependency groups), atests/e2e/**/*.pystaged-file trigger inscripts/pre_commit_lint.sh, and a step in test-linting.yml's lint job that runs only when the PR changed e2e Python files (nothing else can change the tree's type health; the harness does not import litellm). playwright and websockets now live in a newe2e-devdependency group, synced bylint-installand the CI lint job, so the checker resolves the same modules everywhere; tests/e2e/CONTRIBUTING.md's playwright install instructions now point at the groupThe 24 violations are fixed with behavior preserved. Notable ones: the retry loops in management_client.py and budget_client.py ended with
_ = unwrap(last)after amatchthat already returned onSuccess, so the type var could never bind and the call could only ever raise; it is now an explicitraise AssertionError(last)with the same message. test_cache_control.py's poll loop leftsecondpossibly unbound in its failure message; it now always runs at least one iteration._ws_base_urlwas imported by sibling modules despite the private name, so it is public now. TheAnys fromjson.loads,yaml.safe_load, argparse, and pytest marker args are replaced with pydantic validation (TypeAdapter,model_validate) orobject-typed intermediates, per the harness convention of modelling just the fields a test readsAll 34 non-live harness tests pass unchanged, and the registry collector CLI was exercised end to end in both the happy path and the invalid-format path