E2E: Fix canvas waiter in visitSiteEditor

[WunderBart]

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 the actionTimeout is 5 seconds instead of 2 minutes, so we don't really notice that.

How?

By checking if the pageId or the canvas 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.

[ellatrix]

Great, last run was again 34mins 👍

[WunderBart]

Retriggering the whole thing, something's off with the E2E runner (likely unrelated to this PR). 🤞

[github-actions]

Flaky tests detected in 4adcee6.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9174535670
📝 Reported issues:

• [Flaky Test] Clicking on inserter while on zoom-out should open the patterns tab on the inserter #61806 in /test/e2e/specs/site-editor/zoom-out.spec.js

[github-actions]

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: WunderBart <bartkalisz@git.wordpress.org>
Co-authored-by: ellatrix <ellatrix@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[ellatrix]

Just wondering: why did it previously pass the check for the existence of the canvas loader, and then continue on to never disappear?

[WunderBart]

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

[youknowriad]

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.

[WunderBart]

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?

[youknowriad]

I can't think of a "or" condition that wouldn't break the expected behavior :P