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.

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

Add tests for increased coverage #63

Merged
merged 1 commit into from Feb 6, 2020
Merged

Add tests for increased coverage #63

merged 1 commit into from Feb 6, 2020

Conversation

@ryanio
Copy link
Contributor

ryanio commented Feb 5, 2020

This PR adds tests for setCode, getCode, setStorage, getStorage, and isEmpty.

While adding tests for setStorage it looks like there was a type mismatch on the defined cb according to its typedoc so I fixed it to TriePutCb.

The only uncovered line left is for serialize() which is actually tested but ethUtil.defineProperties overwrites its own serialize method, with that removed in the next refactor coverage should hit 100%.


I also updated the test and coverage scripts to their modern equivalent and removed coveralls dep:

"coverage": "npx nyc npm run test",
"test": "npx tape -r ts-node/register test/*.spec.ts",
@github-actions

This comment has been minimized.

Copy link

github-actions bot commented Feb 5, 2020

Coverage Status

Coverage increased (+54.5%) to 96.97% when pulling 4c0a33d on addTests into 485cddb on master.

@ryanio ryanio force-pushed the addTests branch from 4c0a33d to 7a0ef2e Feb 6, 2020
Copy link
Member

holgerd77 left a comment

👍 Nice, this was the most under-tested library in the whole library suite. Looks good to me.

@@ -14,8 +14,7 @@
"scripts": {
"build": "ethereumjs-config-build",
"prepublishOnly": "npm run test && npm run build",
"coverage": "ethereumjs-config-coverage",
"coveralls": "ethereumjs-config-coveralls",
"coverage": "npx nyc npm run test",

This comment has been minimized.

Copy link
@holgerd77

holgerd77 Feb 6, 2020

Member

I'll give this a go since everything config and setup related is so much in a transition anyhow with the move to the monorepo. We nevertheless should avoid to case-by-case drop out the ethereumjs-config configuration unification, but rather make updates on the config library side.

This is more work in the beginning but clearly showed some benefits on having consistent configuration and at the end less work on updating script directives and the like.

We can nevertheless also have some continued respectively revived discussion on how we want to evolve here (with the config library).

This comment has been minimized.

Copy link
@ryanio

ryanio Feb 6, 2020

Author Contributor

Sounds good, I agree I think there is a lot of benefit to ethereumjs-config but the coveralls and coverage commands in this case were not being so helpful since a) we are now using gh actions to push to coveralls so we don't need a coveralls dep and b) we will be using codecov in the monorepo for better monorepo support. c) just running npx nyc npm run test seems to do the trick. I think after the monorepo transition we'll have some more clarity, but nonetheless unified lint, prettier, nyc, etc. configs are super useful.

@@ -215,11 +215,11 @@ export default class Account {
* @param val
* @param cb
*/
setStorage(trie: Trie, key: Buffer | string, val: Buffer | string, cb: () => void) {
setStorage(trie: Trie, key: Buffer | string, val: Buffer | string, cb: TriePutCb) {

This comment has been minimized.

Copy link
@holgerd77

holgerd77 Feb 6, 2020

Member

Interface is matching and also backwards-compatible, so this change should be ok.

account.serialize(),
rlp.encode([raw.nonce, raw.balance, raw.stateRoot, raw.codeHash]),
),
0,

This comment has been minimized.

Copy link
@holgerd77

holgerd77 Feb 6, 2020

Member

For the reference: what is the main reasoning for this switch in constructor invocation on the test?

This comment has been minimized.

Copy link
@ryanio

ryanio Feb 6, 2020

Author Contributor

I was trying to write a better test case, but also realized that we can't test our serialize function since it is being overwritten by ethUtil.defineProperties. Nonetheless I left it as a better test case, as I believe we are going to simplify the constructor in the future to just the params and not accept buffers/arrays/etc.

"husky": "^2.1.0",
"nyc": "^13.2.0",
"merkle-patricia-tree": "^3.0.0",

This comment has been minimized.

Copy link
@holgerd77

holgerd77 Feb 6, 2020

Member

Should be fine to have this as a new dev dependency.

t.end()
})
})
})

This comment has been minimized.

Copy link
@holgerd77
t.equals(Buffer.compare(code!, Buffer.alloc(0)), 0)
t.end()
})
})

This comment has been minimized.

Copy link
@holgerd77
t.notOk(account.isEmpty())
t.end()
})
})

This comment has been minimized.

Copy link
@holgerd77

holgerd77 Feb 6, 2020

Member

Rest looks good.

@holgerd77 holgerd77 merged commit 6bbeb01 into master Feb 6, 2020
14 checks passed
14 checks passed
coverage
Details
coverage
Details
lint
Details
lint
Details
test (8.x)
Details
test (8.x)
Details
test (10.x)
Details
test (10.x)
Details
test (12.x)
Details
test (12.x)
Details
test (13.x)
Details
test (13.x)
Details
LGTM analysis: JavaScript No new or fixed alerts
Details
coverage/coveralls Coverage increased (+54.8%) to 97.222%
Details
@holgerd77 holgerd77 deleted the addTests branch Feb 6, 2020
@holgerd77

This comment has been minimized.

Copy link
Member

holgerd77 commented Feb 6, 2020

@ryanio Ah, is it ok if I often directly merge your PRs or should I just approve and leave this open for merge up to you?

@evertonfraga

This comment has been minimized.

Copy link
Member

evertonfraga commented Feb 6, 2020

Ryan is working hard to make the Monorepo coverage badges all green. 🎉

https://github.com/evertonfraga/ethereumjs-vm/

@ryanio

This comment has been minimized.

Copy link
Contributor Author

ryanio commented Feb 6, 2020

@ryanio Ah, is it ok if I often directly merge your PRs or should I just approve and leave this open for merge up to you?

Yes no problem feel free to merge, keeps things moving along :)

I will be sure to use appropriate labels if my intention is for a PR not to be merged after review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.