Visual diff comments and cleanup (ampproject#21032)

* visual-diff comments, explanation, and minor cleanup

* Make the viewport resizing more clear

* Double the number of parallel tabs

* Fixed typos

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

@@ -54,13 +54,17 @@ const HOST = 'localhost';
 const PORT = 8000;
 const WEBSERVER_TIMEOUT_RETRIES = 10;
 const NAVIGATE_TIMEOUT_MS = 3000;
-const MAX_PARALLEL_TABS = 10;
+const MAX_PARALLEL_TABS = 20;
 const WAIT_FOR_TABS_MS = 1000;
 const BUILD_STATUS_URL = 'https://amphtml-percy-status-checker.appspot.com/status';
 
 const ROOT_DIR = path.resolve(__dirname, '../../../');
-const WRAP_IN_IFRAME_SCRIPT = fs.readFileSync(
+
+// Script snippets that execute inside the page.
+const WRAP_IN_IFRAME_SNIPPET = fs.readFileSync(
     path.resolve(__dirname, 'snippets/iframe-wrapper.js'), 'utf8');
+const REMOVE_AMP_SCRIPTS_SNIPPET = fs.readFileSync(
+    path.resolve(__dirname, 'snippets/remove-amp-scripts.js'), 'utf8');
 
 let browser_;
 let webServerProcess_;
@@ -196,13 +200,18 @@ async function launchBrowser() {
  * Opens a new browser tab, resizes its viewport, and returns a Page handler.
  *
  * @param {!puppeteer.Browser} browser a Puppeteer controlled browser.
+ * @param {JsonObject} viewport optional viewport size object with numeric
+ *     fields `width` and `height`.
  */
-async function newPage(browser) {
+async function newPage(browser, viewport = null) {
+  const width = viewport ? viewport.width : VIEWPORT_WIDTH;
+  const height = viewport ? viewport.height : VIEWPORT_HEIGHT;
+
+  log('verbose', 'Creating new page with viewport size of',
+      colors.yellow(`${width}×${height}`));
+
   const page = await browser.newPage();
-  await page.setViewport({
-    width: VIEWPORT_WIDTH,
-    height: VIEWPORT_HEIGHT,
-  });
+  await page.setViewport({width, height});
   page.setDefaultNavigationTimeout(NAVIGATE_TIMEOUT_MS);
   await page.setJavaScriptEnabled(true);
   return page;
@@ -352,18 +361,10 @@ async function snapshotWebpages(percy, browser, webpages) {
         await sleep(WAIT_FOR_TABS_MS);
       }
 
-      const page = await newPage(browser);
       const name = testName ? `${pageName} (${testName})` : pageName;
       log('verbose', 'Visual diff test', colors.yellow(name));
 
-      if (viewport) {
-        log('verbose', 'Setting explicit viewport size of',
-            colors.yellow(`${viewport.width}×${viewport.height}`));
-        await page.setViewport({
-          width: viewport.width,
-          height: viewport.height,
-        });
-      }
+      const page = await newPage(browser, viewport);
       log('verbose', 'Navigating to page', colors.yellow(webpage.url));
 
       // Navigate to an empty page first to support different webpages that only
@@ -382,51 +383,61 @@ async function snapshotWebpages(percy, browser, webpages) {
             log('verbose', 'Navigation to page', colors.yellow(name),
                 'is done, verifying page');
 
+            // Visibility evaluations can only be performed on the active tab,
+            // even in the headless browser mode.
             await page.bringToFront();
 
+            // Perform visibility checks: wait for all AMP built-in loader dots
+            // to disappear (i.e., all visible components are finished being
+            // layed out and external resources such as images are loaded and
+            // displayed), then, depending on the test configurations, wait for
+            // invisibility/visibility of specific elements that match the
+            // configured CSS selectors.
             await waitForLoaderDots(page, name);
-            if (webpage.loading_incomplete_css) {
+            if (webpage.loading_incomplete_selectors) {
               await verifySelectorsInvisible(
-                  page, name, webpage.loading_incomplete_css);
+                  page, name, webpage.loading_incomplete_selectors);
             }
-            if (webpage.loading_complete_css) {
+            if (webpage.loading_complete_selectors) {
               await verifySelectorsVisible(
-                  page, name, webpage.loading_complete_css);
+                  page, name, webpage.loading_complete_selectors);
             }
 
+            // Based on test configuration, wait for a specific amount of time.
             if (webpage.loading_complete_delay_ms) {
               log('verbose', 'Waiting',
                   colors.cyan(`${webpage.loading_complete_delay_ms}ms`),
                   'for loading to complete');
               await sleep(webpage.loading_complete_delay_ms);
             }
 
+            // Run any other custom code located in the test's interactive_tests
+            // file. If there is no interactive test, this defaults to an empty
+            // function.
             await testFunction(page, name);
 
-            const snapshotOptions = Object.assign({}, DEFAULT_SNAPSHOT_OPTIONS);
+            // Execute post-scripts that clean up the page's HTML and send
+            // prepare it for snapshotting on Percy. See comments inside the
+            // snippet files for description of each.
+            await page.evaluate(REMOVE_AMP_SCRIPTS_SNIPPET);
+            // TODO(#20630): add a snippet to freeze form inputs
 
+            // Create a default set of snapshot options for Percy and modify
+            // them based on the test's configuration.
+            const snapshotOptions = Object.assign({}, DEFAULT_SNAPSHOT_OPTIONS);
             if (webpage.enable_percy_javascript) {
               snapshotOptions.enableJavaScript = true;
-              // Remove all scripts that have an external source, leaving only
-              // those scripts that are inlined in the page inside a <script>
-              // tag.
-              await page.evaluate(
-                  'document.head.querySelectorAll("script[src]").forEach(' +
-                  'node => node./*OK*/remove())');
             }
 
             if (viewport) {
               snapshotOptions.widths = [viewport.width];
               log('verbose', 'Wrapping viewport-constrained page in an iframe');
-              await page.evaluate(WRAP_IN_IFRAME_SCRIPT
+              await page.evaluate(WRAP_IN_IFRAME_SNIPPET
                   .replace(/__WIDTH__/g, viewport.width)
                   .replace(/__HEIGHT__/g, viewport.height));
-              await page.setViewport({
-                width: VIEWPORT_WIDTH,
-                height: VIEWPORT_HEIGHT,
-              });
             }
 
+            // Finally, send the snapshot to percy.
             await percy.snapshot(name, page, snapshotOptions);
             log('travis', colors.cyan('●'));
           })

build-system/tasks/visual-diff/snippets/remove-amp-scripts.js

@@ -0,0 +1,6 @@
+// This file is executed via Puppeteer's page.evaluate on a document to remove
+// all <script> tags that import AMP pages. This makes for cleaner diffs and
+// prevents "double-execution" of AMP scripts when enableJavaScript=true.
+
+document.head.querySelectorAll("script[src]")
+    .forEach(node => node./*OK*/remove());