From 101cffc17e7626e4b0394ca421244c93436e0b4e Mon Sep 17 00:00:00 2001
From: Erwin Mombay <erwinm@google.com>
Date: Thu, 25 Feb 2021 14:23:30 -0800
Subject: [PATCH] apply recommendations

---
 .circleci/config.yml                       | 21 +++++++--------------
 build-system/pr-check/visual-diff-tests.js | 14 ++++++--------
 build-system/tasks/serve.js                |  4 ++--
 build-system/tasks/visual-diff/index.js    |  1 +
 4 files changed, 16 insertions(+), 24 deletions(-)

diff --git a/.circleci/config.yml b/.circleci/config.yml
index b80f22cd66364..862321db64a35 100644
--- a/.circleci/config.yml
+++ b/.circleci/config.yml
@@ -146,14 +146,9 @@ jobs:
       - setup_vm
       - install_chrome
       - run:
-          name: 'Visual Diff Tests (using << parameters.build_type >> build)'
-          command: node build-system/pr-check/visual-diff-tests.js --type << parameters.build_type >>
+          name: 'Visual Diff Tests'
+          command: node build-system/pr-check/visual-diff-tests.js
       - fail_fast
-    parameters:
-      build_type:
-        description: 'Which build type to use'
-        type: string
-        enum: ['module', 'nomodule']
   'Unit Tests':
     executor:
       name: amphtml-large-executor
@@ -198,7 +193,7 @@ jobs:
       - restore_karma_cache:
           cache_name: nomodule
       - run:
-          name: 'Nomodule Tests (using << parameters.config >>-config.json)'
+          name: 'Nomodule Tests (<< parameters.config >>-config.json)'
           command: node build-system/pr-check/nomodule-tests.js
       - save_karma_cache:
           cache_name: nomodule
@@ -219,7 +214,7 @@ jobs:
       - restore_karma_cache:
           cache_name: module
       - run:
-          name: 'Module Tests (using << parameters.config >>-config.json)'
+          name: 'Module Tests (<< parameters.config >>-config.json)'
           command: node build-system/pr-check/module-tests.js
       - save_karma_cache:
           cache_name: module
@@ -333,13 +328,11 @@ workflows:
       - 'Validator Tests':
           <<: *push_and_pr_builds
       - 'Visual Diff Tests':
-          name: 'Visual Diff Tests (<< matrix.build_type >>)'
-          matrix:
-            parameters:
-              build_type: ['module', 'nomodule']
-          <<: *push_and_pr_builds
+          name: 'Visual Diff Tests'
           requires:
+            - 'Module Build'
             - 'Nomodule Build'
+          <<: *push_and_pr_builds
       - 'Unit Tests':
           <<: *push_and_pr_builds
       - 'Unminified Tests':
diff --git a/build-system/pr-check/visual-diff-tests.js b/build-system/pr-check/visual-diff-tests.js
index fa89320e8a703..eff6735c0083e 100644
--- a/build-system/pr-check/visual-diff-tests.js
+++ b/build-system/pr-check/visual-diff-tests.js
@@ -19,7 +19,6 @@
  * @fileoverview Script that runs the visual diff tests during CI.
  */
 
-const argv = require('minimist')(process.argv.slice(2));
 const atob = require('atob');
 const {
   downloadNomoduleOutput,
@@ -31,16 +30,13 @@ const {runCiJob} = require('./ci-job');
 
 const jobName = 'visual-diff-tests.js';
 
-let baseCommand = 'gulp visual-diff';
-if (argv.type === 'module') {
-  baseCommand += ' --esm';
-}
+const baseCommand = 'gulp visual-diff';
 
 function pushBuildWorkflow() {
   downloadNomoduleOutput();
   timedExecOrDie('gulp update-packages');
   process.env['PERCY_TOKEN'] = atob(process.env.PERCY_TOKEN_ENCODED);
-  timedExecOrDie(`gulp visual-diff --nobuild --master${esmFlag}`);
+  timedExecOrDie(`${baseCommand} --nobuild --master`);
 }
 
 function prBuildWorkflow() {
@@ -48,9 +44,11 @@ function prBuildWorkflow() {
   if (buildTargetsInclude(Targets.RUNTIME, Targets.VISUAL_DIFF)) {
     downloadNomoduleOutput();
     timedExecOrDie('gulp update-packages');
-    timedExecOrDie(`gulp visual-diff --nobuild${esmFlag}`);
+    timedExecOrDie(`${baseCommand} --nobuild --esm`);
+    timedExecOrDie(`${baseCommand} --nobuild`);
   } else {
-    timedExecOrDie(`gulp visual-diff --empty${esmFlag}`);
+    timedExecOrDie(`${baseCommand} --empty --esm`);
+    timedExecOrDie(`${baseCommand} --empty`);
     printSkipMessage(
       jobName,
       'this PR does not affect the runtime or visual diff tests'
diff --git a/build-system/tasks/serve.js b/build-system/tasks/serve.js
index 565fd2764cea5..0a2294b6dc047 100644
--- a/build-system/tasks/serve.js
+++ b/build-system/tasks/serve.js
@@ -193,7 +193,7 @@ async function stopServer() {
 
 /**
  * Closes the existing server and restarts it
- * @param {?serverOptionsDef} serverOptions
+ * @param {ServerOptionsDef=} serverOptions
  */
 async function restartServer(serverOptions = {}) {
   stopServer();
@@ -226,7 +226,7 @@ async function serve() {
 
 /**
  * Starts a webserver at the repository root to serve built files.
- * @param {?ServerOptionsDef} serverOptions
+ * @param {ServerOptionsDef=} serverOptions
  */
 async function doServe(serverOptions = {}) {
   createCtrlcHandler('serve');
diff --git a/build-system/tasks/visual-diff/index.js b/build-system/tasks/visual-diff/index.js
index 890d3da20026e..37027c1a60050 100644
--- a/build-system/tasks/visual-diff/index.js
+++ b/build-system/tasks/visual-diff/index.js
@@ -389,6 +389,7 @@ async function generateSnapshots(webpages) {
   // no interactions, and each test that has in interactive tests file should
   // load those tests here.
   for (const webpage of webpages) {
+    webpage.name += argv.esm ? ' (Module)' : ' (Nomodule)';
     webpage.tests_ = {};
     if (!webpage.no_base_test) {
       webpage.tests_[''] = async () => {};