Skip to content
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.

Sign up for GitHub

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

Jump to bottom

E2E: Fix canvas waiter in visitSiteEditor #61816

Merged
merged 2 commits into from
May 21, 2024
Merged

E2E: Fix canvas waiter in visitSiteEditor #61816

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5d7b39f
Ensure we're on a page where canvas is present
WunderBart May 21, 2024
4adcee6
Account for a plain site editor visit
WunderBart May 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 18 additions & 17 deletions packages/e2e-test-utils-playwright/src/admin/visit-site-editor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,31 +37,32 @@ export async function visitSiteEditor(
query.set( 'canvas', canvas );
}

const canvasLoader = this.page.locator(
// Spinner was used instead of the progress bar in an earlier version of
// the site editor.
'.edit-site-canvas-loader, .edit-site-canvas-spinner'
);

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( () => {} );

/**
* @todo This is a workaround for the fact that the editor canvas is seen as
* ready and visible before the loading spinner is hidden. Ideally, the
* content underneath the loading overlay should be marked inert until the
* loading is done.
*/
await canvasLoader.waitFor( {
state: 'hidden',
// 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,
} );
if ( ! query.size || postId || canvas === 'edit' ) {
Copy link
Contributor

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.

Copy link
Member Author

@WunderBart WunderBart May 23, 2024 •
edited
Loading

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?

Copy link
Contributor

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

const canvasLoader = this.page.locator(
// Spinner was used instead of the progress bar in an earlier
// version of the site editor.
'.edit-site-canvas-loader, .edit-site-canvas-spinner'
);

// 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' } );
Copy link
Member

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?

Copy link
Member Author

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. 😅

await canvasLoader.waitFor( {
state: 'hidden',
// 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,
} );
}

if ( ! options.showWelcomeGuide ) {
await this.editor.setPreferences( 'core/edit-site', {
Expand Down
Loading