-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
Fix tests with byzantium and homestead #15631
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not wrong, but I think we'd be better off just skipping the test on unsupported versions. No need to run it multiple times if the parameters don't change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard-coding the EVM and EOF version will not prevent the test case from being executed for all combinations, it will just make it run with the same settings every time. What we should do instead is to skip it in unsupported configurations.
One way to do this would be to check versions in the constructor and set TestCase::m_shouldRun
to false
for unsupported ones.
Another would be to set EVMVersion: =cancun
in test cases. This is IMO something we should be doing in general because it gives us the most flexibility. All tests should be running in all configurations by default and need to be restricted explicitly as necessary. Especially in the context of EOF, this approach lets us keep it mostly disabled but still lets us enable it on a case-by-case basis. There are only ~45 cases in this suite so I think this is easily doable and I think we should go with this option.
BTW, perhaps we should think about introducing current
as a valid value for EVMVersion
to let us select a single version without hard-coding it. This would make this approach easier to reconcile with EVM version updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea with current
. For now (and for the purpose of this PR) I have set the m_shouldRun
flag, but we should do that in a follow up, imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
6a26b71
to
8522c28
Compare
It is guarded by dialect.evmVersion().hasBitwiseShifting().
8522c28
to
394dd92
Compare
dialect.evmVersion().hasBitwiseShifting()
.EVMCodeTransformTest
s use defaulted evm dialect instead of test-configured one: there was a mismatch between the dialect configured in the test'sYulStack
and what was used in the test'sEVMObjectCompiler
, one using the defaulted (latest) version and one the test-configured one, respectively. Also some of the code generation tests fail forbyzantium
andhomestead
if configured with the same dialect. So for now I have reverted back to default. (@cameel @rodiazet)