Added unit tests for mnemonic-test.js #1115
Conversation
Codecov ReportBase: 69.46% // Head: 69.60% // Increases project coverage by
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## master #1115 +/- ##
==========================================
+ Coverage 69.46% 69.60% +0.13%
==========================================
Files 158 158
Lines 26587 26587
==========================================
+ Hits 18469 18506 +37
+ Misses 8118 8081 -37
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
haxwell
changed the title
|
Great job adding coverage here! I will add some specific feedback about your code changes but as a general contribution rule, a PR like this should probably just be one commit -- so please squash the two extra "lint" commits. It'll make review a lot more efficient and keep the master tree cleaner. |
test/mnemonic-test.js
Outdated
|
|
||
| it('should handle fromJSON correctly', () => { | ||
| const m1 = new Mnemonic(); | ||
| const m2 = Mnemonic.fromJSON(m1.toJSON()); |
There was a problem hiding this comment.
To follow up from the last review comment, in tests like this I think you should actually assert that m1.toJSON() does what you think it does before comparing it with something. For example, what if toJSON() just returned null because of some bug? You can just quickly assert that the result of toJSON() is an object with non-null entropy and phrase maybe ?
There was a problem hiding this comment.
I added the code as you suggested. Just an insight into my original thinking here.. we are not testing toJSON(). In this case, it works, we assume it works, and we are testing, in the case that it works, what does fromJSON() do? Also, looking code coverage, toJSON() is called 11 times, so we have other coverage to be sure it works.
| const m1 = new Mnemonic(); | ||
| m1.destroy(); | ||
| assert.strictEqual(m1.phrase, null); | ||
| assert.strictEqual(m1.language, LANGUAGE_ENGLISH); | ||
| assert.strictEqual(m1.bits, MIN_ENTROPY); | ||
| assert.strictEqual(m1.entropy, null); |
There was a problem hiding this comment.
especially here! make sure m1 had values before we assert they were destroyed correctly!
There was a problem hiding this comment.
I'm hoping I can sway you to my thinking.. Again, that's not the subject of the test. True enough, it doesn't detract much, and it is an extra bit of security. Still it is... what's the word.. superfluous. We already test whether or not the constructor works in multiple places. The concern here is whether destroy() sets the state to the expected values.
It doesn't even really matter if there were values before.. one, that is covered in many other places where we use the constructor (one of those should be a specific test: "the constructor should set the expected values"), but two, whether there were values or not, our test should pass when the state of the object is equal to the assertions.
If the state is correct, our test is correct.
Swayed?
| it('should handle destroy correctly when entropy is set', () => { | ||
| const m1 = new Mnemonic(); | ||
| m1.entropy = Buffer.from('00000000000000000000000000000000', 'hex'); | ||
| m1.destroy(); | ||
| assert.strictEqual(m1.phrase, null); | ||
| assert.strictEqual(m1.language, LANGUAGE_ENGLISH); | ||
| assert.strictEqual(m1.bits, MIN_ENTROPY); | ||
| assert.strictEqual(m1.entropy, null); | ||
| }); |
There was a problem hiding this comment.
this test can maybe just get combined with preceding one
There was a problem hiding this comment.
They are separate because they cover two different paths in the code. To combine them would leave one path uncovered.
| it('should handle fromOptions correctly when a phrase is passed in', () => { | ||
| const m1 = new Mnemonic(); | ||
| const m2 = Mnemonic.fromOptions(m1.getPhrase().toString()); | ||
|
|
||
| assert.strictEqual(m1.phrase, m2.phrase); | ||
| assert.strictEqual(LANGUAGE_ENGLISH, m2.language); | ||
| assert.strictEqual(256, m2.bits); | ||
| assert.strictEqual(32, m2.entropy.length); | ||
| }); | ||
|
|
||
| it('should handle fromOption correctly when options.entropy is set', () => { | ||
| const entropy = Buffer.from('00000000000000000000000000000000', 'hex'); | ||
| const m2 = Mnemonic.fromOptions({ entropy: entropy}); | ||
|
|
||
| assert.strictEqual(null, m2.phrase); | ||
| assert.strictEqual(LANGUAGE_ENGLISH, m2.language); | ||
| assert.strictEqual(128, m2.bits); | ||
| assert.strictEqual(16, m2.entropy.length); | ||
| }); | ||
|
|
||
| it('should handle fromOption correctly when no options are set', () => { | ||
| const m2 = Mnemonic.fromOptions({ }); | ||
|
|
||
| assert.strictEqual(null, m2.phrase); | ||
| assert.strictEqual(LANGUAGE_ENGLISH, m2.language); | ||
| assert.strictEqual(256, m2.bits); | ||
| assert.strictEqual(null, m2.entropy); | ||
| }); |
There was a problem hiding this comment.
Take a look at the very tests in this file, I think we are already covered here. And notice in those tests, we pull hard-coded phrases from Trezor's test vectors in two languages. You can add more tests in that section if you think something is missing. I guess you are being explicit about fromOptions() being different from using the new constructor. Even if that is your goal, I think you can add those tests in that first block
There was a problem hiding this comment.
I'm shooting for 100% coverage here, so some tests may seem unnecessary, but they cover some branch that could have been taken in the code. For instance, when I comment out these tests, these paths in the code coverage are untested.
I think maybe there is a typo in your comment ("...at the very [first?] tests in this file"), but I didn't see other code covering the fromOptions() method.
Co-authored-by: Matthew Zipkin <pinheadmz@gmail.com>
Co-authored-by: Matthew Zipkin <pinheadmz@gmail.com>
Thank you, I've enjoyed working on this code! I can do the squashing on my side in the future. But on your side, when you merge the PR, do you get an option to squash the commits? For instance, an option or a drop down associated with the Merge Pull Request button. |
No description provided.