From a734afca322959557694734e179b095f2e6f1039 Mon Sep 17 00:00:00 2001 From: yuneng-jiang Date: Sat, 22 Aug 2026 22:54:30 -0700 Subject: [PATCH] feat(ci): gate patching of SDK internals in tests as TQ008 (#37787) * feat(ci): gate patching of SDK internals in tests as TQ008 TQ002 catches the narrowest symptom of the suite's dominant mocking idiom, patch X then assert only that X was called. The idiom itself is wider: tests reach for litellm's own functions instead of faking the wire, so they pin how the code is wired rather than what it does, and a test that patches internals but makes weak real assertions trips nothing today. TQ008 counts patch targets rooted at `litellm`, both the dotted string form and the attribute chain handed to patch.object, and ratchets like every other rule. Mocking anything outside the SDK is untouched: respx, httpx transports and third-party clients do not trip it, which is the point, since those are the patterns this is meant to move the suite toward. Seeded at 9,643, in line with the ~9.4k patch sites an independent grep found in the mirror. The burn-down horizon is long; the value here is stopping the flow rather than clearing the stock. Five existing rule tests patched `litellm.completion` incidentally and now report TQ008 alongside what they were pinning. Their expected values are updated to the accurate pair rather than loosened, so they keep failing on a regression in either rule. * test: add TQ008 to the shipped-budget rule canary * fix(ci): resolve imported SDK names in TQ008 patch.object(handler.OpenAIChatCompletion, ...) after a from-import reaches the same internal as the dotted string form, but the rule only saw the bare local name and let it through. Import bindings are now resolved to the path they stand for, so the aliased, renamed and from-imported forms all read alike and the reported target is the real one. That is 1,496 patches the ratchet could not see, so the TQ008 limit moves from 9,643 to 11,139. Third-party names and locals with no SDK import behind them stay unflagged. --- scripts/check_test_quality.py | 66 +++++++++ test-quality-budget.json | 3 + tests/test_litellm/test_check_test_quality.py | 137 +++++++++++++++++- tests/test_litellm/test_test_quality_gate.py | 2 +- 4 files changed, 202 insertions(+), 6 deletions(-) diff --git a/scripts/check_test_quality.py b/scripts/check_test_quality.py index 6964aed56e4..41342acd23a 100644 --- a/scripts/check_test_quality.py +++ b/scripts/check_test_quality.py @@ -48,6 +48,10 @@ TQ006 A `pytest.skip` reached only when a credential-shaped environment variab deliberate branch. The gate follows one local or module-level binding, which is the `key = os.getenv(...)` then `if not key: pytest.skip(...)` shape most of these use. +TQ008 A `patch(...)` whose target is a `litellm.` internal. Patching the SDK's own + functions pins the test to the current wiring instead of the behaviour, and it + is the idiom the suite reaches for instead of faking the HTTP boundary. Mocking + a third-party client, a transport, or anything outside `litellm.` is untouched. TQ007 A module global that a conftest saves before every test and restores after it. The save/restore list is a hand-maintained inventory of the leaks the suite already knows about, so it is allowed to shrink and never to grow: a new entry @@ -469,6 +473,67 @@ def iter_global_mutation_violations(path: Path, tree: ast.Module) -> Iterator[Vi ) +def _is_sdk_internal(dotted: str) -> bool: + return dotted == SDK_MODULE or dotted.startswith(f"{SDK_MODULE}.") + + +def _sdk_import_bindings(tree: ast.Module) -> Iterator[tuple[str, str]]: + """(local name, dotted path) for every import that binds something under `litellm`.""" + for node in ast.walk(tree): + if isinstance(node, ast.Import): + yield from ( + (alias.asname, alias.name) if alias.asname else (root, root) + for alias in node.names + if _is_sdk_internal(alias.name) + for root in (alias.name.partition(".")[0],) + ) + elif isinstance(node, ast.ImportFrom) and node.module and _is_sdk_internal(node.module): + yield from ((alias.asname or alias.name, f"{node.module}.{alias.name}") for alias in node.names) + + +def _sdk_aliases(tree: ast.Module) -> Mapping[str, str]: + """Local names bound to something under `litellm`, mapped to the path they stand for. + + `from litellm.llms.openai.chat import handler` then `patch.object(handler.X, ...)` + reaches the same internal as the dotted string form and has to read the same way. + """ + return MappingProxyType({name: dotted for name, dotted in _sdk_import_bindings(tree)}) + + +def _resolved(dotted: str, aliases: Mapping[str, str]) -> str: + root, _, rest = dotted.partition(".") + base: Final = aliases.get(root, root) + return f"{base}.{rest}" if rest else base + + +def _patch_targets(call: ast.Call, aliases: Mapping[str, str]) -> Iterator[str]: + """What a patch installer is replacing: the dotted string it names, or the + attribute chain handed to `patch.object` / `patch.dict`, resolved through the + module's imports so a locally bound SDK object reads as its full path.""" + for first in call.args[:1]: + if isinstance(first, ast.Constant) and isinstance(first.value, str): + yield first.value + elif dotted := _dotted_name(first): + yield _resolved(dotted, aliases) + + +def iter_internal_patch_violations(path: Path, tree: ast.Module) -> Iterator[Violation]: + aliases: Final = _sdk_aliases(tree) + for node in ast.walk(tree): + if not (isinstance(node, ast.Call) and _is_patch_installer(_dotted_name(node.func))): + continue + for target in _patch_targets(node, aliases): + if _is_sdk_internal(target): + yield Violation( + path, + node.lineno, + "TQ008", + f"patches `{target}`, an SDK internal, so the test is pinned to how the code is " + "wired rather than what it does; fake the HTTP boundary (respx / MockTransport) " + f"or inject the collaborator (suppress: `# {SUPPRESSION_TOKEN}: `)", + ) + + def _environ_keys(node: ast.AST) -> Iterator[str]: for inner in ast.walk(node): if isinstance(inner, ast.Call) and _dotted_name(inner.func) in ENVIRON_READERS: @@ -680,6 +745,7 @@ def check_file(path: Path) -> tuple[Violation, ...]: *iter_global_mutation_violations(path, tree), *iter_credential_skip_violations(path, tree), *iter_conftest_inventory_violations(path, tree), + *iter_internal_patch_violations(path, tree), ) if violation.line not in skip ) diff --git a/test-quality-budget.json b/test-quality-budget.json index 0dea4e8fe93..4a7bc7edff2 100644 --- a/test-quality-budget.json +++ b/test-quality-budget.json @@ -19,5 +19,8 @@ }, "TQ007": { "limit": 117 + }, + "TQ008": { + "limit": 11139 } } diff --git a/tests/test_litellm/test_check_test_quality.py b/tests/test_litellm/test_check_test_quality.py index 7d59e5a5dba..a75b1e43fb7 100644 --- a/tests/test_litellm/test_check_test_quality.py +++ b/tests/test_litellm/test_check_test_quality.py @@ -187,7 +187,7 @@ def test_mock_echo_is_flagged(tmp_path): " run()\n" " mock_completion.assert_called_once()\n" ) - assert _codes(tmp_path, source) == ["TQ002"] + assert _codes(tmp_path, source) == ["TQ002", "TQ008"] def test_call_args_inspection_is_mock_echo(tmp_path): @@ -200,7 +200,7 @@ def test_call_args_inspection_is_mock_echo(tmp_path): " run()\n" " assert mock_completion.call_args[1]['model'] == 'gpt-4o'\n" ) - assert _codes(tmp_path, source) == ["TQ002"] + assert _codes(tmp_path, source) == ["TQ002", "TQ008"] def test_patch_decorator_counts_as_installing_a_patch(tmp_path): @@ -213,7 +213,7 @@ def test_patch_decorator_counts_as_installing_a_patch(tmp_path): " run()\n" " mock_completion.assert_called_once()\n" ) - assert _codes(tmp_path, source) == ["TQ002"] + assert _codes(tmp_path, source) == ["TQ002", "TQ008"] def test_patching_but_asserting_the_output_is_not_mock_echo(tmp_path): @@ -227,7 +227,7 @@ def test_patching_but_asserting_the_output_is_not_mock_echo(tmp_path): " mock_completion.assert_called_once()\n" " assert result.choices[0].message.content == 'pong'\n" ) - assert _codes(tmp_path, source) == [] + assert _codes(tmp_path, source) == ["TQ008"] def test_asserting_without_patching_is_not_mock_echo(tmp_path): @@ -244,7 +244,7 @@ def test_a_test_with_no_assertions_is_tq001_not_tq002(tmp_path): " with patch('litellm.completion'):\n" " run()\n" ) - assert _codes(tmp_path, source) == ["TQ001"] + assert _codes(tmp_path, source) == ["TQ001", "TQ008"] def test_sys_path_insert_is_flagged(tmp_path): @@ -554,6 +554,133 @@ def test_a_loop_storing_under_a_key_that_is_not_the_loop_variable_is_not_an_inve assert [v.code for v in checker.check_file(_written(tmp_path, source))] == [] +def test_patching_an_sdk_function_by_string_is_flagged(tmp_path): + source = 'from unittest.mock import patch\n\n\n@patch("litellm.completion")\ndef test_x(m):\n assert m\n' + assert "TQ008" in _codes(tmp_path, source) + + +def test_patching_a_deep_sdk_path_is_flagged(tmp_path): + source = ( + "from unittest.mock import patch\n\n\n" + "def test_x():\n" + ' with patch("litellm.llms.openai.chat.handler.OpenAIChatCompletion.completion"):\n' + " assert True\n" + ) + assert "TQ008" in _codes(tmp_path, source) + + +def test_patch_object_rooted_at_the_sdk_is_flagged(tmp_path): + source = ( + "import litellm\nfrom unittest.mock import patch\n\n\n" + "def test_x():\n" + ' with patch.object(litellm, "api_key", "x"):\n' + " assert True\n" + ) + assert "TQ008" in _codes(tmp_path, source) + + +def test_patch_object_on_a_from_imported_sdk_module_is_flagged(tmp_path): + source = ( + "from litellm.llms.openai.chat import handler\nfrom unittest.mock import patch\n\n\n" + "def test_x():\n" + ' with patch.object(handler.OpenAIChatCompletion, "completion"):\n' + " assert True\n" + ) + assert "TQ008" in _codes(tmp_path, source) + + +def test_patch_object_on_an_aliased_sdk_module_is_flagged(tmp_path): + source = ( + "import litellm.llms.openai.chat.handler as oai\nfrom unittest.mock import patch\n\n\n" + "def test_x():\n" + ' with patch.object(oai.OpenAIChatCompletion, "completion"):\n' + " assert True\n" + ) + assert "TQ008" in _codes(tmp_path, source) + + +def test_patch_object_on_a_renamed_sdk_symbol_is_flagged(tmp_path): + source = ( + "from litellm.utils import get_llm_provider as glp\nfrom unittest.mock import patch\n\n\n" + "def test_x():\n" + ' with patch.object(glp, "__wrapped__"):\n' + " assert True\n" + ) + assert "TQ008" in _codes(tmp_path, source) + + +def test_the_reported_target_is_the_resolved_sdk_path(tmp_path): + source = ( + "from litellm.llms.openai.chat import handler\nfrom unittest.mock import patch\n\n\n" + "def test_x():\n" + ' with patch.object(handler.OpenAIChatCompletion, "completion"):\n' + " assert True\n" + ) + reported = [v.message for v in checker.check_file(_written(tmp_path, source)) if v.code == "TQ008"] + assert reported + assert "litellm.llms.openai.chat.handler.OpenAIChatCompletion" in reported[0] + + +def test_patch_object_on_a_from_imported_third_party_is_not_flagged(tmp_path): + source = ( + "from openai import OpenAI\nfrom unittest.mock import patch\n\n\n" + "def test_x():\n" + ' with patch.object(OpenAI, "chat"):\n' + " assert True\n" + ) + assert "TQ008" not in _codes(tmp_path, source) + + +def test_a_local_name_with_no_sdk_import_behind_it_is_not_flagged(tmp_path): + source = ( + "from unittest.mock import patch\n\n\n" + "def test_x(handler):\n" + ' with patch.object(handler, "completion"):\n' + " assert True\n" + ) + assert "TQ008" not in _codes(tmp_path, source) + + +def test_mocking_a_third_party_client_is_not_flagged(tmp_path): + source = ( + "from unittest.mock import patch\n\n\n" + "def test_x():\n" + ' with patch("openai.OpenAI.chat"):\n' + " assert True\n" + ) + assert "TQ008" not in _codes(tmp_path, source) + + +def test_mocking_the_http_transport_is_not_flagged(tmp_path): + source = ( + "from unittest.mock import patch\n\n\n" + "def test_x():\n" + ' with patch("httpx.AsyncClient.send"):\n' + " assert True\n" + ) + assert "TQ008" not in _codes(tmp_path, source) + + +def test_a_name_merely_starting_with_litellm_is_not_the_sdk(tmp_path): + source = ( + "from unittest.mock import patch\n\n\n" + "def test_x():\n" + ' with patch("litellm_enterprise.thing.go"):\n' + " assert True\n" + ) + assert "TQ008" not in _codes(tmp_path, source) + + +def test_an_sdk_patch_can_be_suppressed(tmp_path): + source = ( + "from unittest.mock import patch\n\n\n" + "def test_x():\n" + ' with patch("litellm.completion"): # test-quality-ok: pinning the router seam\n' + " assert True\n" + ) + assert "TQ008" not in _codes(tmp_path, source) + + _FANS_OUT = checker._worker_count(checker.PARALLEL_MIN_PATHS) > 1 _SERIAL_ONLY = "one usable core, so scan_paths stays serial and there is no fan-out to compare" diff --git a/tests/test_litellm/test_test_quality_gate.py b/tests/test_litellm/test_test_quality_gate.py index 3bf4b89ac4e..8cce6bc735a 100644 --- a/tests/test_litellm/test_test_quality_gate.py +++ b/tests/test_litellm/test_test_quality_gate.py @@ -144,5 +144,5 @@ def test_the_shipped_budget_covers_every_rule_the_checker_can_emit(): import json budget = json.loads((_REPO_ROOT / "test-quality-budget.json").read_text()) - assert set(budget) == {"TQ001", "TQ002", "TQ003", "TQ004", "TQ005", "TQ006", "TQ007"} + assert set(budget) == {"TQ001", "TQ002", "TQ003", "TQ004", "TQ005", "TQ006", "TQ007", "TQ008"} assert all(spec["limit"] >= 0 for spec in budget.values())