Have createNewPost wait for editor canvas contents
#51824
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stokesman
added
the
[Type] Automated Testing
|
Size Change: 0 B Total Size: 1.42 MB ℹ️ View Unchanged
|
stokesman
force-pushed
the
update/e2e-playwright-create-new-post
branch
4 times, most recently
from
945c6c4
to
bdc26b7
Compare
|
Flaky tests detected in c6925b2. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5465466492
|
stokesman
requested review from
kevin940726,
Mamaduka,
ntwb,
nerrad and
ajitbohra
as code owners
|
Haven't gone through the code, but I tested it quickly locally and it fixes #51933 for me! Thanks for working on this! |
kevin940726
reviewed
Jun 28, 2023
| ? this.page | ||
| : this.page.frameLocator( '[name=editor-canvas]' ); | ||
|
|
||
| await canvasRoot.locator( '.editor-styles-wrapper' ).waitFor(); |
There was a problem hiding this comment.
Could we wait for something more accessible than a style wrapper? I'm not sure what we should be looking for here though 🤔
There was a problem hiding this comment.
Ideally we can. Though, this is on par with what it replaces .edit-post-layout. I'll take another look and see if I can find something.
There was a problem hiding this comment.
Maybe .wp-embed-responsive? It's added on load for the iframe, but not for the legacy editor though.
There was a problem hiding this comment.
I've not been able to see anything that's clearly better. I'd started with the post title in my first experiments with this but had the thought that there could be a test for a custom post type that doesn't support a post title. I thought about the 'Add default block' button that's usually present but post types can have templates that won't include it. Maybe we don't and will never have tests for custom post types where this would be a problem.
Here's an idea. Add another option to createNewPost that's a string whose default is 'heading="Add title"'. If we do have any tests that are for post types without titles they’ll have to be updated to pass in something else there. I'm happy to try that but wasn't sure it's any better. WDYT?
EDIT: I'd originally suggested the option could be used instead of the legacyCanvas option but I think it wouldn't work.
There was a problem hiding this comment.
The post title field and default block appender depend too much on post type settings. The title can be disabled and post-type content locked.
What about using the method from visitSiteEditor?
if ( legacyCanvas ) {
await this.page.waitForSelector( '.edit-post-layout' );
} else {
// Wait for the iframed canvas contents to be rendered.
await this.page
.frameLocator( 'iframe[title="Editor canvas"i]' )
.locator( 'body > *' )
.first()
.waitFor();
}There was a problem hiding this comment.
I think that's better. Thanks George! See f585b1f. Style aside the main difference is avoiding waitForSelector because ElementHandle isn't encouraged in the docs.
kevin940726
reviewed
Jun 28, 2023
| 'role=document[name="Block: Test Inner Blocks Async Template"i] >> text=OneTwo' | ||
| ); | ||
| const blockWithTemplateContent = page | ||
| .frameLocator( '[name=editor-canvas]' ) |
There was a problem hiding this comment.
Could you explain why we need to use frameLocator here? Is it still needed if we wait for the page to load in createNewPost?
There was a problem hiding this comment.
It's for reuse after the page.reload() that follows. Now that this test runs in the iframed editor editor.canvas returns a Frame and the locator formed from that isn't reusable after a page reload.
So this can be done differently if there's a style that seems better to you or I can add a comment. I thought it might be ideal to consider doing a follow up to try having editor.canvas return a FrameLocator instead of a Frame and then this could revert back to using that.
There was a problem hiding this comment.
I thought it might be ideal to consider doing a follow up to try having editor.canvas return a FrameLocator instead of a Frame and then this could revert back to using that.
Yeah, I regret using frame in the first place 😅 . One thing to consider is that there are code that depend on some frame-specific API so it's not easy to switch to frameLocator now. I've been trying to find alternatives but haven't found any. Please share if you have an idea! 🙇
There was a problem hiding this comment.
One thing to consider is that there are code that depend on some frame-specific API so it's not easy to switch to frameLocator now
I'd noticed but I figured it should possible to refactor any dependent tests/code. It also looks like many of the Frame methods are deprecated so it might be good for scrubbing that in case we are using some. I don't know Playwright well enough to have a clue if there's any easier alternative.
There was a problem hiding this comment.
I couldn't find an alternative to evaluate or waitForFunction though, which I believe some existing code rely on?
There was a problem hiding this comment.
Thanks for those specifics. It does look like that is a sticking point 🤔.
|
Note: Some tests force legacy canvas by registering v2 blocks at the run time. We should evaluate them after this fix is merged. |
stokesman
force-pushed
the
update/e2e-playwright-create-new-post
branch
from
bdc26b7
to
f585b1f
Compare
|
@stokesman, do you mind rebasing this branch on top of the |
Mamaduka
approved these changes
Jul 4, 2023
stokesman
force-pushed
the
update/e2e-playwright-create-new-post
branch
from
f585b1f
to
f87d11f
Compare
|
The failing Playwright test should be resolved now - #52308, but will require another rebase. |
|
@stokesman, it looks like some unwanted commits show-up in PR after the last rebase. |
stokesman
force-pushed
the
update/e2e-playwright-create-new-post
branch
from
c49b1e9
to
c6925b2
Compare
Yes, that was strange. Those were only showing on this PR page while at the branch they weren't part of the commits ahead of trunk. I'd triple-checked the commands I ran and they were fine. Anyway, after another rebase it now looks right here too. Before that happened, I’d meant to highlight the last two commits as they were added since approval. |
|
Thank you, @stokesman. Let's merge this, and we can do follow-ups as needed. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Improvements to e2e testing (Playwright utils) and some test/plugin updates.
Right now, this only updates
createNewPostfor Playwright and so I don't know if it completely resolves #51933, because I suppose the Puppeteer version may need the same sort of update.Why?
On my modest laptop, it’s almost the rule that tests using
createNewPostget derailed by the late appearance of the editor canvas’s contents and waiting for them alleviates it. In CI, this doesn't seem to happen too often but better that it never does.How?
af06387 updates
createNewPostto wait on an element that's inside the iframe instead of an element outside of it.Doing so makes some other tests fail because they register blocks that trigger the non-iframed canvas.
bbe171a, d1f38a1, c9f7a12, 3e32458 update tests/plugins to register blocks with
apiVersion3so they'll use the iframed editor canvas.Then there was a final test failing that I couldn't find a way to have run with the iframed editor canvas. So I resorted to b937013 which allows a consumer to configure which canvas is awaited in
createNewPostand 3fdf738 which uses that option in the test for meta box saving.Further details
b937013 means that the updates to blocks’
apiVersionaren’t necessary but the work was already done and it seems okay to run those with the iframed canvas. I'd be fine with reverting those if we'd rather reduce the changes.Testing Instructions