Add dark snapshots to 2xl breakpoint visual regression tests

[obulat]

Fixes

Fixes #4305 by @zackkrida

Description

This PR adds the dark snapshots for 2xl tests by setting the useColorMode to true when the snapshot name contains -2xl string. It also adds a sleep of 200ms after turning on the dark mode to make sure that it's been fully rendered before taking the snapshot.

Testing Instructions

It is expected that only the dark snapshots are added in this PR. If the added snapshots are incorrect, it should be addressed in a different PR.

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.
• 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.

Developer Certificate of Origin

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.

[sarayourfriend]

So one issue with this specific version of the approach, is that putting two snapshots in the same test block will cause some confusion about which version of the site is broken. @zackkrida and I touched on yesterday when the two of us spoke synchronously about it. It is a subtle issue, and I'm torn on how best to address it. The worst effect of it is if whichever snapshot goes first fails, it isn't clear whether the second one also would have failed. If dark and light have an issue, or only dark/only light, it would be helpful for debugging and understanding the extent to know the status of both.

I've gone through a bunch of variations I can think of locally, some of them worked, others didn't, but most involved big refactors to existing tests. That was especially true when I tried to separate the setup from the expectation. However, I think there's maybe a simpler way, that results in very minimal diffs across the board, including no change at all to the behaviour for existing e2e tests.

diff --git a/frontend/test/playwright/utils/breakpoints.ts b/frontend/test/playwright/utils/breakpoints.ts
index 4bfcbe8a0..0487164dc 100644
--- a/frontend/test/playwright/utils/breakpoints.ts
+++ b/frontend/test/playwright/utils/breakpoints.ts
@@ -15,9 +15,12 @@ type ExpectSnapshot = <T extends ScreenshotAble>(
   snapshotOptions?: Parameters<ReturnType<Expect>["toMatchSnapshot"]>[0]
 ) => Promise<Buffer | void>
 
+type BaseName = `${string}-${Breakpoint}`
+type NameWithColorScheme = `${BaseName}-${"light" | "dark"}`
+
 type BreakpointBlock = (options: {
   getConfigValues: (name: string) => {
-    name: `${typeof name}-${Breakpoint}.png`
+    name: `${BaseName | NameWithColorScheme}.png`
   }
   breakpoint: Breakpoint
   expectSnapshot: ExpectSnapshot
@@ -54,14 +57,16 @@ interface Options {
   /**
    * Disable animations to remove flakiness.
    */
-  animations: "disabled"
+  animations?: "disabled"
   /**
    * To make sure caret blinking doesn't cause diffs.
    */
-  caret: "hide"
+  caret?: "hide"
+
+  colorScheme?: "all" | "light" | "dark"
 }
 
-const defaultOptions = Object.freeze({
+const defaultOptions = Object.freeze<Options>({
   uaMocking: true,
   animations: "disabled",
   caret: "hide",
@@ -73,41 +78,50 @@ const makeBreakpointDescribe =
     blockOrOptions: T,
     block?: T extends Record<string, unknown> ? BreakpointBlock : undefined
   ) => {
-    test.describe(`screen at breakpoint ${breakpoint} with width ${screenWidth}`, () => {
-      const _block = (
-        typeof blockOrOptions === "function" ? blockOrOptions : block
-      ) as BreakpointBlock
-      const options =
-        typeof blockOrOptions !== "function"
-          ? { ...defaultOptions, ...blockOrOptions }
-          : defaultOptions
-
-      test.use({
-        viewport: { width: screenWidth, height: 700 },
-        userAgent: options.uaMocking ? mockUaStrings[breakpoint] : undefined,
-      })
-
-      const getConfigValues = (name: string) => ({
-        name: `${name}-${breakpoint}.png` as const,
-      })
-
-      const expectSnapshot = async <T extends ScreenshotAble>(
-        name: string,
-        screenshotAble: T,
-        options?: Parameters<T["screenshot"]>[0],
-        snapshotOptions?: Parameters<ReturnType<Expect>["toMatchSnapshot"]>[0]
-      ) => {
-        const { name: snapshotName } = getConfigValues(name)
-        return expect(await screenshotAble.screenshot(options)).toMatchSnapshot(
-          {
-            name: snapshotName,
-            ...snapshotOptions,
-          }
-        )
-      }
-
-      _block({ breakpoint, getConfigValues, expectSnapshot })
-    })
+    const _block = (
+      typeof blockOrOptions === "function" ? blockOrOptions : block
+    ) as BreakpointBlock
+    const options =
+      typeof blockOrOptions !== "function"
+        ? { ...defaultOptions, ...blockOrOptions }
+        : defaultOptions
+
+    const colorSchemes = options.colorScheme === "all" ? ["light", "dark"] as const : [options.colorScheme ?? "light"]
+    for (const colorScheme of colorSchemes) {
+      test.describe(`screen at breakpoint ${breakpoint} with width ${screenWidth} in color scheme ${colorScheme}`, () => {
+        test.beforeEach(`set color scheme ${colorScheme}`, async ({ page }) => {
+          // do whatever to set the color scheme...
+          // maybe only do this beforeEach if `options.colorScheme` != "light" so existing tests don't change?
+          // only if light is truly the "default" and safely so
+        })
+
+        test.use({
+          viewport: { width: screenWidth, height: 700 },
+          userAgent: options.uaMocking ? mockUaStrings[breakpoint] : undefined,
+        })
+
+        const getConfigValues = (name: string) => ({
+          name: options.colorScheme === "light" ? `${name}-${breakpoint}.png` as const : `${name}-${breakpoint}-${colorScheme}.png` as const,
+        })
+
+        const expectSnapshot = async <T extends ScreenshotAble>(
+          name: string,
+          screenshotAble: T,
+          options?: Parameters<T["screenshot"]>[0],
+          snapshotOptions?: Parameters<ReturnType<Expect>["toMatchSnapshot"]>[0]
+        ) => {
+          const { name: snapshotName } = getConfigValues(name)
+          return expect(await screenshotAble.screenshot(options)).toMatchSnapshot(
+            {
+              name: snapshotName,
+              ...snapshotOptions,
+            }
+          )
+        }
+
+        _block({ breakpoint, getConfigValues, expectSnapshot })
+     })
+    }
   }
 
 const capitalize = (s: string): Capitalize<typeof s> =>

With those changes, the only change required to make a test run in multiple colour schemes, is just to pass the colorSchemes option to the breakpoints.describeWhatever function. For example, here is that change for the global-audio-player VR test:

diff --git a/frontend/test/playwright/visual-regression/components/global-audio-player.spec.ts b/frontend/test/playwright/visual-regression/components/global-audio-player.spec.ts
index 0360192e6..c2e6d0682 100644
--- a/frontend/test/playwright/visual-regression/components/global-audio-player.spec.ts
+++ b/frontend/test/playwright/visual-regression/components/global-audio-player.spec.ts
@@ -11,7 +11,7 @@ import { languageDirections, t } from "~~/test/playwright/utils/i18n"
 test.describe.configure({ mode: "parallel" })
 
 for (const dir of languageDirections) {
-  breakpoints.describeXs(async ({ expectSnapshot }) => {
+  breakpoints.describeXs({ colorScheme: "all" }, async ({ expectSnapshot }) => {
     test(`global audio player on the search page - ${dir}`, async ({
       page,
     }) => {

There might be some problems with taking this approach that I haven't considered, that the approach in this PR of using multiple expect calls in the same test block doesn't have. One potential objection I can anticipate is that this is the slowest version, because both colour schemes have to run setup. If we can manage separating the setup block from the expectation block, so that the breakpoints utility can call the setup callback in beforeEach and then just create the test blocks required for the configured colour schemes... that would save time across the board in test execution. I couldn't think of accomplishing that that did not produce huge diffs across the board and much more complicated code in the breakpoints utility. The simplicity of a loop is pretty nice in comparison.

However, I think it's probably more important to optimise for test isolation, even at the expense of than speed, at least to begin with. Then we can explore other approaches for speeding up tests (like parallelising fully isolated test cases, for example). To be clear though, it is good to acknowledge this approach would double the total execution time of our visual regression tests.

---

One other option though... it's kind of wacky... and I do not like it, but I can't resist suggesting it. What if it were possible to create screenshots that included both color schemes of the page, stacked vertically? It isn't possible to just arbitrarily concatenate pngs as far as I can tell, but if it were... 😛. Then we would just use a single snapshot assertion on the stacked screenshots. Voilá. Horrifying, but it solves the speed problem and implicitly parallelises the two colour schemes 😬.

[zackkrida]

🚀