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

Test case naming clean up #3143

Merged
merged 12 commits into from
Dec 13, 2022
Merged

Test case naming clean up #3143

merged 12 commits into from
Dec 13, 2022

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented Nov 30, 2022

Address #3118

Comment on lines +261 to +264
def description(case_description: str):
def entry(fn):
return with_meta_tags({'description': case_description})(fn)
return entry
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not in use in this PR, but it can be utilized to add notes to the test vector results in future PRs.

@hwwhww hwwhww marked this pull request as ready for review December 6, 2022 15:27
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.

wow this is amazing! nice work :)

I left a few comments but generally looks very good, thanks for doing this!

Comment on lines +14 to +22
_new_altair_mods = {
**{'sync_aggregate': [
'eth2spec.test.altair.block_processing.sync_aggregate.test_process_' + key
for key in ['sync_aggregate', 'sync_aggregate_random']
]},
**{key: 'eth2spec.test.altair.block_processing.test_process_' + key for key in [
'deposit',
]}
}
Copy link
Member

Choose a reason for hiding this comment

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

this is outside the scope of this PR but it would be very nice to have some kind of linter that would take the spec and test code and ensure we didn't miss anything...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. 😭
And it would be nice to auto-generate the mods or with more clever settings.

@with_altair_and_later
@spec_state_test
@always_bls
def test_ineffective_deposit_with_bad_fork_version_and(spec, state):
Copy link
Member

Choose a reason for hiding this comment

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

test_ineffective_deposit_with_bad_fork_version_and

... and what? :)

)


@with_phases([ALTAIR])
Copy link
Member

Choose a reason for hiding this comment

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

can this be "altair and later"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this is the control group of bellatrix/block_processing/test_process_deposit.py::test_ineffective_deposit_with_previous_fork_version. Altair's previous fork is exactly the correct GENESIS_FORK_VERSION while others' are not.

Added more comments (34907d2) and current_fork_version test case (e5709a5).

@hwwhww
Copy link
Contributor Author

hwwhww commented Dec 8, 2022

@ralexstokes thanks for the reviews! 😊
I think I addressed all the PR feedback.

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.

wow! very nice.

@hwwhww hwwhww merged commit da3f5af into dev Dec 13, 2022
@hwwhww hwwhww deleted the test-cases-clean-up branch December 13, 2022 07: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.

None yet

3 participants