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 support for doing code coverage reporting entirely on GitHub #189

Open
Cadair opened this issue Mar 6, 2024 · 12 comments · May be fixed by #207
Open

Add support for doing code coverage reporting entirely on GitHub #189

Cadair opened this issue Mar 6, 2024 · 12 comments · May be fixed by #207
Labels
enhancement New feature or request

Comments

@Cadair
Copy link
Member

Cadair commented Mar 6, 2024

It seems not that complex to upload a coverage report from each job and then combine them in an extra job: https://hynek.me/articles/ditch-codecov-python/ I am finding codecov to be more and more annoying so it might be worth having a second option?

@Cadair Cadair added the enhancement New feature or request label Mar 6, 2024
@mhvk
Copy link

mhvk commented May 29, 2024

Yes, please!!!

@Cadair
Copy link
Member Author

Cadair commented Jun 3, 2024

It's unlikely I am getting to this anytime soon if anyone else wants to take a run at it.

@pllim
Copy link
Contributor

pllim commented Jun 8, 2024

Looks like pydata-sphinx-theme did the hard work here (uses https://github.com/py-cov-action/python-coverage-comment-action):

Though, @weaverba137 also said Coveralls still work (at least for DESI), if that is preferable.

I don't quite remember now why we switched from Coveralls to Codecov in the first place. (Something was broken?)

@pllim
Copy link
Contributor

pllim commented Jun 8, 2024

That said, given how much my attempt at #199 broke other repos running CI in different ways, maybe it is better for astropy to also ditch the reusable workflow here for CI and only keep it for wheels. That way, we only have to worry about ditching codecov in a way that works for us.

@astrofrog
Copy link
Contributor

Why don't we just try and figure it out in the reusable workflows then we would get it to work in all coordinated packages etc too for free?

@pllim
Copy link
Contributor

pllim commented Jun 10, 2024

It would be a breaking change. I see very specific codecov settings.

@Cadair
Copy link
Member Author

Cadair commented Jun 10, 2024

There's a generic coverage key: https://github-actions-workflows.openastronomy.org/en/latest/tox.html#coverage so we can add a coverage: github which people can opt-in to. I don't think this has to be a breaking change.

@Cadair
Copy link
Member Author

Cadair commented Jun 10, 2024

From the coverage.py docs I just found this: https://github.com/Bachmann1234/diff_cover we could use that to fail the job based on the patch coverage rather than the whole project.

@zacharyburnett
Copy link
Contributor

zacharyburnett commented Jun 25, 2024

here's a usage of diff-cover (assuming the coverage files are uploaded as artifacts earlier in the run)
https://github.com/spacetelescope/stpipe/pull/160/files#r1653419802

  report_diff_coverage:
     needs: [ test ]
     if: github.event_name == 'pull_request'
     runs-on: ubuntu-latest
     steps:
       - uses: actions/setup-python@v5
       - run: pip install diff-cover coverage
       - uses: actions/checkout@v4
         with:
           fetch-depth: 0
       - run: pip install .
       - uses: actions/download-artifact@v4
         with:
           pattern: ".coverage*"
           merge-multiple: true
       - run: coverage xml
       - run: diff-cover coverage.xml --compare-branch origin/${{ github.base_ref }} --markdown-report $GITHUB_STEP_SUMMARY

@zacharyburnett
Copy link
Contributor

made it even better by having it comment the report on the PR and update that comment with every commit. However, it requires using pull_request_target, so the workflow has to already be on main

  report_diff_coverage:
     needs: [ test ]
     if: github.event_name == 'pull_request_target'
     runs-on: ubuntu-latest
     steps:
       - uses: actions/setup-python@v5
         with:
           python-version: 3
       - run: pip install diff-cover coverage
       - uses: actions/checkout@v4
         with:
           fetch-depth: 0
       - run: pip install .
       - uses: actions/download-artifact@v4
         with:
           pattern: ".coverage*"
           merge-multiple: true
       - run: coverage xml
       - run: diff-cover coverage.xml --compare-branch origin/${{ github.base_ref }} --markdown-report diff_coverage.md
       - run: cat diff_coverage.md >> $GITHUB_STEP_SUMMARY
       - uses: thollander/[email protected]
         with:
           filePath: diff_coverage.md
           pr_number: ${{ github.event.number }}
           comment_tag: diff-coverage

@pllim
Copy link
Contributor

pllim commented Jun 26, 2024

Re: pull_request_target -- I can always test on my astropy fork instead of astropy proper in that case.

tl;dr -- Does it report patch coverage, or just project?

@pllim
Copy link
Contributor

pllim commented Jun 26, 2024

Actually, I have concern... Please see https://github.com/astropy/astropy/pull/16619/files#r1655466104

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants