Skip to content
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

Make Phase0 -> Altair fork transition tests compatible with Altair -> Merge #2706

Merged
merged 7 commits into from
Nov 20, 2021

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented Nov 2, 2021

Issue

I really want to reuse test/altair/transition/ tests instead of duplicating it for Merge fork transition.

How did I fix it

High-level

  1. Add ForkMeta dataclass for passing pre_fork_name, post_fork_name and fork_epoch.
  2. Replace @fork_transition_test(PHASE0, ALTAIR, fork_epoch=2) with @with_fork_metas(Sequence[ForkMeta]).
  3. Replace do_altair_fork with do_fork, which handles both Altair and The Merge updates.

Decorators

  1. Modify build_transition_test into yield_fork_meta.
  2. Modify with_phases:
    • Refactoring:
      • Replace the if PHASE0: ... if ALTAIR: ... code with for-loop. The phases should be been limited with DEFAULT_PYTEST_FORKS.
    • Add for fork_meta in fork_metas loop to run test cases for each fork meta.

Other

  • Make test_transition_with_voluntary_exit_* only run with minimal presets.
  • Make transition test generator run with Merge.

Note

  • Run pytest with --fork=phase0 will get you Phase0 -> Altair fork transition; run pytest with --fork=altair will get you Altair -> Merge fork transition.

TODO

  • Verify the test vectors.

Copy link
Member

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still digesting but so far it looks great! one general comment is that this would be easier to review if it split apart the various concerns.

I see the following that could be N separate PRs:

  1. refactor of with_phases and additional typing in that file
  2. the extension of the fork transition tests to re-use them across multiple pairs of forks/phases

The with_phases refactor isn't huge but it is an important enough function to the integrity of the spec tests that changes to it should be very carefully reviewed! It would be a bit nicer to be able to consider it independently without the cognitive load of the other changes to the fork transition testing.

No worry on this PR but it is something to keep in mind in the future :)

Copy link
Member

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks great!

i like that we can reuse the fork transition tests across multiple pairs of forks.

i left some things below, lets address those and then i'll make another round of review... in particular, it would be good to see the tests actually generated, so we can see all of the new code exercised

tests/core/pyspec/eth2spec/test/context.py Outdated Show resolved Hide resolved
tests/core/pyspec/eth2spec/test/context.py Outdated Show resolved Hide resolved
tests/core/pyspec/eth2spec/test/context.py Outdated Show resolved Hide resolved
tests/generators/transition/main.py Outdated Show resolved Hide resolved
tests/core/pyspec/eth2spec/test/context.py Show resolved Hide resolved
Copy link
Member

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

ill verify if we have verified the test vectors, otherwise i think we can ship this

@hwwhww
Copy link
Contributor Author

hwwhww commented Nov 17, 2021

tarball files of the transition tests (only make gen_transition) at 63c9e5e (at current dev branch head commit: fb34e16):

mainnet.tar.gz
minimal.tar.gz

@hwwhww hwwhww merged commit 29beba6 into dev Nov 20, 2021
@hwwhww hwwhww deleted the transition-reuse branch December 9, 2021 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants