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: change var to const/let - change assert.equal to assert.strictE… #10018

Conversation

aileenvl
Copy link

@aileenvl aileenvl commented Dec 1, 2016

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

Description of change

…qual

Make change in test file per issue received in NINA 2016 involving the change from var to const/let and the change of assert.equal to assert.strictEqual

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 1, 2016
@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
@mscdex mscdex added the crypto Issues and PRs related to the crypto subsystem. label Dec 1, 2016
@mscdex
Copy link
Contributor

mscdex commented Dec 1, 2016

Please see the commit message guidelines here.

@Trott
Copy link
Member

Trott commented Dec 4, 2016

@Trott
Copy link
Member

Trott commented Dec 4, 2016

Thanks for opening a pull request. The make -j8 test box is checked, but this branch fails linting, so I don't think it was actually run.. Can you run it and fix any issues that arise?

@Trott
Copy link
Member

Trott commented Dec 4, 2016

Whoops! Ignore the previous comment. It looks like the lint failure in CI is an infrastructure glitch and not a lint problem. Sorry about that.


function testCipher1(key, iv) {
// Test encyrption and decryption with explicit key and iv
var plaintext =
let plaintext =
Copy link
Member

Choose a reason for hiding this comment

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

It seems like this could be const.

'32|RmVZZkFUVmpRRkp0TmJaUm56ZU9qcnJkaXNNWVNpTTU*|iXmckfRWZBGWWELw' +
'eCBsThSsfUHLeRe0KCsK8ooHgxie0zOINpXxfZi/oNG7uq9JWFVCk70gfzQH8ZUJ' +
'jAfaFg**';
var cipher = crypto.createCipheriv('des-ede3-cbc', key, iv);
var ciph = cipher.update(plaintext, 'utf8', 'hex');
let cipher = crypto.createCipheriv('des-ede3-cbc', key, iv);
Copy link
Member

Choose a reason for hiding this comment

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

Also could be const, it would seem.

@Trott
Copy link
Member

Trott commented Dec 4, 2016

Actually, the lint failures were real after all. The assert.strictEqual() lines were longer than 80 chars. I'll fix that as I rebase to resolve a conflict with another PR that just landed. I'll also use const where let isn't necessary.

@Trott Trott force-pushed the issue-test-crypto-cipheriv-decipheriv_aileen branch from 6be766a to 8f46bcc Compare December 4, 2016 06:44
@Trott
Copy link
Member

Trott commented Dec 4, 2016

(Those let instances that could be const are also called out by the linter.)

Make change in test file from var to const/let.
@Trott Trott force-pushed the issue-test-crypto-cipheriv-decipheriv_aileen branch from 8f46bcc to 0b8800d Compare December 4, 2016 06:49
@Trott
Copy link
Member

Trott commented Dec 4, 2016

Pushed changes. CI: https://ci.nodejs.org/job/node-test-pull-request/5169/

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM if CI is ✅

@Trott
Copy link
Member

Trott commented Dec 4, 2016

CI is good except for one unrelated AIX build failure.

Trott pushed a commit to Trott/io.js that referenced this pull request Dec 4, 2016
Make change in test file from var to const/let.

PR-URL: nodejs#10018
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@Trott
Copy link
Member

Trott commented Dec 4, 2016

Landed in 07ef682. Thanks for the contribution! 🎉

@Trott Trott closed this Dec 4, 2016
addaleax pushed a commit that referenced this pull request Dec 5, 2016
Make change in test file from var to const/let.

PR-URL: #10018
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@Fishrock123 Fishrock123 mentioned this pull request Dec 5, 2016
2 tasks
addaleax pushed a commit to addaleax/node that referenced this pull request Dec 8, 2016
Make change in test file from var to const/let.

PR-URL: nodejs#10018
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jmdarling pushed a commit to jmdarling/node that referenced this pull request Dec 8, 2016
Make change in test file from var to const/let.

PR-URL: nodejs#10018
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 20, 2016
Make change in test file from var to const/let.

PR-URL: #10018
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
Make change in test file from var to const/let.

PR-URL: #10018
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
Make change in test file from var to const/let.

PR-URL: #10018
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This was referenced Dec 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. crypto Issues and PRs related to the crypto subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants