Build tooling performance tests unstable

[getdave]

Description

When running a release it is necessary to get the performance results for the Plugin to compare against previous versions.

However the workflow for doing this is unreliable ~on CI~ on CI and local.

Expected behaviour

The workflow should run and be stable for all release commits. Instead it fails with different errors depending on environment.

Actual behaviour

The workflow is unstable - it sometimes runs successfully but often fails.

Examples

Running Locally

Try running the following locally:

bin/plugin/cli.js perf wp/5.7

You'll see it fail with an error about not being able to find the node #user_login. After much debugging I believe this is due to their being a PHP error on the WordPress instance which means you get the WordPress error screen

You can validate that by browsing to the WP test instance at http://localhost:8889/wp-login.php once the performance test fails.

Sure enough if you switch WP_DEBUG to true on the test WordPress docker instance (that's the one at port :8889 and not the one at port 8888) then you see the actual error:

Fatal error: Cannot declare class WP_Block_Template, because the name is already in use in /var/www/html/wp-content/plugins/55d6de96-6325-4b2b-8e09-dd01a5206917/lib/full-site-editing/class-wp-block-template.php on line 12

Looks like the Plugin from the wp/5.7 branch provides duplicate PHP classes which already exist in Core. Indeed, if I comment on the class mentioned in the error, it simply moves on to another duplicate PHP class and so on and so on. Looks like there's a whole bunch of them.

Running CI

I assume a similar error is at play when the tests fail on CI only in that instance the error is different:

Site Editor Performance › Loading

    TimeoutError: waiting for selector `.edit-site-visual-editor iframe` failed: timeout 30000ms exceeded

      109 |         } ).slice( 1 );
      110 |         await visitAdminPage( 'admin.php', query );
    > 111 |         await page.waitForSelector( '.edit-site-visual-editor iframe' );
          |                    ^
      112 |     },

Again I believe the attempt to visit the Site Editor page fails (due to the PHP error triggering the default WP error screen when WP_DEBUG is false in the test env) and thus the selector is not defined.

[getdave]

@youknowriad @ockham @priethor I've been looking at this for a while now and not getting much further so I'll circle back tomorrow.

Be aware this is holding up posting the "What's Next In Gutenberg 11.0.0` post which I have waiting on Make blog.

Any advice on how we could work around this or fix the issue would be greatly appreciated.

[ockham]

I think it might be okay to publish the post already, and add the perf test results later; but since this is public-facing and somewhat prominent, it'd be good to get confirmation from someone in the know (@youknowriad @priethor @mtias)

[ockham]

I'm not entirely sure about the duplicated PHP file theory, TBH. While the class-wp-block-template.php file is present in current WordPress master, it doesn't seem to have been there for 5.7. Indeed, I believe that we synced a number of these files to Core only after 5.7 was released.

It seems that an error like this in local testing could result from a wp-env environment not properly re-fetching after changing the Core version in .wp-env.json, or a local Core install used for testing not properly updating/rebuilding. The error in CI mighjt be different though 😅

[priethor]

I'd also recommend posting without the performance audit, as late-Thursday/early-Friday seems to be when publications echo new Gutenberg releases, and it's nice to have the official post before then (I learned from experience 🙃 )

[getdave]

@priethor Just a heads up that I'll be AFK for a while now so it might need someone else to pick up on this and update the "Whats new" post if we get these tests working again.

[hellofromtonya]

The duplicated PHP classes exist in Core 5.8+, but not before 5.8. In 5.8, a version check is added to deactivate the Gutenberg plugin when its version is <= 10.7 (i.e. to avoid a fatal error).

It seems that an error like this in local testing could result from a wp-env environment not properly re-fetching after changing the Core version in .wp-env.json, or a local Core install used for testing not properly updating/rebuilding.

As the duplicated classes do not exist in 5.7, I agree with @ockham.

[ockham]

Per discussion with @youknowriad in Slack, I've applied the following patch to disable the Site Editor tests (for which we don't publish any performance numbers in our release announcement posts anyway):

diff --git a/bin/plugin/commands/performance.js b/bin/plugin/commands/performance.js
index 4f5f572774..ae3c77513c 100644
--- a/bin/plugin/commands/performance.js
+++ b/bin/plugin/commands/performance.js
@@ -291,7 +291,7 @@ async function runPerformanceTests( branches, options ) {
        }
        await runShellScript( 'npm run wp-env start', environmentDirectory );

-       const testSuites = [ 'post-editor', 'site-editor' ];
+       const testSuites = [ 'post-editor' /*, 'site-editor' */ ];

        /** @type {Record<string,Record<string, WPFormattedPerformanceResults>>} */
        let results = {};

I've then locally run

./bin/plugin/cli.js perf --ci wp/5.7 release/10.9 release/11.0 --wp-version 5.7

(adapted from this line).

This gave me the following results:

┌──────────────────┬─────────────┬──────────────┬──────────────┐
│     (index)      │   wp/5.7    │ release/10.9 │ release/11.0 │
├──────────────────┼─────────────┼──────────────┼──────────────┤
│       load       │ '5506.4 ms' │ '4686.6 ms'  │ '4816.4 ms'  │
│       type       │ '29.79 ms'  │  '29.54 ms'  │  '29.82 ms'  │
│     minType      │ '25.83 ms'  │  '25.85 ms'  │  '25.38 ms'  │
│     maxType      │ '63.55 ms'  │ '196.34 ms'  │ '207.94 ms'  │
│      focus       │ '67.44 ms'  │  '69.54 ms'  │  '70.28 ms'  │
│     minFocus     │ '54.94 ms'  │  '44.33 ms'  │  '43.83 ms'  │
│     maxFocus     │ '98.77 ms'  │ '195.72 ms'  │ '200.15 ms'  │
│   inserterOpen   │ '299.8 ms'  │ '364.77 ms'  │ '368.81 ms'  │
│ minInserterOpen  │ '266.56 ms' │  '288.4 ms'  │ '285.69 ms'  │
│ maxInserterOpen  │ '459.85 ms' │ '782.09 ms'  │ '792.03 ms'  │
│  inserterHover   │ '23.81 ms'  │  '18.09 ms'  │  '19.4 ms'   │
│ minInserterHover │ '21.46 ms'  │  '14.71 ms'  │  '15.62 ms'  │
│ maxInserterHover │ '30.88 ms'  │  '23.16 ms'  │  '28.37 ms'  │
└──────────────────┴─────────────┴──────────────┴──────────────┘

I only have Author privileges on make/core, so I can't edit Dave's announcement post. @priethor (or whoever reads this first and has the required privileges), would you mind adding the relevant numbers to the post?

[ockham]

As for the general instability of performance tests, I have a PR that seeks to move much of their logic into the GHA workflow in order to cut back a bit on redundant overhead. Maybe this could help with the instability also 🤞 Perhaps someone can pick up work on that while I'm away 🙂

[priethor]

would you mind adding the relevant numbers to the post?

Done! 🎉

[Mamaduka]

Is this still an issue?

Performance tests seem more stable after @WunderBart migrated them to Playwright.