Fix flakiness of pipeflood test#3636

Trott · 2015-11-02T23:41:03Z

Continuation of #2862

Trott · 2015-11-02T23:42:54Z

CI: https://ci.nodejs.org/job/node-test-pull-request/663/

Trott · 2015-11-02T23:58:27Z

Removed flaky status for the test. Rerun CI just because: https://ci.nodejs.org/job/node-test-pull-request/664/

Trott · 2015-11-03T03:50:01Z

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/

Trott · 2015-11-03T03:51:42Z

Although it's failing differently so maybe it can be tweaked...

Trott · 2015-11-07T05:33:37Z

Wrapping the req.socket.on('data',...) business in setImmediate() (so that any events already in the event queue will fire first?), the test works reliably on Windows now.

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.

Trott · 2015-11-07T05:36:25Z

CI: https://ci.nodejs.org/job/node-test-pull-request/685/

Trott · 2015-11-07T05:50:14Z

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

Trott · 2015-11-07T06:26:00Z

Switched to common.platformTimeout() instead of hardcoding the same timeouts for arm as for everything else. CI run looks better. https://ci.nodejs.org/job/node-test-pull-request/686/ Only failures are for other flaky tests and build-bot failures.

Trott · 2015-11-07T06:30:30Z

/cc @dnakamura @rvagg @isaacs

evanlucas · 2015-11-08T12:24:04Z

test/sequential/test-http-pipeline-flood.js

I vaguely remember a PR to make all Errors be called with new. May be worthwhile to make it consistent?

@evanlucas Sure, we can do that. .... Done!

bnoordhuis · 2015-11-11T10:41:14Z

test/sequential/test-http-pipeline-flood.js

"the mechanism" or "the flood prevention mechanism"?

bnoordhuis · 2015-11-11T10:47:39Z

I think this test could live in test/parallel but I'd appreciate it if you did the move in a separate commit.

Trott · 2015-11-12T06:19:04Z

OK, made all the changes suggested by @bnoordhuis, rebased, force pushed. PTAL

Trott · 2015-11-12T06:24:08Z

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 <[email protected]>

MylesBorins · 2015-11-17T18:22:04Z

landed in v4.x-staging in 5254fda...9294523

mhdawson · 2015-11-25T15:39:16Z

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 <[email protected]>

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 <[email protected]>

PR-URL: #3636 Reviewed-By: Ben Noordhuis <[email protected]>

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 <[email protected]>

PR-URL: #3636 Reviewed-By: Ben Noordhuis <[email protected]>

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 <[email protected]>

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 <[email protected]>

mhdawson · 2016-03-09T15:36:51Z

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

Trott · 2016-03-09T20:22:18Z

@mhdawson The explanation is in #4730. Ultimately, it's a no-op.

Trott mentioned this pull request Nov 2, 2015
Fix flaky test test-http-pipeline-flood #2862
Closed

mscdex added http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests. labels Nov 3, 2015

Trott force-pushed the pipeflood_fix branch from 23a0aa8 to 456a455Compare November 7, 2015 03:32

Trott changed the title ~~Pipeflood fix~~Fix flakiness of pipeflood testNov 7, 2015

evanlucas reviewed Nov 8, 2015
View reviewed changes

Trott force-pushed the pipeflood_fix branch from 33c581a to 5f3446aCompare November 8, 2015 15:56

This was referenced Nov 8, 2015
tools: enforce throw new Error() with lint rule #3714
Closed
Flaky tests nodejs/build#248
Closed

Trott force-pushed the pipeflood_fix branch from 5f3446a to b5510aeCompare November 11, 2015 06:01

bnoordhuis reviewed Nov 11, 2015
View reviewed changes

test/sequential/test-http-pipeline-flood.js Outdated
Copy link
Member
bnoordhuisNov 11, 2015
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"the mechanism" or "the flood prevention mechanism"?

Devin Nakamuraand others added 2 commits November 11, 2015 20:01

test: fix flaky test test-http-pipeline-flood
8612e36

test: refactor test-http-pipeline-flood
f003c84

Trott force-pushed the pipeflood_fix branch from da95dbf to f003c84Compare November 12, 2015 06:18

MylesBorins added lts-landed-on-v4.x and removed lts-watch-v4.x labels Nov 17, 2015

mhdawson added the lts-watch-v0.12 label Nov 25, 2015

rvagg pushed a commit that referenced this pull request Dec 4, 2015
test: fix flaky test test-http-pipeline-flood
b084554
PR-URL: #3636 Reviewed-By: Ben Noordhuis <[email protected]>

MylesBorins added land-on-v4.x and removed lts-landed-on-v4.x labels Dec 17, 2015

jasnell mentioned this pull request Dec 17, 2015
Release 4.2.4 Planning #4321
Closed

jasnell pushed a commit that referenced this pull request Dec 17, 2015
test: fix flaky test test-http-pipeline-flood
6c23f67
PR-URL: #3636 Reviewed-By: Ben Noordhuis <[email protected]>

jasnell pushed a commit that referenced this pull request Dec 23, 2015
test: fix flaky test test-http-pipeline-flood
52a9422
PR-URL: #3636 Reviewed-By: Ben Noordhuis <[email protected]>

Trott added dont-land-on-v4.x and removed land-on-v4.x labels Jan 18, 2016

Trott mentioned this pull request Jan 18, 2016
Add missing commits to v4.x-staging #4730
Closed

stefanmb mentioned this pull request Apr 28, 2016
test: test-http-pipeline-flood failures on Win32 #6450
Closed

Trott deleted the pipeflood_fix branch January 13, 2022 22:29

AnnAngela mentioned this pull request Dec 21, 2023
ESM loader cannot detect module system properly in folder whose name ends with node_modules#51245
Closed

Uh oh!

Fix flakiness of pipeflood test#3636

Fix flakiness of pipeflood test #3636

Uh oh!

Conversation

Trott commented Nov 2, 2015

Uh oh!

Trott commented Nov 2, 2015

Uh oh!

Trott commented Nov 2, 2015

Uh oh!

Trott commented Nov 3, 2015

Uh oh!

Trott commented Nov 3, 2015

Uh oh!

Trott commented Nov 7, 2015

Uh oh!

Trott commented Nov 7, 2015

Uh oh!

Trott commented Nov 7, 2015

Uh oh!

Trott commented Nov 7, 2015

Uh oh!

Trott commented Nov 7, 2015

Uh oh!

evanlucasNov 8, 2015

Choose a reason for hiding this comment

Uh oh!

TrottNov 8, 2015

Choose a reason for hiding this comment

Uh oh!

bnoordhuisNov 11, 2015

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Nov 11, 2015

Uh oh!

Trott commented Nov 12, 2015

Uh oh!

Trott commented Nov 12, 2015

Uh oh!

MylesBorins commented Nov 17, 2015

Uh oh!

mhdawson commented Nov 25, 2015

Uh oh!

mhdawson commented Mar 9, 2016

Uh oh!

Trott commented Mar 9, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants