-
Notifications
You must be signed in to change notification settings - Fork 13.4k
test(breadcrumbs): update Playwright tests to use setContent #30518
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Some of these could probably be removed as screenshots and just check the count on the visible breadcrumbs. LMK if you think there are too many screenshots here.
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 think a lot of these could just be programmatic tests, but also they're probably fine as visual regression tests like this. I'm fine either way. I do like this much more granular approach to visually testing them though!
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.
That is a lot of screenshots, but all the tests make sense to me and I really prefer this change. Great job!
Blocks #30513
The latest Playwright update in #30513 exposed an issue in Safari where dynamically setting the RTL direction on the document no longer works. This causes the Breadcrumbs screenshots to fail because they are not flipping the arrow icon.
This PR converts the Breadcrumbs e2e tests to use
setContent
which works properly with RTL and also breaks the tests up more so they aren't one giant screenshot.