Try: Build once for PHP Unit. - #12649
Closed
peterwilsoncc wants to merge 26 commits into
Closed
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| persist-credentials: false | ||
|
|
||
| - name: Set up Node.js | ||
| uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 |
There was a problem hiding this comment.
Pull request overview
This PR updates the PHPUnit GitHub Actions workflow to build WordPress once, upload the build as an artifact, and let the matrix test jobs reuse that artifact to avoid repeated builds.
Changes:
- Adds a dedicated
build-wordpressjob that builds and uploads awordpress-buildartifact. - Extends the reusable PHPUnit workflow with an input to optionally download/unzip the build artifact.
- Wires the PHPUnit matrix jobs to depend on the build job and (in most cases) enable artifact reuse.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| .github/workflows/reusable-phpunit-tests-v3.yml | Adds an input and conditional logic to download/unzip a prebuilt WordPress artifact instead of rebuilding. |
| .github/workflows/phpunit-tests.yml | Adds a build job that uploads a build artifact and updates test jobs to depend on it and enable reuse. |
Comments suppressed due to low confidence (2)
.github/workflows/reusable-phpunit-tests-v3.yml:159
- Skipping
Set up PHPwhen the build artifact is available breaks the subsequent Composer install (and can leavevendor/bin/phpunitmissing for the test runs). The build artifact only addresses built JS/CSS; the PHP/Composer toolchain is still required per test job.
- name: Set up PHP
if: ${{ ! inputs.wordpress-build-artifact-available }}
uses: shivammathur/setup-php@f3e473d116dcccaddc5834248c87452386958240 # 2.37.2
.github/workflows/reusable-phpunit-tests-v3.yml:168
- With
wordpress-build-artifact-availableenabled, this skips installing Composer dependencies, but the test commands invoke./vendor/bin/phpunit. Unlessvendor/is guaranteed to be present in the workspace, this will fail.
# Since Composer dependencies are installed using `composer update` and no lock file is in version control,
# passing a custom cache suffix ensures that the cache is flushed at least once per week.
- name: Install Composer dependencies
if: ${{ ! inputs.wordpress-build-artifact-available }}
uses: ramsey/composer-install@65e4f84970763564f46a70b8a54b90d033b3bdda # 4.0.0
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
137
to
139
| - name: Checkout repository | ||
| if: ${{ ! inputs.wordpress-build-artifact-available }} | ||
| uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 |
Comment on lines
+83
to
+85
| - name: Create ZIP of built files | ||
| run: zip -q -r wordpress-build.zip . -x '.git/*' 'wordpress-build.zip' | ||
|
|
Comment on lines
183
to
186
| uses: ./.github/workflows/reusable-phpunit-tests-v3.yml | ||
| needs: [ build-wordpress ] | ||
| permissions: | ||
| contents: read |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
.github/workflows/reusable-phpunit-tests-v3.yml:156
- When
inputs.wordpress-build-artifact-availableis true, this step is skipped, but later steps still run PHPUnit via./vendor/bin/phpunit(see theRun PHPUnit testsstep). Sincevendor/is not in the repo by default, skipping PHP setup will prevent Composer dependencies from being installed correctly for the job.
This issue also appears on line 160 of the same file.
- name: Set up PHP
if: ${{ ! inputs.wordpress-build-artifact-available }}
uses: shivammathur/setup-php@f3e473d116dcccaddc5834248c87452386958240 # 2.37.2
with:
php-version: '${{ inputs.php }}'
coverage: none
.github/workflows/reusable-phpunit-tests-v3.yml:164
vendor/bin/phpunitis invoked later in this workflow, but Composer dependencies are not installed whenwordpress-build-artifact-availableis true. The build artifact created inphpunit-tests.ymlonly runsnpm ci/build and does not generatevendor/, so the test job will fail without running this step.
- name: Install Composer dependencies
if: ${{ ! inputs.wordpress-build-artifact-available }}
uses: ramsey/composer-install@65e4f84970763564f46a70b8a54b90d033b3bdda # 4.0.0
with:
custom-cache-suffix: $(/bin/date -u --date='last Mon' "+%F")
.github/workflows/phpunit-tests.yml:186
- This job now depends on
build-wordpress, but it never enables the new artifact path (wordpress-build-artifact-available: true). As a result it will still do its own checkout/npm install/build inside the reusable workflow (duplicating work) while also waiting for the separate build job to complete.
test-with-mariadb:
name: PHP ${{ matrix.php }}
uses: ./.github/workflows/reusable-phpunit-tests-v3.yml
needs: [ build-wordpress ]
permissions:
contents: read
.github/workflows/phpunit-tests.yml:85
- The artifact zip is created from the entire working directory after
npm ci, which will includenode_modules/and any other build-time artifacts. With the large PHPUnit matrix, every downstream job will download this ZIP, which can significantly increase workflow time and artifact storage/transfer. Consider limiting the artifact contents to only what downstream jobs need (e.g., built assets) and letting each job restore/install dependencies via cache, or otherwise pruning large directories before zipping.
- name: Create ZIP of built files
run: zip -q -r wordpress-build.zip . -x '.git/*' 'wordpress-build.zip'
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (4)
.github/workflows/phpunit-tests.yml:173
wordpress-build-artifact-availableis passed into the reusable workflow, butreusable-phpunit-tests-v4.ymldoes not declare this as aworkflow_callinput. This will cause the calling job to fail workflow validation with an "Unexpected input" error.
This issue also appears in the following locations of the same file:
- line 272
- line 304
- line 366
memcached: ${{ matrix.memcached }}
phpunit-config: ${{ matrix.multisite && 'tests/phpunit/multisite.xml' || 'phpunit.xml.dist' }}
tests-domain: ${{ matrix.tests-domain }}
report: ${{ matrix.report || false }}
.github/workflows/phpunit-tests.yml:274
wordpress-build-artifact-availableis passed into the reusable workflow, butreusable-phpunit-tests-v4.ymldoes not declare this as aworkflow_callinput. This will cause the calling job to fail workflow validation with an "Unexpected input" error.
report: false
#
.github/workflows/phpunit-tests.yml:308
wordpress-build-artifact-availableis passed into the reusable workflow, butreusable-phpunit-tests-v4.ymldoes not declare this as aworkflow_callinput. This will cause the calling job to fail workflow validation with an "Unexpected input" error.
db-version: ${{ matrix.db-version }}
phpunit-test-groups: ${{ matrix.phpunit-test-groups }}
#
# Runs unit tests for forks.
.github/workflows/phpunit-tests.yml:370
wordpress-build-artifact-availableis passed into the reusable workflow, butreusable-phpunit-tests-v4.ymldoes not declare this as aworkflow_callinput. This will cause the calling job to fail workflow validation with an "Unexpected input" error.
phpunit-test-groups: ${{ matrix.phpunit-test-groups || '' }}
slack-notifications:
name: Slack Notifications
uses: ./.github/workflows/slack-notifications.yml
Comment on lines
+140
to
+145
| - name: Set up Node.js | ||
| uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 | ||
| with: | ||
| node-version-file: '.nvmrc' | ||
| cache: npm | ||
|
|
Comment on lines
+101
to
+118
| # Performs the following steps: | ||
| # - Sets environment variables. | ||
| # - Checks out the repository. | ||
| # - Sets up Node.js. | ||
| # - Sets up PHP. | ||
| # - Installs Composer dependencies. | ||
| # - Installs npm dependencies | ||
| # - Logs general debug information about the runner. | ||
| # - Logs Docker debug information (about the Docker installation within the runner). | ||
| # - Starts the WordPress Docker container. | ||
| # - Logs the running Docker containers. | ||
| # - Logs debug information about what's installed within the WordPress Docker containers. | ||
| # - Install WordPress within the Docker container. | ||
| # - Run the PHPUnit tests. | ||
| # - Upload the code coverage report to Codecov.io. | ||
| # - Ensures version-controlled files are not modified or deleted. | ||
| # - Checks out the WordPress Test reporter repository. | ||
| # - Submit the test results to the WordPress.org host test results. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
.github/workflows/reusable-phpunit-tests-v4.yml:154
- This workflow no longer installs Composer dependencies, but it still runs PHPUnit from ./vendor/bin/phpunit. Since vendor/ is not in the repository, the test commands will fail with "No such file or directory" unless Composer dependencies are installed in this job (as was done in reusable-phpunit-tests-v3.yml).
- name: Set up Node.js
uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0
with:
node-version-file: '.nvmrc'
cache: npm
- name: General debug information
run: |
npm --version
node --version
curl --version
git --version
composer --version
locale -a
.github/workflows/reusable-phpunit-tests-v4.yml:105
- The "Performs the following steps" list is now out of sync with what the workflow actually does (it no longer checks out the repo, builds WordPress, installs npm deps, or runs a git diff check). Updating this comment block will make the workflow easier to maintain.
# Performs the following steps:
# - Sets environment variables.
# - Checks out the repository.
# - Sets up Node.js.
# - Sets up PHP.
Comment on lines
+57
to
+63
| build-wordpress: | ||
| name: Build WordPress | ||
| runs-on: ubuntu-24.04 | ||
| permissions: | ||
| contents: read | ||
| timeout-minutes: 20 | ||
|
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
.github/workflows/phpunit-tests.yml:88
- The build artifact is created by zipping the entire repository root (".") after
npm ci, which will include ignored directories likenode_modules/(see .gitignore:20) and significantly increase artifact size and download/unzip time for every matrix job. It also makes the artifact (and thus the test jobs) implicitly OS/arch-specific.
- name: Create ZIP of built files
run: zip -q -r wordpress-build.zip . -x '.git/*' 'wordpress-build.zip'
.github/workflows/reusable-phpunit-tests-v4.yml:255
- The header comment lists “Ensures version-controlled files are not modified or deleted”, but the workflow no longer performs that check (it existed in v3). Reintroducing the
git diff --exit-codeguard helps catch unexpected file changes during env setup/tests.
- name: Upload test coverage report to Codecov
if: ${{ inputs.coverage-report }}
uses: codecov/codecov-action@fb8b3582c8e4def4969c97caa2f19720cb33a72f # v7.0.0
with:
token: ${{ secrets.CODECOV_TOKEN }}
files: wp-code-coverage${{ inputs.multisite && '-multisite' || '-single' }}-${{ github.sha }}.xml
flags: ${{ inputs.multisite && 'multisite' || 'single' }},php
fail_ci_if_error: true
Comment on lines
+149
to
+154
| - name: Set up Node.js | ||
| uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 | ||
| with: | ||
| node-version-file: '.nvmrc' | ||
| cache: npm | ||
|
|
Comment on lines
+132
to
+140
| - name: Sparse checkout repository metadata | ||
| uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 | ||
| with: | ||
| fetch-depth: 1 | ||
| show-progress: ${{ runner.debug == '1' && 'true' || 'false' }} | ||
| persist-credentials: false | ||
| sparse-checkout: | | ||
| .github | ||
|
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (4)
.github/workflows/reusable-phpunit-tests-v4.yml:118
- The job header comment lists steps (PHP setup, Composer install, npm ci, build, and a post-test git diff) that are no longer performed in this workflow. This makes the workflow misleading to maintainers and reviewers.
# Performs the following steps:
# - Sets environment variables.
# - Checks out the repository.
# - Sets up Node.js.
# - Sets up PHP.
# - Installs Composer dependencies.
# - Installs npm dependencies
# - Logs general debug information about the runner.
# - Logs Docker debug information (about the Docker installation within the runner).
# - Starts the WordPress Docker container.
# - Logs the running Docker containers.
# - Logs debug information about what's installed within the WordPress Docker containers.
# - Install WordPress within the Docker container.
# - Run the PHPUnit tests.
# - Upload the code coverage report to Codecov.io.
# - Ensures version-controlled files are not modified or deleted.
# - Checks out the WordPress Test reporter repository.
# - Submit the test results to the WordPress.org host test results.
.github/workflows/reusable-phpunit-tests-v4.yml:136
- The sparse checkout into "repo-metadata" is currently unused (no later step references that path), and it also means the workspace doesn’t have a .git directory. That prevents restoring the post-test "git diff --exit-code" guard that exists in reusable-phpunit-tests-v3.yml:271, and it makes it harder to debug CI failures locally. Consider doing a normal checkout into the workspace root, and unzip with overwrite enabled.
- name: Sparse checkout repository metadata
uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
with:
path: 'repo-metadata'
fetch-depth: 1
.github/workflows/reusable-phpunit-tests-v4.yml:257
- reusable-phpunit-tests-v3.yml includes a post-test "git diff --exit-code" check (v3:271) to ensure tests/build steps don’t modify tracked files. This workflow drops that guard. If you switch back to a normal checkout (so .git exists), you can restore the check just before bringing in the external test-runner repo.
- name: Checkout the WordPress Test Reporter
.github/workflows/phpunit-tests.yml:88
- The ZIP artifact is created from the entire workspace ("zip -r ... ."), which will also include large, generated directories like node_modules/ from the build step. Because every matrix job downloads/unzips this artifact, this can significantly increase CI time and bandwidth and risks hitting artifact size limits.
- name: Create ZIP of built files
run: zip -q -r wordpress-build.zip . -x '.git/*' 'wordpress-build.zip'
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
.github/workflows/reusable-phpunit-tests-v4.yml:105
- The step list in this header comment is now inaccurate: the workflow no longer checks out the full repository or sets up PHP / installs Composer + npm dependencies (it downloads and unzips a prebuilt artifact instead). Please update the comment to match the actual steps so future changes don’t rely on stale guidance.
# Performs the following steps:
# - Sets environment variables.
# - Checks out the repository.
# - Sets up Node.js.
# - Sets up PHP.
.github/workflows/reusable-phpunit-tests-v4.yml:136
- The “Sparse checkout repository metadata” step checks out
.githubintorepo-metadata/, but nothing in this workflow referencesrepo-metadataafterward. This adds time and complexity without affecting the job outcome.
- name: Sparse checkout repository metadata
uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
with:
path: 'repo-metadata'
fetch-depth: 1
.github/workflows/phpunit-tests.yml:87
zip -r wordpress-build.zip .will include everything in the workspace afternpm ciandnpm run build:dev, includingnode_modules/(and any other build byproducts). That will likely create a very large artifact that must be downloaded/unzipped by every matrix job, which can negate the benefit of “build once” and may hit artifact size/time limits.
- name: Create ZIP of built files
run: zip -q -r wordpress-build.zip . -x '.git/*' 'wordpress-build.zip'
Comment on lines
+247
to
+255
| - name: Upload test coverage report to Codecov | ||
| if: ${{ inputs.coverage-report }} | ||
| uses: codecov/codecov-action@fb8b3582c8e4def4969c97caa2f19720cb33a72f # v7.0.0 | ||
| with: | ||
| token: ${{ secrets.CODECOV_TOKEN }} | ||
| files: wp-code-coverage${{ inputs.multisite && '-multisite' || '-single' }}-${{ github.sha }}.xml | ||
| flags: ${{ inputs.multisite && 'multisite' || 'single' }},php | ||
| fail_ci_if_error: true | ||
|
|
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (6)
tests/phpunit/tests/peter-is-testing-this-pr.php:21
- This test will fail when the PHPUnit suite is run outside GitHub Actions (e.g., locally or on other CI) because it hard-requires GITHUB_* env vars. To keep the core test suite portable, skip these assertions when not running in GitHub Actions.
public function test_github_environment_variable_defined( $variable, $expected_value = null ) {
$variable_value = getenv( $variable );
$this->assertNotFalse( $variable_value, "Environment variable $variable is not defined." );
if ( $expected_value !== null ) {
$this->assertSame( $expected_value, $variable_value, "Environment variable $variable does not have the expected value." );
}
tests/phpunit/tests/peter-is-testing-this-pr.php:46
- Several variables asserted here are only set for specific event types (e.g., GITHUB_HEAD_REF/GITHUB_BASE_REF are only present for pull_request), and expecting GITHUB_EVENT_NAME to always be "pull_request" will break push/schedule runs of
.github/workflows/phpunit-tests.yml. Consider only asserting PR-specific variables when the event is actually a PR, and avoid hard-coding the event name.
public function data_github_environment_variable_defined() {
return array(
array( 'GITHUB_ACTIONS', 'true' ),
array( 'GITHUB_WORKFLOW' ),
array( 'GITHUB_RUN_ID' ),
array( 'GITHUB_RUN_NUMBER' ),
array( 'GITHUB_JOB' ),
array( 'GITHUB_ACTION' ),
array( 'GITHUB_ACTOR' ),
array( 'GITHUB_REPOSITORY' ),
array( 'GITHUB_EVENT_NAME', 'pull_request' ),
array( 'GITHUB_EVENT_PATH' ),
array( 'GITHUB_WORKSPACE' ),
array( 'GITHUB_SHA' ),
array( 'GITHUB_REF' ),
array( 'GITHUB_HEAD_REF' ),
array( 'GITHUB_BASE_REF' ),
);
.github/workflows/reusable-phpunit-tests-v4.yml:107
- The step list in this job comment no longer matches what the workflow actually does (it no longer checks out the full repo, sets up PHP on the runner, installs Composer deps, runs
npm ci, builds WordPress, or runsgit diff). Please update the comment so future edits/debugging aren’t based on incorrect documentation.
# Performs the following steps:
# - Sets environment variables.
# - Checks out the repository.
# - Sets up Node.js.
# - Sets up PHP.
# - Installs Composer dependencies.
# - Installs npm dependencies
.github/workflows/reusable-phpunit-tests-v4.yml:162
- This workflow invokes
php ./vendor/bin/phpunit, but it no longer installs Composer dependencies on the runner (and the new build job only runsnpm ci/npm run build:dev, sovendor/is unlikely to exist after unzipping the artifact). Re-introduce a Composer install step (or ensurevendor/is included in the artifact) so the PHPUnit binary is available.
- name: Set up Node.js
uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0
with:
node-version-file: '.nvmrc'
cache: npm
- name: General debug information
run: |
npm --version
node --version
curl --version
git --version
locale -a
.github/workflows/phpunit-tests.yml:88
zip -r wordpress-build.zip .will package the entire working directory afternpm ci, includingnode_modules/, which can make the artifact very large and slow to upload/download across every matrix job. Consider excludingnode_modules/and re-installing with the existing npm cache in each test job, or otherwise limiting the artifact contents to only what the reusable PHPUnit workflow needs.
- name: Create ZIP of built files
run: zip -q -r wordpress-build.zip . -x '.git/*' 'wordpress-build.zip'
.github/workflows/phpunit-tests.yml:63
build-wordpresscurrently runs unconditionally for any event that triggers the workflow. In forks, this means it can run onpushevents even though all downstream test jobs are skipped (theirif:only allows PRs for non-WordPress repos). Consider adding anif:to align the build job with the conditions under which test jobs will actually run.
build-wordpress:
name: Build WordPress
runs-on: ubuntu-24.04
permissions:
contents: read
timeout-minutes: 20
Comment on lines
+3
to
+6
| /** | ||
| * Peter is testing this PR. | ||
| */ | ||
| class Tests_Peter_Is_Testing_This_PR extends WP_UnitTestCase { |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (3)
tests/phpunit/tests/peter-is-testing-this-pr.php:44
GITHUB_EVENT_NAMEis not alwayspull_request(this workflow also runs onpushandschedule), so this assertion will fail in CI for non-PR runs.
array( 'GITHUB_BASE_REF' ),
array( 'GITHUB_ENV' ),
array( 'GITHUB_EVENT_NAME', 'pull_request' ),
array( 'GITHUB_EVENT_PATH' ),
.github/workflows/reusable-phpunit-tests-v4.yml:105
- The step list in this header comment is now inaccurate (it mentions checking out the full repo, setting up PHP, and installing Composer/npm deps). In v4 the job downloads and unzips a pre-built artifact instead, so the comment should be updated to reflect the actual steps.
# Performs the following steps:
# - Sets environment variables.
# - Checks out the repository.
# - Sets up Node.js.
# - Sets up PHP.
.github/workflows/phpunit-tests.yml:87
zip -r wordpress-build.zip .will package the entire working directory, includingnode_modules/produced bynpm ci. With this workflow's large test matrix, repeatedly uploading/downloading a huge artifact can be slower and risks hitting artifact size/time limits. The repo already uses a narrower build artifact elsewhere (e.g..github/workflows/reusable-test-core-build-process.yml:125-128zips onlybuild/).
- name: Create ZIP of built files
run: zip -q -r wordpress-build.zip . -x '.git/*' 'wordpress-build.zip'
Comment on lines
+17
to
+24
| public function test_github_environment_variable_defined( $variable, $expected_value = null ) { | ||
| $variable_value = getenv( $variable ); | ||
| $this->assertNotFalse( $variable_value, "Environment variable $variable is not defined." ); | ||
|
|
||
| if ( $expected_value !== null ) { | ||
| $this->assertSame( $expected_value, $variable_value, "Environment variable $variable does not have the expected value." ); | ||
| } | ||
| } |
Comment on lines
+132
to
+141
| - name: Sparse checkout repository metadata | ||
| uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 | ||
| with: | ||
| path: 'repo-metadata' | ||
| fetch-depth: 1 | ||
| show-progress: ${{ runner.debug == '1' && 'true' || 'false' }} | ||
| persist-credentials: false | ||
| sparse-checkout: | | ||
| .github | ||
|
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
.github/workflows/phpunit-tests.yml:88
- The build artifact ZIP is created from the entire repo after
npm ci, which will includenode_modules/and greatly inflate the artifact size and upload/download time. Since the downstream PHPUnit jobs don’t neednode_modules/, remove it before zipping (or explicitly exclude it) to avoid excessive artifact storage and transfer overhead.
- name: Create ZIP of built files
run: zip -q -r wordpress-build.zip . -x '.git/*' 'wordpress-build.zip'
.github/workflows/reusable-phpunit-tests-v4.yml:150
- This repository does not have a
composer.lock, so runningcomposer installwill resolve dependencies and write a new lock file into the workspace. That’s a behavior change vs the existing workflow (which usesramsey/composer-installto runcomposer updatewhen no lock exists) and also drops Composer caching. Consider switching back toramsey/composer-installto preserve existing semantics and cache behavior.
- name: Install Composer dependencies
run: composer install --no-interaction --no-progress --prefer-dist
tests/phpunit/tests/peter-is-testing-this-pr.php:8
- This looks like a personal/POC-only test file (and the
@groupname explicitly says it should never be committed). Keeping it requires permanently changingphpunit.xml.distto exclude a one-off group, which adds long-term maintenance burden. Recommend removing this file (and the corresponding group exclusion) before merging, or converting it into a stable, project-scoped CI assertion test with a neutral name/group.
/**
* Peter is testing this PR.
*
* @group this-should-never-be-committed-to-trunk-but-some-of-it-might-be-helpful
*/
class Tests_Peter_Is_Testing_This_PR extends WP_UnitTestCase {
phpunit.xml.dist:34
phpunit.xml.distis being updated to exclude a one-off, PR-specific test group. This bakes temporary debugging into the default PHPUnit configuration and will be hard to justify long-term. If the associated test is not intended to be permanent, it should be removed instead of adding a new excluded group here.
<group>ms-required</group>
<group>external-http</group>
<group>html-api-web-platform-tests</group>
<group>this-should-never-be-committed-to-trunk-but-some-of-it-might-be-helpful</group>
</exclude>
|
|
||
| services: | ||
| database: | ||
| image: ${{ inputs.db-type }}:${{ inputs.db-version }} |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (6)
.github/workflows/phpunit-tests.yml:87
- The build artifact ZIP is created from the repository root after
npm ci, which means it will includenode_modules/and significantly inflate artifact size and transfer time (the later test jobs don’t run any Node tasks). Excludingnode_modules/(and other build-only directories if desired) should reduce runtime and artifact storage without affecting PHPUnit execution.
- name: Create ZIP of built files
run: zip -q -r wordpress-build.zip . -x '.git/*' 'wordpress-build.zip'
.github/workflows/phpunit-tests.yml:94
wordpress-buildis expected to be large, but the artifact upload does not set a retention policy. For a large, per-run artifact this can increase storage pressure over time. Consider setting a shortretention-daysto limit storage impact (especially for a POC).
- name: Upload ZIP as a GitHub Actions artifact
uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
with:
name: wordpress-build
path: wordpress-build.zip
if-no-files-found: error
.github/workflows/reusable-phpunit-tests-v4.yml:147
- Xdebug is enabled unconditionally in
setup-php. This adds noticeable runtime overhead to non-coverage test runs. It’s usually better to only enable Xdebug whencoverage-reportis requested.
- name: Set up PHP
uses: shivammathur/setup-php@f3e473d116dcccaddc5834248c87452386958240 # 2.37.2
with:
php-version: '${{ inputs.php }}'
coverage: xdebug
extensions: gd, mysqli, memcached
.github/workflows/reusable-phpunit-tests-v4.yml:150
- This workflow uses
composer installdirectly even though the repository does not have acomposer.lockfile, which means dependency resolution will happen on every run and there’s no Composer cache integration. Other workflows in this repo useramsey/composer-installwith a cache suffix to both cache dependencies and avoid lockfile expectations.
- name: Install Composer dependencies
run: composer install --no-interaction --no-progress --prefer-dist
tests/phpunit/tests/peter-is-testing-this-pr.php:8
- This adds an ad-hoc test file with a group name explicitly stating it should never be committed. Keeping this in the tree requires configuration changes to exclude it and risks accidental execution/failures later. Consider removing this file from the PR (or converting it into a stable CI-focused test with a neutral group name and robust expectations).
* Peter is testing this PR.
*
* @group this-should-never-be-committed-to-trunk-but-some-of-it-might-be-helpful
*/
class Tests_Peter_Is_Testing_This_PR extends WP_UnitTestCase {
phpunit.xml.dist:34
- This adds a permanent global exclusion for an experimental/one-off test group. Baking temporary experimentation into the default PHPUnit config makes it easier for future ad-hoc tests to land behind excludes. Consider removing this exclusion once the POC is validated, or replacing it with a stable group name that is explicitly included only where needed.
<group>ms-required</group>
<group>external-http</group>
<group>html-api-web-platform-tests</group>
<group>this-should-never-be-committed-to-trunk-but-some-of-it-might-be-helpful</group>
</exclude>
| run: echo "value=$(/bin/date -u --date='last Mon' '+%F')" >> "$GITHUB_OUTPUT" | ||
|
|
||
| - name: Cache npm dependencies | ||
| uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830 # v4.0.3 |
| run: npm install | ||
|
|
||
| - name: Install Composer dependencies | ||
| uses: ramsey/composer-install@65e4f84970763564f46a70b8a54b90d033b3bdda # 4.0.0 |
| run: echo "value=$(/bin/date -u --date='last Mon' '+%F')" >> "$GITHUB_OUTPUT" | ||
|
|
||
| - name: Cache npm dependencies | ||
| uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830 # v4.0.3 |
| run: echo "value=$(/bin/date -u --date='last Mon' '+%F')" >> "$GITHUB_OUTPUT" | ||
|
|
||
| - name: Cache npm dependencies | ||
| uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830 # v4.0.3 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (4)
.github/workflows/reusable-phpunit-tests-v4.yml:160
npm installis non-deterministic in CI compared tonpm ciand can produce different dependency trees over time. The existing reusable PHPUnit workflow (v3) usesnpm ci(.github/workflows/reusable-phpunit-tests-v3.yml:164-165); aligning v4 reduces flakiness and improves repeatability.
- name: Install npm dependencies
run: npm install
.github/workflows/phpunit-tests.yml:99
- For CI repeatability, prefer
npm ciovernpm install(the existing reusable PHPUnit workflow v3 usesnpm ci). This avoids lockfile drift and makes dependency installs deterministic.
- name: Install npm dependencies
run: npm install
tests/phpunit/tests/peter-is-testing-this-pr.php:8
- This adds a permanently committed test file/class/group that is personal/temporary in nature (file and class name reference an individual, and the group literally says it should never be committed). In addition, the test unconditionally calls
markTestIncomplete(), making the assertions below unreachable.
/**
* Peter is testing this PR.
*
* @group this-should-never-be-committed-to-trunk-but-some-of-it-might-be-helpful
*/
class Tests_Peter_Is_Testing_This_PR extends WP_UnitTestCase {
phpunit.xml.dist:34
- Adding a one-off “never be committed” debug group to the global excluded groups in phpunit.xml.dist changes default test selection for everyone. If the related test is not intended to ship, it’s better to remove it rather than codify a permanent exclusion in the shared PHPUnit config.
<group>ms-required</group>
<group>external-http</group>
<group>html-api-web-platform-tests</group>
<group>this-should-never-be-committed-to-trunk-but-some-of-it-might-be-helpful</group>
</exclude>
Comment on lines
+57
to
+62
| build-wordpress: | ||
| name: Build WordPress | ||
| runs-on: ubuntu-24.04 | ||
| permissions: | ||
| contents: read | ||
| timeout-minutes: 20 |
Comment on lines
+158
to
+168
| - name: Install npm dependencies | ||
| run: npm install | ||
|
|
||
| - name: Install Composer dependencies | ||
| uses: ramsey/composer-install@65e4f84970763564f46a70b8a54b90d033b3bdda # 4.0.0 | ||
| with: | ||
| custom-cache-suffix: ${{ steps.weekly-cache-suffix.outputs.value }} | ||
|
|
||
| - name: Build WordPress | ||
| run: npm run build:dev | ||
|
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (10)
.github/workflows/reusable-phpunit-tests-v4.yml:194
- When using GitHub Actions service containers with
ports: - 11211, the host port is typically dynamically assigned. Hardcoding11211inwp-tests-config.phpcan point tests at the wrong port. Usejob.services.memcached.ports['11211']instead.
if [ "${LOCAL_PHP_MEMCACHED}" = 'true' ]; then
cp tests/phpunit/includes/object-cache.php src/wp-content/object-cache.php
echo "\$memcached_servers = array( array( '127.0.0.1', 11211 ) );" >> wp-tests-config.php
fi
.github/workflows/reusable-phpunit-tests-v4.yml:155
setup-phpis enabling Xdebug for every run (coverage: xdebug), which will significantly slow the test suite even wheninputs.coverage-reportis false. Enable Xdebug only for coverage runs.
coverage: xdebug
.github/workflows/reusable-phpunit-tests-v4.yml:146
- Caching
node_modulescan introduce cache pollution (native binaries, postinstall side effects) and is inconsistent with other workflows here that rely on npm's cache. Prefer caching~/.npm(orsetup-node's built-incache: npm) and reinstalling deterministically.
path: |
~/.npm
node_modules
key: npm-${{ runner.os }}-${{ hashFiles('package-lock.json') }}-${{ steps.weekly-cache-suffix.outputs.value }}
.github/workflows/reusable-phpunit-tests-v4.yml:159
- This workflow uses
npm install, whereas the repo's other CI workflows consistently usenpm cifor deterministic installs frompackage-lock.json. Usingnpm installcan change the lockfile and produce non-reproducible builds.
run: npm install
.github/workflows/reusable-phpunit-tests-v4.yml:200
apache2 -vis not guaranteed to exist on the runner image and, if missing, will fail the step (default shell uses-e). If this is only for debugging, guard it so it doesn't break the job.
apache2 -v
.github/workflows/reusable-phpunit-tests-v4.yml:205
mysql --versionis not guaranteed to be installed on the host runner (especially since MySQL is provided via a service container). If this is only for debugging, guard it so it doesn't fail the workflow.
mysql --version
.github/workflows/phpunit-tests.yml:61
- The new
build-wordpressjob does not currently build WordPress (npm run build:dev) nor upload an artifact, yet all PHPUnit matrix jobs nowneeds: [ build-wordpress ]. This serializes the workflow without achieving the stated goal of "build once for use during the PHPUnit workflow".
build-wordpress:
name: Build WordPress
runs-on: ubuntu-24.04
permissions:
contents: read
.github/workflows/phpunit-tests.yml:98
- Use
npm ciin CI for deterministic installs frompackage-lock.json(consistent with other workflows in this repo).
run: npm install
phpunit.xml.dist:33
- Adding a long, PR-specific group name to the global
<exclude>list inphpunit.xml.distis a persistent config change that can mask accidentally-committed placeholder tests. This looks like a temporary POC artifact and should be removed before merging.
<group>this-should-never-be-committed-to-trunk-but-some-of-it-might-be-helpful</group>
tests/phpunit/tests/peter-is-testing-this-pr.php:8
- This test file is explicitly PR-author-specific (file/class name and the group name), and the test is permanently
markTestIncomplete(). Even though it’s excluded by default inphpunit.xml.dist, leaving it in the tree risks masking unfinished CI checks and adds long-term maintenance noise. It would be better to remove this file (or move the useful parts into a properly named, generally applicable test) before merging.
/**
* Peter is testing this PR.
*
* @group this-should-never-be-committed-to-trunk-but-some-of-it-might-be-helpful
*/
class Tests_Peter_Is_Testing_This_PR extends WP_UnitTestCase {
Comment on lines
+119
to
+122
| memcached: | ||
| image: ${{ inputs.memcached && 'memcached:1.6' || '' }} | ||
| ports: | ||
| - 11211 |
Comment on lines
+82
to
+86
| with: | ||
| path: | | ||
| ~/.npm | ||
| node_modules | ||
| key: npm-${{ runner.os }}-${{ hashFiles('package-lock.json') }}-${{ steps.weekly-cache-suffix.outputs.value }} |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (6)
.github/workflows/phpunit-tests.yml:62
- The new
build-wordpressjob does not actually build WordPress or upload any artifact for downstream jobs to consume, but all PHPUnit matrix jobs nowneeds: [ build-wordpress ]. This introduces a hard serialization point (reducing parallelism) without implementing the “build once, reuse artifact” goal described in the PR.
build-wordpress:
name: Build WordPress
runs-on: ubuntu-24.04
permissions:
contents: read
timeout-minutes: 20
.github/workflows/reusable-phpunit-tests-v4.yml:122
- GitHub Actions service containers require a valid
image. Wheninputs.memcachedis false, this setsimageto an empty string, which is invalid and will fail workflow validation/runtime. If conditional services are needed, the service should be defined unconditionally (or split into separate jobs/workflows).
memcached:
image: ${{ inputs.memcached && 'memcached:1.6' || '' }}
ports:
- 11211
.github/workflows/reusable-phpunit-tests-v4.yml:156
- Xdebug is enabled for every run (
coverage: xdebug) even wheninputs.coverage-reportis false, which can significantly slow the full matrix. Consider enabling Xdebug only for coverage runs.
uses: shivammathur/setup-php@f3e473d116dcccaddc5834248c87452386958240 # 2.37.2
with:
php-version: '${{ inputs.php }}'
coverage: xdebug
extensions: gd, mysqli, memcached, imagick
.github/workflows/reusable-phpunit-tests-v4.yml:165
- This reusable workflow still installs npm/Composer dependencies and runs
npm run build:devinside every matrix job. As-is, it does not consume any artifact from the new build job, so the expensive build work is not actually being reused.
- name: Install npm dependencies
run: npm install
phpunit.xml.dist:33
- Adding a permanently-excluded PHPUnit group with a name that explicitly says it should never be committed will silently skip any tests tagged with it in all default runs. If this is only for temporary local/POC tests, it should not be in the shared phpunit config.
<group>this-should-never-be-committed-to-trunk-but-some-of-it-might-be-helpful</group>
tests/phpunit/tests/peter-is-testing-this-pr.php:8
- This test file appears to be personal/temporary debugging code (including the group name indicating it should never be committed and the class name tied to an individual). Keeping this in the tree increases long-term maintenance burden even if excluded by default; consider removing it from the PR or converting it into a properly-scoped, stable CI/environment test under an appropriate group/name.
/**
* Peter is testing this PR.
*
* @group this-should-never-be-committed-to-trunk-but-some-of-it-might-be-helpful
*/
class Tests_Peter_Is_Testing_This_PR extends WP_UnitTestCase {
Comment on lines
+91
to
+104
| - name: Set up PHP | ||
| uses: shivammathur/setup-php@f3e473d116dcccaddc5834248c87452386958240 # 2.37.2 | ||
| with: | ||
| php-version: '8.4' | ||
| coverage: none | ||
|
|
||
| - name: Install npm dependencies | ||
| run: npm install | ||
|
|
||
| - name: Install Composer dependencies | ||
| uses: ramsey/composer-install@65e4f84970763564f46a70b8a54b90d033b3bdda # 4.0.0 | ||
| with: | ||
| custom-cache-suffix: ${{ steps.weekly-cache-suffix.outputs.value }} | ||
|
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (5)
.github/workflows/reusable-phpunit-tests-v4.yml:122
- The
memcachedservice setsimageto an empty string wheninputs.memcachedis false (${{ ... || '' }}). GitHub Actions requires a valid image name, so this will fail job setup for the default case (memcached disabled).
memcached:
image: ${{ inputs.memcached && 'memcached:1.6' || '' }}
ports:
- 11211
.github/workflows/reusable-phpunit-tests-v4.yml:170
- This workflow uses
npm install, but the repo’s workflows consistently usenpm cifor deterministic installs with the committed lockfile (e.g..github/workflows/reusable-test-core-build-process.yml:106-108).npm installcan modifypackage-lock.jsonand cause the latergit diff --exit-codecheck to fail.
- name: Install npm dependencies
run: npm install
.github/workflows/phpunit-tests.yml:104
- The PR description says the workflow will "build WordPress once" and upload an artifact for the PHPUnit jobs to consume. The new
build-wordpressjob currently only installs npm/Composer deps and does not run a build or upload anything, and the reusable workflow still does its ownnpm run build:dev. As written, this adds a serialized prerequisite (needs: [ build-wordpress ]) without actually providing a shared build output.
build-wordpress:
name: Build WordPress
runs-on: ubuntu-24.04
permissions:
contents: read
timeout-minutes: 20
steps:
- name: Checkout repository
uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
with:
show-progress: ${{ runner.debug == '1' && 'true' || 'false' }}
persist-credentials: false
- name: Set up Node.js
uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0
with:
node-version-file: '.nvmrc'
- name: Set weekly cache suffix
id: weekly-cache-suffix
run: echo "value=$(/bin/date -u --date='last Mon' '+%F')" >> "$GITHUB_OUTPUT"
- name: Cache npm dependencies
uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830 # v4.0.3
with:
path: |
~/.npm
node_modules
key: npm-${{ runner.os }}-${{ hashFiles('package-lock.json') }}-${{ steps.weekly-cache-suffix.outputs.value }}
restore-keys: |
npm-${{ runner.os }}-${{ hashFiles('package-lock.json') }}-
npm-${{ runner.os }}-
- name: Set up PHP
uses: shivammathur/setup-php@f3e473d116dcccaddc5834248c87452386958240 # 2.37.2
with:
php-version: '8.4'
coverage: none
- name: Install npm dependencies
run: npm install
- name: Install Composer dependencies
uses: ramsey/composer-install@65e4f84970763564f46a70b8a54b90d033b3bdda # 4.0.0
with:
custom-cache-suffix: ${{ steps.weekly-cache-suffix.outputs.value }}
.github/workflows/phpunit-tests.yml:90
- The cache step stores
node_modulesviaactions/cache. This is more likely to produce polluted/invalid caches than caching the npm cache, and it’s inconsistent with the repo’s pattern of usingactions/setup-nodewithcache: npm. Consider caching only~/.npm(or switching to setup-node’s npm cache) to reduce cache-related flakiness.
- name: Cache npm dependencies
uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830 # v4.0.3
with:
path: |
~/.npm
node_modules
key: npm-${{ runner.os }}-${{ hashFiles('package-lock.json') }}-${{ steps.weekly-cache-suffix.outputs.value }}
restore-keys: |
npm-${{ runner.os }}-${{ hashFiles('package-lock.json') }}-
npm-${{ runner.os }}-
phpunit.xml.dist:33
phpunit.xml.distnow excludes a PR-specific group name (this-should-never-be-committed-to-trunk-but-some-of-it-might-be-helpful). This kind of ad-hoc exclusion is easy to forget and will silently hide tests if anything ever uses that group name again. If the test file is temporary/POC-only, it’s better to remove it rather than baking the exclusion into the shared config.
<group>this-should-never-be-committed-to-trunk-but-some-of-it-might-be-helpful</group>
Comment on lines
+140
to
+149
| - name: Cache npm dependencies | ||
| uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830 # v4.0.3 | ||
| with: | ||
| path: | | ||
| ~/.npm | ||
| node_modules | ||
| key: npm-${{ runner.os }}-${{ hashFiles('package-lock.json') }}-${{ steps.weekly-cache-suffix.outputs.value }} | ||
| restore-keys: | | ||
| npm-${{ runner.os }}-${{ hashFiles('package-lock.json') }}- | ||
| npm-${{ runner.os }}- |
Comment on lines
+97
to
+99
| - name: Install npm dependencies | ||
| run: npm install | ||
|
|
Contributor
Author
|
Nah... there is no change I'll ever take this out of draft. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a POC to build WordPress once for use during the PHP Unit workflow.
Choosing a PR to trunk at random, the machine time for PHPUnit is dropping by about 3 hours. As trunk runs additional tests, this will save additional time there.
A space checkout is retained as that will set up the GHA environment variables needed for the unit test suite to determine if tests that target only trunk are, in fact, targetting trunk. It's also needed for the actions that report back to WP.org.
Questions:
Risks/issues:
Trac ticket:
Use of AI Tools
AI assistance: Yes
Tool(s): VS Code/GitHub Copilot
Model(s): GPT-5.3-Codex
Used for: Most of it as this is a POC.
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.