From e513ddbf4b2385fb6fda780f2bf34bec31e022a4 Mon Sep 17 00:00:00 2001 From: Will Ernest <34519388+williamernest@users.noreply.github.com> Date: Wed, 25 Jul 2018 16:30:30 -0700 Subject: [PATCH 1/4] chore(infrastructure): Fix local tests (#3223) --- karma.conf.js | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/karma.conf.js b/karma.conf.js index 4fddec16d85..4a8cfc70d37 100644 --- a/karma.conf.js +++ b/karma.conf.js @@ -134,15 +134,8 @@ const SAUCE_LAUNCHERS = { // }, }; -const getLaunchers = () => { - if (USING_TRAVISCI) { - return USING_SL ? SAUCE_LAUNCHERS : LOCAL_LAUNCHERS; - } else { - return ['Chrome']; - } -}; - -const getBrowsers = () => Object.keys(getLaunchers()); +const getLaunchers = () => USING_SL ? SAUCE_LAUNCHERS : LOCAL_LAUNCHERS; +const getBrowsers = () => USING_TRAVISCI ? Object.keys(getLaunchers()) : ['Chrome']; module.exports = function(config) { config.set({ From e6ec57cb66066cde059a40da1f4b96001e16eb71 Mon Sep 17 00:00:00 2001 From: "Andrew C. Dvorak" Date: Wed, 25 Jul 2018 17:35:40 -0700 Subject: [PATCH 2/4] chore(infrastructure): Fix Travis tests on feature branch PRs (#3226) - Fetches `$TRAVIS_BRANCH` and `$TRAVIS_PULL_REQUEST_BRANCH` git branches to ensure they are available for subsequent `git log` queries --- test/screenshot/infra/commands/travis.sh | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/screenshot/infra/commands/travis.sh b/test/screenshot/infra/commands/travis.sh index a91db094439..079a02e06c3 100755 --- a/test/screenshot/infra/commands/travis.sh +++ b/test/screenshot/infra/commands/travis.sh @@ -18,6 +18,19 @@ function print_travis_env_vars() { echo } +function maybe_add_git_branch() { + if [[ -n "$1" ]]; then + 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 +96,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 From ce4d9d0b39cef7606986e633f7f9e0898a1b5312 Mon Sep 17 00:00:00 2001 From: "Andrew C. Dvorak" Date: Wed, 25 Jul 2018 21:37:27 -0700 Subject: [PATCH 3/4] chore(infrastructure): Throw more descriptive errors in GitRepo class (#3228) Also: - Fixes typo in `getStackTrace()` --- test/screenshot/infra/commands/travis.sh | 1 + test/screenshot/infra/lib/git-repo.js | 18 +++++++++++++++--- test/screenshot/infra/lib/stacktrace.js | 6 +++--- 3 files changed, 19 insertions(+), 6 deletions(-) diff --git a/test/screenshot/infra/commands/travis.sh b/test/screenshot/infra/commands/travis.sh index 079a02e06c3..df43215681a 100755 --- a/test/screenshot/infra/commands/travis.sh +++ b/test/screenshot/infra/commands/travis.sh @@ -20,6 +20,7 @@ function print_travis_env_vars() { 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 } diff --git a/test/screenshot/infra/lib/git-repo.js b/test/screenshot/infra/lib/git-repo.js index 07c9e3eb476..4d77bd31e07 100644 --- a/test/screenshot/infra/lib/git-repo.js +++ b/test/screenshot/infra/lib/git-repo.js @@ -79,7 +79,11 @@ class GitRepo { * @return {!Promise} */ 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; } /** @@ -87,7 +91,11 @@ class GitRepo { * @return {!Promise} */ 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} */ 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; } /** diff --git a/test/screenshot/infra/lib/stacktrace.js b/test/screenshot/infra/lib/stacktrace.js index 3c7f4b026d5..01d5b454220 100644 --- a/test/screenshot/infra/lib/stacktrace.js +++ b/test/screenshot/infra/lib/stacktrace.js @@ -19,15 +19,15 @@ const colors = require('colors'); /** * @param {string} className - * @param {*=} infoData * @return {function(methodName: string): string} */ -module.exports = function(className, infoData = undefined) { +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 From ffb8bb70d34ed35d2223e332a720a8237dc5fbc5 Mon Sep 17 00:00:00 2001 From: "Andrew C. Dvorak" Date: Thu, 26 Jul 2018 08:33:02 -0700 Subject: [PATCH 4/4] chore(infrastructure): Try to fix Travis for merges into master (#3232) ### What it does - (Hopefully) Fixes failing screenshot tests on Travis for merges into `master` - Uses the `TRAVIS_COMMIT` env var instead of querying `git` - Looking up the commit with `git rev-parse master` seems to fail on Travis for some reason - Stops trying to set GitHub commit status if API credentials are not found - Previously, this would throw an error and kill the whole test - Adds a bit of logging --- test/screenshot/infra/lib/diff-base-parser.js | 27 ++++++++--------- test/screenshot/infra/lib/git-repo.js | 4 +++ test/screenshot/infra/lib/github-api.js | 29 ++++++++++++++++--- 3 files changed, 43 insertions(+), 17 deletions(-) diff --git a/test/screenshot/infra/lib/diff-base-parser.js b/test/screenshot/infra/lib/diff-base-parser.js index 9aaae052728..bbc1f1d99db 100644 --- a/test/screenshot/infra/lib/diff-base-parser.js +++ b/test/screenshot/infra/lib/diff-base-parser.js @@ -114,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(':'); @@ -141,7 +141,7 @@ 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); @@ -149,18 +149,18 @@ class DiffBaseParser { // 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); } /** @@ -201,12 +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; - const logInfo = {travisBranch, travisTag, travisPrNumber, travisPrBranch, travisPrSha}; + 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, getStackTrace('getTravisGitRevision', logInfo)); return GitRevision.create({ type: GitRevision.Type.TRAVIS_PR, golden_json_file_path: GOLDEN_JSON_RELATIVE_PATH, @@ -219,8 +224,6 @@ class DiffBaseParser { } if (travisTag) { - const commit = await this.gitRepo_.getFullCommitHash(travisTag); - const author = await this.gitRepo_.getCommitAuthor(commit, getStackTrace('getTravisGitRevision', logInfo)); return GitRevision.create({ type: GitRevision.Type.REMOTE_TAG, golden_json_file_path: GOLDEN_JSON_RELATIVE_PATH, @@ -231,8 +234,6 @@ class DiffBaseParser { } if (travisBranch) { - const commit = await this.gitRepo_.getFullCommitHash(travisBranch); - const author = await this.gitRepo_.getCommitAuthor(commit, getStackTrace('getTravisGitRevision', logInfo)); return GitRevision.create({ type: GitRevision.Type.LOCAL_BRANCH, golden_json_file_path: GOLDEN_JSON_RELATIVE_PATH, diff --git a/test/screenshot/infra/lib/git-repo.js b/test/screenshot/infra/lib/git-repo.js index 4d77bd31e07..593e3c837c2 100644 --- a/test/screenshot/infra/lib/git-repo.js +++ b/test/screenshot/infra/lib/git-repo.js @@ -196,6 +196,10 @@ class GitRepo { } 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, diff --git a/test/screenshot/infra/lib/github-api.js b/test/screenshot/infra/lib/github-api.js index 0a9d490a61c..fba2ae9f6f1 100644 --- a/test/screenshot/infra/lib/github-api.js +++ b/test/screenshot/infra/lib/github-api.js @@ -18,6 +18,9 @@ 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'); @@ -25,12 +28,13 @@ 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 {