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

Integrate ethereumjs testing logic #808

Merged
merged 6 commits into from Aug 5, 2020

Conversation

holgerd77
Copy link
Member

This PR integrates the code logic from ethereumjs-testing into the VM test runner, see ethereumjs/ethereumjs-testing#48 for reasoning.

It also fixes the file permissions for the state-test-run-test.sh test runner state test config test file (before it was not executable) and adds a new initialization and test runner start output box to make configuration options and test running conditions more transparent, the following are some output examples:

$ node -r ts-node/register --stack-size=1500 ./tests/tester --blockchain --fork=spuriousDragon
+--------------------------------------------------+
| VM -> BlockchainTests                            |
|                                                  |
| TestGetterArgs                                   |
| skipTests           : 54                         |
| forkConfig          : EIP158                     |
|                                                  |
| RunnerArgs                                       |
| forkConfigVM        : spuriousDragon             |
| forkConfigTestSuite : EIP158                     |
+--------------------------------------------------+

$ ts-node ./tests/tester --state --test=CreateCollisionToEmpty --jsontrace
+--------------------------------------------------+
| VM -> GeneralStateTests                          |
|                                                  |
| TestGetterArgs                                   |
| skipTests           : 54                         |
| forkConfig          : Istanbul                   |
| test                : CreateCollisionToEmpty     |
|                                                  |
| RunnerArgs                                       |
| forkConfigVM        : istanbul                   |
| forkConfigTestSuite : Istanbul                   |
| jsontrace           : true                       |
+--------------------------------------------------+

A useful follow-up PR on this would be to actually rework the integrated code and use a more modern async directory traversal logic like e.g. outline here on StackOverflow.

@codecov
Copy link

codecov bot commented Jul 9, 2020

Codecov Report

Merging #808 into master will decrease coverage by 1.01%.
The diff coverage is n/a.

Impacted file tree graph

Flag Coverage Δ
#account 92.85% <ø> (ø)
#block 79.97% <ø> (+8.21%) ⬆️
#blockchain 84.50% <ø> (?)
#common 93.98% <ø> (ø)
#ethash 86.30% <ø> (ø)
#tx 94.02% <ø> (-0.14%) ⬇️
#vm 93.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@evertonfraga
Copy link
Contributor

I guess this is blocked until we find out how to reuse this logic on other packages. Putting a label so it doesn't risk being merged for now. :)

@holgerd77
Copy link
Member Author

@evertonfraga ah no it is not, the VM is the only consumer of this

@holgerd77
Copy link
Member Author

@evertonfraga ah no, you're right, sorry. Have to rethink about this.

@holgerd77
Copy link
Member Author

After letting this sink in for a couple of days I still wonder if we should give up on this code being on the ethereumjs-testing library and rather move to the libraries which need access to the tests submodule. Most parameters on the ethereumjs-testing functions are customized to be used for the VM anyhow. If we would integrate the there used functionality into the tx library e.g. code logic would likely be a lot simpler, especially with more modern async code structures for reading directories and files. So this will likely not cause that much code redundancy. And on most libraries where this code was once used we already switched to static test fixtures copied over from ethereum/tests anyhow - see e.g. this PR ethereumjs/ethereumjs-block#61 - to avoid the heavy load of the submodule and with the reasoning that tests apart from the VM tests over on ethereum/tests very rarely change (often no change in years).

On the VM side we would have a much tighter integration and the unnatural code separation would fall away (currently no one is really overly motivated to do dedicated ethereumjs-testing releases just for some code changes) and there is a good chance that this will directly benefit our CI/test performance efforts, since then we can then experiment a lot easier with different code structures on test run parallelization.

Thoughts?

Likely not be able to further participate in the discussion (holidays), but if there is an agreement maybe just merge here as a starting point - then one can further improve on the code directly on the VM - and we can - for now - just leave the code as well in the ethereumjs-testing repository.

@jochem-brouwer
Copy link
Member

I agree with moving this logic to the VM, see also this comment.

@holgerd77
Copy link
Member Author

holgerd77 commented Aug 3, 2020

@evertonfraga @jochem-brouwer Since Jochem is also supporting this idea do we want to merge this in here so that Jochem can build upon the changes done here on his testing update work (mentioned above)?

I think we can also live with the situation to keep this in some hybrid state and have the ethereumjs-testing logic kept there for some releases if necessary.

Have solved the merge conflicts on this branch (nyc needed to go back to v12 from v14 along some PR change from Everton).

Copy link
Contributor

@ryanio ryanio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm!

@evertonfraga evertonfraga merged commit 51f8af0 into master Aug 5, 2020
@holgerd77 holgerd77 deleted the integrate-ethereumjs-testing-logic branch August 5, 2020 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants