From 65a4a009585973a6d328cd981866b4d8e5188c1a Mon Sep 17 00:00:00 2001 From: yuneng Date: Sun, 20 Sep 2026 08:12:39 +0000 Subject: [PATCH 1/3] fix(ci): excuse retired test-quality rules in the budget ratchet Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- scripts/budget_ratchet_check.py | 43 +++++++++++++++---- scripts/check_test_quality.py | 4 ++ .../test_litellm/test_budget_ratchet_check.py | 30 +++++++++++++ tests/test_litellm/test_check_test_quality.py | 15 +++++++ 4 files changed, 84 insertions(+), 8 deletions(-) diff --git a/scripts/budget_ratchet_check.py b/scripts/budget_ratchet_check.py index 485e118efd2..470adddfcd3 100644 --- a/scripts/budget_ratchet_check.py +++ b/scripts/budget_ratchet_check.py @@ -7,13 +7,17 @@ driven DOWN over time. This check compares every budget file against its own content at the merge-base with the target branch and fails (exits 1, red) if: * a rule's `limit` went up, - * a rule was dropped from a budget (its ceiling effectively became infinite), or + * a rule was dropped from a budget (its ceiling effectively became infinite) while + its checker still emits it, or * an entire budget file was deleted. New rules and lowered/equal limits are fine. So is a rule that graduated: once a paired config (ruff.toml for the ruff-strict budget) selects the rule outright it hard-fails at the first violation, which is stricter than any ceiling the budget could hold, so dropping its entry tightens the guard rather than removing it. +Likewise a retired rule: once the paired checker (check_test_quality.py for the +test-quality budget) no longer emits a code, its entry has no ceiling left to +loosen. This is deliberately NOT a gating check. It should turn the run red so that a loosening is impossible to miss in review, but it must stay OUT of the @@ -29,11 +33,12 @@ Usage: from __future__ import annotations import argparse +import importlib.util import json import subprocess import sys from pathlib import Path -from types import MappingProxyType +from types import MappingProxyType, ModuleType from typing import Final, NamedTuple if sys.version_info >= (3, 11): @@ -49,6 +54,7 @@ DEFAULT_BUDGETS: tuple[str, ...] = ( "test-quality-budget.json", ) GRADUATION_CONFIGS = MappingProxyType({"ruff-strict-budget.json": "ruff.toml"}) +RETIREMENT_SOURCES = MappingProxyType({"test-quality-budget.json": "check_test_quality"}) class Regression(NamedTuple): @@ -139,20 +145,40 @@ def graduated_selectors(rel: str) -> tuple[str, ...]: ) +def _load_script(name: str) -> ModuleType: + if name in sys.modules: + return sys.modules[name] + spec: Final = importlib.util.spec_from_file_location(name, REPO_ROOT / "scripts" / f"{name}.py") + assert spec is not None and spec.loader is not None + module: Final = importlib.util.module_from_spec(spec) + sys.modules[name] = module + spec.loader.exec_module(module) + return module + + +def retired_rules(rel: str, base: dict) -> frozenset[str]: + """Rules in the base budget that the paired checker can no longer emit, so there is no ceiling to loosen.""" + source: Final = RETIREMENT_SOURCES.get(rel) + if source is None: + return frozenset() + return frozenset(_limits(base)) - _load_script(source).RULE_CODES + + def _regression_detail( rule: str, base_limits: dict[str, int], head_limits: dict[str, int], graduated: tuple[str, ...], + retired: frozenset[str] = frozenset(), ) -> str | None: - """Why `rule` regressed vs base, or None when it held flat, fell, or graduated. + """Why `rule` regressed vs base, or None when it held flat, fell, or left the budget legitimately. - A dropped rule is terminal unless it graduated; otherwise the only loosening - left is a raised limit. + A dropped rule is terminal unless it graduated or retired; otherwise the only + loosening left is a raised limit. """ base_limit = base_limits[rule] if rule not in head_limits: - if graduated and rule.startswith(graduated): + if rule in retired or (graduated and rule.startswith(graduated)): return None return f"rule dropped (limit {base_limit} -> removed)" if head_limits[rule] > base_limit: @@ -165,6 +191,7 @@ def regressions_for( base: dict | None, head: dict | None, graduated: tuple[str, ...] = (), + retired: frozenset[str] = frozenset(), ) -> list[Regression]: if base is None: return [] # new budget file: nothing to ratchet against yet @@ -175,7 +202,7 @@ def regressions_for( return [ Regression(rel, rule, detail) for rule in sorted(base_limits) - if (detail := _regression_detail(rule, base_limits, head_limits, graduated)) is not None + if (detail := _regression_detail(rule, base_limits, head_limits, graduated, retired)) is not None ] @@ -209,7 +236,7 @@ def main() -> int: print(f"skip {rel}: new file (no base at {base_ref} to ratchet against)") continue checked.append(rel) - regressions.extend(regressions_for(rel, base, head, graduated_selectors(rel))) + regressions.extend(regressions_for(rel, base, head, graduated_selectors(rel), retired_rules(rel, base))) if regressions: print( diff --git a/scripts/check_test_quality.py b/scripts/check_test_quality.py index dddc9d61982..9f93023cd53 100644 --- a/scripts/check_test_quality.py +++ b/scripts/check_test_quality.py @@ -146,6 +146,10 @@ SDK_MODULE: Final = "litellm" SUBPROCESS_SPAWNS: Final = frozenset(("run", "Popen", "check_output", "check_call", "call")) INTERPRETER_ISOLATION_FLAGS: Final = frozenset(("-I", "-P")) +RULE_CODES: Final = frozenset(( + "TQ000", "TQ001", "TQ002", "TQ003", "TQ004", "TQ005", "TQ006", "TQ007", "TQ009", +)) + CREDENTIAL_NAME_RE: Final = re.compile( r"(?:API_KEY|_KEY|TOKEN|SECRET|PASSWORD|CREDENTIAL|DATABASE_URL|ACCESS_KEY_ID)$" ) diff --git a/tests/test_litellm/test_budget_ratchet_check.py b/tests/test_litellm/test_budget_ratchet_check.py index 22d05f4d00d..d6809b22161 100644 --- a/tests/test_litellm/test_budget_ratchet_check.py +++ b/tests/test_litellm/test_budget_ratchet_check.py @@ -92,6 +92,36 @@ def test_graduation_never_excuses_a_raised_limit(): assert "0 -> 7" in regs[0].detail +def test_dropped_rule_the_checker_retired_is_clean(): + base = {"TQ008": _spec_of(10993)} + assert ratchet.regressions_for("b.json", base, {}, retired=frozenset({"TQ008"})) == [] + + +def test_dropped_rule_the_checker_still_emits_is_a_regression(): + base = {"TQ001": _spec_of(5), "TQ008": _spec_of(10993)} + regs = ratchet.regressions_for("b.json", base, {}, retired=frozenset({"TQ008"})) + assert [r.rule for r in regs] == ["TQ001"] + assert "dropped" in regs[0].detail + + +def test_retirement_never_excuses_a_raised_limit(): + base = {"TQ008": _spec_of(0)} + regs = ratchet.regressions_for("b.json", base, {"TQ008": _spec_of(7)}, retired=frozenset({"TQ008"})) + assert [r.rule for r in regs] == ["TQ008"] + assert "0 -> 7" in regs[0].detail + + +def test_retired_rules_come_from_the_paired_checker(): + base = {"TQ001": _spec_of(5), "TQ008": _spec_of(10993)} + assert ratchet.retired_rules("test-quality-budget.json", base) == frozenset({"TQ008"}) + + +def test_budgets_without_a_paired_checker_never_retire(): + base = {"TQ008": _spec_of(1)} + for rel in ("ruff-strict-budget.json", "type-discipline-budget.json", "basedpyright-code-budget.json"): + assert ratchet.retired_rules(rel, base) == frozenset() + + def test_graduated_selectors_come_from_the_paired_ruff_config(): selectors = ratchet.graduated_selectors("ruff-strict-budget.json") assert "UP006" in selectors diff --git a/tests/test_litellm/test_check_test_quality.py b/tests/test_litellm/test_check_test_quality.py index 05c25fb19fb..5a5c53fc31c 100644 --- a/tests/test_litellm/test_check_test_quality.py +++ b/tests/test_litellm/test_check_test_quality.py @@ -7,8 +7,10 @@ produced against tests/e2e, where the assertions live in a shared helper rather in the test body. """ +import ast import importlib.util import os +import re import subprocess import sys from pathlib import Path @@ -612,6 +614,19 @@ def test_a_fanned_out_run_reports_each_generated_file_exactly_once(tmp_path): assert all(" TQ001 " in line for line in reported) +def test_rule_codes_match_every_code_the_checker_emits(): + source = _MODULE_PATH.read_text(encoding="utf-8") + tree = ast.parse(source) + definition = next( + node + for node in tree.body + if isinstance(node, ast.AnnAssign) and isinstance(node.target, ast.Name) and node.target.id == "RULE_CODES" + ) + lines = source.splitlines() + outside = "\n".join(lines[: definition.lineno - 1] + lines[definition.end_lineno :]) + assert frozenset(re.findall(r'"(TQ\d{3})"', outside)) == checker.RULE_CODES + + def test_sys_executable_child_without_isolation_flag_is_flagged(tmp_path): source = 'import subprocess, sys\nsubprocess.run([sys.executable, "-c", "pass"])\n' assert _codes(tmp_path, source) == ["TQ009"] From 9850cd14f7e72bbe99f0b025cbb722d9a18cb523 Mon Sep 17 00:00:00 2001 From: yuneng Date: Sun, 20 Sep 2026 08:27:02 +0000 Subject: [PATCH 2/3] test(ci): prove RULE_CODES by running every checker rule Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- scripts/budget_ratchet_check.py | 2 +- tests/test_litellm/test_check_test_quality.py | 49 ++++++++++++++----- 2 files changed, 38 insertions(+), 13 deletions(-) diff --git a/scripts/budget_ratchet_check.py b/scripts/budget_ratchet_check.py index 470adddfcd3..3ca5e9f3e9d 100644 --- a/scripts/budget_ratchet_check.py +++ b/scripts/budget_ratchet_check.py @@ -156,7 +156,7 @@ def _load_script(name: str) -> ModuleType: return module -def retired_rules(rel: str, base: dict) -> frozenset[str]: +def retired_rules(rel: str, base: dict[str, object]) -> frozenset[str]: """Rules in the base budget that the paired checker can no longer emit, so there is no ceiling to loosen.""" source: Final = RETIREMENT_SOURCES.get(rel) if source is None: diff --git a/tests/test_litellm/test_check_test_quality.py b/tests/test_litellm/test_check_test_quality.py index 5a5c53fc31c..2a0e32d4de7 100644 --- a/tests/test_litellm/test_check_test_quality.py +++ b/tests/test_litellm/test_check_test_quality.py @@ -7,13 +7,13 @@ produced against tests/e2e, where the assertions live in a shared helper rather in the test body. """ -import ast import importlib.util import os -import re import subprocess import sys from pathlib import Path +from types import MappingProxyType +from typing import Final import pytest @@ -614,17 +614,42 @@ def test_a_fanned_out_run_reports_each_generated_file_exactly_once(tmp_path): assert all(" TQ001 " in line for line in reported) -def test_rule_codes_match_every_code_the_checker_emits(): - source = _MODULE_PATH.read_text(encoding="utf-8") - tree = ast.parse(source) - definition = next( - node - for node in tree.body - if isinstance(node, ast.AnnAssign) and isinstance(node.target, ast.Name) and node.target.id == "RULE_CODES" +_VIOLATING_SNIPPETS: Final = MappingProxyType( + { + "TQ000": ("test_snippet.py", "def test_broken(:\n pass\n"), + "TQ001": ("test_snippet.py", "def test_nothing():\n compute()\n"), + "TQ002": ( + "test_snippet.py", + "from unittest.mock import patch\n" + "\n" + "\n" + "def test_echo():\n" + " with patch('litellm.completion') as mock_completion:\n" + " run()\n" + " mock_completion.assert_called_once()\n", + ), + "TQ003": ("test_snippet.py", "import sys\n\nsys.path.insert(0, '..')\n"), + "TQ004": ("test_snippet.py", "import os\n\nos.environ['KEY'] = 'v'\n"), + "TQ005": ("test_snippet.py", "import litellm\n\nlitellm.drop_params = True\n"), + "TQ006": ("test_snippet.py", _DIRECT_GATE), + "TQ007": ("conftest.py", _SNAPSHOT_CONFTEST), + "TQ009": ( + "test_snippet.py", + 'import subprocess, sys\nsubprocess.run([sys.executable, "-c", "pass"])\n', + ), + } +) + + +def test_rule_codes_match_every_code_the_checker_emits(tmp_path): + emitted = frozenset( + v.code + for name, source in _VIOLATING_SNIPPETS.values() + for v in checker.check_file(_written(tmp_path, source, name)) ) - lines = source.splitlines() - outside = "\n".join(lines[: definition.lineno - 1] + lines[definition.end_lineno :]) - assert frozenset(re.findall(r'"(TQ\d{3})"', outside)) == checker.RULE_CODES + for code, (name, source) in _VIOLATING_SNIPPETS.items(): + assert code in [v.code for v in checker.check_file(_written(tmp_path, source, name))], code + assert emitted == checker.RULE_CODES def test_sys_executable_child_without_isolation_flag_is_flagged(tmp_path): From bbfa853d844b8c54c66746a416cafeef93f15b8f Mon Sep 17 00:00:00 2001 From: yuneng Date: Sun, 20 Sep 2026 09:07:51 +0000 Subject: [PATCH 3/3] test(ci): annotate new test locals as Final Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- tests/test_litellm/test_budget_ratchet_check.py | 15 ++++++++------- tests/test_litellm/test_check_test_quality.py | 2 +- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/tests/test_litellm/test_budget_ratchet_check.py b/tests/test_litellm/test_budget_ratchet_check.py index d6809b22161..b359b8b42e5 100644 --- a/tests/test_litellm/test_budget_ratchet_check.py +++ b/tests/test_litellm/test_budget_ratchet_check.py @@ -9,6 +9,7 @@ import importlib.util import subprocess import sys from pathlib import Path +from typing import Final _MODULE_PATH = ( Path(__file__).resolve().parents[2] / "scripts" / "budget_ratchet_check.py" @@ -93,31 +94,31 @@ def test_graduation_never_excuses_a_raised_limit(): def test_dropped_rule_the_checker_retired_is_clean(): - base = {"TQ008": _spec_of(10993)} + base: Final = {"TQ008": _spec_of(10993)} assert ratchet.regressions_for("b.json", base, {}, retired=frozenset({"TQ008"})) == [] def test_dropped_rule_the_checker_still_emits_is_a_regression(): - base = {"TQ001": _spec_of(5), "TQ008": _spec_of(10993)} - regs = ratchet.regressions_for("b.json", base, {}, retired=frozenset({"TQ008"})) + base: Final = {"TQ001": _spec_of(5), "TQ008": _spec_of(10993)} + regs: Final = ratchet.regressions_for("b.json", base, {}, retired=frozenset({"TQ008"})) assert [r.rule for r in regs] == ["TQ001"] assert "dropped" in regs[0].detail def test_retirement_never_excuses_a_raised_limit(): - base = {"TQ008": _spec_of(0)} - regs = ratchet.regressions_for("b.json", base, {"TQ008": _spec_of(7)}, retired=frozenset({"TQ008"})) + base: Final = {"TQ008": _spec_of(0)} + regs: Final = ratchet.regressions_for("b.json", base, {"TQ008": _spec_of(7)}, retired=frozenset({"TQ008"})) assert [r.rule for r in regs] == ["TQ008"] assert "0 -> 7" in regs[0].detail def test_retired_rules_come_from_the_paired_checker(): - base = {"TQ001": _spec_of(5), "TQ008": _spec_of(10993)} + base: Final = {"TQ001": _spec_of(5), "TQ008": _spec_of(10993)} assert ratchet.retired_rules("test-quality-budget.json", base) == frozenset({"TQ008"}) def test_budgets_without_a_paired_checker_never_retire(): - base = {"TQ008": _spec_of(1)} + base: Final = {"TQ008": _spec_of(1)} for rel in ("ruff-strict-budget.json", "type-discipline-budget.json", "basedpyright-code-budget.json"): assert ratchet.retired_rules(rel, base) == frozenset() diff --git a/tests/test_litellm/test_check_test_quality.py b/tests/test_litellm/test_check_test_quality.py index 2a0e32d4de7..bf05775d09d 100644 --- a/tests/test_litellm/test_check_test_quality.py +++ b/tests/test_litellm/test_check_test_quality.py @@ -642,7 +642,7 @@ _VIOLATING_SNIPPETS: Final = MappingProxyType( def test_rule_codes_match_every_code_the_checker_emits(tmp_path): - emitted = frozenset( + emitted: Final = frozenset( v.code for name, source in _VIOLATING_SNIPPETS.values() for v in checker.check_file(_written(tmp_path, source, name))