test: enable parallel testing in test-ci #4476
Conversation
jbergstroem
force-pushed
the
feature/enable-ci-parallel-tests
branch
from
292fc26
to
3c39897
Compare
|
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. |
|
Hm, I'll have another look at the realpath tests; must've messed that up. |
jbergstroem
force-pushed
the
feature/enable-ci-parallel-tests
branch
from
3c39897
to
58ab1f9
Compare
|
Rebased and new CI: https://ci.nodejs.org/job/node-test-commit/1573/ I've fixed the failing freebsd test by setting $HOME to |
|
The reason arm fails is because the fanned tests aren't called through |
|
YES, those CI times are looking pretty good. What can we do to fix up the failures here? |
|
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 |
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 |
jbergstroem
force-pushed
the
feature/enable-ci-parallel-tests
branch
from
58ab1f9
to
021b682
Compare
|
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
force-pushed
the
feature/enable-ci-parallel-tests
branch
from
021b682
to
3be6c10
Compare
|
https://ci.nodejs.org/job/node-test-commit/1585/ Edit:
|
|
Looking at the SmartOS failure for |
|
(Or, alternatively, leave it in |
|
@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
|
|
|
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 |
|
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 |
|
Added the |
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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Rich Trott <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Rich Trott <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Rich Trott <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Rich Trott <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]> Reviewed-By: Johan Bergström <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Rich Trott <[email protected]>
estliberitas
force-pushed
the
master
branch
2 times, most recently
from
7da4fd4
to
c7066fb
Compare
|
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. |
Run tests in parallel by default in continuous integration.