Migrate Classic block tests to Playwright#46689
Merged
Merged
Conversation
Mamaduka
requested review from
kevin940726 and
talldan
and removed request for
ajitbohra,
nerrad and
ntwb
Mamaduka
added
the
[Type] Automated Testing
Mamaduka
commented
Dec 20, 2022
Comment on lines
+87
to
+89
| // Focus on the editor so that keyboard shortcuts work. | ||
| await galleryBlock.focus(); |
Member
There was a problem hiding this comment.
I think this is an actual bug and we should fix it instead of patching the test. I just opened an issue here: #46844.
In the meantime, I think maybe we should just skip the original test until we fix it upstream. WDYT?
Member
Author
There was a problem hiding this comment.
I think we shouldn't skip the test, considering this is a more general bug in the editor.
Member
There was a problem hiding this comment.
What do you think if we:
- Add a separate test specifically for the focus lost issue and mark it as failing (
test.fail). - Reference the failing test in the comment here before the
.focus()line to get this test pass.
Member
Author
There was a problem hiding this comment.
Sounds good. Should I add this as a separate test spec or include it in this file?
Member
There was a problem hiding this comment.
I think it belongs in a separate test file 👍 . Not sure where to put it though. Probably something like converting-blocks.spec.js?
Member
Author
There was a problem hiding this comment.
I noticed that focus loss doesn't happen when transforming the blocks. I think the focus loss after conversion is only the Classic block's issue.
Maybe I should fix it for the Classic block by focusing on the first block. What do you think?
Update: The focus loss only happens in the Gallery is last or only block after the transformation.
Mamaduka
commented
Dec 20, 2022
| } ) => { | ||
| // Based on docs routing diables caching. | ||
| // See: https://playwright.dev/docs/api/class-page#page-route | ||
| await page.route( '**', ( route ) => { |
Member
Author
There was a problem hiding this comment.
The playwright doesn't have page.setCacheEnabled; this is the best replacement I could find for now.
|
Size Change: 0 B Total Size: 1.33 MB ℹ️ View Unchanged
|
Mamaduka
commented
Dec 28, 2022
| ); | ||
| } | ||
|
|
||
| async upload( inputElement ) { |
Member
Author
There was a problem hiding this comment.
@kevin940726, now we have three places where we use a similar util method. Would it make sense to extract this into global media utility?
Member
Author
There was a problem hiding this comment.
Sounds good. I will follow-up with this.
Mamaduka
force-pushed
the
migrate/classic-block-tests
branch
from
9e0323a to
d5e88cc
Compare
Mamaduka
force-pushed
the
migrate/classic-block-tests
branch
from
d5e88cc to
e3daa84
Compare
|
Flaky tests detected in e3daa84. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/3988869018
|
WunderBart
reviewed
Jan 27, 2023
| await page.click( '.mce-content-body' ); | ||
| await page.keyboard.type( 'test' ); | ||
|
|
||
| await page.getByRole( 'button', { name: /Add Media/i } ).click(); |
Member
There was a problem hiding this comment.
Note
Just a heads up that thenamematching will be case-insensitive and search for a substring by default in Playwright >=1.28. For exact matching, theexactparam will need to be used.See https://playwright.dev/docs/api/class-page#page-get-by-role-option-name
WunderBart
approved these changes
Jan 27, 2023
Member
WunderBart
left a comment
WunderBart
left a comment
There was a problem hiding this comment.
Looks good, thanks! 🚀 Just a couple of nonblocking comments.
| } ); | ||
| await expect( createGallery ).not.toBeDisabled(); | ||
| await createGallery.click(); | ||
| await page.click( 'role=button[name="Insert gallery"i]' ); |
Member
There was a problem hiding this comment.
Can we also use the getByRole method here?
Suggested change
| await page.click( 'role=button[name="Insert gallery"i]' ); | |
| await page.getByRole( 'button' , { name: /Insert gallery/i } ).click(); |
Member
Author
There was a problem hiding this comment.
I think it's a matter of preference. I usually find page.click() simpler in similar cases.
We had a similar discussion here #45193 (comment).
Member
There was a problem hiding this comment.
Thanks for the context! I can see how the page.click() can be more straightforward in some cases. However, a good argument for discontinuing it is that those text role selectors are not in the documentation anymore, so it can be confusing for folks that don't have a lot of experience with Playwright-specific selectors. The getBy* locators are the officially recommended ones now.
Member
There was a problem hiding this comment.
Yeah, I just found out the other day that the role selector is not in the doc anymore. It'd be nice if we recommend using getByRole in our doc from now on too. I think we can also migrate all existing usages in one go during a playwright upgrade if that's feasible.
Member
Author
There was a problem hiding this comment.
Let's migrate all existing usages in one go during the next Playwright update.
Comment on lines
+82
to
+84
| const galleryBlock = page.locator( | ||
| 'role=document[name="Block: Gallery"i]' | ||
| ); |
Member
There was a problem hiding this comment.
Can we use getByRole here as well?
| // Check that you can undo back to a Classic block gallery in one step. | ||
| await pageUtils.pressKeyWithModifier( 'primary', 'z' ); | ||
| await expect( | ||
| page.locator( 'role=document[name="Block: Classic"i]' ) |
Member
There was a problem hiding this comment.
Also getByRole here, and a couple more below! 😅
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Supersedes #42625.
Migrates
classic.test.jstests to Playwright.Why?
To fix flaky tests and help migration efforts.
Closes #36123.
How?
See MIGRATION.md for migration steps.
Testing Instructions