🏗 Fix and restore visual diff tests (#36654)

.circleci/config.yml

@@ -278,7 +278,6 @@ jobs:
       name: node-docker-large
     steps:
       - setup_vm
-      - install_chrome
       - run:
           name: '⭐ Visual Diff Tests ⭐'
           command: node build-system/pr-check/visual-diff-tests.js

build-system/common/ctrlcHandler.js

@@ -12,9 +12,10 @@ const killSuffix = process.platform == 'win32' ? '>NUL' : '';
  * the ongoing `amp` task.
  *
  * @param {string} command
+ * @param {number} pid
  * @return {number}
  */
-exports.createCtrlcHandler = function (command) {
+exports.createCtrlcHandler = function (command, pid = process.pid) {
   logLocalDev(
     green('Running'),
     cyan(command) + green('. Press'),
@@ -31,7 +32,7 @@ exports.createCtrlcHandler = function (command) {
     #!/bin/sh
     ctrlcHandler() {
       echo -e "${killMessage}"
-      ${killCmd} ${process.pid}
+      ${killCmd} ${pid}
       exit 1
     }
     trap 'ctrlcHandler' INT

build-system/pr-check/visual-diff-tests.js

@@ -14,17 +14,15 @@ const jobName = 'visual-diff-tests.js';
  * Steps to run during push builds.
  */
 function pushBuildWorkflow() {
-  // TODO(#36604): restore: timedExecOrDie('amp visual-diff --nobuild --main');
-  timedExecOrDie('amp visual-diff --empty');
+  timedExecOrDie('amp visual-diff --nobuild --main');
 }
 
 /**
  * Steps to run during PR builds.
  */
 function prBuildWorkflow() {
   if (buildTargetsInclude(Targets.RUNTIME, Targets.VISUAL_DIFF)) {
-    // TODO(#36604): restore: timedExecOrDie('amp visual-diff --nobuild');
-    timedExecOrDie('amp visual-diff --empty');
+    timedExecOrDie('amp visual-diff --nobuild');
   } else {
     timedExecOrDie('amp visual-diff --empty');
     skipDependentJobs(

build-system/tasks/visual-diff/index.js

@@ -4,6 +4,7 @@ const argv = require('minimist')(process.argv.slice(2));
 const atob = require('atob');
 const fs = require('fs');
 const JSON5 = require('json5');
+const os = require('os');
 const path = require('path');
 const Percy = require('@percy/core');
 const percySnapshot = require('@percy/puppeteer');
@@ -27,7 +28,7 @@ const {
   shortSha,
 } = require('../../common/git');
 const {buildRuntime} = require('../../common/utils');
-const {cyan, yellow} = require('kleur/colors');
+const {cyan, green, red, yellow} = require('kleur/colors');
 const {isCiBuild} = require('../../common/ci');
 const {startServer, stopServer} = require('../serve');
 
@@ -61,10 +62,11 @@ const VIEWPORT_HEIGHT = 100000;
 const HOST = 'localhost';
 const PORT = 8000;
 const PERCY_AGENT_PORT = 5338;
-const NAVIGATE_TIMEOUT_MS = 30000;
-const MAX_PARALLEL_TABS = 5;
 const WAIT_FOR_TABS_MS = 1000;
 
+// Multiple tabs speed up the performance of the visual diff tests.
+const MAX_PARALLEL_TABS = os.cpus().length;
+
 const ROOT_DIR = path.resolve(__dirname, '../../../');
 
 // JavaScript snippets that execute inside the page.
@@ -231,11 +233,21 @@ async function launchWebServer() {
  */
 async function launchBrowser(browserFetcher) {
   const browserOptions = {
-    args: ['--no-sandbox', '--disable-extensions', '--disable-gpu'],
+    args: [
+      '--disable-background-media-suspend',
+      '--disable-background-timer-throttling',
+      '--disable-backgrounding-occluded-windows',
+      '--disable-extensions',
+      '--disable-gpu',
+      '--disable-renderer-backgrounding',
+      '--no-sandbox',
+      '--no-startup-window',
+    ],
     dumpio: argv.chrome_debug,
     headless: true,
     executablePath: browserFetcher.revisionInfo(PUPPETEER_CHROMIUM_REVISION)
       .executablePath,
+    waitForInitialPage: false,
   };
   return await puppeteer.launch(browserOptions);
 }
@@ -251,7 +263,8 @@ async function launchBrowser(browserFetcher) {
 async function newPage(browser, viewport = null) {
   log('verbose', 'Creating new tab');
 
-  const page = await browser.newPage();
+  const context = await browser.createIncognitoBrowserContext();
+  const page = await context.newPage();
   page.setDefaultNavigationTimeout(0);
   await page.setJavaScriptEnabled(true);
   await page.setRequestInterception(true);
@@ -456,6 +469,32 @@ async function runVisualTests(browser, webpages) {
   }
 }
 
+/**
+ * Pretty-prints the current test status of each page.
+ * @param {!Array<!puppeteer.Page>} allPages
+ * @param {!Array<!puppeteer.Page>} availablePages
+ * @param {!puppeteer.Page} thisPage
+ * @param {string} thisPageText
+ * @return {string}
+ */
+function drawBoxes(allPages, availablePages, thisPage, thisPageText) {
+  return (
+    '[' +
+    allPages
+      .map((page) => {
+        if (page === thisPage) {
+          return thisPageText;
+        } else if (availablePages.includes(page)) {
+          return ' ';
+        } else {
+          return yellow('█');
+        }
+      })
+      .join(' ') +
+    ']'
+  );
+}
+
 /**
  * Generates Percy snapshots for a set of given webpages.
  *
@@ -498,10 +537,9 @@ async function snapshotWebpages(browser, webpages) {
       const name = testName ? `${pageName} (${testName})` : pageName;
       log(
         'info',
+        drawBoxes(allPages, availablePages, page, yellow('▄')),
         'Starting test',
-        yellow(name),
-        'on tab',
-        yellow(`#${allPages.indexOf(page) + 1}`)
+        yellow(name)
       );
 
       await resetPage(page, viewport);
@@ -512,45 +550,10 @@ async function snapshotWebpages(browser, webpages) {
       };
       page.on('console', consoleLogger);
 
-      // Puppeteer is flaky when it comes to catching navigation requests, so
-      // retry the page navigation up to NAVIGATE_RETRIES times and eventually
-      // ignore a final timeout. If this ends up being a real non-loading page
-      // error, this will be caught in the resulting Percy build. Also attempt
-      // to wait until there are no more network requests. This method is flaky
-      // since Puppeteer doesn't always understand Chrome's network activity, so
-      // ignore timeouts again.
       const pagePromise = (async () => {
         try {
-          /** @type {Promise<void>} */
-          const responseWatcher = new Promise((resolve, reject) => {
-            const responseTimeout = setTimeout(() => {
-              reject(
-                new puppeteer.TimeoutError(
-                  `Response was not received in test ${testName} for page ` +
-                    `${webpage.url} after ${NAVIGATE_TIMEOUT_MS}ms`
-                )
-              );
-            }, NAVIGATE_TIMEOUT_MS);
-
-            page.once('response', (response) => {
-              log(
-                'verbose',
-                'Response for url',
-                yellow(response.url()),
-                'with status',
-                cyan(response.status()),
-                cyan(response.statusText())
-              );
-              clearTimeout(responseTimeout);
-              resolve();
-            });
-          });
-
           log('verbose', 'Navigating to page', yellow(webpage.url));
-          await Promise.all([
-            responseWatcher,
-            page.goto(fullUrl, {waitUntil: 'networkidle2'}),
-          ]);
+          await page.goto(fullUrl, {waitUntil: 'networkidle0'});
 
           log(
             'verbose',
@@ -679,12 +682,18 @@ async function snapshotWebpages(browser, webpages) {
         }
 
         log(
-          hasWarnings ? 'warning' : 'info',
+          'info',
+          drawBoxes(
+            allPages,
+            availablePages,
+            page,
+            (hasWarnings ? red : green)('▀')
+          ),
           'Finished test',
           yellow(name),
           hasWarnings ? 'with warnings' : ''
         );
-        page.removeListener('console', consoleLogger);
+        page.off('console', consoleLogger);
         availablePages.push(page);
       })();
       pagePromises.push(pagePromise);
@@ -780,6 +789,10 @@ async function performVisualTests(browserFetcher) {
   setDebuggingLevel();
 
   const browser = await launchBrowser(browserFetcher);
+  const handlerProcess = createCtrlcHandler(
+    'visual-diff:headless-browser',
+    browser.process()?.pid
+  );
   await launchWebServer();
 
   try {
@@ -800,6 +813,7 @@ async function performVisualTests(browserFetcher) {
     }
   } finally {
     await browser.close();
+    exitCtrlcHandler(handlerProcess);
     await stopServer();
   }
 }