Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve test coverage #27

Merged
merged 5 commits into from
Jul 28, 2018
Merged

Improve test coverage #27

merged 5 commits into from
Jul 28, 2018

Conversation

holgerd77
Copy link
Member

Adresses #26

@coveralls
Copy link

coveralls commented Feb 9, 2018 •
edited
Loading

Coverage Status

Coverage increased (+1.8%) to 88.38% when pulling 18ded5b on improve-test-coverage into 86464c9 on master.

@holgerd77
Copy link
Member Author

@axic what is the main problem with the fromV1() testing?

@axic
Copy link
Member

axic commented Feb 9, 2018

what is the main problem with the fromV1() testing?

Real world samples are hard to find.

@holgerd77 holgerd77 force-pushed the improve-test-coverage branch 3 times, most recently from adced59 to 9b26e23 Compare February 14, 2018 22:25
@holgerd77 holgerd77 changed the title [WIP] Improve test coverage Improve test coverage Feb 15, 2018
@holgerd77 holgerd77 requested a review from axic February 15, 2018 08:33
@holgerd77
Copy link
Member Author

holgerd77 commented Feb 15, 2018 •
edited
Loading

@axic I'll leave this here for now, please review and don't close the issue, since this can be done incrementally. For me it was a good occasion to get to know the library a bit better, but other things are more pressing right now.

@holgerd77
Copy link
Member Author

@axic Can we have this merged?

axic
axic reviewed Mar 2, 2018
View reviewed changes
test/index.js Outdated
var fixturePrivKeyBuffer = Buffer.from(fixturePrivKey, 'hex')

var fixturePubKey = '5d4392f450262b276652c1fc037606abac500f3160830ce9df53aa70d95ce7cfb8b06010b2f3691c78c65c21eb4cf3dfdbfc0745d89b664ee10435bb3a0f906c'
var fixturePubKeyStr = '0x' + fixturePubKey
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Buffer isn't shortened, I'd have liked PublicKey, PublicKeyString and PublicKeyBuffer and same for private.

axic
axic reviewed Mar 2, 2018
View reviewed changes
test/index.js Outdated
@@ -132,14 +139,14 @@ describe('.toV3()', function () {
var uuid = Buffer.from('7e59dc028d42d09db29aa8a0f862cc81', 'hex')

it('should work with PBKDF2', function () {
var key = Buffer.from('efca4cdd31923b50f4214af5d2ae10e7ac45a5019e9431cc195482d707485378', 'hex')
var key = Buffer.from(fixturePrivKey, 'hex')
var wallet = Wallet.fromPrivateKey(key)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could just use fixtureWallet here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been done on a later commit, comment is outdated.

axic
axic reviewed Mar 2, 2018
View reviewed changes
test/index.js Outdated
var wallet = Wallet.fromPrivateKey(key)
var w = '{"version":3,"id":"7e59dc02-8d42-409d-b29a-a8a0f862cc81","address":"b14ab53e38da1c172f877dbc6d65e4a1b0474c3c","crypto":{"ciphertext":"01ee7f1a3c8d187ea244c92eea9e332ab0bb2b4c902d89bdd71f80dc384da1be","cipherparams":{"iv":"cecacd85e9cb89788b5aab2f93361233"},"cipher":"aes-128-ctr","kdf":"pbkdf2","kdfparams":{"dklen":32,"salt":"dc9e4a98886738bd8aae134a1f89aaa5a502c3fbd10e336136d4d5fe47448ad6","c":262144,"prf":"hmac-sha256"},"mac":"0c02cd0badfebd5e783e0cf41448f84086a96365fc3456716c33641a86ebc7cc"}}'
// FIXME: just test for ciphertext and mac?
assert.equal(wallet.toV3String('testtest', { kdf: 'pbkdf2', uuid: uuid, salt: salt, iv: iv }), w)
})
it('should work with Scrypt', function () {
var key = Buffer.from('efca4cdd31923b50f4214af5d2ae10e7ac45a5019e9431cc195482d707485378', 'hex')
var key = Buffer.from(fixturePrivKey, 'hex')
var wallet = Wallet.fromPrivateKey(key)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

@axic axic mentioned this pull request Mar 2, 2018
Merged
@holgerd77
Copy link
Member Author

@axic Ok, did the renaming. This should be ready now. Will rebase/update the release commit once you approve/merge this.

@holgerd77
Copy link
Member Author

@axic Have got 15 min left, if you do a speed-review, I could prepare the other PR! :-)

@axic axic merged commit 688bd06 into master Jul 28, 2018
@axic axic deleted the improve-test-coverage branch July 28, 2018 17:54
@bignbeefy bignbeefy mentioned this pull request May 1, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@axic axic axic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@holgerd77 @coveralls @axic