Enhance crypto/hash.js coverage#17447
Closed
Uh oh!
There was an error while loading. Please reload this page.
Conversation
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
Leko changed the title 
lpinca approved these changes Dec 4, 2017
Member
lpinca commented Dec 4, 2017
apapirovski approved these changes Dec 4, 2017
Contributor
apapirovski left a comment
apapirovski left a comment There was a problem hiding this comment.
LGTM but two small changes needed before this lands
| { | ||
| constHash=crypto.Hash; | ||
| constinstance=crypto.Hash('sha256'); | ||
| assert(instanceinstanceofHash,'Hash is expected to return a new instance'+ |
Contributor
There was a problem hiding this comment.
needs a space either at the end of this line (after instance) or on the next line. Right now it says "instancewhen".
ContributorAuthor
There was a problem hiding this comment.
Ah, Sorry. I'll update it soon.
| { | ||
| constHmac=crypto.Hmac; | ||
| constinstance=crypto.Hmac('sha256','Node'); | ||
| assert(instanceinstanceofHmac,'Hmac is expected to return a new instance'+ |
Right now it says "instance when".
mhdawson approved these changes Dec 4, 2017
Member
mhdawson left a comment
mhdawson left a comment There was a problem hiding this comment.
LGTM once changes from @apapirovski are incorporated
Contributor
maclover7 commented Dec 4, 2017
Same deal as #17458 (comment), are we able to use the |
maclover7 approved these changes Dec 5, 2017
Contributor
maclover7 left a comment
maclover7 left a comment There was a problem hiding this comment.
Two quick nits, then LGTM
| { | ||
| constHmac=crypto.Hmac; | ||
| constinstance=crypto.Hmac('sha256','Node'); |
| { | ||
| constHash=crypto.Hash; | ||
| constinstance=crypto.Hash('sha256'); |
BridgeAR approved these changes Dec 6, 2017
Member
BridgeAR left a comment
BridgeAR left a comment There was a problem hiding this comment.
LGTM with @apapirovski comments addressed
Contributor
apapirovski commented Dec 6, 2017
Contributor
maclover7 commented Dec 7, 2017
@apapirovski Can you confirm your comments were addressed? |
maclover7 approved these changes Dec 7, 2017
Contributor
apapirovski commented Dec 7, 2017
Yes and I'm currently landing this |
Contributor
apapirovski commented Dec 7, 2017
apapirovski pushed a commit that referenced this pull request Dec 7, 2017
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 <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
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 <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
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 <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added these case
crypto.Hash
newkeywordcrypto.Hmac
newkeywordCurrent coverage is here: https://coverage.nodejs.org/coverage-06e1b0386196f8f8/root/internal/crypto/hash.js.html
I cannot write test case:
this._handle.updatereturns false inHash#verify.Because I don't know how to make
mdctx_tonullptrin Hash::HashUpdate.See also:
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
test