Fix flakiness of pipeflood test #3636
Conversation
|
Removed flaky status for the test. Rerun CI just because: https://ci.nodejs.org/job/node-test-pull-request/664/ |
mscdex
added
http
|
Well, this is a bummer, but it looks like this does not fix the pipeflood test problem on Windows: https://ci.nodejs.org/job/node-test-binary-windows/189/RUN_SUBSET=1,VS_VERSION=vs2015,label=win2012r2/tapTestReport/test.tap-228/ |
|
Although it's failing differently so maybe it can be tweaked... |
|
Wrapping the With it wrapped and set to run 16 times, no failures:
With it not wrapped and set to run 16 times, failures:
With changes to get rid of ES6-isms, etc., Node v0.10.20 fails on the test still and node v0.10.21 passes, which is as it should be. |
|
Now it's failing on the Pi. (╯°□°）╯︵ ┻━┻ https://ci.nodejs.org/job/node-test-binary-arm/431/RUN_SUBSET=1,nodes=pi2-raspbian-wheezy/console |
|
Switched to |
|
/cc @dnakamura @rvagg @isaacs |
| @@ -8,38 +18,49 @@ switch (process.argv[2]) { | |||
| case 'child': | |||
| return child(); | |||
| default: | |||
| throw new Error('wtf'); | |||
| throw Error(`Unexpected value: ${process.argv[2]}`); | |||
There was a problem hiding this comment.
I vaguely remember a PR to make all Errors be called with new. May be worthwhile to make it consistent?
There was a problem hiding this comment.
@evanlucas Sure, we can do that. .... Done!
| // prevention should kick in. | ||
| // This means the stream should emit no more 'data' events. However we | ||
| // may still be asked to process more requests if they were read before | ||
| // mechanism activated. |
There was a problem hiding this comment.
"the mechanism" or "the flood prevention mechanism"?
|
I think this test could live in test/parallel but I'd appreciate it if you did the move in a separate commit. |
|
OK, made all the changes suggested by @bnoordhuis, rebased, force pushed. PTAL |
|
CI: https://ci.nodejs.org/job/node-test-pull-request/708/ Hope it doesn't have any surprises... |
This extends fixes for test-https-pipeline-flood to hopefully fully eliminate its flakiness on Windows in our continuous integration process. PR-URL: #3636 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
|
landed in v4.x-staging in 5254fda...9294523 |
|
Would be nice to have this landed in 0.12.X as it is the last persistent failure on windows that keeps builds from being green |
PR-URL: #3636 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
This extends fixes for test-https-pipeline-flood to hopefully fully eliminate its flakiness on Windows in our continuous integration process. PR-URL: #3636 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
PR-URL: #3636 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
This extends fixes for test-https-pipeline-flood to hopefully fully eliminate its flakiness on Windows in our continuous integration process. PR-URL: #3636 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
PR-URL: #3636 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
This extends fixes for test-https-pipeline-flood to hopefully fully eliminate its flakiness on Windows in our continuous integration process. PR-URL: #3636 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
This extends fixes for test-https-pipeline-flood to hopefully fully eliminate its flakiness on Windows in our continuous integration process. PR-URL: nodejs#3636 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
|
@Trott I'm wondering why its set as dont-land-on-v4.x and the lts-watch-v0.12 was removed. Sounds like we are not backporting but not clear why from what I've read in the issue. |
Continuation of #2862