Migrate multi-block selection E2E tests to Playwright #48035
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
WunderBart
added
[Type] Automated Testing
|
Size Change: 0 B Total Size: 1.34 MB ℹ️ View Unchanged
|
|
Flaky tests detected in 21b6549. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4319674233
|
|
@WunderBart Have you worked on this yet? I have some WIP code on my local branch. If you don't mind, I'd be happy to contribute to it as well :)! |
👋 @kevin940726, so far only what's committed here, so feel free to push what you got! 🚀 |
|
@WunderBart Nice! I think I've already migrated half of the test cases, so it would be nice if you could hold it up a little bit and help me review it once I'm done 😅 . |
kevin940726
force-pushed
the
migrate/playwright-multi-block-selection
branch
from
4e77743 to
245ad39
Compare
kevin940726
reviewed
Feb 19, 2023
| .toEqual( [ 3 ] ); | ||
| } ); | ||
|
|
||
| test( 'should deselect with Escape', async ( { |
There was a problem hiding this comment.
This was originally marked as a flaky test and skipped.
Comment on lines
+235
to
+242
| // FIXME: This doesn't seem to work anymore. | ||
| // await expect | ||
| // .poll( multiBlockSelectionUtils.getSelectedFlatIndices ) | ||
| // .toEqual( [] ); |
There was a problem hiding this comment.
Pressing escape no longer deselects the blocks. I guess we would want to mark this test as .fail and create an issue for it?
| ] ); | ||
| } ); | ||
|
|
||
| test( 'should copy and paste', async ( { page, editor, pageUtils } ) => { |
There was a problem hiding this comment.
As per the best practices of Playwright, this is now a combination of multiple similar test cases:
should copy and pasteshould place the caret at the end of last pasted paragraph (paste to empty editor)should place the caret at the end of last pasted paragraph (paste mid-block)should place the caret at the end of last pasted paragraph (replace)
| .toBe( 'Post title' ); | ||
| } ); | ||
|
|
||
| test( 'should multi-select in the ListView component with shift + click', async ( { |
There was a problem hiding this comment.
This is now a combination of two test cases because they share the same initial state:
should multi-select in the ListView component with shift + clickshould multi-select in the ListView component with shift + up and down keys
| await expect( | ||
| navButtons.nth( 3 ).getByRole( 'link', { includeHidden: true } ) | ||
| ).toBeFocused(); | ||
| // Press Up twice to highlight the second block. |
There was a problem hiding this comment.
The original comment suggested that it would "focus" the block, but in reality, it only "highlights" the block.
kevin940726
force-pushed
the
migrate/playwright-multi-block-selection
branch
from
e1e9623 to
137481b
Compare
WunderBart
commented
Feb 27, 2023
There was a problem hiding this comment.
This one's massive! 🥵 Awesome work, thank you @kevin940726! 🎉
I left just a few non-blocking comments. I'd approve but I can't since I've opened it 🤷
Comment on lines
+69
to
+75
| // Move to the middle of the first line. | ||
| await pageUtils.pressKeyTimes( 'ArrowUp', 4 ); | ||
| await page.keyboard.press( 'ArrowRight' ); | ||
| // Select mid line one to mid line four. | ||
| await pageUtils.pressKeyTimes( 'Shift+ArrowDown', 3 ); | ||
| // Delete the text to see if the selection was correct. | ||
| await page.keyboard.press( 'Backspace' ); |
There was a problem hiding this comment.
Should we combine pressKeyTimes and pressKeyWithModifier into a single pressKey util? We could then use only that, e.g.:
await pageUtils.pressKey( 'ArrowUp', { times: 4 } );
await pageUtils.pressKey( 'ArrowRight' ); // instead of page.keyboard.press(...
await pageUtils.pressKey( 'ArrowDown', { modifier: 'Shift', times: 3 } );
await pageUtils.pressKey( 'Backspace' );There was a problem hiding this comment.
Yeah! That sounds like a good idea! Probably should do it in another PR though.
Comment on lines
+87
to
+89
| await editor.canvas | ||
| .getByRole( 'button', { name: 'Add default block' } ) | ||
| .click(); |
There was a problem hiding this comment.
Any reason we're not inserting the block programmatically here as well?
Suggested change
| await editor.canvas | |
| .getByRole( 'button', { name: 'Add default block' } ) | |
| .click(); | |
| await editor.insertBlock( { | |
| name: 'core/paragraph', | |
| attributes: { content: '' }, |
There was a problem hiding this comment.
Because insertBlock doesn't place the caret at the end of the paragraph block. It's just a bit easier sometimes to manually type it than focusing and moving the caret after inserting the block.
kevin940726
approved these changes
Mar 3, 2023
There was a problem hiding this comment.
Thanks for the review @WunderBart ! I'll approve it myself then 😆 .
kevin940726
force-pushed
the
migrate/playwright-multi-block-selection
branch
from
20b26eb to
21b6549
Compare
ellatrix
reviewed
Aug 8, 2023
| await clickBlockAppender(); | ||
| await page.keyboard.type( '1' ); | ||
| await page.keyboard.type( '2' ); | ||
| await page.keyboard.press( 'ArrowLeft' ); |
There was a problem hiding this comment.
Unfortunately the new test is not doing the same thing. Restored it in #53418. Why not simply copy over the old test as is?
There was a problem hiding this comment.
Oops! I think I misunderstood the two separate .type() calls and thought that they were inserting two blocks. Thanks for catching that!
Why not simply copy over the old test as is?
Because it's not only a migration but also a refactor trying to fix the longstanding flakiness of the original test.
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?
Migrate multi-block selection E2E tests to Playwright.
Why?
How?
Testing Instructions
The output of the following should be all ✅: