Skip to content

test: fix http-response-multiheaders - #3958

Closed
santigimeno wants to merge 1 commit into
nodejs:masterfrom
santigimeno:fix_multiheaders
Closed

test: fix http-response-multiheaders#3958
santigimeno wants to merge 1 commit into
nodejs:masterfrom
santigimeno:fix_multiheaders

Conversation

@santigimeno

Copy link
Copy Markdown
Member

Make sure the server is not closed until both responses have been
received.

Make sure the server is not closed until both responses have been
received.
@santigimeno

Copy link
Copy Markdown
Member Author

It tries to fix #2815. The same ECONNREFUSED error was happening to me in OS X

@r-52 r-52 added http Issues and PRs related to the http subsystem. test Issues and PRs related to Node.js core tests and test infrastructure. labels Nov 21, 2015
@Trott

Trott commented Nov 21, 2015

Copy link
Copy Markdown
Member

@Trott

Trott commented Dec 24, 2015

Copy link
Copy Markdown
Member

LGTM

I'm seeing problems with this test on Windows too. https://ci.nodejs.org/job/node-test-binary-windows/384/RUN_SUBSET=0,VS_VERSION=vs2015,label=win2012r2/console

So I'm kind of motivated to get this fix in now.

Stress test without the fix proposed here: https://ci.nodejs.org/job/node-stress-single-test/196/nodes=win2012r2/console

Stress test with the fix proposed here: https://ci.nodejs.org/job/node-stress-single-test/197/nodes=win2012r2/console

@Trott

Trott commented Dec 24, 2015

Copy link
Copy Markdown
Member

LGTM

@jbergstroem

Copy link
Copy Markdown
Member

Stress test looks good. LGTM.

Trott pushed a commit that referenced this pull request Dec 24, 2015
Make sure the server is not closed until both responses have been
received.

PR-URL: #3958
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
@Trott

Trott commented Dec 24, 2015

Copy link
Copy Markdown
Member

Landed in 025e4aa. Thanks once again for all these test fixes you've been working on!

@Trott Trott closed this Dec 24, 2015
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Jan 6, 2016
Make sure the server is not closed until both responses have been
received.

PR-URL: nodejs#3958
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
@MylesBorins

Copy link
Copy Markdown
Contributor

It looks like this makes changes to a test that was added in a semver major.

@rvagg I am currently removing the lts-watch tag and adding the do-not-land tag

Please feel free to update this

scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Make sure the server is not closed until both responses have been
received.

PR-URL: nodejs#3958
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

http Issues and PRs related to the http subsystem. test Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants