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())