-
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 Utils: Use frameLocator for retrieving editor canvas #54911
Changes from all commits
687d6ea
a755917
0260da0
e41131e
405b447
5a9bcfa
51a3a75
d7d05d1
683f209
53fcf97
5cdaf1d
17ff61f
730e9d8
08c9b6f
6dcda8a
9378166
5a8f3ef
1262f39
9ca19cd
c5756bb
17fc072
eacc4fd
5bf6ffd
7691627
7851a01
0c9651a
efd7918
72e4fb5
b0c82ea
b84db19
6efac5c
bab65f8
fd96c75
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
/** | ||
* Internal dependencies | ||
*/ | ||
import type { Editor } from './index'; | ||
|
||
/** | ||
* Switches to legacy (non-iframed) canvas. | ||
* | ||
* @param this | ||
*/ | ||
export async function switchToLegacyCanvas( this: Editor ) { | ||
await this.page.waitForFunction( () => window?.wp?.blocks ); | ||
|
||
await this.page.evaluate( () => { | ||
window.wp.blocks.registerBlockType( 'test/v2', { | ||
apiVersion: '2', | ||
title: 'test', | ||
} ); | ||
} ); | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -30,7 +30,9 @@ test.describe( 'Buttons', () => { | |||||||||||||||||
editor, | ||||||||||||||||||
page, | ||||||||||||||||||
} ) => { | ||||||||||||||||||
await editor.canvas.click( 'role=button[name="Add default block"i]' ); | ||||||||||||||||||
await editor.canvas | ||||||||||||||||||
.locator( 'role=button[name="Add default block"i]' ) | ||||||||||||||||||
.click(); | ||||||||||||||||||
Comment on lines
+33
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Nit: I have previously been advised to prefer There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.keyboard.type( '/buttons' ); | ||||||||||||||||||
await page.keyboard.press( 'Enter' ); | ||||||||||||||||||
await page.keyboard.type( 'Content' ); | ||||||||||||||||||
|
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 we should keep this. The missing lazy evaluation from
editor.canvas
wasn't the only reason for this safeguard.Why?
createNewPost
call. Inherited from Puppeteer utils. It could count as a breaking change.Examples
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
I propose this version:
In this alternative, the function waits until the
window.wp.data
resource is ready before proceeding. The immediate use ofevaluate()
presupposes the availability ofwindow.wp.data
, effectively moving the responsibility of ensuring its presence outside this function. To illustrate:If the responsibility to check resource availability lies within
createNewPost()
, the secondinsertBlock()
call could be unstable. However, ifinsertBlock()
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: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.
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.