-
Notifications
You must be signed in to change notification settings - Fork 216
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
Fix Playwright VR test failures #5135
Conversation
Latest k6 run output1
Footnotes
|
Full-stack documentation: https://docs.openverse.org/_preview/5135 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. |
260fd12
to
0861fbe
Compare
@@ -96,7 +137,7 @@ export const expectSnapshot: ExpectSnapshot = async ( | |||
await turnOnDarkMode(page, dir ?? "ltr") | |||
|
|||
// Wait for the theme to change. | |||
await sleep(200) | |||
await sleep(100) |
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.
This was the original value. I thought we needed to increase it to fix the modal rendering, but turns out that switching the theme was done incorrectly before. When the modal is open, the theme switcher is covered by it. We need to close the modal, switch the theme, and then re-open the modal before taking the snapshot.
a68b6f6
to
479eeef
Compare
479eeef
to
0b1bdaf
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.
The changes to the tests and the updated snapshots look good and make sense. I'm approving because it's critical but I do have one question:
Can you explain why there are new tapes as part of this PR? Additionally, if there are new tapes added are some old tapes no longer useful and can be removed?
Thank you!
When the search results page loads, if the thumbnail is not available (in one of the tapes), then the component makes a request for the image's direct URL in on image load error handler, so the rendering is further delayed. If we have all tapes for all of the results on the page, then we don't make any requests to the providers. As for removing tapes, maybe we should remove one of the search terms we use in the Playwright tests. We currently use "cat", "birds" and "galah", which is not necessary. If we decide that this is a good idea, we can update the test files to only have one search terms, and remove unused tapes. |
Fixes
Fixes #5134 by @obulat
Note: the added lines are lines from thumbnail request tapes for rendering the search results page in Playwright
Description
The problem in #5134 was that the dark theme content switcher modal would sometimes display a "loading" state, even though the loading was finished before the light snapshot was taken.
In the Playwright visual regression tests, we have
expectSnapshot
function that takes the light snapshot, and then switches the theme to dark and takes the dark snapshot. The theme switching should not cause any network requests such as re-loading the search results, but in these tests for some reason that was happening.Originally, this PR added a wait for
networkidle
event before taking a snapshot of the filters modal. I also tried various options of using the UI state to detect that all of the images were loaded (so that there are no changes in the snapshot backgrounds) but nothing works reliably.Even with this check, the app was re-fetching data when the theme was changed from light to dark in
expect-snapshot
. I couldn't understand why that was, and couldn't reproduce it in the real app. It was only happening in Playwright trace.So, instead of tweaking the wait for the content settings modal being rendered, I made the test load the filter on SSR (so, instead of searching without filters, and then selecting a filter and snapshotting the filters grid, we now go to the page with
license_type=commercial
query and load the filtered search results on the SSR).Other changes
border-default
instead ofborder-overlay
, and one of the "dark" theme CSS variable lists didn't have a value forborder-overlay
color.aria-hidden=false
for "Show results" button in the modalTesting Instructions
The CI should pass even if re-ran several times
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