diff --git a/tests/e2e/CLAUDE.md b/tests/e2e/CLAUDE.md index 17aee22560c..180639b53e3 100644 --- a/tests/e2e/CLAUDE.md +++ b/tests/e2e/CLAUDE.md @@ -85,6 +85,8 @@ The metric is coverage: the share of registry rows that have a passing covering Tests do not declare a dashboard module directly. They only declare the registry cell id with `@pytest.mark.covers("...")`; the registry row decides the module, tier, endpoint, and dashboard rollup. Run `python -m coverage_registry.collector --strict` when you want CI to reject unknown marker ids. Add `--fail-on-collection-errors` when the job should also fail on pytest collection errors. +Skipping a test gives its cell back to the gap list: the collector counts a cell as covered only when a test pytest would actually run declares it, and prints the cells left claimed only by skipped tests. So a `@pytest.mark.skip` on a red cell is honest bookkeeping, not a way to keep the number up. + ### Naming grammar per module LLMs - endpoint features (subject = the route), seeded from the Claude Code compat matrix. `chat_completions`, `messages`, and `responses` roll up to `Core LLMs`. Other LLM endpoints, including `batches` and `realtime`, roll up to `Non-Core LLMs`. diff --git a/tests/e2e/coverage_registry/README.md b/tests/e2e/coverage_registry/README.md index aef4c16c89a..5627c88dee4 100644 --- a/tests/e2e/coverage_registry/README.md +++ b/tests/e2e/coverage_registry/README.md @@ -37,6 +37,16 @@ def test_openai_streaming_tool_calls(self) -> None: It is static: a collect-only pass reads the markers, so it runs no test and needs no live proxy. Whether a covered cell currently passes or fails is a separate, live concern. +A skipped test asserts nothing, so its markers do not count. A cell is covered only when +at least one test pytest would actually run declares it; a cell claimed by both a live +test and a skipped one stays covered. Skip state comes from pytest's own evaluator, so +`skip` and `skipif` resolve exactly as they do in the e2e run, which also means a +`skipif` on an absent credential makes that cell uncovered in the environments where the +test cannot run. Cells left uncovered this way are listed under the headline (and counted +by `litellm_e2e_coverage_skipped_markers`) so an unskipped-pending gap is visible rather +than inflating the number. The one skip the collector cannot see is `pytest.skip()` +called from inside a test body, since it does not exist until the test runs. + ``` cd tests/e2e && PYTHONPATH=. python -m coverage_registry.collector ``` diff --git a/tests/e2e/coverage_registry/collector.py b/tests/e2e/coverage_registry/collector.py index 50ef23bcb1a..e20f7884f55 100644 --- a/tests/e2e/coverage_registry/collector.py +++ b/tests/e2e/coverage_registry/collector.py @@ -5,6 +5,13 @@ Coverage here is static: it reads the markers via a collect-only pass, so it run no test and needs no live proxy. Whether a covered cell currently passes or fails (covered_pass vs covered_fail) is a separate, live concern layered on top later. +A skipped test asserts nothing, so its markers do not count: a cell is covered +only when at least one test that pytest would actually run declares it. Skip +state is read with pytest's own evaluator, so `skip` and `skipif` are resolved +exactly as the e2e run resolves them in this environment. The one skip the +collector cannot see is `pytest.skip()` called from inside a test body, which +does not exist until the test runs. + cd tests/e2e && PYTHONPATH=. python -m coverage_registry.collector """ @@ -20,6 +27,7 @@ from pathlib import Path from typing import Literal import pytest +from _pytest.skipping import evaluate_skip_marks from pydantic import BaseModel from .registry import load_registry @@ -28,22 +36,57 @@ from .schema import MODULE_ORDER, Cell, Tier, dashboard_module, loki_module_labe E2E_DIR = Path(__file__).resolve().parent.parent +@dataclass(frozen=True, slots=True) +class CollectedMarkers: + """What a collect-only pass saw: cell ids declared by tests that would run, + cell ids only ever declared by skipped tests, and nodes that failed to import.""" + + covered: frozenset[str] + skipped_only: frozenset[str] + collection_errors: tuple[str, ...] + + +def _is_skipped(item: pytest.Item) -> bool: + """True when pytest would skip this test instead of running it. + + A marker pytest cannot evaluate (for example a bare boolean `skipif` with no + reason) turns into a setup failure at run time, so the test asserts nothing + either way and is treated the same as a skip. + """ + try: + return evaluate_skip_marks(item) is not None + except (pytest.fail.Exception, TypeError): + return True + + class _CoversSink: """Pytest plugin: after collection, capture every cell id declared via - @pytest.mark.covers(...), plus any nodes that failed to import.""" + @pytest.mark.covers(...) split by whether its test would run, plus any nodes + that failed to import.""" def __init__(self) -> None: self.covered_ids: frozenset[str] = frozenset() + self.skipped_only_ids: frozenset[str] = frozenset() self.collection_errors: tuple[str, ...] = () def pytest_collection_finish(self, session: pytest.Session) -> None: - marker_args: tuple[tuple[object, ...], ...] = tuple( - marker.args - for item in session.items + marker_args: tuple[tuple[bool, tuple[object, ...]], ...] = tuple( + (skipped, marker.args) + for item, skipped in ((i, _is_skipped(i)) for i in session.items) for marker in item.iter_markers(name="covers") ) + declared = tuple( + (skipped, arg) + for skipped, args in marker_args + for arg in args + if isinstance(arg, str) + ) self.covered_ids = frozenset( - arg for args in marker_args for arg in args if isinstance(arg, str) + cell_id for skipped, cell_id in declared if not skipped + ) + self.skipped_only_ids = ( + frozenset(cell_id for skipped, cell_id in declared if skipped) + - self.covered_ids ) def pytest_collectreport(self, report: pytest.CollectReport) -> None: @@ -51,10 +94,8 @@ class _CoversSink: self.collection_errors = (*self.collection_errors, report.nodeid) -def collect_covered_ids( - e2e_dir: Path = E2E_DIR, -) -> tuple[frozenset[str], tuple[str, ...]]: - """Return (covered cell ids, nodeids that failed to import).""" +def collect_markers(e2e_dir: Path = E2E_DIR) -> CollectedMarkers: + """Read every @pytest.mark.covers marker in `e2e_dir` via a collect-only pass.""" sink = _CoversSink() with contextlib.redirect_stdout(io.StringIO()): pytest.main( @@ -68,7 +109,11 @@ def collect_covered_ids( ], plugins=[sink], ) - return sink.covered_ids, sink.collection_errors + return CollectedMarkers( + covered=sink.covered_ids, + skipped_only=sink.skipped_only_ids, + collection_errors=sink.collection_errors, + ) @dataclass(frozen=True, slots=True) @@ -93,6 +138,7 @@ class CoverageReport: p0_covered: int p0_gaps: tuple[str, ...] orphan_markers: tuple[str, ...] + skipped_markers: tuple[str, ...] collection_errors: tuple[str, ...] @property @@ -122,6 +168,7 @@ def compute_coverage( cells: tuple[Cell, ...], covered: frozenset[str], collection_errors: tuple[str, ...] = (), + skipped_only: frozenset[str] = frozenset(), ) -> CoverageReport: p0_cells = tuple(c for c in cells if c.tier is Tier.P0) registry_ids = frozenset(c.id for c in cells) @@ -132,7 +179,8 @@ def compute_coverage( p0_total=len(p0_cells), p0_covered=sum(1 for c in p0_cells if c.id in covered), p0_gaps=tuple(sorted(c.id for c in p0_cells if c.id not in covered)), - orphan_markers=tuple(sorted(covered - registry_ids)), + orphan_markers=tuple(sorted((covered | skipped_only) - registry_ids)), + skipped_markers=tuple(sorted(skipped_only & registry_ids)), collection_errors=collection_errors, ) @@ -161,6 +209,15 @@ def render(report: CoverageReport) -> str: if report.orphan_markers else () ) + skipped = ( + ( + f"\n{len(report.skipped_markers)} cell(s) are claimed only by skipped tests, " + f"so they count as uncovered (unskip the test or drop the marker):\n " + + "\n ".join(report.skipped_markers), + ) + if report.skipped_markers + else () + ) warning = ( ( f"\nWARNING: {len(report.collection_errors)} node(s) failed to import during " @@ -170,7 +227,7 @@ def render(report: CoverageReport) -> str: if report.collection_errors else () ) - return "\n".join((*lines, *orphans, *warning)) + return "\n".join((*lines, *orphans, *skipped, *warning)) def _report_dict(report: CoverageReport) -> dict[str, object]: @@ -190,6 +247,7 @@ def _report_dict(report: CoverageReport) -> dict[str, object]: for m in report.modules ], "orphan_markers": list(report.orphan_markers), + "skipped_markers": list(report.skipped_markers), "collection_errors": list(report.collection_errors), } @@ -234,6 +292,9 @@ def render_prometheus(report: CoverageReport) -> str: "# HELP litellm_e2e_coverage_orphan_markers Coverage markers not found in the registry.", "# TYPE litellm_e2e_coverage_orphan_markers gauge", f"litellm_e2e_coverage_orphan_markers {len(report.orphan_markers)}", + "# HELP litellm_e2e_coverage_skipped_markers Registry cells claimed only by skipped tests.", + "# TYPE litellm_e2e_coverage_skipped_markers gauge", + f"litellm_e2e_coverage_skipped_markers {len(report.skipped_markers)}", "# HELP litellm_e2e_coverage_collection_errors Pytest nodes that failed during collection.", "# TYPE litellm_e2e_coverage_collection_errors gauge", f"litellm_e2e_coverage_collection_errors {len(report.collection_errors)}", @@ -286,8 +347,13 @@ def main() -> int: ) args = _CliArgs.model_validate(vars(parser.parse_args())) cells = load_registry() - covered, errors = collect_covered_ids() - report = compute_coverage(cells, covered, errors) + markers = collect_markers() + report = compute_coverage( + cells, + markers.covered, + markers.collection_errors, + markers.skipped_only, + ) output = { "text": render, "json": render_json, diff --git a/tests/e2e/coverage_registry/test_collector.py b/tests/e2e/coverage_registry/test_collector.py index 079ee215866..a85190cc3ba 100644 --- a/tests/e2e/coverage_registry/test_collector.py +++ b/tests/e2e/coverage_registry/test_collector.py @@ -12,6 +12,7 @@ from pathlib import Path import pytest from coverage_registry.collector import ( + collect_markers, compute_coverage, render, render_json, @@ -61,6 +62,27 @@ def test_orphan_marker_is_reported_not_counted() -> None: assert report.orphan_markers == ("llm.ghost",) +def test_cell_claimed_only_by_a_skipped_test_is_uncovered() -> None: + cells = (_llm("llm.a", Tier.P0), _llm("llm.b", Tier.P0)) + report = compute_coverage( + cells, frozenset({"llm.a"}), skipped_only=frozenset({"llm.b"}) + ) + assert (report.covered, report.p0_covered) == (1, 1) + assert report.p0_gaps == ("llm.b",) + assert report.skipped_markers == ("llm.b",) + assert "only by skipped tests" in render(report) + assert '"skipped_markers": [\n "llm.b"\n ]' in render_json(report) + assert "litellm_e2e_coverage_skipped_markers 1" in render_prometheus(report) + + +def test_skipped_marker_outside_the_registry_is_still_an_orphan() -> None: + report = compute_coverage( + (_llm("llm.a", Tier.P0),), frozenset(), skipped_only=frozenset({"llm.ghost"}) + ) + assert report.orphan_markers == ("llm.ghost",) + assert report.skipped_markers == () + + def test_logging_and_guardrail_roll_up_into_one_module() -> None: cells = ( LoggingCell( @@ -175,6 +197,81 @@ def test_loki_render_exposes_exact_stdout_lines_for_loki() -> None: ) +_MARKED_TESTS = ''' +import pytest + + +@pytest.mark.covers("llm.runs") +def test_runs() -> None: + pass + + +@pytest.mark.skip(reason="stage red: product gap") +@pytest.mark.covers("llm.skipped") +def test_skipped() -> None: + pass + + +@pytest.mark.skipif(True, reason="credentials absent in this environment") +@pytest.mark.covers("llm.skipif_true") +def test_skipif_true() -> None: + pass + + +@pytest.mark.skipif(False, reason="credentials present in this environment") +@pytest.mark.covers("llm.skipif_false") +def test_skipif_false() -> None: + pass + + +@pytest.mark.skipif("True") +@pytest.mark.covers("llm.skipif_string") +def test_skipif_string_condition() -> None: + pass + + +@pytest.mark.covers("llm.shared") +def test_shared_cell_runs() -> None: + pass + + +@pytest.mark.skip(reason="stage red: product gap") +@pytest.mark.covers("llm.shared") +def test_shared_cell_skipped() -> None: + pass +''' + +_MODULE_LEVEL_SKIP = ''' +import pytest + +pytestmark = pytest.mark.skipif(True, reason="whole module needs a session fixture") + + +@pytest.mark.covers("llm.module_skipped") +def test_module_level_skip() -> None: + pass +''' + + +def test_collection_counts_only_markers_on_tests_that_would_run( + tmp_path: Path, +) -> None: + """The collect-only pass is the numerator, so a test pytest would skip must not + contribute its cell. A cell stays covered as long as one runnable test claims it.""" + (tmp_path / "test_marked.py").write_text(_MARKED_TESTS) + (tmp_path / "test_module_skip.py").write_text(_MODULE_LEVEL_SKIP) + + markers = collect_markers(tmp_path) + + assert markers.covered == frozenset( + {"llm.runs", "llm.skipif_false", "llm.shared"} + ) + assert markers.skipped_only == frozenset( + {"llm.skipped", "llm.skipif_true", "llm.skipif_string", "llm.module_skipped"} + ) + assert markers.collection_errors == () + + def test_real_registry_loads_and_ids_are_unique() -> None: cells = load_registry() ids = [c.id for c in cells]