chore(store/v2): Increase storev2 tests coverage #21094
Conversation
WalkthroughWalkthroughThe changes enhance the testing framework across various packages by introducing multiple new test functions that validate essential functionalities, such as proof retrieval, migration behavior, pruning operations, and snapshot management. Additionally, existing tests have been modified to improve timing reliability, promoting better robustness and concurrency handling within the system. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CommitStore
participant MigrationManager
participant PruningManager
participant Database
participant SnapshotManager
Client->>CommitStore: Commit Changes
CommitStore->>CommitStore: Retrieve Proofs
CommitStore->>PruningManager: Signal Prune
PruningManager->>CommitStore: Validate Data Access
Client->>MigrationManager: Start Migration
MigrationManager->>CommitStore: Apply Changesets
MigrationManager->>Database: Restore State
Database->>Client: Return Restored Data
Client->>SnapshotManager: Take Snapshot
SnapshotManager->>CommitStore: Create Snapshot
PruningManager->>SnapshotManager: Validate Pruning
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 (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…s/cosmos-sdk into son/increase_storev2_tests
…s/cosmos-sdk into son/increase_storev2_tests
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (8)
- store/v2/commitment/store_test_suite.go (1 hunks)
- store/v2/migration/manager_test.go (2 hunks)
- store/v2/pruning/manager_test.go (1 hunks)
- store/v2/root/store_test.go (1 hunks)
- store/v2/snapshots/manager.go (2 hunks)
- store/v2/snapshots/manager_test.go (2 hunks)
- store/v2/storage/storage_test_suite.go (1 hunks)
- tests/e2e/accounts/lockup/periodic_lockup_test_suite.go (2 hunks)
Files skipped from review due to trivial changes (1)
- tests/e2e/accounts/lockup/periodic_lockup_test_suite.go
Additional context used
Path-based instructions (7)
store/v2/commitment/store_test_suite.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.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/pruning/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"store/v2/snapshots/manager.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/storage/storage_test_suite.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.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 (7)
store/v2/commitment/store_test_suite.go (2)
168-209: Ensure comprehensive coverage of edge cases.The function
TestStore_GetProoftests the proof retrieval functionality. It is important to ensure that all edge cases, such as empty keys or non-existent versions, are covered.
211-243: Ensure comprehensive coverage of edge cases.The function
TestStore_Gettests the value retrieval functionality. It is important to ensure that all edge cases, such as empty keys or non-existent versions, are covered.store/v2/migration/manager_test.go (1)
122-220: Ensure comprehensive coverage of edge cases.The function
TestStartMigrateStatetests the migration process. It is important to ensure that all edge cases, such as empty changesets or interrupted migrations, are covered.store/v2/pruning/manager_test.go (1)
156-258: Ensure comprehensive coverage of edge cases.The function
TestSignalCommittests the interaction between commit signaling and pruning. It is important to ensure that all edge cases, such as interrupted commits or concurrent pruning operations, are covered.store/v2/snapshots/manager.go (2)
118-123: LGTM! Ensure proper concurrency handling.The method correctly locks the mutex to ensure thread safety when accessing the current operation.
174-176: LGTM! Ensure proper error handling.The method correctly retrieves the latest snapshot from the store and handles potential errors.
store/v2/root/store_test.go (1)
633-633: Increase timeout duration and polling interval.The timeout duration has been increased from 5 seconds to 10 seconds, and the polling interval has been changed from 100 milliseconds to 1 second. This adjustment should enhance the robustness of the test by reducing the likelihood of false negatives due to timing issues.
store/v2/snapshots/manager_test.go
Outdated
| func TestSnapshot_SnapshotIfApplicable(t *testing.T) { | ||
| store := setupStore(t) | ||
|
|
||
| items := [][]byte{ | ||
| {1, 2, 3}, | ||
| {4, 5, 6}, | ||
| {7, 8, 9}, | ||
| } | ||
| commitSnapshotter := &mockCommitSnapshotter{ | ||
| items: items, | ||
| } | ||
| extSnapshotter := newExtSnapshotter(10) | ||
|
|
||
| snapshotOpts := snapshots.NewSnapshotOptions(1, 1) | ||
|
|
||
| manager := snapshots.NewManager(store, snapshotOpts, commitSnapshotter, &mockStorageSnapshotter{}, nil, coretesting.NewNopLogger()) | ||
| err := manager.RegisterExtensions(extSnapshotter) | ||
| require.NoError(t, err) | ||
|
|
||
| manager.SnapshotIfApplicable(4) | ||
|
|
||
| checkLatestHeight := func() bool { | ||
| latestSnapshot, _ := manager.GetLatestSnapshot() | ||
| return latestSnapshot.Height == 4 | ||
| } | ||
|
|
||
| require.Eventually(t, checkLatestHeight, time.Second*10, time.Second) | ||
|
|
||
| pruned, err := manager.Prune(1) | ||
| require.NoError(t, err) | ||
| require.Equal(t, uint64(0), pruned) |
There was a problem hiding this comment.
Ensure proper error handling for GetLatestSnapshot.
The function does not handle potential errors from GetLatestSnapshot. Ensure error handling to avoid unexpected behavior.
- checkLatestHeight := func() bool {
- latestSnapshot, _ := manager.GetLatestSnapshot()
- return latestSnapshot.Height == 4
- }
+ checkLatestHeight := func() bool {
+ latestSnapshot, err := manager.GetLatestSnapshot()
+ if err != nil {
+ return false
+ }
+ return latestSnapshot.Height == 4
+ }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.
| func TestSnapshot_SnapshotIfApplicable(t *testing.T) { | |
| store := setupStore(t) | |
| items := [][]byte{ | |
| {1, 2, 3}, | |
| {4, 5, 6}, | |
| {7, 8, 9}, | |
| } | |
| commitSnapshotter := &mockCommitSnapshotter{ | |
| items: items, | |
| } | |
| extSnapshotter := newExtSnapshotter(10) | |
| snapshotOpts := snapshots.NewSnapshotOptions(1, 1) | |
| manager := snapshots.NewManager(store, snapshotOpts, commitSnapshotter, &mockStorageSnapshotter{}, nil, coretesting.NewNopLogger()) | |
| err := manager.RegisterExtensions(extSnapshotter) | |
| require.NoError(t, err) | |
| manager.SnapshotIfApplicable(4) | |
| checkLatestHeight := func() bool { | |
| latestSnapshot, _ := manager.GetLatestSnapshot() | |
| return latestSnapshot.Height == 4 | |
| } | |
| require.Eventually(t, checkLatestHeight, time.Second*10, time.Second) | |
| pruned, err := manager.Prune(1) | |
| require.NoError(t, err) | |
| require.Equal(t, uint64(0), pruned) | |
| func TestSnapshot_SnapshotIfApplicable(t *testing.T) { | |
| store := setupStore(t) | |
| items := [][]byte{ | |
| {1, 2, 3}, | |
| {4, 5, 6}, | |
| {7, 8, 9}, | |
| } | |
| commitSnapshotter := &mockCommitSnapshotter{ | |
| items: items, | |
| } | |
| extSnapshotter := newExtSnapshotter(10) | |
| snapshotOpts := snapshots.NewSnapshotOptions(1, 1) | |
| manager := snapshots.NewManager(store, snapshotOpts, commitSnapshotter, &mockStorageSnapshotter{}, nil, coretesting.NewNopLogger()) | |
| err := manager.RegisterExtensions(extSnapshotter) | |
| require.NoError(t, err) | |
| manager.SnapshotIfApplicable(4) | |
| checkLatestHeight := func() bool { | |
| latestSnapshot, err := manager.GetLatestSnapshot() | |
| if err != nil { | |
| return false | |
| } | |
| return latestSnapshot.Height == 4 | |
| } | |
| require.Eventually(t, checkLatestHeight, time.Second*10, time.Second) | |
| pruned, err := manager.Prune(1) | |
| require.NoError(t, err) | |
| require.Equal(t, uint64(0), pruned) |
| func (s *StorageTestSuite) TestDatabase_Restore() { | ||
| db, err := s.NewDB(s.T().TempDir()) | ||
| s.Require().NoError(err) | ||
| defer db.Close() | ||
|
|
||
| toVersion := uint64(10) | ||
| keyCount := 10 | ||
|
|
||
| // for versions 1-10, set 10 keys | ||
| for v := uint64(1); v <= toVersion; v++ { | ||
| cs := corestore.NewChangesetWithPairs(map[string]corestore.KVPairs{storeKey1: {}}) | ||
| for i := 0; i < keyCount; i++ { | ||
| key := fmt.Sprintf("key%03d", i) | ||
| val := fmt.Sprintf("val%03d-%03d", i, v) | ||
|
|
||
| cs.AddKVPair(storeKey1Bytes, corestore.KVPair{Key: []byte(key), Value: []byte(val)}) | ||
| } | ||
|
|
||
| s.Require().NoError(db.ApplyChangeset(v, cs)) | ||
| } | ||
|
|
||
| latestVersion, err := db.GetLatestVersion() | ||
| s.Require().NoError(err) | ||
| s.Require().Equal(uint64(10), latestVersion) | ||
|
|
||
| chStorage := make(chan *corestore.StateChanges, 5) | ||
|
|
||
| go func() { | ||
| for i := uint64(11); i <= 15; i++ { | ||
| kvPairs := []corestore.KVPair{} | ||
| for j := 0; j < keyCount; j++ { | ||
| key := fmt.Sprintf("key%03d-%03d", j, i) | ||
| val := fmt.Sprintf("val%03d-%03d", j, i) | ||
|
|
||
| kvPairs = append(kvPairs, corestore.KVPair{Key: []byte(key), Value: []byte(val)}) | ||
| } | ||
| chStorage <- &corestore.StateChanges{ | ||
| Actor: storeKey1Bytes, | ||
| StateChanges: kvPairs, | ||
| } | ||
| } | ||
| close(chStorage) | ||
| }() | ||
|
|
||
| // restore with snapshot version smaller than latest version | ||
| // should return an error | ||
| err = db.Restore(9, chStorage) | ||
| s.Require().Error(err) | ||
|
|
||
| // restore | ||
| err = db.Restore(11, chStorage) | ||
| s.Require().NoError(err) | ||
|
|
||
| // check the storage | ||
| for i := uint64(11); i <= 15; i++ { | ||
| for j := 0; j < keyCount; j++ { | ||
| key := fmt.Sprintf("key%03d-%03d", j, i) | ||
| val := fmt.Sprintf("val%03d-%03d", j, i) | ||
|
|
||
| v, err := db.Get(storeKey1Bytes, 11, []byte(key)) | ||
| s.Require().NoError(err) | ||
| s.Require().Equal([]byte(val), v) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure proper synchronization when accessing shared resources.
The function uses a goroutine to simulate state changes. Ensure proper synchronization to avoid potential data races.
- go func() {
- for i := uint64(11); i <= 15; i++ {
- kvPairs := []corestore.KVPair{}
- for j := 0; j < keyCount; j++ {
- key := fmt.Sprintf("key%03d-%03d", j, i)
- val := fmt.Sprintf("val%03d-%03d", j, i)
- kvPairs = append(kvPairs, corestore.KVPair{Key: []byte(key), Value: []byte(val)})
- }
- chStorage <- &corestore.StateChanges{
- Actor: storeKey1Bytes,
- StateChanges: kvPairs,
- }
- }
- close(chStorage)
- }()
+ go func() {
+ defer close(chStorage)
+ for i := uint64(11); i <= 15; i++ {
+ kvPairs := []corestore.KVPair{}
+ for j := 0; j < keyCount; j++ {
+ key := fmt.Sprintf("key%03d-%03d", j, i)
+ val := fmt.Sprintf("val%03d-%03d", j, i)
+ kvPairs = append(kvPairs, corestore.KVPair{Key: []byte(key), Value: []byte(val)})
+ }
+ chStorage <- &corestore.StateChanges{
+ Actor: storeKey1Bytes,
+ StateChanges: kvPairs,
+ }
+ time.Sleep(10 * time.Millisecond) // avoid potential data races
+ }
+ }()Committable suggestion was skipped due to low confidence.
|
overall lgtm, I've merged the upgrade PR, maybe need to sync |
gotcha |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (5)
- store/v2/commitment/store_test_suite.go (1 hunks)
- store/v2/migration/manager_test.go (2 hunks)
- store/v2/pruning/manager_test.go (1 hunks)
- store/v2/root/store_test.go (1 hunks)
- store/v2/storage/storage_test_suite.go (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- store/v2/commitment/store_test_suite.go
- store/v2/migration/manager_test.go
- store/v2/pruning/manager_test.go
- store/v2/root/store_test.go
- store/v2/storage/storage_test_suite.go
|
could you please revert new APIs? |
…s/cosmos-sdk into son/increase_storev2_tests
removed |
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- store/v2/snapshots/manager.go (1 hunks)
- store/v2/snapshots/manager_test.go (5 hunks)
Additional context used
Path-based instructions (2)
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"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 (2)
store/v2/snapshots/manager_test.go (1)
527-530: Ensure proper error handling forGetLatestSnapshot.The function does not handle potential errors from
GetLatestSnapshot. Ensure error handling to avoid unexpected behavior.- checkLatestHeight := func() bool { - latestSnapshot, _ := manager.GetLatestSnapshot() - return latestSnapshot.Height == 4 - } + checkLatestHeight := func() bool { + latestSnapshot, err := manager.GetLatestSnapshot() + if err != nil { + return false + } + return latestSnapshot.Height == 4 + }store/v2/snapshots/manager.go (1)
167-169: LGTM!The method encapsulates the call to
m.store.GetLatest(), streamlining access to the latest snapshot data. The code changes are approved.
| go func() { | ||
| checkError := func() bool { | ||
| _, err = manager.Prune(1) | ||
| return err != nil | ||
| } | ||
|
|
||
| require.Eventually(t, checkError, time.Millisecond*200, time.Millisecond) | ||
| }() |
There was a problem hiding this comment.
Ensure proper error handling for manager.Prune in the goroutine.
The function does not handle potential errors from manager.Prune. Ensure error handling to avoid unexpected behavior.
- go func() {
- checkError := func() bool {
- _, err = manager.Prune(1)
- return err != nil
- }
- require.Eventually(t, checkError, time.Millisecond*200, time.Millisecond)
- }()
+ go func() {
+ checkError := func() bool {
+ _, err := manager.Prune(1)
+ if err != nil {
+ return true
+ }
+ return false
+ }
+ require.Eventually(t, checkError, time.Millisecond*200, time.Millisecond)
+ }()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.
| go func() { | |
| checkError := func() bool { | |
| _, err = manager.Prune(1) | |
| return err != nil | |
| } | |
| require.Eventually(t, checkError, time.Millisecond*200, time.Millisecond) | |
| }() | |
| go func() { | |
| checkError := func() bool { | |
| _, err := manager.Prune(1) | |
| if err != nil { | |
| return true | |
| } | |
| return false | |
| } | |
| require.Eventually(t, checkError, time.Millisecond*200, time.Millisecond) | |
| }() |
| go func() { | ||
| checkError := func() bool { | ||
| _, err := manager.Create(4) | ||
| return err != nil | ||
| } | ||
|
|
||
| require.Eventually(t, checkError, time.Millisecond*200, time.Millisecond) | ||
| }() |
There was a problem hiding this comment.
Ensure proper error handling for manager.Create in the goroutine.
The function does not handle potential errors from manager.Create. Ensure error handling to avoid unexpected behavior.
- go func() {
- checkError := func() bool {
- _, err := manager.Create(4)
- return err != nil
- }
- require.Eventually(t, checkError, time.Millisecond*200, time.Millisecond)
- }()
+ go func() {
+ checkError := func() bool {
+ _, err := manager.Create(4)
+ if err != nil {
+ return true
+ }
+ return false
+ }
+ require.Eventually(t, checkError, time.Millisecond*200, time.Millisecond)
+ }()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.
| go func() { | |
| checkError := func() bool { | |
| _, err := manager.Create(4) | |
| return err != nil | |
| } | |
| require.Eventually(t, checkError, time.Millisecond*200, time.Millisecond) | |
| }() | |
| go func() { | |
| checkError := func() bool { | |
| _, err := manager.Create(4) | |
| if err != nil { | |
| return true | |
| } | |
| return false | |
| } | |
| require.Eventually(t, checkError, time.Millisecond*200, time.Millisecond) | |
| }() |
|
@cool-develope i added the percentage in the PR description |
…s/cosmos-sdk into son/increase_storev2_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 (5 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 (3)
store/v2/snapshots/manager_test.go (3)
455-462: Ensure proper error handling formanager.Createin the goroutine.The function does not handle potential errors from
manager.Create. Ensure error handling to avoid unexpected behavior.
483-490: Ensure proper error handling formanager.Prunein the goroutine.The function does not handle potential errors from
manager.Prune. Ensure error handling to avoid unexpected behavior.
528-529: Ensure proper error handling forGetLatest.The function does not handle potential errors from
GetLatest. Ensure error handling to avoid unexpected behavior.
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)
Files skipped from review as they are similar to previous changes (1)
- store/v2/root/store_test.go
Description
ref: #21040
migration 73.0%
pruning 82.4%
snapshot 51.1%
commitment with iavl backend 71.2% ( result from
go tool cover -html=coverage.out)storage 70.2% ( result from
go tool cover -html=coverage.out)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
Tests