test: Add more sims test scenarios #23278
Conversation
📝 WalkthroughWalkthroughThe pull request introduces comprehensive modifications to the Cosmos SDK simulation framework, focusing on enhancing test configurations, updating type representations, and refining simulation testing capabilities. The changes span multiple files across the simulation infrastructure, including updates to test runners, configuration handling, and simulation parameters. Key modifications include transitioning from Changes
Suggested labels
Suggested reviewers
Possibly related PRs
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
alpe
force-pushed
the
alex/sims2_v2_dev_tests
branch
5 times, most recently
from
b55ebe5 to
1821bd2
Compare
alpe
force-pushed
the
alex/sims2_v2_dev_tests
branch
from
1821bd2 to
71c45a7
Compare
alpe
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
simsx/environment.go (1)
266-268: LGTM! Consider adding error documentation.The validation check for empty accounts is a good defensive programming practice. However, consider documenting this panic condition in the function's documentation comment to make it explicit for users of this constructor.
Add this documentation above the function:
// NewChainDataSource constructor +// Panics if oldSimAcc is empty or if any account has an empty bech32 address. func NewChainDataSource(simapp/v2/sim_test.go (2)
28-39: Externalize default seeds for better configurabilityThe default seeds in
TestAppStateDeterminismare hardcoded within the function. Consider externalizing these seeds to a configuration file or constants to enhance maintainability and allow for easier updates without modifying the code.
52-54: Enhance error message for non-determinism detectionIn the non-determinism check, consider including the expected and actual app hashes in the error message. This additional information can aid in debugging by showing the exact differences when non-determinism occurs.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
scripts/build/simulations.mk(3 hunks)simapp/v2/sim_runner.go(10 hunks)simapp/v2/sim_test.go(1 hunks)simsx/environment.go(1 hunks)simsx/runner.go(0 hunks)simsx/v2/valset_history.go(3 hunks)types/simulation/config.go(1 hunks)x/simulation/client/cli/flags.go(2 hunks)
💤 Files with no reviewable changes (1)
- simsx/runner.go
🧰 Additional context used
📓 Path-based instructions (6)
types/simulation/config.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/simulation/client/cli/flags.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
simapp/v2/sim_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
simsx/environment.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
simsx/v2/valset_history.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
simapp/v2/sim_runner.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (24)
simapp/v2/sim_test.go (3)
20-22: Initialization of simulator flags is appropriateThe
initfunction correctly initializes the simulator flags usingsimcli.GetSimulatorFlags(). This ensures that the simulator configuration is set before the tests run.
24-26: Test functionTestFullAppSimulationis properly definedThe
TestFullAppSimulationfunction usesRunWithSeedsto execute the full application simulation with the default seeds. This is appropriate for ensuring comprehensive simulation coverage.
70-102:TestAppSimulationAfterImportfunction is well-implementedThe
TestAppSimulationAfterImportfunction properly tests the simulation after importing the genesis state. It ensures that the application can export its state and re-import it correctly, which is crucial for state consistency.simapp/v2/sim_runner.go (7)
53-54: ExportedDefaultSeedsfor external useThe variable
DefaultSeedsis now exported, allowing it to be used in other packages. This change promotes reusability and consistency across simulations that rely on default seed values.
91-98: AddedInitGenesismethod toSimulationAppinterfaceThe addition of the
InitGenesismethod to theSimulationAppinterface enhances initialization capabilities. It allows for setting up the application state from a genesis file, which is essential for simulations that require a predefined state.
109-109: IncludedSeedfield inTestInstancestructAdding the
Seedfield toTestInstanceimproves traceability and reproducibility of simulation tests. It enables the seed value to be easily accessed throughout the test instance, facilitating debugging and analysis.
125-130: UpdatedSetupTestInstancefunction to accept seed parameterThe
SetupTestInstancefunction now accepts aseedparameter, which aids in creating deterministic test instances. This change enhances the ability to reproduce simulation scenarios using specific seeds.
200-205: RefactoredRunWithSeedsfor greater flexibilityBy modifying
RunWithSeedsto acceptappFactoryandappConfigFactory, the function now allows for customizable application setups. This refactoring increases flexibility and supports diverse simulation configurations.
254-262: IntroducedRunWithSeedXfor custom chain setupsThe new
RunWithSeedXfunction enables running simulations with custom chain state setups. This addition provides more control over simulation parameters and can accommodate specialized testing scenarios.
369-376: ExportedChainStatetype for broader accessibilityChanging
chainStatetoChainStateby exporting the type allows it to be used outside the package. This enhances the usability of the chain state structure in other parts of the application or in external packages.scripts/build/simulations.mk (8)
5-7: Updated simulation test path tosimapp/v2The
test-sim-nondeterminismtarget now correctly points to thesimapp/v2directory, reflecting the updated location of simulation tests. This ensures that the makefile executes the tests in the intended directory.
32-33: Removed outdated test target and parametersThe
test-sim-import-exporttarget has been commented out, indicating that it may no longer be applicable or that it requires updates to work withsimapp/v2. The removal of the-Periodparameter simplifies the command and aligns it with current test configurations.
36-38: Adjustedtest-sim-after-importto usesimapp/v2The
test-sim-after-importtarget now uses thesimapp/v2directory and removes the-Periodparameter. This update ensures that the simulation after import test runs with the latest simulation codebase.
47-49: Modifiedtest-sim-multi-seed-longfor updated simulationsThe target is updated to point to
simapp/v2and removes the-Periodparameter. This ensures that long multi-seed simulations are executed using the current simulation setup.
51-54: Addedtest-sim-multi-seed-shorttargetThe new
test-sim-multi-seed-shorttarget provides a shorter multi-seed simulation test. It is beneficial for quicker feedback during development and testing.
79-81: Commented out deprecatedtest-sim-fuzztargetThe
test-sim-fuzztarget and its associated commands have been commented out, possibly due to deprecation or pending migration tosimapp/v2. This prevents the execution of outdated or non-functional test scripts.
85-86: Commented outtest-sim-benchmarktargetThe
test-sim-benchmarktarget has been commented out, indicating that benchmark tests may need to be updated to work with the new simulation setup or are temporarily disabled.
91-92: Commented outtest-sim-profiletargetSimilar to other targets, the
test-sim-profiletarget is commented out, suggesting that profiling is either not currently supported or requires updates to function withsimapp/v2.types/simulation/config.go (1)
16-16: LGTM! Type changes improve consistency and correctnessThe change from
inttouint64forInitialBlockHeightandNumBlocksis appropriate as these values are inherently non-negative. This also maintains consistency with the CLI flag types.Also applies to: 18-18
simsx/v2/valset_history.go (4)
19-19: LGTM! Type change improves correctnessThe change from
inttouint64forblockOffsetis appropriate as block heights are non-negative values.Also applies to: 19-19
63-63: LGTM! Improved type safetyThe explicit casting to
int64improves type safety in the height calculation.Also applies to: 63-63
73-81: LGTM! Proper type handling in SetMaxHistoryThe method correctly handles the type conversion when updating blockOffset.
23-29: Verify the impact of hardcoding maxElementsThe constructor now hardcodes
maxElementsto 1, which might limit the validator set history functionality. Please confirm if this is intentional and doesn't impact the evidence handling capabilities.✅ Verification successful
Hardcoding maxElements to 1 is intentional and safe
The constructor's maxElements=1 is intentional as evidence generation and validation only require the current validator set. The evidence module handles validation using time/block windows rather than historical validator sets. If needed, the history size can be adjusted using SetMaxHistory.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to maxElements to understand its usage rg -A 5 "maxElements" .Length of output: 1328
Script:
#!/bin/bash # Search for tests and documentation rg -l "TestValSetHistory" . rg -A 10 "SetMaxElements" . | grep -v "^--$" | grep -v "^$" # Check documentation mentioning validator set history fd README.md | xargs rg -i "validator.*history|valset.*history" # Look for evidence-related documentation since the comment mentions it rg -i "evidence.*valid" .Length of output: 109604
Script:
#!/bin/bash # Search for tests of ValSetHistory rg -A 10 "TestValSetHistory|func.*ValSetHistory" . # Look for evidence time window configuration rg -i "evidence.*window|evidence.*duration" .Length of output: 108711
Script:
#!/bin/bash # Search for tests and evidence validation code rg -A 5 "TestValSetHistory|func.*MissBehaviour|func.*Evidence" . # Look for evidence validation implementation ast-grep --pattern 'func $_ ($_, $_, $_) { $$$ evidence $$$ $$$ }'Length of output: 67462
x/simulation/client/cli/flags.go (1)
21-22: LGTM! Consistent type changesThe changes to
uint64for block-related flags and their parsing functions maintain consistency with the Config struct while ensuring non-negative values. The default values remain appropriate.Also applies to: 44-45
| func (ti TestInstance[T]) InitializeChain(tb testing.TB, | ||
| ctx context.Context, | ||
| chainID string, |
There was a problem hiding this comment.
| func (ti TestInstance[T]) InitializeChain(tb testing.TB, | |
| ctx context.Context, | |
| chainID string, | |
| func (ti TestInstance[T]) InitializeChain( | |
| tb testing.TB, | |
| ctx context.Context, | |
| chainID string, |
| @@ -18,51 +19,45 @@ test-sim-nondeterminism: | |||
| test-sim-nondeterminism-streaming: | |||
There was a problem hiding this comment.
Are we planning to bring these tests back as well?
| test-sim-multi-seed-short: | ||
| @echo "Running short multi-seed application simulation. This may take awhile!" | ||
| @cd ${CURRENT_DIR}/simapp/v2 && go test -failfast -mod=readonly -timeout 30m -tags='sims' -run TestFullAppSimulation \ | ||
| -NumBlocks=50 | ||
|
|
||
| test-sim-benchmark-invariants: |
There was a problem hiding this comment.
We can remove this as invariants have been removed
| "github.com/stretchr/testify/require" | ||
|
|
||
| simtypes "github.com/cosmos/cosmos-sdk/types/simulation" | ||
| v2 "github.com/cosmos/cosmos-sdk/x/genutil/v2" |
There was a problem hiding this comment.
probably better to name genutil as the import
Description
See #23265
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
Simulation Testing Improvements
Code Refactoring
uint64typeTesting Enhancements