Skip to content

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 requested review from loechel and ale-rt October 15, 2022 13:46
@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
@icemac icemac removed their request for review April 3, 2025 07:26
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