Skip to content

test: clean up net server try ports test#8458

Closed
tlhunter wants to merge 1 commit intonodejs:masterfrom
tlhunter:test-net-server-try-ports-cleanup
Closed

test: clean up net server try ports test#8458
tlhunter wants to merge 1 commit intonodejs:masterfrom
tlhunter:test-net-server-try-ports-cleanup

Conversation

@tlhunter
Copy link
Copy Markdown
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
Copy Markdown
Contributor

MylesBorins commented Sep 9, 2016

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

@MylesBorins
Copy link
Copy Markdown
Contributor

MylesBorins commented Sep 9, 2016

LGTM if green

mscdex
mscdex reviewed Sep 9, 2016
View reviewed changes
Comment thread test/parallel/test-net-server-try-ports.js Outdated
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

tlhunter commented Sep 9, 2016

@mscdex thanks for the advice

@lpinca
Copy link
Copy Markdown
Member

lpinca commented Sep 9, 2016

LGTM

@mscdex
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

mscdex commented Sep 9, 2016

@tlhunter Yeah that can be ignored for this PR

santigimeno
santigimeno reviewed Sep 9, 2016
View reviewed changes
Comment thread test/parallel/test-net-server-try-ports.js Outdated
Copy link
Copy Markdown
Member

@santigimeno santigimeno Sep 9, 2016

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
Copy Markdown
Member

santigimeno commented Sep 9, 2016

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
Copy Markdown
Contributor Author

tlhunter commented Sep 9, 2016

@santigimeno console.error calls have been removed

@santigimeno
Copy link
Copy Markdown
Member

santigimeno commented Sep 9, 2016

LGTM. Thanks!

@Trott
Copy link
Copy Markdown
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
Copy Markdown
Member

Trott commented Sep 10, 2016

CI errors are unrelated. CI looks good.

@jasnell
Copy link
Copy Markdown
Member

jasnell commented Sep 12, 2016

LGTM

@Trott
Copy link
Copy Markdown
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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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