test(simulation): re-enable simulation tests #1735
Conversation
WalkthroughThe project has introduced a workflow for simulation testing, enhanced the naming clarity in the application code, and improved the simulation testing framework. It has also updated the module management within the application files and extended the functionality of the Changes
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? TipsChat with CodeRabbit Bot (
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1735 +/- ##
==========================================
- Coverage 73.89% 73.86% -0.03%
==========================================
Files 192 193 +1
Lines 15142 15206 +64
==========================================
+ Hits 11189 11232 +43
- Misses 3297 3312 +15
- Partials 656 662 +6
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- simapp/params.json
Files selected for processing (12)
- .github/workflows/simulation-tests.yml (1 hunks)
- CHANGELOG.md (2 hunks)
- app/app.go (4 hunks)
- app/export.go (1 hunks)
- app/keepers.go (2 hunks)
- app/upgrades.go (1 hunks)
- contrib/make/simulation.mk (1 hunks)
- simapp/sim_test.go (3 hunks)
- simapp/state_test.go (1 hunks)
- x/common/testutil/testapp/testapp.go (3 hunks)
- x/sudo/module.go (2 hunks)
- x/sudo/simulation/genesis.go (1 hunks)
Additional comments: 35
app/upgrades.go (1)
- 23-23: The change from
app.mmtoapp.ModuleManagerin theCreateUpgradeHandlermethod call is consistent with the renaming of the field. Ensure that all references to the oldmmfield have been updated across the codebase to maintain consistency.x/sudo/simulation/genesis.go (3)
23-25: The
GenCommunityTaxfunction generates a random community tax rate. The calculation seems correct, but it's important to ensure that the resulting tax rate is within acceptable bounds for the application.28-30: The
GenWithdrawEnabledfunction has a 95% chance to enable withdrawals. This hardcoded probability should be documented or configurable if the business logic requires flexibility.33-47: The
RandomizedGenStatefunction correctly generates a randomGenesisStatefor thex/sudomodule. It's important to ensure that thesimState.Accounts[0].Address.String()call is safe and that the account at index 0 is guaranteed to exist.contrib/make/simulation.mk (4)
6-14: The
test-sim-nondeterminismtarget has been updated with new parameters. Verify that these parameters (-Enabled,-Params,-NumBlocks,-BlockSize,-Commit,-Period,-Verbose) are correctly used in the test and that the test is functioning as expected with these new settings.19-27: The
test-sim-default-genesis-fasttarget has been updated similarly totest-sim-nondeterminism. Ensure that the parameters are appropriate for the intended test scenarios and that the test behaves correctly.32-39: The new
test-sim-import-exporttarget is introduced for running application import/export simulation tests. Confirm that the parameters provided are suitable for the tests and that the tests cover the necessary scenarios.44-51: The
test-sim-after-importtarget is added for running application simulation-after-import tests. As with the other targets, ensure that the parameters are correctly set and that the tests are comprehensive..github/workflows/simulation-tests.yml (4)
17-27: The
test-sim-nondeterminismjob is set up to run on theubuntu-latestrunner with Go version 1.21. Ensure that this Go version is compatible with the project's requirements and that the job will execute themake test-sim-nondeterminismcommand as intended.29-39: The
test-sim-default-genesis-fastjob is configured similarly totest-sim-nondeterminism. Confirm that the Go version and the runner are appropriate for the project and that the command is correctly set up to run the test.41-51: The
test-sim-import-exportjob is introduced to run the import/export simulation tests. Verify that the setup is correct and that the test command will run as expected.53-63: The
test-sim-after-importjob is added for running simulation-after-import tests. Ensure that the setup is correct and that the test command will run as expected.simapp/state_test.go (1)
- 67-67: An empty line has been added at line 67. This does not affect the functionality but improves readability by separating logical blocks of code.
x/common/testutil/testapp/testapp.go (1)
- 107-107: The addition of
baseAppOptionsas variadic arguments to theNewNibiruTestAppfunction signature allows for more flexible configuration of theBaseApp. Ensure that all calls to this function have been updated to pass the appropriate options where necessary.app/export.go (1)
- 32-32: The change from
app.mm.ExportGenesisForModulestoapp.ModuleManager.ExportGenesisForModulesis consistent with the renaming of the field. Ensure that all references to the oldmmfield have been updated across the codebase to maintain consistency.x/sudo/module.go (3)
14-14: The addition of the
simtypesimport is necessary for the simulation functionality.26-28: The
AppModulestruct now implements themodule.AppModuleSimulationinterface, which is required for simulation functionality.162-164: The
GenerateGenesisStatefunction correctly implements the required interface method for simulation state generation.app/app.go (5)
93-93: Renaming the
mmfield toModuleManagerin theNibiruAppstruct improves code readability and clarity.182-182: The method
initSimulationManageris correctly renamed to follow Go's convention of unexported methods starting with a lowercase letter.254-254: The
BeginBlockermethod correctly references the renamedModuleManagerfield.259-259: The
EndBlockermethod also correctly references the renamedModuleManagerfield.268-269: The
InitChainermethod correctly uses the renamedModuleManagerfield and its methodInitGenesis. This is a key part of the application initialization process.simapp/sim_test.go (8)
8-9: The addition of
"runtime/debug"and"strings"imports suggests new error handling and string manipulation functionality. Ensure these are used appropriately in the test functions.58-61: The new type
StoreKeysPrefixesis added. Verify that this type is used effectively in the tests and that the naming convention is consistent with the project's standards.64-109: The
TestFullAppSimulationfunction has been refactored to use thesimtestutilpackage for setup and simulation. Ensure that the simulation setup and teardown are correctly handled and that the simulation parameters are properly configured.5-143: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [111-175]
The
TestAppStateDeterminismfunction has been modified to use thesimclipackage for configuration and setup. Verify that the determinism test is correctly implemented and that the simulation parameters are properly configured.
148-158: The simulation function
SimulateFromSeedis called with the new setup. Ensure that the simulation is executed correctly and that the error handling is appropriate.160-161: The
PrintStatsfunction is called conditionally based on theconfig.Commitflag. Verify that the statistics are printed correctly and that the condition is appropriate.177-301: The
TestAppImportExportfunction is added to test the import/export functionality of the application. Verify that the import/export simulation is set up correctly, that the application state is exported and imported as expected, and that the store comparison is correctly implemented.303-389: The
TestAppSimulationAfterImportfunction is added to test the application simulation after import. Verify that the simulation after import is set up correctly, that the application state is exported and imported as expected, and that the simulation is executed correctly.app/keepers.go (2)
- 708-724: The refactoring from
app.mmtoapp.ModuleManageris consistent with the summary provided. Ensure that all references to the oldapp.mmhave been updated across the entire codebase to prevent any runtime errors.Verification successful
The search for references to
app.mmin Go files has found a commented-out line inapp/keepers.go. Since it is a comment, it does not affect the execution of the code. Therefore, there are no runtime errors to be concerned about due to references toapp.mm.* 850-858: The update to the simulation manager initialization using `module.NewSimulationManagerFromAppModules` and the override map is a significant change. Verify that the override modules are correctly configured and that the simulation manager is properly initialized with these changes.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to 'app.mm' which should have been replaced. rg --type go 'app\.mm'Length of output: 83
Verification successful
The verification of the
initSimulationManagerfunction and theoverrideModulesmap inapp/keepers.goindicates that the simulation manager is being initialized correctly with the provided override modules. The outputs from the shell scripts confirm that the changes align with the PR objectives and the AI-generated summaries.
- The
app.smis properly assigned usingmodule.NewSimulationManagerFromAppModules.- The
overrideModulesmap is declared and used correctly, with at least one override forauthtypes.ModuleName.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the usage of the simulation manager to ensure it's being initialized correctly. rg --type go 'app\.sm' # Verify that the overrideModules map is correctly used in the simulation manager. ast-grep --lang go --pattern $'map[string]module.AppModuleSimulation{$$$}'Length of output: 556
CHANGELOG.md (2)
75-91: The changelog entries are well-structured and adhere to the guidelines provided in the comments at the beginning of the file.
97-99: The format for versioning and dates is consistent and follows the project's standards.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (3)
- CHANGELOG.md (3 hunks)
- simapp/README.md (1 hunks)
- x/sudo/module.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- x/sudo/module.go
Additional comments: 3
simapp/README.md (3)
1-16: The description for the non-determinism test case is clear and concise, explaining the purpose and the expected outcome of the test.
18-25: The description for the full app simulation test is clear, indicating what the test does and what it checks for.
51-59: The explanation of the
params.jsonfile provides important context for why certain operations are weighted as zero. This is a good documentation of a known issue and the current workaround.
| ### Import/Export | ||
|
|
||
| ```sh | ||
| make test-sim-import-export | ||
| ``` | ||
|
|
||
| This test case runs the simulation with the default genesis file. It checks that | ||
| the simulation does not panic and that the resulting state is valid. It then | ||
| exports the state to a file and imports it back. It checks that the imported | ||
| state is the same as the exported state. |
There was a problem hiding this comment.
The description for the import/export test case is clear, but consider if the repeated explanation in the simulation after import section is necessary, as it may be seen as redundant.
| ### Simulation After Import | ||
|
|
||
| ```sh | ||
| make test-sim-after-import | ||
| ``` | ||
|
|
||
| This test case runs the simulation with the default genesis file. It checks that | ||
| the simulation does not panic and that the resulting state is valid. It then | ||
| exports the state to a file and imports it back. It checks that the imported | ||
| state is the same as the exported state. It then runs the simulation again with | ||
| the imported state. It checks that the simulation does not panic and that the | ||
| resulting state is valid. |
There was a problem hiding this comment.
The simulation after import test case description repeats information from the import/export test case. It might be more concise to reference the previous test and focus on the additional steps that are unique to this test.
| @@ -90,7 +90,7 @@ type NibiruApp struct { | |||
| AppKeepers // embed all module keepers | |||
|
|
|||
| // the module manager | |||
| mm *module.Manager | |||
| ModuleManager *module.Manager | |||
There was a problem hiding this comment.
do we have some benefit of adding it public?
There was a problem hiding this comment.
It's required for TestAppImportExport.
Purpose / Abstract
Re-enables and fixes the following simulation tests:
The tests were copied over from https://github.com/cosmos/cosmos-sdk/blob/d09f40d0a1160dc576c68b6be9c54e4ac07aa92f/simapp/sim_test.go and adapted to run with the Nibiru app.
The PR also adds a GHA workflow to run the simulation tests once a day on the
mainbranch.There are follow-up PRs to add simulation operations for Nibiru custom modules (see #1737)
Summary by CodeRabbit
This update does not change the existing bullet-point list. Therefore, the previous list remains valid and can be repeated verbatim:
Summary by CodeRabbit
New Features
Improvements
Documentation
Refactor
Bug Fixes