test(server/v2): Add system-test for store's command #21357
Conversation
WalkthroughWalkthroughThe recent changes introduce enhanced testing capabilities for a blockchain application, focusing on CLI command execution and snapshot management. A new method for running commands with arguments was added, along with comprehensive system tests for snapshot operations such as creation, deletion, and restoration. Additionally, the ability to start and stop individual validator nodes was integrated, allowing for improved control during testing scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant CLIWrapper
participant SystemUnderTest
participant SnapshotManager
CLIWrapper->>SystemUnderTest: RunCommandWithArgs(args)
SystemUnderTest->>CLIWrapper: Execute command
alt Command Success
CLIWrapper-->>SystemUnderTest: Return output
SystemUnderTest->>SnapshotManager: Manage snapshot operations
SnapshotManager->>SnapshotManager: Create, List, Delete, Load snapshots
else Command Failure
CLIWrapper-->>SystemUnderTest: Return error output
end
Assessment against linked issues
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
|
![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
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- tests/systemtests/cli.go (1 hunks)
- tests/systemtests/snapshots_test.go (1 hunks)
- tests/systemtests/system.go (1 hunks)
Additional context used
Path-based instructions (3)
tests/systemtests/snapshots_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tests/systemtests/cli.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"tests/systemtests/system.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Additional comments not posted (15)
tests/systemtests/snapshots_test.go (12)
16-26: Ensure chain is properly reset and started.The chain is reset and started before performing snapshot operations, which is good practice to ensure a clean test environment.
34-37: Verify snapshot export operation.The test correctly verifies the snapshot export operation by checking for the presence of a success message and the existence of the snapshot directory.
39-41: Check snapshot list operation.The snapshot list operation is verified by checking for the expected height in the output.
43-46: Validate snapshot dump operation.The test ensures that the snapshot dump operation is successful by checking for the existence of the output file.
48-50: Confirm snapshot delete operation.The snapshot delete operation is validated by confirming the absence of the snapshot directory.
52-54: Verify snapshot load operation.The snapshot load operation is verified by checking for the re-creation of the snapshot directory.
58-62: Ensure databases are removed before restore.The test correctly removes the application and state sync databases before attempting to restore from a snapshot, ensuring a clean state.
64-66: Validate snapshot restore operation.The snapshot restore operation is confirmed by checking the existence of the restored databases.
68-69: Restart node after snapshot operations.The node is restarted after completing snapshot operations, which is necessary to ensure the system is back to a running state.
72-82: Ensure chain is properly reset and started for prune test.The chain is reset and started before performing the prune operation, ensuring a clean test environment.
90-92: Verify prune operation.The prune operation is validated by checking for a success message in the output.
94-95: Restart node after prune operation.The node is restarted after the prune operation, ensuring the system is back to a running state.
tests/systemtests/cli.go (1)
172-180: Well-implemented method for running CLI commands.The
RunCommandWithArgsmethod is correctly implemented as a wrapper around therunmethod, providing a clear interface for executing CLI commands with arguments.tests/systemtests/system.go (2)
334-351: Ensure error handling for stopping nodes.The
StopSingleNodemethod is well-implemented for stopping a single validator node. Ensure that the method is used in contexts where stopping the first node is appropriate, as it assumes the first PID corresponds to the node to be stopped.
353-362: Correct implementation for starting a single node.The
StartSingleNodemethod is correctly implemented, executing the command to start a node and ensuring no errors occur during the process.
julienrbrt
added
backport/v0.52.x
tests/systemtests/snapshots_test.go
Outdated
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| const nodeDir = "./testnet/node0/simdv2" |
There was a problem hiding this comment.
this wont work with v1, we should make sure it works with both
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- tests/systemtests/snapshots_test.go (1 hunks)
- tests/systemtests/system.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- tests/systemtests/snapshots_test.go
- tests/systemtests/system.go
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- tests/systemtests/snapshots_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- tests/systemtests/snapshots_test.go
tests/systemtests/cli.go
Outdated
| // RunCommandWithArgs use for run cli command, not tx | ||
| func (c CLIWrapper) RunCommandWithArgs(args ...string) string { | ||
| c.t.Helper() | ||
| execOutput, ok := c.run(args) |
There was a problem hiding this comment.
It always returns the execOutput here, shouldn't it return an error if not okay? Or is that expected that ok is ignored?
There was a problem hiding this comment.
I think should ignore it with the local command. execOutput already returns err if cmd failed, handler by run func.
tests/systemtests/system.go
Outdated
| } | ||
|
|
||
| // Kill the 1st node | ||
| return p.Kill() |
There was a problem hiding this comment.
Do we really want to kill it that way? This sends iirc a SIGKILL, we may want to stop the node more gracefully with SIGTERM
tests/systemtests/snapshots_test.go
Outdated
| @@ -0,0 +1,121 @@ | |||
| //go:build system_test | |||
There was a problem hiding this comment.
As the commands are really different between simapp and simapp v2, shall we not split the test in two tests?
The multiple if conditions will make it harder to maintain.
We can then just skip the tests when we run with COSMOS_BUILD_OPTIONS v2 and vice versa
Wdyt? cc @tac0turtle
There was a problem hiding this comment.
should we update the prefix, most diff is that v2 has store prefix and v1 has snapshots?
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- tests/systemtests/cli.go (1 hunks)
- tests/systemtests/system.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- tests/systemtests/cli.go
- tests/systemtests/system.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- tests/systemtests/system.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- tests/systemtests/system.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- tests/systemtests/cli.go (1 hunks)
- tests/systemtests/system.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- tests/systemtests/cli.go
- tests/systemtests/system.go
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (4)
- tests/systemtests/Makefile (1 hunks)
- tests/systemtests/snapshots_v1_test.go (1 hunks)
- tests/systemtests/snapshots_v2_test.go (1 hunks)
- tests/systemtests/system.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- tests/systemtests/system.go
Additional context used
Path-based instructions (3)
tests/systemtests/Makefile (1)
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"tests/systemtests/snapshots_v1_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tests/systemtests/snapshots_v2_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
Additional comments not posted (1)
tests/systemtests/Makefile (1)
8-8: LGTM!The dynamic construction of the
-tagsoption based on the presence ofv2in theCOSMOS_BUILD_OPTIONSvariable enhances the flexibility of the test command.The code changes are approved.
| func TestSnapshots(t *testing.T) { | ||
| sut.ResetChain(t) | ||
| cli := NewCLIWrapper(t, sut, verbose) | ||
| // add genesis account with some tokens | ||
| account1Addr := cli.AddKey("account1") | ||
| sut.ModifyGenesisCLI(t, | ||
| []string{"genesis", "add-genesis-account", account1Addr, "10000000stake"}, | ||
| ) | ||
|
|
||
| sut.StartChain(t) | ||
| nodeDir = filepath.Join(WorkDir, "testnet", "node0", "simd") | ||
|
|
||
| // Wait for chain produce some blocks | ||
| time.Sleep(time.Second * 10) | ||
| // Stop 1 node | ||
| err := sut.StopSingleNode() | ||
| require.NoError(t, err) | ||
| time.Sleep(time.Second * 5) | ||
|
|
||
| // export snapshot at height 5 | ||
| res := cli.RunCommandWithArgs("snapshots", "export", "--height=5", fmt.Sprintf("--home=%s", nodeDir)) | ||
| require.Contains(t, res, "Snapshot created at height 5") | ||
| require.DirExists(t, fmt.Sprintf("%s/data/snapshots/5/3", nodeDir)) | ||
|
|
||
| // Check snapshots list | ||
| res = cli.RunCommandWithArgs("snapshots", "list", fmt.Sprintf("--home=%s", nodeDir)) | ||
| require.Contains(t, res, "height: 5") | ||
|
|
||
| // Dump snapshot | ||
| res = cli.RunCommandWithArgs("snapshots", "dump", "5", "3", fmt.Sprintf("--home=%s", nodeDir), fmt.Sprintf("--output=%s/5-3.tar.gz", nodeDir)) | ||
| // Check if output file exist | ||
| require.FileExists(t, fmt.Sprintf("%s/5-3.tar.gz", nodeDir)) | ||
|
|
||
| // Delete snapshots | ||
| res = cli.RunCommandWithArgs("snapshots", "delete", "5", "3", fmt.Sprintf("--home=%s", nodeDir)) | ||
| require.NoDirExists(t, fmt.Sprintf("%s/data/snapshots/5/3", nodeDir)) | ||
|
|
||
| // Load snapshot from file | ||
| res = cli.RunCommandWithArgs("snapshots", "load", fmt.Sprintf("%s/5-3.tar.gz", nodeDir), fmt.Sprintf("--home=%s", nodeDir)) | ||
| require.DirExists(t, fmt.Sprintf("%s/data/snapshots/5/3", nodeDir)) | ||
|
|
||
| // Restore from snapshots | ||
|
|
||
| // Remove database | ||
| err = os.RemoveAll(fmt.Sprintf("%s/data/application.db", nodeDir)) | ||
| require.NoError(t, err) | ||
|
|
||
| res = cli.RunCommandWithArgs("snapshots", "restore", "5", "3", fmt.Sprintf("--home=%s", nodeDir)) | ||
| require.DirExists(t, fmt.Sprintf("%s/data/application.db", nodeDir)) | ||
| } |
There was a problem hiding this comment.
Consider using a more reliable synchronization mechanism.
The hardcoded sleep durations might make the tests flaky. Consider using a more reliable synchronization mechanism, such as waiting for a specific log message or event, to ensure the chain has produced the required blocks or the node has stopped.
| func TestPrune(t *testing.T) { | ||
| sut.ResetChain(t) | ||
| cli := NewCLIWrapper(t, sut, verbose) | ||
| // add genesis account with some tokens | ||
| account1Addr := cli.AddKey("account1") | ||
| sut.ModifyGenesisCLI(t, | ||
| []string{"genesis", "add-genesis-account", account1Addr, "10000000stake"}, | ||
| ) | ||
|
|
||
| sut.StartChain(t) | ||
| nodeDir = filepath.Join(WorkDir, "testnet", "node0", "simd") | ||
|
|
||
| // Wait for chain produce some blocks | ||
| time.Sleep(time.Second * 10) | ||
| // Stop 1 node | ||
| err := sut.StopSingleNode() | ||
| require.NoError(t, err) | ||
| time.Sleep(time.Second) | ||
|
|
||
| // prune | ||
| res := cli.RunCommandWithArgs("prune", "everything", fmt.Sprintf("--home=%s", nodeDir)) | ||
| require.Contains(t, res, "successfully pruned the application root multi stores") | ||
| } |
There was a problem hiding this comment.
Consider using a more reliable synchronization mechanism.
The hardcoded sleep durations might make the tests flaky. Consider using a more reliable synchronization mechanism, such as waiting for a specific log message or event, to ensure the chain has produced the required blocks or the node has stopped.
| func TestSnapshots(t *testing.T) { | ||
| fmt.Println("TestSnapshots") | ||
| sut.ResetChain(t) | ||
| cli := NewCLIWrapper(t, sut, verbose) | ||
| // add genesis account with some tokens | ||
| account1Addr := cli.AddKey("account1") | ||
| sut.ModifyGenesisCLI(t, | ||
| []string{"genesis", "add-genesis-account", account1Addr, "10000000stake"}, | ||
| ) | ||
|
|
||
| sut.StartChain(t) | ||
| nodeDir = filepath.Join(WorkDir, "testnet", "node0", "simdv2") | ||
|
|
||
| // Wait for chain produce some blocks | ||
| time.Sleep(time.Second * 10) | ||
| // Stop 1 node | ||
| err := sut.StopSingleNode() | ||
| require.NoError(t, err) | ||
| time.Sleep(time.Second * 5) | ||
|
|
||
| // export snapshot at height 5 | ||
| res := cli.RunCommandWithArgs("store", "export", "--height=5", fmt.Sprintf("--home=%s", nodeDir)) | ||
| require.Contains(t, res, "Snapshot created at height 5") | ||
| require.DirExists(t, fmt.Sprintf("%s/data/snapshots/5/3", nodeDir)) | ||
|
|
||
| // Check snapshots list | ||
| res = cli.RunCommandWithArgs("store", "list", fmt.Sprintf("--home=%s", nodeDir)) | ||
| require.Contains(t, res, "height: 5") | ||
|
|
||
| // Dump snapshot | ||
| res = cli.RunCommandWithArgs("store", "dump", "5", "3", fmt.Sprintf("--home=%s", nodeDir), fmt.Sprintf("--output=%s/5-3.tar.gz", nodeDir)) | ||
| // Check if output file exist | ||
| require.FileExists(t, fmt.Sprintf("%s/5-3.tar.gz", nodeDir)) | ||
|
|
||
| // Delete snapshots | ||
| res = cli.RunCommandWithArgs("store", "delete", "5", "3", fmt.Sprintf("--home=%s", nodeDir)) | ||
| require.NoDirExists(t, fmt.Sprintf("%s/data/snapshots/5/3", nodeDir)) | ||
|
|
||
| // Load snapshot from file | ||
| res = cli.RunCommandWithArgs("store", "load", fmt.Sprintf("%s/5-3.tar.gz", nodeDir), fmt.Sprintf("--home=%s", nodeDir)) | ||
| require.DirExists(t, fmt.Sprintf("%s/data/snapshots/5/3", nodeDir)) | ||
|
|
||
| // Restore from snapshots | ||
|
|
||
| // Remove database | ||
| err = os.RemoveAll(fmt.Sprintf("%s/data/application.db", nodeDir)) | ||
| require.NoError(t, err) | ||
| err = os.RemoveAll(fmt.Sprintf("%s/data/ss", nodeDir)) | ||
| require.NoError(t, err) | ||
|
|
||
| res = cli.RunCommandWithArgs("store", "restore", "5", "3", fmt.Sprintf("--home=%s", nodeDir)) | ||
| require.DirExists(t, fmt.Sprintf("%s/data/application.db", nodeDir)) | ||
| require.DirExists(t, fmt.Sprintf("%s/data/ss", nodeDir)) | ||
| } |
There was a problem hiding this comment.
Consider using a more reliable synchronization mechanism.
The hardcoded sleep durations might make the tests flaky. Consider using a more reliable synchronization mechanism, such as waiting for a specific log message or event, to ensure the chain has produced the required blocks or the node has stopped.
| func TestPrune(t *testing.T) { | ||
| sut.ResetChain(t) | ||
| cli := NewCLIWrapper(t, sut, verbose) | ||
| // add genesis account with some tokens | ||
| account1Addr := cli.AddKey("account1") | ||
| sut.ModifyGenesisCLI(t, | ||
| []string{"genesis", "add-genesis-account", account1Addr, "10000000stake"}, | ||
| ) | ||
|
|
||
| sut.StartChain(t) | ||
| nodeDir = filepath.Join(WorkDir, "testnet", "node0", "simdv2") | ||
|
|
||
| // Wait for chain produce some blocks | ||
| time.Sleep(time.Second * 10) | ||
| // Stop 1 node | ||
| err := sut.StopSingleNode() | ||
| require.NoError(t, err) | ||
| time.Sleep(time.Second) | ||
|
|
||
| // prune | ||
| res := cli.RunCommandWithArgs("store", "prune", "--keep-recent=1", fmt.Sprintf("--home=%s", nodeDir)) | ||
| require.Contains(t, res, "successfully pruned the application root multi stores") | ||
| } |
There was a problem hiding this comment.
Consider using a more reliable synchronization mechanism.
The hardcoded sleep durations might make the tests flaky. Consider using a more reliable synchronization mechanism, such as waiting for a specific log message or event, to ensure the chain has produced the required blocks or the node has stopped.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- tests/systemtests/system.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- tests/systemtests/system.go
There was a problem hiding this comment.
the test looks failing, plus I don't think it runs the v2 in CI until #21539
| @@ -5,7 +5,7 @@ WAIT_TIME ?= 45s | |||
| all: test | |||
|
|
|||
| test: | |||
| go test -mod=readonly -failfast -tags='system_test' ./... --wait-time=$(WAIT_TIME) --verbose $(if $(findstring v2,$(COSMOS_BUILD_OPTIONS)),--binary=simdv2) | |||
| go test -mod=readonly -failfast -tags='system_test $(if $(findstring v2,$(COSMOS_BUILD_OPTIONS)),v2)' ./... --wait-time=$(WAIT_TIME) --verbose $(if $(findstring v2,$(COSMOS_BUILD_OPTIONS)),--binary=simdv2) | |||
There was a problem hiding this comment.
In https://github.com/cosmos/cosmos-sdk/pull/21539/files#diff-d430b065976a77ab03608cab74b5c1db97ec128880faab2ea96b24db767ed75eR16-R20, we added a isV2() function, maybe let's use that instead of build tags and just use t.Skip within the test.
…mos-sdk into hieu/systemtest/store
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- store/snapshots/manager.go (1 hunks)
- store/v2/snapshots/manager.go (1 hunks)
Additional context used
Path-based instructions (2)
store/snapshots/manager.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/snapshots/manager.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (1)
store/snapshots/manager.go (1)
391-395: Correct grammatical error and verify new logic for payload handling.The error message on line 392 contains a grammatical error. It should be corrected to:
- return fmt.Errorf("extension %s don't exhausted payload stream", metadata.Name) + return fmt.Errorf("extension %s has not exhausted the payload stream", metadata.Name)Additionally, verify that the new logic correctly handles cases where the payload is not fully consumed, which could prevent potential errors in snapshot restoration.
| payload := nextItem.GetExtensionPayload() | ||
| if payload != nil && len(payload.Payload) != 0 { | ||
| return fmt.Errorf("extension %s don't exhausted payload stream", metadata.Name) | ||
| } else { | ||
| break |
There was a problem hiding this comment.
Grammar correction needed in error message.
The error message in the condition check uses incorrect grammar. It should be corrected to maintain professionalism and clarity in the codebase. Here's the suggested correction:
- return fmt.Errorf("extension %s don't exhausted payload stream", metadata.Name)
+ return fmt.Errorf("extension %s has not exhausted the payload stream", metadata.Name)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| payload := nextItem.GetExtensionPayload() | |
| if payload != nil && len(payload.Payload) != 0 { | |
| return fmt.Errorf("extension %s don't exhausted payload stream", metadata.Name) | |
| } else { | |
| break | |
| payload := nextItem.GetExtensionPayload() | |
| if payload != nil && len(payload.Payload) != 0 { | |
| return fmt.Errorf("extension %s has not exhausted the payload stream", metadata.Name) | |
| } else { | |
| break |
|
ping on this pr |
Description
Closes: #21184
Add system-test for
snapshots&pruneCurrently v1 TestSnapshots failed because of #21404
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
Summary by CodeRabbit
New Features
Bug Fixes