Merge pull request #39870 from BerriAI/litellm_lit_6917_make_check_test_tree_ruff

fix(make check): lint the test tree on tests-only changes like CI does
This commit is contained in:
Mateo Wang 2026-09-07 10:55:28 -07:00 committed by GitHub
commit 8e7f748e8a
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 305 additions and 16 deletions

View file

@ -12,6 +12,10 @@
# - litellm/ Python -> `make lint` (test-linting.yml's lint job)
# - tests/e2e Python -> `make lint-e2e-basedpyright` (test-linting.yml's e2e type-check step)
# + raw HTTP client ban (test-code-quality.yml's check_e2e_no_raw_requests)
# - tests/ Python, ruff-tests.toml, test-quality-budget.json, scripts/check_test_quality.py,
# scripts/test_quality_gate.py
# -> ruff over ruff-tests.toml + `make lint-test-quality` (test-linting.yml's
# test-tree ruff and test-quality budget steps)
# - dashboard -> prettier + eslint + lint budgets (test-litellm-ui-build.yml's frontend-lint)
# - proxy/types -> regenerate the lazy OpenAPI snapshot and dashboard API types, fail on drift (check-ui-api-types.yml)
#
@ -88,15 +92,14 @@ existing_files() {
litellm_py_pattern='^litellm/.*\.py$'
e2e_py_pattern='^tests/e2e/.*\.py$'
test_tree_pattern='^(tests/.*\.py|ruff-tests\.toml|test-quality-budget\.json|scripts/(check_test_quality|test_quality_gate)\.py)$'
spec_pattern='^(litellm/(proxy|types)/.*|ui/litellm-dashboard/(scripts/gen-api-types\.mjs|package\.json|package-lock\.json|src/lib/http/schema\.d\.ts))$'
ui_prettier_pattern='^ui/litellm-dashboard/.*\.(js|jsx|ts|tsx|mjs|cjs|json|css|scss|md|mdx|yml|yaml|html)$'
ui_eslint_pattern='^ui/litellm-dashboard/.*\.(js|jsx|ts|tsx|mjs|cjs)$'
# CI's lint job (test-linting.yml) only inspects litellm/, so a tests-only or
# scripts-only commit can't turn it red; scope the trigger there to skip the slow
# make lint when it couldn't catch anything.
litellm_py_files=$(scope_match "$litellm_py_pattern")
e2e_py_files=$(scope_match "$e2e_py_pattern")
test_tree_files=$(scope_match "$test_tree_pattern")
# ruff format (and CI's format step) skip enterprise; the rest of make lint covers it.
fmt_files=$(printf '%s\n' "$litellm_py_files" | grep -v '^litellm/enterprise/' | existing_files)
# check-ui-api-types.yml triggers on any file under litellm/proxy or litellm/types
@ -136,6 +139,7 @@ if [ -n "$staged" ]; then
}
warn_skipped "Python lint (make lint)" "$litellm_py_pattern" "$litellm_py_files"
warn_skipped "tests/e2e checks (basedpyright + raw HTTP client ban)" "$e2e_py_pattern" "$e2e_py_files"
warn_skipped "test-tree lint (ruff-tests.toml + test-quality budget)" "$test_tree_pattern" "$test_tree_files"
warn_skipped "dashboard lint (prettier + eslint + lint budgets)" "$ui_prettier_pattern" "$ui_prettier_changed"
warn_skipped "dashboard API-type sync (npm run gen:api)" "$spec_pattern" "$spec_files"
fi
@ -288,6 +292,15 @@ if [ -n "$spec_files" ]; then
set +m
fi
if [ -n "$test_tree_files" ] && [ -z "$litellm_py_files" ]; then
echo "check: linting the test tree (ruff check --config ruff-tests.toml tests)"
uv run --no-sync ruff check --config ruff-tests.toml tests \
|| { echo "✗ Test-tree ruff failed. Fix the errors above, then re-run make check." >&2; status=1; }
echo "check: checking the test-quality budget (make lint-test-quality)"
make lint-test-quality \
|| { echo "✗ Test-quality budget failed. Fix the errors above, then re-run make check." >&2; status=1; }
fi
if [ -n "${python_pid:-}" ]; then
wait "$python_pid" || status=1
cat "$python_log"; rm -f "$python_log"
@ -313,10 +326,12 @@ summary_item() {
echo "check: summary"
summary_item "Python lint (make lint)" "$litellm_py_files" "no litellm/ Python files in scope"
summary_item "tests/e2e checks (basedpyright + raw HTTP client ban)" "$e2e_py_files" "no tests/e2e Python files in scope"
summary_item "test-tree lint (ruff-tests.toml + test-quality budget)" "$test_tree_files" \
"no tests/ Python files or test-tree lint inputs in scope"
summary_item "dashboard lint (prettier + eslint + lint budgets)" "$ui_prettier_changed$ui_eslint_changed" "no dashboard files in scope"
summary_item "dashboard API-type sync (npm run gen:api)" "$spec_files" "no litellm/proxy, litellm/types, or generator files in scope"
if [ -z "$litellm_py_files$e2e_py_files$ui_prettier_changed$ui_eslint_changed$spec_files" ]; then
if [ -z "$litellm_py_files$e2e_py_files$test_tree_files$ui_prettier_changed$ui_eslint_changed$spec_files" ]; then
echo "check: NOTE - no gating lint check matches the files in scope, so nothing ran:" >&2
printf '%s\n' "$scope" | sed 's/^/ /' >&2
echo " A pass here is a no-op, not a lint verdict." >&2

View file

@ -29,13 +29,14 @@ import argparse
import json
import re
import shutil
import signal
import subprocess
import sys
import tempfile
from collections import Counter
from collections.abc import Mapping, Sequence
from pathlib import Path
from types import MappingProxyType
from types import FrameType, MappingProxyType
from typing import Final, NamedTuple
REPO_ROOT: Final = Path(__file__).resolve().parent.parent
@ -43,6 +44,7 @@ CHECKER: Final = REPO_ROOT / "scripts" / "check_test_quality.py"
BUDGET_PATH: Final = REPO_ROOT / "test-quality-budget.json"
TARGET: Final = "tests"
DEFAULT_BASE: Final = "origin/litellm_internal_staging"
TERMINATION_SIGNALS: Final = (signal.SIGTERM, signal.SIGHUP)
_HUNK: Final = re.compile(r"^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@", re.MULTILINE)
_FILE_HEADER: Final = re.compile(r"^\+\+\+ b/(.+)$", re.MULTILINE)
@ -111,22 +113,33 @@ def count_by_rule(violations: Sequence[Violation]) -> Mapping[str, int]:
return MappingProxyType(dict(Counter(v.code for v in violations)))
def base_counts(ref: str) -> Mapping[str, int]:
def _exit_on_termination(signum: int, _frame: FrameType | None) -> None:
raise SystemExit(128 + signum)
def _install_termination_handlers() -> None:
for termination in TERMINATION_SIGNALS:
if signal.getsignal(termination) == signal.SIG_DFL:
signal.signal(termination, _exit_on_termination)
def base_counts(ref: str, repo_root: Path = REPO_ROOT, checker: Path = CHECKER) -> Mapping[str, int]:
"""Rule counts at `ref`, measured with the *current* rule logic rather than
whatever the checker looked like at that commit."""
_install_termination_handlers()
parent: Final = Path(tempfile.mkdtemp(prefix="tq_base_"))
worktree: Final = parent / "wt"
try:
_run(["git", "worktree", "add", "--detach", str(worktree), ref])
_run(["git", "worktree", "add", "--detach", str(worktree), ref], cwd=repo_root)
(worktree / "scripts").mkdir(parents=True, exist_ok=True)
checker: Final = worktree / "scripts" / "check_test_quality.py"
shutil.copy(CHECKER, checker)
return count_by_rule(_check(worktree, checker))
base_checker: Final = worktree / "scripts" / "check_test_quality.py"
shutil.copy(checker, base_checker)
return count_by_rule(_check(worktree, base_checker))
finally:
# Teardown must never raise, or it masks the real error when the body failed.
subprocess.run(
["git", "worktree", "remove", "--force", str(worktree)],
cwd=REPO_ROOT, capture_output=True, text=True,
cwd=repo_root, capture_output=True, text=True,
)
shutil.rmtree(parent, ignore_errors=True)

View file

@ -11,6 +11,12 @@ import pytest
ROOT = Path(__file__).resolve().parents[2]
SCRIPT = ROOT / "scripts" / "pre_commit_lint.sh"
WHOLE_TREE_RUFF = "run --no-sync ruff check --config ruff-tests.toml tests"
TEST_TREE_RAN = "ran: test-tree lint (ruff-tests.toml + test-quality budget)"
TEST_TREE_SKIPPED = (
"skipped: test-tree lint (ruff-tests.toml + test-quality budget) "
"(no tests/ Python files or test-tree lint inputs in scope)"
)
BARRIER_HELPER = """barrier_sync() {
touch "$STUB_BARRIER_DIR/$1.started"
@ -30,6 +36,7 @@ BARRIER_HELPER = """barrier_sync() {
MAKE_STUB = """#!/bin/sh
. "$STUB_BIN/barrier.sh"
[ -n "${STUB_ARGS_DIR:-}" ] && echo "$*" >> "$STUB_ARGS_DIR/make.args"
case "$*" in
lint)
[ "${STUB_FAIL:-}" = "make-lint" ] && exit 1
@ -40,6 +47,9 @@ case "$*" in
sleep 60
fi
;;
lint-test-quality)
[ "${STUB_FAIL:-}" = "test-quality" ] && exit 1
;;
esac
exit 0
"""
@ -69,6 +79,10 @@ case "$*" in
*orjson*)
[ -n "${STUB_BARRIER_DIR:-}" ] && barrier_sync genapi "python dashboard"
;;
"run --no-sync ruff check --config ruff-tests.toml"*)
[ -n "${STUB_ARGS_DIR:-}" ] && echo "$*" >> "$STUB_ARGS_DIR/ruff_tests.args"
[ "${STUB_FAIL:-}" = "tests-ruff" ] && exit 1
;;
esac
exit 0
"""
@ -153,6 +167,13 @@ def _set_base_ref(repo: Path) -> None:
)
def _stage_file(repo: Path, relative: str, body: str) -> None:
path = repo / relative
path.parent.mkdir(parents=True, exist_ok=True)
path.write_text(body)
subprocess.run(["git", "add", relative], cwd=repo, check=True)
def test_nothing_staged_scopes_to_working_tree_diff_and_runs_checks(tmp_path: Path) -> None:
repo, bin_dir = _sandbox(tmp_path)
_commit_all(repo, "base")
@ -405,6 +426,7 @@ def test_run_ends_with_a_summary_of_ran_and_skipped_blocks(tmp_path: Path) -> No
assert "ran: dashboard lint (prettier + eslint + lint budgets)" in proc.stdout
assert "ran: dashboard API-type sync (npm run gen:api)" in proc.stdout
assert "skipped: tests/e2e checks (basedpyright + raw HTTP client ban) (no tests/e2e Python files in scope)" in proc.stdout
assert TEST_TREE_SKIPPED in proc.stdout
assert "check: PASS" in proc.stdout
assert "check: FAIL" not in proc.stdout
@ -412,20 +434,146 @@ def test_run_ends_with_a_summary_of_ran_and_skipped_blocks(tmp_path: Path) -> No
def test_staged_files_matching_no_check_print_an_explicit_noop_note_and_nonempty_log(tmp_path: Path) -> None:
repo, bin_dir = _sandbox(tmp_path)
_commit_all(repo, "base")
tests_dir = repo / "tests" / "test_litellm"
tests_dir.mkdir(parents=True)
(tests_dir / "test_x.py").write_text("def test_x() -> None: ...\n")
subprocess.run(["git", "add", "tests"], cwd=repo, check=True)
_stage_file(repo, "scripts/tool.py", "def main() -> None: ...\n")
proc = _run(repo, bin_dir, {})
assert proc.returncode == 0, proc.stdout + proc.stderr
assert "no gating lint check matches the files in scope, so nothing ran" in proc.stdout
assert "tests/test_litellm/test_x.py" in proc.stdout
assert "scripts/tool.py" in proc.stdout
assert "a no-op, not a lint verdict" in proc.stdout
assert "check: PASS" in proc.stdout
assert "linting Python" not in proc.stdout
log = (repo / ".git" / "pre_commit_lint.log").read_text()
assert "check: summary" in log
assert "skipped: Python lint (make lint) (no litellm/ Python files in scope)" in log
assert TEST_TREE_SKIPPED in log
def _recorded(args_dir: Path, name: str) -> list[str]:
path = args_dir / name
return path.read_text().splitlines() if path.exists() else []
def test_tests_only_change_runs_the_whole_test_tree_ruff_and_the_quality_gate(tmp_path: Path) -> None:
repo, bin_dir = _sandbox(tmp_path)
_commit_all(repo, "base")
args_dir = tmp_path / "args"
args_dir.mkdir()
_stage_file(repo, "tests/test_a.py", "def test_a() -> None: ...\n")
_stage_file(repo, "tests/fixtures/data.json", "{}\n")
proc = _run(repo, bin_dir, {"STUB_ARGS_DIR": str(args_dir)})
assert proc.returncode == 0, proc.stdout + proc.stderr
assert _recorded(args_dir, "ruff_tests.args") == [WHOLE_TREE_RUFF]
assert _recorded(args_dir, "make.args") == ["lint-test-quality"]
assert TEST_TREE_RAN in proc.stdout
assert "no gating lint check matches" not in proc.stdout
assert "linting Python" not in proc.stdout
assert "check: PASS" in proc.stdout
@pytest.mark.parametrize(
"changed",
[
"ruff-tests.toml",
"test-quality-budget.json",
"scripts/check_test_quality.py",
"scripts/test_quality_gate.py",
"tests/e2e/test_x.py",
],
)
def test_test_tree_lint_inputs_trigger_the_test_tree_checks(tmp_path: Path, changed: str) -> None:
repo, bin_dir = _sandbox(tmp_path)
_commit_all(repo, "base")
args_dir = tmp_path / "args"
args_dir.mkdir()
_stage_file(repo, changed, "x = 1\n")
proc = _run(repo, bin_dir, {"STUB_ARGS_DIR": str(args_dir)})
assert proc.returncode == 0, proc.stdout + proc.stderr
assert _recorded(args_dir, "ruff_tests.args") == [WHOLE_TREE_RUFF]
assert "lint-test-quality" in _recorded(args_dir, "make.args")
assert TEST_TREE_RAN in proc.stdout
def test_nothing_staged_tests_only_working_tree_change_runs_the_test_tree_checks(tmp_path: Path) -> None:
repo, bin_dir = _sandbox(tmp_path)
_stage_file(repo, "tests/test_a.py", "def test_a() -> None: ...\n")
_commit_all(repo, "base")
_set_base_ref(repo)
args_dir = tmp_path / "args"
args_dir.mkdir()
(repo / "tests" / "test_a.py").write_text("def test_a() -> None:\n assert True\n")
proc = _run(repo, bin_dir, {"STUB_ARGS_DIR": str(args_dir)})
assert proc.returncode == 0, proc.stdout + proc.stderr
assert "nothing staged; scoping to the working tree's diff" in proc.stdout
assert _recorded(args_dir, "ruff_tests.args") == [WHOLE_TREE_RUFF]
assert _recorded(args_dir, "make.args") == ["lint-test-quality"]
def test_a_failing_test_tree_ruff_fails_the_run_and_still_runs_the_quality_gate(tmp_path: Path) -> None:
repo, bin_dir = _sandbox(tmp_path)
_commit_all(repo, "base")
args_dir = tmp_path / "args"
args_dir.mkdir()
_stage_file(repo, "tests/test_a.py", "def test_a() -> None: ...\n")
proc = _run(repo, bin_dir, {"STUB_ARGS_DIR": str(args_dir), "STUB_FAIL": "tests-ruff"})
assert proc.returncode == 1
assert "Test-tree ruff failed" in proc.stdout + proc.stderr
assert "check: FAIL" in proc.stdout
assert _recorded(args_dir, "make.args") == ["lint-test-quality"]
def test_a_failing_quality_gate_fails_a_tests_only_run(tmp_path: Path) -> None:
repo, bin_dir = _sandbox(tmp_path)
_commit_all(repo, "base")
_stage_file(repo, "tests/test_a.py", "def test_a() -> None: ...\n")
proc = _run(repo, bin_dir, {"STUB_FAIL": "test-quality"})
assert proc.returncode == 1
assert "Test-quality budget failed" in proc.stdout + proc.stderr
assert "check: FAIL" in proc.stdout
def test_tests_changed_alongside_litellm_files_defer_to_make_lint(tmp_path: Path) -> None:
repo, bin_dir = _sandbox(tmp_path)
_commit_all(repo, "base")
args_dir = tmp_path / "args"
args_dir.mkdir()
_stage_file(repo, "litellm/foo.py", "x = 2\n")
_stage_file(repo, "tests/test_a.py", "def test_a() -> None: ...\n")
proc = _run(repo, bin_dir, {"STUB_ARGS_DIR": str(args_dir), "STUB_FAIL": "test-quality"})
assert proc.returncode == 0, proc.stdout + proc.stderr
assert "linting Python" in proc.stdout
assert _recorded(args_dir, "ruff_tests.args") == []
assert _recorded(args_dir, "make.args") == ["lint"]
assert TEST_TREE_RAN in proc.stdout
def test_deleted_test_file_still_runs_the_test_tree_checks(tmp_path: Path) -> None:
repo, bin_dir = _sandbox(tmp_path)
_stage_file(repo, "tests/test_a.py", "def test_a() -> None: ...\n")
_commit_all(repo, "base")
args_dir = tmp_path / "args"
args_dir.mkdir()
subprocess.run(["git", "rm", "-q", "tests/test_a.py"], cwd=repo, check=True)
proc = _run(repo, bin_dir, {"STUB_ARGS_DIR": str(args_dir)})
assert proc.returncode == 0, proc.stdout + proc.stderr
assert _recorded(args_dir, "ruff_tests.args") == [WHOLE_TREE_RUFF]
assert _recorded(args_dir, "make.args") == ["lint-test-quality"]
assert TEST_TREE_RAN in proc.stdout
def test_partial_staging_warns_when_test_files_are_left_unstaged(tmp_path: Path) -> None:
repo, bin_dir = _sandbox(tmp_path)
_stage_file(repo, "tests/test_a.py", "def test_a() -> None: ...\n")
_commit_all(repo, "base")
args_dir = tmp_path / "args"
args_dir.mkdir()
_stage_file(repo, "notes.md", "hi\n")
(repo / "tests" / "test_a.py").write_text("def test_a() -> None:\n assert True\n")
proc = _run(repo, bin_dir, {"STUB_ARGS_DIR": str(args_dir)})
assert proc.returncode == 0, proc.stdout + proc.stderr
assert "SKIPPED test-tree lint (ruff-tests.toml + test-quality budget)" in proc.stdout
assert "tests/test_a.py" in proc.stdout
assert _recorded(args_dir, "ruff_tests.args") == []
assert _recorded(args_dir, "make.args") == []
def test_run_queues_through_the_machine_wide_gate_slot_lock(tmp_path: Path) -> None:

View file

@ -7,8 +7,15 @@ limit can never rise. Both live in pure functions, so they are tested directly:
"""
import importlib.util
import os
import signal
import subprocess
import sys
import time
from collections.abc import Callable
from contextlib import suppress
from pathlib import Path
from typing import NamedTuple
_REPO_ROOT = Path(__file__).resolve().parents[2]
_MODULE_PATH = _REPO_ROOT / "scripts" / "test_quality_gate.py"
@ -21,6 +28,16 @@ _spec.loader.exec_module(gate)
_BUDGET = {"TQ001": {"limit": 10}, "TQ003": {"limit": 5}}
_SCAN_BASE = (
"import importlib.util, pathlib, sys\n"
"spec = importlib.util.spec_from_file_location('test_quality_gate', sys.argv[1])\n"
"gate = importlib.util.module_from_spec(spec)\n"
"sys.modules[spec.name] = gate\n"
"spec.loader.exec_module(gate)\n"
"gate.base_counts('HEAD', repo_root=pathlib.Path(sys.argv[2]), checker=pathlib.Path(sys.argv[3]))\n"
)
_SCAN_BASE_WITH_SIGHUP_IGNORED = "import signal\nsignal.signal(signal.SIGHUP, signal.SIG_IGN)\n" + _SCAN_BASE
def test_a_rule_within_its_limit_is_not_a_breach():
assert gate.evaluate({"TQ001": 10}, {"TQ001": 10}, _BUDGET) == ()
@ -121,3 +138,99 @@ def test_the_shipped_budget_covers_every_rule_the_checker_can_emit():
budget = json.loads((_REPO_ROOT / "test-quality-budget.json").read_text())
assert set(budget) == {"TQ001", "TQ002", "TQ003", "TQ004", "TQ005", "TQ006", "TQ007", "TQ008"}
assert all(spec["limit"] >= 0 for spec in budget.values())
def _git(cwd: Path, *args: str) -> str:
proc = subprocess.run(["git", *args], cwd=cwd, capture_output=True, text=True)
assert proc.returncode == 0, proc.stderr
return proc.stdout.strip()
def _committed_repo(tmp_path: Path) -> Path:
repo = tmp_path / "repo"
(repo / "tests").mkdir(parents=True)
(repo / "tests" / "test_seed.py").write_text("def test_seed():\n assert True\n")
_git(repo, "init", "-q", "-b", "main")
_git(repo, "config", "user.email", "gate@example.com")
_git(repo, "config", "user.name", "gate")
_git(repo, "config", "commit.gpgsign", "false")
_git(repo, "add", "-A")
_git(repo, "commit", "-q", "-m", "seed")
return repo
def _wait_until(predicate: Callable[[], bool], timeout_seconds: float) -> bool:
deadline = time.monotonic() + timeout_seconds
while time.monotonic() < deadline:
if predicate():
return True
time.sleep(0.05)
return predicate()
def _reap(process: subprocess.Popen[bytes]) -> None:
with suppress(subprocess.TimeoutExpired):
process.wait(timeout=10)
if process.poll() is None:
process.kill()
process.wait(timeout=10)
def _registered_worktrees(repo: Path) -> int:
listing = _git(repo, "worktree", "list", "--porcelain")
return sum(line.startswith("worktree ") for line in listing.splitlines())
class _StalledScan(NamedTuple):
process: subprocess.Popen[bytes]
repo: Path
release: Path
temp_dir: Path
def _base_scan_stalled_in_its_checker(tmp_path: Path, driver: str) -> _StalledScan:
repo = _committed_repo(tmp_path)
scanning = tmp_path / "scanning"
release = tmp_path / "release"
slow_checker = tmp_path / "slow_checker.py"
slow_checker.write_text(
"import pathlib, time\n"
f"pathlib.Path({str(scanning)!r}).touch()\n"
f"while not pathlib.Path({str(release)!r}).exists():\n"
" time.sleep(0.05)\n"
)
temp_dir = tmp_path / "tmp"
temp_dir.mkdir()
scan = subprocess.Popen(
[sys.executable, "-c", driver, str(_MODULE_PATH), str(repo), str(slow_checker)],
env={**os.environ, "TMPDIR": str(temp_dir)},
)
if not _wait_until(scanning.exists, 30):
_reap(scan)
raise AssertionError("the base scan never reached the checker")
return _StalledScan(scan, repo, release, temp_dir)
def test_a_terminated_base_scan_still_removes_its_worktree(tmp_path: Path) -> None:
stalled = _base_scan_stalled_in_its_checker(tmp_path, _SCAN_BASE)
try:
stalled.process.send_signal(signal.SIGTERM)
assert stalled.process.wait(timeout=30) == 128 + signal.SIGTERM
finally:
_reap(stalled.process)
assert _registered_worktrees(stalled.repo) == 1
assert list(stalled.temp_dir.iterdir()) == []
def test_a_base_scan_keeps_ignoring_the_hangup_its_parent_ignored(tmp_path: Path) -> None:
stalled = _base_scan_stalled_in_its_checker(tmp_path, _SCAN_BASE_WITH_SIGHUP_IGNORED)
try:
stalled.process.send_signal(signal.SIGHUP)
time.sleep(1)
assert stalled.process.poll() is None, "a hangup the parent ignored killed the scan"
stalled.release.touch()
assert stalled.process.wait(timeout=30) == 0
finally:
_reap(stalled.process)
assert _registered_worktrees(stalled.repo) == 1
assert list(stalled.temp_dir.iterdir()) == []