Enhance crypto/sig.js coverage - #17426
Conversation
Sorry, something went wrong.
| { | ||
| 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.
sign -> verify
Sorry, something went wrong.
There was a problem hiding this comment.
Ah, I made a mistake.
Thank you for your review.
Sorry, something went wrong.
apapirovski
left a comment
There was a problem hiding this comment.
LGTM but the assert message should be adjusted, IMHO.
Sorry, something went wrong.
| { | ||
| 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"?
Sorry, something went wrong.
There was a problem hiding this comment.
Thank you for review.
I got it. I'll revise error message.
Sorry, something went wrong.
| { | ||
| const Verify = crypto.Verify; | ||
| const instance = Verify('SHA256'); | ||
| assert.ok(instance instanceof Verify, 'call verify constructor without new'); |
There was a problem hiding this comment.
Same as above.
Sorry, something went wrong.
| 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:
- assert instead of assert.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!
Sorry, something went wrong.
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 ?
Sorry, something went wrong.
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. :)
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks for your detailed information.
I understood that assert was better.
Sorry, something went wrong.
Sorry, something went wrong.
- 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>
I added these case:
Current 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
Affected core subsystem(s)
test