test: disable flaky macOS panel test & refactor screen capture testing (#41441)

* Disable flaky test * Add helper for storing test artifacts * Refactor screen capture tests We have a pattern for inspecting a screen capture, so this refactor codifies that pattern into a helper. This gives us shorter test code, consistency (previously, the display in test code and the display captured could theoretically be different), and better debugging/observability on failure.
2024-02-27 20:54:20 -07:00 · 2024-02-27 20:54:20 -07:00 · a6133e85d1
--- a/.circleci/config/base.yml
+++ b/.circleci/config/base.yml
@ -1642,6 +1642,8 @@ commands:
            fi
      - store_test_results:
          path: src/junit
+      - store_artifacts:
+          path: src/electron/spec/artifacts

      - *step-verify-mksnapshot
      - *step-verify-chromedriver
--- a/spec/api-browser-view-spec.ts
+++ b/spec/api-browser-view-spec.ts
@ -3,7 +3,7 @@ import * as path from 'node:path';
 import { BrowserView, BrowserWindow, screen, webContents } from 'electron/main';
 import { closeWindow } from './lib/window-helpers';
 import { defer, ifit, startRemoteControlApp } from './lib/spec-helpers';
-import { areColorsSimilar, captureScreen, getPixelColor } from './lib/screen-helpers';
+import { ScreenCapture } from './lib/screen-helpers';
 import { once } from 'node:events';

 describe('BrowserView module', () => {
@ -88,13 +88,8 @@ describe('BrowserView module', () => {
      w.setBrowserView(view);
      await view.webContents.loadURL('data:text/html,hello there');

-      const screenCapture = await captureScreen();
-      const centerColor = getPixelColor(screenCapture, {
-        x: display.size.width / 2,
-        y: display.size.height / 2
-      });
-
-      expect(areColorsSimilar(centerColor, WINDOW_BACKGROUND_COLOR)).to.be.true();
+      const screenCapture = await ScreenCapture.createForDisplay(display);
+      await screenCapture.expectColorAtCenterMatches(WINDOW_BACKGROUND_COLOR);
    });

    // Linux and arm64 platforms (WOA and macOS) do not return any capture sources
@ -114,13 +109,8 @@ describe('BrowserView module', () => {
      w.setBackgroundColor(VIEW_BACKGROUND_COLOR);
      await view.webContents.loadURL('data:text/html,hello there');

-      const screenCapture = await captureScreen();
-      const centerColor = getPixelColor(screenCapture, {
-        x: display.size.width / 2,
-        y: display.size.height / 2
-      });
-
-      expect(areColorsSimilar(centerColor, VIEW_BACKGROUND_COLOR)).to.be.true();
+      const screenCapture = await ScreenCapture.createForDisplay(display);
+      await screenCapture.expectColorAtCenterMatches(VIEW_BACKGROUND_COLOR);
    });
  });

--- a/spec/api-browser-window-spec.ts
+++ b/spec/api-browser-window-spec.ts
@ -11,7 +11,7 @@ import { app, BrowserWindow, BrowserView, dialog, ipcMain, OnBeforeSendHeadersLi
 import { emittedUntil, emittedNTimes } from './lib/events-helpers';
 import { ifit, ifdescribe, defer, listen } from './lib/spec-helpers';
 import { closeWindow, closeAllWindows } from './lib/window-helpers';
-import { areColorsSimilar, captureScreen, HexColors, getPixelColor, hasCapturableScreen } from './lib/screen-helpers';
+import { HexColors, hasCapturableScreen, ScreenCapture } from './lib/screen-helpers';
 import { once } from 'node:events';
 import { setTimeout } from 'node:timers/promises';
 import { setTimeout as syncSetTimeout } from 'node:timers';
@ -1246,6 +1246,7 @@ describe('BrowserWindow module', () => {
        }
      });

+      // FIXME: disabled in `disabled-tests.json`
      ifit(process.platform === 'darwin')('it does not activate the app if focusing an inactive panel', async () => {
        // Show to focus app, then remove existing window
        w.show();
@ -6496,18 +6497,22 @@ describe('BrowserWindow module', () => {
      await foregroundWindow.loadFile(colorFile);

      await setTimeout(1000);
-      const screenCapture = await captureScreen();
-      const leftHalfColor = getPixelColor(screenCapture, {
-        x: display.size.width / 4,
-        y: display.size.height / 2
-      });
-      const rightHalfColor = getPixelColor(screenCapture, {
-        x: display.size.width - (display.size.width / 4),
-        y: display.size.height / 2
-      });

-      expect(areColorsSimilar(leftHalfColor, HexColors.GREEN)).to.be.true();
-      expect(areColorsSimilar(rightHalfColor, HexColors.RED)).to.be.true();
+      const screenCapture = await ScreenCapture.createForDisplay(display);
+      await screenCapture.expectColorAtPointOnDisplayMatches(
+        HexColors.GREEN,
+        (size) => ({
+          x: size.width / 4,
+          y: size.height / 2
+        })
+      );
+      await screenCapture.expectColorAtPointOnDisplayMatches(
+        HexColors.RED,
+        (size) => ({
+          x: size.width * 3 / 4,
+          y: size.height / 2
+        })
+      );
    });

    ifit(process.platform === 'darwin')('Allows setting a transparent window via CSS', async () => {
@ -6537,13 +6542,9 @@ describe('BrowserWindow module', () => {
      await once(ipcMain, 'set-transparent');

      await setTimeout(1000);
-      const screenCapture = await captureScreen();
-      const centerColor = getPixelColor(screenCapture, {
-        x: display.size.width / 2,
-        y: display.size.height / 2
-      });

-      expect(areColorsSimilar(centerColor, HexColors.PURPLE)).to.be.true();
+      const screenCapture = await ScreenCapture.createForDisplay(display);
+      await screenCapture.expectColorAtCenterMatches(HexColors.PURPLE);
    });

    // Linux and arm64 platforms (WOA and macOS) do not return any capture sources
@ -6560,15 +6561,11 @@ describe('BrowserWindow module', () => {
        await window.webContents.loadURL('data:text/html,<head><meta name="color-scheme" content="dark"></head>');

        await setTimeout(1000);
-        const screenCapture = await captureScreen();
-        const centerColor = getPixelColor(screenCapture, {
-          x: display.size.width / 2,
-          y: display.size.height / 2
-        });
-        window.close();
-
+        const screenCapture = await ScreenCapture.createForDisplay(display);
        // color-scheme is set to dark so background should not be white
-        expect(areColorsSimilar(centerColor, HexColors.WHITE)).to.be.false();
+        await screenCapture.expectColorAtCenterDoesNotMatch(HexColors.WHITE);
+
+        window.close();
      }
    });
  });
@ -6590,13 +6587,9 @@ describe('BrowserWindow module', () => {
      await once(w, 'ready-to-show');

      await setTimeout(1000);
-      const screenCapture = await captureScreen();
-      const centerColor = getPixelColor(screenCapture, {
-        x: display.size.width / 2,
-        y: display.size.height / 2
-      });

-      expect(areColorsSimilar(centerColor, HexColors.BLUE)).to.be.true();
+      const screenCapture = await ScreenCapture.createForDisplay(display);
+      await screenCapture.expectColorAtCenterMatches(HexColors.BLUE);
    });
  });

--- a/spec/disabled-tests.json
+++ b/spec/disabled-tests.json
@ -7,5 +7,6 @@
  "session module ses.cookies should set cookie for standard scheme",
  "webFrameMain module WebFrame.visibilityState should match window state",
  "reporting api sends a report for a deprecation",
-  "chromium features SpeechSynthesis should emit lifecycle events"
+  "chromium features SpeechSynthesis should emit lifecycle events",
+  "BrowserWindow module focus and visibility BrowserWindow.focus() it does not activate the app if focusing an inactive panel"
 ]
--- a/spec/guest-window-manager-spec.ts
+++ b/spec/guest-window-manager-spec.ts
@ -1,6 +1,6 @@
 import { BrowserWindow, screen } from 'electron';
 import { expect, assert } from 'chai';
-import { areColorsSimilar, captureScreen, HexColors, getPixelColor } from './lib/screen-helpers';
+import { HexColors, ScreenCapture } from './lib/screen-helpers';
 import { ifit } from './lib/spec-helpers';
 import { closeAllWindows } from './lib/window-helpers';
 import { once } from 'node:events';
@ -209,12 +209,8 @@ describe('webContents.setWindowOpenHandler', () => {
    childWindow.setBounds(display.bounds);
    await childWindow.webContents.executeJavaScript("const meta = document.createElement('meta'); meta.name = 'color-scheme'; meta.content = 'dark'; document.head.appendChild(meta); true;");
    await setTimeoutAsync(1000);
-    const screenCapture = await captureScreen();
-    const centerColor = getPixelColor(screenCapture, {
-      x: display.size.width / 2,
-      y: display.size.height / 2
-    });
+    const screenCapture = await ScreenCapture.createForDisplay(display);
    // color-scheme is set to dark so background should not be white
-    expect(areColorsSimilar(centerColor, HexColors.WHITE)).to.be.false();
+    await screenCapture.expectColorAtCenterDoesNotMatch(HexColors.WHITE);
  });
 });
--- a/spec/lib/artifacts.ts
+++ b/spec/lib/artifacts.ts
@ -0,0 +1,36 @@
+import path = require('node:path');
+import fs = require('node:fs/promises');
+import { randomBytes } from 'node:crypto';
+
+const IS_CI = !!process.env.CI;
+const ARTIFACT_DIR = path.join(__dirname, '..', 'artifacts');
+
+async function ensureArtifactDir (): Promise<void> {
+  if (!IS_CI) {
+    return;
+  }
+
+  await fs.mkdir(ARTIFACT_DIR, { recursive: true });
+}
+
+export async function createArtifact (
+  fileName: string,
+  data: Buffer
+): Promise<void> {
+  if (!IS_CI) {
+    return;
+  }
+
+  await ensureArtifactDir();
+  await fs.writeFile(path.join(ARTIFACT_DIR, fileName), data);
+}
+
+export async function createArtifactWithRandomId (
+  makeFileName: (id: string) => string,
+  data: Buffer
+): Promise<string> {
+  const randomId = randomBytes(12).toString('hex');
+  const fileName = makeFileName(randomId);
+  await createArtifact(fileName, data);
+  return fileName;
+}
--- a/spec/lib/screen-helpers.ts
+++ b/spec/lib/screen-helpers.ts
@ -1,60 +1,18 @@
-import * as path from 'node:path';
-import * as fs from 'node:fs';
 import { screen, desktopCapturer, NativeImage } from 'electron';
-
-const fixtures = path.resolve(__dirname, '..', 'fixtures');
+import { createArtifactWithRandomId } from './artifacts';
+import { AssertionError } from 'chai';

 export enum HexColors {
  GREEN = '#00b140',
  PURPLE = '#6a0dad',
  RED = '#ff0000',
  BLUE = '#0000ff',
-  WHITE = '#ffffff'
-};
+  WHITE = '#ffffff',
+}

-/**
- * Capture the screen at the given point.
- *
- * NOTE: Not yet supported on Linux in CI due to empty sources list.
- */
-export const captureScreen = async (point: Electron.Point = { x: 0, y: 0 }): Promise<NativeImage> => {
-  const display = screen.getDisplayNearestPoint(point);
-  const sources = await desktopCapturer.getSources({ types: ['screen'], thumbnailSize: display.size });
-  // Toggle to save screen captures for debugging.
-  const DEBUG_CAPTURE = process.env.DEBUG_CAPTURE || false;
-  if (DEBUG_CAPTURE) {
-    for (const source of sources) {
-      await fs.promises.writeFile(path.join(fixtures, `screenshot_${source.display_id}_${Date.now()}.png`), source.thumbnail.toPNG());
-    }
-  }
-  const screenCapture = sources.find(source => source.display_id === `${display.id}`);
-  // Fails when HDR is enabled on Windows.
-  // https://bugs.chromium.org/p/chromium/issues/detail?id=1247730
-  if (!screenCapture) {
-    const displayIds = sources.map(source => source.display_id);
-    throw new Error(`Unable to find screen capture for display '${display.id}'\n\tAvailable displays: ${displayIds.join(', ')}`);
-  }
-  return screenCapture.thumbnail;
-};
-
-const formatHexByte = (val: number): string => {
-  const str = val.toString(16);
-  return str.length === 2 ? str : `0${str}`;
-};
-
-/**
- * Get the hex color at the given pixel coordinate in an image.
- */
-export const getPixelColor = (image: Electron.NativeImage, point: Electron.Point): string => {
-  // image.crop crashes if point is fractional, so round to prevent that crash
-  const pixel = image.crop({ x: Math.round(point.x), y: Math.round(point.y), width: 1, height: 1 });
-  // TODO(samuelmaddock): NativeImage.toBitmap() should return the raw pixel
-  // color, but it sometimes differs. Why is that?
-  const [b, g, r] = pixel.toBitmap();
-  return `#${formatHexByte(r)}${formatHexByte(g)}${formatHexByte(b)}`;
-};
-
-const hexToRgba = (hexColor: string) => {
+function hexToRgba (
+  hexColor: string
+): [number, number, number, number] | undefined {
  const match = hexColor.match(/^#([0-9a-fA-F]{6,8})$/);
  if (!match) return;

@ -63,12 +21,37 @@ const hexToRgba = (hexColor: string) => {
    parseInt(colorStr.substring(0, 2), 16),
    parseInt(colorStr.substring(2, 4), 16),
    parseInt(colorStr.substring(4, 6), 16),
-    parseInt(colorStr.substring(6, 8), 16) || 0xFF
+    parseInt(colorStr.substring(6, 8), 16) || 0xff
  ];
-};
+}
+
+function formatHexByte (val: number): string {
+  const str = val.toString(16);
+  return str.length === 2 ? str : `0${str}`;
+}
+
+/**
+ * Get the hex color at the given pixel coordinate in an image.
+ */
+function getPixelColor (
+  image: Electron.NativeImage,
+  point: Electron.Point
+): string {
+  // image.crop crashes if point is fractional, so round to prevent that crash
+  const pixel = image.crop({
+    x: Math.round(point.x),
+    y: Math.round(point.y),
+    width: 1,
+    height: 1
+  });
+  // TODO(samuelmaddock): NativeImage.toBitmap() should return the raw pixel
+  // color, but it sometimes differs. Why is that?
+  const [b, g, r] = pixel.toBitmap();
+  return `#${formatHexByte(r)}${formatHexByte(g)}${formatHexByte(b)}`;
+}

 /** Calculate euclidian distance between colors. */
-const colorDistance = (hexColorA: string, hexColorB: string) => {
+function colorDistance (hexColorA: string, hexColorB: string): number {
  const colorA = hexToRgba(hexColorA);
  const colorB = hexToRgba(hexColorB);
  if (!colorA || !colorB) return -1;
@ -77,20 +60,109 @@ const colorDistance = (hexColorA: string, hexColorB: string) => {
      Math.pow(colorB[1] - colorA[1], 2) +
      Math.pow(colorB[2] - colorA[2], 2)
  );
-};
+}

 /**
 * Determine if colors are similar based on distance. This can be useful when
 * comparing colors which may differ based on lossy compression.
 */
-export const areColorsSimilar = (
+function areColorsSimilar (
  hexColorA: string,
  hexColorB: string,
  distanceThreshold = 90
-): boolean => {
+): boolean {
  const distance = colorDistance(hexColorA, hexColorB);
  return distance <= distanceThreshold;
-};
+}
+
+function imageCenter (image: NativeImage): Electron.Point {
+  const size = image.getSize();
+  return {
+    x: size.width / 2,
+    y: size.height / 2
+  };
+}
+/**
+ * Utilities for creating and inspecting a screen capture.
+ *
+ * NOTE: Not yet supported on Linux in CI due to empty sources list.
+ */
+export class ScreenCapture {
+  /** Use the async constructor `ScreenCapture.create()` instead. */
+  private constructor (image: NativeImage) {
+    this.image = image;
+  }
+
+  public static async create (): Promise<ScreenCapture> {
+    const display = screen.getPrimaryDisplay();
+    return ScreenCapture._createImpl(display);
+  }
+
+  public static async createForDisplay (
+    display: Electron.Display
+  ): Promise<ScreenCapture> {
+    return ScreenCapture._createImpl(display);
+  }
+
+  public async expectColorAtCenterMatches (hexColor: string) {
+    return this._expectImpl(imageCenter(this.image), hexColor, true);
+  }
+
+  public async expectColorAtCenterDoesNotMatch (hexColor: string) {
+    return this._expectImpl(imageCenter(this.image), hexColor, false);
+  }
+
+  public async expectColorAtPointOnDisplayMatches (
+    hexColor: string,
+    findPoint: (displaySize: Electron.Size) => Electron.Point
+  ) {
+    return this._expectImpl(findPoint(this.image.getSize()), hexColor, true);
+  }
+
+  private static async _createImpl (display: Electron.Display) {
+    const sources = await desktopCapturer.getSources({
+      types: ['screen'],
+      thumbnailSize: display.size
+    });
+
+    const captureSource = sources.find(
+      (source) => source.display_id === display.id.toString()
+    );
+    if (captureSource === undefined) {
+      const displayIds = sources.map((source) => source.display_id).join(', ');
+      throw new Error(
+        `Unable to find screen capture for display '${display.id}'\n\tAvailable displays: ${displayIds}`
+      );
+    }
+
+    return new ScreenCapture(captureSource.thumbnail);
+  }
+
+  private async _expectImpl (
+    point: Electron.Point,
+    expectedColor: string,
+    matchIsExpected: boolean
+  ) {
+    const actualColor = getPixelColor(this.image, point);
+    const colorsMatch = areColorsSimilar(expectedColor, actualColor);
+    const gotExpectedResult = matchIsExpected ? colorsMatch : !colorsMatch;
+
+    if (!gotExpectedResult) {
+      // Save the image as an artifact for better debugging
+      const artifactName = await createArtifactWithRandomId(
+        (id) => `color-mismatch-${id}.png`,
+        this.image.toPNG()
+      );
+      throw new AssertionError(
+        `Expected color at (${point.x}, ${point.y}) to ${
+          matchIsExpected ? 'match' : '*not* match'
+        } '${expectedColor}', but got '${actualColor}'. See the artifact '${artifactName}' for more information.`
+      );
+    }
+  }
+
+  private image: NativeImage;
+}

 /**
 * Whether the current VM has a valid screen which can be used to capture.
@ -101,6 +173,8 @@ export const areColorsSimilar = (
 * - Win32 ia32: skipped
 */
 export const hasCapturableScreen = () => {
-  return process.platform === 'darwin' ||
-    (process.platform === 'win32' && process.arch === 'x64');
+  return (
+    process.platform === 'darwin' ||
+    (process.platform === 'win32' && process.arch === 'x64')
+  );
 };
--- a/spec/webview-spec.ts
+++ b/spec/webview-spec.ts
@ -1,6 +1,6 @@
 import * as path from 'node:path';
 import * as url from 'node:url';
-import { BrowserWindow, session, ipcMain, app, WebContents, screen } from 'electron/main';
+import { BrowserWindow, session, ipcMain, app, WebContents } from 'electron/main';
 import { closeAllWindows } from './lib/window-helpers';
 import { emittedUntil } from './lib/events-helpers';
 import { ifit, ifdescribe, defer, itremote, useRemoteContext, listen } from './lib/spec-helpers';
@ -9,7 +9,7 @@ import * as http from 'node:http';
 import * as auth from 'basic-auth';
 import { once } from 'node:events';
 import { setTimeout } from 'node:timers/promises';
-import { areColorsSimilar, captureScreen, HexColors, getPixelColor } from './lib/screen-helpers';
+import { HexColors, ScreenCapture } from './lib/screen-helpers';

 declare let WebView: any;
 const features = process._linkedBinding('electron_common_features');
@ -804,14 +804,8 @@ describe('<webview> tag', function () {

      await setTimeout(1000);

-      const display = screen.getPrimaryDisplay();
-      const screenCapture = await captureScreen();
-      const centerColor = getPixelColor(screenCapture, {
-        x: display.size.width / 2,
-        y: display.size.height / 2
-      });
-
-      expect(areColorsSimilar(centerColor, WINDOW_BACKGROUND_COLOR)).to.be.true();
+      const screenCapture = await ScreenCapture.create();
+      await screenCapture.expectColorAtCenterMatches(WINDOW_BACKGROUND_COLOR);
    });

    // Linux and arm64 platforms (WOA and macOS) do not return any capture sources
@ -823,14 +817,8 @@ describe('<webview> tag', function () {

      await setTimeout(1000);

-      const display = screen.getPrimaryDisplay();
-      const screenCapture = await captureScreen();
-      const centerColor = getPixelColor(screenCapture, {
-        x: display.size.width / 2,
-        y: display.size.height / 2
-      });
-
-      expect(areColorsSimilar(centerColor, WINDOW_BACKGROUND_COLOR)).to.be.true();
+      const screenCapture = await ScreenCapture.create();
+      await screenCapture.expectColorAtCenterMatches(WINDOW_BACKGROUND_COLOR);
    });

    // Linux and arm64 platforms (WOA and macOS) do not return any capture sources
@ -842,14 +830,8 @@ describe('<webview> tag', function () {

      await setTimeout(1000);

-      const display = screen.getPrimaryDisplay();
-      const screenCapture = await captureScreen();
-      const centerColor = getPixelColor(screenCapture, {
-        x: display.size.width / 2,
-        y: display.size.height / 2
-      });
-
-      expect(areColorsSimilar(centerColor, HexColors.WHITE)).to.be.true();
+      const screenCapture = await ScreenCapture.create();
+      await screenCapture.expectColorAtCenterMatches(HexColors.WHITE);
    });
  });