Skip to content

Remove async dependency / Test runner code cleanup #759

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

Closed
holgerd77 opened this issue Jun 3, 2020 · 4 comments
Closed

Remove async dependency / Test runner code cleanup #759

holgerd77 opened this issue Jun 3, 2020 · 4 comments

Comments

@holgerd77
Copy link
Member

The VM still uses the async library in the production code in runBlockchain.ts as well as in the test runner files (see also Developer Documentation on test run information).

These occurrences should be replaced by native async code structures, so that the dependency can be removed from the library.

(likely also be able to extend this issue to other libraries, not sure about the usage there as well, but likely it is still used there as well)

Might also be a good occasion to do some further clean-up of the test runner code - if apparent - while working on this.

@jochem-brouwer
Copy link
Member

It looks like some tests might succeed preliminary (i.e. https://github.com/ethereumjs/ethereumjs-vm/blob/61c3dcaeeb8152f48f023c803aabbc79392ba208/packages/vm/tests/api/state/stateManager.js#L166). I'll fix these tests if they cover the async library. I think cleaning all tests (and checking if the tests are correct) should be a different issue @holgerd77.

CC @evertonfraga

@evertonfraga
Copy link
Contributor

That makes sense.

This particular case is recent and haven't yet made to production, I am grateful that you caught it before it hits npm :)

To prevent these false positive cases, at least for the async scenario, we can replace test.end() by test.plan(x).

@jochem-brouwer
Copy link
Member

(Writing some notes for myself here but feel free to comment)

I have edited the blockchain package tests which use anything of the async library. I have included test.plan for these tests to explicitly check they are (still) correct (and did not run into failing tests =))

@ryanio
Copy link
Contributor

ryanio commented Aug 18, 2020

completed in #779

@ryanio ryanio closed this as completed Aug 18, 2020
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

4 participants