-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
E2E: Fix canvas waiter in visitSiteEditor #61816
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, last run was again 34mins 👍
3d36395
to
4adcee6
Compare
Retriggering the whole thing, something's off with the E2E runner (likely unrelated to this PR). 🤞 |
Flaky tests detected in 4adcee6. 🔍 Workflow run URL: https:/WordPress/gutenberg/actions/runs/9174535670
|
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. |
|
||
// Wait for the canvas loader to appear first, so that the locator that | ||
// waits for the hidden state doesn't resolve prematurely. | ||
await canvasLoader.waitFor( { state: 'visible' } ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering: why did it previously pass the check for the existence of the canvas loader, and then continue on to never disappear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It only passed the check because there was an empty catch
for the scenarios in which the loader disappeared before the test reached that line. It was a premature optimization on my end – we didn't need it, and it only made things worse by swallowing the error that would prevent the original PR from being merged. 😅
// make it under the default timeout value. | ||
timeout: 60_000, | ||
} ); | ||
if ( ! query.size || postId || canvas === 'edit' ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this check is probably a bit fragile and not something that will stay consistent over time. A better check would be to check if the "preview area" exist in the DOM or something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, although checking if something exists is what we had before this PR, and it didn't work well. We were checking whether the loader was there, and if not, we were swallowing the waitFor()
promise error and continuing with the test. The downside of such a solution is that whenever the loader element is not there, we add the current actionTimeout
value to the test (before the error is swallowed), which is 10s for E2E tests and 120s for perf tests. We could explicitly fix that timeout for that waitFor()
call to, e.g., 5s, but it would still add 5s for no-loader situations and wouldn't guarantee stability.
IMO, the most stable solution for these situations is to use locator.or
or parallel locator waits with Promise.any
, which immediately returns the element first on the page. We need to know which element is there when the canvas is not there and vice-versa, though. For example:
await Promise.any([
page.locator('.canvas-loader').waitFor(),
page.locator('.some-panel-instead-of-canvas').waitFor(),
]);
I couldn't quickly trace an element like that when making this fix. Do you know any we could use here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't think of a "or" condition that wouldn't break the expected behavior :P
Co-authored-by: WunderBart <[email protected]> Co-authored-by: ellatrix <[email protected]>
Co-authored-by: WunderBart <[email protected]> Co-authored-by: ellatrix <[email protected]>
Reported in: #61629 (comment)
Related convo: https://wordpress.slack.com/archives/C02QB2JS7/p1716226087226579
What?
Fix the canvas loading bar waiter step so that it only applies when we open a page for editing, e.g., by passing the
pageId
param.Why?
The current implementation adds 2 minutes to each performance test that visits the site editor's page that doesn't load the canvas, e.g., the
All Templates
view. It also does that for E2E tests, only there theactionTimeout
is 5 seconds instead of 2 minutes, so we don't really notice that.How?
By checking if the
pageId
or thecanvas
query params are present, so that we know the intent is to actually edit content.Testing Instructions
All E2E tests should pass, and the total time of the performance tests should be reduced from ~50 to ~30 minutes.