-
Notifications
You must be signed in to change notification settings - Fork 35
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
base: master
Are you sure you want to change the base?
Conversation
@@ -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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
- 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.
- 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.
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