E2E: Ensure SE canvas loader appears before waiting for it to disappear #61629
Conversation
|
Flaky tests detected in cda9613. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9065822238
|
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| await this.visitAdminPage( 'site-editor.php', query.toString() ); | ||
|
|
||
| // Try waiting for the canvas loader to appear first, so that the locator | ||
| // that waits for it to disappear doesn't resolve prematurely. | ||
| await canvasLoader.waitFor().catch( () => {} ); |
There was a problem hiding this comment.
What's the error we're expecting to catch here?
There was a problem hiding this comment.
In an unlikely case where the loader has already disappeared by the time we get to this line, it shouldn't fail the test – the action should continue executing. TBH, it hasn't happened to me so far, but it is possible so I think it's worth having this extra precaution as it doesn't really hurt us. Does that make sense?
There was a problem hiding this comment.
@ellatrix, I think this line is the cause of regression. Now, each Site Editor > Navigation test takes around 2 minutes to run locally, instead of 5 seconds when I comment out this line. Running the tests in UI Mode gives a better breakdown of locator timers.
@WunderBart, I also realized that the util expects every site editor page it visits to have a canvasLoader, which isn't true. The DataView routes have no loaders for the moment.
There was a problem hiding this comment.
For reference: #61816
|
Looks like this increased e2e tests times by quite a bit! The performance test increased by ~20 mins. Parent commit finished in 34 mins, while this one finished in 54 mins. |
| // Bigger timeout is needed for larger entities, like the Large Post | ||
| // HTML fixture that we load for performance tests, which often doesn't | ||
| // make it under the default timeout value. | ||
| timeout: 60_000, |
There was a problem hiding this comment.
What is 60_000? Is this a typo?
There was a problem hiding this comment.
It is called a numeric separator that helps readability.
There was a problem hiding this comment.
Lol, how is this the first time I hear about this after all these years 🫠
What?
When the Site Editor takes longer to load (e.g., due to large canvas content), the
visitSiteEditorpromise resolves prematurely as it incorrectly assumes the canvas loader has already disappeared. In reality, it hasn't even appeared yet. It can lead to, e.g., subsequent locator assertions timing out as their 10s timeout gets eaten up by the canvas loading time. This PR fixes that by attempting to wait for the loader element immediately after the page is loaded so that we can reliably wait for it to disappear.Failure example: trace.zip (from WooCommerce Blocks)
Testing Instructions
All tests should pass.