Expand Playwright E2E, migrate to wp-env, PHP 8.2 minimum, and modernize internals (XWPENG-44–47) - #1977
Expand Playwright E2E, migrate to wp-env, PHP 8.2 minimum, and modernize internals (XWPENG-44–47)#1977shadyvb wants to merge 2 commits into
Conversation
ba6eb08 to
4844598
Compare
4844598 to
0067067
Compare
There was a problem hiding this comment.
Pull request overview
Migrates local development and testing from Docker Compose to wp-env, while expanding Playwright coverage and aligning with stacked PR #1974.
Changes:
- Adds wp-env configuration and removes legacy Docker assets.
- Adds shared E2E helpers and five Playwright specifications.
- Updates PHPUnit, CI, documentation, and editor tooling.
Review assessment:
- Critical: None.
- High: E2E exclusion cleanup has a concurrency/data-loss risk.
- Medium/Low: Several migrated npm scripts regress behavior; the debug documentation is ineffective.
Reviewed changes
Copilot reviewed 39 out of 41 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
.distignore |
Removes obsolete Compose exclusions. |
.github/workflows/docker-images.yml |
Removes legacy image publishing. |
.github/workflows/lint-and-test.yml |
Migrates CI to wp-env. |
.gitignore |
Ignores wp-env overrides. |
.vscode/launch.json |
Updates wp-env path mappings. |
.wp-env.json |
Defines multisite wp-env environments. |
contributing.md |
Documents wp-env workflows. |
docker-compose.build.yml |
Removes image build configuration. |
docker-compose.yml |
Removes Compose environment. |
local/docker/mkcert/Dockerfile |
Removes certificate image. |
local/docker/wordpress/Dockerfile |
Removes custom WordPress image. |
local/docker/wordpress/apache-ssl.conf |
Removes legacy TLS configuration. |
local/docker/wordpress/php.ini |
Removes custom PHP configuration. |
local/public/.htaccess |
Removes legacy rewrite rules. |
local/public/index.php |
Removes legacy front controller. |
local/public/wp-cli.yml |
Removes Compose WP-CLI configuration. |
local/public/wp-config.php |
Removes legacy WordPress configuration. |
local/public/wp-content/sunrise.php |
Removes legacy Mercator bootstrap. |
local/public/wp-tests-config.php |
Removes Compose PHPUnit configuration. |
package.json |
Migrates development and test commands. |
phpunit-multisite.xml |
Activates additional test plugins. |
phpunit.xml |
Activates Email Logger in tests. |
playwright.config.js |
Adds authenticated, ordered projects. |
readme.md |
Corrects the CI badge. |
tests/bootstrap.php |
Resolves wp-env PHPUnit paths. |
tests/e2e/admin-orphan-cleanup.spec.js |
Uses shared wp-env helpers. |
tests/e2e/admin-ui-smoke.spec.js |
Uses authenticated contexts. |
tests/e2e/alert-create.spec.js |
Tests highlight alert creation. |
tests/e2e/editor-new-post.spec.js |
Updates authentication and cleanup. |
tests/e2e/export-download.spec.js |
Tests CSV and JSON exports. |
tests/e2e/helpers/stream-plugin.js |
Adds shared Stream E2E utilities. |
tests/e2e/network-activated-records.spec.js |
Tests multisite record visibility. |
tests/e2e/network-activated.spec.js |
Isolates activation-state testing. |
tests/e2e/records-filter.spec.js |
Tests record-list filters. |
tests/e2e/settings-save.spec.js |
Tests settings persistence. |
tests/e2e/setup/setup.js |
Ensures network activation and cleanup. |
tests/phpunit/Abilities_REST_Integration_Test.php |
Updates unknown-ability handling. |
tests/phpunit/Alerts_List_Test.php |
Derives the alert nonce path. |
tests/phpunit/connectors/Connector_Installer_Test.php |
Uses the displayed theme name. |
tests/wp-tests-config-wp-env.php |
Adds tests-cli database configuration. |
Suppressed comments (1)
package.json:57
- Composer installs this executable under
vendor/bin, but npm only addsnode_modules/.bintoPATH, so this command fails withphp-coveralls: not foundon a normal checkout. Invoke the existing Composer script, which resolves Composer binaries correctly.
"test-report": "php-coveralls --verbose",
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| * | ||
| * @param {string} [ipAddress] IP of the row this run added (TEST-NET-3). | ||
| */ | ||
| export function clearNetworkExcludeRulesViaWpCli( ipAddress = '203.0.113.44' ) { |
| "test-xdebug": "wp-env start --xdebug && wp-env run tests-cli --env-cwd=wp-content/plugins/stream -- vendor/bin/phpunit", | ||
| "test-one": "wp-env run tests-cli --env-cwd=wp-content/plugins/stream -- vendor/bin/phpunit --", |
| By default, tests have `WP_DEBUG` as false. To enable it, prefix the PHPUnit command with `WP_STREAM_TEST_DEBUG=yes`, for example: | ||
|
|
||
| ```sh | ||
| WP_STREAM_TEST_DEBUG=yes npm run test:php |
Add five specs plus a shared helper, keep Stream network-activated so workers can run in parallel, and isolate settings-ui and network-activated projects so shared settings saves and plugin toggles do not race.
Replace the custom Docker Compose stack with @wordpress/env so contributors get a standard WordPress multisite environment without maintained images or checked-in bootstrap files; align CI, PHPUnit, Playwright, and docs with the new workflow.
0067067 to
1a4675d
Compare
bartoszgadomski
left a comment
bartoszgadomski
left a comment
There was a problem hiding this comment.
@shadyvb Thank you for working on this PR! Please check AI-assisted inline comments below.
| * record. Concurrent saves race on the success notice, so they run in the | ||
| * serial `settings-ui` project instead of `chromium`. | ||
| */ | ||
| const settingsUiSpecs = [ |
There was a problem hiding this comment.
Race: settings-save.spec.js is not isolated.
It is not in settingsUiSpecs and it is not networkActivatedSpec, so it is not covered by the chromium testIgnore on line 68 and runs in chromium with fullyParallel: true (2 workers on CI, 4 locally) — while it mutates network-wide Stream settings (records TTL, cron tracking, exclude IP).
Concretely: network-activated-records.spec.js also runs in chromium, and its beforeEach calls clearNetworkExcludeRulesViaWpCli() with no argument. The helper's default parameter is '203.0.113.44' — the same IP settings-save saves and then asserts on after reload. A concurrent worker can delete that row between the save and the assertion.
Suggest adding '**/settings-save.spec.js' to settingsUiSpecs (it saves shared settings, which is exactly the reason that project exists), and changing the no-arg helper default to clear only empty rows so global cleanup cannot delete a rule another spec owns.
| </coverage> | ||
| <php> | ||
| <const name="WP_TEST_ACTIVATED_PLUGINS" value="advanced-custom-fields/acf.php,easy-digital-downloads/easy-digital-downloads.php,jetpack/jetpack.php,user-switching/user-switching.php,wordpress-seo/wp-seo.php,two-factor/two-factor.php"/> | ||
| <const name="WP_TEST_ACTIVATED_PLUGINS" value="advanced-custom-fields/acf.php,easy-digital-downloads/easy-digital-downloads.php,jetpack/jetpack.php,user-switching/user-switching.php,wordpress-seo/wp-seo.php,two-factor/two-factor.php,wp-email-logger/wp-email-logger.php"/> |
There was a problem hiding this comment.
Two things about force-activating wp-email-logger in the PHPUnit suites:
- It generates Stream records. On multisite the plugin stores its log via
update_site_option( 'network_emails_log', … ). Stream's Settings connector registersupdate_site_option, andcallback_updated_option()logs any option not in the smallis_option_ignored()list (_transient_*,_site_transient_*, trailing$, and three image/size options) —network_emails_logisn't excluded. So every mail sent during the multisite suite also writes asettings/updatedrecord whose value is the flattened email log. The suite passes today, but it is hidden coupling for any record count/ordering assertion, plus larger record meta. (Single-site is unaffected:callback_update_option()only logs for WP-CLI/customizer and only for options in$labels.) - Is it needed for PHPUnit at all?
alert-create.spec.jsdeliberately uses the Highlight alert type rather than asserting via Email Logger, so this dependency reads as E2E/manual-testing oriented. Leaving it out ofWP_TEST_ACTIVATED_PLUGINSwould avoid both the record noise and making the integration suite depend on themake.wordpress.orgzip being reachable.
For the record, the zip layout is fine — it contains wp-email-logger/wp-email-logger.php, so the path used here is correct.
| </coverage> | ||
| <php> | ||
| <const name="WP_TEST_ACTIVATED_PLUGINS" value="advanced-custom-fields/acf.php,easy-digital-downloads/easy-digital-downloads.php,jetpack/jetpack.php,user-switching/user-switching.php,two-factor/two-factor.php"/> | ||
| <const name="WP_TEST_ACTIVATED_PLUGINS" value="advanced-custom-fields/acf.php,easy-digital-downloads/easy-digital-downloads.php,jetpack/jetpack.php,user-switching/user-switching.php,wordpress-seo/wp-seo.php,two-factor/two-factor.php,wp-email-logger/wp-email-logger.php"/> |
There was a problem hiding this comment.
This also adds wordpress-seo/wp-seo.php, which was previously single-site only (it was in phpunit.xml but not here). tests/bootstrap.php still contains:
if ( ! is_multisite() ) {
activate_plugin( 'wordpress-seo/wp-seo.php' );
}Since the constant is applied through the site_option_active_sitewide_plugins filter, Yoast is now network-active for the whole multisite suite while the bootstrap still deliberately skips activating it there — the two now disagree.
Intentional? If yes, the bootstrap guard is misleading and should go (or get a comment); if it was just config alignment, this quietly changes what loads for every multisite test.
| "SCRIPT_DEBUG": true | ||
| }, | ||
| "lifecycleScripts": { | ||
| "afterStart": "wp-env run cli -- wp plugin activate stream wp-email-logger --network" |
There was a problem hiding this comment.
wp plugin activate stream assumes the plugin is mounted at wp-content/plugins/stream, but wp-env derives that name from the checkout directory, not the plugin slug. For a local source like ".", packages/env/lib/config/parse-source-string.js does const basename = path.basename( sourcePath ).
So a clone into stream-plugin/, stream-wp/, stream-2/, etc. mounts at wp-content/plugins/<dir name> and then:
- this
afterStartscript fails (streamis not installed), --env-cwd=wp-content/plugins/streamin thetest:*scripts fails,- the
launch.jsonpath mapping silently stops matching.
CI is unaffected (checkout dir = repo name), so this is a contributor sharp edge. Either resolve the directory name dynamically or document in contributing.md that the clone must be named stream.
| "test:php": "npm run cli -- composer test --working-dir=wp-content/plugins/stream-src", | ||
| "test:php-multisite": "npm run cli -- composer test-multisite --working-dir=wp-content/plugins/stream-src", | ||
| "test:php-unit": "wp-env start && wp-env run tests-cli --env-cwd=wp-content/plugins/stream -- composer test-unit", | ||
| "test:php": "wp-env start && wp-env run tests-cli --env-cwd=wp-content/plugins/stream -- vendor/bin/phpunit --no-coverage", |
There was a problem hiding this comment.
These now bypass the Composer scripts rather than delegating to them, which changes behavior beyond the container swap:
composer testisphpunit --coverage-text+make-clover-relative.php. With--no-coverageand no post-step,tests/reports/clover.xmlis never written — sotest-report(php-coveralls, line 57) has nothing to upload, and the coveralls badge in readme.md can't be refreshed from this path.test:php-multisite(line 52) re-declares inline whatcomposer test-multisitealready owns, so the multisite suite config now lives in two places.test-xdebug(line 55) andtest-one(line 56) each run a single suite, while their Composer counterparts run single-site and multisite.
Delegating to composer test / test-multisite / test-xdebug / test-one inside wp-env run tests-cli keeps one source of truth — test:php-unit on line 50 already does exactly that.
Minor: every test:* script prefixes wp-env start, and CI already starts the env in its own step, so a full npm run test boots wp-env four times. Probably part of why the job timeout went 10 → 20.
| * active; this call is synchronous. | ||
| */ | ||
| export function networkActivateStreamViaWpCli() { | ||
| const slugs = [ 'stream', 'stream-src' ]; |
There was a problem hiding this comment.
networkActivateStreamViaWpCli() has no call sites in this PR (only the definition), and the stream-src fallback is the Compose-era mount name that no longer exists under wp-env — plugins: ["."] mounts at the checkout directory name, never stream-src. So the fallback can only ever fail before rethrowing.
Either drop the function, or if it is kept for upcoming specs, derive the slug from the actual mount name rather than hardcoding two guesses.
| "/var/www/html/wp-content/plugins/stream": "${workspaceFolder}/build", | ||
| "/var/www/html": "${workspaceFolder}/local/public" | ||
| "/var/www/html/wp-content/plugins/stream": "${workspaceFolder}", | ||
| "/var/www/html": "${workspaceFolder}/local/public/wp" |
There was a problem hiding this comment.
wp-env doesn't serve core from local/public/wp — that path is the Composer (roots/wordpress) install from extra.wordpress-install-dir. The container's /var/www/html is wp-env's own WordPress under ~/.wp-env/<hash>/WordPress.
As written, Xdebug maps container core files onto a different WordPress copy, so stepping into core lands in files that only coincidentally match. The plugin mapping on line 13 is the one that matters here; I'd drop this entry (or point it at the wp-env directory).
|
|
||
| - name: Pull custom Docker images | ||
| run: docker compose pull wordpress | ||
| - name: Start wp-env |
There was a problem hiding this comment.
Leftovers now that GHCR and Compose are gone:
WORDPRESS_IMAGE_VERSION: php8.2(line 9) no longer has a consumer — PHP is pinned byphpVersionin.wp-env.json, and wp-env's env override isWP_ENV_PHP_VERSION(whichswitch-to:php*uses).packages: read(lines 19 and 73) was only needed for theghcr.iologin this PR removes.
Both are harmless, just dead config worth clearing while this file is already being touched.
shadyvb
changed the title
Fixes XWPENG-44, XWPENG-45, XWPENG-46, XWPENG-47.
Base branch:
ticket/XWPENG-43-phpunit-11(#1974). This PR stacks on the PHPUnit 11 migration and delivers local-dev migration, expanded E2E coverage, the PHP 8.2 minimum / 5.0.0 release alignment, Settings/DB hardening, and a Rector-driven modernization pass.Summary
XWPENG-45 — wp-env local dev & CI
@wordpress/env(.wp-env.json): multisite, Email Logger mu-plugin,lifecycleScripts.afterStartnetwork activation, andenv.tests.mappingsfor PHPUnit helper plugins.docker-compose*.yml,local/docker/,local/public/WordPress bootstrap) and thedocker-images.ymlworkflow.lint-and-test.yml, deploy workflows,contributing.md,tests/bootstrap.php, andtests/wp-tests-config-wp-env.phpfor wp-env.switch-to:php8.2|8.3|8.4npm scripts and documents MCP / Application Passwords on localhost.XWPENG-44 — Playwright E2E
alert-create,export-download,network-activated-records,records-filter,settings-save.tests/e2e/helpers/stream-plugin.js; updates existing specs andplaywright.config.jsfor wp-env URLs and setup.XWPENG-46 — PHP 8.2 minimum & 5.0.0 release
stream.php,class-plugin.php, andreadme.txt.require.php, PHPCStestVersion, CI matrix (8.2 / 8.3 / 8.4), andchangelog.md.Fail_Php_Version_Unit_TestandConnectors_Unit_Testcoverage for the version gate.XWPENG-47 — Settings filter boundaries & DB driver contract
wp_stream_settings_option_key,wp_stream_settings_options, andwp_stream_settings_option_fieldsso non-string / non-array filter returns fall back safely before typed-property assignment.DB_Driver::setup_storage(): Installon the interface and WPDB implementation (breaking for third-party drivers omitting the return type).Codebase modernization (Rector 2.6)
rector.phpandcomposer lint-rector/composer rectorscripts.classes/, connectors, alerts, and exporters; constructor property promotion; twoswitch→matchconversions.Plugin::boot()from the constructor.Admininto focused collaborators (Admin_Menu,Admin_Assets,Admin_Screen_Records,Admin_Screen_Settings,Admin_Ajax,Admin_Purge) with colocated hook registration and dedicated PHPUnit coverage.Commits on this PR (
ticket/XWPENG-43-phpunit-11..HEAD)ae576e711a4675dc0c21fd720956d6492c0932e5rector.php.47ed6f88classes/via Rector.ea122a51DB_Driver::setup_storagecontract.49cde6ba3a0f708c77ce1e093ad23fc113f24b07Plugin::boot()from the constructor.251a8f98ef7ac652Test plan
composer test-unit)npm run lint)npm run wp-env startand smoke-test admin URLs locallyChecklist
contributing.md).Release Changelog
DB_Driverimplementations must declaresetup_storage(): Install(XWPENG-47).Release Checklist
masterbranch.readme.txt.stream.php.Stable taginreadme.txt.classes/class-plugin.php.