Try running phpunit on the GHA machine rather than a docker env. - #12760
Draft
peterwilsoncc wants to merge 16 commits into
Draft
Try running phpunit on the GHA machine rather than a docker env.#12760peterwilsoncc wants to merge 16 commits into
peterwilsoncc wants to merge 16 commits into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| --health-retries=10 | ||
| --health-start-period=60s | ||
| memcached: | ||
| image: ${{ inputs.memcached && 'memcached' || 'alpine' }} |
| --health-retries=10 | ||
| --health-start-period=60s | ||
| memcached: | ||
| image: ${{ inputs.memcached && 'memcached' || 'alpine' }} |
There was a problem hiding this comment.
Pull request overview
This PR updates the reusable PHPUnit GitHub Actions workflow to run PHPUnit directly on the GitHub-hosted runner (using setup-php) instead of running tests inside the project’s Docker environment.
Changes:
- Adds GitHub Actions service containers for MySQL/MariaDB (and memcached) to support runner-based test execution.
- Switches PHPUnit invocations from the Docker wrapper script to direct
./vendor/bin/phpunitexecution on the runner. - Reworks code coverage setup to install/configure PCOV directly on the runner.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+224
to
+233
| - name: Set up test configuration | ||
| run: | | ||
| npm run env:start | ||
|
|
||
| - name: Log running Docker containers | ||
| run: docker ps -a | ||
|
|
||
| - name: WordPress Docker container debug information | ||
| run: | | ||
| docker compose run --rm mysql "${LOCAL_DB_CMD}" --version | ||
| docker compose run --rm php php --version | ||
| docker compose run --rm php php -m | ||
| docker compose run --rm php php -i | ||
| docker compose run --rm php locale -a | ||
| env: | ||
| LOCAL_DB_CMD: ${{ env.LOCAL_DB_TYPE == 'mariadb' && contains( fromJSON('["5.5", "10.0", "10.1", "10.2", "10.3"]'), env.LOCAL_DB_VERSION ) && 'mysql' || env.LOCAL_DB_TYPE }} | ||
|
|
||
| - name: Install WordPress | ||
| run: npm run env:install | ||
| sed \ | ||
| -e 's/youremptytestdbnamehere/wordpress_develop_tests/' \ | ||
| -e 's/yourusernamehere/root/' \ | ||
| -e 's/yourpasswordhere/password/' \ | ||
| -e 's/localhost/127.0.0.1/' \ | ||
| -e "s/'WP_TESTS_DOMAIN', 'example.org'/'WP_TESTS_DOMAIN', '${LOCAL_WP_TESTS_DOMAIN}'/" \ | ||
| wp-tests-config-sample.php > wp-tests-config.php | ||
| echo "define( 'FS_METHOD', 'direct' );" >> wp-tests-config.php |
Comment on lines
+151
to
+154
| memcached: | ||
| image: ${{ inputs.memcached && 'memcached' || 'alpine' }} | ||
| ports: | ||
| - 11211:11211 |
|
|
||
| services: | ||
| mysql: | ||
| image: ${{ inputs.db-type }}:${{ inputs.db-version }} |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
.github/workflows/reusable-phpunit-tests-v3.yml:160
- The memcached service falls back to the
alpineimage wheninputs.memcachedis false. The defaultalpinecontainer exits immediately (no long-running command), which can cause the job to fail because a declared service container is not running. Prefer always using thememcachedimage (the PHP extension can remain conditional) so the service container stays up.
memcached:
image: ${{ inputs.memcached && 'memcached' || 'alpine' }}
ports:
- 11211:11211
.github/workflows/reusable-phpunit-tests-v3.yml:203
setup-phpenables Xdebug for every job via the extensions list. Xdebug’s default mode can significantly slow down the entire PHPUnit run and can affect runtime behavior in non-Xdebug jobs. Install/enable Xdebug only for the job variants that actually run the Xdebug test group, and keep it disabled otherwise.
uses: shivammathur/setup-php@f3e473d116dcccaddc5834248c87452386958240 # 2.37.2
with:
php-version: '${{ inputs.php }}'
extensions: "gd, opcache, mysqli, zip, exif, intl, mbstring, xml, xsl, imagick, xdebug${{ inputs.memcached && ', memcached' || '' }}"
coverage: none
Comment on lines
+255
to
+263
| pecl install --force pcov-1.0.12 | ||
| PHP_INI_SCAN_DIR=$(php -r 'echo PHP_CONFIG_FILE_SCAN_DIR;') | ||
| { | ||
| echo "extension=pcov" | ||
| echo "pcov.enabled=1" | ||
| echo "pcov.directory=${GITHUB_WORKSPACE}/src" | ||
| echo "pcov.initial.files=10000" | ||
| } > "$PHP_INI_SCAN_DIR/pcov.ini" | ||
| php -m | grep -i pcov |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
.github/workflows/reusable-phpunit-tests-v3.yml:246
- When
inputs.memcachedis enabled, this workflow installs the PHP memcached extension and starts a memcached service, but it doesn’t install theobject-cache.phpdrop-in. Without that drop-in, WordPress won’t actually use Memcached, so the “with memcached” matrix jobs likely won’t be exercising the intended cache backend. The Docker-based local environment handles this by copyingtests/phpunit/includes/object-cache.phpintosrc/wp-content/object-cache.phpwhen memcached is enabled.
- name: Set up test configuration
run: |
sed \
-e "s/youremptytestdbnamehere/${WP_DB_DATABASE}/" \
-e 's/yourusernamehere/root/' \
-e "s/yourpasswordhere/${WP_DB_PASSWORD}/" \
-e 's/localhost/127.0.0.1/' \
-e "s/'WP_TESTS_DOMAIN', 'example.org'/'WP_TESTS_DOMAIN', '${LOCAL_WP_TESTS_DOMAIN}'/" \
wp-tests-config-sample.php > wp-tests-config.php
echo "define( 'FS_METHOD', 'direct' );" >> wp-tests-config.php
.github/workflows/reusable-phpunit-tests-v3.yml:269
- PCOV config writes to "$PHP_INI_SCAN_DIR/pcov.ini" where
PHP_INI_SCAN_DIRis sourced fromPHP_CONFIG_FILE_SCAN_DIR. That constant can be empty or contain multiple paths (colon-separated), which would make this write fail or target an invalid path. It’s safer to derive the scanned ini directory fromphp --iniand fail fast if it can’t be determined.
pecl install --force pcov-1.0.12
PHP_INI_SCAN_DIR=$(php -r 'echo PHP_CONFIG_FILE_SCAN_DIR;')
{
echo "extension=pcov"
echo "pcov.enabled=1"
echo "pcov.directory=${GITHUB_WORKSPACE}/src"
echo "pcov.initial.files=10000"
} > "$PHP_INI_SCAN_DIR/pcov.ini"
.github/workflows/reusable-phpunit-tests-v3.yml:246
- This workflow removes the earlier Docker-based PHPUnit execution, but the final reporting path still shells out to
docker compose run ... php php test-runner/report.php(later in this file). With the Docker environment no longer being pulled/started, enablinginputs.reportwill either fail or silently reintroduce a Docker dependency for reporting. Consider updating the reporting step to run directly on the runner’s PHP (or reintroduce the required Docker setup under the same condition).
- name: Set up test configuration
run: |
sed \
-e "s/youremptytestdbnamehere/${WP_DB_DATABASE}/" \
-e 's/yourusernamehere/root/' \
-e "s/yourpasswordhere/${WP_DB_PASSWORD}/" \
-e 's/localhost/127.0.0.1/' \
-e "s/'WP_TESTS_DOMAIN', 'example.org'/'WP_TESTS_DOMAIN', '${LOCAL_WP_TESTS_DOMAIN}'/" \
wp-tests-config-sample.php > wp-tests-config.php
echo "define( 'FS_METHOD', 'direct' );" >> wp-tests-config.php
Comment on lines
+157
to
+160
| memcached: | ||
| image: ${{ inputs.memcached && 'memcached' || 'alpine' }} | ||
| ports: | ||
| - 11211:11211 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
.github/workflows/reusable-phpunit-tests-v3.yml:160
- When
inputs.memcachedis false, this service uses thealpineimage, which typically exits immediately (no long-running process). GitHub Actions expects service containers to remain running, so this can fail the entire job even when memcached is disabled. Consider always running a real memcached service (and just omit the PHP extension/tests when disabled), or restructure the workflow to only define this service when needed.
memcached:
image: ${{ inputs.memcached && 'memcached' || 'alpine' }}
ports:
- 11211:11211
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 (2)
.github/workflows/reusable-phpunit-tests-v3.yml:266
- This
sedreplacement forWP_TESTS_DOMAINdoesn’t match the actual line inwp-tests-config-sample.php(which isdefine( 'WP_TESTS_DOMAIN', 'example.org' );). As a result the tests-domain input won’t be applied and the config will always useexample.org.
-e "s/'WP_TESTS_DOMAIN', 'example.org'/'WP_TESTS_DOMAIN', '${LOCAL_WP_TESTS_DOMAIN}'/" \
.github/workflows/reusable-phpunit-tests-v3.yml:259
- The workflow-level step list comment for this job still describes running PHPUnit inside the Docker environment (pull/start containers,
env:install, etc.), but those steps have been removed in this change. Updating that comment block will help keep the workflow self-documenting and avoid confusion when debugging CI.
- name: Set up test configuration
| --health-retries=10 | ||
| --health-start-period=60s | ||
| memcached: | ||
| image: ${{ inputs.memcached && 'memcached' || 'alpine' }} |
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 (4)
.github/workflows/reusable-phpunit-tests-v3.yml:263
- This workflow removes the Docker-based test environment steps, but the reporting step later in the job still runs
docker compose run ... php php test-runner/report.php(see theSubmit test results...step). With noenv:pull/env:startequivalent remaining, that reporting path is very likely to fail ontrunkruns whereinputs.reportis true.
- name: Set up test configuration
run: |
sed \
-e "s/youremptytestdbnamehere/${WP_DB_DATABASE}/" \
-e 's/yourusernamehere/root/' \
.github/workflows/reusable-phpunit-tests-v3.yml:156
- The MySQL service maps a fixed host port (
3306:3306) and the generatedwp-tests-config.phphard-codes127.0.0.1with no port. This can conflict on self-hosted runners (or any runner already using 3306). A more robust pattern used elsewhere in this repo is to expose only the container port and read the assigned host port viajob.services.<service>.ports['3306'](e.g..github/workflows/install-testing.yml:107-136).
ports:
- 3306:3306
options: >-
.github/workflows/reusable-phpunit-tests-v3.yml:196
actions/setup-nodeis now skipped wheninputs.wordpress-build-artifactis set, but later steps (e.g.General debug information) still runnpm --version/node --version. On runners without Node preinstalled (possible whenvars.PHPUNIT_RUNNERpoints to a custom runner), the job can fail even though Node isn’t otherwise required when using a pre-built artifact.
Either guard the later Node/NPM usage, or keep setup-node unconditional so Node is always present.
- name: Set up Node.js
if: inputs.wordpress-build-artifact == ''
uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0
with:
node-version-file: '.nvmrc'
cache: npm
.github/workflows/reusable-prepare-gutenberg.yml:69
- This workflow’s job timeout is still set to 10 minutes, but it now runs
npm ciplusnpm run build:dev, which can exceed that on slower runners or during cache misses. If this job times out, every downstream PHPUnit job that needs the artifact will be skipped/fail.
Consider increasing the job timeout-minutes to account for the added build work.
- name: Install npm dependencies
run: npm ci
env:
PUPPETEER_SKIP_DOWNLOAD: true
- name: Build WordPress
run: npm run build:dev
Comment on lines
+74
to
+79
| run: | | ||
| { git diff --name-only HEAD; git ls-files --others --exclude-standard; } \ | ||
| > /tmp/build-manifest.txt | ||
| echo "Files included in build artifact:" | ||
| cat /tmp/build-manifest.txt | ||
| tar -czf /tmp/wordpress-build.tar.gz --files-from=/tmp/build-manifest.txt |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
.github/workflows/reusable-phpunit-tests-v3.yml:295
- Writing
pcov.inivia shell redirection toPHP_CONFIG_FILE_SCAN_DIRcan fail due to permissions (this directory is typically under/etc/php/...and root-owned). That would break coverage-report jobs.
echo "pcov.enabled=1"
echo "pcov.directory=${GITHUB_WORKSPACE}/src"
echo "pcov.initial.files=10000"
} > "$PHP_INI_SCAN_DIR/pcov.ini"
php -m | grep -i pcov
.github/workflows/reusable-phpunit-tests-v3.yml:195
actions/setup-nodeis now skipped wheninputs.wordpress-build-artifact != '', but later steps still callnode/npm(e.g. the "General debug information" step). On runners without preinstalled Node (notably whenvars.PHPUNIT_RUNNERpoints to a custom runner), this will fail even though Node is only being skipped for performance.
- name: Set up Node.js
if: inputs.wordpress-build-artifact == ''
uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0
with:
node-version-file: '.nvmrc'
.github/workflows/reusable-phpunit-tests-v3.yml:230
apt-get installis run without anapt-get update. On GitHub-hosted images (and especially over time), stale package indexes can cause intermittent "Unable to locate package" failures for otherwise valid packages.
- name: Install system dependencies
run: |
sudo apt-get install -y --no-install-recommends ghostscript libheif1 libheif-plugin-aomenc libheif-plugin-aomdec libavif16 locales
# Generate the locales used by WordPress tests.
sudo locale-gen ru_RU.UTF-8 fr_FR.UTF-8 de_DE.UTF-8 es_ES.UTF-8 ja_JP.UTF-8
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.
Trac ticket:
Use of AI Tools
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.