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: restore no-op function in test #14065

Closed
wants to merge 1 commit into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Jul 3, 2017

Remove common.mustCall() in test that might connect to a server already
running on the local host.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test http

Remove common.mustCall() in test that might connect to a server already
running on the local host.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jul 3, 2017
@Trott
Copy link
Member Author

Trott commented Jul 3, 2017

R=@addaleax

@Trott
Copy link
Member Author

Trott commented Jul 3, 2017

OK to fast-track this one as trivial and important to fix tests?

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

OK to fast-track this one as trivial and important to fix tests?

I would say so – I can’t really tell how many other people have a server on port 80 running regularly on the machine on which they run tests.

@Trott
Copy link
Member Author

Trott commented Jul 3, 2017

@@ -21,7 +21,7 @@ vals.forEach((v) => {
// These values are OK and should not throw synchronously
['', undefined, null].forEach((v) => {
assert.doesNotThrow(() => {
http.request({hostname: v}).on('error', common.mustCall()).end();
http.request({host: v}).on('error', common.mustCall()).end();
http.request({hostname: v}).on('error', () => {}).end();
Copy link
Contributor

@refack refack Jul 3, 2017

Choose a reason for hiding this comment

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

Maybe add a comment that we don't care about the connection just about hostname type checking?
Maybe even go as far as:

const dontCare = () => {};
http.request({hostname: v}).on('error', dontCare).end();
http.request({host: v}).on('error', dontCare).end();

Just so no one revert-reverts this

@Trott
Copy link
Member Author

Trott commented Jul 4, 2017

Landed in 5b1d12a

@Trott Trott closed this Jul 4, 2017
Trott added a commit to Trott/io.js that referenced this pull request Jul 4, 2017
Remove common.mustCall() in test that might connect to a server already
running on the local host.

PR-URL: nodejs#14065
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 11, 2017
Remove common.mustCall() in test that might connect to a server already
running on the local host.

PR-URL: #14065
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@addaleax addaleax mentioned this pull request Jul 11, 2017
addaleax pushed a commit that referenced this pull request Jul 18, 2017
Remove common.mustCall() in test that might connect to a server already
running on the local host.

PR-URL: #14065
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
Remove common.mustCall() in test that might connect to a server already
running on the local host.

PR-URL: #14065
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@Trott Trott deleted the restore-noop branch January 13, 2022 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants