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

test: fixup test-http-hostname-typechecking #12627

Closed
wants to merge 1 commit into from

Conversation

@addaleax
Copy link
Member

commented Apr 24, 2017

This test would currently create HTTP requests to localhost:80
and would time out on machines that actually had an server listening
there.

To address that, end() the requests that are generated.

/cc @jasnell

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@addaleax

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2017

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

I’d be very okay with fast-tracking to unbreak master.

@addaleax addaleax force-pushed the addaleax:fix-http-master branch Apr 24, 2017

@addaleax addaleax added the http label Apr 24, 2017

test/parallel/test-http-hostname-typechecking.js Outdated
// These values are OK and should not throw synchronously
['', undefined, null].forEach((v) => {
assert.doesNotThrow(() => {
http.request({hostname: v}).on('error', common.noop);
http.request({host: v}).on('error', common.noop);
reqs.push(http.request({hostname: v}).on('error', common.noop));

This comment has been minimized.

Copy link
@lpinca

lpinca Apr 25, 2017

Member

Nit: any reason for preferring this over a simple .end() on the original version?

This comment has been minimized.

Copy link
@addaleax

addaleax Apr 25, 2017

Author Member

@lpinca See @jasnell’s comment in #12494 (comment) … I tried to keep this bit of the test just testing what it’s supposed to. Personally, I would totally be fine with having .end() here.

This comment has been minimized.

Copy link
@lpinca

lpinca Apr 25, 2017

Member

@addaleax not sure I understand.

http.request({hostname: v}).on('error', common.noop).end();

would still work as intended no? The error event would still be emitted. It should only check that it doesn't throw synchronously.

This comment has been minimized.

Copy link
@addaleax

addaleax Apr 25, 2017

Author Member

@lpinca Yea, like I said, I’m perfectly fine with that. :) I’ve updated it, let’s see whether @jasnell complains. ;)

@danbev
danbev approved these changes Apr 25, 2017

@addaleax addaleax force-pushed the addaleax:fix-http-master branch Apr 25, 2017

test: fixup test-http-hostname-typechecking
This test would currently create HTTP requests to localhost:80
and would time out on machines that actually had an server listening
there.

To address that, `end()` the requests that are generated.

PR-URL: #12627
Ref: #12494

@addaleax addaleax force-pushed the addaleax:fix-http-master branch to 0229581 Apr 25, 2017

@lpinca
lpinca approved these changes Apr 25, 2017
@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 25, 2017

Lgtm

@addaleax

This comment has been minimized.

Copy link
Member Author

commented Apr 27, 2017

Landed in 28f535a

@addaleax addaleax closed this Apr 27, 2017

@addaleax addaleax deleted the addaleax:fix-http-master branch Apr 27, 2017

addaleax added a commit that referenced this pull request Apr 27, 2017
test: fixup test-http-hostname-typechecking
This test would currently create HTTP requests to localhost:80
and would time out on machines that actually had an server listening
there.

To address that, `end()` the requests that are generated.

PR-URL: #12627
Ref: #12494
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell jasnell referenced this pull request May 11, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
@gibfahn

This comment has been minimized.

Copy link
Member

commented Jun 18, 2017

Depends on #12494 which is major.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.