E2E Utils: Use frameLocator for retrieving editor canvas #54911
Conversation
|
@WunderBart, I just remembered this conversation #51824 (comment). Just in case, it's helpful. |
It is, thanks! Some useful context there, like the issue with replacing |
|
Size Change: +5.03 kB (0%) Total Size: 1.65 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in bab65f8. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6416319195
|
|
Fantastic work, @WunderBart! Do you mind listing the major changes in this PR so I can watch for those? I'll try to review it this week. |
Thanks, @Mamaduka! I've listed the changes in the description. Hope it helps! |
There was a problem hiding this comment.
Awesome work! I hope that you didn't just change these files by hand because that's a lot of work 😱 . Glad to see that it's possible, and perhaps not too late to make the switch!
The .locator( ':root' ) is a bit weird but I think it's not too bad. I just have to rethink canvas as a locator and not a frame now in my head.
| if ( this.useLegacyCanvas ) { | ||
| return this.page; | ||
| } |
There was a problem hiding this comment.
I wonder if we still need this. If the user has to opt-in to legacy canvas then they should be able to just use page than editor.canvas?
This also prevents the typescript awkwardness with FrameLocator | Page which doesn't really extend each other 😅 .
There was a problem hiding this comment.
This assumes that switching to legacy canvas is intentional, but that's not always true. Can we still return the this.page page as a fallback?
There was a problem hiding this comment.
I might be missing something, but won't useLegacyCanvas only be true if we call switchToLegacyCanvas? If the user opts in to call switchToLegacyCanvas then I guess we can expect them to use page instead.
There was a problem hiding this comment.
Let's say the consumer enables the metabox plugin in the test (which forces the editor to render legacy canvas) and forgets to call switchToLegacyCanvas; this won't work.
It would be great to hide editors' implementation details for legacy canvas vs. iframe. It shouldn't be noticeable for people writing tests or using the editor, IMO.
There was a problem hiding this comment.
I don't like it as well and would be more than happy to remove it! 😄 There might be an issue with some Editor utils that reference canvas (e.g. selectBlocks() IIRC) while running against the legacy one, but it might as well just pass - will check it! 🤞
There was a problem hiding this comment.
It would be great to hide editors' implementation details for legacy canvas vs. iframe. It shouldn't be noticeable for people writing tests or using the editor, IMO.
This is also a good point. Unfortunately, I don't think automatically "piercing" through iframe boundaries can be done trivially in Playwright, yet (I don't know really, I haven't given it any thoughts 😅). There's a trade-off to make here, and I would personally prefer predictable types and autocomplete for package-level utility here.
There was a problem hiding this comment.
I missed a part of this exchange - GH is not great at refreshing discussions. 😑
Anyway, I've been on edge with this one because, on one side, there's the desirable autocomplete without the type ambiguity (FrameLocator | Page), and on the other side, there's the implementation detail or the potentially broken editor utils that depend on the canvas reference (like the aforementioned selectBlocks().
As for the solution, we can't do it the old "page fallback" way because frameLocator doesn't return null like the frame does – it will time out if not found. The only solution that worked for me (considering we want to hide the implementation detail) has been turning the canvas into an async getter and evaluating the parent element by wrapping framed and unframed locators with Promise.any(). Unfortunately, it would be still far from ideal as it also would:
- involve a hefty refactor caused by the inability to chain from
editor.canvas(it would become(await editor.canvas()).getByRole(.... - still mess with the canvas types.
|
Do you think the reported flaky tests are related? The most recent reports seem to only happen on this branch. 🤔 If we can fix it here it will be best, but I think we can also fix them in follow-ups. |
|
@Mamaduka, just a heads up as I've added more examples to the changes involved! 🙌 |
| await editor.canvas | ||
| .locator( 'role=button[name="Add default block"i]' ) | ||
| .click(); |
There was a problem hiding this comment.
Sidenote: This is a really common pattern; I wonder if we should extract it into a utility method.
There was a problem hiding this comment.
| await editor.canvas | |
| .locator( 'role=button[name="Add default block"i]' ) | |
| .click(); | |
| await editor.canvas | |
| .getByRole( 'button', { | |
| name: "Add default block" | |
| }) | |
| .click(); |
Nit: I have previously been advised to prefer getByRole wherever possible.
There was a problem hiding this comment.
It's probably not worth refactoring old code for this standard, especially if we plan to extract this in helper.
Both ways are fine, but using RTL-style locators is preferred.
| await page.evaluate( () => { | ||
| window.wp.blocks.registerBlockType( 'test/v2', { | ||
| apiVersion: '2', | ||
| title: 'test', | ||
| } ); | ||
| } ); |
There was a problem hiding this comment.
I assume there's no longer a need for a legacy canvas switch. There're some left overs, so removal is appreciated.
There was a problem hiding this comment.
Some tests still fail when running against iframed canvas, so I'd leave the switch. I tried migrating most of the legacy tests to iframed canvas, but it wasn't possible for some of them. For example, cancels dragging blocks from the global inserter by pressing Escape in inserting-blocks.spec.js does not hide the insertion point after pressing Escape, no matter what I do. It seems to be an issue with Playwright, because the tested functionality works as expected 100% of the time when testing manually.
| // Wait for both iframed and non-iframed canvas and resolve once the | ||
| // currently available one is ready. To make this work, we need an inner | ||
| // legacy canvas selector that is unavailable directly when the canvas is | ||
| // iframed. | ||
| await Promise.any( [ | ||
| this.page.locator( '.wp-block-post-content' ).waitFor(), | ||
| this.page | ||
| .frameLocator( '[name=editor-canvas]' ) | ||
| .locator( 'body > *' ) | ||
| .first() | ||
| .waitFor(), | ||
| ] ); |
There was a problem hiding this comment.
I think we should keep this. The missing lazy evaluation from editor.canvas wasn't the only reason for this safeguard.
Why?
- Expectation that editor canvas is loaded when performing an action after the
createNewPostcall. Inherited from Puppeteer utils. It could count as a breaking change. - Some of the test patterns don't use locators for creating blocks. See examples below.
Examples
// Insert a block using `editor` utils. Was flaky before https://github.com/WordPress/gutenberg/pull/51824.
await admin.createNewPost();
await editor.insertBlock( { name: 'core/heading' } );
// Add blocks using writing flow "shortcut":
await admin.createNewPost();
await page.keyboard.press( 'Enter' );
await page.keyboard.type( 'Content' );The last pattern was very common in Puppeteer but became flaky after #48286 (before #51824), so we had to switch to editor.canvas.click( 'role=button[name="Add default block"i]' ).
There was a problem hiding this comment.
Believing that the entire editor is immediately ready upon invoking callingNewPost(), especially considering editor's dynamic nature, can be a risky assumption. Instead of making such assumptions, we should directly ensure that the resources we intend to use are available at the time of reference in the context where the resource is accessed.
From the sample you shared, it seems more logical to adjust the insertBlock() function.
Rather than having:
async function insertBlock(
this: Editor,
blockRepresentation: BlockRepresentation
) {
await this.page.evaluate( ( _blockRepresentation ) => {
//...
window.wp.data.dispatch( 'core/block-editor' ).insertBlock( block );
}, blockRepresentation );I propose this version:
async function insertBlock(
this: Editor,
blockRepresentation: BlockRepresentation
) {
await this.page.waitForFunction( ( _blockRepresentation ) => {
if ( ! window?.wp?.data ) {
return false;
}
//...
window.wp.data.dispatch( 'core/block-editor' ).insertBlock( block );
}, blockRepresentation );In this alternative, the function waits until the window.wp.data resource is ready before proceeding. The immediate use of evaluate() presupposes the availability of window.wp.data, effectively moving the responsibility of ensuring its presence outside this function. To illustrate:
await admin.createNewPost();
await editor.insertBlock( { name: 'core/heading' } );
await this.page.reload();
await editor.insertBlock( { name: 'core/heading' } );If the responsibility to check resource availability lies within createNewPost(), the second insertBlock() call could be unstable. However, if insertBlock() self-validates that its required resources are ready, we ensure a more robust outcome.
Does that perspective make sense?
There was a problem hiding this comment.
Does that perspective make sense?
For me, it does. I don't feel a strong attachment to previous behavior 😄
As I mentioned, this changes the "expected" behavior of createNewPost(), and not everyone actively follows changes to the e2e test environment.
I like your suggestion for improving the insertBlock utility 🚀
There was a problem hiding this comment.
As for the second example you provided, it involves some ambiguity because we assume the cursor is in the correct spot, but we don't really know what that spot is. Instead of shifting the burden of ensuring the cursor is where it should be to createNewPost() (which is not even the case, because we'd only wait for the editor body to be visible, which doesn't ensure cursor position!), I propose adding a step that explicitly waits for the cursor to be in the right spot:
await admin.createNewPost();
await expect(
editor.canvas.getByRole( 'textbox', { name: 'Add title' } )
).toBeFocused();
await page.keyboard.press( 'Enter' );
await page.keyboard.type( 'Content' );
There was a problem hiding this comment.
One DX benefit of removing this is it cleans up a little noise in the trace viewer that is caused by one of these locators timing out.
|
Thank you, @WunderBart! I left a couple of notes, but the changes look good in general ✅ I agree with @kevin940726. Fixing the related flaky test reports would be nice, but this can done after merging as well. |
It very well might be, @kevin940726. The locator itself is lazily evaluated but there still can be some flakiness from situations where we e.g. I'm checking the flakies now! |
Ensure cursor position before navigating
|
@kevin940726 PR-specific flakies seem to be gone. 🥳 There's one more that is not specific to this PR so I've opened a separate PR for it. |
What?
Improve E2E tests stability by referencing the editor canvas via lazily evaluated
frameLocatorinstead offrame. For example, we'll no longer need to explicitly wait for the canvas frame before callingeditor.canvas.*:Before
After
Related: WordPress/wordpress-develop#5333 (comment)
Bonus: Like this one, I was able to switch many other tests to be run against the iframed canvas. 🎉
Some still required legacy canvas to pass, but addressing them is out of the scope of this PR.
Major changes
This migration enforces some changes to how we work with
canvas, as it's no longer of typeFrame:Use
canvas.locator('selector').click()instead ofcanvas.click('selector').This will now be enforced, but it's also recommended as the direct
frame.clickhas been deprecated. This also includes other actions likefill,focus,hover, etc.Before
After
Use
editor.canvas.locator(':root').evaluate()instead ofeditor.canvas.evaluate(). For example:Before
After
Use
expect.poll(editor.canvas.evaluate())instead ofeditor.canvas.waitForFunction().This is not an equivalent, but I'm afraid there is no better alternative ATM. There's an open request for
frameLocator.waitForFunctionwe can +1, though. :)Before
After
Inner locators cannot use
frameLocator, so at this time it's not possible to match via thehas:property if the inner locator containsframeLocator.Before
After
For tests that require the legacy (non-iframed) canvas, use the
editor.switchToLegacyCanvas()utility.Before
After
Testing instructions
E2E tests should pass.