Try to implement coverage action instead of using codecov. #1756
Conversation
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: coverage-${{ matrix.python-version }}-${{ matrix.os }}-${{ matrix.sphinx-version }} | ||
| path: .coverage.${{ matrix.python-version }}.${{ matrix.os }}.${{ matrix.sphinx-version }} |
There was a problem hiding this comment.
We need to the names to be unique for collection of all the results.
There was a problem hiding this comment.
Right now are only really looking at the coverage for Python 3.12 but I prefer collecting across this.
| runs-on: ubuntu-latest | ||
| needs: run-pytest | ||
| # run both on previous step success and failure | ||
| if: "!cancelled()" |
There was a problem hiding this comment.
I'm assuming we want coverage even if tests are failing, but not if workflow is cancelled.
There was a problem hiding this comment.
This makes absolute sense as we have the concurrency setting enabled
|
Thanks @Carreau I will check later today |
There was a problem hiding this comment.
I quite like this PR and the addition of a coverage comment to our PRs.
This still keeps the upload to codecov step but if #1759 gets merged then that completely removes this and would make a nice pairing of this PR comment + uploading to GH artefacts.
I will approve on my end and leave it to the rest of the maintainers to raise any potential objections/hesitations to get this merged.
| runs-on: ubuntu-latest | ||
| needs: run-pytest | ||
| # run both on previous step success and failure | ||
| if: "!cancelled()" |
There was a problem hiding this comment.
This makes absolute sense as we have the concurrency setting enabled
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: coverage-${{ matrix.python-version }}-${{ matrix.os }}-${{ matrix.sphinx-version }} | ||
| path: .coverage.${{ matrix.python-version }}.${{ matrix.os }}.${{ matrix.sphinx-version }} |
There was a problem hiding this comment.
Right now are only really looking at the coverage for Python 3.12 but I prefer collecting across this.
trallard
added
kind: enhancement
|
Small question here, is this tool still generating a dashboard ? to me that's the main functionality of codecov, the interactivity is great and it's super easy to spot what is wrong. If we really want to drop codecov (which seems to be the consensus) I'm using this for creating dashboard artifact in Azure pipelines I guess that could be used here as well: https://github.com/danielpalme/ReportGenerator |
|
No this does not cover the report this only adds the pr comment. |
|
Any specific reason why you want to add this information in PRs then ? because the only instruction we are giving to codecov currently is to not send report to PR: https://github.com/pydata/pydata-sphinx-theme/blob/main/codecov.yml |
|
for me at least, seeing coverage info in the PR comment tells me whether or not I should pester the contributor to add/modify a test (or add one myself). So it's useful. |
|
I can look into not having a comment but having a status check that is red only of coverage decreases. Maybe that would lower the notification noise, but without removing the notification of needing more coverage ? |
|
I am +1 with @drammock having coverage info is useful on PRs. I am not sure if one of the reasons why we do not have codecov PR messages right now has to do with the flakiness we are experiencing, but also I am not too worried about this PR adding too much noise as is. |
|
I just wanted to point out the current status, if you are fine receiving notification I'm fine as well (I never silence them myself) |
|
Since we all seem to be +1 on trying this codec implementation, the PR has been reviewed. I suggest we merge, see how this goes and make changes/tweaks as needed. |
Co-authored-by: Tania Allard <[email protected]>
* Try to implement coverage action instead of using codecov. See pydata#1753 * Update .github/workflows/tests.yml * Apply suggestions from code review Co-authored-by: Tania Allard <[email protected]> --------- Co-authored-by: Tania Allard <[email protected]> Co-authored-by: Daniel McCloy <[email protected]>
See #1753