Merge branch 'master' into fix/select/add-adapter

karma.conf.js

@@ -135,7 +135,7 @@ const SAUCE_LAUNCHERS = {
 };
 
 const getLaunchers = () => USING_SL ? SAUCE_LAUNCHERS : LOCAL_LAUNCHERS;
-const getBrowsers = () => Object.keys(getLaunchers());
+const getBrowsers = () => USING_TRAVISCI ? Object.keys(getLaunchers()) : ['Chrome'];
 
 module.exports = function(config) {
   config.set({

test/screenshot/infra/commands/test.js

@@ -257,8 +257,21 @@ class TestCommand {
    * @private
    */
   getPrComment_({masterDiffReportData, snapshotGitRev}) {
-    const reportPageUrl = masterDiffReportData.meta.report_html_file.public_url;
+    const masterReportPageUrl = masterDiffReportData.meta.report_html_file.public_url;
     const masterScreenshots = masterDiffReportData.screenshots;
+    const masterGitRev = masterDiffReportData.meta.golden_diff_base.git_revision;
+
+    const numTotal = masterScreenshots.actual_screenshot_list.length;
+    const numChanged =
+      masterScreenshots.changed_screenshot_list.length +
+      masterScreenshots.added_screenshot_list.length +
+      masterScreenshots.removed_screenshot_list.length;
+    const plural = numChanged === 1 ? '' : 's';
+
+    if (numChanged === 0) {
+      const range = `commit ${snapshotGitRev.commit} vs. \`${masterGitRev.branch}\``;
+      return `**All ${numTotal} screenshot tests passed** for ${range}! 💯🎉`;
+    }
 
     const listMarkdown = [
       this.getChangelistMarkdown_(
@@ -272,38 +285,22 @@ class TestCommand {
       ),
     ].filter((str) => Boolean(str)).join('\n\n');
 
-    let contentMarkdown;
 
-    const numChanged =
-      masterScreenshots.changed_screenshot_list.length +
-      masterScreenshots.added_screenshot_list.length +
-      masterScreenshots.removed_screenshot_list.length;
+    return `
+### Screenshot test report ⚠️
+
+**${numChanged}** screenshot${plural} changed from \`${masterGitRev.branch}\` on commit ${snapshotGitRev.commit}:
+
+* ${masterReportPageUrl}
 
-    if (listMarkdown) {
-      contentMarkdown = `
 <details>
-  <summary><b>${numChanged} screenshot${numChanged === 1 ? '' : 's'} changed ⚠️</b></summary>
+  <summary><b>Details</b></summary>
   <div>
 
 ${listMarkdown}
 
   </div>
 </details>
-`.trim();
-    } else {
-      contentMarkdown = '### No diffs! 💯🎉';
-    }
-
-    return `
-🤖 Beep boop!
-
-### Screenshot test report
-
-Commit ${snapshotGitRev.commit} vs. \`master\`:
-
-* ${reportPageUrl}
-
-${contentMarkdown}
 `.trim();
   }
 

test/screenshot/infra/commands/travis.sh

@@ -18,6 +18,20 @@ function print_travis_env_vars() {
   echo
 }
 
+function maybe_add_git_branch() {
+  if [[ -n "$1" ]]; then
+    # https://github.com/marionebl/commitlint/issues/6#issuecomment-231186598
+    git remote set-branches --add origin "$1"
+  fi
+}
+
+function maybe_fetch_git_branches() {
+  maybe_add_git_branch 'master'
+  maybe_add_git_branch "$TRAVIS_BRANCH"
+  maybe_add_git_branch "$TRAVIS_PULL_REQUEST_BRANCH"
+  git fetch --tags
+}
+
 function maybe_extract_api_credentials() {
   if [[ -z "$encrypted_eead2343bb54_key" ]] || [[ -z "$encrypted_eead2343bb54_iv" ]]; then
     log_error
@@ -83,6 +97,7 @@ function maybe_install_gcloud_sdk() {
 
 if [[ "$TEST_SUITE" == 'screenshot' ]]; then
   print_travis_env_vars
+  maybe_fetch_git_branches
   maybe_extract_api_credentials
   maybe_install_gcloud_sdk
 fi

test/screenshot/infra/lib/diff-base-parser.js

@@ -25,6 +25,7 @@ const {GOLDEN_JSON_RELATIVE_PATH} = require('./constants');
 const Cli = require('./cli');
 const GitHubApi = require('./github-api');
 const GitRepo = require('./git-repo');
+const getStackTrace = require('./stacktrace')('DiffBaseParser');
 
 const HTTP_URL_REGEX = new RegExp('^https?://');
 
@@ -113,14 +114,14 @@ class DiffBaseParser {
     // E.g.: `--diff-base=https://storage.googleapis.com/.../golden.json`
     const isUrl = HTTP_URL_REGEX.test(rawDiffBase);
     if (isUrl) {
-      return this.createPublicUrlDiffBase_(rawDiffBase);
+      return await this.createPublicUrlDiffBase_(rawDiffBase);
     }
 
     // Diff against a local `golden.json` file.
     // E.g.: `--diff-base=/tmp/golden.json`
     const isLocalFile = await fs.exists(rawDiffBase);
     if (isLocalFile) {
-      return this.createLocalFileDiffBase_(rawDiffBase);
+      return await this.createLocalFileDiffBase_(rawDiffBase);
     }
 
     const [inputGoldenRef, inputGoldenPath] = rawDiffBase.split(':');
@@ -140,26 +141,26 @@ class DiffBaseParser {
     // Diff against a specific git commit.
     // E.g.: `--diff-base=abcd1234`
     if (!fullGoldenRef) {
-      return this.createCommitDiffBase_(inputGoldenRef, goldenFilePath);
+      return await this.createCommitDiffBase_(inputGoldenRef, goldenFilePath);
     }
 
     const {remoteRef, localRef, tagRef} = this.getRefType_(fullGoldenRef);
 
     // Diff against a remote git branch.
     // E.g.: `--diff-base=origin/master` or `--diff-base=origin/feat/button/my-fancy-feature`
     if (remoteRef) {
-      return this.createRemoteBranchDiffBase_(remoteRef, goldenFilePath);
+      return await this.createRemoteBranchDiffBase_(remoteRef, goldenFilePath);
     }
 
     // Diff against a remote git tag.
     // E.g.: `--diff-base=v0.34.1`
     if (tagRef) {
-      return this.createRemoteTagDiffBase_(tagRef, goldenFilePath);
+      return await this.createRemoteTagDiffBase_(tagRef, goldenFilePath);
     }
 
     // Diff against a local git branch.
     // E.g.: `--diff-base=master` or `--diff-base=HEAD`
-    return this.createLocalBranchDiffBase_(localRef, goldenFilePath);
+    return await this.createLocalBranchDiffBase_(localRef, goldenFilePath);
   }
 
   /**
@@ -200,10 +201,17 @@ class DiffBaseParser {
     const travisPrNumber = Number(process.env.TRAVIS_PULL_REQUEST);
     const travisPrBranch = process.env.TRAVIS_PULL_REQUEST_BRANCH;
     const travisPrSha = process.env.TRAVIS_PULL_REQUEST_SHA;
+    const travisCommit = process.env.TRAVIS_COMMIT;
+    const commit = travisPrSha || travisCommit;
+
+    if (!commit) {
+      return null;
+    }
+
+    const logInfo = {travisBranch, travisTag, travisPrNumber, travisPrBranch, travisPrSha, travisCommit};
+    const author = await this.gitRepo_.getCommitAuthor(commit, getStackTrace('getTravisGitRevision', logInfo));
 
     if (travisPrNumber) {
-      const commit = await this.gitRepo_.getFullCommitHash(travisPrSha);
-      const author = await this.gitRepo_.getCommitAuthor(commit);
       return GitRevision.create({
         type: GitRevision.Type.TRAVIS_PR,
         golden_json_file_path: GOLDEN_JSON_RELATIVE_PATH,
@@ -216,8 +224,6 @@ class DiffBaseParser {
     }
 
     if (travisTag) {
-      const commit = await this.gitRepo_.getFullCommitHash(travisTag);
-      const author = await this.gitRepo_.getCommitAuthor(commit);
       return GitRevision.create({
         type: GitRevision.Type.REMOTE_TAG,
         golden_json_file_path: GOLDEN_JSON_RELATIVE_PATH,
@@ -228,8 +234,6 @@ class DiffBaseParser {
     }
 
     if (travisBranch) {
-      const commit = await this.gitRepo_.getFullCommitHash(travisBranch);
-      const author = await this.gitRepo_.getCommitAuthor(commit);
       return GitRevision.create({
         type: GitRevision.Type.LOCAL_BRANCH,
         golden_json_file_path: GOLDEN_JSON_RELATIVE_PATH,
@@ -298,7 +302,7 @@ class DiffBaseParser {
    * @private
    */
   async createCommitDiffBase_(commit, goldenJsonFilePath) {
-    const author = await this.gitRepo_.getCommitAuthor(commit);
+    const author = await this.gitRepo_.getCommitAuthor(commit, getStackTrace('createCommitDiffBase_'));
 
     return DiffBase.create({
       type: DiffBase.Type.GIT_REVISION,
@@ -323,7 +327,7 @@ class DiffBaseParser {
     const remote = allRemoteNames.find((curRemoteName) => remoteRef.startsWith(curRemoteName + '/'));
     const branch = remoteRef.substr(remote.length + 1); // add 1 for forward-slash separator
     const commit = await this.gitRepo_.getFullCommitHash(remoteRef);
-    const author = await this.gitRepo_.getCommitAuthor(commit);
+    const author = await this.gitRepo_.getCommitAuthor(commit, getStackTrace('createRemoteBranchDiffBase_'));
 
     return DiffBase.create({
       type: DiffBase.Type.GIT_REVISION,
@@ -347,7 +351,7 @@ class DiffBaseParser {
    */
   async createRemoteTagDiffBase_(tagRef, goldenJsonFilePath) {
     const commit = await this.gitRepo_.getFullCommitHash(tagRef);
-    const author = await this.gitRepo_.getCommitAuthor(commit);
+    const author = await this.gitRepo_.getCommitAuthor(commit, getStackTrace('createRemoteTagDiffBase_'));
 
     return DiffBase.create({
       type: DiffBase.Type.GIT_REVISION,
@@ -371,7 +375,7 @@ class DiffBaseParser {
    */
   async createLocalBranchDiffBase_(branch, goldenJsonFilePath) {
     const commit = await this.gitRepo_.getFullCommitHash(branch);
-    const author = await this.gitRepo_.getCommitAuthor(commit);
+    const author = await this.gitRepo_.getCommitAuthor(commit, getStackTrace('createLocalBranchDiffBase_'));
 
     return DiffBase.create({
       type: DiffBase.Type.GIT_REVISION,

test/screenshot/infra/lib/git-repo.js

@@ -79,15 +79,23 @@ class GitRepo {
    * @return {!Promise<string>}
    */
   async getFullCommitHash(ref = 'HEAD') {
-    return this.exec_('revparse', [ref]);
+    const hash = this.exec_('revparse', [ref]);
+    if (!hash) {
+      throw new Error(`Unable to get commit hash for git ref "${ref}"`);
+    }
+    return hash;
   }
 
   /**
    * @param {string=} ref
    * @return {!Promise<string>}
    */
   async getBranchName(ref = 'HEAD') {
-    return this.exec_('revparse', ['--abbrev-ref', ref]);
+    const branch = this.exec_('revparse', ['--abbrev-ref', ref]);
+    if (!branch) {
+      throw new Error(`Unable to get branch name for git ref "${ref}"`);
+    }
+    return branch;
   }
 
   /**
@@ -102,7 +110,11 @@ class GitRepo {
    * @return {!Promise<string>}
    */
   async getFullSymbolicName(ref = 'HEAD') {
-    return this.exec_('revparse', ['--symbolic-full-name', ref]);
+    const fullName = this.exec_('revparse', ['--symbolic-full-name', ref]);
+    if (!fullName) {
+      throw new Error(`Unable to get full symbolic name for git ref "${ref}"`);
+    }
+    return fullName;
   }
 
   /**
@@ -164,18 +176,30 @@ class GitRepo {
     try {
       return this.repo_.checkIgnore(filePaths);
     } catch (err) {
-      throw new VError(err, `Failed to run GitRepo.getIgnoredPaths(${filePaths.length} file paths)`);
+      throw new VError(err, `Unable to check gitignore status of ${filePaths.length} file paths`);
     }
   }
 
   /**
-   * @param {string=} commit
+   * @param {string} commit
+   * @param {string} stackTrace
    * @return {!Promise<!mdc.proto.User>}
    */
-  async getCommitAuthor(commit = undefined) {
+  async getCommitAuthor(commit, stackTrace) {
     /** @type {!Array<!DefaultLogFields>} */
-    const logEntries = await this.getLog([commit]);
+    let logEntries;
+
+    try {
+      logEntries = await this.getLog([commit]);
+    } catch (err) {
+      throw new VError(err, `Unable to get author for commit "${commit}":\n${stackTrace}`);
+    }
+
     const logEntry = logEntries[0];
+    if (!logEntry) {
+      throw new VError(err, `Unable to get author for commit "${commit}":\n${stackTrace}`);
+    }
+
     return User.create({
       name: logEntry.author_name,
       email: logEntry.author_email,

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

@@ -18,19 +18,23 @@ const VError = require('verror');
 const debounce = require('debounce');
 const octokit = require('@octokit/rest');
 
+/** @type {!CliColor} */
+const colors = require('colors');
+
 const GitRepo = require('./git-repo');
 const getStackTrace = require('./stacktrace')('GitHubApi');
 
 class GitHubApi {
   constructor() {
     this.gitRepo_ = new GitRepo();
     this.octokit_ = octokit();
+    this.isAuthenticated_ = false;
+    this.hasWarnedNoAuth_ = false;
     this.authenticate_();
+    this.initStatusThrottle_();
   }
 
-  /**
-   * @private
-   */
+  /** @private */
   authenticate_() {
     let token;
 
@@ -46,6 +50,11 @@ class GitHubApi {
       token: token,
     });
 
+    this.isAuthenticated_ = true;
+  }
+
+  /** @private */
+  initStatusThrottle_() {
     const throttle = (fn, delay) => {
       let lastCall = 0;
       return (...args) => {
@@ -163,7 +172,19 @@ class GitHubApi {
    * @private
    */
   async createStatusUnthrottled_({state, targetUrl, description = undefined}) {
-    const sha = process.env.TRAVIS_PULL_REQUEST_SHA || await this.gitRepo_.getFullCommitHash();
+    if (!this.isAuthenticated_) {
+      if (!this.hasWarnedNoAuth_) {
+        const warning = colors.magenta('WARNING');
+        console.warn(`${warning}: Cannot set GitHub commit status because no API credentials were found.`);
+        this.hasWarnedNoAuth_ = true;
+      }
+      return null;
+    }
+
+    const travisPrSha = process.env.TRAVIS_PULL_REQUEST_SHA;
+    const travisCommit = process.env.TRAVIS_COMMIT;
+    const sha = travisPrSha || travisCommit || await this.gitRepo_.getFullCommitHash();
+
     let stackTrace;
 
     try {

test/screenshot/infra/lib/stacktrace.js

@@ -24,12 +24,14 @@ const colors = require('colors');
 module.exports = function(className) {
   /**
    * @param {string} methodName
+   * @param {*=} infoData
    * @return {string}
    */
-  function getStackTrace(methodName) {
+  function getStackTrace(methodName, infoData = undefined) {
+    const infoStr = typeof infoData === 'object' ? '\n' + JSON.stringify(infoData, null, 2) : '';
     const fullStack = new Error(`${className}.${methodName}()`).stack;
     // Remove THIS function from the stack trace because it's not useful
-    return fullStack.split('\n').filter((line, index) => index !== 1).join('\n');
+    return fullStack.split('\n').filter((line, index) => index !== 1).join('\n') + infoStr;
   }
 
   return getStackTrace;