-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Migrate 'block editor keyboard shortcuts' e2e tests to Playwright #57422
Conversation
Co-authored-by: Alvi Tazwar <55917380+alvitazwar@users.noreply.github.com>
// Duplicate via keyboard. | ||
await pageUtils.pressKeys( 'primaryShift+d' ); | ||
|
||
await expect.poll( editor.getBlocks ).toMatchObject( [ |
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'm not sure if it's an issue with the editor.getBlocks
method or a bug in the editor, but after duplication, the newly inserted blocks content
attribute is undefined
.
I've omitted attributes in this test since it's only testing the event propagation via portals. I think it's worth to look into this separately.
P.S. The assertion works correctly if I insert another block.
Example test case
test( 'duplication test', async ( { editor, page, pageUtils } ) => {
await editor.canvas
.getByRole( 'button', { name: 'Add default block' } )
.click();
await page.keyboard.type( '1st' );
// Duplicate via keyboard.
await pageUtils.pressKeys( 'primaryShift+d' );
// Insert another block manually.
await page.keyboard.press( 'ArrowDown' );
await page.keyboard.press( 'Enter' );
await expect.poll( editor.getBlocks ).toMatchObject( [
{
name: 'core/paragraph',
attributes: { content: '1st' },
},
{
name: 'core/paragraph',
attributes: { content: '1st' },
},
{
name: 'core/paragraph',
attributes: { 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 can't reproduce this. The content
attribute is correctly showing for me 🤔 .
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.
Try removing the // Insert another block manually.
part of the example test.
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 can reproduce this after re-pulling the latest trunk.
It seems like a side-effect of #43204. After duplication, they shared the same RichTextData
content and triggered the loop-detection optimization of toMatchObject
, and bailed out comparing it. I don't know why inserting a new block fixes it though.
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 guess we can add a check in editor.getBlocks()
to automatically convert RichTextData
to plain string
for testing purposes.
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.
cc @ellatrix
Size Change: 0 B Total Size: 1.69 MB ℹ️ View Unchanged
|
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.
LGTM. Thanks!
*/ | ||
const { test, expect } = require( '@wordpress/e2e-test-utils-playwright' ); | ||
|
||
async function addTestParagraphBlocks( { editor, page } ) { |
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.
Nit: What do you think if we just use editor.setContent
or createNewPost
with the content
option? If it's easier to manage focus this way then nvm.
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 forgot about the editor.setContent
😅 I think it should work; IIRC, the resetBlocks
should focus on the last block, where these tests expect it.
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 was mistaken. The resetBlocks
doesn't handle the focus; the replaceBlocks
does. I guess we can update the setContent
to focus on the last inserted block, but that's probably out of the scope of this PR.
} ); | ||
} | ||
|
||
test.describe( 'Block editor keyboard shortcuts', () => { |
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.
Nit: I think a lot of these tests can be grouped into one to speed up the process. Especially we use mostly the same setup for each test, and each test only does one assertion. It's not a requirement for this migration though!
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.
We might need to adjust test shortcuts handling through portals in the same tree
tests, but it should be doable.
What's the preferred method for having multiple checks in a single file, test.step
or custom failure messages?
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.
What's the preferred method for having multiple checks in a single file,
test.step
or custom failure messages?
Either! Whichever looks the best to you 😉.
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.
Let's skip this for now. The whole suite runs in less than 10s.
I thought I could use editor.setContent
to reset to the content state, but #57422 (comment) will complicate tests.
// Duplicate via keyboard. | ||
await pageUtils.pressKeys( 'primaryShift+d' ); | ||
|
||
await expect.poll( editor.getBlocks ).toMatchObject( [ |
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 can't reproduce this. The content
attribute is correctly showing for me 🤔 .
What?
Part of #38851.
Supersedes and closes #50930.
PR migrates
block-editor-keyboard-shortcuts.test.js
e2e tests to Playwright.Why?
See #38851.
How?
I've included the author of the previous PR so they get credit for their contribution.
Testing Instructions