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

Clean up and improve coverage #449

Closed
holgerd77 opened this issue Feb 23, 2019 · 3 comments
Closed

Clean up and improve coverage #449

holgerd77 opened this issue Feb 23, 2019 · 3 comments

Comments

@holgerd77
Copy link
Member

holgerd77 commented Feb 23, 2019

Will go a bit through the current master coverage build to roughly identify uncovered code parts to get a bit more of the total picture.

Goals are a bit in both directions:

  • to identify where coverage is still low and how this can be improved, but also
  • to identify unused or unoptimized code parts, (non-)coverage is also often an indicator here that code parts are not that much in production or even unnecessary

Some first things:

  1. We were still running blockchain and coverage test runs towards Byzantium leaving the new opcodes oncovered (or reported to be covered at least), this PR is addressing this: Complete switch to Petersburg on tests | Fix coverage #448
  2. Since we are not running Constantinople (the old one) fork rules for coverage the whole new SSTORE test is now left completely untested, we actually do might want to reintegrate some of just removed constantinopleSstoreTest test cases (see Test cleanup #437) to get some basic coverage, but maybe just as some single test cases in the new runCode API tests in some adopted format. Eventually these tests need also some modified setup to be a bit more flexible on adding new test cases, nevertheless a superb start that we have that at all
  3. The vm/logTable.js file significantly adds to non-coverage. This is actually some static value setup for the EXP opcode. At the moment I see no reason why it shouldn't be possible to just inline a dynamic 1-2 line version of this into the EXP opcode and get rid of the file. I did a short test because I thought if there might be some performance reason for the code, but even running this for (let i=0; i<1000000; i++) { pow32 = new BN('010000000000000000000000000000000000000000000000000000000000000000', 16) } doesn't take a significant amount of time, addressed in Replaced static vm logTable with dynamic inline version in EXP opcode #450
  4. fakeBlockchain is also contributing to non-coverage, getting rid of this is discussed here: Incorrect behavior of fakeBlockChain.prototype.getBlock #446
  5. Same with createHookedVM, deprecation is in discussion here: Deprecate createHookedVm #435, addressed in Drop createHookedVm #451

List is likely not complete, if you have additions feel free to directly edit the post, eventually with closing with your GitHub name on the list item.

@holgerd77
Copy link
Member Author

StateManager.dumpStorage() completely untested: https://coveralls.io/builds/21805704/source?filename=lib/state/stateManager.js#L432

@holgerd77
Copy link
Member Author

Eventually for 2. some of the structure of the old hookedVM tests can be taken for inspiration.

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

No branches or pull requests

2 participants