-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Upload junit reports codecov #13778
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
base: main
Are you sure you want to change the base?
Upload junit reports codecov #13778
Conversation
webknjaz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a great start! I normally avoid adding --junitxml= (even if the path value is empty) to normal/local invocations. Instead, I like adding the entire CLI flag in the CI definition: https://github.com/cherrypy/cheroot/blob/06a6d67e549ccaefe332cd0539c21e1483031071/.github/workflows/ci-cd.yml#L786.
I've made suggestions below. Let's see if that works..
As for the change note, sometimes it's a good idea to use the contrib type for such changes.
|
Do you recommend doing this because it is cleaner to have the entire CLI flag in one place instead of splitting it across the workflow and tox.ini file? If so that makes perfect sense (I'm asking for my personal learning) @webknjaz |
pass entire CLI flag in workflow Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
for more information, see https://pre-commit.ci
3e622d3 to
6af094b
Compare
| @@ -0,0 +1 @@ | |||
| automated uploading JUnit reports to codecov in the test workflow with codecov GitHub Action | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change notes are supposed to communicate the effect of upgrading since the previous release. They make up a coherent change log document, which is expected to follow consistent writing style and use full sentences and such. They shouldn't generally reference internal implementation details and be higher level and approachable to the end-users.
This is a contrib note, though, so the "end users" are other contributors (and core devs).
Here's a preview of your addition: https://pytest--13778.org.readthedocs.build/en/13778/changelog.html#contributor-facing-changes.
To me, it looks a bit out of place because it doesn't have a full stop in the sentence, nor does the sentence start with a capital letter.
This will probably fit better:
| automated uploading JUnit reports to codecov in the test workflow with codecov GitHub Action | |
| The test reports are now published to Codecov from GitHub Actions. | |
| The test statistics is visible `on the web interface | |
| <https://app.codecov.io/gh/pytest-dev/pytest/tests>`__. | |
| -- by :user:`aleguy02` |
P.S. Technically, this PR does not require a change note but since you added it, let's make it more coherent with the rest.
| if: "! matrix.use_coverage" | ||
| shell: bash | ||
| env: | ||
| _PYTEST_TOX_POSARGS_JUNIT: --junitxml=junit-${{ matrix.name }}.xml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this produces one file per job that doesn't go anywhere with this name, we can skip making the name dynamic.
| _PYTEST_TOX_POSARGS_JUNIT: --junitxml=junit-${{ matrix.name }}.xml | |
| _PYTEST_TOX_POSARGS_JUNIT: --junitxml=junit.xml |
| if: "matrix.use_coverage" | ||
| shell: bash | ||
| env: | ||
| _PYTEST_TOX_POSARGS_JUNIT: --junitxml=junit-${{ matrix.name }}.xml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| _PYTEST_TOX_POSARGS_JUNIT: --junitxml=junit-${{ matrix.name }}.xml | |
| _PYTEST_TOX_POSARGS_JUNIT: --junitxml=junit.xml |
| uses: codecov/codecov-action@5a1091511ad55cbe89839c7260b706298ca349f7 | ||
| with: | ||
| fail_ci_if_error: false | ||
| files: junit-${{ matrix.name }}.xml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| files: junit-${{ matrix.name }}.xml | |
| files: junit.xml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency with the coverage upload step done above this one, let's use ./junit.xml.
Perhaps. But this is more due to the fact that configuration should be where it belongs. If you add a |
nicoddemus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description
This PR automates uploading JUnit reports to codecov in the test pipeline. An example pipeline run with the changes can be found here (https://github.com/aleguy02/fork-pytest/actions/runs/18184970357). An example of what the codecov UI looks like with these changes can be found here (https://app.codecov.io/gh/aleguy02/fork-pytest/tests).
Closes #12689
Checklist
Additional Notes
I didn't create a changelog since the documented behavior of pytest was not modified.