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: check error type in net.Server.listen() callback and avoid setTimeout() in test #1821

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented May 28, 2015

This implements a task from a TODO comment.

ref: #264

@mscdex mscdex added the test Issues and PRs related to the tests. label May 28, 2015
});


server1.listen(common.PORT, function() {
console.error('server1 listening');
server1listening = true;

Copy link
Contributor

Choose a reason for hiding this comment

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

could you please remove this whitespace change?

@brendanashworth
Copy link
Contributor

Besides the little whitespace nitpicks, this LGTM. Thanks for the PR 😄 it reduces runtime of this test by about a tenth of a second, too.

@bnoordhuis
Copy link
Member

Not a review but can you reword the commit log so it conforms to the guidelines from CONTRIBUTING.md?

This change eliminates an unnecessary setTimeout() in the test.
@Trott
Copy link
Member Author

Trott commented May 30, 2015

Pushed a new version of the commit that fixes the whitespace goofs identified by @brendanashworth and has a conforming commit message as pointed out by @bnoordhuis

bnoordhuis pushed a commit that referenced this pull request May 30, 2015
This change eliminates an unnecessary setTimeout() in the test.

PR-URL: #1821
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
@bnoordhuis
Copy link
Member

Thanks Rich, landed in 8059393.

@bnoordhuis bnoordhuis closed this May 30, 2015
@rvagg rvagg mentioned this pull request May 31, 2015
andrewdeandrade pushed a commit to andrewdeandrade/node that referenced this pull request Jun 3, 2015
This change eliminates an unnecessary setTimeout() in the test.

PR-URL: nodejs/node#1821
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
@Trott Trott deleted the eaddrinuse branch October 14, 2021 13:47
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.

4 participants