test: refactor test-crypto-timing-safe-equal#9843
test: refactor test-crypto-timing-safe-equal#9843targos wants to merge 1 commit intonodejs:masterfrom
Conversation
Add RegExp arguments to throws assertions.
targos
added
crypto
Trott
left a comment
Trott
left a comment
There was a problem hiding this comment.
LGTM if CI is ✅
You may already be thinking along these lines, but perhaps when you're done with all of these, we can create a lint rule that prohibits a string for the second argument of assert.throws() since it is usually an error and (in cases like this file) when it is not an error, the assertion is still probably better with a constructor, RegExp, or validation function added.
|
@Trott: targos@8909496 ;) |
| crypto.timingSafeEqual(Buffer.from([1, 2, 3]), Buffer.from([1, 2])); | ||
| }, 'should throw when given buffers with different lengths'); | ||
| }, /^TypeError: Input buffers must have the same length$/, | ||
| 'should throw when given buffers with different lengths'); |
There was a problem hiding this comment.
Is indentation ok? It seems off by one.
There was a problem hiding this comment.
I don't really know... I aligned the arguments together.
There was a problem hiding this comment.
Seems right to me. I think it's just a visual effect because of the / and the '.
There was a problem hiding this comment.
@gibfahn I'm fine with this but there are 3 spaces not 2.
There was a problem hiding this comment.
Yes, but that's to align with the }, on the previous line right?
There was a problem hiding this comment.
The ' should be aligned with the c of crypto but it doesn't matter, there is no need to waste time over this nit 😄
There was a problem hiding this comment.
Actually rereading @targos comment above I think this correct, so ignore me :)
|
Landed in cf71915 |
Add RegExp arguments to throws assertions. PR-URL: #9843 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Add RegExp arguments to throws assertions. PR-URL: #9843 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Add RegExp arguments to throws assertions. PR-URL: nodejs#9843 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Add RegExp arguments to throws assertions. PR-URL: nodejs#9843 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Add RegExp arguments to throws assertions. PR-URL: #9843 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
7 participants
Checklist
make -j8 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
test, crypto
Description of change
Add RegExp arguments to throws assertions.
CI: https://ci.nodejs.org/job/node-test-pull-request/5023/