Testing: Add an e2e test to check the interactions in write mode #65819
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
youknowriad
added
the
[Type] Automated Testing
youknowriad
requested review from
getdave,
oandregal,
jorgefilipecosta and
ntsekouras
|
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. |
22 tasks
|
Size Change: +11 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
kevin940726
reviewed
Oct 2, 2024
packages/e2e-test-utils-playwright/src/editor/switch-editor-tool.ts
Outdated
Show resolved
Hide resolved
| * @param this | ||
| * @param label The text string of the button label. | ||
| */ | ||
| export async function switchEditorTool( this: Editor, label: string ) { |
There was a problem hiding this comment.
Nit: Prefer using unions of string literals in types since we only have two options for now. Something like 'Design' | 'Write'.
There was a problem hiding this comment.
I'm hesitant, someone could decide to test another language.
There was a problem hiding this comment.
They're only types so I guess they can always bypass that. We can also add | string to the end to allow arbitrary strings.
| name: 'Editor settings', | ||
| } ); | ||
| await expect( | ||
| editorSettings.locator( '.block-editor-block-card__title' ) |
There was a problem hiding this comment.
Using class name selectors is not recommended. Could we instead select it by getByRole and optionally with a level field here?
There was a problem hiding this comment.
I know but for me, this is not easy to pinpoint and avoid random failures when we add headings... I prefer the selector here.
There was a problem hiding this comment.
I think there's already a heading in place? What do you mean by pinpointing random failures? I'd appreciate if we could follow the established best practices whenever possible and add comments if we could not. Thanks! 🙏
There was a problem hiding this comment.
The h2 is not enough to say that I'm targeting the "title" of the inspector panel. There could be more h2 in the inspector panel, in fact there are already. (content).
We could theoretically say h2 > eq( 0 ) but that's not very stable, any random design change can break that. (moving titles around).
So for me, in this case, I prefer a non ambiguous assertion rather than a generic one.
I agree on following best practices, but we need to understand what's important and why they exist.
I'm happy to update If there's a less ambiguous approach using that doesn't use selectors.
There was a problem hiding this comment.
A common workaround practice throughout the codebase is using expect( getByRole('heading', {name: 'Group', level: 2}) ).toBeVisible().
There was a problem hiding this comment.
I'd echo Riad about until fixing the a11y issue, we should stick with that classname for now and unblock this PR. Let's create an issue for this.
There was a problem hiding this comment.
No need to block this PR. But we should raise a followup Issue as suggested with the appropriate labels to get Accessibility feedback.
There was a problem hiding this comment.
IMO, getByRole is always preferred, in cases when it doesn't apply, we should still fallback to alternatives like getByTestId than class name selectors. I don't understand the argument of using class name selectors being more "solid". TBH, this has happened too many times that this is becoming frustrating to me. Could we at least add a comment above indicating that this is an edge case so that people won't accidentally follow the practice? I don't think it should block this PR, but I think this type of issue is still important, especially given that it just flagged a potential accessibility concern here by trying to avoid the class name selectors.
There was a problem hiding this comment.
I'll add a comment. I'm not sure we used test ids before.
There was a problem hiding this comment.
Thanks for the comment.
Nit: the wording isn't quite accurate and should read (emphasis added for clarity):
// Ideally we should **_NOT_** be using CSS selectors
youknowriad
force-pushed
the
add/write-mode-e2e-test
branch
from
30071fb
to
f158e0e
Compare
|
Flaky tests detected in f158e0e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11141739869
|
ntsekouras
approved these changes
Oct 4, 2024
There was a problem hiding this comment.
LGTM, thanks!
We need these kind of tests sooner than later. Let's create an issue for this and ship this.
kevin940726
reviewed
Oct 7, 2024
| @@ -104,6 +104,9 @@ test.describe( 'Write/Design mode', () => { | |||
| name: 'Editor settings', | |||
| } ); | |||
| await expect( | |||
| // Ideally we should be using CSS selectors | |||
There was a problem hiding this comment.
"not" be using CSS selectors? 😅
There was a problem hiding this comment.
It's fixed in trunk.
karthick-murugan
pushed a commit
to karthick-murugan/gutenberg
that referenced
this pull request
Nov 13, 2024
…dPress#65819) Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: kevin940726 <kevin940726@git.wordpress.org> Co-authored-by: getdave <get_dave@git.wordpress.org> Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org>
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.
Related #60021
What?
This PR adds a first e2e test to check the different interactions in the "write" mode.
Starting with inspector and mouse selection.