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.

Sign up for GitHub

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

Jump to bottom

Refactor sync committee tests #2554

Merged
merged 4 commits into from
Aug 18, 2021
Merged

Refactor sync committee tests #2554

merged 4 commits into from
Aug 18, 2021

Conversation

ralexstokes
Copy link
Member

@ralexstokes ralexstokes commented Aug 16, 2021 •
edited
Loading

This PR refactors the sync committee operation tests so that:

  1. Rewards verification applies to all sync committee tests
    • there are multiple entries to running the spec processing handler and not all of them verified rewards (e.g. run_successful_sync_committee_test vs run_sync_committee_processing
    • fix by: consolidate test runners into helper code and target all into one "driver" function
  2. Tests are generated with randomized variants of SyncAggregate
    • we have randomized versions with other objects and part of this PR was created during an audit of the existing tests and new tests in Altair
    • fix by: pulling out "randomized" tests into separate file, following the pattern in other parts of the test suite (e.g. test_leak_basic.py vs test_leak_random.py)
    • this portion is a refactoring to align w/ the rest of the code organization used throughout the repo

djrtwo
djrtwo approved these changes Aug 17, 2021
View reviewed changes
Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

looks good!

tests/core/pyspec/eth2spec/test/altair/block_processing/test_process_sync_aggregate_random.py Outdated Show resolved Hide resolved
@ralexstokes
Copy link
Member Author

NOTE: there are 27 spec tests generated from the operations/sync_aggregate files and I confirmed that 27 are still generated after this refactor (and extended the test gen diagnostics to include this info)

ralexstokes
ralexstokes commented Aug 18, 2021
View reviewed changes
...e/pyspec/eth2spec/test/altair/block_processing/sync_aggregate/test_process_sync_aggregate.py
@@ -502,150 +406,3 @@ def test_proposer_in_committee_with_participation(spec, state):
else:
state_transition_and_sign_block(spec, state, block)
raise AssertionError("failed to find a proposer in the sync committee set; check test setup")

Copy link
Member Author

Choose a reason for hiding this comment

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

the deletions from here and below are simply moved into the ..._random.py file added

@ralexstokes ralexstokes changed the title Refactor sync committee tests Refactor + extend sync committee tests Aug 18, 2021
@ralexstokes ralexstokes marked this pull request as ready for review August 18, 2021 17:05
@ralexstokes ralexstokes changed the title Refactor + extend sync committee tests Refactor sync committee tests Aug 18, 2021
@djrtwo djrtwo merged commit f6aa54b into ethereum:dev Aug 18, 2021
@ralexstokes ralexstokes deleted the refactor-sync-committee-tests branch August 19, 2021 12:53
@ralexstokes ralexstokes mentioned this pull request Aug 24, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djrtwo djrtwo djrtwo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ralexstokes @djrtwo