-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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: add test examples for prefixdb.go #22752
base: main
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughA new test file named 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.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
store/db/prefixdb_test.go (2)
3-10
: Reorder imports according to style guidelinesThe imports should be grouped in the following order:
- Standard library
- Third-party packages
- First-party packages
Apply this diff to fix the import ordering:
import ( "testing" - "github.com/stretchr/testify/require" - "cosmossdk.io/store/db" - "cosmossdk.io/store/mock" - "go.uber.org/mock/gomock" + "go.uber.org/mock/gomock" + "github.com/stretchr/testify/require" + + "cosmossdk.io/store/db" + "cosmossdk.io/store/mock" )🧰 Tools
🪛 golangci-lint (1.62.2)
6-6: File is not
gci
-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order(gci)
9-9: File is not
gci
-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order(gci)
8-8: File is not
gofumpt
-ed with-extra
(gofumpt)
12-19
: Consider using table-driven tests for better coverageWhile the current test structure is functional, consider converting it to a table-driven test to:
- Test different prefix values
- Test edge cases (empty prefix, empty key)
- Test error scenarios from the underlying DB
Would you like me to provide an example of how to restructure this as a table-driven test?
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
store/db/prefixdb_test.go
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
store/db/prefixdb_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"
🪛 golangci-lint (1.62.2)
store/db/prefixdb_test.go
6-6: File is not gci
-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
9-9: File is not gci
-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
49-49: File is not gci
-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
8-8: File is not gofumpt
-ed with -extra
(gofumpt)
key := []byte("key1") | ||
value := []byte("value1") | ||
mockDB.EXPECT().Set(gomock.Eq(append(prefix, key...)), gomock.Eq(value)).Return(nil) | ||
|
||
err := pdb.Set(key, value) | ||
require.NoError(t, err) | ||
|
||
mockDB.EXPECT().Get(gomock.Eq(append(prefix, key...))).Return(value, nil) | ||
|
||
returnedValue, err := pdb.Get(key) | ||
require.NoError(t, err) | ||
require.Equal(t, value, returnedValue) | ||
|
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.
🛠️ Refactor suggestion
Add test cases for error scenarios and edge cases
The current test coverage for Set/Get operations could be enhanced by adding:
- Error scenarios from the underlying DB
- Edge cases with nil/empty values
- Concurrent access scenarios
- Large key/value pairs
Example test cases to add:
// Test error propagation
mockDB.EXPECT().Set(gomock.Any(), gomock.Any()).Return(errors.New("db error"))
err := pdb.Set(key, value)
require.Error(t, err)
// Test nil value
mockDB.EXPECT().Set(gomock.Any(), nil).Return(nil)
err = pdb.Set(key, nil)
require.NoError(t, err)
mockDB.EXPECT().Has(gomock.Eq(append(prefix, key...))).Return(true, nil) | ||
|
||
has, err := pdb.Has(key) | ||
require.NoError(t, err) | ||
require.True(t, has) | ||
|
||
mockDB.EXPECT().Delete(gomock.Eq(append(prefix, key...))).Return(nil) | ||
|
||
err = pdb.Delete(key) | ||
require.NoError(t, err) | ||
|
||
mockDB.EXPECT().Has(gomock.Eq(append(prefix, key...))).Return(false, nil) | ||
|
||
has, err = pdb.Has(key) | ||
require.NoError(t, err) | ||
require.False(t, has) | ||
} |
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.
🛠️ Refactor suggestion
Enhance Has/Delete operations test coverage
The test coverage for Has/Delete operations should include:
- Error scenarios from the underlying DB
- Multiple key operations
- Delete of non-existent keys
Example test cases to add:
// Test Has error propagation
mockDB.EXPECT().Has(gomock.Any()).Return(false, errors.New("db error"))
_, err = pdb.Has(key)
require.Error(t, err)
// Test Delete non-existent key
mockDB.EXPECT().Delete(gomock.Any()).Return(nil)
err = pdb.Delete([]byte("non-existent-key"))
require.NoError(t, err)
🧰 Tools
🪛 golangci-lint (1.62.2)
49-49: File is not gci
-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
package db_test | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/stretchr/testify/require" | ||
"cosmossdk.io/store/db" | ||
"cosmossdk.io/store/mock" | ||
"go.uber.org/mock/gomock" | ||
) | ||
|
||
func TestPrefixDB(t *testing.T) { | ||
mockCtrl := gomock.NewController(t) | ||
defer mockCtrl.Finish() | ||
|
||
mockDB := mock.NewMockKVStoreWithBatch(mockCtrl) | ||
prefix := []byte("test:") | ||
pdb := db.NewPrefixDB(mockDB, prefix) | ||
|
||
key := []byte("key1") | ||
value := []byte("value1") | ||
mockDB.EXPECT().Set(gomock.Eq(append(prefix, key...)), gomock.Eq(value)).Return(nil) | ||
|
||
err := pdb.Set(key, value) | ||
require.NoError(t, err) | ||
|
||
mockDB.EXPECT().Get(gomock.Eq(append(prefix, key...))).Return(value, nil) | ||
|
||
returnedValue, err := pdb.Get(key) | ||
require.NoError(t, err) | ||
require.Equal(t, value, returnedValue) | ||
|
||
mockDB.EXPECT().Has(gomock.Eq(append(prefix, key...))).Return(true, nil) | ||
|
||
has, err := pdb.Has(key) | ||
require.NoError(t, err) | ||
require.True(t, has) | ||
|
||
mockDB.EXPECT().Delete(gomock.Eq(append(prefix, key...))).Return(nil) | ||
|
||
err = pdb.Delete(key) | ||
require.NoError(t, err) | ||
|
||
mockDB.EXPECT().Has(gomock.Eq(append(prefix, key...))).Return(false, nil) | ||
|
||
has, err = pdb.Has(key) | ||
require.NoError(t, err) | ||
require.False(t, has) | ||
} |
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.
💡 Codebase verification
Test coverage needs significant improvement
The current test file is missing critical test coverage for required interface methods:
Iterator
andReverseIterator
methods are completely untested, despite being core requirements of the KVStore interface- Batch operations through the KVStoreWithBatch interface are not covered
- No tests for edge cases like empty prefix, nil keys, or concurrent access scenarios
Required test additions:
- Add tests for forward and reverse iteration over prefixed keys
- Include batch operation tests as required by KVStoreWithBatch interface
- Add tests for concurrent access safety (the struct has a mutex)
- Cover edge cases with empty/nil inputs
🔗 Analysis chain
Verify test coverage with additional scenarios
While the test provides good basic coverage, consider adding:
- Batch operations testing
- Iterator testing if supported by PrefixDB
- Performance tests for large datasets
- Recovery from panic scenarios
Run the following script to check test coverage:
Let me gather more information about the interface methods and implementations to provide a comprehensive assessment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for prefixdb package
# Search for PrefixDB methods that might need testing
ast-grep --pattern 'type PrefixDB struct {
$$$
}'
# Search for interface methods that should be tested
ast-grep --pattern 'type DB interface {
$$$
}'
Length of output: 547
Script:
#!/bin/bash
# Search for all methods implemented by PrefixDB
ast-grep --pattern 'func (p *PrefixDB) $_($$$) $$$'
# Search for KVStoreWithBatch interface definition
ast-grep --pattern 'type KVStoreWithBatch interface {
$$$
}'
# Search for any existing iterator or batch related tests
rg -l "Iterator|Batch" store/db/prefixdb_test.go
Length of output: 500
Script:
#!/bin/bash
# Let's check the KVStore interface methods
ast-grep --pattern 'type KVStore interface {
$$$
}'
# Check PrefixDB implementation for iterator and batch methods
rg -A 5 "func \(p \*PrefixDB\)" store/db/prefixdb.go
# Check if there are any other test files for PrefixDB
fd prefixdb.*test
Length of output: 1513
🧰 Tools
🪛 golangci-lint (1.62.2)
6-6: File is not gci
-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
9-9: File is not gci
-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
49-49: File is not gci
-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
8-8: File is not gofumpt
-ed with -extra
(gofumpt)
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.
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
PrefixDB
implementation, ensuring reliable operations for setting, getting, checking existence, and deleting keys.