-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate table block tests to Playwright #41945
Conversation
Size Change: +40 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
f86db2c
to
ed7f216
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this. Looks good so far, just a few comments on minor things that can be improved.
Thanks for your review!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very well done!
But wow, there are a lot of snapshots here 😅. I'm gonna give it a pass since this is just a migration PR, but we should probably think of ways to reduce these snapshot usage in the future 😅.
I thought that too, but I don't see any easy way to avoid them. It is hard to look at the snapshots to determine if they're correct, because they're very complex. On the other hand, it's also hard to make assertions that a table is correct (that it has the right number of sections, rows and columns, and the right content in each cell). Snapshots definitely seem like the easiest way, the alternatives will probably be a bit convoluted (i.e. counting the number of cells in each row.) |
How about cheking the content string directly by Here is an example code: gutenberg/test/e2e/specs/editor/blocks/buttons.spec.js Lines 16 to 24 in 286e6be
|
I'm thinking of creating a snapshot-like helper specifically for this test suite: // GFM-style
await expect.poll( tableBlock.visualize ).toBe(
`
| This | Is | Table | Block |
`
);
// Unicode-style
await expect.poll( tableBlock.visualize ).toBe(
`
┌──────┬────┬───────┬───────┐
│ This │ Is │ Table │ Block │
└──────┴────┴───────┴───────┘
`
); So it's snapshot-like, but readable and not that detailed and complex. It might be too much work for a simple test like this though 😅. |
A better idea might be using visual comparison directly. But it requires architectural changes (running the browser in a docker environment). It might be worth the efforts in the long run though. |
@kevin940726 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Just a small nitpick left and then we can merge it 💯.
Thank you so much for checking out the details 😀 |
Nice work @t-hamano! |
What?
Part of #38851. Migrate
table.test.js
to Playwright.Why?
See this post for an overview of the migration.
How?
By following the migration guide.
Testing Instructions