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

Fix flakiness of pipeflood test #3636

Closed
wants to merge 2 commits into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Nov 2, 2015

Continuation of #2862

@Trott
Copy link
Member Author

Trott commented Nov 2, 2015

@Trott
Copy link
Member Author

Trott commented Nov 2, 2015

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

@mscdex mscdex added http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests. labels Nov 3, 2015
@Trott
Copy link
Member Author

Trott commented Nov 3, 2015

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
Copy link
Member Author

Trott commented Nov 3, 2015

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

@Trott
Copy link
Member Author

Trott commented Nov 7, 2015

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
Copy link
Member Author

Trott commented Nov 7, 2015

@Trott
Copy link
Member Author

Trott commented Nov 7, 2015

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
Copy link
Member Author

Trott commented Nov 7, 2015

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 Trott changed the title Pipeflood fix Fix flakiness of pipeflood test Nov 7, 2015
@Trott
Copy link
Member Author

Trott commented Nov 7, 2015

/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]}`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.
Copy link
Member

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

@bnoordhuis
Copy link
Member

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

@Trott
Copy link
Member Author

Trott commented Nov 12, 2015

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

@Trott
Copy link
Member Author

Trott commented Nov 12, 2015

CI: https://ci.nodejs.org/job/node-test-pull-request/708/ Hope it doesn't have any surprises...

Trott added a commit that referenced this pull request Nov 17, 2015
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>
@MylesBorins
Copy link
Contributor

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

@mhdawson
Copy link
Member

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

rvagg pushed a commit that referenced this pull request Dec 4, 2015
PR-URL: #3636
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Trott added a commit that referenced this pull request Dec 4, 2015
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>
@jasnell jasnell mentioned this pull request Dec 17, 2015
jasnell pushed a commit that referenced this pull request Dec 17, 2015
PR-URL: #3636
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Trott added a commit that referenced this pull request Dec 17, 2015
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>
jasnell pushed a commit that referenced this pull request Dec 23, 2015
PR-URL: #3636
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Trott added a commit that referenced this pull request Dec 23, 2015
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>
Trott added a commit to Trott/io.js that referenced this pull request Jan 17, 2016
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>
@mhdawson
Copy link
Member

mhdawson commented Mar 9, 2016

@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
Copy link
Member Author

Trott commented Mar 9, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants