-
Notifications
You must be signed in to change notification settings - Fork 274
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice selection of test cases, thanks, will approve and merge.
@@ -14,7 +14,7 @@ import { toBuffer, baToJSON, stripZeros } from './bytes' | |||
* @param data data to be validated against the definitions | |||
* @deprecated | |||
*/ | |||
export const defineProperties = function(self: any, fields: any, data: any) { | |||
export const defineProperties = function(self: any, fields: any, data?: any) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference: if I understand correctly this is a correction of the API regarding the currently existing optionality of the data
param within the function code. It can be regarded as a bug fix, but should be pointed out in the next release notes.
@@ -13,8 +14,7 @@ const { | |||
isValidChecksumAddress, | |||
isValidAddress, | |||
toChecksumAddress, | |||
} = require('../src') | |||
import BN = require('bn.js') | |||
} from '../src' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some unrelated import improvements, nice, and justifiable since only touching the tests.
assert.equal(buf.toString('hex'), '5b9ac8') | ||
}) | ||
}) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests moved to the externals.spec.ts
file, ok.
assert.throws(function() { | ||
new src.BN(0).iaddn(0x4000000) | ||
}, /^Error: Assertion failed$/) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice selection, ok.
For my understanding: what is this 0x4000000 case?
} catch (e) {} | ||
assert.equal(res, undefined) | ||
}) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
For my understanding, assume some valid reasoning for it: the error cases can't be addressed with assert.throws
?
src.secp256k1.privateKeyImport(buffer) | ||
}) | ||
}) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
assert.throws(() => (src as any).getKeys([], 3289), Error) | ||
|
||
// should detect invalid length hex string | ||
assert.equal((src as any).isHexString('0x0', 2), false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
This PR adds some basic tests for the re-exports in the new test file
externals.spec.ts
:Closes #234.