diff --git a/.github/workflows/test-linting.yml b/.github/workflows/test-linting.yml index 6acb4e93899..af2759cf642 100644 --- a/.github/workflows/test-linting.yml +++ b/.github/workflows/test-linting.yml @@ -132,7 +132,7 @@ jobs: run: | uv run --no-sync python scripts/type_discipline_gate.py --base "$GATE_BASE_SHA" - - name: Check test-quality budget (zero-assert / mock-echo tests, sys.path.insert, raw env writes, litellm global mutation, delta vs base) + - name: Check test-quality budget (zero-assert / mock-echo tests, sys.path.insert, raw env writes, litellm global mutation, credential-gated skips, delta vs base) if: steps.changes.outputs.decision != 'skip' run: | uv run --no-sync python scripts/test_quality_gate.py --base "$GATE_BASE_SHA" diff --git a/Makefile b/Makefile index c80f147bf49..b265ae5a009 100644 --- a/Makefile +++ b/Makefile @@ -203,7 +203,8 @@ lint-type-discipline: $(LINT_DEP_INSTALL) $(LINT_DEP_BASE) $(UV_RUN) python scripts/type_discipline_gate.py --base origin/litellm_internal_staging # Test-quality budget (zero-assert / mock-echo tests, sys.path.insert, raw env writes, -# litellm module-global mutation), counted across tests/ the same delta-vs-base way. +# litellm module-global mutation, credential-gated skips), counted across tests/ the +# same delta-vs-base way. lint-test-quality: $(LINT_DEP_INSTALL) $(LINT_DEP_BASE) $(UV_RUN) python scripts/test_quality_gate.py --base origin/litellm_internal_staging diff --git a/scripts/check_test_quality.py b/scripts/check_test_quality.py index dd1e6b97c59..5b0b03c60fb 100644 --- a/scripts/check_test_quality.py +++ b/scripts/check_test_quality.py @@ -38,6 +38,16 @@ TQ005 `litellm. = ...` module-global mutation. The SDK's module globals process-wide, so this is the same leak as TQ004 one level up, and it is what the 491-line save/restore conftest exists to paper over. Inject the dependency or use a fixture that restores it. +TQ006 A `pytest.skip` reached only when a credential-shaped environment variable is + absent. Absence is what the condition has to say: `not key`, `key is None`, + `"KEY" not in os.environ`. A skip taken when the credential is present is + somebody's deliberate branch and is left alone. On a runner that does not hold that credential the guard fires every + time, so the test reports green having executed nothing and is indistinguishable + from coverage that exists. Fake the provider at the HTTP boundary, or fail + loudly, so a missing credential shows up as a missing credential. The gate is + followed through one local or module-level binding, which is the + `key = os.getenv(...)` then `if not key: pytest.skip(...)` shape most of these + use. Every rule is suppressible with `# test-quality-ok: ` on the reported line, following the repo's `*-ok: ` convention. A suppression without a @@ -110,6 +120,13 @@ MOCK_ASSERTION_PREFIX: Final = "assert_" PATCH_MEMBERS: Final = frozenset(("object", "dict", "multiple")) +ENVIRON_READERS: Final = frozenset(("os.environ.get", "environ.get", "os.getenv", "getenv")) +ENVIRON_MAPPINGS: Final = frozenset(("os.environ", "environ")) +SKIP_CALLS: Final = frozenset(("pytest.skip", "skip")) +CREDENTIAL_NAME_RE: Final = re.compile( + r"(?:API_KEY|_KEY|TOKEN|SECRET|PASSWORD|CREDENTIAL|DATABASE_URL|ACCESS_KEY_ID)$" +) + FunctionNode = ast.FunctionDef | ast.AsyncFunctionDef @@ -439,6 +456,97 @@ def iter_global_mutation_violations(path: Path, tree: ast.Module) -> Iterator[Vi ) +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: + yield from ( + argument.value + for argument in inner.args[:1] + if isinstance(argument, ast.Constant) and isinstance(argument.value, str) + ) + elif isinstance(inner, ast.Subscript) and _dotted_name(inner.value) in ENVIRON_MAPPINGS: + if isinstance(inner.slice, ast.Constant) and isinstance(inner.slice.value, str): + yield inner.slice.value + elif isinstance(inner, ast.Compare) and any(isinstance(op, (ast.In, ast.NotIn)) for op in inner.ops): + if any(_dotted_name(right) in ENVIRON_MAPPINGS for right in inner.comparators): + if isinstance(inner.left, ast.Constant) and isinstance(inner.left.value, str): + yield inner.left.value + + +def _credential_bindings(tree: ast.Module) -> Mapping[str, str]: + return MappingProxyType({ + target.id: key + for node in ast.walk(tree) + if isinstance(node, ast.Assign) + for key in tuple(k for k in _environ_keys(node.value) if CREDENTIAL_NAME_RE.search(k))[:1] + for target in node.targets + if isinstance(target, ast.Name) + }) + + +def _absence_operands(test: ast.expr) -> Iterator[ast.expr]: + """The subtrees of an `if` condition that are true when what they name is missing.""" + for node in ast.walk(test): + if isinstance(node, ast.UnaryOp) and isinstance(node.op, ast.Not): + yield node.operand + elif isinstance(node, ast.Compare) and _is_absent_from_environ(node): + yield node + elif isinstance(node, ast.Compare) and _is_compared_to_none(node): + yield node.left + + +def _is_absent_from_environ(node: ast.Compare) -> bool: + return any(isinstance(op, ast.NotIn) for op in node.ops) and any( + _dotted_name(right) in ENVIRON_MAPPINGS for right in node.comparators + ) + + +def _is_compared_to_none(node: ast.Compare) -> bool: + return all(isinstance(op, (ast.Is, ast.Eq)) for op in node.ops) and any( + isinstance(right, ast.Constant) and right.value is None for right in node.comparators + ) + + +def _gating_credential(test: ast.expr, bindings: Mapping[str, str]) -> str | None: + return next( + ( + credential + for operand in _absence_operands(test) + for credential in _named_credentials(operand, bindings) + ), + None, + ) + + +def _named_credentials(node: ast.expr, bindings: Mapping[str, str]) -> Iterator[str]: + yield from (key for key in _environ_keys(node) if CREDENTIAL_NAME_RE.search(key)) + yield from ( + bindings[inner.id] for inner in ast.walk(node) if isinstance(inner, ast.Name) and inner.id in bindings + ) + + +def iter_credential_skip_violations(path: Path, tree: ast.Module) -> Iterator[Violation]: + bindings: Final = _credential_bindings(tree) + for node in ast.walk(tree): + if not isinstance(node, ast.If): + continue + credential: Final = _gating_credential(node.test, bindings) + if credential is None: + continue + for statement in node.body: + for inner in ast.walk(statement): + if isinstance(inner, ast.Call) and _dotted_name(inner.func) in SKIP_CALLS: + yield Violation( + path, + inner.lineno, + "TQ006", + f"this test skips itself when {credential} is absent, so a run without " + "that credential reports green having executed nothing; fake the provider at " + "the HTTP boundary, or fail loudly so the missing credential is visible " + f"(suppress: `# {SUPPRESSION_TOKEN}: `)", + ) + + def check_file(path: Path) -> tuple[Violation, ...]: try: source: Final = path.read_text(encoding="utf-8") @@ -458,6 +566,7 @@ def check_file(path: Path) -> tuple[Violation, ...]: *iter_sys_path_violations(path, tree), *iter_environ_violations(path, tree), *iter_global_mutation_violations(path, tree), + *iter_credential_skip_violations(path, tree), ) if violation.line not in skip ) diff --git a/test-quality-budget.json b/test-quality-budget.json index 189e2609ce2..2a5945fe36c 100644 --- a/test-quality-budget.json +++ b/test-quality-budget.json @@ -13,5 +13,8 @@ }, "TQ005": { "limit": 2835 + }, + "TQ006": { + "limit": 34 } } diff --git a/tests/test_litellm/test_check_test_quality.py b/tests/test_litellm/test_check_test_quality.py index f9e907a4fde..7f8ce4c36d0 100644 --- a/tests/test_litellm/test_check_test_quality.py +++ b/tests/test_litellm/test_check_test_quality.py @@ -305,3 +305,95 @@ def test_unparseable_source_degrades_to_tq000(tmp_path): def test_every_violation_renders_as_path_line_code_message(): rendered = checker.Violation(Path("tests/test_x.py"), 7, "TQ001", "nothing asserted").render() assert rendered == "tests/test_x.py:7: TQ001 nothing asserted" + + +_DIRECT_GATE = """import os +import pytest + + +def test_live_call(): + if not os.getenv("ACME_API_KEY"): + pytest.skip("no key") + assert call() == "ok" +""" + +_BOUND_GATE = """import os +import pytest + + +def test_live_call(): + api_key = os.getenv("ACME_API_KEY") + if not api_key: + pytest.skip("no key") + assert call() == "ok" +""" + +_MEMBERSHIP_GATE = """import os +import pytest + + +def test_live_call(): + if "ACME_API_KEY" not in os.environ: + pytest.skip("no key") + assert call() == "ok" +""" + + +def test_a_skip_gated_on_a_missing_credential_is_flagged(tmp_path): + assert _codes(tmp_path, _DIRECT_GATE) == ["TQ006"] + + +def test_the_gate_is_followed_through_the_local_it_was_bound_to(tmp_path): + assert _codes(tmp_path, _BOUND_GATE) == ["TQ006"] + + +def test_a_membership_test_against_os_environ_gates_just_the_same(tmp_path): + assert _codes(tmp_path, _MEMBERSHIP_GATE) == ["TQ006"] + + +def test_a_skip_gated_on_something_that_is_not_a_credential_is_left_alone(tmp_path): + source = _DIRECT_GATE.replace("ACME_API_KEY", "CI_RUNNER_OS") + assert _codes(tmp_path, source) == [] + + +def test_reading_a_credential_without_skipping_on_it_is_left_alone(tmp_path): + source = 'import os\n\n\ndef test_live_call():\n assert call(os.getenv("ACME_API_KEY")) == "ok"\n' + assert _codes(tmp_path, source) == [] + + +def test_a_skip_outside_the_credential_branch_is_left_alone(tmp_path): + source = ( + "import os\n" + "import pytest\n" + "\n" + "\n" + "def test_live_call():\n" + ' if not os.getenv("ACME_API_KEY"):\n' + " configure()\n" + ' pytest.skip("unconditional")\n' + ' assert call() == "ok"\n' + ) + assert _codes(tmp_path, source) == [] + + +def test_the_credential_skip_is_suppressible_like_every_other_rule(tmp_path): + source = _DIRECT_GATE.replace( + 'pytest.skip("no key")', + 'pytest.skip("no key") # test-quality-ok: the live suite owns this one', + ) + assert _codes(tmp_path, source) == [] + + +def test_a_skip_taken_when_the_credential_is_present_is_left_alone(tmp_path): + source = _DIRECT_GATE.replace('if not os.getenv("ACME_API_KEY")', 'if os.getenv("ACME_API_KEY")') + assert _codes(tmp_path, source) == [] + + +def test_a_none_comparison_reads_as_absence(tmp_path): + source = _BOUND_GATE.replace("if not api_key:", "if api_key is None:") + assert _codes(tmp_path, source) == ["TQ006"] + + +def test_a_membership_test_without_the_negation_is_left_alone(tmp_path): + source = _MEMBERSHIP_GATE.replace('"ACME_API_KEY" not in os.environ', '"ACME_API_KEY" in os.environ') + assert _codes(tmp_path, source) == [] diff --git a/tests/test_litellm/test_test_quality_gate.py b/tests/test_litellm/test_test_quality_gate.py index 3bfaac866d7..4caadca3d09 100644 --- a/tests/test_litellm/test_test_quality_gate.py +++ b/tests/test_litellm/test_test_quality_gate.py @@ -121,5 +121,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"} + assert set(budget) == {"TQ001", "TQ002", "TQ003", "TQ004", "TQ005", "TQ006"} assert all(spec["limit"] >= 0 for spec in budget.values())