test: update var to let const in tests by GreenPioneer · Pull Request #9917 · nodejs/node

GreenPioneer · 2016-12-01T16:28:55Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

Test - test/parallel/test-tis-on-empty-socket

Description of change

Updated the var to const and lets in test-tis-on-empty-socket. This is coming from node interactive - code & learn

GreenPioneer · 2016-12-01T16:33:29Z

In the task i was asked to also change process.on('exit') to use common.mustCall but after looking into it with @jasnell @Fishrock123 .We determined that it has problems working with the exit and in the end we decided against it.

cjihrig · 2016-12-01T16:36:46Z

That's correct. process.on('exit', ...) doesn't need a common.mustCall(). LGTM.

lpinca · 2016-12-05T08:11:16Z

test/parallel/test-child-process-stdout-flush-exit.js

Can't this be const?

Yea it can & it should be . I ll Fix that now

lpinca · 2016-12-05T08:12:34Z

test/parallel/test-crypto-cipher-decipher.js

Nit: would you mind moving this require after the hasCrypto check?

GreenPioneer · 2016-12-05T08:35:08Z

@lpinca Updated everything to your points

lpinca · 2016-12-05T08:36:28Z

test/parallel/test-https-timeout-server.js

@GreenPioneer sorry to pester, this can also be moved after the hasCrypto check.

lpinca · 2016-12-05T08:36:40Z

test/parallel/test-tls-on-empty-socket.js

GreenPioneer · 2016-12-05T15:00:56Z

Hey @lpinca its all good, Id rather do it right. Let me know if there is anything else you want done on these files.

lpinca · 2016-12-05T15:04:31Z

@GreenPioneer Awesome, LGTM.
Thanks!

lpinca · 2016-12-05T15:05:59Z

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

jasnell

LGTM. @GreenPioneer, if you would, please squash the commits down to either a single commit or one commit per file changed. Thank you!

fhinkel

Thanks! LGTM with commits squashed.

Also updating assertStrict and moving the assert required.

GreenPioneer · 2016-12-06T15:28:34Z

All Squashed

jasnell · 2016-12-06T15:33:23Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/5253/

Also updating assertStrict and moving the assert required. PR-URL: #9917 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

jasnell · 2016-12-06T21:07:07Z

Landed in a1bd070. Thank you for the PR and for participating in the code-and-learn!

GreenPioneer · 2016-12-06T21:17:03Z

@Fishrock123 & @jasnell thank you for helping me out at the event. Really enjoyed it !!!

Also updating assertStrict and moving the assert required. PR-URL: #9917 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

Also updating assertStrict and moving the assert required. PR-URL: nodejs#9917 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

Also updating assertStrict and moving the assert required. PR-URL: #9917 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 1, 2016

mscdex added the tls Issues and PRs related to the tls subsystem. label Dec 1, 2016

cjihrig approved these changes Dec 1, 2016

View reviewed changes

imyller added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Dec 1, 2016

GreenPioneer changed the title ~~test: update var to let const in test-tis-on-empty-socket~~ test: update var to let const in test-tis-on-empty-socket and test-child-process-stdout-flush-exit Dec 1, 2016

GreenPioneer changed the title ~~test: update var to let const in test-tis-on-empty-socket and test-child-process-stdout-flush-exit~~ test: update var to let const in tests Dec 1, 2016

lpinca reviewed Dec 5, 2016

View reviewed changes

test/parallel/test-crypto-cipher-decipher.js Outdated

Copy link

Member

lpinca Dec 5, 2016

Choose a reason for hiding this comment

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

Nit: would you mind moving this require after the hasCrypto check?

lpinca reviewed Dec 5, 2016

View reviewed changes

test/parallel/test-https-timeout-server.js Outdated

Copy link

Member

lpinca Dec 5, 2016

Choose a reason for hiding this comment

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

@GreenPioneer sorry to pester, this can also be moved after the hasCrypto check.

lpinca reviewed Dec 5, 2016

View reviewed changes

test/parallel/test-tls-on-empty-socket.js Outdated

Copy link

Member

lpinca Dec 5, 2016

Choose a reason for hiding this comment

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

Ditto.

lpinca approved these changes Dec 5, 2016

View reviewed changes

jasnell approved these changes Dec 5, 2016

View reviewed changes

fhinkel approved these changes Dec 6, 2016

View reviewed changes

test: use ES6 to update let & const

2cb7994

Also updating assertStrict and moving the assert required.

GreenPioneer force-pushed the master branch from 7cebb4b to 2cb7994 Compare December 6, 2016 15:28

jasnell closed this Dec 6, 2016

addaleax added the land-on-v6.x label Dec 8, 2016

italoacasas mentioned this pull request Dec 15, 2016

v7.3.0 proposal #10277

Merged

MylesBorins added the dont-land-on-v4.x label Dec 20, 2016

MylesBorins mentioned this pull request Dec 21, 2016

V6.9.3 proposal #10394

Merged

Uh oh!

Conversation

GreenPioneer commented Dec 1, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

GreenPioneer commented Dec 1, 2016

Uh oh!

cjihrig commented Dec 1, 2016

Uh oh!

lpinca Dec 5, 2016

Choose a reason for hiding this comment

Uh oh!

GreenPioneer Dec 5, 2016

Choose a reason for hiding this comment

Uh oh!

lpinca Dec 5, 2016

Choose a reason for hiding this comment

Uh oh!

GreenPioneer commented Dec 5, 2016

Uh oh!

lpinca Dec 5, 2016

Choose a reason for hiding this comment

Uh oh!

lpinca Dec 5, 2016

Choose a reason for hiding this comment

Uh oh!

GreenPioneer commented Dec 5, 2016

Uh oh!

lpinca commented Dec 5, 2016

Uh oh!

lpinca commented Dec 5, 2016

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

fhinkel left a comment

Choose a reason for hiding this comment

Uh oh!

GreenPioneer commented Dec 6, 2016

Uh oh!

jasnell commented Dec 6, 2016

Uh oh!

jasnell commented Dec 6, 2016

Uh oh!

GreenPioneer commented Dec 6, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants