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: update var to let const in tests #9917

Closed
wants to merge 1 commit into from
Closed

test: update var to let const in tests #9917

wants to merge 1 commit into from

Conversation

GreenPioneer
Copy link
Contributor

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 1, 2016
@mscdex mscdex added the tls Issues and PRs related to the tls subsystem. label Dec 1, 2016
@GreenPioneer
Copy link
Contributor Author

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
Copy link
Contributor

cjihrig commented Dec 1, 2016

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

@imyller 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 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 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
lpinca reviewed Dec 5, 2016
View reviewed changes
test/parallel/test-child-process-stdout-flush-exit.js

// spawn self as child
var child = spawn(process.argv[0], [process.argv[1], 'child']);
let child = spawn(process.argv[0], [process.argv[1], 'child']);
Copy link
Member

Choose a reason for hiding this comment

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

Can't this be const?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

lpinca
lpinca reviewed Dec 5, 2016
View reviewed changes
test/parallel/test-crypto-cipher-decipher.js
var common = require('../common');
var assert = require('assert');
const common = require('../common');
const assert = require('assert');
Copy link
Member

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?

@GreenPioneer
Copy link
Contributor Author

@lpinca Updated everything to your points

lpinca
lpinca reviewed Dec 5, 2016
View reviewed changes
test/parallel/test-https-timeout-server.js
var common = require('../common');
var assert = require('assert');
const common = require('../common');
const assert = require('assert');
Copy link
Member

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
lpinca reviewed Dec 5, 2016
View reviewed changes
test/parallel/test-tls-on-empty-socket.js
var common = require('../common');
var assert = require('assert');
const common = require('../common');
const assert = require('assert');
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

@GreenPioneer
Copy link
Contributor Author

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

lpinca commented Dec 5, 2016

@GreenPioneer Awesome, LGTM.
Thanks!

@lpinca
Copy link
Member

lpinca commented Dec 5, 2016

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

jasnell
jasnell approved these changes Dec 5, 2016
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

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

fhinkel
fhinkel approved these changes Dec 6, 2016
View reviewed changes
Copy link
Member

@fhinkel fhinkel left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM with commits squashed.

@GreenPioneer
test: use ES6 to update let & const
2cb7994 
Also updating assertStrict and moving the assert required.
@GreenPioneer
Copy link
Contributor Author

All Squashed

@jasnell
Copy link
Member

jasnell commented Dec 6, 2016

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

jasnell pushed a commit that referenced this pull request Dec 6, 2016
@GreenPioneer @jasnell
test: use ES6 to update let & const
a1bd070 
Also updating assertStrict and moving the assert required.

PR-URL: #9917
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@jasnell
Copy link
Member

jasnell commented Dec 6, 2016

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

@jasnell jasnell closed this Dec 6, 2016
@GreenPioneer
Copy link
Contributor Author

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

addaleax pushed a commit that referenced this pull request Dec 8, 2016
@GreenPioneer @addaleax
test: use ES6 to update let & const
4d66578 
Also updating assertStrict and moving the assert required.

PR-URL: #9917
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
addaleax pushed a commit to addaleax/node that referenced this pull request Dec 8, 2016
@GreenPioneer @addaleax
test: use ES6 to update let & const
9af076e 
Also updating assertStrict and moving the assert required.

PR-URL: nodejs#9917
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@italoacasas italoacasas mentioned this pull request Dec 15, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
@GreenPioneer @MylesBorins
test: use ES6 to update let & const
77c5b36 
Also updating assertStrict and moving the assert required.

PR-URL: #9917
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 21, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

@fhinkel fhinkel fhinkel approved these changes

Assignees
No one assigned
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. test Issues and PRs related to the tests. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@GreenPioneer @cjihrig @lpinca @jasnell @fhinkel @mscdex @MylesBorins @addaleax @imyller @nodejs-github-bot