WordPress / openverse

Openverse is a search engine for openly-licensed media. This monorepo includes all application code.
https://openverse.org
MIT License
251 stars 202 forks source link

Setup and add initial Dark Mode visual regression test screenshots #4305

Closed zackkrida closed 2 weeks ago

zackkrida commented 5 months ago

This issue is part of the Dark Mode project: #3592. Please see the implementation plan for additional detail and context.

[!IMPORTANT]
This issue is blocked until #4303 and #4304 are completed.

Description

Once the dark mode palette is added and can be forced via feature flag, update our visual regression tests to create new dark mode snapshots. Existing screenshots should be renamed to _light and the new dark mode screenshots should be named _dark.

For setting dark mode in the tests, update frontend/test/playwright/utils/breakpoints.ts so that each breakpoint produces and expects a dark mode screenshot to pass as well as the existing light mode screenshot. Emulating dark mode can be easily handled via Playwright's emulation controls.

This will produce a large file diff. When this PR lands @fcoveram and @WordPress/openverse-frontend should review the full dark mode appearance for correctness and sufficient color contrast (see the “Accessibility” section for more details).

fcoveram commented 2 months ago

I see the two issues mentioned as blockers done, so feel free to ping me for any review. I have this ticket in my priorities.

zackkrida commented 1 month ago

@obulat and I discussed this a bit an actually came up with a seemingly much simpler approach:

         - name: playwright_vr
            script: "ENABLE_DARK_MODE=true test:playwright visual-regression -u"

Then, we change the snapshot directory to one for dark mode screenshots using the https://playwright.dev/docs/api/class-testproject#test-project-snapshot-path-template config based on the presence of ENABLE_DARK_MODE

Finally, we setup the dark mode config with something like this:

export default defineConfig({ use: { colorScheme: ENABLE_DARK_MODE !== "true" ? 'light' : 'dark', }, });

sarayourfriend commented 1 month ago

@zackkrida just to make sure, but these visual regression tests are separate from tests that would confirm behaviour of the system's reported preference, and toggles, in each of the available combinations, right? I see we have unit tests for the use-dark-mode composable, I'm getting at more integration-level tests that e2e would be good for to confirm that use-dark-mode is actually operating on the page. In other words, that the result is used correctly to render a particular colour scheme.

One aspect that isn't covered by the proposed configuration, for example, is the "no-preference" option supported by use.colorScheme: https://playwright.dev/docs/api/class-testoptions#test-options-color-scheme. But a test outside the visual regression suite could cover all three easily on a single page (maybe just the home page?); however, all the playwright frontend tests share the same configuration, so use.colorScheme in the root configuration might limit the flexibility of the non-vr tests?

Maybe using separate playwright configurations for visual regression tests would be better for this?

Using an environment variable for dark mode rather than building it into the tests would also require either adding a light and dark-mode run within the same existing package.json script for running playwright tests, or else contributors will need to run two separate Playwright scripts to update visual regression tests locally.

I guess I'm wondering how whether the approach y'all are proposing is actually simpler in the end than treating dark mode as a new "breakpoint" :sweat_smile:. Just want to make sure the different scenarios are covered in at least one non-vr test.

To clarify, I agree with what I think y'all are working from, that the visual regression tests do not need to exercise the logic of toggling dark mode on/off and the combinations with system's reported preference. I just want to make sure that such a test is still possible and easy to execute in the regular course of testing... and that testing overall stays easy to execute and understand, rather than over-emphasising simplicity in CI.

zackkrida commented 1 month ago

@sarayourfriend yes, separate. These tests are solely for verifying the correct visual appearance of components and pages in each color mode (so currently light and dark but in the future our high-contrast variants, as well).

That said, we don't have an existing issue for an e2e test as you've described. Would you mind creating one and adding it to the second dark mode milestone?

zackkrida commented 1 month ago

@obulat, @sarayourfriend and I chatted about this a bit and I'm thinking it'll actually be best to take a different approach. I'll take some time to describe the approach here tomorrow!

sarayourfriend commented 1 month ago

That said, we don't have an existing issue for an e2e test as you've described. Would you mind creating one and adding it to the second dark mode milestone?

I will do this today, it's needed regardless of the visual regression testing approach you end up describing :+1: