Enhance crypto/sig.js coverage#17426
Enhance crypto/sig.js coverage#17426Leko wants to merge 9 commits intonodejs:masterfrom Leko:enhance_crypto_sig_coverage
Conversation
| { | ||
| const Verify = crypto.Verify; | ||
| const instance = Verify('SHA256'); | ||
| assert.ok(instance instanceof Verify, 'call sign constructor without new'); |
There was a problem hiding this comment.
Ah, I made a mistake.
Thank you for your review.
apapirovski
left a comment
apapirovski
left a comment
There was a problem hiding this comment.
LGTM but the assert message should be adjusted, IMHO.
| { | ||
| const Sign = crypto.Sign; | ||
| const instance = Sign('SHA256'); | ||
| assert.ok(instance instanceof Sign, 'call sign constructor without new'); |
There was a problem hiding this comment.
The message here seems to be more of a description of the test than communicate what was expected. Maybe something like "expected Sign to return a new instance when called without new keyword"?
There was a problem hiding this comment.
Thank you for review.
I got it. I'll revise error message.
| { | ||
| const Verify = crypto.Verify; | ||
| const instance = Verify('SHA256'); | ||
| assert.ok(instance instanceof Verify, 'call verify constructor without new'); |
| const Sign = crypto.Sign; | ||
| const instance = Sign('SHA256'); | ||
| assert.ok(instance instanceof Sign, 'Sign is expected to return a new \ | ||
| instance when called without `new` keyword'); |
There was a problem hiding this comment.
Sorry, two more things:
assertinstead ofassert.ok- We usually try to break strings using + and line them up beneath each other, so something like this:
assert(instance instanceof Sign, 'Sign is expected to return a new instance ' +
'when called without `new` keyword');Same below. Thanks!
There was a problem hiding this comment.
I don't mind.
assert instead of assert.ok
What difference between assert.ok and assert ?
I think assert() is better because there are fewer characters, right ?
There was a problem hiding this comment.
assert.ok is just an alias for assert (or, well, the other way around...). Not better but I think in general we try to use assert.
The practical difference in this case is it lets us fit that first line instead of having to split everything significantly more awkwardly. :)
There was a problem hiding this comment.
Thanks for your detailed information.
I understood that assert was better.
|
Landed in 0ab98f1 |
- Call Sign without new - Call Verify without new - Call Verify#verify with options.padding !== options.padding >> 0 - Call Verify#verify with options.saltLength !== options.saltLength >> 0 PR-URL: #17426 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
- Call Sign without new - Call Verify without new - Call Verify#verify with options.padding !== options.padding >> 0 - Call Verify#verify with options.saltLength !== options.saltLength >> 0 PR-URL: #17426 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
- Call Sign without new - Call Verify without new - Call Verify#verify with options.padding !== options.padding >> 0 - Call Verify#verify with options.saltLength !== options.saltLength >> 0 PR-URL: #17426 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
7 participants
I added these case:
Signwithout newVerifywithout newVerify#verifywithoptions.padding !== options.padding >> 0Verify#verifywithoptions.saltLength !== options.saltLength >> 0Current coverage: https://coverage.nodejs.org/coverage-06e1b0386196f8f8/root/internal/crypto/sig.js.html
After this PR: crypto/sig.js become 100% covered even in branch coverage.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
test