STEEL Merge #1280
STEEL Merge #1280
Conversation
gurukamath
force-pushed
the
steel-merge
branch
7 times, most recently
from
8986393 to
ed3dae9
Compare
|
The PR is ready for review. The CI fails after around 30 mins because it looks like the runner is terminating the job (there aren't any actual test failures). Perhaps contacting devops is required here but I'm gonna take a quick look first. |
setup.cfg
Outdated
| @@ -136,6 +136,8 @@ install_requires = | |||
| ethereum-types>=0.2.1,<0.3 | |||
| ethereum-rlp>=0.1.4,<0.2 | |||
| cryptography>=45.0.1,<46 | |||
| ethereum-execution-spec-tests @ git+https://github.com/ethereum/execution-spec-tests@dd83fba10492e6f63453b1a35608900b3aee1a87 | |||
| ethereum-spec-evm-resolver @ git+https://github.com/petertdavies/ethereum-spec-evm-resolver | |||
There was a problem hiding this comment.
We still need the resolver?
There was a problem hiding this comment.
We don't in the long run. But the idea was to have a bridge-over period where the tests folder from EEST would be made a sub-module in EELS and the functionality to fill via resolver would be retained. Once we are past this phase however, we can move the tests fully into EELS and get rid of the resolver. Here is more context on this
| t8n = T8N(t8n_options, out_stream, in_stream) | ||
| t8n.run() | ||
|
|
||
| output_dict = json.loads(out_stream.getvalue()) |
There was a problem hiding this comment.
We really should make this interface less... insane 🤣
There was a problem hiding this comment.
I agree. For EELS, we can make this much more simple and native. However, EEST has to have this interface to be able to work with the other clients.
For EELS, this PR is only the first step to make the merge possible. There are a series of optimizations that we can undertake after this.
There was a problem hiding this comment.
I mean specifically our t8n interface. We don't need a JSON round trip here.
There was a problem hiding this comment.
Ah. I see. Yes. That is definitely the long term plan
|
Have incorporated some of the review comments and re-based on the latest |
gurukamath
force-pushed
the
steel-merge
branch
5 times, most recently
from
2141f39 to
06aeac2
Compare
|
Summary of a discussion with @SamWilsn A second command has been added in this PR for the fork under active development. See this. This is because this command needs to only focus on the active development fork folder. Otherwise some of the historical tests will fail since they are not updated in real time. For example, Osaka tests need to only focus on Since this is going to be a general thing, we should make an ethereum-spec-print-dev-fork command so we can single source hardfork information. We don't want to end up skipping amsterdam's tests for example because we forgot to update |
| -n auto --maxprocesses 3 \ | ||
| --basetemp="{temp_dir}/pytest" \ | ||
| --clean \ | ||
| --fork Osaka \ |
There was a problem hiding this comment.
As mentioned on Discord, we should single source fork information.
|
Looks good to merge! |
What was wrong?
Currently, the test cases live in a different repository (
execution-spec-tests).How was it fixed?
Move the test cases to
execution-specsand start usingexecution-spec-testsas a library.Related EEST PR
Cute Animal Picture