Add test reporter to prevent stale screenshots (#12743)

* Split up slow Playwright tests

To optimise parallelism

Deals with:

```
Slow test file: read-receipts/redactions.spec.ts (5.4m)
Slow test file: read-receipts/new-messages.spec.ts (3.9m)
Slow test file: read-receipts/high-level.spec.ts (3.6m)
Slow test file: read-receipts/editing-messages.spec.ts (3.1m)
Slow test file: read-receipts/reactions.spec.ts (2.2m)
Slow test file: crypto/crypto.spec.ts (2.4m)
Slow test file: settings/appearance-user-settings-tab/appearance-user-settings-tab.spec.ts (1.2m)
Slow test file: composer/composer.spec.ts (1.1m)
Slow test file: crypto/verification.spec.ts (1.1m)
```

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Move around snapshots

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Add test reporter to prevent stale screenshots

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Iterate

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Fix test

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Iterate

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Remove darwin screenshots which should not have been checked in

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Fix absolute vs relative path mismatch

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Revert "Remove darwin screenshots which should not have been checked in"

This reverts commit 1e189977fa9ec873339fc02b2b231a314809b2d5.

* Iterate

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Revert "Revert "Remove darwin screenshots which should not have been checked in""

This reverts commit 5144b9b28e31ca543b2c5d02820c3f957dbd8c04.

* Iterate

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Remove stale screenshots

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Revert "Remove stale screenshots"

This reverts commit 9beae9974557c1ffa99c2372da280bb0da407bd1.

* Apply same sanitization as Playwright for file name consistency

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* add dev dep

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Remove stale screenshots

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Discard changes to playwright/flaky-reporter.ts

* Update end-to-end-tests.yaml

---------

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

.github/workflows/end-to-end-tests.yaml

@@ -190,13 +190,14 @@ jobs:
 
             - name: Merge into HTML Report
               if: inputs.skip != true
-              run: yarn playwright merge-reports --reporter=html,./playwright/flaky-reporter.ts ./all-blob-reports
+              run: yarn playwright merge-reports --reporter=html,./playwright/flaky-reporter.ts,./playwright/stale-screenshot-reporter.ts ./all-blob-reports
               env:
                   # Only pass creds to the flaky-reporter on main branch runs
                   GITHUB_TOKEN: ${{ github.ref_name == 'develop' && secrets.ELEMENT_BOT_TOKEN || '' }}
 
+            # Upload the HTML report even if one of our reporters fails, this can happen when stale screenshots are detected
             - name: Upload HTML report
-              if: inputs.skip != true
+              if: always() && inputs.skip != true
               uses: actions/upload-artifact@v4
               with:
                   name: html-report

package.json

@@ -213,6 +213,7 @@
         "fake-indexeddb": "^6.0.0",
         "fetch-mock-jest": "^1.5.1",
         "fs-extra": "^11.0.0",
+        "glob": "^11.0.0",
         "jest": "^29.6.2",
         "jest-canvas-mock": "^2.5.2",
         "jest-environment-jsdom": "^29.6.2",
@@ -223,6 +224,7 @@
         "matrix-web-i18n": "^3.2.1",
         "mocha-junit-reporter": "^2.2.0",
         "node-fetch": "2",
+        "playwright-core": "^1.45.1",
         "postcss-scss": "^4.0.4",
         "prettier": "3.3.2",
         "raw-loader": "^4.0.2",

playwright/@types/playwright-core.d.ts

@@ -0,0 +1,20 @@
+/*
+Copyright 2024 The Matrix.org Foundation C.I.C.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+declare module "playwright-core/lib/utils" {
+    // This type is not public in playwright-core utils
+    export function sanitizeForFilePath(filePath: string): string;
+}

playwright/e2e/settings/general-room-settings-tab.spec.ts

@@ -34,7 +34,7 @@ test.describe("General room settings tab", () => {
         // Assert that "Show less" details element is rendered
         await expect(settings.getByText("Show less")).toBeVisible();
 
-        await expect(settings).toMatchScreenshot();
+        await expect(settings).toMatchScreenshot("General-room-settings-tab-should-be-rendered-properly-1.png");
 
         // Click the "Show less" details element
         await settings.getByText("Show less").click();

playwright/e2e/settings/preferences-user-settings-tab.spec.ts

@@ -31,7 +31,7 @@ test.describe("Preferences user settings tab", () => {
 
         // Assert that the top heading is rendered
         await expect(tab.getByRole("heading", { name: "Preferences" })).toBeVisible();
-        await expect(tab).toMatchScreenshot();
+        await expect(tab).toMatchScreenshot("Preferences-user-settings-tab-should-be-rendered-properly-1.png");
     });
 
     test("should be able to change the app language", async ({ uut, user }) => {

playwright/e2e/settings/security-user-settings-tab.spec.ts

@@ -47,7 +47,9 @@ test.describe("Security user settings tab", () => {
             test("should be rendered properly", async ({ app, page }) => {
                 const tab = await app.settings.openUserSettings("Security");
                 await tab.getByRole("button", { name: "Learn more" }).click();
-                await expect(page.locator(".mx_AnalyticsLearnMoreDialog_wrapper .mx_Dialog")).toMatchScreenshot();
+                await expect(page.locator(".mx_AnalyticsLearnMoreDialog_wrapper .mx_Dialog")).toMatchScreenshot(
+                    "Security-user-settings-tab-with-posthog-enable-b5d89-csLearnMoreDialog-should-be-rendered-properly-1.png",
+                );
             });
         });
 

playwright/e2e/user-onboarding/user-onboarding-new.spec.ts

@@ -35,7 +35,9 @@ test.describe("User Onboarding (new user)", () => {
     });
 
     test("page is shown and preference exists", async ({ page, app }) => {
-        await expect(page.locator(".mx_UserOnboardingPage")).toMatchScreenshot();
+        await expect(page.locator(".mx_UserOnboardingPage")).toMatchScreenshot(
+            "User-Onboarding-new-user-page-is-shown-and-preference-exists-1.png",
+        );
         await app.settings.openUserSettings("Preferences");
         await expect(page.getByText("Show shortcut to welcome checklist above the room list")).toBeVisible();
     });

playwright/element-web-test.ts

@@ -15,9 +15,10 @@ limitations under the License.
 */
 
 import { test as base, expect as baseExpect, Locator, Page, ExpectMatcherState, ElementHandle } from "@playwright/test";
+import { sanitizeForFilePath } from "playwright-core/lib/utils";
 import AxeBuilder from "@axe-core/playwright";
 import _ from "lodash";
-import { basename } from "node:path";
+import { basename, extname } from "node:path";
 
 import type mailhog from "mailhog";
 import type { IConfigOptions } from "../src/IConfigOptions";
@@ -298,11 +299,18 @@ export const test = base.extend<{
     },
 });
 
+// Based on https://github.com/microsoft/playwright/blob/2b77ed4d7aafa85a600caa0b0d101b72c8437eeb/packages/playwright/src/util.ts#L206C8-L210C2
+function sanitizeFilePathBeforeExtension(filePath: string): string {
+    const ext = extname(filePath);
+    const base = filePath.substring(0, filePath.length - ext.length);
+    return sanitizeForFilePath(base) + ext;
+}
+
 export const expect = baseExpect.extend({
     async toMatchScreenshot(
         this: ExpectMatcherState,
         receiver: Page | Locator,
-        name?: `${string}.png`,
+        name: `${string}.png`,
         options?: {
             mask?: Array<Locator>;
             omitBackground?: boolean;
@@ -311,6 +319,9 @@ export const expect = baseExpect.extend({
             css?: string;
         },
     ) {
+        const testInfo = test.info();
+        if (!testInfo) throw new Error(`toMatchScreenshot() must be called during the test`);
+
         const page = "page" in receiver ? receiver.page() : receiver;
 
         let hideTooltipsCss: string | undefined;
@@ -354,9 +365,18 @@ export const expect = baseExpect.extend({
             `,
         })) as ElementHandle<Element>;
 
-        await baseExpect(receiver).toHaveScreenshot(name, options);
+        const screenshotName = sanitizeFilePathBeforeExtension(name);
+        await baseExpect(receiver).toHaveScreenshot(screenshotName, options);
 
         await style.evaluate((tag) => tag.remove());
+
+        testInfo.annotations.push({
+            // `_` prefix hides it from the HTML reporter
+            type: "_screenshot",
+            // include a path relative to `playwright/snapshots/`
+            description: testInfo.snapshotPath(screenshotName).split("/playwright/snapshots/", 2)[1],
+        });
+
         return { pass: true, message: () => "", name: "toMatchScreenshot" };
     },
 });

playwright/stale-screenshot-reporter.ts

@@ -0,0 +1,74 @@
+/*
+Copyright 2024 The Matrix.org Foundation C.I.C.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+/**
+ * Test reporter which compares the reported screenshots vs those on disk to find stale screenshots
+ * Only intended to run from within GitHub Actions
+ */
+
+import path from "node:path";
+import { glob } from "glob";
+
+import type { Reporter, TestCase } from "@playwright/test/reporter";
+
+const snapshotRoot = path.join(__dirname, "snapshots");
+
+class StaleScreenshotReporter implements Reporter {
+    private screenshots = new Set<string>();
+    private success = true;
+
+    public onTestEnd(test: TestCase): void {
+        for (const annotation of test.annotations) {
+            if (annotation.type === "_screenshot") {
+                this.screenshots.add(annotation.description);
+            }
+        }
+    }
+
+    private error(msg: string, file: string) {
+        if (process.env.GITHUB_ACTIONS) {
+            console.log(`::error file=${file}::${msg}`);
+        }
+        console.error(msg, file);
+        this.success = false;
+    }
+
+    public async onExit(): Promise<void> {
+        const screenshotFiles = new Set(await glob(`**/*.png`, { cwd: snapshotRoot }));
+        for (const screenshot of screenshotFiles) {
+            if (screenshot.split("-").at(-1) !== "linux.png") {
+                this.error(
+                    "Found screenshot belonging to different platform, this should not be checked in",
+                    screenshot,
+                );
+            }
+        }
+        for (const screenshot of this.screenshots) {
+            screenshotFiles.delete(screenshot);
+        }
+        if (screenshotFiles.size > 0) {
+            for (const screenshot of screenshotFiles) {
+                this.error("Stale screenshot file", screenshot);
+            }
+        }
+
+        if (!this.success) {
+            process.exit(1);
+        }
+    }
+}
+
+export default StaleScreenshotReporter;

yarn.lock

@@ -7418,7 +7418,7 @@ pkg-dir@^4.2.0:
   dependencies:
     find-up "^4.0.0"
 
-playwright-core@1.45.1:
+playwright-core@1.45.1, playwright-core@^1.45.1:
   version "1.45.1"
   resolved "https://registry.yarnpkg.com/playwright-core/-/playwright-core-1.45.1.tgz#549a2701556b58245cc75263f9fc2795c1158dc1"
   integrity sha512-LF4CUUtrUu2TCpDw4mcrAIuYrEjVDfT1cHbJMfwnE2+1b8PZcFzPNgvZCvq2JfQ4aTjRCCHw5EJ2tmr2NSzdPg==