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.

Sign up for GitHub

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

Jump to bottom

Move coverage settings to coverage cfg #462

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

Move coverage settings to coverage cfg #462

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
532a74c
chore: move coverage settings to coverage cfg
Olegt0rr Feb 5, 2024
d59539f
coverage orchestrate testing
Olegt0rr Feb 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .coveragerc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
# .coveragerc to control coverage.py
[run]
branch = True
include =
aiohttp_sse/*
tests/*

[report]
exclude_also =
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,10 @@ jobs:
with:
path: requirements.txt
- name: Run unittests
run: pytest tests
run: coverage run -m pytest ./tests
env:
COLOR: 'yes'
- run: python -m coverage xml
- run: coverage xml
- name: Upload coverage
uses: codecov/codecov-action@v3
with:
Expand Down
2 changes: 0 additions & 2 deletions pytest.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

asyncio_mode = auto
filterwarnings =
error
Expand Down
Loading