FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Enhance crypto/hash.js coverage by Leko · Pull Request #17447 · nodejs/node · GitHub

/ node Public

Enhance crypto/hash.js coverage - #17447

Closed
Leko wants to merge 7 commits into
nodejs:masterfrom
Leko:add_missing_test_in_crypto_Hash_Hmac
ConversationCommits (7)ChecksFiles changed
Closed

Enhance crypto/hash.js coverage#17447
Leko wants to merge 7 commits into
nodejs:masterfrom
Leko:add_missing_test_in_crypto_Hash_Hmac

Conversation

Leko commented Dec 4, 2017

Copy link
Copy Markdown
Contributor

Added these case

crypto.Hash
  • Call constructor without new keyword
crypto.Hmac
  • Call constructor without new keyword
  • Call constructor with typeof hmac != string
  • Call constructor with typeof hmac = string, typeof key != string

Current coverage is here: https://coverage.nodejs.org/coverage-06e1b0386196f8f8/root/internal/crypto/hash.js.html

I cannot write test case: this._handle.update returns false in Hash#verify.
Because I don't know how to make mdctx_ to nullptr in Hash::HashUpdate.
See also:

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 4, 2017
Leko changed the title Add missing test in crypto hash hmac Enhance crypto/hash.js coverage Dec 4, 2017
mscdex added the crypto Issues and PRs related to the crypto subsystem. label Dec 4, 2017

lpinca commented Dec 4, 2017

Copy link
Copy Markdown
Member

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

apapirovski approved these changes Dec 4, 2017
View reviewed changes

apapirovski left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM but two small changes needed before this lands

Comment thread test/parallel/test-crypto-hash.js
{
const Hash = crypto.Hash;
const instance = crypto.Hash('sha256');
assert(instance instanceof Hash, 'Hash is expected to return a new instance' +

apapirovski Dec 4, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

needs a space either at the end of this line (after instance) or on the next line. Right now it says "instancewhen".

Leko Dec 4, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ah, Sorry. I'll update it soon.

Comment thread test/parallel/test-crypto-hmac.js
{
const Hmac = crypto.Hmac;
const instance = crypto.Hmac('sha256', 'Node');
assert(instance instanceof Hmac, 'Hmac is expected to return a new instance' +

apapirovski Dec 4, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Same as above.

test: add space after new line
380a158 
Right now it says "instance when".
mhdawson approved these changes Dec 4, 2017
View reviewed changes

mhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM once changes from @apapirovski are incorporated

maclover7 commented Dec 4, 2017

Copy link
Copy Markdown
Contributor

Same deal as #17458 (comment), are we able to use the create functions here?

maclover7 approved these changes Dec 5, 2017
View reviewed changes

maclover7 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Two quick nits, then LGTM

Comment thread test/parallel/test-crypto-hmac.js

{
const Hmac = crypto.Hmac;
const instance = crypto.Hmac('sha256', 'Node');

maclover7 Dec 5, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

createHmac

Comment thread test/parallel/test-crypto-hash.js

{
const Hash = crypto.Hash;
const instance = crypto.Hash('sha256');

maclover7 Dec 5, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

createHash

Leko Dec 5, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Same adobe #17458 (comment)

BridgeAR approved these changes Dec 6, 2017
View reviewed changes

BridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM with @apapirovski comments addressed

apapirovski commented Dec 6, 2017

Copy link
Copy Markdown
Contributor

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

maclover7 commented Dec 7, 2017

Copy link
Copy Markdown
Contributor

@apapirovski Can you confirm your comments were addressed?

apapirovski commented Dec 7, 2017

Copy link
Copy Markdown
Contributor

Yes and I'm currently landing this

apapirovski commented Dec 7, 2017

Copy link
Copy Markdown
Contributor

Landed in 3d64533

Thanks @Leko! 👍

apapirovski closed this Dec 7, 2017
apapirovski pushed a commit that referenced this pull request Dec 7, 2017
test: expand coverage for crypto
3d64533 
crypto.Hash
- Call constructor without new keyword

crypto.Hmac
- Call constructor without new keyword
- Call constructor with typeof hmac != string
- Call constructor with typeof hmac = string, typeof key != string

PR-URL: #17447
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
test: expand coverage for crypto
e50ab91 
crypto.Hash
- Call constructor without new keyword

crypto.Hmac
- Call constructor without new keyword
- Call constructor with typeof hmac != string
- Call constructor with typeof hmac = string, typeof key != string

PR-URL: #17447
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
test: expand coverage for crypto
a407a48 
crypto.Hash
- Call constructor without new keyword

crypto.Hmac
- Call constructor without new keyword
- Call constructor with typeof hmac != string
- Call constructor with typeof hmac = string, typeof key != string

PR-URL: #17447
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins mentioned this pull request Dec 12, 2017
Merged
Leko mentioned this pull request Dec 18, 2017
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

lpinca lpinca approved these changes
BridgeAR BridgeAR approved these changes
+3 more reviewers
apapirovski apapirovski approved these changes
maclover7 maclover7 approved these changes
mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

crypto Issues and PRs related to the crypto subsystem. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

Back | FazBrowse Home | New Git URL