diff --git a/.github/ci-coverage-allowlist.yml b/.github/ci-coverage-allowlist.yml index f9c28e8b98a..9f98d60d857 100644 --- a/.github/ci-coverage-allowlist.yml +++ b/.github/ci-coverage-allowlist.yml @@ -4,6 +4,25 @@ description: >- by a job nor listed here, so every entry below is a decision on the record. test_paths: + - reason: >- + The caching suite in tests/local_testing, which runs nowhere. Every job that globs that + directory either deselects it (local_testing_part1 and part2 carry `-k "... and not caching + and not cache"`) or keeps only another keyword (langfuse, router, assistants), and no job + names these files the way redis_caching_unit_tests names test_dual_cache.py. Measured + 2026-08-20 by collecting the directory under each job's own selector: 118 tests across + these eight files are selected by none of them. Listed so the gap is a decision rather + than an accident, and so the --slices guard has a baseline to ratchet down from. Revisit + when tests/local_testing is ported off CircleCI, where the keyless part of this suite + belongs in a real job + paths: + - tests/local_testing/test_cache_preset_key.py + - tests/local_testing/test_caching.py + - tests/local_testing/test_caching_handler.py + - tests/local_testing/test_disk_cache_unit_tests.py + - tests/local_testing/test_gcs_cache_unit_tests.py + - tests/local_testing/test_prompt_caching.py + - tests/local_testing/test_responses_stream_cache_keys.py + - tests/local_testing/test_unit_test_caching.py - reason: >- The end-to-end suite runs against a deployed proxy from its own in-cluster rig rather than from a pull request; it needs a live gateway and provider credentials no PR job holds diff --git a/.github/scripts/assert_ci_coverage.py b/.github/scripts/assert_ci_coverage.py index 651c7d34553..86a6b7d4e72 100644 --- a/.github/scripts/assert_ci_coverage.py +++ b/.github/scripts/assert_ci_coverage.py @@ -1,10 +1,13 @@ from __future__ import annotations +import ast import pathlib import re import sys +import warnings from collections.abc import Iterable, Mapping, Sequence from dataclasses import dataclass +from typing import Final import yaml @@ -117,9 +120,11 @@ def _built_dockerfile_tokens(scalars: Iterable[Scalar]) -> frozenset[str]: def _glob_to_regex(token: str, *, subtree: bool) -> re.Pattern[str]: - parts = re.split(r"(\*\*/|\*\*|\*|\?)", token) + parts = re.split(r"(\*\*/|\*\*|\*|\?|\[[^\]]*\])", token) translated = "".join( - {"**/": r"(?:.*/)?", "**": r".*", "*": r"[^/]*", "?": r"[^/]"}.get(part, re.escape(part)) for part in parts + {"**/": r"(?:.*/)?", "**": r".*", "*": r"[^/]*", "?": r"[^/]"}.get(part) + or (part if part.startswith("[") and part.endswith("]") else re.escape(part)) + for part in parts ) return re.compile(rf"{translated}(?:/.*)?$" if subtree else rf"{translated}$") @@ -187,6 +192,135 @@ def _describe(paths: tuple[str, ...]) -> str: return f"{len(paths)} test file(s) invoked by no job: {names}{suffix}" +GLOB_CALL_RE = re.compile(r'circleci tests glob "([^"]+)"') +KEYWORD_RE = re.compile(r"-k\s+\\?[\"']([^\"'\\]+)") + + +@dataclass(frozen=True, slots=True) +class Slice: + """One job's selection: the files it globs, narrowed by its `-k` expression.""" + + job: str + globs: tuple[str, ...] + named: frozenset[str] + required: tuple[str, ...] + excluded: tuple[str, ...] + understood: bool + + def claims(self, relative_path: str, inner_names: frozenset[str]) -> bool: + """Whether this job runs any test in the file. + + The question is deliberately per-file, not per-test. An excluded term is only + honoured when it appears in the path, because that is the case where it takes + the whole module with it; a term matching one function inside drops that test + and leaves the file claimed. Losing a whole file is the failure worth a gate, + and answering per-test would mean a baseline of test ids that churns on every + rename. + """ + if relative_path in self.named: + return True + if not any(_token_covers(glob, relative_path) for glob in self.globs): + return False + if not self.understood: + return True # a `-k` this parser cannot model is assumed to claim everything + if any(term.lower() in relative_path.lower() for term in self.excluded): + return False + return not self.required or any( + term.lower() in name.lower() for term in self.required for name in inner_names + ) + + +def _strings(node: object) -> Iterable[str]: + if isinstance(node, str): + yield node + elif isinstance(node, dict): + for value in node.values(): + yield from _strings(value) + elif isinstance(node, list): + for value in node: + yield from _strings(value) + + +def _keyword_terms( + expressions: Sequence[str], *, attributable: bool = True +) -> tuple[tuple[str, ...], tuple[str, ...], bool]: + """A `-k` expression as (required, excluded, understood). + + Only flat `and` chains of bare terms are modelled. Anything with `or`, parentheses + or negation of a group is left unmodelled, and its job is then treated as claiming + every file it globs, so an unparsed selector can never raise a false alarm. + + `attributable` is False when a job runs several pytest commands, since a selector + read out of the job's text cannot then be tied to the glob it belongs to, and + pairing one command's exclusion with another's glob would invent a gap. + """ + terms: Final = tuple(part.strip() for expression in expressions for part in expression.split(" and ")) + if not attributable and terms: + return (), (), False + if any(("or " in term) or ("(" in term) or (term.startswith("not ") and " " in term[4:]) for term in terms): + return (), (), False + return ( + tuple(term for term in terms if term and not term.startswith("not ")), + tuple(term[4:].strip() for term in terms if term.startswith("not ")), + True, + ) + + +def _slices() -> tuple[Slice, ...]: + if not CIRCLECI_CONFIG.exists(): + return () + jobs: Final = yaml.safe_load(CIRCLECI_CONFIG.read_text()).get("jobs", {}) + return tuple( + Slice(job=job, globs=globs, named=named, required=required, excluded=excluded, understood=understood) + for job, body in jobs.items() + for text in ("\n".join(_strings(body)),) + if "pytest" in text + for globs in (tuple(GLOB_CALL_RE.findall(text)),) + for named in (frozenset(TEST_TOKEN_RE.findall(text)) & frozenset(_test_files()),) + for required, excluded, understood in ( + _keyword_terms(tuple(KEYWORD_RE.findall(text)), attributable=len(globs) < 2), + ) + if globs or named + ) + + +def _matchable_names(relative_path: str) -> frozenset[str]: + """Every name a `-k` term can match for this file: its path, plus the names inside it. + + pytest matches a keyword against an item's own name and each of its parents', so a + positive term hits a file when it appears in the path or in a class or function name. + """ + try: + with warnings.catch_warnings(): + warnings.simplefilter("ignore") # test files carry stray escapes; their names still parse + tree: Final = ast.parse((REPO_ROOT / relative_path).read_text()) + except (OSError, SyntaxError): + return frozenset({relative_path}) + return frozenset({relative_path}) | frozenset( + node.name + for node in ast.walk(tree) + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)) + ) + + +def _deselected_everywhere(allowlist: Allowlist) -> tuple[Finding, ...]: + slices: Final = _slices() + globbed: Final = tuple( + path + for path in _test_files() + if any(_token_covers(glob, path) for slice_ in slices for glob in slice_.globs) + ) + return tuple( + Finding( + subject=path, + detail="globbed by a job, then deselected by every one of their -k expressions", + ) + for path in globbed + if not allowlist.covers_test(path) + and not any(slice_.claims(path, _matchable_names(path)) for slice_ in slices) + ) + + def _holds_tests(directory: pathlib.Path) -> bool: return any(directory.rglob("test_*.py")) @@ -289,6 +423,21 @@ def _report(title: str, findings: tuple[Finding, ...], remedy: str) -> None: _write("") +def _check_slices() -> int: + findings: Final = _deselected_everywhere(_load_allowlist()) + if findings: + _report( + "test files a -k expression removes from every job that globs them", + findings, + "Give each one a job whose -k keeps it, or list it in " + ".github/ci-coverage-allowlist.yml with the reason it may stay unrun.", + ) + return 1 + + _write(f"OK: no test file is globbed by a job and then deselected by every -k across {len(_slices())} slices.") + return 0 + + def _check_shards() -> int: findings = _unassigned_shard_children(_invoked_test_tokens(_all_scalars())) if findings: @@ -308,6 +457,8 @@ def _check_shards() -> int: def main() -> int: if "--shards" in sys.argv[1:]: return _check_shards() + if "--slices" in sys.argv[1:]: + return _check_slices() allowlist = _load_allowlist() scalars = _all_scalars() diff --git a/.github/workflows/ci-coverage.yml b/.github/workflows/ci-coverage.yml index c95921297a2..486587fc27d 100644 --- a/.github/workflows/ci-coverage.yml +++ b/.github/workflows/ci-coverage.yml @@ -40,3 +40,9 @@ jobs: run: | python -m pip install "pyyaml==6.0.3" python .github/scripts/assert_ci_coverage.py + + # The census asks whether a job names a file; this asks whether that job's -k + # then throws it back out. A file both globbed and deselected everywhere runs + # nowhere while counting as covered, which is how the caching suite went unrun. + - name: Assert no -k expression deselects a file from every job that globs it + run: python .github/scripts/assert_ci_coverage.py --slices diff --git a/tests/test_litellm/test_assert_ci_coverage.py b/tests/test_litellm/test_assert_ci_coverage.py index a7ba603e00f..59cfff52992 100644 --- a/tests/test_litellm/test_assert_ci_coverage.py +++ b/tests/test_litellm/test_assert_ci_coverage.py @@ -1,10 +1,11 @@ """Tests for .github/scripts/assert_ci_coverage.py. -Two guards share one workflow parser. The census asks whether a test file is run at +Three guards share one workflow parser. The census asks whether a test file is run at all, so an ancestor path standing in for everything below it is a valid answer. The shard guard asks whether a sharded tree, which has no catch-all bucket, names each -child outright, so that same ancestor path must NOT be an answer. The pair of -matchers that splits those two questions is what these tests pin. +child outright, so that same ancestor path must NOT be an answer. The slice guard asks +the question neither covers: whether the job that globs a file then deselects it with +`-k`, which is how a file counts as covered while running nowhere. """ import importlib.util @@ -117,3 +118,94 @@ def test_every_sharded_root_named_in_the_script_exists_on_disk(): def test_the_repo_as_it_stands_has_every_shard_child_assigned(): findings = coverage._unassigned_shard_children(coverage._invoked_test_tokens(coverage._all_scalars())) assert [f.subject for f in findings] == [] + + +# --------------------------------------------------------------------------- # +# Slice guard: a job can glob a file and its -k can then throw the file out +# --------------------------------------------------------------------------- # + + +def _slice(**overrides): + defaults = dict( + job="a_job", globs=("tests/x/**/test_*.py",), named=frozenset(), + required=(), excluded=(), understood=True, + ) + return coverage.Slice(**{**defaults, **overrides}) + + +def test_a_term_in_the_path_deselects_the_whole_file(): + # -k matches the module's path as well as the names inside it, so "not caching" + # removes every test in test_caching.py, not merely the ones named for a cache. + slice_ = _slice(excluded=("caching",)) + assert slice_.claims("tests/x/test_caching.py", frozenset({"test_get"})) is False + assert slice_.claims("tests/x/test_router.py", frozenset({"test_get"})) is True + + +def test_matching_is_substring_not_word_so_cache_and_caching_are_different_terms(): + # The real config excludes both, because "cache" does not occur inside "caching"; + # collapsing them to one term would quietly let a whole file back in. + assert _slice(excluded=("cache",)).claims("tests/x/test_caching.py", frozenset()) is True + assert _slice(excluded=("cache",)).claims("tests/x/test_dual_cache.py", frozenset()) is False + + +def test_a_positive_term_can_be_satisfied_by_a_name_inside_the_file(): + # A job running -k "langfuse" claims test_logging.py when a test inside is named + # for langfuse, so treating the path alone as the match would report a false gap. + slice_ = _slice(required=("langfuse",)) + assert slice_.claims("tests/x/test_logging.py", frozenset({"test_langfuse_emits"})) is True + assert slice_.claims("tests/x/test_logging.py", frozenset({"test_datadog_emits"})) is False + + +def test_a_file_the_job_never_globs_is_not_its_problem(): + assert _slice().claims("tests/other/test_a.py", frozenset()) is False + + +def test_an_explicitly_named_file_is_claimed_whatever_the_keywords_say(): + # redis_caching_unit_tests names test_dual_cache.py outright, which is what keeps + # that file out of the report even though every -k in the globbing jobs drops it. + slice_ = _slice(globs=(), named=frozenset({"tests/x/test_dual_cache.py"}), excluded=("cache",)) + assert slice_.claims("tests/x/test_dual_cache.py", frozenset()) is True + + +def test_an_unparsed_keyword_expression_claims_everything_it_globs(): + # Staying silent beats guessing: an expression this parser cannot model must never + # be the reason a file is reported as unrun. + assert _slice(understood=False, excluded=("cache",)).claims( + "tests/x/test_caching.py", frozenset() + ) is True + + +def test_keyword_terms_splits_an_and_chain_into_required_and_excluded(): + required, excluded, understood = coverage._keyword_terms(("langfuse and not cache and not router",)) + assert (required, excluded, understood) == (("langfuse",), ("cache", "router"), True) + + +def test_keyword_terms_refuses_to_model_an_or_expression(): + assert coverage._keyword_terms(("cache or router",)) == ((), (), False) + + +def test_keyword_terms_refuses_to_attribute_a_selector_across_several_commands(): + # A job running two pytest commands offers no way to tell which glob a -k belongs + # to, and pairing one command's exclusion with the other's glob would invent a gap. + assert coverage._keyword_terms(("not cache",), attributable=False) == ((), (), False) + assert coverage._keyword_terms((), attributable=False) == ((), (), True) + + +def test_an_excluded_term_matching_only_an_inner_name_leaves_the_file_claimed(): + # -k "not cache" drops test_cache_key inside test_router.py and keeps the rest, so + # the file still runs. Reporting it would be a false alarm; the guard is per-file. + slice_ = _slice(excluded=("cache",)) + assert slice_.claims("tests/x/test_router.py", frozenset({"test_cache_key"})) is True + + +def test_character_class_globs_match_the_letter_shards_circleci_uses(): + # tests/local_testing is split by first letter; without character-class support every + # file in it looks unglobbed, and the slice guard would report the whole directory. + glob = "tests/local_testing/**/test_[a-mA-M]*.py" + assert coverage._token_covers(glob, "tests/local_testing/test_caching.py") is True + assert coverage._token_covers(glob, "tests/local_testing/test_router.py") is False + + +def test_the_repo_as_it_stands_has_no_unrecorded_slice_gap(): + findings = coverage._deselected_everywhere(coverage._load_allowlist()) + assert [f.subject for f in findings] == []