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

fix: combined coverage report #238

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

Conversation

rudaporto
Copy link

@rudaporto rudaporto commented Oct 15, 2022

Currently, we have some issues regarding the combined coverage:

  • we have two sections in the tox.ini to check the coverage: one using only Python 3.8 and one extra combine all the coverage generated from each python version
  • the combined-coverage section does not work because of the order of the coverage section that runs before
  • the coverage section is used in the CI together with coveralls but it only checks with one Python version meaning it can not enforce it to be 100% because i.e: https://coveralls.io/jobs/107753481

This PR is a POC to try to find a better way to implement both, the local coverage check and the CI one.

** I',m aware that these files are autogenerated by the meta package. If we agree that this makes sense I would like to propose the creation of new templates in the meta package to handle this use case. **

Here follow all the changes I implemented:

Tox:

  • coverage env should always be the last
  • default coverage should be enough (no need for the extra combined-coverage config)
  • fix the coverage env to do the combine and fail under 100%

CI:

  • build: remove the coverage from the matrix since it will become a separate job
  • build: add a new upload artifact step to run after tests
  • coverage: a new job that depends on the build job
  • coverage: add a step to download all artifacts uploaded from the build steps
  • coverage: add a step to combine the coverage artifacts and upload them to coveralls
  • coverage: add a step to build the report and check if coverage matches 100%
  • coverage (optional): add a step to upload the HTML report for when the job failed

Next step:

@rudaporto rudaporto force-pushed the fix-tox-config-file-coverage-report branch 2 times, most recently from e352897 to 579c0b1 Compare October 15, 2022 15:15
@rudaporto rudaporto force-pushed the fix-tox-config-file-coverage-report branch from 579c0b1 to 95272c2 Compare October 15, 2022 15:33
@rudaporto
Copy link
Author

When using the matrix execution we can not collect the combined coverage from each python version.

@rudaporto rudaporto closed this Oct 15, 2022
@davisagli
Copy link
Member

https://hynek.me/articles/ditch-codecov-python/ has a suggestion of how to do this

@rudaporto rudaporto reopened this Oct 16, 2022
@rudaporto rudaporto force-pushed the fix-tox-config-file-coverage-report branch 3 times, most recently from 3eeac16 to 9730e11 Compare October 16, 2022 08:52
@dataflake
Copy link
Member

FYI, the file .github/workflows/tests.yml is auto-generated and should not be edited by hand. It uses the meta/config package, see https://github.com/zopefoundation/meta/tree/master/config, which is configured using the .meta.toml file.

@rudaporto rudaporto force-pushed the fix-tox-config-file-coverage-report branch 10 times, most recently from 619000e to 338da05 Compare October 16, 2022 10:56
Build job:
- add upload artifact step after each test in the matrix.
- update coveralls step to upload partial data.

Converage job (new):
- complete the coveralls in the coverage job.
- add step to download all uploaded artifacts
- add step to combine coverage data, create report and fail if not
  enough
- add step to upload html report if coverage is not enough
@rudaporto rudaporto force-pushed the fix-tox-config-file-coverage-report branch from 338da05 to f3704f2 Compare October 17, 2022 07:05
@rudaporto
Copy link
Author

rudaporto commented Oct 17, 2022

FYI, the file .github/workflows/tests.yml is auto-generated and should not be edited by hand. It uses the meta/config package, see https://github.com/zopefoundation/meta/tree/master/config, which is configured using the .meta.toml file.

Thank you @dataflake for the reminder. Yes, I know that this should not be changed directly.
I've updated the description with a full explanation of what I tried to achieve.

I will convert this PR to a draft to avoid confusion.

@rudaporto rudaporto marked this pull request as draft October 17, 2022 07:12
@icemac
Copy link
Member

icemac commented Oct 26, 2022

I think this approach presented here is promising. It requires some changes in meta/config so it can produce the required tests.yaml.
Currently we are running coverage just for one Python version. In most projects this is enough because (after dropping Python 2 support) there is nearly no Python version specific code. So for the other projects I think it will be enough just to run coverage on a single Python version, but here we have different goals.

I currently do not have the time and energy to push meta/config forward to be able to support the kind of changes required here, sorry.

@icemac icemac mentioned this pull request Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants