-
Notifications
You must be signed in to change notification settings - Fork 109
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
Run unit tests only for plugins with changes #1838
base: trunk
Are you sure you want to change the base?
Run unit tests only for plugins with changes #1838
Conversation
Signed-off-by: Shyamsundar Gadde <[email protected]>
Signed-off-by: Shyamsundar Gadde <[email protected]>
Signed-off-by: Shyamsundar Gadde <[email protected]>
Signed-off-by: Shyamsundar Gadde <[email protected]>
Signed-off-by: Shyamsundar Gadde <[email protected]>
Signed-off-by: Shyamsundar Gadde <[email protected]>
Signed-off-by: Shyamsundar Gadde <[email protected]>
Signed-off-by: Shyamsundar Gadde <[email protected]>
Signed-off-by: Shyamsundar Gadde <[email protected]>
Signed-off-by: Shyamsundar Gadde <[email protected]>
Signed-off-by: Shyamsundar Gadde <[email protected]>
Signed-off-by: Shyamsundar Gadde <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## trunk #1838 +/- ##
=======================================
Coverage 65.98% 65.98%
=======================================
Files 88 88
Lines 6896 6896
=======================================
Hits 4550 4550
Misses 2346 2346
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
uses: tj-actions/changed-files@v45 | ||
with: | ||
dir_names: true # Output unique changed directories. | ||
dir_names_max_depth: 2 |
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.
This limits the directory output to a maximum depth of 2. For example, plugins/<plugin-name>/tests
will be returned as plugins/<plugin-name>
.
Since dir_names: true
ensures only unique directories are listed, combining it with dir_names_max_depth: 2
allows us to extract a clean list of changed plugin names. This avoids additional logic in the next step for determining modified plugins.
# Define and add plugin dependencies (e.g., optimization-detective triggers others). | ||
declare -A PLUGIN_DEPENDENCIES=( | ||
["optimization-detective"]="embed-optimizer image-prioritizer" | ||
) | ||
for PLUGIN in "${ALL_CHANGED_PLUGINS[@]}"; do | ||
if [[ -n "${PLUGIN_DEPENDENCIES[$PLUGIN]}" ]]; then | ||
for DEP in ${PLUGIN_DEPENDENCIES[$PLUGIN]}; do | ||
if [[ ! " ${ALL_CHANGED_PLUGINS[@]} " =~ " ${DEP} " ]]; then | ||
ALL_CHANGED_PLUGINS+=("$DEP") | ||
fi | ||
done | ||
fi | ||
done |
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.
Currently, optimization-detective
is the only plugin with dependencies, so we could simplify this by checking for it explicitly instead of maintaining a general dependency structure. However, I think this approach keeps things flexible if more dependencies are introduced in the future.
config: | ||
- '.github/workflows/php-test-plugins.yml' | ||
- '.wp-env.json' | ||
- '**/package.json' | ||
- 'package-lock.json' | ||
- 'phpunit.xml.dist' | ||
- 'composer.json' | ||
- 'composer.lock' |
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.
Should we add tools/phpunit/bootstrap.php
here along with phpunit.xml.dist
?
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, that makes sense to me. If the plugin bootstrap file changes, we should re-run tests. But what does this files_yaml
do here?
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.
Looks like it results in populating steps.changed-files.outputs
. So yeah, if the plugin bootstrap file changes than all tests should run as steps.changed-files.outputs.config_any_changed
is used below.
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.
Exactly. files_yaml
allows defining file and directory patterns in YAML to detect changes. The action then prefixes outputs with the specified key (e.g., in steps.changed-files.outputs.config_any_changed
, the any_changed
output is prefixed with config_
), simplifying condition checks downstream.
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
for PLUGIN in "${ALL_CHANGED_PLUGINS[@]}"; do | ||
if [[ -n "${PLUGIN_DEPENDENCIES[$PLUGIN]}" ]]; then | ||
for DEP in ${PLUGIN_DEPENDENCIES[$PLUGIN]}; do | ||
if [[ ! " ${ALL_CHANGED_PLUGINS[@]} " =~ " ${DEP} " ]]; then |
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.
PhpStorm (via Shellcheck) is flagging this line:
Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
See SC2199.
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.
Fixed it in 11949ac.
Signed-off-by: Shyamsundar Gadde <[email protected]>
Signed-off-by: Shyamsundar Gadde <[email protected]>
Signed-off-by: Shyamsundar Gadde <[email protected]>
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.
@ShyamGadde This looks great to me so far, just one minor comment.
Before we merge this, I would like to discuss though under which conditions we should do this. To me, it makes sense to only run selected unit tests for PRs, to speed up the process. But running the whole test suite also has value, e.g. to warn us early in case something breaks somewhere else where we didn't change any files - which can happen e.g. based on WordPress Core updates.
How about we implement this change so that it only applies to pull requests, but for merges to our main branches it would still run all test suites? IMO this would be the best of both worlds:
- Running tests against PRs would be more efficient, faster and cost less resources.
- Running tests against code merged into
trunk
and release branches would still be ensured to be more comprehensively tested.
It probably wouldn't be a huge lift to change that, we could make the step that determines the changed files and plugins conditional to pull_request
events and otherwise return the list of all plugins.
WDYT? cc @westonruter @thelovekesh
done | ||
|
||
# Define and add plugin dependencies (e.g., optimization-detective triggers others). | ||
declare -A PLUGIN_DEPENDENCIES=( |
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.
Maybe call this PLUGIN_DEPENDENTS
to clarify? In this case, Optimization Detective is a dependency of the other two, but the other two are dependents of Optimization Detective.
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.
That makes sense. Updated it in 90a5ab7.
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.
Marking this as "Request changes" until we resolve the outstanding discussion points on the approach in #1792.
This should already be the case with this logic: performance/.github/workflows/php-test-plugins.yml Lines 83 to 87 in af6f8ff
If any of the config files are changed or if the changes are not part of a PR (i.e. a push to |
@westonruter Thanks, I missed that. LGTM in that regard then 👍 In that case, this leaves the outstanding point @swissspidy raised about confusing codecov reports. |
I think a good solution to this would be the Another possible approach is to only show patch coverage in the PR comment, as that isn’t affected by partial testing. This would be the simpler solution, ensuring that PR reports remain clear and focused on the actual changes. Since full test runs still occur on merge commits and commits to That said, the first approach (using Screenshots: |
Signed-off-by: Shyamsundar Gadde <[email protected]>
Signed-off-by: Shyamsundar Gadde <[email protected]>
Ref: https://docs.codecov.com/docs/commit-status#base Signed-off-by: Shyamsundar Gadde <[email protected]>
So, after experimenting with Codecov's coverage reporting and reviewing its documentation, I’ve concluded that if we want to continue with partial testing, there are two possible ways to handle Codecov’s potentially confusing reports—depending on whether we want to display the project's overall coverage in the PR comment or not. Option 1: Show Project Coverage in PR CommentIf we want to display project coverage in the PR comment, we need to use the To address this, we can:
That way when we don't test a particular plugin the result will look something like in the below screenshot and even the project coverage which is displayed won't be affected when partial testing. I personally find the reports (for both single and multisite) grouped by plugin flags more intuitive to read (because of how Codecov merges these reports) than the old reporting format, where we see For this to work, we'll have to change the workflow and Codecov's config file somewhat as follows:
diff --git a/.github/workflows/php-test-plugins.yml b/.github/workflows/php-test-plugins.yml
index 15e2101d..9211255f 100644
--- a/.github/workflows/php-test-plugins.yml
+++ b/.github/workflows/php-test-plugins.yml
@@ -128,6 +128,19 @@ jobs:
run: npm run wp-env run tests-cli -- --env-cwd="wp-content/plugins/$(basename $(pwd))" composer install --no-interaction --no-progress
- name: Update Composer Dependencies
run: composer update --with-all-dependencies --no-interaction --no-progress
+ - name: Download Codecov CLI
+ if: ${{ matrix.coverage == true }}
+ run: |
+ # Create a temporary directory for GPG operations
+ mkdir -p .gpg
+
+ curl https://keybase.io/codecovsecurity/pgp_keys.asc | gpg --homedir .gpg --no-default-keyring --keyring trustedkeys.gpg --import
+ curl -Os https://cli.codecov.io/latest/linux/codecov
+ curl -Os https://cli.codecov.io/latest/linux/codecov.SHA256SUM
+ curl -Os https://cli.codecov.io/latest/linux/codecov.SHA256SUM.sig
+ gpg --homedir .gpg --no-default-keyring --keyring trustedkeys.gpg --verify codecov.SHA256SUM.sig codecov.SHA256SUM
+ shasum -a 256 -c codecov.SHA256SUM
+ chmod +x codecov
- name: Install PHPUnit
run: |
if [ "${{ matrix.php }}" == "8.2" ]; then
@@ -141,6 +154,13 @@ jobs:
if [ "${{ matrix.coverage }}" == "true" ]; then
for PLUGIN in ${{ steps.changed-plugins.outputs.all_changed_plugins }}; do
npm run test-php:$PLUGIN -- -- -- --coverage-clover=./single-site-reports/coverage-$PLUGIN.xml
+
+ # Upload coverage report to Codecov.
+ ./codecov --verbose upload-process --disable-search --fail-on-error \
+ -t ${{ secrets.CODECOV_TOKEN }} \
+ -n ${{ matrix.php }}-$PLUGIN-single-site-coverage \
+ -F $PLUGIN \
+ -f ./single-site-reports/coverage-$PLUGIN.xml
done
else
for PLUGIN in ${{ steps.changed-plugins.outputs.all_changed_plugins }}; do
@@ -152,27 +172,16 @@ jobs:
if [ "${{ matrix.coverage }}" == "true" ]; then
for PLUGIN in ${{ steps.changed-plugins.outputs.all_changed_plugins }}; do
npm run test-php-multisite:$PLUGIN -- -- -- --coverage-clover=./multisite-reports/coverage-multisite-$PLUGIN.xml
+
+ # Upload coverage report to Codecov.
+ ./codecov --verbose upload-process --disable-search --fail-on-error \
+ -t ${{ secrets.CODECOV_TOKEN }} \
+ -n ${{ matrix.php }}-$PLUGIN-multisite-coverage \
+ -F $PLUGIN \
+ -f ./multisite-reports/coverage-multisite-$PLUGIN.xml
done
else
for PLUGIN in ${{ steps.changed-plugins.outputs.all_changed_plugins }}; do
npm run test-php-multisite:$PLUGIN
done
fi
- - name: Upload single site coverage reports to Codecov
- if: ${{ matrix.coverage == true }}
- uses: codecov/codecov-action@v5
- with:
- token: ${{ secrets.CODECOV_TOKEN }}
- directory: ./single-site-reports
- flags: single
- name: ${{ matrix.php }}-single-site-coverage
- fail_ci_if_error: true
- - name: Upload multisite coverage reports to Codecov
- if: ${{ matrix.coverage == true }}
- uses: codecov/codecov-action@v5
- with:
- token: ${{ secrets.CODECOV_TOKEN }}
- directory: ./multisite-reports
- flags: multisite
- name: ${{ matrix.php }}-multisite-coverage
- fail_ci_if_error: true
diff --git a/codecov.yml b/codecov.yml
index f91d5654..83035cea 100644
--- a/codecov.yml
+++ b/codecov.yml
@@ -12,3 +12,38 @@ coverage:
default:
threshold: 80%
informational: true
+comment:
+ hide_comment_details: true
+ hide_project_coverage: false
+ show_carryforward_flags: true
+flag_management:
+ default_rules:
+ carryforward: true
+ individual_flags:
+ - name: auto-sizes
+ paths:
+ - plugins/auto-sizes/
+ - name: dominant-color-images
+ paths:
+ - plugins/dominant-color-images/
+ - name: embed-optimizer
+ paths:
+ - plugins/embed-optimizer/
+ - name: image-prioritizer
+ paths:
+ - plugins/image-prioritizer/
+ - name: optimization-detective
+ paths:
+ - plugins/optimization-detective/
+ - name: performance-lab
+ paths:
+ - plugins/performance-lab/
+ - name: speculation-rules
+ paths:
+ - plugins/speculation-rules/
+ - name: web-worker-offloading
+ paths:
+ - plugins/web-worker-offloading/
+ - name: webp-uploads
+ paths:
+ - plugins/webp-uploads/ Option 2: Show Only Patch Coverage in PR CommentA simpler alternative is to only show patch coverage in PR comments. This won't show project-wide coverage fluctuations when running partial tests and keeps the report focused on the actual changes. Additionally, if we were to only show "Patch coverage", we could:
With this approach, the PR comment will look like this: Given these options, assuming we still want to do partial testing, which approach would be preferable? |
@ShyamGadde Option 2 sounds like the better approach to me! Thank you for doing that research. |
Summary
Fixes #1792
Relevant technical choices
trunk
branch.