Add e2e tests for keyboard interactions in DataViews ListView #61648
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
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
oandregal
added
[Type] Code Quality
|
Size Change: +255 B (+0.01%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
mcsf
approved these changes
May 14, 2024
test/e2e/specs/site-editor/dataviews-list-layout-keyboard.spec.js
Outdated
Show resolved
Hide resolved
youknowriad
approved these changes
May 14, 2024
Co-authored-by: Miguel Fonseca <150562+mcsf@users.noreply.github.com>
oandregal
force-pushed
the
add/e2e-test-for-list-view-keyboard
branch
from
f767c81 to
21a87ee
Compare
Mamaduka
reviewed
May 14, 2024
| ).toBeFocused(); | ||
|
|
||
| // Make sure the items have loaded before reaching for the 1st item in the list. | ||
| await page.getByRole( 'grid' ).waitFor(); |
There was a problem hiding this comment.
The waitFor is "anti-pattern" in PW tests, and we should try to avoid it when possible. Can we swap similar calls with toBeVisible() assertions?
There was a problem hiding this comment.
Happy to learn more about this and update it. I followed what we're doing in multiple other places: block directory, autosave, block bindings, multi-entity saving, editing widgets, etc. Could you expand on why is it an anti-pattern and in which situations should we avoid it?
There was a problem hiding this comment.
I think those are all copy pasted from the other tests or util methods. I should probably better to update those before it spreads even further :D
The waitFor isn't explicit, and it's unclear at a glance that the method also waits for the element to be visible. I think it's OK to use in test utils, where test flow or readability isn't essential.
The following examples do the same thing:
await page.getByRole( 'grid' ).waitFor()
// Explicit asertion. It's clear without a comment what the test is doing.
await expect( page.geByRole( 'grid' ) ).toBeVisible()This is also highlighter in the e2e test best practices - https://github.com/WordPress/gutenberg/blob/trunk/docs/contributors/code/e2e/README.md#make-explicit-assertions.
cc @WunderBart, @kevin940726, in case I missed something :)
There was a problem hiding this comment.
I don't have an opinion on a way or the other, happy to change it if that's preferred f4b9d6d
It sounds like we should make a lint rule to prevent people from using what our existing tests do :)
There was a problem hiding this comment.
Yeah, waitFor should be really rare, mostly in test utils. In Playwright, if we want to explicitly test the element is visible we should just use toBeVisible(). If we just want to wait for the element to load for whatever actions below, we shouldn't need to, Playwright auto-waits for us.
I don't think this is a major anti-pattern though, just a good-to-know tip and helps readability in the long term 😄. (Thank you @Mamaduka!)
There was a problem hiding this comment.
@WunderBart, thanks for sharing the case.
I personally like the idea. Should we continue discussions in an issue?
There was a problem hiding this comment.
What I don't like about the approach is that you no longer can easily run the e2e tests by just providing a URL, you need access to CI and that database which makes running the e2e tests against Core (with the plugin disabled) for instance harder or remote urls or things like that. Thought one solution could be to provide this "reset DB" step as a REST API or something.
There was a problem hiding this comment.
@Mamaduka, should I handle creating the issue? 😄
There was a problem hiding this comment.
If you don’t mind, @WunderBart 🙇
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.
Follow-up to #60805
What?
Adds a few e2e test for the keyboard interactions merged by #60805
Why?
We want to make sure these don't break.
How?
A few different tests that cover all scenarios targeted in the PR above:
How to test
Make sure tests pass in CI. They can be executed locally by
npm run test:e2e:playwright -- test/e2e/specs/site-editor/dataviews-list-layout-keyboard.spec.js.