fix(e2e): exclude skipped tests from coverage-registry numerator - #35327
Merged
yuneng-berri merged 2 commits intoJul 31, 2026
Merged
Conversation
The collector read @pytest.mark.covers off every collected item, and collection does not evaluate skips, so a test carrying both a skip and a covers marker reported its cell as covered while asserting nothing. 17 files under tests/e2e do exactly that, which inflated the headline from 290/434 to 311/434. A cell now counts as covered only when at least one test pytest would actually run declares it; a cell claimed by both a live and a skipped test stays covered. Skip state comes from pytest's own evaluator, so skip, skipif (bool and string conditions), and module-level pytestmark resolve exactly as they do in the e2e run. Cells left uncovered this way are listed under the headline and exported as skipped_markers (JSON) and litellm_e2e_coverage_skipped_markers (Prometheus) so the gap surfaces instead of disappearing; the Loki line contract is unchanged. A marker on a skipped test that points outside the registry is still an orphan, so --strict keeps its reach. Because skipif resolves against the environment the collector runs in, the number now depends on that environment; run it where the e2e suite runs. A pytest.skip() call inside a test body remains invisible to a static pass, which the module docstring and README both state.
…itellm_/coverage-collector-skip-markers-d3f666
Contributor
Greptile SummaryThis PR corrects static e2e coverage reporting so skipped tests no longer contribute to the covered numerator.
Confidence Score: 5/5The PR appears safe to merge, with the changed reporting semantics covered by focused tests and no affected repository consumers left incompatible. The collector now excludes markers from tests pytest would skip, preserves coverage when another runnable test claims the same cell, and consistently reports skipped-only and orphan markers without introducing a reachable build or reporting failure.
|
| Filename | Overview |
|---|---|
| tests/e2e/coverage_registry/collector.py | Correctly separates runnable and skipped-only markers and propagates the resulting data through supported report formats without breaking repository callers. |
| tests/e2e/coverage_registry/test_collector.py | Adds focused regression coverage for skip evaluation, shared marker ownership, reporting output, and orphan handling. |
| tests/e2e/coverage_registry/README.md | Accurately documents environment-dependent skip evaluation and the static collector’s runtime-skip limitation. |
| tests/e2e/CLAUDE.md | Updates test-harness guidance to clarify that skipped tests do not retain coverage credit. |
Reviews (1): Last reviewed commit: "Merge remote-tracking branch 'origin/lit..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
ryan-crabbe-berri
approved these changes
Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TLDR
Problem this solves:
How it solves it:
coversmarkers by whether pytest would run the testRelevant issues
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
Up front, so a reviewer does not go looking for it: this change has no proxy or provider surface to curl.
coverage_registry/collector.pyis a static reporting tool that reads markers out of a collect-only pass, spins up no proxy, and issues no LLM call. What the consumer of this tool actually sees is its stdout and the Grafana series built from it, so the proof below is that output, captured before and after against the same registry (434 rows either way) with no test edits in betweenBefore, at
83c256f609(the parent of the fix, both files incoverage_registry/checked out at that commit):After, at
b97e29eb5f, same command:Every one of those 21 traces back to a real marker pair in the tree; spot-check any of them the same way:
Two nuances a reviewer should know about that list.
mcp.list_tools.oauth.succeedsandmcp.call_tool.oauth.succeedscome from a module-levelskipifonE2E_LINEAR_STORAGE_STATE, so they drop out only in an environment without that session captured; where the e2e suite has it, both count as covered again. That is the intended behavior, and it does mean the number is now a property of the environment the collector runs in, so run it where the suite runs. Separately, apytest.skip()call inside a test body still cannot be seen by a static pass, since it does not exist until the test runs; the module docstring and the registry README both say so rather than leaving it impliedType
🐛 Bug Fix
Changes
_CoversSinkread@pytest.mark.covers(...)off every collected item, and collection never evaluates skips, so a test carrying both a skip and a covers marker handed its cell full credit while asserting nothing. Seventeen files undertests/e2edo exactly that todayA cell now counts as 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, since one runnable assertion is enough. Skip state is resolved with pytest's own evaluator rather than a local reimplementation, so
skip,skipif(both boolean and string conditions), and module-levelpytestmarkbehave here exactly as they behave in the run itselfCells knocked out of the numerator by a skip do not just vanish. They print under the headline, ship as
skipped_markersin the JSON output, and ship aslitellm_e2e_coverage_skipped_markersin the Prometheus output, so an unskip-pending gap stays visible. The Loki formatter keeps its exact line contract, oneCOVERAGE_TOTALplus oneCOVERAGE_MODULEper module; only its numbers change. A marker on a skipped test that points outside the registry is still reported as an orphan, so--strictgains no blind spotcollect_covered_idsbecomescollect_markersand returns a frozenCollectedMarkersinstead of a widening tuple. Nothing outside the module called itQA runbook
The collector is harness tooling rather than a product feature, so its tests carry no
e2emarker and need no live proxy (tests/e2e/CLAUDE.mdcalls outcoverage_registry/test_collector.pyas exactly this exception). The manual steps below are therefore local commands, and they map 1:1 to what the new tests asserttests/e2e/coverage_registry/test_collector.py::test_collection_counts_only_markers_on_tests_that_would_run - a real collect-only pass credits a cell only when the test claiming it would run
@pytest.mark.skip, one underskipif(True, reason=...), one underskipif(False, reason=...), one underskipif("True"), each with its own@pytest.mark.coversidpytestmark = pytest.mark.skipif(True, reason=...)and holds one more covering testcollect_markers(Path("<that dir>"))fromtests/e2ewithPYTHONPATH=.and expectcoveredto hold only the plain andskipif(False)ids, with the other four inskipped_onlycoveredand absent fromskipped_onlytests/e2e/coverage_registry/test_collector.py::test_cell_claimed_only_by_a_skipped_test_is_uncovered - a skipped-only cell is uncovered everywhere the report is consumed, not just in the headline
compute_coveragewith two P0 cells, one incoveredand one inskipped_onlycoveredandp0_coveredof 1, and the skipped id back inp0_gapsskipped_markersin the JSON render, and counted bylitellm_e2e_coverage_skipped_markersin the Prometheus rendertests/e2e/coverage_registry/test_collector.py::test_skipped_marker_outside_the_registry_is_still_an_orphan - skipping a test does not hide a typo'd cell id from
--strictcompute_coveragewith one registry cell and an unknown id inskipped_onlyorphan_markersandskipped_markersemptyWhole file, from
tests/e2e:The first of those three was checked red before green: with the skip lookup stubbed out to
return False, it fails naming exactly the four cells that leak back intocovered.make lint-e2e-basedpyrightreports 0 errors overtests/e2eFinal Attestation