-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
soltest pass for EOF #15552
soltest pass for EOF #15552
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.
LGTM, but have you tested with EOF version 1?
@@ -36,12 +36,44 @@ set -e | |||
|
|||
OPTIMIZE=${OPTIMIZE:-"0"} | |||
EVM=${EVM:-"invalid"} | |||
EOF_VERSION=${EOF_VERSION:-0} |
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.
This change actually made me wonder if we should also consider changing OPTIMIZE and ABI_ENCODER_V1 to integers, even though they are used as boolean values.
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.
Would not hurt. The variables should also be quoted. And we should validate the values - currently anything other than 1
means "optimizer disabled", even "true"
.
I'm going to leave it as is here though. There were just too many small things like this in the script that could be improved and I didn't want to get sidetracked by cleaning it up :)
Oh, my mistake, I see it in the |
Yeah, it's now running for EOF v1 too. That was the whole point of the PR :) |
We already have some tests for EOF, but the whole test suite doesn't pass yet, so we were not running them in CI. This often results in failing tests not being noticed in EOF PRs.
This PR adds a soltest pass with
--eof-version 1
in CI but with a temporary blacklist for tests that are not expected to pass yet.Note that syntax and semantic tests are not blacklisted. They all pass because for them (and any other tests based on
EVMVersionRestrictedTestCase
) EOF is excluded unless explicitly requested viaEOFVersion: >=EOFv1
setting. This will allow us to enable them on a case-by-case basis for now and only when all of them are ready we'll flip the default to include EOF.