mirror of
https://github.com/BerriAI/litellm.git
synced 2026-08-28 05:25:59 +00:00
feat(ci): catch files a -k expression deselects from every job (#37601)
* feat(ci): catch files a -k expression deselects from every job The coverage census asks whether some job names a file. It cannot ask what that job's -k then does with it, and the gap is not hypothetical: tests/local_testing is globbed by five jobs, two of which carry -k "... and not router and not assistants and not langfuse and not caching and not cache" while the other three keep one keyword each. Any file whose path holds an excluded term is dropped by the first two and matched by none of the rest, so it runs nowhere while the census counts it as covered. 118 tests across eight caching files sit in exactly that hole today. The new mode reads the same CircleCI jobs the census already parses and asks whether each globbed file survives its job's selector. Two facts about -k make that decidable without running pytest: it matches an item's own name and its parents', so a term appearing in the module path deselects the whole file; and the names it can match are otherwise the classes and functions in the file, which ast reads. A positive term is therefore satisfied by the path or by a name inside, which is what keeps a langfuse-named test inside test_logging.py from being reported. Where the parser is unsure it stays quiet. An expression with or, parentheses, or a negated group is left unmodelled and its job is treated as claiming everything it globs, so an unparsed selector can never raise a false alarm. Glob translation learned character classes, without which tests/local_testing/**/test_[a-mA-M]*.py matches nothing and the guard would report that whole directory. The census and shard counts are unchanged by it, 2423 files and 327 shard children before and after. Validated against the real thing: collecting tests/local_testing under each job's own selector leaves 175 of 1577 tests unselected, in exactly the ten files this check derives statically, no more and no fewer. Two of the ten are named outright by other jobs, which the check credits, leaving the eight now recorded in the allowlist as a decision rather than an accident. Verified red-first: dropping one of those eight from the allowlist reports it, and adding 'and not embedding' to the two part jobs reports test_embedding.py and test_get_optional_params_embeddings.py. * fix(ci): keep the slice guard from pairing one command's -k with another's glob Two accuracy notes from review, both about the parser's model rather than its current verdicts. A job that runs several pytest commands offers no way to tell which glob a -k belongs to, since both are read out of the same flattened job text. Combining them could pair one command's exclusion with another command's glob and report a file that in fact runs. Such a job is now left unmodelled, which means it claims everything it globs, matching how the parser already treats an expression it cannot read. Only one job in the config has two globs today and it carries no -k at all, so no verdict changes. The second is a deliberate limit, now stated where it lives: an excluded term is only honoured when it sits in the module path, because that is the case that takes the whole file with it. A term matching one function inside drops that test and leaves the file running, and reporting it would be a false alarm. Answering per-test instead would need a baseline of test ids that churns on every rename, for a smaller failure than a file going dark. Both are pinned by tests.
This commit is contained in:
parent
569dcf435d
commit
a48baefc95
4 changed files with 273 additions and 5 deletions
19
.github/ci-coverage-allowlist.yml
vendored
19
.github/ci-coverage-allowlist.yml
vendored
|
|
@ -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
|
||||
|
|
|
|||
155
.github/scripts/assert_ci_coverage.py
vendored
155
.github/scripts/assert_ci_coverage.py
vendored
|
|
@ -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()
|
||||
|
|
|
|||
6
.github/workflows/ci-coverage.yml
vendored
6
.github/workflows/ci-coverage.yml
vendored
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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] == []
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue