Fix Playwright VR test failures

obulat commented 3 weeks ago

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

Instead of taking the picture of the whole page with the content settings modal, we take a picture of only the content settings modal.
The border above the tab panel in the content settings modal in the dark theme wasn't being set correctly: it was using border-default instead of border-overlay, and one of the "dark" theme CSS variable lists didn't have a value for border-overlay color.
initialize the theme switcher icon based on the store theme, if it's available (rather than undefined) to make it render faster
unset aria-hidden instead of using aria-hidden=false for "Show results" button in the modal
save search result thumb tapes because if the thumb doesn't exist, the page tries to load the image from the provider, making network requests.

Testing Instructions

The CI should pass even if re-ran several times

Checklist

[ ] My pull request has a descriptive title (not a vague title likeUpdate index.md).
[ ] My pull request targets the default branch of the repository (main) or a parent feature branch.
[ ] My commit messages follow [best practices][best_practices].
[ ] My code follows the established code style of the repository.
[ ] I added or updated tests for the changes I made (if applicable).
[ ] I added or updated documentation (if applicable).
[ ] I tried running the project locally and verified that there are no visible errors.
[ ] I ran the DAG documentation generator (ov just catalog/generate-docs for catalog PRs) 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.

[best_practices]: https://git-scm.com/book/en/v2/Distributed-Git-Contributing-to-a-Project#_commit_guidelines

Developer Certificate of Origin

``` Developer Certificate of Origin Version 1.1 Copyright (C) 2004, 2006 The Linux Foundation and its contributors. 1 Letterman Drive Suite D4700 San Francisco, CA, 94129 Everyone is permitted to copy and distribute verbatim copies of this license document, but changing it is not allowed. Developer's Certificate of Origin 1.1 By making a contribution to this project, I certify that: (a) The contribution was created in whole or in part by me and I have the right to submit it under the open source license indicated in the file; or (b) The contribution is based upon previous work that, to the best of my knowledge, is covered under an appropriate open source license and I have the right under that license to submit that work with modifications, whether created in whole or in part by me, under the same open source license (unless I am permitted to submit under a different license), as indicated in the file; or (c) The contribution was provided directly to me by some other person who certified (a), (b) or (c) and I have not modified it. (d) I understand and agree that this project and the contribution are public and that a record of the contribution (including all personal information I submit with it, including my sign-off) is maintained indefinitely and may be redistributed consistent with this project or the open source license(s) involved. ```

github-actions[bot] commented 3 weeks ago

Latest k6 run output[^update]

     ✓ status was 200

     checks.........................: 100.00% ✓ 6400      ✗ 0   
     data_received..................: 1.5 GB  8.8 MB/s
     data_sent......................: 837 kB  4.9 kB/s
     http_req_blocked...............: avg=26.13µs  min=1.98µs  med=3.81µs   max=10.48ms p(90)=5.43µs   p(95)=5.83µs  
     http_req_connecting............: avg=21.21µs  min=0s      med=0s       max=10.42ms p(90)=0s       p(95)=0s      
     http_req_duration..............: avg=587.37ms min=67.44ms med=511.21ms max=2.34s   p(90)=1.06s    p(95)=1.17s   
       { expected_response:true }...: avg=587.37ms min=67.44ms med=511.21ms max=2.34s   p(90)=1.06s    p(95)=1.17s   
   ✓ http_req_failed................: 0.00%   ✓ 0         ✗ 6400
     http_req_receiving.............: avg=142.05µs min=38.59µs med=115.11µs max=18.73ms p(90)=183.23µs p(95)=219.32µs
     http_req_sending...............: avg=21.71µs  min=6.96µs  med=19.24µs  max=5.1ms   p(90)=25.21µs  p(95)=27.11µs 
     http_req_tls_handshaking.......: avg=0s       min=0s      med=0s       max=0s      p(90)=0s       p(95)=0s      
     http_req_waiting...............: avg=587.2ms  min=67.3ms  med=511.04ms max=2.34s   p(90)=1.06s    p(95)=1.17s   
     http_reqs......................: 6400    37.795415/s
     iteration_duration.............: avg=3.13s    min=1.06s   med=2.56s    max=9.23s   p(90)=6.13s    p(95)=6.49s   
     iterations.....................: 1200    7.08664/s
     vus............................: 8       min=8       max=30
     vus_max........................: 30      min=30      max=30

[^update]: This comment will automatically update with new output each time k6 runs for this PR

github-actions[bot] commented 2 weeks ago

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.

obulat commented 2 weeks ago

Thank you!

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?

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.

WordPress / openverse