🏗 Silence subprocesses in visual diffs by default #16704

danielrozenberg · 2018-07-12T18:38:29Z

codecov-io · 2018-07-12T18:58:52Z

Codecov Report

Merging #16704 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #16704      +/-   ##
==========================================
- Coverage   78.12%   78.09%   -0.04%     
==========================================
  Files         553      553              
  Lines       40336    40325      -11     
==========================================
- Hits        31513    31492      -21     
- Misses       8823     8833      +10

Flag	Coverage Δ
#integration_tests	`34.91% <ø> (-0.03%)`	⬇️
#unit_tests	`77.16% <ø> (-0.03%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e2735b4...3f3ce4d. Read the comment docs.

rsimha

A few comments below.

rsimha · 2018-07-12T19:03:48Z

build-system/tasks/visual-diff.js

-    process.env.WEBSERVER_QUIET = '';
+    argv.chrome_debug = true; // eslint-disable-line google-camelcase/google-camelcase
+    argv.gulp_debug = true; // eslint-disable-line google-camelcase/google-camelcase
+    argv.webserver_debug = true; // eslint-disable-line google-camelcase/google-camelcase


I think you can avoid having to disable the camel case warning by using argv['chrome_debug'], etc.

Thank the heavens

rsimha · 2018-07-12T20:47:18Z

build-system/exec.js

@@ -62,9 +62,10 @@ exports.execScriptAsync = function(script, options) {
 * Executes the provided command, and terminates the program in case of failure.
 *
 * @param {string} cmd Command line to execute.
+ * @param {<Object>} options extra options send to the process.


nit: Extra options to send to the process

rsimha · 2018-07-12T20:56:56Z

build-system/tasks/visual-diff.js

+    // The child node process has an asynchronous stdout. See #10409.
+    await sleep(100);
+  }
+  process.exit();


process.exit() doesn't do a clean exit, and could cause stray console output to be printed after the gulp process has printed its completion status. I see that you're doing a sleep after killing webServerProcess to avoid this.

A better way to signal task completion is to return a promise or call done(). If the exit code needs to be something other than 0, you can set the value of process.exitCode before signaling task completion.

Having said that, I'm not even sure you need to call process.exit here, since there's nothing more to do after returning from shutdown.

What is this "done" that you're talking about? I saw it in tests but not in any of the gulp files

Unfortunately the process doesn't exit unless I explicitly call process.exit(), for some reason
My guess is that somehow the child processes are hanging it. I'd be happy if you can help me figure this out! I want the process to exit cleanly...

done is just a callback you can pass to a gulp task (can be called anything). Look up the gulp 3.9 docs.

For the child task, you could try sending it a kill instead of an int, and then call done in the main task. If all that fails, add a TODO.

rsimha · 2018-07-12T20:59:30Z

build-system/tasks/visual-diff.js

@@ -306,7 +319,8 @@ function cleanupAmpConfig() {
  log('verbose', 'Cleaning up existing AMP config');
  AMP_RUNTIME_TARGET_FILES.forEach(targetFile => {
    execOrDie(
-        `gulp prepend-global --local_dev --target ${targetFile} --remove`);
+        `gulp prepend-global --local_dev --target ${targetFile} --remove`,
+        getStdioOptions(argv.gulp_debug));


gulp prepend-global doesn't really print any useful info, so I'm not convinced that the gulp_debug flag is necessary. (The log('verbose') above should be sufficient.)

I think you can simply pass in {'stdio': 'ignore'} here.

rsimha · 2018-07-12T21:00:33Z

build-system/tasks/visual-diff.js

-        `gulp prepend-global --local_dev --target ${targetFile} ` +
-        `--${config}`);
+        `gulp prepend-global --local_dev --target ${targetFile} --${config}`,
+        getStdioOptions(argv.gulp_debug));


Same comment as above.

rsimha · 2018-07-12T21:01:43Z

build-system/tasks/visual-diff.js

@@ -116,7 +116,7 @@ function setPercyBranch() {
 async function launchWebServer() {
  const gulpServeAsync = execScriptAsync(
      `gulp serve --host ${HOST} --port ${PORT} ${process.env.WEBSERVER_QUIET}`,
-      {stdio: 'inherit'});
+      getStdioOptions(argv.gulp_debug || argv.webserver_debug));

  gulpServeAsync.on('exit', code => {
    if (code != 0) {


I'm seeing an error code of null being returned by gulp serve in the .on('exit', ...) handler: https://travis-ci.org/ampproject/amphtml/jobs/403246979#L627

[18:55:04] ERROR: 'serve' errored with code null. Cannot continue with visual diff tests

From the spawn documentation, it appears you should be handling errors in .on('close', ...) and not .on('exit', ...).

rsimha · 2018-07-12T21:03:05Z

build-system/tasks/visual-diff.js

@@ -116,7 +116,7 @@ function setPercyBranch() {
 async function launchWebServer() {
  const gulpServeAsync = execScriptAsync(
      `gulp serve --host ${HOST} --port ${PORT} ${process.env.WEBSERVER_QUIET}`,
-      {stdio: 'inherit'});
+      getStdioOptions(argv.gulp_debug || argv.webserver_debug));


I think the server output should depend only on whether webserver_debug is enabled. This could be simplified to:

{'stdio': argv.webserver_debug ? 'inherit' : 'ignore'},

Sure, but in this case argv.gulp_debug becomes useless. I'll remove it too :)

rsimha · 2018-07-12T21:04:29Z

build-system/tasks/visual-diff.js

+ * @param {boolean} shouldPrint true if the sub-process should print its output
+ * @return {<Object>} sub-process options
+ */
+function getStdioOptions(shouldPrint) {


I don't think this function is required. See other comments.

rsimha · 2018-07-12T21:28:51Z

build-system/tasks/visual-diff.js

+ * @param {!ChildProcess} webServerProcess the webserver process to shut down.
+ */
+async function shutdown(webServerProcess) {
+  if (webServerProcess.exitCode == null) {


Is this check meant to see if webServerProcess is still running? If so, I think you can use webServerProcess.connected. See reference.

I failed to figure this out, and since the current code, fragile as it might be, "works™", I just added a TODO in your name to figure this out :)

rsimha · 2018-07-13T16:26:34Z

build-system/exec.js

@@ -62,7 +62,7 @@ exports.execScriptAsync = function(script, options) {
 * Executes the provided command, and terminates the program in case of failure.
 *
 * @param {string} cmd Command line to execute.
- * @param {<Object>} options extra options send to the process.
+ * @param {<Object>} options Extra options send to the process.


The nit was that you were missing to before send :)

🙄🙄🙄

Silence subprocesses in visual diffs by default

9899611

danielrozenberg requested a review from rsimha July 12, 2018 18:38

googlebot added the cla: yes label Jul 12, 2018

rsimha suggested changes Jul 12, 2018

View reviewed changes

@rsimha's comments

c949da2

rsimha approved these changes Jul 13, 2018

View reviewed changes

TO

3f3ce4d

danielrozenberg merged commit 7afc160 into ampproject:master Jul 13, 2018

danielrozenberg deleted the silence-visual-diff-logs-travis branch July 13, 2018 17:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🏗 Silence subprocesses in visual diffs by default #16704

🏗 Silence subprocesses in visual diffs by default #16704

danielrozenberg commented Jul 12, 2018

codecov-io commented Jul 12, 2018 •

edited

rsimha left a comment

rsimha Jul 12, 2018

danielrozenberg Jul 13, 2018

rsimha Jul 12, 2018

danielrozenberg Jul 13, 2018

rsimha Jul 12, 2018

danielrozenberg Jul 13, 2018

rsimha Jul 13, 2018

rsimha Jul 12, 2018

danielrozenberg Jul 13, 2018

rsimha Jul 12, 2018

danielrozenberg Jul 13, 2018

rsimha Jul 12, 2018 •

edited

rsimha Jul 12, 2018

danielrozenberg Jul 13, 2018

rsimha Jul 12, 2018

danielrozenberg Jul 13, 2018

rsimha Jul 12, 2018 •

edited

danielrozenberg Jul 13, 2018

rsimha Jul 13, 2018

danielrozenberg Jul 13, 2018

🏗 Silence subprocesses in visual diffs by default #16704

🏗 Silence subprocesses in visual diffs by default #16704

Conversation

danielrozenberg commented Jul 12, 2018

codecov-io commented Jul 12, 2018 • edited

Codecov Report

rsimha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsimha Jul 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsimha Jul 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jul 12, 2018 •

edited

rsimha Jul 12, 2018 •

edited

rsimha Jul 12, 2018 •

edited