Migrate list block test to Playwright#41555
Merged
Merged
Conversation
|
Size Change: 0 B Total Size: 1.25 MB ℹ️ View Unchanged
|
Member
Author
|
The e2e test passes on Github-Actions, but when I run it on my machine some tests fail. ( tests with names starting with "should undo asterisk transform with backspace" ) Also, it does not happen on my old machine (Mac Book Pro 2015). If I add |
torounit
force-pushed
the
refactor/list-block-e2e-playwright
branch
from
ff20239 to
cb2cb81
Compare
Member
|
Hi, thanks for the PR! Please follow the best practices while you're at it, especially with the part of using role selectors, so that we can keep the utils as simple to follow as possible 👍. While normally I think using snapshots is acceptable, this test has too many snapshots though maybe just inline them using Also, don't forget to remove the original test file and its snapshots too :)! |
Member
Author
|
Thanks for the review. |
kevin940726
reviewed
Jun 20, 2022
Member
kevin940726
left a comment
kevin940726
left a comment
There was a problem hiding this comment.
Thanks for working on this! Tests are looking good! There are some minor issues where we can follow the best practices to make it more reliable though.
| page, | ||
| } ) => { | ||
| await page.locator( 'role=button[name="Add default block"i]' ).click(); | ||
| await page.evaluate( () => delete window.requestIdleCallback ); |
Member
Author
There was a problem hiding this comment.
I think this test is for environments that do not have requestIdleCallback.
Member
There was a problem hiding this comment.
I see. Now that we're using Playwright, I think we can just remove this and enable running Safari for the test suite.
However, we don't have the necessary architecture for that yet. So I guess we can let this pass for now.
This code should probably stay in unit tests though, not sure why it's in e2e tests.
torounit
force-pushed
the
refactor/list-block-e2e-playwright
branch
from
8d8e9df to
6a3eb63
Compare
torounit
commented
Jun 22, 2022
torounit
commented
Jun 23, 2022
torounit
force-pushed
the
refactor/list-block-e2e-playwright
branch
from
5d5b78e to
efc4454
Compare
Member
Author
|
@kevin940726 I've fixed it, can you please check? |
Member
Oops! I totally missed the notification! I'll review it today 🙇♂️ |
kevin940726
reviewed
Jul 4, 2022
Member
|
Hi! Did you see #41555 (comment) and #41555 (comment)? Other than that this looks pretty awesome! |
Member
Author
|
Thanks! |
Member
|
There's a conflict, we can merge this after resolving it! 💯 |
torounit
force-pushed
the
refactor/list-block-e2e-playwright
branch
from
a5bedaa to
0f50011
Compare
Member
Author
|
Thanks for all your reviews!!! |
Member
|
Why have all snapshots be inlined? This has now become a pain to update! It's no longer possible to update the snapshots with the |
Member
|
Yeah, it'd be ideal if we could use However, I don't think it's that much of a pain though. It's slightly inconvenient, but not that bad IMO. Overusing snapshots seems like a bigger problem to me. IMHO, most existing snapshot tests are there mainly because we don't have other more helpful tools, and snapshot is the easiest and fastest. Now that Playwright has all these advanced assertions, I hope that we can start moving away from snapshots (if there's a better alternative). |
Member
|
@kevin940726 Unless you change the list block and all these are invalidated. |
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. Migrate list.test.js to Playwright.
Why?
See this post for an overview of the migration.
How?
By following the migration guide.
Testing Instructions