-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
refactor(store/v2): Migrate some v1 tests for rootmultistore #20368
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThe changes introduce two new functions, Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant Tester
participant RootStoreTestSuite
participant Store
Tester->>RootStoreTestSuite: Call newStoreWithPruneConfig(config)
RootStoreTestSuite->>Store: Initialize store with prune configuration
Store-->>RootStoreTestSuite: Store initialized
Tester->>RootStoreTestSuite: Call newStoreWithBackendMount(ss, sc)
RootStoreTestSuite->>Store: Initialize store with backend mount
Store-->>RootStoreTestSuite: Store initialized
Tester->>RootStoreTestSuite: Run TestPrune
RootStoreTestSuite->>Store: Perform prune operations
Store-->>RootStoreTestSuite: Prune results
Tester->>RootStoreTestSuite: Run TestMultiStore_Pruning_SameHeightsTwice
RootStoreTestSuite->>Store: Perform prune operations at same heights twice
Store-->>RootStoreTestSuite: Prune results
Tester->>RootStoreTestSuite: Run TestMultiStore_PruningRestart
RootStoreTestSuite->>Store: Perform prune operations and restart
Store-->>RootStoreTestSuite: Prune and restart results
Tester->>RootStoreTestSuite: Run TestMultiStoreRestart
RootStoreTestSuite->>Store: Perform store restart operations
Store-->>RootStoreTestSuite: Restart results
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 Configration File (
|
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: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- store/v2/root/store_test.go (2 hunks)
Files not reviewed due to errors (1)
- store/v2/root/store_test.go (no review received)
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"
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: 2
Outside diff range and nitpick comments (1)
store/v2/root/store_test.go (1)
82-89
: Consider adding detailed comments explaining the purpose ofnewStoreWithBackendMount
.Adding comments will improve maintainability and understandability of the code, especially for new contributors.
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- store/v2/root/store_test.go (2 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 (3)
store/v2/root/store_test.go (3)
413-462
: Verify the behavior ofTestMultiStore_Pruning_SameHeightsTwice
.
536-650
: Ensure thatTestMultiStoreRestart
correctly handles state after restarts.
63-80
: Ensure proper error handling innewStoreWithPruneConfig
.
store/v2/root/store_test.go
Outdated
func (s *RootStoreTestSuite) TestMultiStore_PruningRestart() { | ||
// perform changes | ||
cs := corestore.NewChangeset() | ||
cs.Add(testStoreKeyBytes, []byte("key"), []byte("val"), false) | ||
|
||
pruneOpt := store.PruneOptions{ | ||
KeepRecent: 2, | ||
Interval: 11, | ||
} | ||
|
||
noopLog := log.NewNopLogger() | ||
|
||
sqliteDB, err := sqlite.New(s.T().TempDir()) | ||
s.Require().NoError(err) | ||
|
||
ss := storage.NewStorageStore(sqliteDB, &pruneOpt, noopLog) | ||
|
||
tree := iavl.NewIavlTree(dbm.NewMemDB(), noopLog, iavl.DefaultConfig()) | ||
|
||
sc, err := commitment.NewCommitStore(map[string]commitment.Tree{testStoreKey: tree}, dbm.NewMemDB(), &pruneOpt, noopLog) | ||
s.Require().NoError(err) | ||
|
||
s.newStoreWithBackendMount(ss, sc) | ||
s.Require().NoError(s.rootStore.LoadLatestVersion()) | ||
|
||
// Commit enough to build up heights to prune, where on the next block we should | ||
// batch delete. | ||
for i := uint64(0); i < 10; i++ { | ||
// execute Commit | ||
cHash, err := s.rootStore.Commit(cs) | ||
s.Require().NoError(err) | ||
s.Require().NotNil(cHash) | ||
} | ||
|
||
latestVer, err := s.rootStore.GetLatestVersion() | ||
s.Require().NoError(err) | ||
|
||
ok, actualHeightToPrune := pruneOpt.ShouldPrune(latestVer) | ||
s.Require().False(ok) | ||
s.Require().Equal(uint64(0), actualHeightToPrune) | ||
|
||
// "restart" | ||
s.newStoreWithBackendMount(ss, sc) | ||
err = s.rootStore.LoadLatestVersion() | ||
s.Require().NoError(err) | ||
|
||
latestVer, err = s.rootStore.GetLatestVersion() | ||
s.Require().NoError(err) | ||
|
||
ok, actualHeightToPrune = pruneOpt.ShouldPrune(latestVer) | ||
s.Require().False(ok) | ||
s.Require().Equal(uint64(0), actualHeightToPrune) | ||
|
||
// commit one more block and ensure the heights have been pruned | ||
// execute Commit | ||
cHash, err := s.rootStore.Commit(cs) | ||
s.Require().NoError(err) | ||
s.Require().NotNil(cHash) | ||
|
||
latestVer, err = s.rootStore.GetLatestVersion() | ||
s.Require().NoError(err) | ||
|
||
ok, actualHeightToPrune = pruneOpt.ShouldPrune(latestVer) | ||
s.Require().True(ok) | ||
s.Require().Equal(uint64(8), actualHeightToPrune) | ||
|
||
for v := uint64(1); v <= actualHeightToPrune; v++ { | ||
err := s.rootStore.LoadVersion(v) | ||
s.Require().Error(err, "expected error when loading height: %d", v) | ||
} | ||
} |
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.
Check for potential optimizations in TestMultiStore_PruningRestart
.
The repeated setup and teardown within the test could be optimized by using setup and teardown hooks or by abstracting common logic into helper functions.
store/v2/root/store_test.go
Outdated
func (s *RootStoreTestSuite) TestPrune() { | ||
// perform changes | ||
cs := corestore.NewChangeset() | ||
for i := 0; i < 10; i++ { | ||
key := fmt.Sprintf("key%03d", i) // key000, key001, ..., key099 | ||
val := fmt.Sprintf("val%03d", i) // val000, val001, ..., val099 | ||
|
||
cs.Add(testStoreKeyBytes, []byte(key), []byte(val), false) | ||
} | ||
|
||
testCases := []struct { | ||
name string | ||
numVersions int64 | ||
po store.PruneOptions | ||
deleted []uint64 | ||
saved []uint64 | ||
}{ | ||
{"prune nothing", 10, *store.DefaultPruneOptions(), nil, []uint64{1, 2, 3, 4, 5, 6, 7, 8, 9, 10}}, | ||
{"prune everything", 12, store.PruneOptions{ | ||
KeepRecent: 1, | ||
Interval: 10, | ||
}, []uint64{1, 2, 3, 4, 5, 6, 7, 8}, []uint64{9, 10, 11, 12}}, | ||
{"prune some; no batch", 10, store.PruneOptions{ | ||
KeepRecent: 2, | ||
Interval: 1, | ||
}, []uint64{1, 2, 3, 4, 6, 5, 7}, []uint64{8, 9, 10}}, | ||
{"prune some; small batch", 10, store.PruneOptions{ | ||
KeepRecent: 2, | ||
Interval: 3, | ||
}, []uint64{1, 2, 3, 4, 5, 6}, []uint64{7, 8, 9, 10}}, | ||
{"prune some; large batch", 10, store.PruneOptions{ | ||
KeepRecent: 2, | ||
Interval: 11, | ||
}, nil, []uint64{1, 2, 3, 4, 5, 6, 7, 8, 9, 10}}, | ||
} | ||
|
||
for _, tc := range testCases { | ||
tc := tc | ||
|
||
s.newStoreWithPruneConfig(&tc.po) | ||
|
||
// write keys over multiple versions | ||
for i := int64(0); i < tc.numVersions; i++ { | ||
// execute Commit | ||
cHash, err := s.rootStore.Commit(cs) | ||
s.Require().NoError(err) | ||
s.Require().NotNil(cHash) | ||
} | ||
|
||
for _, v := range tc.saved { | ||
ro, err := s.rootStore.StateAt(v) | ||
s.Require().NoError(err, "expected no error when loading height %d at test %s", v, tc.name) | ||
|
||
for i := 0; i < 10; i++ { | ||
key := fmt.Sprintf("key%03d", i) // key000, key001, ..., key099 | ||
val := fmt.Sprintf("val%03d", i) // val000, val001, ..., val099 | ||
|
||
reader, err := ro.GetReader(testStoreKeyBytes) | ||
s.Require().NoError(err) | ||
result, err := reader.Get([]byte(key)) | ||
s.Require().NoError(err) | ||
s.Require().Equal([]byte(val), result, "value should be equal for test: %s", tc.name) | ||
} | ||
} | ||
|
||
for _, v := range tc.deleted { | ||
_, err := s.rootStore.StateAt(v) | ||
s.Require().Error(err, "expected error when loading height %d at test %s", v, tc.name) | ||
} | ||
} | ||
} |
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.
Review the logic in TestPrune
for potential simplifications.
The test case structure and the repeated setup for each test scenario could be simplified or abstracted to reduce redundancy and improve readability.
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: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- store/v2/root/store_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- store/v2/root/store_test.go
i will add a test for TestUnevenStoresHeightCheck after this PR get merged |
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: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- store/v2/root/store_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- store/v2/root/store_test.go
@@ -58,6 +58,34 @@ func (s *RootStoreTestSuite) SetupTest() { | |||
s.rootStore = rs | |||
} | |||
|
|||
func (s *RootStoreTestSuite) newStoreWithPruneConfig(config *store.PruneOptions) { |
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 doc would be useful
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.
is this pr still needed?
is resolve first item of this issue |
Description
Closes: #XXXX
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.md
Reviewers 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
New Features
Tests