Merge branch 'master' into experimental/feature-targeting

package.json

@@ -42,7 +42,7 @@
     "test:site": "npm run clean:site && ./scripts/site-generator-test.sh"
   },
   "devDependencies": {
-    "@google-cloud/datastore": "^2.0.0",
+    "@google-cloud/datastore": "^3.0.1",
     "@octokit/rest": "^16.1.0",
     "argparse": "^1.0.10",
     "ascii-table": "0.0.9",
@@ -90,6 +90,7 @@
     "istanbul": "^0.4.4",
     "istanbul-instrumenter-loader": "^3.0.0",
     "jimp": "^0.2.28",
+    "jsdom": "^13.1.0",
     "json-stable-stringify": "^1.0.1",
     "karma": "^3.0.0",
     "karma-chrome-launcher": "^2.0.0",

packages/mdc-tab-scroller/util.js

@@ -56,7 +56,7 @@ function computeHorizontalScrollbarHeight(documentObj, shouldCacheResult = true)
 
 /**
  * @param {!Object} HTMLElementPrototype
- * @return {!Array<string>}
+ * @return {string}
  */
 function getMatchesProperty(HTMLElementPrototype) {
   return [

test/screenshot/.gitignore

@@ -1 +1,2 @@
 index.html
+*.pid

test/screenshot/golden.json

@@ -853,21 +853,21 @@
     }
   },
   "spec/mdc-menu/classes/menu-selection-group.html": {
-    "public_url": "https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2018/12/13/21_10_43_629/spec/mdc-menu/classes/menu-selection-group.html?utm_source=golden_json",
+    "public_url": "https://storage.googleapis.com/mdc-web-screenshot-tests/advorak/2019/01/18/16_42_10_651/spec/mdc-menu/classes/menu-selection-group.html?utm_source=golden_json",
     "screenshots": {
       "desktop_windows_chrome@latest": "https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2018/12/13/21_10_43_629/spec/mdc-menu/classes/menu-selection-group.html.windows_chrome_71.png",
-      "desktop_windows_edge@latest": "https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2018/12/13/21_10_43_629/spec/mdc-menu/classes/menu-selection-group.html.windows_edge_17.png",
+      "desktop_windows_edge@latest": "https://storage.googleapis.com/mdc-web-screenshot-tests/advorak/2019/01/18/16_42_10_651/spec/mdc-menu/classes/menu-selection-group.html.windows_edge_17.png",
       "desktop_windows_firefox@latest": "https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2018/12/13/21_10_43_629/spec/mdc-menu/classes/menu-selection-group.html.windows_firefox_63.png",
       "desktop_windows_ie@11": "https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2018/12/13/21_10_43_629/spec/mdc-menu/classes/menu-selection-group.html.windows_ie_11.png"
     }
   },
   "spec/mdc-menu/issues/4025.html": {
-    "public_url": "https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2018/12/14/20_42_47_011/spec/mdc-menu/issues/4025.html?utm_source=golden_json",
+    "public_url": "https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2019/01/18/07_50_32_667/spec/mdc-menu/issues/4025.html?utm_source=golden_json",
     "screenshots": {
-      "desktop_windows_chrome@latest": "https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2018/12/14/20_42_47_011/spec/mdc-menu/issues/4025.html.windows_chrome_71.png",
-      "desktop_windows_edge@latest": "https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2018/12/14/20_42_47_011/spec/mdc-menu/issues/4025.html.windows_edge_17.png",
-      "desktop_windows_firefox@latest": "https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2018/12/14/20_42_47_011/spec/mdc-menu/issues/4025.html.windows_firefox_63.png",
-      "desktop_windows_ie@11": "https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2018/12/14/20_42_47_011/spec/mdc-menu/issues/4025.html.windows_ie_11.png"
+      "desktop_windows_chrome@latest": "https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2019/01/18/07_50_32_667/spec/mdc-menu/issues/4025.html.windows_chrome_71.png",
+      "desktop_windows_edge@latest": "https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2019/01/18/07_50_32_667/spec/mdc-menu/issues/4025.html.windows_edge_17.png",
+      "desktop_windows_firefox@latest": "https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2019/01/18/07_50_32_667/spec/mdc-menu/issues/4025.html.windows_firefox_64.png",
+      "desktop_windows_ie@11": "https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2019/01/18/07_50_32_667/spec/mdc-menu/issues/4025.html.windows_ie_11.png"
     }
   },
   "spec/mdc-radio/classes/baseline.html": {

test/screenshot/infra/commands/build.js

@@ -33,7 +33,8 @@ const IndexCommand = require('./index');
 const Logger = require('../lib/logger');
 const ProcessManager = require('../lib/process-manager');
 const ProtoCommand = require('./proto');
-const {TEST_DIR_RELATIVE_PATH} = require('../lib/constants');
+const {ExitCode} = require('../lib/constants');
+const {TEST_DIR_RELATIVE_PATH, BUILD_PID_FILE_PATH_RELATIVE} = require('../lib/constants');
 
 class BuildCommand {
   constructor() {
@@ -59,6 +60,7 @@ class BuildCommand {
       return;
     }
 
+    await this.setRunningProcessId_();
     await this.cleanCommand_.runAsync();
 
     this.logger_.foldStart('screenshot.build', 'Compiling source files');
@@ -159,22 +161,38 @@ class BuildCommand {
   }
 
   /**
-   * TODO(acvdorak): Store PID in local text file instead of scanning through running processes
-   * @return {!Promise<?number>}
+   * @return {!Promise<?string>}
    * @private
    */
   async getExistingProcessId_() {
-    /** @type {!Array<!PsNodeProcess>} */
-    const allProcs = await this.processManager_.getRunningProcessesInPwdAsync('node', 'build');
-    const buildProcs = allProcs.filter((proc) => {
-      const [script, command] = proc.arguments;
-      return (
-        script.endsWith('/run.js') &&
-        command === 'build'
-      );
-    });
+    return await this.processManager_.getRunningPid(BUILD_PID_FILE_PATH_RELATIVE, 'node', 'build');
+  }
+
+  /**
+   * @return {!Promise<void>}
+   * @private
+   */
+  async setRunningProcessId_() {
+    await this.processManager_.setRunningPid(BUILD_PID_FILE_PATH_RELATIVE, process.pid);
+
+    const exit = (code, err) => {
+      if (err) {
+        console.error(err);
+      }
+      this.processManager_.deletePidFileSync(BUILD_PID_FILE_PATH_RELATIVE);
+      process.exit(code);
+    };
+
+    // TODO(acdvorak): Create a centralized class to manage global exit handlers?
+
+    // catches ctrl+c event
+    process.on('SIGINT', () => exit(ExitCode.SIGINT));
+
+    // catches "kill pid"
+    process.on('SIGTERM', () => exit(ExitCode.SIGTERM));
 
-    return buildProcs.length > 0 ? buildProcs[0].pid : null;
+    process.on('uncaughtException', (err) => exit(ExitCode.UNCAUGHT_EXCEPTION, err));
+    process.on('unhandledRejection', (err) => exit(ExitCode.UNHANDLED_PROMISE_REJECTION, err));
   }
 }
 

test/screenshot/infra/lib/cloud-datastore.js

@@ -21,7 +21,7 @@
  * THE SOFTWARE.
  */
 
-const Datastore = require('@google-cloud/datastore');
+const {Datastore} = require('@google-cloud/datastore');
 const {ShieldState} = require('../types/status-types');
 
 const KIND = 'ScreenshotStatus';

test/screenshot/infra/lib/constants.js

@@ -36,6 +36,11 @@ module.exports = {
    */
   GOLDEN_JSON_RELATIVE_PATH: 'test/screenshot/golden.json',
 
+  /**
+   * Path to the plain text file in which the process ID of `npm run screenshot:build` will be read/written.
+   */
+  BUILD_PID_FILE_PATH_RELATIVE: 'test/screenshot/.mdc.build.pid',
+
   /**
    * Name of the Google Cloud Storage bucket to use for public file uploads.
    * @type {string}

test/screenshot/infra/lib/local-storage.js

@@ -212,6 +212,13 @@ class LocalStorage {
     }
   }
 
+  /**
+   * @param {string|!Array<string>} pathPatterns
+   */
+  deleteSync(pathPatterns) {
+    del.sync(pathPatterns);
+  }
+
   /**
    * @param {string} filePath
    * @return {!Promise<boolean>}

test/screenshot/infra/lib/process-manager.js

@@ -25,9 +25,18 @@
 
 const childProcess = require('child_process');
 const ps = require('ps-node');
+const LocalStorage = require('../lib/local-storage');
 const {ExitCode} = require('../lib/constants');
 
 class ProcessManager {
+  constructor() {
+    /**
+     * @type {!LocalStorage}
+     * @private
+     */
+    this.localStorage_ = new LocalStorage();
+  }
+
   /**
    * @param {string} cmd
    * @param {!Array<string>} args
@@ -83,44 +92,61 @@ class ProcessManager {
   }
 
   /**
-   * @param {string} commandName
-   * @param {string} argumentPattern Regular expression
-   * @return {!Promise<!Array<!PsNodeProcess>>}
+   * @param {string} pidFilePath
+   * @param {string=} commandName
+   * @param {string=} argumentPattern Regular expression
+   * @return {!Promise<?string>}
    */
-  async getRunningProcessesInPwdAsync(commandName, argumentPattern) {
+  async getRunningPid(pidFilePath, commandName = undefined, argumentPattern = undefined) {
+    /** @type {string} */
+    const pid = await this.localStorage_.readTextFile(pidFilePath).then((content) => content.trim(), () => null);
+    if (!pid) {
+      return null;
+    }
+
     return new Promise((resolve, reject) => {
       ps.lookup(
         {
+          pid,
           command: commandName,
           arguments: argumentPattern, // Regular expression
         },
         /**
          * @param {?*} err
-         * @param {?Array<!PsNodeProcess>} unfiltered
+         * @param {?Array<!PsNodeProcess>} procs
          */
-        (err, unfiltered) => {
+        /* eslint-disable no-unused-vars */
+        (err, procs) => {
           if (err) {
-            reject(err);
-            return;
+            return reject(err);
           }
 
-          unfiltered.forEach((proc) => {
-            proc.pid = Number(proc.pid);
-            proc.ppid = Number(proc.ppid);
-          });
-
-          const filtered = unfiltered.filter((proc) => {
-            const [script] = proc['arguments'];
-            return (
-              proc.pid !== process.pid &&
-              script.startsWith(process.env.PWD)
-            );
-          });
-
-          resolve(filtered);
-        });
+          // Process is still running
+          if (procs.find((proc) => proc.pid === pid)) {
+            return resolve(pid);
+          }
+
+          return resolve(null);
+        }
+      );
     });
   }
+
+  /**
+   * @param {string} pidFilePath
+   * @param {number|string} pid
+   * @return {!Promise<void>}
+   */
+  async setRunningPid(pidFilePath, pid) {
+    await this.localStorage_.writeTextFile(pidFilePath, String(pid));
+  }
+
+  /**
+   * @param {string} pidFilePath
+   */
+  deletePidFileSync(pidFilePath) {
+    this.localStorage_.deleteSync(pidFilePath);
+  }
 }
 
 module.exports = ProcessManager;

test/screenshot/infra/lib/selenium-api.js

@@ -29,6 +29,7 @@ const Jimp = require('jimp');
 const VError = require('verror');
 const UserAgentParser = require('useragent');
 const path = require('path');
+const {JSDOM} = require('jsdom');
 
 const mdcProto = require('../proto/mdc.pb').mdc.proto;
 const seleniumProto = require('../proto/selenium.pb').selenium.proto;
@@ -612,8 +613,8 @@ class SeleniumApi {
 
     // TODO(acdvorak): Find a better way to do this
     const screenshotQueues = [
-      [true, screenshotQueueAll.filter((screenshot) => this.isSmallComponent_(screenshot.html_file_path))],
-      [false, screenshotQueueAll.filter((screenshot) => !this.isSmallComponent_(screenshot.html_file_path))],
+      [true, screenshotQueueAll.filter((screenshot) => this.isSmallComponent_(screenshot.actual_html_file))],
+      [false, screenshotQueueAll.filter((screenshot) => !this.isSmallComponent_(screenshot.actual_html_file))],
     ];
 
     for (const [isSmallComponent, screenshotQueue] of screenshotQueues) {
@@ -657,17 +658,18 @@ class SeleniumApi {
   }
 
   /**
-   * @param {string} url
+   * @param {!mdc.proto.TestFile} actualHtmlFile
    * @return {boolean}
    * @private
    */
-  isSmallComponent_(url) {
-    // TODO(acdvorak): Find a better way to do this
-    const smallComponentNames = [
-      'animation', 'button', 'card', 'checkbox', 'chips', 'elevation', 'fab', 'icon-button', 'icon-toggle',
-      'list', 'menu', 'radio', 'ripple', 'select', 'switch', 'textfield', 'theme', 'tooltip', 'typography',
-    ];
-    return new RegExp(`/mdc-(${smallComponentNames.join('|')})/`).test(url);
+  isSmallComponent_(actualHtmlFile) {
+    const htmlStr = this.localStorage_.readTextFileSync(actualHtmlFile.absolute_path);
+    const jsdom = new JSDOM(htmlStr);
+
+    /** @type {!Document} */
+    const document = jsdom.window.document;
+
+    return Boolean(document.querySelector('.test-viewport--mobile'));
   }
 
   /**

test/screenshot/infra/lib/status-notifier.js

@@ -168,7 +168,10 @@ class StatusNotifier {
 
     const numScreenshotsTotal = runnableScreenshots.length;
     const numScreenshotsFinished = runnableScreenshots.filter(isFinished).length;
-    const numChanged = runnableScreenshots.filter(hasDiffs).length;
+    const numDiffs = runnableScreenshots.filter(hasDiffs).length;
+    const numAdded = this.reportData_.screenshots.added_screenshot_list.length;
+    const numRemoved = this.reportData_.screenshots.removed_screenshot_list.length;
+    const numChanged = numDiffs + numAdded + numRemoved;
     const isTerminal = numScreenshotsFinished === numScreenshotsTotal;
     const isPassed = isTerminal && numChanged === 0;
     const isFailed = isTerminal && numChanged > 0;
@@ -267,7 +270,7 @@ class StatusNotifier {
       description = `All ${strTotal} screenshots match PR's golden.json`;
     } else if (shieldState === ShieldState.FAILED) {
       state = GitHubApi.PullRequestState.FAILURE;
-      description = `${strChanged} screenshot${changedPlural} differ from PR's golden.json`;
+      description = `${strChanged} screenshot${changedPlural} differ${changedPlural ? '' : 's'} from PR's golden.json`;
     } else {
       state = GitHubApi.PullRequestState.PENDING;
       description = `${strDone} of ${strTotal} (${strPercent}) - ${strChanged} diff${changedPlural}`;