Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: enable parallel testing in test-ci #4476

Conversation

jbergstroem
Copy link
Member

Run tests in parallel by default in continuous integration.

@jbergstroem jbergstroem added the test Issues and PRs related to the tests. label Dec 30, 2015
@jbergstroem jbergstroem force-pushed the feature/enable-ci-parallel-tests branch from 292fc26 to 3c39897 Compare December 30, 2015 05:20
@jbergstroem
Copy link
Member Author

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/

@jbergstroem
Copy link
Member Author

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

@mscdex mscdex added the build Issues and PRs related to build files or the CI. label Dec 30, 2015
@jbergstroem jbergstroem force-pushed the feature/enable-ci-parallel-tests branch from 3c39897 to 58ab1f9 Compare December 30, 2015 06:54
@jbergstroem
Copy link
Member Author

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)

@jbergstroem
Copy link
Member Author

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

@rvagg
Copy link
Member

rvagg commented Dec 30, 2015

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

@jbergstroem
Copy link
Member Author

@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.

@Trott
Copy link
Member

Trott commented Dec 30, 2015

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.

Trott added a commit to Trott/io.js that referenced this pull request Dec 30, 2015
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)
@Trott
Copy link
Member

Trott commented Dec 30, 2015

Fedora fix (hopefully): #4490

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

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.

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

aad4a33d39d0759cf0b95d8e53b85595 596x329x1

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.

@Trott
Copy link
Member

Trott commented Dec 31, 2015

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?

@Trott
Copy link
Member

Trott commented Dec 31, 2015

(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.)

@jbergstroem
Copy link
Member Author

@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.

Trott added a commit to Trott/io.js that referenced this pull request Dec 31, 2015
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)
@Trott
Copy link
Member

Trott commented Dec 31, 2015

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

Trott added a commit to Trott/io.js that referenced this pull request Dec 31, 2015
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
@Trott Trott mentioned this pull request Dec 31, 2015
@Trott
Copy link
Member

Trott commented Dec 31, 2015

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

@jbergstroem
Copy link
Member Author

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..

@jbergstroem
Copy link
Member Author

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)

@Trott
Copy link
Member

Trott commented Jan 20, 2016

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?

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

Trott commented Jan 20, 2016

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".

MylesBorins pushed a commit that referenced this pull request Jan 28, 2016
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>
MylesBorins pushed a commit that referenced this pull request Jan 28, 2016
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>
MylesBorins pushed a commit that referenced this pull request Jan 28, 2016
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>
MylesBorins pushed a commit that referenced this pull request Feb 11, 2016
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>
MylesBorins pushed a commit that referenced this pull request Feb 11, 2016
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>
MylesBorins pushed a commit that referenced this pull request Feb 11, 2016
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>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 11, 2016
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>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 11, 2016
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>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 11, 2016
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>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 15, 2016
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>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 15, 2016
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>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 15, 2016
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>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
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>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
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>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
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>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
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>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
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>
@jasnell
Copy link
Member

jasnell commented Apr 30, 2016

Any updates on this?

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Apr 30, 2016
@jbergstroem
Copy link
Member Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. stalled Issues and PRs that are stalled. test Issues and PRs related to the tests. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants