feat(ci): ratchet tests that skip themselves when a credential is absent (#37612)

* feat(ci): ratchet tests that skip themselves when a credential is absent

* docs(ci): name the new rule where the gate's rules are listed

* fix(ci): require the condition to test for absence before TQ006 fires

.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"

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
 

scripts/check_test_quality.py

@@ -38,6 +38,16 @@ TQ005   `litellm.<attr> = ...` 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: <reason>` on the reported
 line, following the repo's `*-ok: <reason>` 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}: <reason>`)",
+                    )
+
+
 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
     )

test-quality-budget.json

@@ -13,5 +13,8 @@
   },
   "TQ005": {
     "limit": 2835
+  },
+  "TQ006": {
+    "limit": 34
   }
 }

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) == []

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