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

vm/tests: add support for test networks with activated EIPs #1617

Merged
merged 9 commits into from
Feb 1, 2022

Conversation

jochem-brouwer
Copy link
Member

@jochem-brouwer jochem-brouwer commented Dec 28, 2021

This PR adds support for ethereum/tests forks where there are added EIPs; these are usually experimental and it is not clear if they will ever be in an actual fork, but they have tests anyways.

To test;

cd packages/ethereum-tests
git checkout push0
npm run test:blockchain -- --fork=London+3855

Results in 12 passing tests, so looks like we pass the (draft) tests for EIP3855 as well 😄

@codecov
Copy link

codecov bot commented Dec 28, 2021

Codecov Report

Merging #1617 (0dbe980) into master (b224247) will decrease coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

Flag Coverage Δ
block 85.01% <ø> (ø)
blockchain 83.28% <ø> (ø)
client 71.22% <ø> (+0.09%) ⬆️
common 93.89% <ø> (ø)
devp2p 82.50% <ø> (ø)
ethash 90.76% <ø> (ø)
rlp ?
trie 86.18% <ø> (ø)
tx 89.94% <ø> (ø)
util 92.62% <ø> (ø)
vm 81.27% <ø> (ø)

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

@ryanio
Copy link
Contributor

ryanio commented Dec 28, 2021

There are a few leftover console.logs

@jochem-brouwer
Copy link
Member Author

Don't know why I did not notice that when linting 🤔 Removed them.

@ryanio
Copy link
Contributor

ryanio commented Dec 28, 2021

Thanks! I believe we have the no-console rule overridden to be ignored in tester.ts since we log some other important output. You can see that in packages/vm/.eslintrc.js

acolytec3
acolytec3 previously approved these changes Jan 6, 2022
Copy link
Contributor

@acolytec3 acolytec3 left a comment

Choose a reason for hiding this comment

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

I made one change to use common.custom instead of the deprecated common.forCustomChain but looks great otherwise! I tested with a couple of different hardfork combinations and it parsed them all correctly (and threw an error if trying to set an unsupported hardfork - as expected).

@holgerd77
Copy link
Member

Can we also add 1-2 lines on how to use this in the developer README somewhere here?

Also ok of course to somewhat restructure along if the current structure of the section is generally not so optimal or something.

@holgerd77
Copy link
Member

Can this be updated at some point?

@jochem-brouwer
Copy link
Member Author

I will update this before the end of this month.

@jochem-brouwer
Copy link
Member Author

Updated docs, thanks for the reviews 😄

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

LGTM

@holgerd77 holgerd77 merged commit 64b48b0 into master Feb 1, 2022
@holgerd77 holgerd77 deleted the tests3855 branch February 1, 2022 16:13
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.

4 participants