chore(test-cli): check EIP reference versions up to Amsterdam - #3592
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## forks/amsterdam #3592 +/- ##
================================================
Coverage 94.44% 94.44%
================================================
Files 624 624
Lines 36928 36928
Branches 3326 3326
================================================
Hits 34875 34875
Misses 1450 1450
Partials 603 603
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
danceratopz
left a comment
danceratopz
left a comment
There was a problem hiding this comment.
Thanks for catching this! Yes, in execution-spec-tests we had the Check EIP Versions action which ran weekly and generated these issues.
They became an annoyance. I think the main reason they were so annoying was the frequency of changes that weren't spec-bearing, so you would just create a pr to silence the action. Perhaps monthly would be a better tact, or, even better, update the CLI too to actually perform the change and create a PR instead of an issue. This could potentially trigger a rebase though if the hashes across the automated and other test pr both change (but not to the same hash); with this in mind, perhaps weekly is better after all.
Perhaps our multibranch (eips/*) world will protect us from too much noise if we only run it on the default development fork, but then it has less impact?
One comment below, but this is definitely and improvement for now! 🙂
Yeah, this isn't a great idea if the changes are actually spec-bearing though! 😅 gn! |
Took this as a one-source-of-truth suggestion and applied it bc I like it 😅. But I put the default in the checker plugin rather than the Justfile. A plain The plugin now defaults to every fork in the fork registry, including forks under development, so bonus: I also removed the "Automated Checks via GitHub Actions" docs section, which described a daily workflow that is nonexistent? 👀 will ask for re-review 👀 |
`pytest_collection_modifyitems` only inserted an `EIPSpecTestItem` for items whose direct parent is a `Module`. Tests declared inside a class have the class as their parent, so those modules' reference specs were never checked at all. Walk up the collection tree with `getparent()` instead. This recovers three modules that declare a reference spec but were silently skipped: `amsterdam/eip7976_increase_calldata_floor_cost` (two modules) and `prague/eip7623_increase_calldata_cost`.
The help output only listed `--github-token`, so there was no indication
that collection can be narrowed to a directory or module, even though the
docs describe doing so.
`show_specific_help` builds its own parser, so add an optional positional
to it and enable it for `check_eip_versions`. The usage line now reads:
usage: check_eip_versions [-h] [--github-token GITHUB_TOKEN]
[<test-path> ...]
danceratopz
left a comment
danceratopz
left a comment
There was a problem hiding this comment.
Thanks @fselmo! This is much nicer! I made two small follow-up commits directly to your branch, one fix for module collection and one CLI help output improvement.
About this:
The plugin now defaults to every fork in the fork registry, including forks under development, so
uv run check_eip_versionsandjust check-eip-versions-collectcheck the same range and nothing needs bumping when a fork is added.--forkand--untilstill work as before. As a side effect,--forkworks again! lol. It used to error because the CLI always added--untiland they would clash.
I only realized this during the second review that while these flags do correspond to the fill behavior, they might not do what a user expects here (i.e., only check Amsterdam EIP versions if --fork=Amsterdam). They will limit the checks, but only to all tests until Amsterdam.
So if you really only want to check Amsterdam tests, you need a positional argument that specifies the test folder, just like for fill. I added this to the CLI help output in a commit directly to this branch to nudge users in the right direction.
While we dug deeper in the review, Claude noticed that some modules were ignored, so I had him fix this up (it's a two line fix). Hope you don't mind me pushing this upfront!
|
Thanks @danceratopz! No qualms at all with you pushing. I'm a big proponent if you've done the work locally with AI or what-have-you, then pushing it and reviewing / changing makes things go faster. If the work is already done, no need to comment it – just push and we can still discuss it. I like the changes 👍🏼... I just refactored both tests here to be in the same file since they have some shared setup. Nothing else. Will merge when CI passes. |
2 participants
Description
Every local
check_eip_versionsrun and the CI collect job skipped all Amsterdam suites. This was leading to some bad hashes (commit sha instead of blob sha) passing by blindly. This updates the CI job to run these checks--until Amsterdam.The only CI job, though, runs
--collect-onlyand asserts a count floor. It never executes the version assertions, for any fork. A stale pin still won't fail CI after this... but any local checks would catch this. Perhaps this is something we should think about, but I think we may have had this at some point, posting to an issue? (I think this was before my time in STEEL), and it produced way too much noise? @danceratopz am I remembering this correctly?Related Issues or PRs
N/A.
Checklist
just static<type>(<area>): <title>, where<type>and<area>come from an appropriateC-<type>, respectivelyA-<area>, label. The title should match the target squash commit message.Cute Animal Picture