Run unit tests only for plugins with changes #1838
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -13,6 +13,7 @@ on: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - '**/package.json' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - 'package-lock.json' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - 'phpunit.xml.dist' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - 'tools/phpunit/bootstrap.php' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - 'composer.json' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - 'composer.lock' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pull_request: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -24,6 +25,7 @@ on: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - '**/package.json' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - 'package-lock.json' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - 'phpunit.xml.dist' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - 'tools/phpunit/bootstrap.php' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - 'composer.json' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - 'composer.lock' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| types: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -61,6 +63,55 @@ jobs: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| steps: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - uses: styfle/cancel-workflow-action@85880fa0301c86cca9da44039ee3bb12d3bedbfa # 0.12.1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - name: Get changed files | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| id: changed-files | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uses: tj-actions/changed-files@e0021407031f5be11a464abee9a0776171c79891 # v47.0.1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| with: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| dir_names: true # Output unique changed directories. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| dir_names_max_depth: 2 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| files_yaml: | | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| plugins: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - 'plugins/**' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| config: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - '.github/workflows/php-test-plugins.yml' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - '.wp-env.json' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - '**/package.json' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - 'package-lock.json' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - 'phpunit.xml.dist' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - 'tools/phpunit/bootstrap.php' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - 'composer.json' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - 'composer.lock' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - name: Get changed plugins | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| id: changed-plugins | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| run: | | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if [[ "${{ github.event_name }}" == "push" || "${{ steps.changed-files.outputs.config_any_changed }}" == "true" ]]; then | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ALL_CHANGED_PLUGINS=($(ls plugins)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| echo "all_changed_plugins=${ALL_CHANGED_PLUGINS[*]}" >> $GITHUB_OUTPUT | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| exit 0 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fi | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+87
to
+91
There was a problem hiding this comment. The condition checks if github.event_name equals "push" but doesn't verify which branch was pushed to. According to the workflow trigger configuration, pushes can occur on both "trunk" and "release/**" branches. The description states "All tests will run for all plugins when commits are added to the trunk branch", but this condition will run all tests for any push, including release branches. Consider adding a branch check to ensure this behavior only applies to trunk pushes if that's the intended behavior. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| declare -a ALL_CHANGED_PLUGINS=() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for DIR in ${{ steps.changed-files.outputs.plugins_all_changed_files }}; do | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| PLUGIN_NAME=$(basename "$DIR") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ALL_CHANGED_PLUGINS+=("$PLUGIN_NAME") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| done | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Define and add plugin dependents (e.g., changes to Optimization Detective should trigger its dependents). | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| declare -A PLUGIN_DEPENDENTS=( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ["optimization-detective"]="embed-optimizer image-prioritizer" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for PLUGIN in "${ALL_CHANGED_PLUGINS[@]}"; do | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if [[ -n "${PLUGIN_DEPENDENTS[$PLUGIN]}" ]]; then | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for DEP in ${PLUGIN_DEPENDENTS[$PLUGIN]}; do | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if [[ ! " ${ALL_CHANGED_PLUGINS[*]} " =~ " ${DEP} " ]]; then | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ALL_CHANGED_PLUGINS+=("$DEP") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fi | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| done | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fi | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| done | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. The shell script doesn't handle the case where no plugins have changed. If ALL_CHANGED_PLUGINS is empty, the subsequent for loops in the test execution steps will fail silently or produce errors. Add a check to ensure at least one plugin needs testing, or exit early with a message if no plugins changed.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ALL_CHANGED_PLUGINS=($(echo "${ALL_CHANGED_PLUGINS[@]}" | tr ' ' '\n' | sort | tr '\n' ' ')) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| echo "all_changed_plugins=${ALL_CHANGED_PLUGINS[*]}" >> $GITHUB_OUTPUT | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - name: Setup Node.js (.nvmrc) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uses: actions/setup-node@395ad3262231945c25e8478fd5baf05154b1d79f # v6.1.0 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| with: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -93,50 +144,70 @@ jobs: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - name: Running single site unit tests | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| run: | | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| done | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+147
to
+149
There was a problem hiding this comment. The script uses unquoted variable expansion in the for loops (${{ steps.changed-plugins.outputs.all_changed_plugins }}), which could cause issues if plugin names contain spaces or special characters. While plugin names in this repository don't have spaces, it's a best practice to quote these expansions for robustness. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for PLUGIN in ${{ steps.changed-plugins.outputs.all_changed_plugins }}; do | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| npm run test-php:$PLUGIN | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| done | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fi | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - name: Running multisite unit tests | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| run: | | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| done | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for PLUGIN in ${{ steps.changed-plugins.outputs.all_changed_plugins }}; do | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| npm run test-php-multisite:$PLUGIN | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| done | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fi | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - name: Download Codecov CLI | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ShyamGadde marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+166
to
+178
There was a problem hiding this comment. The codecov CLI verification steps create a .gpg directory but don't clean it up afterward. This could leave sensitive keyring files on the runner. Add a cleanup step or use the system's default GPG directory instead. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - name: Upload coverage reports to Codecov | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if: ${{ matrix.coverage == true }} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| run: | | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Build common arguments for all uploads | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cc_args=() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cc_args+=(--fail-on-error) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cc_args+=(--disable-search) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cc_args+=(--git-service github) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cc_args+=(--gcov-executable "gcov") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Add SHA for PRs | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if [ -n "${{ github.event.pull_request.head.sha }}" ]; then | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cc_args+=(--sha "${{ github.event.pull_request.head.sha }}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fi | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Handle authentication differently for forks vs. internal PRs | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if [ -n "${{ github.event.pull_request.head.repo.full_name }}" ] && \ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| [ "${{ github.event.pull_request.head.repo.full_name }}" != "${{ github.repository }}" ]; then | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| echo "::notice::Processing PR from fork - using tokenless upload with branch override." | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cc_args+=(--branch "${{ github.event.pull_request.head.label }}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| echo "::notice::Processing internal PR or push - using token." | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cc_args+=(--token "${{ secrets.CODECOV_TOKEN }}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fi | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Upload reports for each changed plugin | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for PLUGIN in ${{ steps.changed-plugins.outputs.all_changed_plugins }}; do | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| echo "::group::Uploading coverage for plugin: $PLUGIN" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ./codecov upload-coverage "${cc_args[@]}" \ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| --file ./single-site-reports/coverage-$PLUGIN.xml \ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| --file ./multisite-reports/coverage-multisite-$PLUGIN.xml \ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| --flag $PLUGIN \ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| --name $PLUGIN-coverage | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+207
to
+211
There was a problem hiding this comment. The codecov upload command assumes that coverage files exist for all changed plugins, but if a test execution fails for any plugin, the corresponding coverage file won't be generated. This will cause the upload to fail with --fail-on-error. Consider checking if files exist before attempting to upload them, or handle missing files gracefully.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| echo "::endgroup::" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| done | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -8,8 +8,48 @@ coverage: | |||||
| target: auto | ||||||
| threshold: 80% | ||||||
| base: auto | ||||||
| branches: | ||||||
| - trunk | ||||||
| informational: true | ||||||
| patch: | ||||||
| default: | ||||||
| threshold: 20% | ||||||
|
There was a problem hiding this comment. The patch threshold was lowered from 80% to 20%, which is a significant decrease that could allow code with poor test coverage to be merged. This change should be documented in the PR description or reconsidered, as it represents a substantial reduction in code quality standards.
Suggested change
|
||||||
| informational: true | ||||||
| comment: | ||||||
| hide_comment_details: true | ||||||
| hide_project_coverage: false | ||||||
| show_carryforward_flags: true | ||||||
|
Contributor
Author
There was a problem hiding this comment. @westonruter Would you prefer that we display the carried forward flags in the PR comment, as shown below, or would you rather exclude them?
Member
There was a problem hiding this comment. I suppose it doesn't hurt to include? |
||||||
| 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/ | ||||||
| - plugins/web-worker-offloading/ | ||||||
| - name: view-transitions | ||||||
| paths: | ||||||
| - plugins/view-transitions/ | ||||||
| - name: web-worker-offloading | ||||||
| paths: | ||||||
| - name: webp-uploads | ||||||
| paths: | ||||||
| - plugins/webp-uploads/ | ||||||
westonruter marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
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>/testswill be returned asplugins/<plugin-name>.Since
dir_names: trueensures only unique directories are listed, combining it withdir_names_max_depth: 2allows us to extract a clean list of changed plugin names. This avoids additional logic in the next step for determining modified plugins.