test: refactor test-crypto-timing-safe-equal#9843
Uh oh!
There was an error while loading. Please reload this page.
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.
targos commented Nov 29, 2016
@Trott: targos@8909496 ;) |
| crypto.timingSafeEqual(Buffer.from([1,2,3]),Buffer.from([1,2])); | ||
| },'should throw when given buffers with different lengths'); | ||
| },/^TypeError:Inputbuffersmusthavethesamelength$/, | ||
| '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 :)
targos commented Dec 1, 2016
Landed in cf71915 |
Add RegExp arguments to throws assertions. PR-URL: #9843 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Add RegExp arguments to throws assertions. PR-URL: #9843 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Add RegExp arguments to throws assertions. PR-URL: nodejs#9843 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Add RegExp arguments to throws assertions. PR-URL: nodejs#9843 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Add RegExp arguments to throws assertions. PR-URL: #9843 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
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/