add Node.js 8 to Travis CI #2086

alexlamsl · 2017-06-12T17:47:32Z

closes #2081

kzc · 2017-06-12T17:55:35Z

test/mocha/release.js

@@ -8,7 +8,7 @@ function run(command, args, done) {
        process.stdout.write("\0");
    }, 5 * 60 * 1000);
    spawn(command, args, {
-        stdio: "ignore"
+        stdio: [ "ignore", 1, 2 ]


Am I correct in assuming you want to ignore both stdout and stderr instead of just stdout?

Here I'm ignoring stdin, and pipe both stdout and stderr out.

Used to be I just ignore them all to avoid getting flooded on the console, but now that I'm comfortable enough navigating around them, I think it makes sense to have Travis CI record these details down, so we can have extra data points for diagnosis and even say performance tracking via test/benchmark.js timings.

If this works out I will also be removing that process.stdout.write("\0"); at 5min intervals, as that was a workaround for Travis CI (& none of the individual scripts from test/benchmark.js or test/jetstream.js should take more than 10 minutes to be uglified).

FYI, these extra lines are the result of the aforementioned change:

https://travis-ci.org/mishoo/UglifyJS2/jobs/242117329#L1299-L1402

Good idea. I guess we'll know in a few minutes why it's failing.

travis timeout on node 0.12:
https://travis-ci.org/mishoo/UglifyJS2/builds/242114674?utm_source=github_status&utm_medium=notification

Just saw that - job restarted 👍

alexlamsl · 2017-06-12T18:40:49Z

So it looks to me that with Node.js 8, test/jetstream.js somehow fails to terminate its process:

https://travis-ci.org/mishoo/UglifyJS2/jobs/242117333#L2580

kzc · 2017-06-12T18:48:23Z

Spawned processes are inherently unreliable. Wouldn't surprise me if a bug cropped up in node 8 as historically node has had problems with flushing streams upon process.exit() and uncaught exceptions.

If test/jetstream.js sees the output "JetStream completed successfully." can termination be forced?

alexlamsl · 2017-06-12T18:51:05Z

If test/jetstream.js sees the output "JetStream completed successfully." can termination be forced?

I can just force process.exit() at the end if that turns out to be necessary, so no trouble there. I just have this bad habit of letting Nature takes its course 😉

kzc · 2017-06-12T18:59:14Z

I can just force process.exit() at the end if that turns out to be necessary, so no trouble there. I just have this bad habit of letting Nature takes its course

God knows what node is doing at that point. Probably deadlocked itself or waiting for something to close that will never happen.

alexlamsl · 2017-06-12T19:04:41Z

If there is indeed something left in the event dispatch loop that prevents node from terminating automatically, it's either Linux or Travis specific, since I've tried on Windows:

$ git clean -xdf
$ npm install --no-optional --no-save
$ set UGLIFYJS_TEST_ALL=1
$ npm test

and I have yet to observe any failures locally.

kzc · 2017-06-12T19:15:37Z

test/jetstream.js

@@ -64,6 +64,7 @@ if (typeof phantom == "undefined") {
                    server.close();
                    if (code) throw new Error("JetStream failed!");
                    console.log("JetStream completed successfully.");
+                    process.exit();


Use process.exit(0); to convey it's a success.

Done in df1c48e

alexlamsl · 2017-06-12T20:17:11Z

With the addition of process.exit(0), Node.js 8 test now passes.
https://travis-ci.org/mishoo/UglifyJS2/jobs/242146287

kzc · 2017-06-12T21:01:12Z

node 0.12 timed out: https://travis-ci.org/mishoo/UglifyJS2/jobs/242153107

Here's an idea - can you do a SHA1 or MD5 checksum on each JS file with the CLI options prepended to it and cache the uglify result in memory the first time it is run? Since the jetstream tests are run 3 times each it should cut the runtime by 66%.

alexlamsl · 2017-06-12T21:04:18Z

Since the jetstream tests are run 3 times each it should cut the runtime by 66%.

Heh - we are only running those tests once 😉

https://github.com/mishoo/UglifyJS2/blob/b0eab71470fa887752bcf5442fbb26da3f724ef1/test/jetstream.js#L96

kzc · 2017-06-12T21:07:23Z

Curses! Foiled again!

alexlamsl · 2017-06-12T21:07:45Z

What it does show is the console I/O seems to play a significant part in job performance. Now this could be Travis having transient slowdown, so we'll need to run more of them in the near future to see if the pattern holds.

- explicitly terminate `test/jetstream.js` upon completion - log verbose output from `test/benchmark.js` & `test/jetstream.js` - remove obsolete workaround for Travis CI

kzc · 2017-06-12T21:13:01Z

Pure speculation: when there's an I/O event the job gets demoted in priority. What else could explain how these tests take 5 minutes locally and 50 minutes on travis CI?

How many active uglify sub-processes are active at any point in time? You know how most browsers have 6 connections to the same web server to parallelize downloads - can we limit that concurrency on the PhantomJS fake browser client to just 2 max?

alexlamsl · 2017-06-12T21:27:59Z

can we limit that concurrency on the PhantomJS fake browser client to just 2 max?

JetStream benchmark itself seems to only download the relevant script files, one after another, right before executing them. So regardless of the parallelism offered by phantomjs, I have yet to observe multiple instance of bin/uglifyjs being active simultaneously locally.

As for the I/O, I think they are storing that in some form of database backend, which seems to frequently be the cause of slow-downs / outages judging from their status page.

Edit: for instance, https://www.traviscistatus.com/incidents/93wfcv0ksqrz

kzc · 2017-06-12T21:49:56Z

Can we store the Jetstream JS and html files in the uglify repo to not incur as much I/O? I think it's a job priority problem.

alexlamsl · 2017-06-12T21:55:47Z

Can we store the Jetstream JS and html files in the uglify repo to not incur as much I/O? I think it's a job priority problem.

Oh you mean the network I/O over the Internet? I was comparing stdout volume as there seems to be a correlation between more text printed and longer total runtime.

Storing the full copy of JetStream locally seems a little excessive. Technically, we can make the CI job go fetch (via wget?) those contents in advance before we run the tests, so at least that way we don't get interrupted network I/O activities. I'm no expert in using *nix for site ripping though.

kzc · 2017-06-12T22:30:31Z

Oh you mean the network I/O over the Internet?

Yes.

I was comparing stdout volume as there seems to be a correlation between more text printed and longer total runtime.

Anything's possible. Just throwing out ideas.

Technically, we can make the CI job go fetch (via wget?) those contents in advance before we run the tests, so at least that way we don't get interrupted network I/O activities.

That could work.

kzc reviewed Jun 12, 2017

View reviewed changes

kzc mentioned this pull request Jun 12, 2017

[WIP] add node 8 to .travis.yml #2081

Closed

kzc reviewed Jun 12, 2017

View reviewed changes

add Node.js 8 to Travis CI

bcd2a0b

- explicitly terminate `test/jetstream.js` upon completion - log verbose output from `test/benchmark.js` & `test/jetstream.js` - remove obsolete workaround for Travis CI

alexlamsl force-pushed the travis-node-8 branch from df1c48e to bcd2a0b Compare June 12, 2017 21:13

alexlamsl changed the title ~~[WIP] add Node.js 8 to Travis CI~~ add Node.js 8 to Travis CI Jun 12, 2017

alexlamsl merged commit 3dc9e14 into mishoo:master Jun 12, 2017

alexlamsl deleted the travis-node-8 branch June 12, 2017 22:21

alexlamsl mentioned this pull request Jun 13, 2017

cache web assets between CI runs #2089

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add Node.js 8 to Travis CI #2086

add Node.js 8 to Travis CI #2086

alexlamsl commented Jun 12, 2017

kzc Jun 12, 2017

alexlamsl Jun 12, 2017

alexlamsl Jun 12, 2017

alexlamsl Jun 12, 2017

kzc Jun 12, 2017

kzc Jun 12, 2017

alexlamsl Jun 12, 2017

alexlamsl commented Jun 12, 2017

kzc commented Jun 12, 2017

alexlamsl commented Jun 12, 2017

kzc commented Jun 12, 2017

alexlamsl commented Jun 12, 2017

kzc Jun 12, 2017

alexlamsl Jun 12, 2017

alexlamsl commented Jun 12, 2017

kzc commented Jun 12, 2017

alexlamsl commented Jun 12, 2017

kzc commented Jun 12, 2017

alexlamsl commented Jun 12, 2017

kzc commented Jun 12, 2017

alexlamsl commented Jun 12, 2017 •

edited

kzc commented Jun 12, 2017

alexlamsl commented Jun 12, 2017

kzc commented Jun 12, 2017

add Node.js 8 to Travis CI #2086

add Node.js 8 to Travis CI #2086

Conversation

alexlamsl commented Jun 12, 2017

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

alexlamsl commented Jun 12, 2017

kzc commented Jun 12, 2017

alexlamsl commented Jun 12, 2017

kzc commented Jun 12, 2017

alexlamsl commented Jun 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexlamsl commented Jun 12, 2017

kzc commented Jun 12, 2017

alexlamsl commented Jun 12, 2017

kzc commented Jun 12, 2017

alexlamsl commented Jun 12, 2017

kzc commented Jun 12, 2017

alexlamsl commented Jun 12, 2017 • edited

kzc commented Jun 12, 2017

alexlamsl commented Jun 12, 2017

kzc commented Jun 12, 2017

alexlamsl commented Jun 12, 2017 •

edited