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

Improve VM browser compatibilty / test coverage #465

Closed
4 of 5 tasks
holgerd77 opened this issue Mar 6, 2019 · 10 comments · Fixed by #653
Closed
4 of 5 tasks

Improve VM browser compatibilty / test coverage #465

holgerd77 opened this issue Mar 6, 2019 · 10 comments · Fixed by #653

Comments

@holgerd77
Copy link
Member

holgerd77 commented Mar 6, 2019

With the merge of #461 there is now browser testing done on the VM to improve VM browser compatibility and avoid introducing bugs like #457.

Currently browser tests are not run on all API test files and many files are excluded since tests were not passing, see karma.conf.js exclude directive.

Excluded tests should be integrated and the underlying issues addressed. Initial experiments indicated that there probably will be some deeper investigation needed and it's probably most practical to address step-by-step on separate PRs. It is also likely that these fixes will spawn over different libraries and not only target the VM, so this should be addressed in a holistic way with PRs where necessary.

The following is a list of the test files which need to be activated:

  • ./tests/api/state/stateManager.js
  • ./tests/api/state/storageReader.js
  • ./tests/api/index.js
  • ./tests/api/runBlock.js'
  • ./tests/api/runBlockchain.js
@cdetrio
Copy link
Contributor

cdetrio commented Mar 6, 2019

It would also be nice to document and test all ethereumjs-vm methods that remix uses. For example, a recent change to stateManager.checkpoint() broke remix (though it was easily fixed): ethereum/remix#1103 (comment)

@holgerd77
Copy link
Member Author

@cdetrio StateManager is a bit a special case since it is just in the transition from private to public with the API currently marked as experimental.

@s1na
Copy link
Contributor

s1na commented Mar 11, 2019

Some of the issues (will update list):

  • One of the stateManager tests requires ethereumjs-testing which depends on asyncawait which depends on node-fibers which doesn't work in browsers.
  • In tests/api/index.js, runBlockchain calls somewhere stateManagers generateCanonicalGenesis which puts ~8000 accounts to the trie. This causes the tests to time out (even 200s is not enough).

@holgerd77
Copy link
Member Author

For ethereumjs-testing we probably should revert asyncawait introduction (from October 2017 for Node 6 support) since we have now dropped Node 6.

This will likely cause similar problems in other libraries as well.

@holgerd77
Copy link
Member Author

Did a test run for the failing StateManager browser tests, the asyncawait removal in ethereumjs-testing alone doesn't fix the tests yet, will nevertheless do a release for that.

@holgerd77
Copy link
Member Author

Re-checked, StateManager tests are still excluded in karma.conf.js so this needs some analysis if the tests would still fail and eventually some follow-up fixing.

@ryanio
Copy link
Contributor

ryanio commented Jan 22, 2020

It looks like there are two slow tests in ./tests/api/state/stateManager.js:

  1. should generate the genesis state correctly
  2. should generate correct genesis state root for all chains

As @s1na commented:

generateCanonicalGenesis puts ~8000 accounts to the trie. This causes the tests to time out (even 200s is not enough).

I couldn't find a timeout that works on my machine to successfully complete the tests. Commenting them out finishes the test suite without any errors.

My proposed solution is to add this code to the beginning of both those tests:

if (window.__karma__) {
  st.skip('skip slow test when running in karma');
  return st.end();
}

Let me know if you'd like for me to open a PR with the above.

@evertonfraga
Copy link
Contributor

That’s great, @ryanio!

Can you ensure it works with a low amount of accounts, like 800?

@evertonfraga
Copy link
Contributor

Doing that of course will generate a different state root hash, might not be worth the trouble.

@holgerd77
Copy link
Member Author

I think skipping is easiest, might not be worth the effort, not sure. @ryanio eventually you can do some sort experiment in it but otherwise implement your solution?

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