tests: remove binary assumption, fixes #34 #36
Conversation
|
resolves #34, doesn't add the warning though. Ping @dominictarr @indutny @calvinmetcalf and @fanatid on whether we should add that warning, reference: #34 (comment) |
|
Naturally, this is going to start failing on Travis on all the pre-binary versions... I'm not sure what the best course of action is. |
|
Since this package for browser not whether we should follow by latest active LTS node release? |
@fanatid the issue is, Node 6 is the only that one that isn't failing after this PR |
this should fix the issue: if (!process.browser && pVersionMajor >= 6) runTests('node pbkdf2', require('../'))@dcousens under drop 0.10 support I meant remove shim pbkdf2Sync |
|
@fanatid so we just curtail the valid test fixtures with UTF8 vs non-UTF8? I guess I'm ~OK with that, but, it just sounds like it could hurt someone. |
|
we also can use 2 sets of fixtures (utf8 and binary) |
| if (!Buffer.isBuffer(password)) password = new Buffer(password, 'binary') | ||
| if (!Buffer.isBuffer(salt)) salt = new Buffer(salt, 'binary') | ||
| if (!Buffer.isBuffer(password)) password = new Buffer(password, 'utf-8') | ||
| if (!Buffer.isBuffer(salt)) salt = new Buffer(salt, 'utf-8') |
There was a problem hiding this comment.
You don't need second argument here now, encoding is 'utf8' by default.
Maybe even use Buffer.from(..) ?
There was a problem hiding this comment.
wouldn't Buffer.from completely break previous versions?
There was a problem hiding this comment.
yea, sorry, .from will break all previous versions :(
|
Other than one nit by @fanatid , LGTM |
| if (!Buffer.isBuffer(password)) password = new Buffer(password, 'binary') | ||
| if (!Buffer.isBuffer(salt)) salt = new Buffer(salt, 'binary') | ||
| if (!Buffer.isBuffer(password)) password = new Buffer(password) | ||
| if (!Buffer.isBuffer(salt)) salt = new Buffer(salt) |
There was a problem hiding this comment.
@fanatid they are now defaulted
dcousens
force-pushed
the
testfix
branch
2 times, most recently
from
cbd6cd3 to
a7aaf54
Compare
| if (!Buffer.isBuffer(password)) password = new Buffer(password, 'binary') | ||
| if (!Buffer.isBuffer(salt)) salt = new Buffer(salt, 'binary') | ||
| if (!Buffer.isBuffer(password)) password = new Buffer(password, defaultEncoding) | ||
| if (!Buffer.isBuffer(salt)) salt = new Buffer(salt, defaultEncoding) |
There was a problem hiding this comment.
gah, this sucks. any ideas @calvinmetcalf @fanatid @indutny ?
There was a problem hiding this comment.
Just the situation, wondering if you had any thoughts on impl.
Otherwise, I assume this PR is good to go now
after 2016-10-01 node 0.10 will not be supported, so why not remove shim? |
|
Right, sorry, not sure how that didn't read correctly. |
|
@fanatid removed |
|
Ready to merge |
|
Published as |
No description provided.