On-demand testing for VM State and Blockchain #951
Conversation
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
evertonfraga
requested review from
ryanio,
cgewecke,
holgerd77,
jochem-brouwer and
rumkin
evertonfraga
added
the
type: test all hardforks
evertonfraga
force-pushed
the
ci/vm-tests-on-demand
branch
from
161e47f to
c13ce86
Compare
| - run: npm run test:blockchain -- ${{ matrix.args }} | ||
|
|
||
| vm-blockchain-extended: | ||
| if: contains(join(github.event.pull_request.labels.*.name, ' '), 'Test all hardforks') |
There was a problem hiding this comment.
I could not use the entire label name type: Test all hardforks, because YAML libraries have trouble with character escaping. The colon was interpreted as a regular key: value, resulting in syntax errors. 🤷♂️
There was a problem hiding this comment.
interesting, ok, good to know! and I noticed the label has a lowercase t for test.
| # '--fork=Petersburg --dir=GeneralStateTests/stTimeConsuming --expected-test-amount=15561', | ||
| # '--fork=Petersburg --excludeDir=stTimeConsuming --expected-test-amount=17174', |
There was a problem hiding this comment.
did you mean to keep these commented out here, or should they be removed?
There was a problem hiding this comment.
@ryanio they are OK to be removed. In fact, they were just moved to these lines: https://github.com/ethereumjs/ethereumjs-vm/pull/951/files/c13ce861a06ce6969e59ede971db755dc2490734#diff-8ac46f60fffb24cf4daf7b192aee8bcd313d6fba95b005aa644e40600739a713R173-R179
|
love it, this is a really cool and elegant solution. should we add some docs to readme or developer.md about this feature? another good place for this info could be a pull request template. |
@ryanio those are great ideas :) |
|
Ah, there we have it with the tests breaking along the move to the latest So I think I would revise my statement from there and rather pledge that we go back for now to the safer way and fixing the |
|
@holgerd77 here it is: #953 (comment) |
holgerd77
force-pushed
the
ci/vm-tests-on-demand
branch
from
16274c4 to
3fb8eb5
Compare
There was a problem hiding this comment.
Ok, I've made the submodule fixes discussed with @evertonfraga (go back to a ethereumjs-testing v1.3.3 submodule commit state, move ethereum-tests to packages, some fixes along), test now pass, thanks Everton, this is a really great addition 😀 , will merge.
On the ethereum/tests update question we still need to minimally introduce some kind of documentation here or some process. The optimal way would be to directly have the ethereum/tests releases come more frequently, I might suggest over there a new process with me doing the releases there on a more frequent basis and in parallel to our updates here. Not sure if this suggestion will get positive perception, we'll see. 😄
This is the implementation of regular and extended VM State and Blockchain tests. It took lots of research and testing to have a concise code. The focus is on maintainability :)
On an ordinary PR, these tests will be fired:
But
vm-state-extendedandvm-blockchain-extendedwill be skipped.All workflows running after adding the special label
type: test all hardforkswill have tests for all hardforks. Analogously, if the label is removed, the extended tests will not run anymore.How it can be improved in the future:
vm-state-extendedandvm-blockchain-extendedon the checks start to bother you guys, there's an alternative: create one job with a script that outputs the matrix variables, just like I did with thenode-versions.ymlbuild file. The downside is to have more code to take care of, something I am always avoiding.A run without the label.
A run with the
type: test all hardforkslabel.🙃