Fix #70, Use SHA1 digest with pbkdf2 method #72
Conversation
* Added SHA1 digest method to pbkdf2 * fixed inconsistent error message for non-empty strings * test fixed for correct error message comparision
|
https://github.com/ericelliott/credential/blob/master/test/credential-test.js#L248 test is failing as I may have hit the 0.01% chance for it to fail. But its more like a timing issue I think. |
sushantdhiman
changed the title
|
|
||
| t.is(actual, expected); | ||
| t.throws(function (){ |
There was a problem hiding this comment.
Why not test against the stderr.message property? As you have demonstrated here, .throws() is for testing whether or not a function throws. What we really want to test is whether or not we get the expected error message, right?
There was a problem hiding this comment.
I thought it would be better to use tape api to check against errors. But as we already have stderr stream, we can use it to make it simple. Will make this change soon
There was a problem hiding this comment.
We're still using the tape API when we check against the stderr.message property. Again, .throws() is for testing whether or not a function throws. Note how you had to create a fake function and test against that to use it... That just forces us to look harder to see what you're really trying to test...
|
@ericelliott changed the test. I have used pattern match just like #71 . Its simpler than empty function and |
|
Hi @ericelliott , Do you need any improvements in this. I could really use this update in my projects :) |
|
Can this be published as 1.0.1? The deprecation warning is unfortunate when making the case that this is the "right" module for the job. Thanks! |
* Add explicit SHA1 digest in crypto.pbkdf2 calls Deprecation mentioned in ericelliott/credential#70, implemented in ericelliott/credential#72. * Update node_js versions in .travis.yml Source: https://docs.travis-ci.com/user/languages/javascript-with-nodejs/
Fixes #70
Closes #71
Changelog
SHA1digest forpbkdf2, will be ignored forv0.10v6Essentially this would remove the deprecation warning in Node
v6, but still remains a non breaking change because of version check.cc @ericelliott @tjconcept @brianmhunt