-
Notifications
You must be signed in to change notification settings - Fork 98
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 @cover annotation in test-load file and add support for codecov #1586
base: trunk
Are you sure you want to change the base?
Add @cover annotation in test-load file and add support for codecov #1586
Conversation
@sarthak-19 Welcome, and thanks for opening this! I just stumbled upon the PR and left a couple of comments for things that caught my eye. |
@swissspidy Thank you so much for the feedback. I've addressed those and looking forward to keep improving 😄 |
@@ -68,6 +68,20 @@ jobs: | |||
- name: Composer Install | |||
run: npm run wp-env run tests-cli -- --env-cwd="wp-content/plugins/$(basename $(pwd))" composer install --no-interaction --no-progress | |||
- name: Running single site unit tests | |||
run: npm run test-php | |||
run: npm run test-php -- --coverage-clover=coverage.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 collecting code coverage takes much longer, as I recall, should the coverage only happen for one of the jobs instead of for all of them?
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.
Yes you're right, collecting code coverage takes time, but I'm not quite sure that for which job it'll be ideal to run.
Should I run for PHP v8.0
and if this is a viable option, then will the below implementation work since we are using the latest WP version anyways?
- name: Running single site unit tests with coverage
if: matrix.php == '8.0'
run: npm run test-php -- --coverage-clover=coverage.xml
- name: Running multisite unit tests with coverage
if: matrix.php == '8.0'
run: npm run test-php-multisite -- --coverage-clover=coverage-multisite.xml
- name: Upload single site coverage reports to Codecov
if: matrix.php == '8.0'
uses: codecov/codecov-action@v4
with:
token: ${{ secrets.CODECOV_TOKEN }}
files: coverage.xml
flags: unittests
name: ${{ matrix.php }}-single-site-coverage
- name: Upload multisite coverage reports to Codecov
if: matrix.php == '8.0'
uses: codecov/codecov-action@v2
with:
token: ${{ secrets.CODECOV_TOKEN }}
files: coverage-multisite.xml
flags: multisite
name: ${{ matrix.php }}-multisite-coverage
cc : @westonruter
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.
You may want to refer to what was done for the AMP plugin: https://github.com/ampproject/amp-wp/blob/60a655cdad9e3431b47b2dcc3b2e5ddb86ac5c5e/.github/workflows/build-test-measure.yml#L261-L337
Note the new coverage
matrix variable which is only set to true
for one configuration. This would be better than having to test against a specific PHP version everywhere. Yes, I think running coverage on the most recent PHP version would be best as it should complete the fastest. So maybe do it in PHP 8.2?
cc @thelovekesh
I am really glad to see I have been try to get into core ten+ years |
Summary
This PR adds @cover annotation for test cases and add .yml file that can be used to configure codecov for automated test coverage in github actions.
Addresses #1284
Relevant technical choices
I've addressed the following file
plugins/webp-uploads/tests/test-load.php
for adding @cover annotation, if the approach is approved will extend this to other files.