Skip to content
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.

Sign up for GitHub

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

Testing: Add an e2e test to check the interactions in write mode #65819

Merged
merged 3 commits into from
Oct 7, 2024
Merged

Testing: Add an e2e test to check the interactions in write mode #65819

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3265fdf
Testing: Add an e2e test to check the interactions in write mode
youknowriad Oct 2, 2024
f158e0e
Changes per review
youknowriad Oct 2, 2024
2bded86
Add a comment to the e2e test about the usage of the CSS selector
youknowriad Oct 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions test/e2e/specs/editor/various/write-design-mode.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,9 @@ test.describe( 'Write/Design mode', () => {
name: 'Editor settings',
} );
await expect(
// Ideally we should be using CSS selectors
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"not" be using CSS selectors? 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fixed in trunk.

// but in this case there's no easy role/label
// to retrieve the "selected block title"
editorSettings.locator( '.block-editor-block-card__title' )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using class name selectors is not recommended. Could we instead select it by getByRole and optionally with a level field here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know but for me, this is not easy to pinpoint and avoid random failures when we add headings... I prefer the selector here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A common workaround practice throughout the codebase is using expect( getByRole('heading', {name: 'Group', level: 2}) ).toBeVisible().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to block this PR. But we should raise a followup Issue as suggested with the appropriate labels to get Accessibility feedback.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a comment. I'm not sure we used test ids before.

Copy link
Contributor

@getdave getdave Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

).toHaveText( 'Group' );
await expect(
Expand Down
Loading