Review comments

estherkim · Feb 14, 2019 · 7db4b28 · 7db4b28
1 parent ced9c5d
commit 7db4b28
Show file tree

Hide file tree

Showing 6 changed files with 49 additions and 28 deletions.
diff --git a/build-system/pr-check/build.js b/build-system/pr-check/build.js
@@ -96,7 +96,7 @@ function main() {
     timedExecOrDie('gulp update-packages');
     timedExecOrDie('gulp build --fortesting');
 
-    uploadBuildOutput();
+    uploadBuildOutput(FILENAME);
   } else {
     console.log('Skipping build job because this commit does ' +
      'not affect the runtime, build system, or test files');

diff --git a/build-system/pr-check/local-tests.js b/build-system/pr-check/local-tests.js
@@ -39,7 +39,7 @@ function main() {
   const buildTargets = determineBuildTargets();
   printChangeSummary(FILENAME);
 
-  downloadBuildOutput();
+  downloadBuildOutput(FILENAME);
   if (!isTravisPullRequestBuild()) {
     timedExecOrDie('gulp test --integration --nobuild --coverage');
     timedExecOrDie('gulp test --unit --nobuild --headless --coverage');

diff --git a/build-system/pr-check/remote-tests.js b/build-system/pr-check/remote-tests.js
@@ -41,7 +41,7 @@ function main() {
   const buildTargets = determineBuildTargets();
   printChangeSummary(FILENAME);
 
-  downloadBuildOutput();
+  downloadBuildOutput(FILENAME);
   startSauceConnect(FILENAME);
 
   if (!isTravisPullRequestBuild()) {

diff --git a/build-system/pr-check/utils.js b/build-system/pr-check/utils.js
@@ -136,26 +136,47 @@ function timedExecOrDie(cmd, fileName = 'utils.js') {
   stopTimer(cmd, startTime);
 }
 
-
-function downloadBuildOutput() {
-  execOrDie('gsutil cp ' +
-      `${BUILD_OUTPUT_STORAGE_LOCATION}/${BUILD_OUTPUT_FILE} ` +
-      `${BUILD_OUTPUT_FILE}`);
-  execOrDie(
-      `log="$(unzip -o ${BUILD_OUTPUT_FILE})" && ` +
-      'echo travis_fold:start:unzip_results && echo ${log} && ' +
-      'echo travis_fold:end:unzip_results');
-  execOrDie(
-      `log="$(ls -la ${BUILD_OUTPUT_DIRS})" && ` +
-      'echo travis_fold:start:verify_unzip_results && echo ${log} && ' +
-      'echo travis_fold:end:verify_unzip_results');
+/**
+ * Downloads build output from storage
+ * @param {string} functionName
+ */
+function downloadBuildOutput(functionName) {
+  const fileLogPrefix = colors.bold(colors.yellow(`${functionName}:`));
+  const buildOutputDownloadUrl =
+    `${BUILD_OUTPUT_STORAGE_LOCATION}/${BUILD_OUTPUT_FILE}`;
+  console.log(fileLogPrefix,
+      'Downloading build output from',
+      colors.cyan(buildOutputDownloadUrl) + '...');
+  execOrDie(`gsutil cp ${buildOutputDownloadUrl} ${BUILD_OUTPUT_FILE}`);
+  console.log(fileLogPrefix,
+      'Extracting',
+      colors.cyan(BUILD_OUTPUT_FILE) + '...');
+  exec('echo travis_fold:start:unzip_results');
+  execOrDie(`unzip -o ${BUILD_OUTPUT_FILE}`);
+  exec('echo travis_fold:end:unzip_results');
+  console.log(fileLogPrefix, 'Verifying extracted files...');
+  exec('echo travis_fold:start:verify_unzip_results');
+  execOrDie(`ls -la ${BUILD_OUTPUT_DIRS}`);
+  exec('echo travis_fold:end:verify_unzip_results');
 }
 
-function uploadBuildOutput() {
-  execOrDie(
-      `log="$(zip -r ${BUILD_OUTPUT_FILE} ${BUILD_OUTPUT_DIRS})" && ` +
-      'echo travis_fold:start:zip_results && echo ${log} && ' +
-      'echo travis_fold:end:zip_results');
+/**
+ * Zips and uploads the build output to a remote storage location
+ * @param {string} functionName
+ */
+function uploadBuildOutput(functionName) {
+  const fileLogPrefix = colors.bold(colors.yellow(`${functionName}:`));
+  console.log(fileLogPrefix,
+      'Compressing contents of directories',
+      colors.cyan(BUILD_OUTPUT_DIRS),
+      ' into', colors.cyan(BUILD_OUTPUT_FILE) + '...');
+  exec('echo travis_fold:start:zip_results');
+  execOrDie(`zip -r ${BUILD_OUTPUT_FILE} ${BUILD_OUTPUT_DIRS}`);
+  exec('echo travis_fold:end:zip_results');
+  console.log(fileLogPrefix,
+      'Uploading',
+      colors.cyan(BUILD_OUTPUT_FILE),
+      'to', colors.cyan(BUILD_OUTPUT_STORAGE_LOCATION) + '...');
   execOrDie(`gsutil -m cp -r ${BUILD_OUTPUT_FILE} `
     + `${BUILD_OUTPUT_STORAGE_LOCATION}`);
 }

diff --git a/build-system/pr-check/visual-diff-tests.js b/build-system/pr-check/visual-diff-tests.js
@@ -41,7 +41,7 @@ function main() {
   printChangeSummary(FILENAME);
 
   if (!isTravisPullRequestBuild()) {
-    downloadBuildOutput();
+    downloadBuildOutput(FILENAME);
     process.env['PERCY_TOKEN'] = atob(process.env.PERCY_TOKEN_ENCODED);
     timedExecOrDie('gulp visual-diff --nobuild --master');
   } else {
@@ -51,7 +51,7 @@ function main() {
         buildTargets.has('VISUAL_DIFF') ||
         buildTargets.has('FLAG_CONFIG')) {
 
-      downloadBuildOutput();
+      downloadBuildOutput(FILENAME);
       process.env['PERCY_TOKEN'] = atob(process.env.PERCY_TOKEN_ENCODED);
       timedExecOrDie('gulp visual-diff --nobuild');
     } else {

diff --git a/build-system/tasks/presubmit-checks.js b/build-system/tasks/presubmit-checks.js
@@ -106,12 +106,12 @@ const forbiddenTerms = {
       'build-system/check-package-manager.js',
       'build-system/pr-check/build.js',
       'build-system/pr-check/checks.js',
-      'build-system/pr-check/dist-test.js',
-      'build-system/pr-check/local-test.js',
-      'build-system/pr-check/remote-test.js',
+      'build-system/pr-check/dist-tests.js',
+      'build-system/pr-check/local-tests.js',
+      'build-system/pr-check/remote-tests.js',
       'build-system/pr-check/utils.js',
-      'build-system/pr-check/validator.js',
-      'build-system/pr-check/visual-diff-test.js',
+      'build-system/pr-check/validator-tests.js',
+      'build-system/pr-check/visual-diff-tests.js',
       'validator/nodejs/index.js', // NodeJs only.
       'validator/engine/parse-css.js',
       'validator/engine/validator-in-browser.js',