refactor(storev2): update snapshot manager and migration manager tests #20441
Conversation
WalkthroughWalkthroughThe recent updates enhance the testing framework for snapshot management and migration functionalities within the Manager module. New test functions, Changes
Sequence Diagram(s)sequenceDiagram
participant Tester as Tester
participant Manager as Manager
participant Snapshotter as Snapshotter
Note over Tester: Test "TestSnapshot_Take_Restore"
Tester->>+Manager: Call Snapshotter to take snapshot
Manager->>Snapshotter: Take snapshot
Snapshotter-->>Manager: Snapshot created
Tester->>Manager: Restore snapshot from chunk
Manager->>Snapshotter: Restore from chunk
Snapshotter-->>Manager: Chunk restored
Tester-->>Manager: Verify snapshots list
Note over Tester: Test "TestSnapshot_Take_Prune"
Tester->>+Manager: Call Snapshotter to take multiple snapshots
Manager->>Snapshotter: Take snapshots
Snapshotter-->>Manager: Snapshots created
Tester->>Manager: Prune old snapshots
Manager->>Snapshotter: Prune snapshots based on height
Snapshotter-->>Manager: Old snapshots pruned
Tester-->>Manager: Verify snapshots list after prune
These diagrams illustrate the interactions between the Tester, Manager, and Snapshotter components during the new 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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
sontrinh16
changed the title
This reverts commit 214605b.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
sontrinh16
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
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- store/v2/migration/manager_test.go (1 hunks)
- store/v2/snapshots/manager_test.go (1 hunks)
Additional context used
Path-based instructions (2)
store/v2/migration/manager_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"store/v2/snapshots/manager_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"
Additional comments not posted (3)
store/v2/migration/manager_test.go (1)
82-85: Handle conflicting processes during migration effectively.The added code checks for an error when a snapshot creation process conflicts with a migration process. This is a crucial check to ensure that migrations do not interfere with ongoing snapshot operations. The error handling appears correct and aligns with the intended functionality described in the PR.
store/v2/snapshots/manager_test.go (2)
258-317: Comprehensive testing of snapshot creation and restoration.This test function covers the process of taking a snapshot and then restoring it. The assertions are well-placed to ensure the snapshot's properties and the integrity of the restoration process. It's good to see thorough validation of the snapshot metadata and chunk handling.
However, ensure that the error handling for restoration after the target has contents (line 314-316) is intended to simulate a real-world scenario where the target's state can affect restoration success. If this is a common scenario, consider adding more detailed error handling or logging to inform the user why the restoration failed.
[APROVED]
319-396: Testing snapshot pruning under various conditions.The
TestSnapshot_Take_Prunefunction tests the pruning functionality under normal conditions and when a snapshot is being taken. The error handling on line 324 is appropriate as it prevents pruning operations during critical snapshot operations, which could lead to data loss or corruption.The tests also cover scenarios of repeated snapshot creation and pruning at the same height, which is crucial for ensuring the robustness of the snapshot management system. The assertions on lines 358 and 391 validate the expected behavior correctly.
|
|
||
| // Starting a new restore should fail now, because the target already has contents. | ||
| err = manager.Restore(*snapshot) | ||
| require.Error(t, err) |
There was a problem hiding this comment.
how to check if the restore is done? maybe add more confirmation after restoring
There was a problem hiding this comment.
oh this is the check for error when performing restore, i already added a confirmation after restoring above
There was a problem hiding this comment.
what I mean is, should we compare the restored data?
i think the snapshot_take_prune took care of that |
my intention is, the pruning manager should be blocked while taking snapshots, but not seeing this testcase |
got it i will add it in |
…smos-sdk into son/add_store_v2_tests
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)
- store/v2/snapshots/manager_test.go (1 hunks)
Additional context used
Path-based instructions (1)
store/v2/snapshots/manager_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"
Additional comments not posted (2)
store/v2/snapshots/manager_test.go (2)
258-341: Consider verifying the restored data.While the test covers snapshot creation and restoration, it would be beneficial to compare the restored data with the original data to ensure that the restoration process is accurate.
343-419: LGTM!The test function covers various scenarios for snapshot creation and pruning effectively.
letme add some quick tests in this PR |
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)
- store/v2/migration/manager_test.go (1 hunks)
- store/v2/snapshots/manager_test.go (1 hunks)
Additional context used
Path-based instructions (2)
store/v2/migration/manager_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"store/v2/snapshots/manager_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"
Additional comments not posted (3)
store/v2/migration/manager_test.go (1)
82-85: LGTM! The added checks enhance test coverage.The added lines ensure that the snapshot manager handles conflicts correctly during migration, which is crucial for maintaining data integrity.
store/v2/snapshots/manager_test.go (2)
258-341: Comprehensive test coverage for snapshot creation and restoration.The
TestSnapshot_Take_Restorefunction thoroughly tests the creation and restoration of snapshots, including handling errors for duplicate restores. This enhances the robustness of the snapshot management system.
343-420: Comprehensive test coverage for snapshot pruning.The
TestSnapshot_Take_Prunefunction thoroughly tests the pruning functionality of the snapshot manager, including handling errors for concurrent operations. This enhances the robustness of the snapshot management system.
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)
- store/v2/snapshots/manager_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- store/v2/snapshots/manager_test.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)
- store/v2/root/store_test.go (1 hunks)
Additional context used
Path-based instructions (1)
store/v2/root/store_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"
Additional comments not posted (1)
store/v2/root/store_test.go (1)
633-633: Verify the appropriateness of the increased timeout duration.The timeout duration for the
s.Require().Eventuallymethod has been increased from 2 seconds to 5 seconds. Ensure that this change is necessary and does not mask underlying performance issues.
* main: (48 commits) build(deps): add missing replaces and remove unnecessary ones (#21033) build(deps): Bump bufbuild/buf-setup-action from 1.34.0 to 1.35.0 (#21028) chore: fix some comments for struct field (#21027) chore(x): replace `fmt.Errorf` without parameters with `errors.New` (#21032) chore: fix errors reported by running `make lint` (#21015) ci: skip spelling check in go.mod/go.sum (#21021) chore(all)!: use gogoproto/any instead of codec/types/any (#21013) chore(server/v2/cometbft): ensure consistent dash-case in app.toml (#21018) docs(server): wrong function comments (#21017) refactor(storev2): update snapshot manager and migration manager tests (#20441) feat(server/v2/cometbft): config (#20989) refactor: set `help` as default target of Makefile (#21011) fix(simapp): duplicated import (#21014) chore(docs): fix functions and struct comments (#21010) fix(simapp/v2): panic with testnet init-files command (#21012) fix: make help won't work (#21005) fix: NewIntegrationApp does not write default genesis to state (#21006) chore(x/staking,x/upgrade): replace `fmt.Errorf` without parameters with `errors.New` (#21004) chore: prepare depinject 1.0.0 (#21001) docs: Fix typos in RFC Creation Process (#20998) ...
Description
ref: 20198
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.
I have...
Summary by CodeRabbit