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

test(coverage): make coverage consistent and reproducible #1388

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

noirbizarre
Copy link
Contributor

@noirbizarre noirbizarre commented Oct 28, 2023

While working on #1386, I noticed unexpected coverage changes on some module. This is the consequence of 2 things:

  • tests are included in coverage, meaning that pytest-cov should be loaded early to avoid depending on plugins and tests load order
  • using pytest-xdist without --cov-append leading to some reports parts being erased/overwritten

This mean coverage is subject to change with:

  • test additions/removal, renaming, moves..., even if not related
  • plugin changes (added or removed plugin but also plugin update)

For reference:

So this PR:

  • use -p pytest_cov to ensure pytest-cov is loaded before other plugins
  • move all possible parameters into pyproject.toml to ensure consistent parameters whether tests/coverage are running in CI or locally
  • add the --cov-append parameter to coverage for better pytest-xdist handling
  • make the CI use poe coverage to ensure developers can run the same test suite locally

Note: test coverage has been kept, but I think codebase coverage should be enough and in this case, classical pytest/pytest-cov should be enough as you don't need to ensure coverage tooling is loaded before everything else.

pyproject.toml Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 29, 2023

Codecov Report

Merging #1388 (26695de) into master (9ea980f) will increase coverage by 0.72%.
Report is 1 commits behind head on master.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1388      +/-   ##
==========================================
+ Coverage   97.27%   97.99%   +0.72%     
==========================================
  Files          48       42       -6     
  Lines        4327     4285      -42     
==========================================
- Hits         4209     4199      -10     
+ Misses        118       86      -32     
Flag Coverage Δ
unittests 97.99% <ø> (+0.72%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 8 files with indirect coverage changes

@noirbizarre
Copy link
Contributor Author

noirbizarre commented Oct 29, 2023

My bad, I have been through multiple iterations to find one working, and I didn't submit the right one. This should be fixed.

But honestly, I would simply remove coverage on tests/ as this is totally unreliable (as you can see, you already have coverage reports for jinja files, or other templates, I tried to exclude them with the omit setting). You can try locally, even with this configuration (which will only remove some false-negative cases but not all), you might notice coverage changes between 2 consecutive runs without a single code change as well as part marked as uncovered while this is a test which has run.
I also think it dilute the real project coverage because most of the time you will have more tests than code to test and test coverage will always be 100% as soon as the test is executed if properly accounted, and the overall coverage will be higher than it should.

In the case of #1386 I think this is only a codecov misreport as the file flagged as loosing coverage was in fact already not covered (but this PR should fix its coverage except for the first test and I don't know why)

@yajo
Copy link
Member

yajo commented Oct 30, 2023

I tested it and now I get the report I expected.

I noticed this generates hundreds of .coverage.$hostname.* files. Can't that be avoided?

I would simply remove coverage on tests/ as this is totally unreliable

I think it's useful in the sense that some parts of the test you write could be not really exercised for some reason. Maybe you're missing some parametrization.

Keep in mind that codecov is configured to aggregate all reports. Some tests run only on some OS, but we push all of them and then they get summed. Maybe that affects?

@yajo yajo marked this pull request as draft January 15, 2024 18:15
@yajo
Copy link
Member

yajo commented Jan 15, 2024

Setting as draft until attended. @noirbizarre do you have plans to complete this?

@noirbizarre
Copy link
Contributor Author

OMG, sorry, I completly forgot this one.
I'm going to complete it soon

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.

None yet

2 participants