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

support for accounts tests loading accounts with excluded feature #29142

Merged
merged 1 commit into from
Dec 8, 2022

Conversation

jeffwashington
Copy link
Contributor

@jeffwashington jeffwashington commented Dec 7, 2022

Problem

This allows us to test with and without the activation of the feature in #28690, which has to do with fixing rent_epoch at a consistent value for rent exempt accounts.

Summary of Changes

Introduce parameter for excluding features from activation in tests in accounts.rs.

Fixes #

@jeffwashington jeffwashington marked this pull request as ready for review December 7, 2022 22:10
Copy link
Contributor

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

I did something that sounds similar for the stake program to test out raise the minimum delegation amount. I ran basically every test with and without the feature.

The feature settings:

/// The "new" behavior enables all features
fn feature_set_new_behavior() -> FeatureSet {
FeatureSet::all_enabled()
}
/// The "old" behavior is before the stake minimum delegation was raised
fn feature_set_old_behavior() -> FeatureSet {
let mut feature_set = feature_set_new_behavior();
feature_set.deactivate(&feature_set::stake_raise_minimum_delegation_to_1_sol::id());
feature_set
}

The tests:

#[test_case(feature_set_old_behavior(); "old_behavior")]
#[test_case(feature_set_new_behavior(); "new_behavior")]
fn test_split_full_amount_minimum_stake_delegation(feature_set: FeatureSet) {

Comment on lines +1947 to +1948
let loaded_accounts =
load_accounts_with_excluded_features(tx, &accounts, &mut error_counters, None);
Copy link
Contributor

Choose a reason for hiding this comment

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

For all of these changes where the optional excluded features is None, what's the benefit of calling load_accounts_with_excluded_features() vs load_accounts()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because they're going to change soon, once there is a feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and then once the feature is activated, this can all go away again.

@jeffwashington
Copy link
Contributor Author

I did something that sounds similar for the stake program to test out raise the minimum delegation amount. I ran basically every test with and without the feature.

The feature settings:

/// The "new" behavior enables all features
fn feature_set_new_behavior() -> FeatureSet {
FeatureSet::all_enabled()
}
/// The "old" behavior is before the stake minimum delegation was raised
fn feature_set_old_behavior() -> FeatureSet {
let mut feature_set = feature_set_new_behavior();
feature_set.deactivate(&feature_set::stake_raise_minimum_delegation_to_1_sol::id());
feature_set
}

The tests:

#[test_case(feature_set_old_behavior(); "old_behavior")]
#[test_case(feature_set_new_behavior(); "new_behavior")]
fn test_split_full_amount_minimum_stake_delegation(feature_set: FeatureSet) {

I did something that sounds similar for the stake program to test out raise the minimum delegation amount. I ran basically every test with and without the feature.

The feature settings:

/// The "new" behavior enables all features
fn feature_set_new_behavior() -> FeatureSet {
FeatureSet::all_enabled()
}
/// The "old" behavior is before the stake minimum delegation was raised
fn feature_set_old_behavior() -> FeatureSet {
let mut feature_set = feature_set_new_behavior();
feature_set.deactivate(&feature_set::stake_raise_minimum_delegation_to_1_sol::id());
feature_set
}

The tests:

#[test_case(feature_set_old_behavior(); "old_behavior")]
#[test_case(feature_set_new_behavior(); "new_behavior")]
fn test_split_full_amount_minimum_stake_delegation(feature_set: FeatureSet) {

An issue is sometimes I can't deactivate it on the bank because at that point it already exists and its initial existence can change the behavior of the test. I have run into this with this feature.

Copy link
Contributor

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

lgtm

@jeffwashington jeffwashington merged commit ea19fe9 into solana-labs:master Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants