Migrate wallet tests to `tape
#2742
Conversation
Codecov Report
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
acolytec3
force-pushed
the
wallet-tests-to-tape
branch
from
b53f2b0
to
14875ac
Compare
packages/wallet/test/hdkey.spec.ts
Outdated
| tape('.fromExtendedKey()', (t) => { | ||
| const hdnode1 = EthereumHDKey.fromExtendedKey( | ||
| 'xpub661MyMwAqRbcGout4B6s29b6gGQsowyoiF6UgXBEr7eFCWYfXuZDvRxP9zEh1Kwq3TLqDQMbkbaRpSnoC28oWvjLeshoQz1StZ9YHM1EpcJ' | ||
| ) | ||
| t.deepEqual( | ||
| hdnode1.publicExtendedKey(), | ||
| 'xpub661MyMwAqRbcGout4B6s29b6gGQsowyoiF6UgXBEr7eFCWYfXuZDvRxP9zEh1Kwq3TLqDQMbkbaRpSnoC28oWvjLeshoQz1StZ9YHM1EpcJ' | ||
| ) | ||
| t.throws(function () { | ||
| hdnode1.privateExtendedKey() | ||
| }, /^Error: No private key$/) | ||
| const hdnode2 = EthereumHDKey.fromExtendedKey( | ||
| 'xprv9s21ZrQH143K4KqQx9Zrf1eN8EaPQVFxM2Ast8mdHn7GKiDWzNEyNdduJhWXToy8MpkGcKjxeFWd8oBSvsz4PCYamxR7TX49pSpp3bmHVAY' | ||
| ) | ||
| t.deepEqual( | ||
| hdnode2.publicExtendedKey(), | ||
| 'xpub661MyMwAqRbcGout4B6s29b6gGQsowyoiF6UgXBEr7eFCWYfXuZDvRxP9zEh1Kwq3TLqDQMbkbaRpSnoC28oWvjLeshoQz1StZ9YHM1EpcJ' | ||
| ) | ||
| t.deepEqual( | ||
| hdnode2.privateExtendedKey(), | ||
| 'xprv9s21ZrQH143K4KqQx9Zrf1eN8EaPQVFxM2Ast8mdHn7GKiDWzNEyNdduJhWXToy8MpkGcKjxeFWd8oBSvsz4PCYamxR7TX49pSpp3bmHVAY' | ||
| ) | ||
| t.end() |
There was a problem hiding this comment.
Could we perhaps add some comments that makes these two cases a bit clearer (i.e. hdnode1 testing fromExtendedKey with a public key, and hdnode2 is testing it with a private key) ?
There was a problem hiding this comment.
(Could just be added as a third arg to deepEqual)
packages/wallet/test/index.spec.ts
Outdated
|
|
||
| // t.test('.toV3() should fail', function () { | ||
| // assert.throws(function () { | ||
| // Wallet.fromPublicKey(pubKey).toV3() | ||
| // }, /^Error: This is a public key only wallet$/) | ||
| // }) |
There was a problem hiding this comment.
Why is this test commented out? can we just remove it?
There was a problem hiding this comment.
Not sure but I added it back in and it passes.
packages/wallet/test/index.spec.ts
Outdated
| 'should generate an account compatible with ICAP Direct' | ||
| ) | ||
| const addr = bytesToHex(wallet.getAddress()) | ||
| t.equal(BigInt('0x' + addr) <= max, true) |
There was a problem hiding this comment.
| t.equal(BigInt('0x' + addr) <= max, true) | |
| t.equal(BigInt('0x' + addr) <= max, true, 'should generate an account compatible with ICAP Direct') |
There was a problem hiding this comment.
Could also remove it from the previous test, personally I think it makes more sense as the last message rather than the first (and that's what you seem to be doing in other tests)
packages/wallet/test/index.spec.ts
Outdated
| return permus | ||
| } | ||
| const strKdfOptions = { iv, salt, uuid } | ||
| const buffKdfOptions = { |
There was a problem hiding this comment.
Super nit, but could be renamed to bytesKdfOptions
packages/wallet/test/index.spec.ts
Outdated
| for (const perm of permutations) { | ||
| const encFixtureWallet = await fixtureWallet.toV3String(pw, { | ||
| kdf: 'pbkdf2', | ||
| c: n, | ||
| uuid: perm.uuid, | ||
| salt: perm.salt, | ||
| iv: perm.iv, | ||
| }) |
There was a problem hiding this comment.
I think it might be more efficient to keep the Promise.all rather than looping through promises, since with Promise.all all promises will be started simultaneously, while here they will be run sequentially.
There was a problem hiding this comment.
Hmm, ok, let me look at it with it changed back. This whole section of the tests was almost incomprehensible to me until I unwound everything.
packages/wallet/test/index.spec.ts
Outdated
| } catch (err: any) { | ||
| t.ok( | ||
| err.message.includes( | ||
| 'Invalid salt, must be a string (empty or a non-zero even number of hex characters) or buffer' |
There was a problem hiding this comment.
| 'Invalid salt, must be a string (empty or a non-zero even number of hex characters) or buffer' | |
| 'Invalid salt, must be a string (empty or a non-zero even number of hex characters) or uint8array' |
packages/wallet/test/index.spec.ts
Outdated
| describe('.fromV1()', function () { | ||
| it('should work', function () { | ||
| tape('.fromV1()', (t) => { | ||
| t.test('should work', function () { | ||
| const sample = '{"Address":"d4584b5f6229b7be90727b0fc8c6b91bb427821f","Crypto":{"CipherText":"07533e172414bfa50e99dba4a0ce603f654ebfa1ff46277c3e0c577fdc87f6bb4e4fe16c5a94ce6ce14cfa069821ef9b","IV":"16d67ba0ce5a339ff2f07951253e6ba8","KeyHeader":{"Kdf":"scrypt","KdfParams":{"DkLen":32,"N":262144,"P":1,"R":8,"SaltLen":32},"Version":"1"},"MAC":"8ccded24da2e99a11d48cda146f9cc8213eb423e2ea0d8427f41c3be414424dd","Salt":"06870e5e6a24e183a5c807bd1c43afd86d573f7db303ff4853d135cd0fd3fe91"},"Id":"0498f19a-59db-4d54-ac95-33901b4f1870","Version":"1"}' | ||
| const wallet = Wallet.fromV1(sample, 'foo') | ||
| assert.strictEqual(wallet.getAddressString(), '0xd4584b5f6229b7be90727b0fc8c6b91bb427821f') | ||
| t.deepEqual(wallet.getAddressString(), '0xd4584b5f6229b7be90727b0fc8c6b91bb427821f') | ||
| }) | ||
| }) | ||
| */ | ||
|
|
There was a problem hiding this comment.
Do we still need to keep this commented out test?
There was a problem hiding this comment.
Not sure. Let me look at the comments from @paulmillr when the big upgrade was done a few months back. It fails if I uncomment it so not sure if it's necessary or not.
There was a problem hiding this comment.
I looked at the git history for this test and it's been commented out for at least 4 years, so I think we're safe to remove it.
Replaced
mochawithtapeas test runner forwallettests and decomposes for better readability