Migrate Classic block tests to Playwright #46689
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
Mamaduka
requested review from
talldan and
kevin940726
and removed request for
ntwb,
ajitbohra and
nerrad
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(); |
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?
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.
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.
There was a problem hiding this comment.
Sounds good. Should I add this as a separate test spec or include it in this file?
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?
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 ) => { |
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 ) { |
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?
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(); |
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
| } ); | ||
| await expect( createGallery ).not.toBeDisabled(); | ||
| await createGallery.click(); | ||
| await page.click( 'role=button[name="Insert gallery"i]' ); |
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(); |
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).
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.
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.
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]' | ||
| ); |
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]' ) |
There was a problem hiding this comment.
Also getByRole here, and a couple more below! 😅
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.
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