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

Refactor feature testing for spec tests #6737

Open
wants to merge 3 commits into
base: unstable
Choose a base branch
from

Conversation

jimmygchen
Copy link
Member

@jimmygchen jimmygchen commented Dec 20, 2024

Proposed Changes

  • Simplify logic to set up feature tests in preparation for new PeerDAS test updates, and supporting testing multiple features.
  • Add docs to FeatureName usage.

Additional Details

Removed previously hard coded EIP7594_* constants, and specify the feature tests at the handler level, this is more flexible, and allows for multiple features to be developed in parallel. For example:

impl<T, E> Handler for SszStaticHandler<T, E>
where
    T: cases::SszStaticType + tree_hash::TreeHash + ssz::Decode + TypeName,
    E: TypeName,
{
    fn is_enabled_for_feature(&self, feature_name: FeatureName) -> bool {
        feature_name == FeatureName::Eip7594
    }
}

When running a test for a feature, tests used to be written this way:

SszStaticHandler::<DataColumnSidecar<MainnetEthSpec>, MainnetEthSpec>::deneb_only()
    .run_for_feature(ForkName::Deneb, FeatureName::Eip7594);

This is quite verbose and may lead to inconsistency between tests. This has been modified to specify the fork at the FeatureName level, so the fork used is only specified once in the feature_name.fork_name() function.

SszStaticHandler::<DataColumnSidecar<MainnetEthSpec>, MainnetEthSpec>::deneb_only()
    .run_for_feature(FeatureName::Eip7594);

@jimmygchen jimmygchen added test improvement Improve tests work-in-progress PR is a work-in-progress labels Dec 20, 2024
@jimmygchen jimmygchen changed the base branch from stable to unstable December 20, 2024 01:10
@jimmygchen jimmygchen force-pushed the refactor-ef-tests-features branch from 6078560 to b43440f Compare December 20, 2024 01:10
@jimmygchen jimmygchen force-pushed the refactor-ef-tests-features branch from b43440f to aa593cf Compare December 20, 2024 06:32
@jimmygchen jimmygchen added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Dec 24, 2024
@jimmygchen jimmygchen marked this pull request as ready for review December 24, 2024 03:41
@jimmygchen
Copy link
Member Author

@michaelsproul @pawanjay176 would you mind reviewing this since you're both working on spec tests on electra? Thanks!

@jimmygchen
Copy link
Member Author

FYI I'm working on the PeerDAS spec tests for alpha.10 as part of #6736.

jimmygchen added a commit to jimmygchen/lighthouse that referenced this pull request Dec 24, 2024
Squashed commit of the following:

commit 898d05e
Merge: ffbd25e 7e0cdde
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Tue Dec 24 14:41:19 2024 +1100

    Merge branch 'unstable' into refactor-ef-tests-features

commit ffbd25e
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Tue Dec 24 14:40:38 2024 +1100

    Fix `SszStatic` tests for PeerDAS: exclude eip7594 test vectors when testing Electra types.

commit aa593cf
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Fri Dec 20 12:08:54 2024 +1100

    Refactor spec testing for features and simplify usage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-review The code is ready for review test improvement Improve tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant