-
Notifications
You must be signed in to change notification settings - Fork 105
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
test: simulation import and export #3033
base: develop
Are you sure you want to change the base?
Conversation
Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]>
Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3033 +/- ##
===========================================
+ Coverage 64.12% 64.54% +0.42%
===========================================
Files 412 408 -4
Lines 28961 28518 -443
===========================================
- Hits 18570 18407 -163
+ Misses 9596 9329 -267
+ Partials 795 782 -13 |
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces a new GitHub Actions workflow for simulation testing, along with enhancements to the Makefile and various Go files. The workflow automates simulation tests triggered by code pushes, pull requests, and scheduled runs, while allowing manual execution. The Makefile has been updated to include new test commands and adjust timeouts for existing tests. Additionally, new methods and functions have been added to the application code to improve modularity and error handling. Documentation has also been updated to reflect these new testing methodologies. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer shorter names like sim.yaml
and name: sim
which makes the github checks on PRs more easy to read quickly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (5)
tests/simulation/sim/sim_utils.go (1)
74-76
: Enhance function documentation with more details.While the current documentation describes the basic purpose, it would be more helpful to include:
- Parameter descriptions
- Return value explanation
- Example usage
Consider expanding the documentation:
-// CheckExportSimulation exports the app state and simulation parameters to JSON -// if the export paths are defined. +// CheckExportSimulation exports the app state and simulation parameters to JSON files +// if the export paths are defined in the configuration. +// +// Parameters: +// - app: The application interface implementing ExportAppStateAndValidators +// - config: Simulation configuration containing export paths +// - params: Simulation parameters to be exported +// +// Returns an error if any export operation failsMakefile (1)
409-415
: LGTM: Well-implemented long-running testsThe long-running variants leverage parallel execution effectively. Consider adding a comment explaining the rationale behind the different parameters (50 iterations, 5 block size) compared to regular tests.
Add documentation above the long-running tests:
+# Long-running tests use different parameters: +# - 50 iterations instead of 100 to balance coverage with execution time +# - 5 transactions per block instead of 200 to reduce memory usage in parallel executionapp/app.go (1)
Active IBC dependencies require cleanup for production readiness
The verification reveals two areas requiring attention for a complete IBC disablement:
The ante handler still contains active IBC dependencies:
- Imports of
ibcante
andibckeeper
- Usage in
HandlerOptions
struct andRedundantRelayDecorator
- These should be commented out as they are part of the IBC functionality
The
IBCCrosschainKeeper
interface in crosschain module:
- While the implementation is commented out in
app.go
- The interface definition and related code in
x/crosschain
remain active- This creates unnecessary coupling with disabled functionality
Suggested cleanup:
- Comment out IBC-related code in
app/ante/handler_options.go
- Comment out
IBCCrosschainKeeper
interface and related code inx/crosschain/types/expected_keepers.go
- Add these files to the scope of issue Turn IBC back on #2573 for re-enablement tracking
🔗 Analysis chain
Line range hint
1-1066
: Verify complete removal of IBC functionality.While the IBC functionality has been properly commented out, let's verify no active IBC references remain and ensure this is tracked for future re-enablement.
Consider adding a version-gated feature flag for IBC functionality to make future re-enablement smoother.
Based on the search results, I can now provide a comprehensive verification of the IBC removal status.
IBC functionality properly disabled but requires cleanup
The verification shows that while IBC is properly disabled through commenting, there are still some active references that should be addressed:
- Active imports in
app/ante/handler_options.go
:ibcante "github.com/cosmos/ibc-go/v7/modules/core/ante" ibckeeper "github.com/cosmos/ibc-go/v7/modules/core/keeper"- Active interface in
x/crosschain/types/expected_keepers.go
:type IBCCrosschainKeeper interface {Let's verify if these references are actually used in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify complete removal of IBC functionality and track the related issue # Test 1: Check for any remaining active IBC imports or references echo "Checking for any remaining active IBC references..." rg -i "ibc" --type go -g '!*.md' -g '!*_test.go' -g '!*/testutil/*' -g '!*/tests/*' # Test 2: Verify the tracking issue exists echo "Verifying tracking issue..." gh issue view 2573Length of output: 22943
Script:
#!/bin/bash # Check if the IBC imports in ante handler are actually used echo "Checking ante handler IBC usage..." rg "ibcante|ibckeeper" app/ante/ -A 5 -B 5 # Check if the IBCCrosschainKeeper interface is referenced echo "Checking IBCCrosschainKeeper interface usage..." rg "IBCCrosschainKeeper" --type go -g '!*/testutil/*' -g '!*/tests/*' -g '!x/crosschain/types/expected_keepers.go'Length of output: 6412
changelog.md (1)
47-47
: Enhance the changelog entry description.The current entry "initialize simulation tests for import and export" could be more descriptive about the specific test scenarios being added.
Consider expanding the entry to:
-* [3033](https://github.com/zeta-chain/node/pull/3033) - initialize simulation tests for import and export +* [3033](https://github.com/zeta-chain/node/pull/3033) - initialize simulation tests for application state import/export and post-import simulation validationtests/simulation/sim_test.go (1)
334-335
: Uselogger.Error
for logging errors and stack tracesCurrently, the code uses
logger.Info
to log error messages and stack traces. To adhere to logging best practices and improve the clarity of log outputs, error messages should utilizelogger.Error
.Apply this diff to adjust the logging level:
logger.Info("Skipping simulation as all validators have been unbonded") - logger.Info("err", err, "stacktrace", string(debug.Stack())) + logger.Error("err", err, "stacktrace", string(debug.Stack()))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- .github/workflows/simulation-test.yaml (1 hunks)
- Makefile (2 hunks)
- app/app.go (1 hunks)
- app/export.go (2 hunks)
- changelog.md (1 hunks)
- docs/development/SIMULATION_TESTING.md (1 hunks)
- tests/simulation/sim/sim_utils.go (2 hunks)
- tests/simulation/sim_test.go (7 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
app/app.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.app/export.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.tests/simulation/sim/sim_utils.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.tests/simulation/sim_test.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🪛 LanguageTool
docs/development/SIMULATION_TESTING.md
[uncategorized] ~26-~26: Consider adding a comma between these intensifiers.
Context: ...te at the end of the run. This state is then then imported into a new simulation. At the ...(RB_RB_COMMA)
🪛 Markdownlint
docs/development/SIMULATION_TESTING.md
52-52: null
Multiple headings with the same content(MD024, no-duplicate-heading)
🪛 GitHub Check: codecov/patch
tests/simulation/sim/sim_utils.go
[warning] 76-81: tests/simulation/sim/sim_utils.go#L76-L81
Added lines #L76 - L81 were not covered by tests
[warning] 83-85: tests/simulation/sim/sim_utils.go#L83-L85
Added lines #L83 - L85 were not covered by tests
[warning] 88-92: tests/simulation/sim/sim_utils.go#L88-L92
Added lines #L88 - L92 were not covered by tests
[warning] 94-96: tests/simulation/sim/sim_utils.go#L94-L96
Added lines #L94 - L96 were not covered by tests
[warning] 98-98: tests/simulation/sim/sim_utils.go#L98
Added line #L98 was not covered by tests
🔇 Additional comments (11)
tests/simulation/sim/sim_utils.go (1)
76-99
: Add unit tests for the new function.The static analysis indicates that the new function lacks test coverage. This is critical for simulation-related code.
Would you like me to help create unit tests for this function? The tests should cover:
- Successful export of both state and params
- Error cases for ExportAppStateAndValidators
- Error cases for file operations
- Cases where export paths are not defined
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 76-81: tests/simulation/sim/sim_utils.go#L76-L81
Added lines #L76 - L81 were not covered by tests
[warning] 83-85: tests/simulation/sim/sim_utils.go#L83-L85
Added lines #L83 - L85 were not covered by tests
[warning] 88-92: tests/simulation/sim/sim_utils.go#L88-L92
Added lines #L88 - L92 were not covered by tests
[warning] 94-96: tests/simulation/sim/sim_utils.go#L94-L96
Added lines #L94 - L96 were not covered by tests
[warning] 98-98: tests/simulation/sim/sim_utils.go#L98
Added line #L98 was not covered by tests.github/workflows/simulation-test.yaml (3)
1-20
: Consider using a more concise workflow name.As suggested in the previous review, using shorter names like
name: sim
would improve readability in PR checks while maintaining clarity.
22-27
: Well-implemented concurrency control.The concurrency configuration effectively prevents parallel runs and manages resources efficiently.
65-76
:⚠️ Potential issueSecurity: Fix command injection vulnerability in manual run.
The current implementation is vulnerable to command injection. While the environment variable is properly quoted, this job could be merged with the main simulation job.
Merge this with the main job by adding a conditional step:
- name: Manual Test Run if: github.event_name == 'workflow_dispatch' env: TEST_TARGET: ${{ github.event.inputs.test-target }} run: make "$TEST_TARGET"app/export.go (1)
162-162
: Document key structure and verify address extraction.The validator address extraction uses a magic number (2) for the key offset. This assumes a specific key structure that should be documented.
Let's verify the key structure:
Consider adding a comment explaining the key structure:
+ // Validator address starts at index 2 in the key following the prefix bytes addr := sdk.ValAddress(iter.Key()[2:])
Makefile (4)
384-384
: LGTM: Echo command formatting improvementThe removal of trailing ellipsis enhances clarity while maintaining functionality.
395-399
: LGTM: Well-structured new simulation testsThe new import/export simulation tests are properly configured with parameters matching existing tests, ensuring consistent test execution patterns.
421-425
: LGTM: Proper .PHONY declarationsAll new simulation test targets are correctly declared as .PHONY targets, ensuring proper Make behavior.
390-390
: Verify the reduced timeout valuesThe timeout reduction from 2h to 30m is significant. While this improves CI pipeline efficiency, please ensure that these tests consistently complete within the new 30m limit across different environments and conditions.
Also applies to: 393-393
app/app.go (1)
1063-1065
: LGTM: Clean implementation of ModuleManager getter.The method provides a clean way to access the module manager, which aligns well with the PR's testing objectives.
changelog.md (1)
Line range hint
1-1000
: LGTM! Well-structured changelog.The changelog follows best practices with clear version headers, categorized changes, and proper PR references. Breaking changes are appropriately highlighted at the start of each version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (3)
tests/simulation/sim/sim_utils.go (1)
74-76
: Enhance function documentation with parameter descriptionsConsider adding parameter descriptions to improve documentation clarity:
// CheckExportSimulation exports the app state and simulation parameters to JSON // if the export paths are defined. +// +// Parameters: +// - app: The application interface implementing ExportAppStateAndValidators +// - config: Simulation configuration containing export paths +// - params: Simulation parameters to be exportedMakefile (1)
395-399
: LGTM: Well-structured test targets with parallel execution support.The new test targets are well-implemented and align with the PR objectives. The long-running versions leverage
runsim
for parallel execution, which is ideal for CI/CD pipelines.Consider the following optimizations:
- Add comments describing the purpose and expectations of each test target
- Consider parameterizing the number of jobs in runsim (currently hardcoded to 4) to allow for environment-specific optimization
Also applies to: 409-415
changelog.md (1)
47-47
: Enhance changelog entry with more details.The current entry is too brief. Consider expanding it to better describe the simulation tests added:
-* [3033](https://github.com/zeta-chain/node/pull/3033) - initialize simulation tests for import and export +* [3033](https://github.com/zeta-chain/node/pull/3033) - add simulation tests for data consistency verification: + * TestAppImportExport: Tests import/export cycle data consistency + * TestAppSimulationAfterImport: Tests simulation functionality post-import
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- .github/workflows/simulation-test.yaml (1 hunks)
- Makefile (2 hunks)
- app/app.go (1 hunks)
- app/export.go (2 hunks)
- changelog.md (1 hunks)
- docs/development/SIMULATION_TESTING.md (1 hunks)
- tests/simulation/sim/sim_utils.go (2 hunks)
- tests/simulation/sim_test.go (7 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
app/app.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.app/export.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.tests/simulation/sim/sim_utils.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.tests/simulation/sim_test.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🪛 LanguageTool
docs/development/SIMULATION_TESTING.md
[uncategorized] ~26-~26: Consider adding a comma between these intensifiers.
Context: ...te at the end of the run. This state is then then imported into a new simulation. At the ...(RB_RB_COMMA)
🪛 Markdownlint
docs/development/SIMULATION_TESTING.md
52-52: null
Multiple headings with the same content(MD024, no-duplicate-heading)
🔇 Additional comments (11)
tests/simulation/sim/sim_utils.go (1)
4-13
: LGTM: Import organization follows best practicesThe new imports are properly organized and all are utilized in the new functionality.
.github/workflows/simulation-test.yaml (4)
1-21
: LGTM: Well-structured workflow triggers.The workflow configuration provides comprehensive coverage with automated triggers (push, PR, scheduled) and manual execution capability.
22-27
: LGTM: Effective concurrency control.The concurrency configuration efficiently manages resources by preventing parallel runs and canceling outdated executions.
30-44
: LGTM: Clean dependency installation setup.The job properly sets up the environment with specific versions and required dependencies.
65-76
: LGTM: Secure manual test execution.The manual-run job properly handles user input through environment variables, preventing shell injection vulnerabilities.
app/export.go (1)
78-78
: LGTM: Error handling aligns with expected behavior.The explicit error ignoring is appropriate here as some validators might not have any commission to withdraw.
Makefile (3)
384-384
: LGTM: Echo command formatting improvement.The removal of trailing ellipsis enhances command output clarity.
421-425
: LGTM: Proper .PHONY declarations for new targets.All new test targets are correctly declared as .PHONY targets, following Makefile best practices.
390-393
: Verify if 30m timeout is sufficient for simulation tests.The timeout reduction from 2h to 30m is significant. While this change improves CI pipeline efficiency, we should verify that 30m is sufficient based on historical test execution times.
✅ Verification successful
After analyzing the simulation tests implementation and context, I can now provide a final response:
30-minute timeout appears sufficient for simulation tests
The simulation tests are well-structured with reasonable parameters:
- Tests run with 100 blocks and 200 transactions per block
- Multiple test variations run with controlled complexity:
- TestAppStateDeterminism: 3 seeds × 5 runs each
- TestFullAppSimulation: Single comprehensive run
- Import/Export tests: Single iteration with verification
The 30-minute timeout should be adequate because:
- The tests use moderate simulation parameters (100 blocks, 200 tx/block)
- The implementation includes proper cleanup and resource management
- No timeout-related failures were found in the git history
- The test structure allows for parallel execution where applicable
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check historical test execution times in GitHub Actions # Search for previous test runs and their durations gh run list --workflow simulation-test.yaml --json conclusion,durationMs,name --jq '.[] | select(.name | contains("test-sim-nondeterminism") or contains("test-sim-fullappsimulation")) | {name: .name, duration_minutes: (.durationMs/60000), conclusion: .conclusion}'Length of output: 502
Script:
#!/bin/bash # Let's try a different approach to check workflow runs using available fields gh run list --workflow simulation-test.yaml --json conclusion,startedAt,updatedAt,name --jq '.[] | select(.name | contains("test-sim-nondeterminism") or contains("test-sim-fullappsimulation")) | {name: .name, duration_minutes: (((.updatedAt | fromdateiso8601) - (.startedAt | fromdateiso8601))/60), conclusion: .conclusion}' | head -n 10Length of output: 339
Script:
#!/bin/bash # Also check if there's a dedicated workflow file for these tests fd -e yml -e yaml simulationLength of output: 30
Script:
#!/bin/bash # Check if there are any comments or documentation about these timeouts rg -i "timeout.*simulation|simulation.*timeout" -A 2 -B 2Length of output: 267
Script:
#!/bin/bash # Let's examine the run-sim-test function definition and its usage context rg "run-sim-test" -A 5 -B 5 MakefileLength of output: 1036
Script:
#!/bin/bash # Check for any historical comments or discussions about simulation timeouts git log -p | rg -i "simulation.*test.*timeout|timeout.*simulation" -A 3 -B 3Length of output: 1128
Script:
#!/bin/bash # Look for simulation test implementations to understand their complexity fd -e go . -x rg -l "TestAppStateDeterminism|TestFullAppSimulation" {}Length of output: 105
Script:
#!/bin/bash # Let's examine the simulation test implementations cat tests/simulation/sim_test.goLength of output: 14779
Script:
#!/bin/bash # Check if there are any recent CI failures related to timeouts git log -p | rg -i "fail.*timeout.*simulation|simulation.*timeout.*fail" -A 3 -B 3Length of output: 84
Script:
#!/bin/bash # Look for any configuration files that might contain historical timeout values fd -e toml -e yaml -e json . -x rg -l "timeout"Length of output: 207
app/app.go (1)
1063-1065
: LGTM: Clean getter implementation.The
ModuleManager()
method provides a clean way to access the module manager, which is necessary for the new simulation tests. The implementation follows the standard Go getter pattern.changelog.md (1)
Line range hint
1-1000
: LGTM! Well-structured changelog.The changelog follows best practices with:
- Clear version sections
- Categorized changes
- PR links for traceability
- Breaking changes clearly highlighted
@@ -1,12 +1,16 @@ | |||
package sim |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need a directory tests
, we can directly put simulation
at the root
I think this can also be removed from test coverage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was planning to have a directory test that can hold all test-related files , and have directories for simulation, e2e etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would do ./simtest/*
directly in the project's root in a single package to keep it simple.
simtest
utils.go
config.go
state.go
abc_test.go
xyz_test.go
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would keep e2e
at the root
if !strings.Contains(err, "validator set is empty after InitGenesis") { | ||
panic(r) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, I don't think we ever need panics
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you referring to the use of the recover?
Having the recovery here is useful as we initiate two runs, and we can panic in either one of them.
We would ideally want to skip the simulation if the validator set is empty after genesis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant I think we can use require
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking pretty good
Co-authored-by: Alex Gartner <[email protected]>
- name: Set up Go | ||
uses: actions/setup-go@v4 | ||
with: | ||
go-version: '1.21' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go-version: '1.21' | |
go-version: '1.22' |
make-targets: | ||
description: 'Comma separated list of make targets to run (e.g., test-sim-nondeterminism, test-sim-fullappsimulation)' | ||
required: true | ||
default: '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider placing a default value here
push: | ||
branches: | ||
- develop | ||
pull_request: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I doubt running these tests every PR. Think of zetaclient
PRs - most of the time they don't touch zetacore
part. Probably we should do the following:
- for every PR if
x/...
files were touched - if a PR has
SIM_TESTS
label
Otherwise the CI will become even slower
@@ -1,12 +1,16 @@ | |||
package sim |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would do ./simtest/*
directly in the project's root in a single package to keep it simple.
simtest
utils.go
config.go
state.go
abc_test.go
xyz_test.go
...
@@ -173,8 +197,12 @@ func TestFullAppSimulation(t *testing.T) { | |||
require.NoError(t, err, "simulation setup failed") | |||
|
|||
defer func() { | |||
require.NoError(t, db.Close()) | |||
require.NoError(t, os.RemoveAll(dir)) | |||
if err := db.Close(); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider using t.Cleanup(func(){...})
instead
Description
This pr adds two new simulation tests ,
It add the simulation tests to the CI , which is set to execute at
Closes : #3035
#2961
How Has This Been Tested?
Summary by CodeRabbit
Release Notes
New Features
test-sim-import-export
andtest-sim-after-import
.Bug Fixes
WithdrawValidatorCommission
method.Documentation
Tests