test: enable parallel testing in test-ci by jbergstroem · Pull Request #4476 · nodejs/node · GitHub

Run tests in parallel by default in continuous integration.

Just added a second commit to test it properly in CI. Once landed, we'll add the temp-dir path by environment so its backwards compatible.

CI: https://ci.nodejs.org/job/node-test-commit/1570/

Hm, I'll have another look at the realpath tests; must've messed that up.

Rebased and new CI: https://ci.nodejs.org/job/node-test-commit/1573/

I've fixed the failing freebsd test by setting $HOME to /usr/home/iojs (/home is hardlinked to /usr/home on freebsd making path comparisons based on $HOME fail)

The reason arm fails is because the fanned tests aren't called through make test-ci (and therefore doesn't change the temporary directory)

YES, those CI times are looking pretty good. What can we do to fix up the failures here?

@rvagg: It's a work in progress. I'll look at it; can hopefully get @Trott involved as well. I think SmartOS will need most work.

A fix (or, depending on your perspective, a workaround) for the arm-fanned failures is already in. Woot.

I'm speculating but the one error on fedora may be because 50 milliseconds is just not enough time if the host is very busy with other stuff (like, say, running some other tests in parallel, one or two of which might be resource-intensive).

I'm going to refactor that test to get rid of the setTimeout() so that won't be an issue, hopefully.

Refactor test to remove unnecessary booleans and one unnecesary timer. Instead, throw Error objects where appropriate and rely on common.mustCall(). The timer seemed to be the source of an issue when parallelizing tests. Ref: nodejs#4476 (comment)

Fedora fix (hopefully): #4490

I just backed out the path commit since I've added it to jenkins as well as rebased to get a few more fixes. New run shortly.

https://ci.nodejs.org/job/node-test-commit/1585/

Edit:

smartos32 -- my bad.
arm path fails: I forgot to edit the binary test job to add test path. Also, since we don't invoke test-ci we don't get -J.

Looking at the SmartOS failure for test-child-process-fork-net2.js, I wonder if that test really belongs in sequential since it seems to be failing on a sort of timing/performance benchmark that may be due to other tests consuming resources?

(Or, alternatively, leave it in parallel but get rid of the time check. I'm not sure performance checks should be mixed in with functionality checks like that in tests.)

@Trott: I'd be happy to get rid of a lot of time checks in the suite. I wonder if its a remnant of something else when node was slower.

Refactor test to remove unnecessary booleans and one unnecesary timer. Instead, throw Error objects where appropriate and rely on common.mustCall(). The timer seemed to be the source of an issue when parallelizing tests. Ref: nodejs#4476 (comment)

OK, I'll get rid of the time checks and send over a PR in a bit...

test-child-process-fork-net2.js checks that things happen within certain time constraints, thus doubling as a benchmark test in addition to a functionality test. This change removes the time check, as it was causing the test to fail on SmartOS and Windows (and possibly elsewhere) when the tests were run in parallel on CI. There is no guarantee that other tests won't consume enough resources to slow this test down, so don't check the time constraints (beyond the generous timeout that the test is given by test.py in the first place, of course). If we want to do benchmark/performance tests, we should keep them separate from pure functionality tests. The time check may have been a remnant of the distant past when Node.js was much slower. It predates io.js Ref: nodejs#4476

test-child-process-fork-net2.js fix (hopefully): #4494

Because we more than one way compiling/testing from jenkins at the moment. I'd rather make the way we call compiling/testing more consistent down the line and move it back then. Haven't decided but thats at least my rationale. Need to look through all jobs..

Also, the environment variables JOBS and NODE_TMP_DIR should be set in all init scripts at the runners to make sure we don't run into failures as a result of not setting them (pathmax, to high parallelism, etc)

Should we close this PR and you can open another one to parallelize the builds a different way (especially if those changes need to happen over in nodejs/build instead of this repo or something like that)? Or will you add on to this PR to get it to be the way you want?

Added the in progress label to this PR which hopefully is the equivalent of "hey, don't land this because it's not fully baked yet".

To enable greater parallelization of tests on CI, move resource intensive tests out of parallel and into sequential. Ref: #4476 PR-URL: #4615 Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: James M Snell <jasnell@gmail.com>

A 50ms timeout results in a race condition. Instead, enforce expected order through callbacks. This has the side effect of speeding up the test in most situations. Ref: #4476 PR-URL: #4637 Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: James M Snell <jasnell@gmail.com>

Prior to this commit, the test was flaky because it was executing the majority of its logic in a function called from the client and multiple events on the server. This commit simplifies the test by separating the server's connection and listening events, and isolating the client logic. Refs: #4476 Refs: #4644 PR-URL: #4650 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

To enable greater parallelization of tests on CI, move resource intensive tests out of parallel and into sequential. Ref: #4476 PR-URL: #4615 Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: James M Snell <jasnell@gmail.com>

A 50ms timeout results in a race condition. Instead, enforce expected order through callbacks. This has the side effect of speeding up the test in most situations. Ref: #4476 PR-URL: #4637 Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: James M Snell <jasnell@gmail.com>

Prior to this commit, the test was flaky because it was executing the majority of its logic in a function called from the client and multiple events on the server. This commit simplifies the test by separating the server's connection and listening events, and isolating the client logic. Refs: #4476 Refs: #4644 PR-URL: #4650 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

To enable greater parallelization of tests on CI, move resource intensive tests out of parallel and into sequential. Ref: nodejs#4476 PR-URL: nodejs#4615 Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: James M Snell <jasnell@gmail.com>

A 50ms timeout results in a race condition. Instead, enforce expected order through callbacks. This has the side effect of speeding up the test in most situations. Ref: nodejs#4476 PR-URL: nodejs#4637 Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: James M Snell <jasnell@gmail.com>

Prior to this commit, the test was flaky because it was executing the majority of its logic in a function called from the client and multiple events on the server. This commit simplifies the test by separating the server's connection and listening events, and isolating the client logic. Refs: nodejs#4476 Refs: nodejs#4644 PR-URL: nodejs#4650 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

To enable greater parallelization of tests on CI, move resource intensive tests out of parallel and into sequential. Ref: nodejs#4476 PR-URL: nodejs#4615 Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: James M Snell <jasnell@gmail.com>

A 50ms timeout results in a race condition. Instead, enforce expected order through callbacks. This has the side effect of speeding up the test in most situations. Ref: nodejs#4476 PR-URL: nodejs#4637 Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: James M Snell <jasnell@gmail.com>

Prior to this commit, the test was flaky because it was executing the majority of its logic in a function called from the client and multiple events on the server. This commit simplifies the test by separating the server's connection and listening events, and isolating the client logic. Refs: nodejs#4476 Refs: nodejs#4644 PR-URL: nodejs#4650 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Refactor test to remove unnecessary booleans and one unnecesary timer. Instead, throw Error objects where appropriate and rely on common.mustCall(). The timer seemed to be the source of an issue when parallelizing tests. Ref: nodejs#4476 (comment) PR-URL: nodejs#4490 Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

test-child-process-fork-net2.js checks that things happen within certain time constraints, thus doubling as a benchmark test in addition to a functionality test. This change removes the time check, as it was causing the test to fail on SmartOS and Windows (and possibly elsewhere) when the tests were run in parallel on CI. There is no guarantee that other tests won't consume enough resources to slow this test down, so don't check the time constraints (beyond the generous timeout that the test is given by test.py in the first place, of course). If we want to do benchmark/performance tests, we should keep them separate from pure functionality tests. The time check may have been a remnant of the distant past when Node.js was much slower. It predates io.js Ref: nodejs#4476 PR-URL: nodejs#4494 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu>

To enable greater parallelization of tests on CI, move resource intensive tests out of parallel and into sequential. Ref: nodejs#4476 PR-URL: nodejs#4615 Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: James M Snell <jasnell@gmail.com>

A 50ms timeout results in a race condition. Instead, enforce expected order through callbacks. This has the side effect of speeding up the test in most situations. Ref: nodejs#4476 PR-URL: nodejs#4637 Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: James M Snell <jasnell@gmail.com>

Prior to this commit, the test was flaky because it was executing the majority of its logic in a function called from the client and multiple events on the server. This commit simplifies the test by separating the server's connection and listening events, and isolating the client logic. Refs: nodejs#4476 Refs: nodejs#4644 PR-URL: nodejs#4650 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Any updates on this?

@jasnell I'll close this. We've done enough work in the src repo. What's left is implementing environment variables across the buildfarm which we'll be doing over at nodejs/build#291.

jbergstroem added the test Issues and PRs related to the tests. label Dec 30, 2015

jbergstroem force-pushed the feature/enable-ci-parallel-tests branch from 292fc26 to 3c39897 Compare December 30, 2015 05:20

jbergstroem mentioned this pull request Dec 30, 2015

test: don't use cwd for relative path #4477

Merged

mscdex added the build Issues and PRs related to build files or the CI. label Dec 30, 2015

jbergstroem force-pushed the feature/enable-ci-parallel-tests branch from 3c39897 to 58ab1f9 Compare December 30, 2015 06:54

rvagg mentioned this pull request Dec 30, 2015

test-joyent-smartos14-x86-1 is painfully slow nodejs/build#293

Closed

jasnell added lts-watch-v4.x and removed lts-watch-v4.x labels Dec 30, 2015

Trott mentioned this pull request Dec 30, 2015

test: refactor test-fs-empty-readStream #4490

Closed

jbergstroem force-pushed the feature/enable-ci-parallel-tests branch from 58ab1f9 to 021b682 Compare December 30, 2015 23:58

jbergstroem force-pushed the feature/enable-ci-parallel-tests branch from 021b682 to 3be6c10 Compare December 31, 2015 00:14

Trott mentioned this pull request Dec 31, 2015

test: remove time check #4494

Closed

Trott added the wip Issues and PRs that are still a work in progress. label Jan 20, 2016

jbergstroem mentioned this pull request Apr 5, 2016

Enable parallel testing in CI nodejs/build#379

Closed

estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fb Compare April 26, 2016 05:22

jasnell added the stalled Issues and PRs that are stalled. label Apr 30, 2016

jbergstroem closed this May 3, 2016

MylesBorins removed the lts-watch-v4.x label May 3, 2016

test: enable parallel testing in test-ci by jbergstroem · Pull Request #4476 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: enable parallel testing in test-ci - #4476

test: enable parallel testing in test-ci#4476jbergstroem wants to merge 1 commit intonodejs:masternodejs/node:masterfrom jbergstroem:feature/enable-ci-parallel-testsjbergstroem/node:feature/enable-ci-parallel-testsCopy head branch name to clipboard

Conversation

jbergstroem commented Dec 30, 2015

Uh oh!

jbergstroem commented Dec 30, 2015

Uh oh!

jbergstroem commented Dec 30, 2015

Uh oh!

jbergstroem commented Dec 30, 2015

Uh oh!

jbergstroem commented Dec 30, 2015

Uh oh!

rvagg commented Dec 30, 2015

Uh oh!

jbergstroem commented Dec 30, 2015

Uh oh!

Trott commented Dec 30, 2015

Uh oh!

Trott commented Dec 30, 2015

Uh oh!

jbergstroem commented Dec 31, 2015

Uh oh!

jbergstroem commented Dec 31, 2015

Uh oh!

Trott commented Dec 31, 2015

Uh oh!

Trott commented Dec 31, 2015

Uh oh!

jbergstroem commented Dec 31, 2015

Uh oh!

Trott commented Dec 31, 2015

Uh oh!

Trott commented Dec 31, 2015

Uh oh!

jbergstroem commented Jan 20, 2016

Uh oh!

jbergstroem commented Jan 20, 2016

Uh oh!

Trott commented Jan 20, 2016

Uh oh!

Trott commented Jan 20, 2016

Uh oh!

jasnell commented Apr 30, 2016

Uh oh!

jbergstroem commented May 3, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

test: enable parallel testing in test-ci#4476
jbergstroem wants to merge 1 commit into
nodejs:masterfrom
jbergstroem:feature/enable-ci-parallel-tests