tests(app): 😌 deduplicate staking test boilerplate #4173
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
this is some common glue code in integration tests for the staking component. in order to smooth the inspection of component logic that runs at epoch boundaries, the defaults for `AppState`'s inner settings for e.g. epoch duration are overriden to shorter values. as is, this requres stating object literals that are somewhat verbose, even with `..T::default()` elision. this commit adds some small `with_` helpers to allow test cases to manipulate these settings succinctly.
this adds an extension method that can be used to fast forward to the next epoch. this is a very common part of test logic that exercises the staking component. this allows test cases to invoke an extension method to jump ahead, without needing to import the clock component and write a loop inspecting the current epoch after creating and executing an empty block.
this adds a `ValidatorDataRead::validator_identity_keys()` trait method. frequently, test logic needs to keep track of validators by their identity keys. this led to a bit of common glue code that would get the latest snapshot, the set of all validator definitions, assert that only one validator is present, and then take the identity key of that single validator. now we can just get the keys, bind that single entry to a variable, or return an error if something went wrong.
cratelyn
added
A-staking
cratelyn
force-pushed
the
kate/staking-test-utilities
branch
from
19169b5 to
1c5b90a
Compare
cratelyn
commented
Apr 8, 2024
Comment on lines
-61
to
+64
| let app_state = AppState::Content(penumbra_app::genesis::Content { | ||
| sct_content: penumbra_sct::genesis::Content { | ||
| sct_params: penumbra_sct::params::SctParameters { | ||
| epoch_duration: EPOCH_DURATION, | ||
| }, | ||
| }, | ||
| stake_content: penumbra_stake::genesis::Content { | ||
| stake_params: penumbra_stake::params::StakeParameters { | ||
| unbonding_delay: UNBONDING_DELAY, | ||
| ..Default::default() | ||
| }, | ||
| ..Default::default() | ||
| }, | ||
| ..Default::default() | ||
| }); | ||
| let app_state = AppState::Content( | ||
| genesis::Content::default() | ||
| .with_epoch_duration(EPOCH_DURATION) | ||
| .with_unbonding_delay(UNBONDING_DELAY), | ||
| ); |
There was a problem hiding this comment.
a nice example of these methods simplifying test setup.
Comment on lines
-76
to
+78
| // Retrieve the validator definition from the latest snapshot. | ||
| let existing_validator_id = { | ||
| use penumbra_stake::component::validator_handler::validator_store::ValidatorDataRead; | ||
| let validators = &storage | ||
| .latest_snapshot() | ||
| .validator_definitions() | ||
| .tap(|_| info!("getting validator definitions")) | ||
| .await?; | ||
| match validators.as_slice() { | ||
| [Validator { identity_key, .. }] => *identity_key, | ||
| unexpected => panic!("there should be one validator, got: {unexpected:?}"), | ||
| } | ||
| }; | ||
| let [existing_validator_id] = storage | ||
| .latest_snapshot() | ||
| .validator_identity_keys() | ||
| .await? | ||
| .try_into() | ||
| .map_err(|keys| anyhow::anyhow!("expected one key, got: {keys:?}"))?; |
There was a problem hiding this comment.
an example of the benefit of being able to request a Vec<IdentityKey> out of the temporary storage's latest snapshot.
Comment on lines
+230
to
-252
| node.fast_forward_to_next_epoch(&storage).await?; | ||
|
|
||
| // The new validator should now be in the consensus set. | ||
| { | ||
| let get_epoch = || async { storage.latest_snapshot().get_current_epoch().await }; | ||
| let start = get_epoch() | ||
| .await? | ||
| .tap(|start| tracing::info!(?start, "fast forwarding to next epoch")); | ||
| let next = loop { | ||
| node.block().execute().await?; | ||
| let current = get_epoch().await?; | ||
| if current != start { | ||
| break current; | ||
| } | ||
| }; | ||
| tracing::info!(?start, ?next, "finished fast forwarding to next epoch"); |
There was a problem hiding this comment.
an example of the benefit of fast_forward_to_next_epoch.
erwanor
approved these changes
Apr 8, 2024
crates/core/component/stake/src/component/validator_handler/validator_store.rs
Show resolved
Hide resolved
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
this branch adds some small tweaks to cut down on common boilerplate code found
in many of the staking component tests. this allows us to (a) easily set the
epoch duration, (b) fast forward to the end of an epoch, and (c) obtain the
identity keys of the currently defined validators.
changes
f3ecf6d tests(app): 🔶 add genesis content setters
this is some common glue code in integration tests for the staking component.
in order to smooth the inspection of component logic that runs at epoch
boundaries, the defaults for
AppState's inner settings for e.g. epochduration are overriden to shorter values.
as is, this requres stating object literals that are somewhat verbose, even
with
..T::default()elision. this commit adds some smallwith_helpers toallow test cases to manipulate these settings succinctly.
bf8f4d1 tests(app): 🏃 add fast forward to next epoch helper
this adds an extension method that can be used to fast forward to the next
epoch. this is a very common part of test logic that exercises the staking
component.
this allows test cases to invoke an extension method to jump ahead, without
needing to import the clock component and write a loop inspecting the current
epoch after creating and executing an empty block.
1c5b90a tests(app): 🔑 use
validator_identity_keysaccessorthis adds a
ValidatorDataRead::validator_identity_keys()trait method.frequently, test logic needs to keep track of validators by their identity
keys. this led to a bit of common glue code that would get the latest snapshot,
the set of all validator definitions, assert that only one validator is
present, and then take the identity key of that single validator.
now we can just get the keys, bind that single entry to a variable, or return
an error if something went wrong.
checklist before requesting a review
"consensus-breaking" label. Otherwise, I declare my belief that there are not
consensus-breaking changes, for the following reason: