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: refactor test-net-server-bind #13273

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented May 29, 2017

  • Use common.mustNotCall() and common.mustCall() as appropriate
  • Use block scoping
  • Move assertions out of exit handler and into callbacks
  • Order assert.strictEqual() args per docs
  • Remove console.log() calls
  • Move test from parallel to sequential so common.PORT can be used
    without conflicting with OS-provided ports in other parallel tests
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test net

@Trott Trott added net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests. labels May 29, 2017
{
const server = net.createServer(common.mustNotCall());

server.listen(common.PORT + 1, function() {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I'd use common.mustCall() here and below.

Copy link
Member Author

Choose a reason for hiding this comment

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

@lpinca Done! Thanks!

@Trott Trott force-pushed the test-net-server-bind branch from 82ec4d3 to fd49d14 Compare May 31, 2017 17:26
{
const server = net.createServer(common.mustNotCall());

server.listen(common.mustCall(common.mustCall(function() {
Copy link
Member

@lpinca lpinca May 31, 2017

Choose a reason for hiding this comment

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

Nit: there is common.mustCall() inception now :) One can be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Embarrassingly, that wasn't even a copy/paste error.

Fixed.

* Use common.mustNotCall() and common.mustCall() as appropriate
* Use block scoping
* Move assertions out of `exit` handler and into callbacks
* Order assert.strictEqual() args per docs
* Remove console.log() calls
* Move test from `parallel` to `sequential` so `common.PORT` can be used
  without conflicting with OS-provided ports in other `parallel` tests
@Trott Trott force-pushed the test-net-server-bind branch from fd49d14 to 5221c22 Compare May 31, 2017 18:02
@Trott
Copy link
Member Author

Trott commented May 31, 2017

Trott added a commit to Trott/io.js that referenced this pull request Jun 1, 2017
* Use common.mustNotCall() and common.mustCall() as appropriate
* Use block scoping
* Move assertions out of `exit` handler and into callbacks
* Order assert.strictEqual() args per docs
* Remove console.log() calls
* Move test from `parallel` to `sequential` so `common.PORT` can be used
  without conflicting with OS-provided ports in other `parallel` tests

PR-URL: nodejs#13273
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@Trott
Copy link
Member Author

Trott commented Jun 1, 2017

Landed in e824c75

@Trott Trott closed this Jun 1, 2017
jasnell pushed a commit that referenced this pull request Jun 5, 2017
* Use common.mustNotCall() and common.mustCall() as appropriate
* Use block scoping
* Move assertions out of `exit` handler and into callbacks
* Order assert.strictEqual() args per docs
* Remove console.log() calls
* Move test from `parallel` to `sequential` so `common.PORT` can be used
  without conflicting with OS-provided ports in other `parallel` tests

PR-URL: #13273
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@jasnell jasnell mentioned this pull request Jun 5, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
MylesBorins pushed a commit that referenced this pull request Aug 14, 2017
* Use common.mustNotCall() and common.mustCall() as appropriate
* Use block scoping
* Move assertions out of `exit` handler and into callbacks
* Order assert.strictEqual() args per docs
* Remove console.log() calls
* Move test from `parallel` to `sequential` so `common.PORT` can be used
  without conflicting with OS-provided ports in other `parallel` tests

PR-URL: #13273
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
* Use common.mustNotCall() and common.mustCall() as appropriate
* Use block scoping
* Move assertions out of `exit` handler and into callbacks
* Order assert.strictEqual() args per docs
* Remove console.log() calls
* Move test from `parallel` to `sequential` so `common.PORT` can be used
  without conflicting with OS-provided ports in other `parallel` tests

PR-URL: #13273
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants