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,net: add tests for server.connections #10762

Closed
wants to merge 2 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jan 12, 2017

There were no tests confirming situations where server.connections
should return null. Add a test for that situation.

Expand existing server.connection test slightly to check value.

Refactor (mostly spacing) code for server.connections setter.

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 net

There were no tests confirming situations where server.connections
should return `null`. Add a test for that situation.

Expand existing server.connection test slightly to check value.

Refactor (mostly spacing) code for server.connections setter.
@Trott Trott added net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests. labels Jan 12, 2017
@nodejs-github-bot nodejs-github-bot added net Issues and PRs related to the net subsystem. dont-land-on-v7.x labels Jan 12, 2017
assert.strictEqual(Object.keys(server).indexOf('connections'), -1);

assert.strictEqual(server.connections, 0);
Copy link
Member

@jasnell jasnell Jan 12, 2017

Choose a reason for hiding this comment

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

You might also consider adding a check for the deprecation warning... e.g.

common.expectWarning('DeprecationWarning',
                     'Server.connections property is deprecated.');

@Trott
Copy link
Member Author

Trott commented Jan 13, 2017

Trott added a commit to Trott/io.js that referenced this pull request Jan 15, 2017
There were no tests confirming situations where server.connections
should return `null`. Add a test for that situation.

Expand existing server.connection test slightly to check value.

Refactor (mostly spacing) code for server.connections setter.

PR-URL: nodejs#10762
Reviewed-By: James M Snell <[email protected]>
@Trott
Copy link
Member Author

Trott commented Jan 15, 2017

Landed in 97f001a

@Trott Trott closed this Jan 15, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
There were no tests confirming situations where server.connections
should return `null`. Add a test for that situation.

Expand existing server.connection test slightly to check value.

Refactor (mostly spacing) code for server.connections setter.

PR-URL: nodejs#10762
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Jan 28, 2017
There were no tests confirming situations where server.connections
should return `null`. Add a test for that situation.

Expand existing server.connection test slightly to check value.

Refactor (mostly spacing) code for server.connections setter.

PR-URL: #10762
Reviewed-By: James M Snell <[email protected]>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
There were no tests confirming situations where server.connections
should return `null`. Add a test for that situation.

Expand existing server.connection test slightly to check value.

Refactor (mostly spacing) code for server.connections setter.

PR-URL: nodejs#10762
Reviewed-By: James M Snell <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
There were no tests confirming situations where server.connections
should return `null`. Add a test for that situation.

Expand existing server.connection test slightly to check value.

Refactor (mostly spacing) code for server.connections setter.

PR-URL: nodejs#10762
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Mar 8, 2017

A backport PR would be required in order for this to land in v6 or v4

@Trott Trott deleted the connections 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
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.

4 participants