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.

Sign up for GitHub

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

Jump to bottom

test: clean up net server try ports test #8458

Closed
wants to merge 1 commit into from
Closed

test: clean up net server try ports test #8458

wants to merge 1 commit into from

Conversation

tlhunter
Copy link
Contributor

@tlhunter tlhunter commented Sep 9, 2016 •
edited
Loading

Checklist
  • make -j4 test (UNIX)
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change
  • Replace var's with const and let
  • Replace boolean flags with common.mustCall()
  • Using stricter comparisons
  • Fixed typo in comment

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 9, 2016
@MylesBorins
Copy link
Contributor

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

@MylesBorins
Copy link
Contributor

LGTM if green

mscdex
mscdex reviewed Sep 9, 2016
View reviewed changes
test/parallel/test-net-server-try-ports.js
socket.destroy();
});

var server2errors = 0;
let server2errors = 0;

server2.on('error', function(e) {
server2errors++;
Copy link
Contributor

@mscdex mscdex Sep 9, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Why not get rid of this variable too and wrap the 'error' event handler in a common.mustCall() instead? That will ensure it gets called exactly once.

@tlhunter tlhunter force-pushed the test-net-server-try-ports-cleanup branch from e91f6c5 to d2af1ef Compare September 9, 2016 16:13
@tlhunter
Copy link
Contributor Author

tlhunter commented Sep 9, 2016

@mscdex thanks for the advice

@lpinca
Copy link
Member

lpinca commented Sep 9, 2016

LGTM

@mscdex
Copy link
Contributor

mscdex commented Sep 9, 2016

LGTM if CI is still ok with it: https://ci.nodejs.org/job/node-test-pull-request/3988/

@tlhunter
Copy link
Contributor Author

tlhunter commented Sep 9, 2016

Looks like an unrelated failure ^

not ok 1208 sequential/test-crypto-timing-safe-equal
# 
# assert.js:85
#   throw new assert.AssertionError({
#   ^
# AssertionError: timingSafeEqual should not leak information from its execution time (t=8.734740624863479)
#     at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux/nodes/debian8-x86/test/sequential/test-crypto-timing-safe-equal.js:153:1)
#     at Module._compile (module.js:560:32)
#     at Object.Module._extensions..js (module.js:569:10)
#     at Module.load (module.js:477:32)
#     at tryModuleLoad (module.js:436:12)
#     at Function.Module._load (module.js:428:3)
#     at Module.runMain (module.js:594:10)
#     at run (bootstrap_node.js:382:7)
#     at startup (bootstrap_node.js:137:9)
#     at bootstrap_node.js:497:3
  ---

@mscdex
Copy link
Contributor

mscdex commented Sep 9, 2016

@tlhunter Yeah that can be ignored for this PR

santigimeno
santigimeno reviewed Sep 9, 2016
View reviewed changes
test/parallel/test-net-server-try-ports.js

server1.listen(0, function() {
server1.listen(0, common.mustCall(function() {
console.error('server1 listening');
Copy link
Member

Choose a reason for hiding this comment

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

I think removing both console.error could be a good idea.

@santigimeno
Copy link
Member

LGTM with one suggestion

@tlhunter tlhunter force-pushed the test-net-server-try-ports-cleanup branch from d2af1ef to a75f5c1 Compare September 9, 2016 20:20
@tlhunter
test: clean up net server try ports test
7449831 
* Replace var's with const and let
* Replace boolean flags with common.mustCall()
* Using stricter comparisons
* Fixed typo in comment
@tlhunter tlhunter force-pushed the test-net-server-try-ports-cleanup branch from a75f5c1 to 7449831 Compare September 9, 2016 20:22
@tlhunter
Copy link
Contributor Author

tlhunter commented Sep 9, 2016

@santigimeno console.error calls have been removed

@santigimeno
Copy link
Member

LGTM. Thanks!

@Trott
Copy link
Member

Trott commented Sep 9, 2016

One last CI because you never know: https://ci.nodejs.org/job/node-test-pull-request/3993/

@Trott
Copy link
Member

Trott commented Sep 10, 2016

CI errors are unrelated. CI looks good.

@jasnell
Copy link
Member

jasnell commented Sep 12, 2016

LGTM

@Trott
Copy link
Member

Trott commented Sep 12, 2016

Landed in 2d7fa3d. 🎉

@Trott Trott closed this Sep 12, 2016
Trott pushed a commit that referenced this pull request Sep 12, 2016
@tlhunter @Trott
test: clean up net server try ports test
2d7fa3d 
* Replace var's with const and let
* Replace boolean flags with common.mustCall()
* Using stricter comparisons
* Fixed typo in comment

PR-URL: #8458
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@tlhunter tlhunter deleted the test-net-server-try-ports-cleanup branch September 12, 2016 05:04
Fishrock123 pushed a commit that referenced this pull request Sep 14, 2016
@tlhunter @Fishrock123
test: clean up net server try ports test
5baa4e0 
* Replace var's with const and let
* Replace boolean flags with common.mustCall()
* Using stricter comparisons
* Fixed typo in comment

PR-URL: #8458
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 30, 2016
@tlhunter
test: clean up net server try ports test
0f2b65c 
* Replace var's with const and let
* Replace boolean flags with common.mustCall()
* Using stricter comparisons
* Fixed typo in comment

PR-URL: #8458
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 10, 2016
@tlhunter
test: clean up net server try ports test
9e8d1c7 
* Replace var's with const and let
* Replace boolean flags with common.mustCall()
* Using stricter comparisons
* Fixed typo in comment

PR-URL: #8458
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this pull request Oct 18, 2016
@tlhunter @rvagg
test: clean up net server try ports test
b9465d0 
* Replace var's with const and let
* Replace boolean flags with common.mustCall()
* Using stricter comparisons
* Fixed typo in comment

PR-URL: #8458
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
@tlhunter @MylesBorins
test: clean up net server try ports test
6d03170 
* Replace var's with const and let
* Replace boolean flags with common.mustCall()
* Using stricter comparisons
* Fixed typo in comment

PR-URL: #8458
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Oct 26, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@tlhunter @MylesBorins @lpinca @mscdex @santigimeno @Trott @jasnell @nodejs-github-bot