-
Notifications
You must be signed in to change notification settings - Fork 374
fix(tests): Remove BAL checks in osaka; opt for eip7928 branch #1737
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
base: forks/osaka
Are you sure you want to change the base?
fix(tests): Remove BAL checks in osaka; opt for eip7928 branch #1737
Conversation
33d6cb6 to
bc6598e
Compare
bc6598e to
68cdfa4
Compare
- Remove BAL-specific checks in Osaka so we have a cleaner implementation of Amsterdam when re-creating. This was a remnant of working on tests and specs separately but then merging the repos together implementation ``forks/osaka`` as the base branch.
68cdfa4 to
8204ed4
Compare
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## forks/osaka #1737 +/- ##
============================================
Coverage 86.08% 86.08%
============================================
Files 743 743
Lines 44072 44072
Branches 3891 3891
============================================
Hits 37938 37938
Misses 5656 5656
Partials 478 478
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks @fselmo , i checkout this branch and create the |
marioevz
left a comment
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. Although it seemed to me that there must be a way for these changes to exist in forks/osaka without affecting test filling, because the BAL code should be completely inactive when filling for earlier forks.
🗒️ Description
Working on BAL tests and specs and then merging EEST into EELS with
forks/osakaas the base branch made it so that Osaka inherited the BAL changes. We develop EIPs in separate branches targeted toward the appropriate forks they should be in. So that others can work on top offorks/amsterdamwithout BAL support, some of these checks should be turned off so that they don't get validation issues. These removals will need to be added intoeips/amsterdam/eip-7928(BAL feature working branch) to turn these back on. I will PR the changes to add these checks back in there.I didn't turn everything off as that would be a very large sweeping change, but this should get others unstuck from the constraints related to BAL checks. cc: @LouisTsai-Csie if you can check that this works for your benchmarking branch, that would be great.
What I did to check this is to cherry pick this change to
forks/amsterdamlocally and ranuv run fill --clean --fork=Amsterdamand this seems to resolve all the fails that were happening related to BAL checks. Hopefully this is the minimal changeset required to get others unstuck.Note: This will require re-creating
forks/amsterdamfromforks/osakaand I can handle this as well once this is approved and merged.✅ Checklist
toxchecks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:uvx tox -e statictype(scope):.Cute Animal Picture