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

add option to generate test coverage report without codecov #207

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

zacharyburnett
Copy link
Contributor

@zacharyburnett zacharyburnett commented Jun 9, 2024

using the guide here: https://hynek.me/articles/ditch-codecov-python/

This does not create coverage diffs or report to PRs, just creates a markdown summary table

closes #189

EDIT: here's what the summary table looks like:
image

@Cadair
Copy link
Member

Cadair commented Jun 10, 2024

I tried this out on sunpy and it seems to explode. sunpy/sunpy#7668

@Cadair
Copy link
Member

Cadair commented Jun 10, 2024

It seems that it's not picking up the coverage.xml to upload here: https://github.com/sunpy/sunpy/actions/runs/9445798095/job/26014155532?pr=7668#step:14:16 🤔

Copy link
Member

@Cadair Cadair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for taking a swing at this!

.github/workflows/tox.yml Outdated Show resolved Hide resolved
.github/workflows/tox.yml Outdated Show resolved Hide resolved
- run: python -Im pip install --upgrade coverage[toml]
- run: python -Im coverage combine
- run: python -Im coverage report --format=markdown >> $GITHUB_STEP_SUMMARY

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be nice to add the ability to fail this job based on either total coverage % or diff coverage %.

@pllim
Copy link
Contributor

pllim commented Jun 10, 2024

tl;dr -- this does not generate patch coverage, right?

@Cadair
Copy link
Member

Cadair commented Jun 10, 2024

Not yet, but I found a way to make it.

@zacharyburnett
Copy link
Contributor Author

tl;dr -- this does not generate patch coverage, right?

Not yet, but I found a way to make it.

For patch coverage we'd need to store the coverage from main somewhere, to compare it to; some people I've seen online have done that by storing it in a hidden page in the repository's wiki, reading and writing it with a github token with ${{ secrets.GITHUB_TOKEN }}

tools/tox_matrix.py Outdated Show resolved Hide resolved
@pllim
Copy link
Contributor

pllim commented Jun 11, 2024

@zacharyburnett
Copy link
Contributor Author

I am trying this out too at spacetelescope/acstools#205 and it still crashes:

https://github.com/spacetelescope/acstools/actions/runs/9463279566/job/26068438067?pr=205

oh whoops, I fixed the artifact issue:
image
but didn't pass any filenames to coverage combine 😅
image

@pllim

This comment was marked as resolved.

@pllim

This comment was marked as outdated.

@pllim

This comment was marked as outdated.

@pllim
Copy link
Contributor

pllim commented Jun 12, 2024

Even when CI passes, it failed with the same error.

Coverage XML written to file coverage.xml

mv: cannot stat '.coverage': No such file or directory

@pllim pllim mentioned this pull request Jun 12, 2024
@zacharyburnett
Copy link
Contributor Author

Even when CI passes, it failed with the same error.

Coverage XML written to file coverage.xml

mv: cannot stat '.coverage': No such file or directory

it should be uploading / downloading the .coverage database, to be able to combine multiple machine types, so if that's not present then something is messed up

@zacharyburnett
Copy link
Contributor Author

zacharyburnett commented Jun 12, 2024

it looks like --cov-report=xml is what's creating the coverage.xml; however, it should still be creating a .coverage database file... I'll so some local testing to see what files it spits out
https://github.com/spacetelescope/acstools/actions/runs/9463279566/job/26118526244#step:10:105

pytest --pyargs acstools --cov-report=xml --cov=acstools /home/runner/work/acstools/acstools/doc --cov --remote-data -v

@zacharyburnett
Copy link
Contributor Author

running pytest --pyargs acstools --cov-report=xml --cov=acstools --cov --remote-data -v locally does appear to create the .coverage file:

❯ ls -a
.							CITATION.md
..							CODE_OF_CONDUCT.md
.bandit.yaml						LICENSE.md
.coverage						MANIFEST.in
.coverage.urelialia2022.local.stsci.edu.35749.XCMKciAx	README.rst
.git							__pycache__
.github							acstools
.gitignore						acstools.egg-info
.mypy_cache						conftest.py
.pytest_cache						coverage.xml
.readthedocs.yaml					doc
.tmp							pyproject.toml
.tox							tox.ini

@pllim
Copy link
Contributor

pllim commented Jun 12, 2024

What is the difference between .coverage and coverage.xml?

@zacharyburnett
Copy link
Contributor Author

zacharyburnett commented Jun 12, 2024

What is the difference between .coverage and coverage.xml?

I'm not intimately familiar with it, but as far as I know .coverage is a SQLite database file with support for parallel-mode (storing coverage for multiple machine configurations / runs) while coverage.xml is report-focused and so is expected to only contain results from a single run.

We could use coverage.xml, but then we'd need to have a summary table of coverage for each and every run, instead of a single table collating all runs

@pllim
Copy link
Contributor

pllim commented Jun 12, 2024

Tried to debug at #209 but no luck. I see the coverage ran but the the upload failed: https://github.com/spacetelescope/acstools/actions/runs/9483014271/job/26134042811?pr=205

@zacharyburnett
Copy link
Contributor Author

Tried to debug at #209 but no luck. I see the coverage ran but the the upload failed: https://github.com/spacetelescope/acstools/actions/runs/9483014271/job/26134042811?pr=205

could you also add -a to the ls?

@pllim
Copy link
Contributor

pllim commented Jun 12, 2024

@pllim
Copy link
Contributor

pllim commented Jun 25, 2024

BTW I think this fix #189 but only a maintainer can add that to the original post for auto-close to take effect. cc @Cadair

@pllim
Copy link
Contributor

pllim commented Jun 25, 2024

@zacharyburnett is this ready for re-review or you still working on it? Thanks!

@zacharyburnett
Copy link
Contributor Author

zacharyburnett commented Jun 25, 2024

@zacharyburnett is this ready for re-review or you still working on it? Thanks!

This is ready now, thanks!

BTW I think this fix #189 but only a maintainer can add that to the original post for auto-close to take effect. cc @Cadair

Oh I didn't even see that 😅

@pllim
Copy link
Contributor

pllim commented Jun 25, 2024

Alas, still cannot get this to work with acstools. What am I doing wrong there?

spacetelescope/acstools#205

https://github.com/spacetelescope/acstools/actions/runs/9669917380/job/26677517727?pr=205

@zacharyburnett
Copy link
Contributor Author

Alas, still cannot get this to work with acstools. What am I doing wrong there?

spacetelescope/acstools#205

https://github.com/spacetelescope/acstools/actions/runs/9669917380/job/26677517727?pr=205

I'm not sure, this seems to be correct but the .coverage file isn't being created (only coverage.xml)

@pllim
Copy link
Contributor

pllim commented Jun 26, 2024

A lot of CI only outputs coverage.xml. I think this case also has to be handled.

@zacharyburnett
Copy link
Contributor Author

zacharyburnett commented Jun 26, 2024

A lot of CI only outputs coverage.xml. I think this case also has to be handled.

It was my belief that coverage always creates a .coverage file and uses this as the base data file, and coverage.xml is an additional report file generated with coverage xml or --cov-report=xml; however, I guess that's wrong

EDIT: wait a second, I just cloned acstools and ran tox -e py312-test-alldeps -- --cov on my machine, and it created the .coverage file along with coverage.xml. Now I am very confused why it creates it locally but not on GitHub Actions... 😓

@zacharyburnett
Copy link
Contributor Author

@pllim would you mind rerunning that job now that 1f4a851 is pushed? It should pick it up

@pllim
Copy link
Contributor

pllim commented Jun 26, 2024

@pllim
Copy link
Contributor

pllim commented Jun 26, 2024

Maybe the coverage stuff is inside .tox

@zacharyburnett
Copy link
Contributor Author

zacharyburnett commented Jun 26, 2024

Maybe the coverage stuff is inside .tox

ah! You're using pyargs; perhaps that is causing this issue by moving the working dir to somewhere else

that might explain why it worked locally on my machine; perhaps there's some shell shenanigans going on with github actions

@pllim
Copy link
Contributor

pllim commented Jun 26, 2024

astropy also uses pyargs, so it is not uncommon. See https://github.com/astropy/astropy/blob/5519605a3b6ec71e9ddb4c6827b8ac1658647f58/tox.ini#L121-L122

@zacharyburnett
Copy link
Contributor Author

looks like that fixed it https://github.com/spacetelescope/acstools/actions/runs/9683845227/attempts/1#summary-26720539461 thanks for being so patient! 😄

@zacharyburnett
Copy link
Contributor Author

zacharyburnett commented Jun 26, 2024

the failing test_conda test on macOS is fixed by #123

@pllim
Copy link
Contributor

pllim commented Jun 26, 2024

Awesome, thanks! Now that the simple case of acstools works, I plan to open an experimental PR to astropy with this PR branch, or feel free to do it if you have time now as I cannot until later today.

@zacharyburnett
Copy link
Contributor Author

Awesome, thanks! Now that the simple case of acstools works, I plan to open an experimental PR to astropy with this PR branch, or feel free to do it if you have time now as I cannot until later today.

sure thing, here: astropy/astropy#16619

I don't have permissions to label that PR though

@pllim
Copy link
Contributor

pllim commented Jun 28, 2024

@astrofrog , do you want to take this for a spin on your repos?

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.

Add support for doing code coverage reporting entirely on GitHub
3 participants