Build/Test Tools: Make the WordPress installation E2E test repeatable - #13302
Build/Test Tools: Make the WordPress installation E2E test repeatable#13302anamwp wants to merge 5 commits into
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
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. |
|
Pushed a follow-up commit extracting wp_e2e_ into a single TEST_TABLE_PREFIX constant. Discussion continues on the Trac ticket. |
|
Suggested title: Thanks for digging into this @anamwp. I reproduced both failure modes, and the bounded navigation retry looks good. To get this closer to land in core: could you extract the table cleanup into a helper, run it before changing the prefix as well as during teardown, and fail when any The current patch still fails once when stale |
…es that break subsequent test runs.
`afterEach` reverted `wp-config.php`'s table prefix but never dropped the
`wp_e2e_*` tables the test's own install wizard creates, so any run after
the first found a pre-existing install and never reached the installer.
Also retries the initial navigation itself, since a single `page.goto('/')`
right after the config swap can occasionally observe a stale config on
some hosts.
See https://core.trac.wordpress.org/ticket/65982
…stant. Per review feedback on the Trac ticket, avoid repeating the `wp_e2e_` literal in both the config rewrite and the cleanup query — use one `TEST_TABLE_PREFIX` constant instead so the two can't drift out of sync. See https://core.trac.wordpress.org/ticket/65982
…il loudly if cleanup fails. Per review feedback on the Trac ticket: - Extract the `wp_e2e_*` table cleanup into a `dropE2eTables()` helper, used from both `beforeEach` and `afterEach` instead of duplicating it. - Run it before the prefix swap as well as after. Previously, stale tables left behind by an interrupted prior run caused this test to fail once and only self-heal on the next run, once `afterEach` caught up. - Check each `DROP TABLE`'s result and call `WP_CLI::error()` on failure. `wp eval` exits 0 even when a `$wpdb->query()` call inside it returns false, so a failed cleanup was previously invisible and could leave a broken install in place while still reporting green. See https://core.trac.wordpress.org/ticket/65982
5b32a5b to
b02645f
Compare
|
Thanks for the thorough review, @lancewillett ! Pushed a follow-up addressing all three points:
Let me know if you'd like anything else adjusted. |
… in afterEach. Found while manually testing the previous commit's fail-loudly behavior: if the pre-test dropE2eTables() call in beforeEach throws, wpConfigOriginal was never assigned, so Playwright's unconditional afterEach then crashed with `TypeError: ... Received undefined` trying to restore wp-config.php from it — masking the real cleanup error behind a confusing second one. Read wpConfigOriginal before the cleanup call instead, since that read is side-effect-free and can't itself be the thing that's failing. See https://core.trac.wordpress.org/ticket/65982
|
@lancewillett Went through additional manual testing beyond the code changes:
That last check surfaced a real bug, now fixed in a follow-up commit: since the cleanup call in beforeEach can throw, and Playwright always runs afterEach regardless, a cleanup failure was getting masked by a second crash (TypeError: ... Received undefined) in afterEach trying to restore wp-config.php from a variable that was never assigned. Fixed by reading the original config before the cleanup call instead of after. Pushed as a follow-up commit. |
|
Changes look good to me. One thing worth adding while you're in here. But not a blocker.
The test goes green while the checkout's Two lines close it: const wpConfigPatched = wpConfigOriginal.replace(
`$table_prefix = 'wp_';`,
`$table_prefix = '${ TEST_TABLE_PREFIX }';`
);
if ( wpConfigPatched === wpConfigOriginal ) {
throw new Error(
`wp-config.php does not contain the default table prefix. An interrupted run may have left it on '${ TEST_TABLE_PREFIX }'.`
);
}
writeFileSync( wpConfig, wpConfigPatched );This composes with the read-before-cleanup ordering you asked for: CI is unaffected — Adversarial review · |
… the test prefix. Per review feedback: beforeEach's dropE2eTables() call closed the leftover- tables failure mode, but opened a subtler one. If a run is killed after the prefix rewrite but before afterEach restores it, wp-config.php is left on TEST_TABLE_PREFIX. On the next run, `.replace()` finds no `$table_prefix = 'wp_';` to match and silently no-ops, dropE2eTables() has already cleared the tables, so the site looks freshly uninstalled under the already- stranded prefix — the test passes while the checkout stays broken. Before this PR the same state failed loudly, since the leftover tables blocked the redirect; the new cleanup made it silent instead. Now throws explicitly when the replace is a no-op. wpConfigOriginal is already read before this point, so afterEach still restores cleanly rather than crashing a second time. CI is unaffected, since env:install regenerates wp-config.php via `wp config create --force` on every run — this only matters for a local checkout that survives between runs. See https://core.trac.wordpress.org/ticket/65982
|
Good catch @lancewillett, thanks for spelling out the exact sequence. Applied your suggested guard as-is (the composition with the read-before-cleanup ordering worked exactly as you described — wpConfigOriginal is already set, so afterEach restores cleanly instead of crashing a second time). Verified: stranded wp-config.php on wp_e2e_ directly and confirmed the guard fires with your exact error message. (Couldn't drive this through the full Playwright run — a stranded config also breaks global-setup.js's own auth step before any individual test file runs, which matches your point that this is a real environment-breaking state, not just an install.test.js-local one.) Re-ran the normal happy path 3x afterward with no regression. |
afterEach in tests/e2e/specs/install.test.js reverted wp-config.php's table prefix but never dropped the wp_e2e_* tables the test's own install wizard creates. As a result:
This PR:
Testing
Isolation data — three consecutive runs of each variant, with wp_e2e_* tables dropped before each:
Both changes are independently necessary — neither one alone is sufficient to make the test reliably repeatable.
Trac ticket: https://core.trac.wordpress.org/ticket/65982
Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Sonnet 5
Used for: Diagnosing the root cause (including tracing a Playwright network trace and DB state to rule out a false lead), drafting the fix, and building the A/B isolation harness used to produce the testing data above. I reviewed the diagnosis, ran and verified the fix and the isolation tests myself, and take responsibility for the change.