Migrate list block test to Playwright #41555
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
|
Size Change: 0 B Total Size: 1.25 MB ℹ️ View Unchanged
|
|
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
|
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 :)! |
|
Thanks for the review. |
kevin940726
reviewed
Jun 20, 2022
| page, | ||
| } ) => { | ||
| await page.locator( 'role=button[name="Add default block"i]' ).click(); | ||
| await page.evaluate( () => delete window.requestIdleCallback ); |
There was a problem hiding this comment.
I think this test is for environments that do not have requestIdleCallback.
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
|
@kevin940726 I've fixed it, can you please check? |
Oops! I totally missed the notification! I'll review it today 🙇♂️ |
kevin940726
reviewed
Jul 4, 2022
|
Hi! Did you see #41555 (comment) and #41555 (comment)? Other than that this looks pretty awesome! |
|
Thanks! |
|
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
|
Thanks for all your reviews!!! |
|
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 |
|
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). |
|
@kevin940726 Unless you change the list block and all these are invalidated. |
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. Migrate list.test.js to Playwright.
Why?
See this post for an overview of the migration.
How?
By following the migration guide.
Testing Instructions