Review comments

.travis.yml

@@ -24,6 +24,13 @@ branches:
     - canary
     - /^amp-release-.*$/
     - /^revert-.*$/
+addons:
+  hosts:
+    - ads.localhost
+    - iframe.localhost
+    # Requested by some tests because they need a valid font host,
+    # but should not resolve in tests.
+    - fonts.googleapis.com
 stages:
   - build
   - test
@@ -68,12 +75,6 @@ jobs:
         - pip install gsutil --user
       addons:
         chrome: stable
-        hosts:
-          - ads.localhost
-          - iframe.localhost
-          # Requested by some tests because they need a valid font host,
-          # but should not resolve in tests.
-          - fonts.googleapis.com
       script:
         - node build-system/pr-check/dist-test.js
     - stage: test
@@ -92,12 +93,6 @@ jobs:
         - pip install gsutil --user
       addons:
         chrome: stable
-        hosts:
-          - ads.localhost
-          - iframe.localhost
-          # Requested by some tests because they need a valid font host,
-          # but should not resolve in tests.
-          - fonts.googleapis.com
       script:
         - node build-system/pr-check/local-test.js
     - stage: test

build-system/pr-check/build-target.js

@@ -16,13 +16,9 @@
 'use strict';
 
 /**
- * @fileoverview This file is executed by Travis (configured via
- * .travis.yml in the root directory) and is the main driver script
- * for running tests.  Execution herein is entirely synchronous, that
- * is, commands are executed on after the other (see the exec
- * function). Should a command fail, this script will then also fail.
- * This script attempts to introduce some granularity for our
- * presubmit checking, via the determineBuildTargets method.
+ * @fileoverview
+ * This script sets the build targets for our PR check, where the build targets
+ * determine which tasks are required to run for pull request builds.
  */
 const config = require('../config');
 const minimatch = require('minimatch');

build-system/pr-check/build.js

@@ -23,53 +23,19 @@
 
 const colors = require('ansi-colors');
 const {
-  gitBranchName,
-  gitDiffColor,
-  gitDiffCommitLog,
-  gitDiffStatMaster,
-  gitMergeBaseMaster,
-  gitTravisMasterBaseline,
-  shortSha,
-} = require('../git');
-const {
-  isTravisBuild,
-  travisPullRequestSha,
-} = require('../travis');
-const {
+  printChangeSummary,
   startTimer,
   stopTimer,
   timedExecOrDie: timedExecOrDieBase,
   zipBuildOutput} = require('./utils');
 const {determineBuildTargets} = require('./build-target');
 const {getStderr} = require('../exec');
+const {gitDiffColor} = require('../git');
 const FILENAME = 'build.js';
 const FILELOGPREFIX = colors.bold(colors.yellow(`${FILENAME}:`));
 const timedExecOrDie =
   (cmd, unusedFileName) => timedExecOrDieBase(cmd, FILENAME);
 
-/**
- * Prints a summary of files changed by, and commits included in the PR.
- */
-function printChangeSummary_() {
-  if (isTravisBuild()) {
-    console.log(FILELOGPREFIX, colors.cyan('origin/master'),
-        'is currently at commit',
-        colors.cyan(shortSha(gitTravisMasterBaseline())));
-    console.log(FILELOGPREFIX,
-        'Testing the following changes at commit',
-        colors.cyan(shortSha(travisPullRequestSha())));
-  }
-
-  const filesChanged = gitDiffStatMaster();
-  console.log(filesChanged);
-
-  const branchPoint = gitMergeBaseMaster();
-  console.log(FILELOGPREFIX, 'Commit log since branch',
-      colors.cyan(gitBranchName()), 'was forked from',
-      colors.cyan('master'), 'at', colors.cyan(shortSha(branchPoint)) + ':');
-  console.log(gitDiffCommitLog() + '\n');
-}
-
 /**
  * Makes sure package.json and yarn.lock are in sync.
  * @private
@@ -119,7 +85,7 @@ function main() {
   // Make sure package.json and yarn.lock are in sync and up-to-date.
   runYarnIntegrityCheck_();
   runYarnLockfileCheck_();
-  printChangeSummary_();
+  printChangeSummary(FILENAME);
   const buildTargets = determineBuildTargets();
 
   if (buildTargets.has('RUNTIME') ||
@@ -128,7 +94,6 @@ function main() {
       buildTargets.has('BUILD_SYSTEM')) {
 
     timedExecOrDie('gulp update-packages');
-    timedExecOrDie('gulp css');
     timedExecOrDie('gulp build --fortesting');
 
     zipBuildOutput();

build-system/pr-check/checks.js

@@ -23,11 +23,12 @@
  */
 
 const {
+  printChangeSummary,
   startTimer,
   stopTimer,
   timedExecOrDie: timedExecOrDieBase} = require('./utils');
 const {determineBuildTargets} = require('./build-target');
-const {isTravisPushBuild} = require('../travis');
+const {isTravisPullRequestBuild} = require('../travis');
 
 const FILENAME = 'checks.js';
 const timedExecOrDie =
@@ -36,20 +37,20 @@ const timedExecOrDie =
 function main() {
   const startTime = startTimer(FILENAME);
   const buildTargets = determineBuildTargets();
+  printChangeSummary(FILENAME);
 
   timedExecOrDie('gulp update-packages');
   timedExecOrDie('gulp presubmit');
   timedExecOrDie('gulp lint');
 
-  if (isTravisPushBuild()) {
+  if (!isTravisPullRequestBuild()) {
     timedExecOrDie('gulp ava');
     timedExecOrDie('node node_modules/jest/bin/jest.js');
     timedExecOrDie('gulp check-types');
     timedExecOrDie('gulp caches-json');
     timedExecOrDie('gulp json-syntax');
     timedExecOrDie('gulp dep-check');
-  }
-  else {
+  } else {
     if (buildTargets.has('RUNTIME') ||
         buildTargets.has('BUILD_SYSTEM')) {
 

build-system/pr-check/dist-test.js

@@ -22,12 +22,13 @@
  */
 
 const {
+  printChangeSummary,
   startTimer,
   stopTimer,
   timedExecOrDie: timedExecOrDieBase,
   unzipBuildOutput} = require('./utils');
 const {determineBuildTargets} = require('./build-target');
-const {isTravisPushBuild} = require('../travis');
+const {isTravisPullRequestBuild} = require('../travis');
 
 const FILENAME = 'dist-test.js';
 const timedExecOrDie =
@@ -44,13 +45,13 @@ function runSinglePassTest_() {
 function main() {
   const startTime = startTimer(FILENAME);
   const buildTargets = determineBuildTargets();
+  printChangeSummary(FILENAME);
 
-  if (isTravisPushBuild()) {
+  if (!isTravisPullRequestBuild()) {
     timedExecOrDie('gulp dist --fortesting --noextensions');
     timedExecOrDie('gulp bundle-size --on-push-build');
     runSinglePassTest_();
-  }
-  else {
+  } else {
     let ranTests = false;
 
     if (buildTargets.has('RUNTIME')) {

build-system/pr-check/local-test.js

@@ -22,12 +22,13 @@
  */
 
 const {
+  printChangeSummary,
   startTimer,
   stopTimer,
   timedExecOrDie: timedExecOrDieBase,
   unzipBuildOutput} = require('./utils');
 const {determineBuildTargets} = require('./build-target');
-const {isTravisPushBuild} = require('../travis');
+const {isTravisPullRequestBuild} = require('../travis');
 
 const FILENAME = 'local-test.js';
 const timedExecOrDie =
@@ -36,15 +37,16 @@ const timedExecOrDie =
 function main() {
   const startTime = startTimer(FILENAME);
   const buildTargets = determineBuildTargets();
+  printChangeSummary(FILENAME);
+
   unzipBuildOutput();
-  if (isTravisPushBuild()) {
+  if (!isTravisPullRequestBuild()) {
     timedExecOrDie('gulp test --integration --nobuild --coverage');
     timedExecOrDie('gulp test --unit --nobuild --headless --coverage');
     timedExecOrDie('gulp test --dev_dashboard --nobuild');
     //TODO(estherkim): turn on when stabilized :)
     //timedExecOrDie('gulp e2e --nobuild');
-  }
-  else {
+  } else {
     let ranTests = false;
 
     if (buildTargets.has('RUNTIME') ||

build-system/pr-check/remote-test.js

@@ -22,14 +22,15 @@
  */
 
 const {
+  printChangeSummary,
   startTimer,
   stopTimer,
   startSauceConnect,
   stopSauceConnect,
   timedExecOrDie: timedExecOrDieBase,
   unzipBuildOutput} = require('./utils');
 const {determineBuildTargets} = require('./build-target');
-const {isTravisPushBuild} = require('../travis');
+const {isTravisPullRequestBuild} = require('../travis');
 
 const FILENAME = 'remote-test.js';
 const timedExecOrDie =
@@ -38,14 +39,15 @@ const timedExecOrDie =
 function main() {
   const startTime = startTimer(FILENAME);
   const buildTargets = determineBuildTargets();
+  printChangeSummary(FILENAME);
+
   unzipBuildOutput();
   startSauceConnect(FILENAME);
 
-  if (isTravisPushBuild()) {
+  if (!isTravisPullRequestBuild()) {
     timedExecOrDie('gulp test --unit --nobuild --saucelabs_lite');
     timedExecOrDie('gulp test --integration --nobuild --compiled --saucelabs');
-  }
-  else {
+  } else {
     let ranTests = false;
 
     if (buildTargets.has('RUNTIME') ||

build-system/pr-check/utils.js

@@ -16,12 +16,44 @@
 'use strict';
 
 const colors = require('ansi-colors');
+const {
+  gitBranchName,
+  gitDiffCommitLog,
+  gitDiffStatMaster,
+  gitMergeBaseMaster,
+  gitTravisMasterBaseline,
+  shortSha,
+} = require('../git');
 const {execOrDie, exec, getStdout} = require('../exec');
-const {travisBuildNumber} = require('../travis');
+const {travisBuildNumber, travisPullRequestSha} = require('../travis');
+
 const BUILD_OUTPUT_FILE = `amp_build_${travisBuildNumber()}.zip`;
 const BUILD_OUTPUT_DIRS = 'build/ dist/ dist.3p/ EXTENSIONS_CSS_MAP';
 const BUILD_OUTPUT_STORAGE_LOCATION = 'gs://amp-travis-builds';
 
+/**
+ * Prints a summary of files changed by, and commits included in the PR.
+ * @param {string} fileName
+ */
+function printChangeSummary(fileName) {
+  const fileLogPrefix = colors.bold(colors.yellow(`${fileName}:`));
+  console.log(fileLogPrefix, colors.cyan('origin/master'),
+      'is currently at commit',
+      colors.cyan(shortSha(gitTravisMasterBaseline())));
+  console.log(fileLogPrefix,
+      'Testing the following changes at commit',
+      colors.cyan(shortSha(travisPullRequestSha())));
+
+  const filesChanged = gitDiffStatMaster();
+  console.log(filesChanged);
+
+  const branchPoint = gitMergeBaseMaster();
+  console.log(fileLogPrefix, 'Commit log since branch',
+      colors.cyan(gitBranchName()), 'was forked from',
+      colors.cyan('master'), 'at', colors.cyan(shortSha(branchPoint)) + ':');
+  console.log(gitDiffCommitLog() + '\n');
+}
+
 /**
  * Starts connection to Sauce Labs after getting account credentials
  * @param {string} functionName
@@ -130,6 +162,7 @@ function zipBuildOutput() {
 
 
 module.exports = {
+  printChangeSummary,
   startTimer,
   stopTimer,
   startSauceConnect,

build-system/pr-check/validator.js

@@ -22,11 +22,12 @@
  */
 
 const {
+  printChangeSummary,
   startTimer,
   stopTimer,
   timedExecOrDie: timedExecOrDieBase} = require('./utils');
 const {determineBuildTargets} = require('./build-target');
-const {isTravisPushBuild} = require('../travis');
+const {isTravisPullRequestBuild} = require('../travis');
 
 const FILENAME = 'validator.js';
 const timedExecOrDie =
@@ -35,18 +36,16 @@ const timedExecOrDie =
 function main() {
   const startTime = startTimer(FILENAME);
   const buildTargets = determineBuildTargets();
+  printChangeSummary(FILENAME);
 
-  if (isTravisPushBuild()) {
+  if (!isTravisPullRequestBuild()) {
     timedExecOrDie('gulp validator');
     timedExecOrDie('gulp validator-webui');
-  }
-  else if (buildTargets.has('VALIDATOR')) {
+  } else if (buildTargets.has('VALIDATOR')) {
     timedExecOrDie('gulp validator');
-  }
-  else if (buildTargets.has('VALIDATOR_WEBUI')) {
+  } else if (buildTargets.has('VALIDATOR_WEBUI')) {
     timedExecOrDie('gulp validator-webui');
-  }
-  else {
+  } else {
     console.log('Skipping validator job because this commit does ' +
       'not affect the validator or validator web UI.');
   }