Retake visual screenshots that differ from the baseline or are new

.github/workflow-scripts/__tests__/visual-compare-test.js

@@ -9,7 +9,7 @@
  */
 
 const {compareCapture, compareImages, policyFor} = require('../visual/compare');
-const {resolveContext} = require('../visual/compare-run');
+const {resolveContext, stageBaseline} = require('../visual/compare-run');
 const {buildManifest, sha256} = require('../visual/manifest');
 const {readPngDimensions} = require('../visual/png');
 const {validateCapture} = require('../visual/validate');
@@ -117,6 +117,26 @@ describe('visual comparison', () => {
       expect(() => validateCapture(directory)).toThrow('path must be');
     });
 
+    test.each([
+      [true, null],
+      [false, 'unstable must be true or absent'],
+      ['yes', 'unstable must be true or absent'],
+    ])('validates unstable: %p', (unstable, error) => {
+      const manifest = writeCapture(directory, {
+        'text/width-mode/default': makePng(4, 4),
+      });
+      manifest.images[0].unstable = unstable;
+      fs.writeFileSync(
+        path.join(directory, 'manifest.json'),
+        JSON.stringify(manifest),
+      );
+      if (error == null) {
+        expect(validateCapture(directory)).toEqual(manifest);
+      } else {
+        expect(() => validateCapture(directory)).toThrow(error);
+      }
+    });
+
     test('rejects duplicates, mismatched hashes and unlisted files', () => {
       const manifest = writeCapture(directory, {
         'text/width-mode/default': makePng(4, 4),
@@ -261,6 +281,39 @@ describe('visual comparison', () => {
     });
   });
 
+  describe('stageBaseline', () => {
+    test('leaves out unstable screenshots', () => {
+      const captureDir = path.join(directory, 'capture');
+      const outDir = path.join(directory, 'out');
+      const manifest = writeCapture(captureDir, {
+        'text/stable/default': makePng(4, 4),
+        'text/unstable/default': makePng(4, 4),
+      });
+      manifest.images[1].unstable = true;
+      stageBaseline({outDir, captureDir, manifest, sha: SHA_A});
+      const prefix = `baselines/${manifest.environmentKey}/${SHA_A}`;
+      const staged = JSON.parse(
+        fs.readFileSync(
+          path.join(outDir, 'baseline-manifests', prefix, 'manifest.json'),
+          'utf8',
+        ),
+      );
+      expect(staged.images.map(image => image.id)).toEqual([
+        'text/stable/default',
+      ]);
+      expect(
+        fs.existsSync(
+          path.join(
+            outDir,
+            'baseline-images',
+            prefix,
+            'text/unstable/default.png',
+          ),
+        ),
+      ).toBe(false);
+    });
+  });
+
   describe('resolveContext', () => {
     const ENV = {
       CAPTURE_HEAD_SHA: SHA_A,

.github/workflow-scripts/__tests__/visual-report-test.js

@@ -150,14 +150,17 @@ describe('visual reports and approval', () => {
             {id: 'a/b/same', status: 'unchanged'},
             changed('a/b/changed'),
           ]),
-          platform('ios', [{id: 'a/b/new', status: 'added', captured: {}}]),
+          platform('ios', [
+            {id: 'a/b/new', status: 'added', captured: {}},
+            {id: 'a/b/flaky', status: 'added', captured: {unstable: true}},
+          ]),
         ],
         headSha: SHA_A,
         gateSummary: 'Needs approval.',
       });
       expect(body.startsWith(COMMENT_MARKER)).toBe(true);
       expect(body).toContain(
-        '**2 visual difference(s)** at `aaaaaaaaaaaa`. Needs approval.',
+        '**3 visual difference(s)** at `aaaaaaaaaaaa`. Needs approval.',
       );
       expect(body).toContain(
         '### android `a/b/changed`: Changed, 3 px (3.000%)',
@@ -168,7 +171,10 @@ describe('visual reports and approval', () => {
       expect(body).toContain(
         '<details><summary>Expected and Captured</summary>',
       );
-      expect(body).toContain('### ios `a/b/new`: New');
+      expect(body).toContain('### ios `a/b/new`: New\n');
+      expect(body).toContain(
+        '### ios `a/b/flaky`: New, unstable: its two captures differed',
+      );
       expect(body).toContain('<details open><summary>Captured</summary>');
       expect(body).not.toContain('a/b/same');
     });

.github/workflow-scripts/__tests__/visual-retake-test.js

@@ -0,0 +1,188 @@
+/**
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ *
+ * @noflow
+ * @format
+ */
+
+const {buildManifest} = require('../visual/manifest');
+const {
+  flowsTaking,
+  judgeRetakes,
+  retakeCandidates,
+  retakeScreenshots,
+  screenshotIds,
+} = require('../visual/retake');
+const {ANDROID_ENVIRONMENT, SHA_A} = require('./__fixtures__/visual');
+const fs = require('node:fs');
+const os = require('node:os');
+const path = require('node:path');
+
+const baselineManifest = {
+  images: [
+    {id: 'switch/basic/default', sha256: 'a'},
+    {id: 'switch/basic/toggled', sha256: 'b'},
+  ],
+};
+
+const BUNNY = path.join(
+  __dirname,
+  '../../../packages/rn-tester/js/assets/bunny.png',
+);
+
+const images = [
+  {id: 'switch/basic/default', sha256: 'a'},
+  {id: 'switch/basic/toggled', sha256: 'x'},
+  {id: 'view/basic/default', sha256: 'n'},
+];
+
+describe('visual retake', () => {
+  test('reads the test IDs a flow captures', () => {
+    const flow = [
+      'appId: ${APP_ID}',
+      '---',
+      '- takeScreenshot:',
+      '    path: ${VISUAL_OUTPUT_DIR}/switch/basic/default',
+      '    cropOn:',
+      "      id: 'switch-basic'",
+      '- takeScreenshot:',
+      "    path: '${VISUAL_OUTPUT_DIR}/switch/basic/toggled'",
+    ].join('\n');
+    expect(screenshotIds(flow)).toEqual([
+      'switch/basic/default',
+      'switch/basic/toggled',
+    ]);
+  });
+
+  test('selects only the flows that take the given IDs', () => {
+    const files = {
+      '/flows/switch.yml':
+        '- takeScreenshot: ${VISUAL_OUTPUT_DIR}/switch/basic/toggled',
+      '/flows/text.yml':
+        '- takeScreenshot: ${VISUAL_OUTPUT_DIR}/text/width-mode/default',
+      '/flows/view.yml':
+        '- takeScreenshot: ${VISUAL_OUTPUT_DIR}/view/basic/default',
+    };
+    expect(
+      flowsTaking(
+        Object.keys(files),
+        ['text/width-mode/default', 'switch/basic/toggled'],
+        file => files[file],
+      ),
+    ).toEqual(['/flows/switch.yml', '/flows/text.yml']);
+  });
+
+  test('retakes changed and new screenshots but not unchanged ones', () => {
+    expect(retakeCandidates(images, baselineManifest)).toEqual({
+      changed: ['switch/basic/toggled'],
+      added: ['view/basic/default'],
+    });
+  });
+
+  test.each([
+    ['matches the baseline', 'b', ['switch/basic/toggled']],
+    ['still differs', 'y', []],
+    ['is missing', null, []],
+  ])('replaces a changed screenshot if its retake %s', (_, sha, replaced) => {
+    expect(
+      judgeRetakes({
+        changed: ['switch/basic/toggled'],
+        added: [],
+        images,
+        baselineManifest,
+        retakeSha: () => sha,
+      }).replaced,
+    ).toEqual(replaced);
+  });
+
+  test.each([
+    ['is identical', 'n', []],
+    ['differs', 'y', ['view/basic/default']],
+    ['is missing', null, ['view/basic/default']],
+  ])('marks a new screenshot unstable if its retake %s', (_, sha, unstable) => {
+    expect(
+      judgeRetakes({
+        changed: [],
+        added: ['view/basic/default'],
+        images,
+        baselineManifest,
+        retakeSha: () => sha,
+      }).unstable,
+    ).toEqual(unstable);
+  });
+
+  test('retakes every screenshot when the environment has no baseline', async () => {
+    // The repo's Jest config fakes timers, and the retake waits on one.
+    jest.useRealTimers();
+    const directory = fs.mkdtempSync(path.join(os.tmpdir(), 'visual-test-'));
+    const fetchSpy = jest
+      .spyOn(global, 'fetch')
+      .mockResolvedValue(new Response(null, {status: 404}));
+    try {
+      const outputDir = path.join(directory, 'out');
+      const png = fs.readFileSync(BUNNY);
+      const write = (dir, id, contents) => {
+        fs.mkdirSync(path.join(dir, path.dirname(id)), {recursive: true});
+        fs.writeFileSync(path.join(dir, `${id}.png`), contents);
+      };
+      write(outputDir, 'view/basic/default', png);
+      write(outputDir, 'view/basic/flaky', png);
+      const flow = path.join(directory, 'view.yml');
+      fs.writeFileSync(
+        flow,
+        [
+          '- takeScreenshot: ${VISUAL_OUTPUT_DIR}/view/basic/default',
+          '- takeScreenshot: ${VISUAL_OUTPUT_DIR}/view/basic/flaky',
+        ].join('\n'),
+      );
+      const runFlows = jest.fn((flows, retakeDir) => {
+        write(retakeDir, 'view/basic/default', png);
+        write(retakeDir, 'view/basic/flaky', Buffer.concat([png, png]));
+      });
+      const {unstable} = await retakeScreenshots({
+        platform: 'android',
+        environment: ANDROID_ENVIRONMENT,
+        flows: [flow],
+        outputDir,
+        publicBaseUrl: 'https://storage.example',
+        runFlows,
+        candidateShas: [SHA_A],
+        delayMs: 0,
+      });
+      expect(runFlows).toHaveBeenCalledWith([flow], `${outputDir}-retake`);
+      expect(unstable).toEqual(['view/basic/flaky']);
+    } finally {
+      fetchSpy.mockRestore();
+      fs.rmSync(directory, {recursive: true, force: true});
+    }
+  });
+
+  test('marks unstable screenshots in the manifest', () => {
+    const directory = fs.mkdtempSync(path.join(os.tmpdir(), 'visual-test-'));
+    try {
+      const png = fs.readFileSync(BUNNY);
+      for (const id of ['view/basic/default', 'view/basic/flaky']) {
+        fs.mkdirSync(path.join(directory, path.dirname(id)), {recursive: true});
+        fs.writeFileSync(path.join(directory, `${id}.png`), png);
+      }
+      const manifest = buildManifest({
+        captureDir: directory,
+        platform: 'android',
+        environment: ANDROID_ENVIRONMENT,
+        git: null,
+        unstableIds: ['view/basic/flaky'],
+      });
+      expect(manifest.images.map(({id, unstable}) => ({id, unstable}))).toEqual(
+        [
+          {id: 'view/basic/default', unstable: undefined},
+          {id: 'view/basic/flaky', unstable: true},
+        ],
+      );
+    } finally {
+      fs.rmSync(directory, {recursive: true, force: true});
+    }
+  });
+});

.github/workflow-scripts/visual/capture.js

@@ -12,9 +12,12 @@
  * Captures every visual flow in one Maestro invocation, so the JVM and
  * driver startup (around two minutes on an iOS simulator) is paid once per
  * job rather than once per flow. Flows that fail are rerun, and only those,
- * on later attempts.
+ * on later attempts. Screenshots that then differ from the baseline, or
+ * have none, are retaken once (see retake.js).
  */
 
+const {readAndroidEnvironment, readIosEnvironment} = require('./manifest');
+const {retakeScreenshots} = require('./retake');
 const childProcess = require('node:child_process');
 const fs = require('node:fs');
 const path = require('node:path');
@@ -28,7 +31,10 @@ node capture.js <platform> <app_path> <app_id> <flows_dir> <output_dir> [ios_udi
 
 Installs the app, then runs every flow in <flows_dir> in one Maestro
 invocation, writing screenshots under <output_dir>. Failed flows are retried
-up to ${DEFAULT_ATTEMPTS} attempts in total (VISUAL_CAPTURE_ATTEMPTS).
+up to ${DEFAULT_ATTEMPTS} attempts in total (VISUAL_CAPTURE_ATTEMPTS). With
+VISUAL_PUBLIC_BASE_URL set, screenshots that differ from the baseline or have
+none are retaken once, and the IDs of new screenshots whose two captures
+differ are written as a JSON array to VISUAL_UNSTABLE_PATH.
 `;
 
 /** The flows directly inside `flowsDir`, sorted. Helpers live elsewhere. */
@@ -140,7 +146,7 @@ function writeSummary(lines) {
   }
 }
 
-function main(args = process.argv.slice(2)) {
+async function main(args = process.argv.slice(2)) {
   const [platform, appPath, appId, flowsDir, outputDir, udid] = args;
   if (
     !['android', 'ios'].includes(platform) ||
@@ -193,14 +199,57 @@ function main(args = process.argv.slice(2)) {
     summary.push('', `Failed after ${attempts} attempt(s):`);
     summary.push(...pending.map(flow => `- \`${flow}\``));
   }
+
+  const publicBaseUrl = process.env.VISUAL_PUBLIC_BASE_URL;
+  if (publicBaseUrl != null && publicBaseUrl !== '') {
+    try {
+      const {unstable, lines} = await retakeScreenshots({
+        platform,
+        environment:
+          platform === 'android'
+            ? readAndroidEnvironment()
+            : readIosEnvironment(udid),
+        flows: allFlows,
+        outputDir,
+        publicBaseUrl,
+        runFlows: (flows, retakeDir) =>
+          runAttempt(
+            maestroArgs({
+              flows,
+              appId,
+              outputDir: retakeDir,
+              reportPath: path.resolve('visual-capture-report-retake.xml'),
+              udid,
+              attempt: attempts + 1,
+            }),
+          ),
+      });
+      if (lines.length > 0) {
+        summary.push('', ...lines);
+      }
+      if (process.env.VISUAL_UNSTABLE_PATH) {
+        fs.writeFileSync(
+          process.env.VISUAL_UNSTABLE_PATH,
+          `${JSON.stringify(unstable)}\n`,
+        );
+      }
+    } catch (error) {
+      // The original screenshots are still valid, so compare goes ahead
+      // with them.
+      console.warn('Retaking screenshots failed:', error);
+    }
+  }
   writeSummary(summary);
   if (pending.length > 0) {
     process.exitCode = 1;
   }
 }
 
 if (require.main === module) {
-  main();
+  main().catch(error => {
+    console.error(error);
+    process.exitCode = 1;
+  });
 }
 
 module.exports = {collectFlows, failedFlowsFromJUnit, maestroArgs};