Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move coverage settings to coverage cfg #462

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Olegt0rr
Copy link
Collaborator

@Olegt0rr Olegt0rr commented Feb 5, 2024

What do these changes do?

Enables pyCharm coverage tool compatibility

Are there changes in behavior for the user?

Enables pyCharm coverage tool compatibility

Related issue number

Fixes #461

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes

@Olegt0rr Olegt0rr marked this pull request as draft February 5, 2024 11:14
@@ -8,8 +8,6 @@ addopts =
-ra
# show values of the local vars in errors:
--showlocals
# coverage reports
--cov=aiohttp_sse/ --cov=tests/ --cov-report term
Copy link
Member

Choose a reason for hiding this comment

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

We currently use this in every repo, are you just missing pytest-cov?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, it's installed :)

It works perfect if projects are set in .coveragerc
But throws tracebacks if projects are set like this.

Running from console works both ways, but there're no green/red coverage lines near the code.

Copy link
Member

Choose a reason for hiding this comment

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

@webknjaz Any thoughts on this? Given that I've been copying this to every repo.

Copy link
Member

Choose a reason for hiding this comment

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

@Dreamsorcerer so I've got a few points to relay (should probably post to the org discussions about this too...)

TL;DR

  1. I use an annotated config that's copied over across many projects with minimal changes/same ordering/indentation/etc. — this helps reduce merge conflicts when the same change has to be done everywhere. So I suggest reproducing the same here.
  2. Here's my ideal configs for now:

Justification

When integrating tools (like pytest or coveragepy) into workflow wrappers (like tox, nox, GNU/make etc.), we can pass arguments to those tools via CLI. But then, whenever those tools are called outside our scripts/automation, their behavior differs, because those CLI args aren't copied over. This happens when contributors integrate pytest/flake8/pylint etc. into their code editors directly, bypassing our wrappers. By putting settings into configs located in respective default discoverable locations, we make sure that if these low-level tools are called differently, they still pretty much behave the same. So the takeaway is to put as much as possible there. Everything that could be common/universal.

That said, there are some tool features that don't really make sense in other contexts/environments. And putting them into the shared config might be disruptive. As an example, we only really need a coverage.xml in CI — only to upload that to Codecov. The contributors don't need that file laying on disk for no reason. And some CLI options may result in making testing slower by enabling something that isn't needed. So --cov-report=xml would be added to CI, but not the config file. Same goes for --junitxml — both produce structured output files that can then be used to render Markdown tables in the GHA job summary with test report and coverage summaries. The config file would still enable the coverage collection through --cov globally (if the contributors want to, they can add --no-cov to their CLI or $PYTEST_ADDOPTS to override that).

--cov=<importable-or-folder> is ambiguous and prone to misbehaving. It attempts limiting what's collected but I think it didn't work for me in a number of cases, as I discovered. Besides, we really want to collect all the coverage possible and only limit what's displayed in report. A bare --cov does that.

Returning to the idea of having a separate config per low-level tool vs a wrapper, there's .coveragerc that configures coveragepy. If we set up stuff there, pytest-cov will just invoke the tool and its config would act as defaults. Specifying paths mappings is important and works better than whatever pytest-cov tries to do with --cov=<importable-or-folder>, it seems. And [report] is what configures --cov-report=term which is the default output, I suppose. The [run] section of the coveragepy config now has separate source_pkgs and source that lets us separate the runtime/library code from things like tests explicitly, no more --cov=<importable-or-folder> ambiguity.

Sometimes, pytest-cov may race with pytest-xdist since the loading order is non-deterministic. Plus, invocations like python -m pytest put cwd into $PYTHONPATH which sometimes may result in our project modules being imported earlier than coverage. To work well, it has to be loaded as early as possible. The only way to do that in pytest is through an explicit -p pytest_cov — it'll get activated before the auto-discovered plugins or conftest modules.

--cov-context=test is another thing that I set though CLI — I don't think it's possible to have it in the coveragepy config due to the dynamic interactions between the tools, necessary for this feature.

This leaves us with an almost vanilla pytest invocation in GHA:

python -Im pytest -v --cov-report=xml --junitxml=.test-results/pytest/test.xml

With the above setup, coverage xml invocation is indeed unnecessary, neither is coverage run--cov-report=xml encapsulates all that. The tests can be displayed through test-summary/action and coverage through python -Im coverage report --format=markdown >> "${GITHUB_STEP_SUMMARY}", in addition to the Codecov uploads.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Coverage config incompatible with pyCharm
3 participants