test: add test for http outgoing internal headers#13980
Uh oh!
There was an error while loading. Please reload this page.
Conversation
There was a problem hiding this comment.
Perhaps just:
const{ OutgoingMessage }=require('http');
Trott commented Jun 30, 2017
The first word after the This can be fixed by whoever lands the pull request, but if you do it for them, it will save them a few keystrokes. See commit message guidelines at https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#commit-message-guidelines. (You probably saw them and were trying to follow them but there's a lot of stuff there so it's easy to miss a small thing here or there.) |
gergelyke commented Jul 1, 2017
gergelyke changed the title gergelyke commented Jul 4, 2017
is there anything else I should fix? |
Trott commented Jul 4, 2017
@nodejs/testing @nodejs/http |
cjihrig left a comment
cjihrig left a comment There was a problem hiding this comment.
My only comment would be to separate the tests using block scopes. It prevents the tests from interacting with each other and eliminates numbers on the end of the variable names (outgoingMessage2).
gergelyke commented Jul 17, 2017
should be fixed @cjihrig |
There was a problem hiding this comment.
For consistency...
const{ outHeadersKey }=require('internal/http');
There was a problem hiding this comment.
lint: file needs to end with \n
refack commented Jul 17, 2017
jasnell commented Jul 17, 2017
Looks like this needs a |
gergelyke commented Jul 18, 2017
should be all fixed :) |
refack commented Jul 18, 2017
gergelyke commented Jul 18, 2017
@refack is it me who should fi things, or the CI has some issues? |
refack commented Jul 18, 2017
@gergelyke CI had issues. AFAICT the jobs that did finish were green. |
PR-URL: nodejs#13980 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
refack commented Jul 18, 2017
Landed in f406a7e |
refack commented Jul 18, 2017
Extra sanity test on |
PR-URL: #13980 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
PR-URL: #13980 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
8 participants
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
http