Migrate undo to Playwright #48701
Merged
Migrate undo to Playwright #48701
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
|
Size Change: +112 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in d1219a6. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4805307444
|
torounit
force-pushed
the
add/playwright-undo
branch
from
11a67ed
to
4a48a98
Compare
|
Thanks for the PR! I'll try to find some time to review it. Thanks for your patience! ❤️ |
kevin940726
reviewed
Apr 21, 2023
Comment on lines
87
to
93
| const after = await editor.getEditedPostContent(); | ||
|
|
||
| expect( after ).toBe( | ||
| `<!-- wp:paragraph --> | ||
| <p>before pause after pause</p> | ||
| <!-- /wp:paragraph -->` | ||
| ); |
There was a problem hiding this comment.
Nit: editor.getBlocks could be more readable, see https://github.com/WordPress/gutenberg/blob/trunk/docs/contributors/code/e2e/overusing-snapshots.md.
Suggested change
| const after = await editor.getEditedPostContent(); | |
| expect( after ).toBe( | |
| `<!-- wp:paragraph --> | |
| <p>before pause after pause</p> | |
| <!-- /wp:paragraph -->` | |
| ); | |
| await expect.poll( editor.getBlocks ).toMatchObject( [ { | |
| name: 'core/paragraph', | |
| attributes: { content: 'before pause after pause' }, | |
| } ] ); |
Same for other assertions below.
Comment on lines
62
to
63
| blockIndex, | ||
| editableIndex, |
There was a problem hiding this comment.
Nit: TBH, I have no idea what these mean 😅 . Could we instead use public APIs like wp.data.select( 'core/block-editor' ).getBlockIndex() to compute some of these fields?
However, I think we can keep these if we can't figure out a better way to test the same thing in this PR. If it's possible, I'd like to refactor it to better match the best practices, as shown in the PR #48035. I don't think that's a focus for this migration PR though!
There was a problem hiding this comment.
@kevin940726 Do you think we need editableIndex ?
There was a problem hiding this comment.
I don't know what it does or trying to achieve, TBH. Seems like every assertion is checking if it's equal to 0. I'm open to delete it if you also think it doesn't help with the test.
torounit
commented
Apr 26, 2023
| }, | ||
| ] ); | ||
| await expect.poll( undoUtils.getSelection ).toEqual( { | ||
| blockIndex: 0, |
There was a problem hiding this comment.
The value of blockIndex has been reduced by one because the order has been changed to return the order without the title of the post.
torounit
force-pushed
the
add/playwright-undo
branch
from
01e510b
to
15985ec
Compare
kevin940726
approved these changes
Apr 26, 2023
There was a problem hiding this comment.
This LGTM 👍 ! I'll tag @Mamaduka for another pair of eyes in case I missed something. Either way I think we can still merge it and iterate if necessary!
| await pageUtils.pressKeys( 'primary+a' ); | ||
| await pageUtils.pressKeys( 'primary+b' ); | ||
| await pageUtils.pressKeys( 'primary+z' ); | ||
| const activeElementLocator = page.locator( ':focus' ); |
torounit
commented
Apr 26, 2023
|
|
||
| await expect.poll( editor.getEditedPostContent ).toBe( '' ); | ||
| await expect.poll( undoUtils.getSelection ).toEqual( { | ||
| blockIndex: 0, |
There was a problem hiding this comment.
If undo is used to empty the block, getSelectionStart().offset will be undefined.
|
@kevin940726 Thank you for reviewing! I refactored getSelection now, can you check? |
kevin940726
approved these changes
Apr 26, 2023
Mamaduka
approved these changes
Apr 26, 2023
23 tasks
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?
Part of #38851
Why?
See this post for an overview of the migration.
How?
By following the migration guide.
Testing Instructions
npm run test:e2e:playwright -- test/e2e/specs/editor/various/undo.spec.js