-
Notifications
You must be signed in to change notification settings - Fork 198
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
Replace hard-coded strings in Playwright tests #4804
Conversation
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.
LGTM. Just one small requested change to expand on the comment about Playwright's behaviour. It's a blocker because of the potential for error or confusion with it, but otherwise this is great. Love all the changes to getByRole
(and similar)!
// `.select()` doesn't work for radio buttons because Playwright | ||
// incorrectly states that the element is covered by the icon SVG. |
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.
If there is a bug report or something to link to to explain why Playwright incorrectly reports this is the case, it would be helpful to document it here. Otherwise, it seems to easy to write Playwright's error off as "incorrect" when there may actually be an underlying issue we can solve. IIRC there is a bug report about something related to SVGs and click events.
I vaguely remember something like this being the case when working on #4088, and it ended up being a bug in our pointer-events configuration for the checkboxes, leading to this change: https://github.com/WordPress/openverse/pull/4088/files#diff-9cab0d134035b3db0d42b453e18b3fff640f27e4155601385449cbe17b7788f8R50
When I was working on that, I remember coming across a Playwright bug report that seemed to indicate there was a general problem with Playwright's "click" and SVGs and that the issue seemed to imply it was just broken, rather than there being a real fix for it (in this case, passing through pointer events). In the end there was a fix in our code that did actually resolve a real (potential) problem. Apparently click events "happened" to work, but didn't technically always appear like they would work (which could affect accessibility tools maybe?).
If either is the case here, if it is a Playwright bug that we cannot do anything to fix, or if there is something we can change about the radio buttons to fix it, like there was for the checkboxes, then we should include that here explicitly.
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.
I thought there was a bug because I was adding pointer-events-none
to the VIcon.vue
component, and it was still getting the click event. Only to realize that the element in the report component is VSvg
! 🤦
Added pointer-events-none
to VRadiomark
and removed the comment 4cf211e
Signed-off-by: Olga Bulat <[email protected]>
Signed-off-by: Olga Bulat <[email protected]>
f45e682
to
4cf211e
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.
Nice! 🚀
Fixes
Fixes #619 by @sarayourfriend
Description
This PR audits the Playwright tests to not use hard-coded strings, replacing them with i18n translations
It also replaces some
page.locator
withpage.getByRole
selectors, and uses better selectors for checkboxes.Testing Instructions
The CI should pass, and the changes should make sense. The Playwright tests should not have hard-coded strings left.
Checklist
Update index.md
).main
) or a parent feature branch.ov just catalog/generate-docs
for catalogPRs) or the media properties generator (
ov just catalog/generate-docs media-props
for the catalog or
ov just api/generate-docs
for the API) where applicable.Developer Certificate of Origin
Developer Certificate of Origin