Skip to content

Introduce a local coverage plugin + add conditional exclusions#2347

Open
sirosen wants to merge 11 commits into
jazzband:mainfrom
sirosen:testing/coverage-local-plugin
Open

Introduce a local coverage plugin + add conditional exclusions#2347
sirosen wants to merge 11 commits into
jazzband:mainfrom
sirosen:testing/coverage-local-plugin

Conversation

@sirosen
Copy link
Copy Markdown
Member

@sirosen sirosen commented Mar 5, 2026

This PR is a continuation from #2268 .
As I mentioned in #2264 , I've worked up a local plugin and tried out using PYTHONPATH to make it importable under coverage, and locally my testing of it seems to work.

The plugin is defined in plugins/coverage/ as a module, and injects coverage excludes of the form

pragma: pip{comparator}{version} no cover

I've added it on top of the commits from #2268, rebased, and updated to use the new "pip-version pragmas" where possible.

Contributor checklist
  • Included tests for the changes.
  • A change note is created in changelog.d/ (see changelog.d/README.md
    for instructions) or the PR text says "no changelog needed".
Maintainer checklist
  • If no changelog is needed, apply the bot:chronographer:skip label.
  • Assign the PR to an existing or new milestone for the target version
    (following Semantic Versioning).

@sirosen
Copy link
Copy Markdown
Member Author

sirosen commented Mar 5, 2026

codecov was unhappy -- my guess about the issue, based on the output, was that it was treating the coverage plugin as "uncovered", so I've pushed a fix for that.

If the coverage still looks bad, I may have to look more deeply and potentially mark this draft until I can fix.

Copy link
Copy Markdown
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool that you got to looking into this! I've left a few comments inline.

Comment thread tests/test_cli_compile.py Outdated

# for older pip versions, recompute the output path to be relative to the input path
if not pip_produces_absolute_paths:
if not pip_produces_absolute_paths: # FIXME: figure out how to cover piplowest
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that wasn't supposed to still be there! 😳

I think that's from before I had the plugin working.
But it's wrong. Let me replace with a pragma.

Comment thread tests/conftest.py
@pytest.fixture
def runner():
if Version(version_of("click")) < Version("8.2"):
# Coverage is excluded because we only test with the latest Click
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we fix that? Or maybe make it not branchy by updating deps in the meta?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should go after a fix, but that it doesn't fit within the scope of this PR.

I've been considering the possible argument for vendoring click, on account of the fact that it would allow pip-tools to be used in environments with really old click versions.

Like pip, I think we should not have Python package dependencies, other than pip itself. (Also like pip, I think this will pose a licensing, SBOM, and transparency challenge! )

Comment thread .coveragerc
piptools_coverage
omit =
piptools/_compat/*
# any local plugins we use aren't part of normal testing/coverage
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, I'd argue that our own tooling is subject to being tested as well. Especially since it may grow into something that could eventually be ready to be moved out. So if possible, I'd add a test for the plugin.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I agree in principle, I think we'll have trouble unless we add a dedicated test job in tox and CI, with a dedicated and distinct .coveragerc. The reason is that coverage imports this code on startup, so it's imported too early to measure. I can't think of a way to measure it right now, though I was also considering reaching out to Ned (he's active in the main Python discord) to see if he could help point me in the right direction.

I'm happy to write tests for it, but I think it might belong outside of coverage reporting until I can figure that out?
I was hoping we could include it early, and I could continue to work on this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right.. I imagine we could maybe have some tests w/ the pytester fixture invoking them in a subprocess, wrapped in a coverage run maybe, shielded from auto-importing coverage and something along the lines of -p no:pytest_cov, perhaps.

I'll think about getting this in early.

Comment thread plugins/coverage/piptools_coverage.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I'm not sure I like a top-level public name like plugins/. Wonder if it could live in tests/_plugins/ or smth like that since it's a part of our test toolchain.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can move it around. tests/_plugins/_coverage/ works for me, if that sounds good?

Comment thread plugins/coverage/piptools_coverage.py Outdated
config.set_option("report:exclude_lines", sorted(exclude))


def coverage_init(registry: Plugins, options: dict[str, _t.Any]) -> None:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MyPy recommends object instead of _t.Any. Can we use that?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I circle back on this to do more work, I'll give it a try. 👍

Comment thread tests/test_cli_compile.py Outdated
(first_posarg, *_tail_args), _kwargs = PyPIRepository.call_args

if _pip_api.PIP_VERSION_MAJOR_MINOR >= (25, 3): # pragma: >=3.9 cover
if _pip_api.PIP_VERSION_MAJOR_MINOR >= (25, 3): # pragma: pip<25.3 no cover
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels a bit contradictary: the check is <= and the pragma's inversed. And then the opposite branch adds inversion that looks like what's in the if-expression down below. So I'd suggest supporting cover, not just no cover.

Althouh, can we analyse expressions with the known constants like _pip_api.PIP_VERSION_MAJOR_MINOR / _pip_api.PIP_VERSION and apply coverage inclusion/exclusion based on that automatically even w/o the comments? The else-branch could be handled the same way.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't I work on cover first, and we can think about supporting comparisons with _pip_api.PIP_VERSION attributes as a future bit of work?

Comment thread tox.ini Outdated
pipmain: https://github.com/pypa/pip/archive/main.zip
setenv =
# local plugin inclusion
coverage: PYTHONPATH=./plugins/coverage/
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know if pytest's pythonpath setting would be enough instead of this?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can give it a try! 👍

Comment thread plugins/coverage/piptools_coverage.py Outdated
CURRENT_YEAR_EPOCH = datetime.date.today().year - 2000
_PRAGMA_SUPPORTED_PIP_VERSIONS: list[tuple[int, int]] = [
(major, minor)
for major in range(22, CURRENT_YEAR_EPOCH + 1)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we read the lowest supported from pyproject.toml?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that's a good idea. Yes, we can extract it from there.

@sirosen sirosen marked this pull request as draft March 5, 2026 20:40
@sirosen
Copy link
Copy Markdown
Member Author

sirosen commented Mar 5, 2026

It looks like codecov is still displeased with me. 😀

I've marked as a draft while I continue to work on this. I'll mark it ready when I feel that I've gotten it to a good place again. I have lots of lovely feedback to act upon, noted above, so changes to be made.

As for codecov, I'm not totally sure. I clearly misdiagnosed the "loss of coverage" it's flagging, but the reporting it shows doesn't quite match my expectations there. I think it might be that it sees the missing else branches of various conditionals as "untested" because they aren't explicit and flagged with appropriate pragmas. That will be one of my next experiments.

@sirosen
Copy link
Copy Markdown
Member Author

sirosen commented Mar 6, 2026

I moved the plugin into tests/_plugins/_coverage/, and tweaked our pytest config to put it in pythonpath but exclude it from test discovery. That works!

If we go with this approach, I plan to write a short blog post to share the technique of a local coverage plugin. It's not something I've seen anyone write about before, and it wasn't hard but it did have some non-obvious requirements (most notably, pythonpath manipulation).


I've enhanced the plugin with pragma: pip{comparator}X.Y cover pragmas, and added a suite of unit tests to help get those bits correct. I find these easy to get confused when implementing -- there's some reasoning about double-negatives involved in checking this. I've been doing some local test runs and looking at coverage reporting, and it seems to be working as desired.

I've also explicitly expanded a lot of else branches for pip-version conditionals, almost all of which are else: pass, so that they can be marked with the appropriate pragmas. That fixed coverage branch reporting showing up as partial, and seems to have gotten codecov to pass. As part of that body of work, I aligned the pragmas with the comparisons being done. They now typically read like so:

if _pip_api.PIP_VERSION_MAJOR_MINOR >= (22.3):  # pragma: pip>=22.3 cover
    ...
else:  # pragma: pip>=22.3 no cover
    ...

That is, I've been using the matching comparison in both branches, but with cover vs no cover.


Deriving the minimum pip version from pyproject.toml involved making a few decisions:

  1. How do we find pyproject.toml? I went with cwd() / pyproject.toml, since tox will always execute from the repo root.
  2. What do we do if there are 0 or multiple pip versions listed? I treat this as an error.
  3. How sophisticated do we want to be in parsing the pip version specifier? Again, I treated many cases as errors.

My reasoning around bailing out early with errors is that we definitely don't want to try to solve the general case of "what is the minimum version of package X which satisfies this dependency list?" as it's a hard problem. If we make some simplifying assumptions -- and treat anything unexpected as an error -- we get an easy problem for now and can improve it if we ever need to.


I'm watching CI to see if it comes up healthy. If everything looks okay, I feel like I can mark this ready.

@sirosen sirosen marked this pull request as ready for review March 6, 2026 04:52
@sirosen
Copy link
Copy Markdown
Member Author

sirosen commented Apr 3, 2026

I've been working on self-testing this plugin and getting full coverage reporting, including the plugin itself.

I intend to open it up as a separate draft PR for follow-up work. I'm not certain that we'll easily agree about exactly how to do that. (I've tried a couple of things and have something working.)

Because it's more fraught, I'd like to separate that from the PR here which introduces the plugin and its usage. I'm rebasing now to cleanup conflicts.

if _pip_version.PIP_VERSION_MAJOR_MINOR >= (26, 0):
if _pip_version.PIP_VERSION_MAJOR_MINOR >= (26, 0): # pragma: pip>=26.0 cover
cmdoptions.check_release_control_exclusive(options)
else: # pragma: pip>=26.0 no cover
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any way to apply this to the implicit else-branches automatically?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may or may not be possible, but I don't have a theory as to how as of today.
It would need a different mechanism from the current excludes -- I looked a little at the coveragepy plugin interfaces, but I don't know how to do it (yet?)!

I wouldn't want to block on this, but I agree that it's desirable.



def get_min_supported_pip_major_version() -> int:
pyproject_toml = read_project_pyproject_toml()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if it'd be possible to use a .coveragerc.toml instead.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, never mind. We don't have own config.

return datetime.date.today().year - 2000


def read_project_pyproject_toml() -> dict[str, _t.Any]:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we returned the deps from this function? The typing would be better.
Alternatively, we could use a typed dict that has project⇾dependencies defined. This would help us avoid typing.Any.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I can make that tweak.

pip_requirements = [r for r in parsed_dependencies if r.name == "pip"]

# we will presently assume that there's exactly one declared dependency on 'pip'
# which sets the lower bound
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sirosen now that we have that stable extra, we should probably parse it for the upper bound too.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though, I guess get_max_pip_major_version() takes care of that..

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[stable] isn't merged yet, but I'd be happy to rebase and switch to it if we get it in. (Right now it's marked as a draft, but I don't think there is any blocker for merging it? I'd have to re-check.)

Comment on lines +112 to +115
if opfunc((current_major, current_minor), (major, minor)):
result.append(rf"# pragma: pip{opname}{major}.{minor} no cover\b")
else:
result.append(rf"# pragma: pip{opname}{major}.{minor} cover\b")
Copy link
Copy Markdown
Member

@webknjaz webknjaz May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about

Suggested change
if opfunc((current_major, current_minor), (major, minor)):
result.append(rf"# pragma: pip{opname}{major}.{minor} no cover\b")
else:
result.append(rf"# pragma: pip{opname}{major}.{minor} cover\b")
nope = "no " if opfunc((current_major, current_minor), (major, minor)) else ""
result.append(rf"# pragma: pip{opname}{major}\.{minor} {nope}cover\b")
if not opfunc((current_major, current_minor), (major, minor)):
result.append(fr"^\s*if\s*_pip_api\.PIP_VERSION_MAJOR_MINOR\s*{opname}\s*({major},\s*{minor}):\b")

?

Though, the addition of native _pip_api.PIP_VERSION_MAJOR_MINOR checks reveals the need to have a mapping of opposite COMPARATORS names...

return result


class PipVersionPragmas(CoveragePlugin): # type: ignore[misc]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment on what's ignored exactly?

def compute_pip_version_exclude_pragmas() -> list[str]:
current_major, current_minor = get_pip_major_minor()

result: list[str] = [
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use a set right away, seeing that the caller works with sets?

def configure(self, config: CoverageConfig) -> None:
exclude = set(config.get_option("report:exclude_lines"))
exclude.update(compute_pip_version_exclude_pragmas())
config.set_option("report:exclude_lines", sorted(exclude))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we use exclude_also for this?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think either works equivalently in this case, because we preserve the pre-existing value. Let me try switching it to report:exclude_also and I'll confirm back here if it works.

Comment on lines +122 to +124
exclude = set(config.get_option("report:exclude_lines"))
exclude.update(compute_pip_version_exclude_pragmas())
config.set_option("report:exclude_lines", sorted(exclude))
Copy link
Copy Markdown
Member

@webknjaz webknjaz May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the helper were to return a set, this could be something like

Suggested change
exclude = set(config.get_option("report:exclude_lines"))
exclude.update(compute_pip_version_exclude_pragmas())
config.set_option("report:exclude_lines", sorted(exclude))
exclude_also_line_regexes = (
compute_pip_version_exclude_pragmas()
| set(config.get_option("report:exclude_also"))
)
config.set_option("report:exclude_also", sorted(exclude_also_line_regexes))

def test_copy_install_requirement_removes_pip_25_3_unsupported_opts(use_pep517):
def test_copy_install_requirement_removes_pip_25_3_unsupported_opts(
use_pep517,
): # pragma: pip<25.3 no cover
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also have marks like @pytest.mark.requires_pip_25_3_plus (or just requires_pip_25_3_plus defined as requires_pip_25_3_plus = pytest.mark.skipif( _pip_api.PIP_VERSION_MAJOR_MINOR < (25, 3), reason="test requires pip>=25.3" ) for runtime) that would work for coverage skips too and would be better integrated.
This is how I skip xfail elsewhere, for example: https://github.com/aio-libs/yarl/blob/e25e8d23e6912db52a23513ef1f6a17f889751ef/.coveragerc#L18C3-L18C27.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the other suggested changes (e.g., switch the return type in the plugin to set) are things I'm game to tinker with, but I'd like to leave this one out at least for a first draft. I'm not sure if it should be @pytest.mark.requires_pip_25_3_plus or maybe more like... @pytest.mark.requires_pip(">=25.3") or something else. I'd like to have more time and space to explore this without blocking the rest of the work.

rf"# pragma: pip=={current_major}.{current_minor} no cover\b",
]

for major, minor in list_supported_pip_versions():
Copy link
Copy Markdown
Member

@webknjaz webknjaz May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we also generate entries matching if _pip_api.PIP_VERSION_MAJOR_MINOR <= (major, minor):?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do that, yeah! There's a bit of a question for me of how many patterns we want to have here at the start. I'll add all of the comparators with _pip_api.PIP_VERSION_MAJOR_MINOR as the left-hand-side.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this for a couple of hours and I'm not sure why, but it didn't work. I'm happy to try again, but I don't want to include it in this PR.

webknjaz and others added 7 commits May 16, 2026 13:02
These are either chronically uncovered up to a certain line or
have flaky coverage. Either way, having them in is rather harmful
for measurements.
These are version-dependent but are sometimes applied to conditionals
checking the deps versions.
They won't work perfectly but it's a start.
A new local plugin in `plugins/coverage/` as a standalone module, which
injects coverage excludes of the form

    pragma: pip{comparator}{version} no cover

In order for this plugin to be picked up, `.coveragerc` is updated to
list the plugin by name and `PYTHONPATH` is set via `setenv` in order to
make the module importable.

All conditional checks on the pip version are now marked with the
relevant matching pragmas.
When looking at lost coverage reporting in codecov, it appears that it is
counting the new plugin code as "uncovered". Though true -- it isn't
being tested rigorously -- trying to measure it seems incorrect at
present.
In order to support these, the logic of the plugin had to become a bit
more organized -- it now uses a list of supported operators to build out
the full suite of pragmas.

With these new pragmas available, update the various usage sites to use
the affirmative 'cover' pragmas on comparisons, with the corresponding
'no cover' pragmaes on else branches.
To improve branch coverage, all of the `else` branches are explicitly
enumerated and captured with the appropriate pragmas, even though many
are `else: pass`.
This is added to the python path via pytest options, and is excluded from
pytest discovery (norecursedirs).

Also, add more pip-version pragmas to test code which only runs on
specific pip versions.
In order to determine the lowest supported version of `pip`, read the
`project.dependencies` list from `pyproject.toml`, find the only listed
`pip` dependency, and pull its specifier.

If any of the data does not match expectations, a ValueError will be
thrown (crashing any run of coverage/pytest-cov) with a message stating
that the plugin needs to be updated. This lets us get a working solution
without trying to solve a more general case than what we need.
sirosen and others added 4 commits May 16, 2026 13:02
Unit tests of the local coverage plugin exercise its various helpers, and
the module-level docstring explains that we can test the parts even if we
can't easily measure code coverage on the plugin itself.
These tests need `coverage` to be installed, and it is missing
1. In CI builds on pypy where`coverage` is not installed
2. When local tests run without the `coverage` factor
Local coverage runs won't report on this file, as configured, but
codecov is still flagging it as uncovered code.
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <578543+webknjaz@users.noreply.github.com>
@sirosen sirosen force-pushed the testing/coverage-local-plugin branch from 663029d to eb49cc3 Compare May 16, 2026 18:02
@nedbat
Copy link
Copy Markdown

nedbat commented May 18, 2026

Sorry I'm late to this, @sirosen showed this to me at PyCon. I'm wondering if there is a way to get what you need without code. For example, this tox.ini defines environment variables:

[testenv]
setenv =
    py27: COVERAGE_MAJOR = 2
    py27: COVERAGE_MAJORMINOR = 27
    py{34,35}: COVERAGE_MAJOR = 3
    py34: COVERAGE_MAJORMINOR = 34
    py35: COVERAGE_MAJORMINOR = 35

then uses coverage settings to define pragmas:

[report]
exclude_lines =
    ✘py${COVERAGE_MAJOR}
    ✘py${COVERAGE_MAJORMINOR}
    pragma: no ?cover

@sirosen
Copy link
Copy Markdown
Member Author

sirosen commented May 20, 2026

Thanks for the tips, @nedbat ! I'm pretty interested in this topic. I think this would expand out into a really large configuration in our case.

I'll try to produce a worked example, maybe in a separate repo, and I'll share it with you when I'm able. I think the abstract version of the plugin I've written here is pretty simple. It's pip-tools-specific stuff that makes it more complicated looking!

Update:
I shared this in the coverage-py channel of the Python discord, but I worked out the plugin as a standalone example to make it easier to dsicuss. I'm happy to talk about it here or there; wherever is convenient. 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants