Add Separate API Tests for the VM

[s1na]

Fixes #310.

This PR will attempt to implement the tests mentioned in #310, however it is still in the very early stages.

I will try to refactor as the PR progresses and the need arises. Please let me know if the structure could be improved, or you want test cases added/removed/modified, or there are any other issues with it.

[axic]

For some reason coveralls is not running on forks, but does this has a >90% coverage of bloom?

[holgerd77]

@axic Oh, just realize that the Travis -> CircleCI switch deactivated Coveralls for the moment. Will try to reintroduce this with the next PR.

Generally give this some time since work has just started.

[holgerd77]

@s1na Cool that you started! 👍 Sorry, as mentioned above test coverage is not working atm, which obviously would be relevant for you to have insight into. Will try to fix soon.

[holgerd77]

Hi @s1na, I've readded the coverage reporting with this PR #316. Once you rebase on top of this (would recommend a local file backup of your repository if you do this for the first time) you should get reports again.

[s1na]

@holgerd77 thanks for merging the coveralls PR, I was using istanbul directly to see the coverage.

However, it seems that coveralls has failed again, a little search suggested using COVERALLS_REPO_TOKEN might help to make it work for forks.

[axic]

Can you create a PR for this? Should merge it right away. Also usually we follow the pattern return cb(new Error..., thought it doesn't matter.

[axic]

Also please note with #295 test/bloom.js was merged, perhaps you want to git move the file to test/api/bloom.js and then apply your changes to it?

[s1na]

@axic sure, will do both right away

[axic]

I also suggest to add a new task to package.json called testAPI and also call that from .circle/config.yml under its own name. Have a look at how for example test_state_byzantium works.

Never mind, you could just require them in https://github.com/ethereumjs/ethereumjs-vm/blob/master/tests/tester.js#L248.

However it may be a good idea to introduce testAPI nevertheless, @holgerd77 what do you think?

[axic]

I think it may max sense creating a PR per internal module to get this merged more quickly. @holgerd77 what do you think?

[s1na]

@axic I just noticed that coverage for runBlockchain and runBlock is so low on coveralls, because the coveralls command only runs the state tests. Is this on purpose, or should I make a PR which adds testBlockchainTotalDifficulty to coverage command, to get a more realistic coverage report?

[axic]

I just noticed that coverage for runBlockchain and runBlock is so low on coveralls, because the coveralls command only runs the state tests. Is this on purpose, or should I make a PR which adds testBlockchainTotalDifficulty to coverage command, to get a more realistic coverage report?

I think we should include the others in the report too. @holgerd77 what do you think?

[s1na]

@holgerd77 @axic Hey guys, I think the PR is ready for your feedback now :)

Two cases are failing, which I'm not sure if they should fail. One is bloom's multiCheck, which converts inputs via hex to buffer, although check itself doesn't do such a thing.

The other case is when populateCache: false in runTx. it might be a bit confusing to have this flag, but then use cache for getting accounts.

A third one also fails, which should be fixed if this is merged.

Btw, I haven't still added the api tests to the coverage command, if it should be added, please let me know and I will add it.

[holgerd77]

Hi Sina, very cool! 👍 I was unavailable in the last 10 days due to personal reasons and have to catch up a bit, so it can take some more days until I find some time to have a look.

[holgerd77]

Follow-up-note: had a first very rough look just to grasp the size. This looks really really great, wonderful, with such a broad and thought-through coverage of usage variants, already thank you very much, this will very much help to keep up the quality within the library! 😄 👍

[s1na]

Hey Holger :) thanks for having a look, I'd be indeed really glad if I manage to help EthereumJS which I use directly or indirectly so much, even a tiny bit 😄
Please let me know if there's anything I can do, which would make the review process easier.

[axic]

@s1na can you please create a PR for each component you are testing? I think that could speed up the process a lot. Maybe just create 2 PRs for two components first, and lets see how quickly we can finish them.

[s1na]

@axic sure thing. I've started creating smaller PRs by cherry-picking these commits.

[holgerd77]

Just had a short look, coverage on the library increased from 86% - taking this https://coveralls.io/builds/18728918 PR as a reference - to roughly 93% with the latest merge https://coveralls.io/builds/19299976, so so great!!! 👍 👍 👍 😄

Some detailed stats:

index.js: 82.35% -> 92.65%
runBlock.js: 6.2% (!!!!) -> 75.71%
runBlockchain.js: 5.71% (!!!!!!!!!) -> 91.43%
runTx.js: 92.86% -> 98.53%
StateManager.js: 72.29% -> 82.25%

🏆 🏆 🏆

I know this is tempting, but don't sit down immediately and try to further improve this 😄, if this was the last PR we should really make some (short, hehe) break here.

I think I'll write out another bounty with a second round on this.

[s1na]

We did it! 🎉 🎉 🦄 :) A break sounds good 🍷

Thanks a lot Holger for bearing with me, and the guidance throughout the process 😄

I believe this PR can be closed.

[axic]

Great work @s1na! (& @holgerd77)