test(integration): port x/bank tests to server/v2 app #21912
Conversation
Is there an example usage you can point to that the API should fulfill? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
tests/integration/v2/app.go (1)
46-51: Consider using a more descriptive constant name.The constant
DefaultGenTxGasis clear and descriptive. However, the constantsGenesis_COMMIT,Genesis_NOCOMMIT, andGenesis_SKIPcould benefit from more descriptive names to improve readability and maintainability. Consider prefixing them withGenesisMode_or a similar prefix to indicate their purpose more clearly.-const ( - Genesis_COMMIT = iota - Genesis_NOCOMMIT - Genesis_SKIP -) +const ( + GenesisMode_COMMIT = iota + GenesisMode_NOCOMMIT + GenesisMode_SKIP +)tests/integration/v2/bank/determinisitic_test.go (2)
285-288: Avoid redundant re-initialization of fixtureIn
TestQueryTotalSupply, after the rapid check, the fixture is re-initialized. Since the test is already isolated per iteration, re-initializing here might be unnecessary and could mask issues that arise from state changes during tests.Consider removing the re-initialization unless it's specifically required:
-f = initDeterministicFixture(t) // reset -f.ctx = f.app.StateLatestContext(t) -gasMeterFactory = integration.GasMeterFactory(f.ctx) -queryFn = queryFnFactory[*banktypes.QueryTotalSupplyRequest, *banktypes.QueryTotalSupplyResponse](f)
345-346: Handle error fromSetParamsinstead of just requiring no errorIn
TestQueryParams, the error fromf.bankKeeper.SetParamsis checked usingrequire.NoError, which will halt the test on failure. Consider handling or logging the error to provide more context if the test fails.Alternatively, ensure that the error provides sufficient context:
err := f.bankKeeper.SetParams(f.ctx, params) -require.NoError(t, err) +require.NoErrorf(t, err, "failed to set bank params: %v", err)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
tests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
simapp/v2/app_di.go(1 hunks)tests/go.mod(5 hunks)tests/integration/v2/app.go(1 hunks)tests/integration/v2/bank/app_test.go(1 hunks)tests/integration/v2/bank/determinisitic_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/go.mod
🧰 Additional context used
📓 Path-based instructions (4)
simapp/v2/app_di.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tests/integration/v2/app.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/integration/v2/bank/app_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/integration/v2/bank/determinisitic_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 (25)
simapp/v2/app_di.go (1)
51-51: Add a comment explaining DefaultProviders.
The addition of codec.DefaultProviders improves code organization by centralizing codec-related dependency injection. However, a brief comment would help explain its purpose and the providers it includes.
Add a comment above this line:
+// DefaultProviders includes standard codec providers (InterfaceRegistry, Amino, ProtoCodec, AddressCodec)
codec.DefaultProviders,Let's verify that this change doesn't break existing codec usage:
✅ Verification successful
Let me verify the usage of codec providers by checking where DefaultProviders is used to ensure there are no conflicts.
The addition of codec.DefaultProviders is safe and follows the intended usage pattern
The verification shows that:
DefaultProvidersis used consistently in both test and app configurations- Individual codec providers (
ProvideInterfaceRegistry,ProvideLegacyAmino, etc.) are properly bundled inDefaultProviders - No conflicts exist between
DefaultProvidersand individual provider usage, as direct provider usage occurs in test utilities and the codec package itself
The suggestion to add a comment remains valid for better code documentation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of individual codec providers that might conflict
rg -A 2 "ProvideInterfaceRegistry|ProvideLegacyAmino|ProvideProtoCodec|ProvideAddressCodec" --type go
Length of output: 2686
Script:
#!/bin/bash
# Search for DefaultProviders usage
rg -A 2 "DefaultProviders" --type go
Length of output: 473
tests/integration/v2/app.go (13)
1-44: LGTM!
The package declaration and imports are correctly defined. The package name integration is appropriate for an integration testing package. The imported packages are relevant and necessary for the functionality provided in this file.
53-53: LGTM!
The type alias stateMachineTx is appropriately defined and improves code readability by providing a more meaningful name for the transaction.Tx type.
55-76: LGTM!
The DefaultConsensusParams variable is correctly defined with appropriate values for each consensus parameter. The comments provide clear explanations of the purpose and usage of these default parameters.
78-90: LGTM!
The StartupConfig struct is well-defined with clear field names and comments explaining the purpose of each field. The struct encapsulates the necessary configuration options for initializing a new test application.
92-116: LGTM!
The DefaultStartUpConfig function is implemented correctly. It generates a default StartupConfig with appropriate values for testing purposes. The function uses helper functions to create a random validator set, generates a genesis account with a default balance, and sets up a temporary home directory for the application.
277-283: LGTM!
The App struct is correctly defined as a wrapper around runtime.App with additional testing utilities. The struct fields are appropriately named and typed, providing access to the underlying application, store, transaction config, and last block height.
302-310: LGTM!
The StateLatestContext method is implemented correctly. It retrieves the latest state from the store, creates a writable branch, and returns a new context with the state value set. This method is useful for obtaining a context with the latest state for testing purposes.
312-320: LGTM!
The Commit method is implemented correctly. It retrieves the state changes from the given writable state, creates a changeset, and commits the changes to the store. The method returns the new state hash or an error if the commit fails.
322-397: Verify the function signature change in the codebase.
The SignCheckDeliver method is quite complex and involves several steps for signing and delivering transactions. While the implementation looks correct, it's important to verify that the function signature change doesn't break any existing code that relies on this method.
Run the following script to verify the function usage:
#!/bin/bash
# Description: Verify all calls to `SignCheckDeliver` match the new signature.
# Test: Search for the method usage. Expect: Only occurrences of the new signature.
rg -A 5 $'SignCheckDeliver'This script searches for all occurrences of the SignCheckDeliver method and prints the surrounding lines. Review the output to ensure that all calls to this method have been updated to match the new signature, especially regarding the txErrString parameter.
399-406: LGTM!
The CheckBalance method is implemented correctly. It retrieves the balances of the given address using the provided bank keeper and asserts that the balances match the expected coins. This method is useful for verifying account balances in integration tests.
240-243:
Remove non-existent fields from DefaultConsensusParams usage.
The DefaultConsensusParams struct does not have fields named Abci, Synchrony, or Feature. Attempting to access these fields will result in a compilation error.
Apply this diff to remove the invalid field references:
Authority: "consensus",
Block: DefaultConsensusParams.Block,
Evidence: DefaultConsensusParams.Evidence,
Validator: DefaultConsensusParams.Validator,
- Abci: DefaultConsensusParams.Abci,
- Synchrony: DefaultConsensusParams.Synchrony,
- Feature: DefaultConsensusParams.Feature,
},251-251:
Avoid using time.Now() directly in tests for determinism.
Using time.Now() can introduce non-determinism in tests. Consider providing a fixed time or allowing the time to be injected, ensuring that test runs are consistent.
Apply this diff to use a fixed time:
- Time: time.Now(),
+ Time: fixedTime,Define fixedTime in your test setup:
fixedTime := time.Date(2024, time.January, 1, 0, 0, 0, 0, time.UTC)285-300: 🛠️ Refactor suggestion
Consider adding a helper DeliverAndCommit method.
The Deliver method is implemented correctly and provides a convenient way to deliver a block with the given transactions. However, it doesn't automatically commit the resulting state, which is a common pattern in integration tests.
Consider adding a new helper method DeliverAndCommit that combines the functionality of Deliver and Commit:
func (a *App) DeliverAndCommit(
t *testing.T, ctx context.Context, txs []stateMachineTx,
) (*server.BlockResponse, []byte) {
t.Helper()
resp, state := a.Deliver(t, ctx, txs)
hash, err := a.Commit(state)
require.NoError(t, err)
return resp, hash
}This method would simplify the usage in integration tests by automatically committing the state after delivering the block.
tests/integration/v2/bank/app_test.go (7)
106-143: LGTM!
The TestSendNotEnoughBalance test case is well-structured and covers the scenario of sending coins with insufficient balance. It verifies the expected behavior and checks the account balances and sequences after the transaction.
145-237: LGTM!
The TestMsgMultiSendWithAccounts test case thoroughly covers various scenarios for the MsgMultiSend message type. It includes test cases for valid transactions, wrong account numbers, wrong account sequences, and multiple inputs. The test cases are well-defined and cover the expected behavior and error conditions.
239-291: LGTM!
The TestMsgMultiSendMultipleOut test case covers the scenario of sending coins to multiple output addresses using the MsgMultiSend message type. It sets up the necessary accounts, funds them, and verifies the expected balances after the transaction. The test case is well-structured and covers the desired functionality.
293-351: LGTM!
The TestMsgMultiSendDependent test case covers the scenario of sending coins between dependent accounts using the MsgMultiSend message type. It sets up the necessary accounts with different account numbers, funds them, and verifies the expected balances after the transactions. The test case is well-structured and covers the desired functionality.
353-430: LGTM!
The TestMsgSetSendEnabled test case covers the functionality of the MsgSetSendEnabled message type. It includes test cases for wrong authority, right authority with wrong signer, and submitting a valid governance proposal. The test cases are well-defined and cover the expected behavior and error conditions.
432-469: LGTM!
The TestSendToNonExistingAccount test case covers the scenario of sending coins to a non-existing account. It verifies that the account is not created when sending coins to it, but the balance is updated. It also checks that the account is created when sending coins back from the non-existing account. The test case is well-structured and covers the desired functionality.
40-40:
Handle the error returned by secec.NewPrivateKeyFromScalar.
In line 40, the error returned by secec.NewPrivateKeyFromScalar is ignored. It's best practice in Go to handle errors to prevent unexpected issues.
Apply the following changes to handle the error:
var (
- stablePrivateKey, _ = secec.NewPrivateKeyFromScalar(secp256k1_internal.NewScalarFromUint64(100))
+ stablePrivateKey, err = secec.NewPrivateKeyFromScalar(secp256k1_internal.NewScalarFromUint64(100))
)
+require.NoError(t, err)Likely invalid or redundant comment.
tests/integration/v2/bank/determinisitic_test.go (4)
523-536: Ensure addresses are valid and unique in TestDenomOwners
In TestDenomOwners, random addresses are generated, but there could be a risk of duplicates or invalid addresses, which might affect the test outcome.
Add a check to ensure that addresses are unique and valid. Run the following script to verify uniqueness:
#!/bin/bash
# Description: Check for duplicate addresses in TestDenomOwners
# Expected: No duplicate addresses in the generated list
awk '/addr := testdata.AddressGenerator/ {print $0}' tests/integration/v2/bank/deterministic_test.go | sort | uniq -d215-216:
Avoid potential panic due to duplicate denominations
In TestQuerySpendableBalances, using rapid.StringMatching(denomRegex) directly might generate duplicate denominations, leading to a panic when creating sdk.NewCoins. Ensure that the generated denominations are unique.
Use rapid.SliceOfNDistinct to guarantee unique denominations:
- denoms := rapid.SliceOf(rapid.StringMatching(denomRegex)).Filter(func(denoms []string) bool {
- return len(denoms) >= 1 && len(denoms) <= 10
- }).Draw(rt, "denoms")
+ denoms := rapid.SliceOfNDistinct(
+ rapid.StringMatching(denomRegex),
+ 1,
+ 10,
+ rapid.ID[string],
+ ).Draw(rt, "denoms")Likely invalid or redundant comment.
63-71:
Improve type assertion to prevent potential panic
In the queryFnFactory function, the type assertion could cause a panic if res is not of the expected type ResponseT. To enhance robustness and prevent potential runtime errors, consider introducing an ok check for the type assertion.
Apply this diff to safely perform the type assertion:
func queryFnFactory[RequestT, ResponseT proto.Message](
f *deterministicFixture,
) func(RequestT) (ResponseT, error) {
return func(req RequestT) (ResponseT, error) {
- var emptyResponse ResponseT
res, err := f.app.Query(f.ctx, 0, req)
if err != nil {
- return emptyResponse, err
+ var zero ResponseT
+ return zero, err
}
- castedRes, ok := res.(ResponseT)
+ castedRes, ok := res.(ResponseT)
if !ok {
- return emptyResponse, fmt.Errorf("unexpected response type: %T", res)
+ return zero, fmt.Errorf("unexpected response type: %T", res)
}
return castedRes, nil
}
}Likely invalid or redundant comment.
380-395: Validate metadata fields to ensure proper test coverage
In createAndReturnMetadatas, random strings are generated for metadata fields without validation. This could lead to invalid metadata, especially if empty strings or invalid formats are generated.
Ensure that the generated metadata fields meet any required validation criteria. You can add validation logic or constraints to the random generation. Run the following script to check for empty or invalid fields:
| } | ||
|
|
||
| store := storeBuilder.Get() | ||
| if store == nil { | ||
| return nil, fmt.Errorf("failed to build store: %w", err) | ||
| } | ||
| err = store.SetInitialVersion(1) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to set initial version: %w", err) | ||
| } | ||
|
|
||
| integrationApp := &App{App: app, Store: store, txConfig: txConfig, lastHeight: 1} | ||
| if startupConfig.GenesisBehavior == Genesis_SKIP { | ||
| return integrationApp, nil | ||
| } | ||
|
|
||
| // create validator set | ||
| valSet, err := startupConfig.ValidatorSet() | ||
| if err != nil { | ||
| return nil, errors.New("failed to create validator set") | ||
| } | ||
|
|
||
| var ( | ||
| balances []banktypes.Balance | ||
| genAccounts []authtypes.GenesisAccount | ||
| ) | ||
| for _, ga := range startupConfig.GenesisAccounts { | ||
| genAccounts = append(genAccounts, ga.GenesisAccount) | ||
| balances = append( | ||
| balances, | ||
| banktypes.Balance{ | ||
| Address: ga.GenesisAccount.GetAddress().String(), | ||
| Coins: ga.Coins, | ||
| }, | ||
| ) | ||
| } | ||
|
|
||
| genesisJSON, err := genesisStateWithValSet( | ||
| cdc, | ||
| app.DefaultGenesis(), | ||
| valSet, | ||
| genAccounts, | ||
| balances...) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create genesis state: %w", err) | ||
| } | ||
|
|
||
| // init chain must be called to stop deliverState from being nil | ||
| genesisJSONBytes, err := cmtjson.MarshalIndent(genesisJSON, "", " ") | ||
| if err != nil { | ||
| return nil, fmt.Errorf( | ||
| "failed to marshal default genesis state: %w", | ||
| err, | ||
| ) | ||
| } | ||
|
|
||
| ctx := context.WithValue( | ||
| context.Background(), | ||
| corecontext.CometParamsInitInfoKey, | ||
| &consensustypes.MsgUpdateParams{ | ||
| Authority: "consensus", | ||
| Block: DefaultConsensusParams.Block, | ||
| Evidence: DefaultConsensusParams.Evidence, | ||
| Validator: DefaultConsensusParams.Validator, | ||
| Abci: DefaultConsensusParams.Abci, | ||
| Synchrony: DefaultConsensusParams.Synchrony, | ||
| Feature: DefaultConsensusParams.Feature, | ||
| }, | ||
| ) | ||
|
|
||
| emptyHash := sha256.Sum256(nil) | ||
| _, genesisState, err := app.InitGenesis( | ||
| ctx, | ||
| &server.BlockRequest[stateMachineTx]{ | ||
| Height: 1, | ||
| Time: time.Now(), | ||
| Hash: emptyHash[:], | ||
| ChainId: "test-chain", | ||
| AppHash: emptyHash[:], | ||
| IsGenesis: true, | ||
| }, | ||
| genesisJSONBytes, | ||
| &genesisTxCodec{txConfigOptions}, | ||
| ) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed init genesis: %w", err) | ||
| } | ||
|
|
||
| if startupConfig.GenesisBehavior == Genesis_NOCOMMIT { | ||
| integrationApp.lastHeight = 0 | ||
| return integrationApp, nil | ||
| } | ||
|
|
||
| _, err = integrationApp.Commit(genesisState) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to commit initial version: %w", err) | ||
| } | ||
|
|
||
| return integrationApp, nil | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Error handling is properly implemented with one minor inconsistency
The error handling in the NewApp function is generally well-implemented with proper error wrapping and propagation, but there is one inconsistency:
- At line 147, the error is returned without wrapping:
return nil, errors.New("failed to create validator set")while there's anerrvariable available that should be wrapped.
All other error cases properly:
- Use descriptive error messages
- Wrap the underlying error using
%w - Return both nil and the error
- Follow consistent error message formatting
🔗 Analysis chain
Verify error handling and propagation.
The NewApp function is quite complex and involves multiple steps for initializing a new application. While the error handling seems to be in place, it's important to verify that all potential errors are properly handled and propagated to the caller.
To verify the error handling, you can use the following script:
This script checks if all errors are handled and returned using fmt.Errorf, and if all errors are propagated to the caller using %w in the error format string. If any of these checks fail, it indicates that there might be some errors that are not properly handled or propagated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if all errors are handled and returned
if ! rg -q -e 'return nil, fmt.Errorf'; then
echo "Not all errors are handled and returned"
exit 1
fi
# Check if all errors are propagated to the caller
if ! rg -q -e 'return nil, fmt.Errorf("failed to.*: %w", err)'; then
echo "Not all errors are propagated to the caller"
exit 1
fi
echo "Error handling and propagation looks good"
Length of output: 201
Script:
#!/bin/bash
# Let's check all error handling patterns in the NewApp function
rg -A 1 "return nil, " "tests/integration/v2/app.go"
# Also check for any direct error returns without wrapping
rg -A 1 "return nil, err" "tests/integration/v2/app.go"
# Check for any other error handling patterns we might have missed
rg -A 1 "if err [!:]" "tests/integration/v2/app.go"
Length of output: 1763
There was a problem hiding this comment.
@octavio12345300, understood.
| if exists, _ := coins.Find(coin.Denom); exists { | ||
| t.Skip("duplicate denom") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid skipping tests due to duplicate denominations
In TestQueryAllBalances, the test skips when a duplicate denomination is found. Skipping tests within deterministic iterations can reduce test coverage and may hide potential issues. Instead of skipping, consider regenerating the coin or ensuring denominations are unique from the start.
Refactor the coin generation to ensure uniqueness without needing to skip:
for i := 0; i < numCoins; i++ {
- coin := getCoin(rt)
- if exists, _ := coins.Find(coin.Denom); exists {
- t.Skip("duplicate denom")
- }
+ denom := rapid.StringMatching(denomRegex).Filter(func(d string) bool {
+ _, exists := coins.Find(d)
+ return !exists
+ }).Draw(rt, "denom")
+ coin := sdk.NewCoin(denom, math.NewInt(rapid.Int64Min(1).Draw(rt, "amount")))
// NewCoins sorts the denoms
coins = sdk.NewCoins(append(coins, coin)...)
}Committable suggestion skipped: line range outside the PR's diff.
| addrStr, err := codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(addr) | ||
| require.NoError(t, err) | ||
|
|
There was a problem hiding this comment.
Handle potential error from address conversion
In TestQueryBalance, the error from BytesToString(addr) is being checked, but the test continues even if there's an error. This could lead to unexpected behavior if addrStr is not valid. Ensure that the test exits or handles the error appropriately upon failure.
Modify the error handling to fail the test if addrStr is not obtained:
addrStr, err := codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(addr)
if err != nil {
- require.NoError(t, err)
+ t.Fatalf("failed to convert address bytes to string: %v", err)
}Committable suggestion skipped: line range outside the PR's diff.
| allDenoms := []string{} | ||
|
|
||
| rapid.Check(t, func(rt *rapid.T) { | ||
| count := rapid.IntRange(0, 10).Draw(rt, "count") | ||
| denoms := make([]string, 0, count) | ||
|
|
||
| for i := 0; i < count; i++ { | ||
| coin := banktypes.SendEnabled{ | ||
| Denom: rapid.StringMatching(denomRegex).Draw(rt, "denom"), | ||
| Enabled: rapid.Bool().Draw(rt, "enabled-status"), | ||
| } | ||
|
|
||
| f.bankKeeper.SetSendEnabled(f.ctx, coin.Denom, coin.Enabled) | ||
| denoms = append(denoms, coin.Denom) | ||
| } | ||
|
|
||
| allDenoms = append(allDenoms, denoms...) |
There was a problem hiding this comment.
Prevent race conditions by avoiding shared mutable state
In TestSendEnabled, the allDenoms slice is shared across parallel tests, which can lead to race conditions when appending to it concurrently.
Make allDenoms a local variable within the test function to prevent concurrent access:
func TestSendEnabled(t *testing.T) {
t.Parallel()
f := initDeterministicFixture(t)
f.ctx = f.app.StateLatestContext(t)
gasMeterFactory := integration.GasMeterFactory(f.ctx)
queryFn := queryFnFactory[*banktypes.QuerySendEnabledRequest, *banktypes.QuerySendEnabledResponse](f)
- allDenoms := []string{}
rapid.Check(t, func(rt *rapid.T) {
+ allDenoms := []string{}
count := rapid.IntRange(0, 10).Draw(rt, "count")
denoms := make([]string, 0, count)
for i := 0; i < count; i++ {
coin := banktypes.SendEnabled{
Denom: rapid.StringMatching(denomRegex).Draw(rt, "denom"),
Enabled: rapid.Bool().Draw(rt, "enabled-status"),
}
f.bankKeeper.SetSendEnabled(f.ctx, coin.Denom, coin.Enabled)
denoms = append(denoms, coin.Denom)
}
allDenoms = append(allDenoms, denoms...)Committable suggestion skipped: line range outside the PR's diff.
| numCoins := rapid.IntRange(1, 3).Draw(rt, "num-count") | ||
| coins := make(sdk.Coins, 0, numCoins) | ||
|
|
||
| for i := 0; i < numCoins; i++ { | ||
| coin := sdk.NewCoin( | ||
| rapid.StringMatching(denomRegex).Draw(rt, "denom"), | ||
| math.NewInt(rapid.Int64Min(1).Draw(rt, "amount")), | ||
| ) | ||
|
|
||
| coins = coins.Add(coin) | ||
| } | ||
|
|
||
| require.NoError(t, f.bankKeeper.MintCoins(f.ctx, minttypes.ModuleName, coins)) | ||
|
|
||
| initialSupply = initialSupply.Add(coins...) |
There was a problem hiding this comment.
Reset state between iterations to prevent state leakage
In TestQueryTotalSupply, the initialSupply is accumulated across iterations, which can cause tests to depend on state from previous runs. This may lead to flaky tests or incorrect assertions.
Reset initialSupply at the beginning of each iteration:
rapid.Check(t, func(rt *rapid.T) {
+ res, err := queryFn(&banktypes.QueryTotalSupplyRequest{})
+ require.NoError(t, err)
+ initialSupply := res.GetSupply()
+
numCoins := rapid.IntRange(1, 3).Draw(rt, "num-count")
coins := make(sdk.Coins, 0, numCoins)
for i := 0; i < numCoins; i++ {
coin := sdk.NewCoin(
rapid.StringMatching(denomRegex).Draw(rt, "denom"),
math.NewInt(rapid.Int64Min(1).Draw(rt, "amount")),
)
coins = coins.Add(coin)
}
require.NoError(t, f.bankKeeper.MintCoins(f.ctx, minttypes.ModuleName, coins))
- initialSupply = initialSupply.Add(coins...)Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@octavio12345300, the new unit tests in deterministic_test.go provide comprehensive coverage of the bank module's query functionalities. The use of deterministic iterations and rapid testing ensures robustness across various scenarios. Great job on enhancing the test coverage!
julienrbrt
added
the
backport/v0.52.x
| @@ -440,3 +441,71 @@ func NewAppConfig(opts ...ModuleOption) depinject.Config { | |||
|
|
|||
| return appconfig.Compose(&appv1alpha1.Config{Modules: modules}) | |||
| } | |||
|
|
|||
| func NewAppV2Config(opts ...ModuleOption) depinject.Config { | |||
There was a problem hiding this comment.
I'd put a go.mod in that tests/integration/v2 package and move that there too
There was a problem hiding this comment.
A bit like we do for system tests
There was a problem hiding this comment.
lgtm!
@sontrinh16 will probably do a follow with what is missing
should the store interface in runtime v2 has commit func ? or we could just use store v2 store interface |
|
@kocubinski can i push in this PR, i can help finish this PR |
Feel free to open a PR targeting this branch |
It looks like |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests/integration/v2/app.go (1)
54-77: Consider movingDefaultConsensusParamsto a separate configuration file.While having
DefaultConsensusParamsdefined here is acceptable, consider moving it to a separate configuration file (e.g.,config.go) to keep the main application file focused on the core logic.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
tests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
tests/go.mod(5 hunks)tests/integration/v2/app.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/go.mod
🧰 Additional context used
📓 Path-based instructions (1)
tests/integration/v2/app.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 (10)
tests/integration/v2/app.go (10)
1-52: LGTM!
The package declaration, imports, and constants look good. The constants are well-defined and follow the naming conventions.
79-91: LGTM!
The StartupConfig struct is well-defined with clear comments explaining each field. The field names are descriptive and follow the naming conventions.
93-117: LGTM!
The DefaultStartUpConfig function is implemented correctly. It generates a default configuration with a random validator set, genesis accounts, and a temporary home directory for testing.
278-284: LGTM!
The App struct is a wrapper around runtime.App that provides additional testing utilities. The struct fields are well-defined and follow the naming conventions.
286-301: LGTM!
The Deliver method is implemented correctly. It delivers a block with the given transactions, updates the application state, and returns the block response and the resulting state.
303-311: LGTM!
The StateLatestContext method is implemented correctly. It creates a new context with the latest application state for testing purposes.
313-321: LGTM!
The Commit method is implemented correctly. It commits the given state changes and returns the new state hash.
400-407: LGTM!
The CheckBalance method is implemented correctly. It checks the balance of the given address against the expected balance using the bank keeper.
119-276: Verify error handling and propagation.
The NewApp function is quite complex and involves multiple steps for initializing a new application. While the error handling seems to be in place, it's important to verify that all potential errors are properly handled and propagated to the caller.
Run the following script to verify the error handling:
✅ Verification successful
Error handling is properly implemented with clear error wrapping
The error handling in the NewApp function is well-implemented:
- All error cases return both nil and a wrapped error
- Error messages are descriptive and follow a consistent "failed to X" pattern
- Errors are properly wrapped using
fmt.Errorfwith%wverb for error chain preservation - One case uses
errors.Newwhich is appropriate as it's a new error without wrapping
The only exception is the marshal default genesis state error which spans multiple lines but still follows the same pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if all errors are handled and returned
if ! rg -q -e 'return nil, fmt.Errorf'; then
echo "Not all errors are handled and returned"
exit 1
fi
# Check if all errors are propagated to the caller
if ! rg -q -e 'return nil, fmt.Errorf("failed to.*: %w", err)'; then
echo "Not all errors are propagated to the caller"
exit 1
fi
echo "Error handling and propagation looks good"
Length of output: 201
Script:
#!/bin/bash
# Let's examine all error handling patterns in the NewApp function
rg -A 2 "if err.*{" "tests/integration/v2/app.go"
# Check for direct error returns without wrapping
rg "return.*err[^o]" "tests/integration/v2/app.go"
# Check for all error handling patterns
rg "return nil, (fmt\.Errorf|errors\.|err)" "tests/integration/v2/app.go"
Length of output: 1473
323-398: Verify the correctness of the signing process.
The SignCheckDeliver method is responsible for signing and delivering a set of messages. It's crucial to ensure that the signing process is implemented correctly and follows the expected behavior.
Run the following script to verify the signing process:
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
tests/integration/v2/bank/determinisitic_test.go (7)
59-74: Enhance error handling in queryFnFactoryThe
queryFnFactoryfunction is well-implemented, but it could benefit from improved error handling. Consider returning the error immediately iff.app.Queryreturns an error, and use a type assertion with anokcheck to handle unexpected response types gracefully.Apply this diff to enhance the error handling:
func queryFnFactory[RequestT, ResponseT proto.Message]( f *deterministicFixture, ) func(RequestT) (ResponseT, error) { return func(req RequestT) (ResponseT, error) { var emptyResponse ResponseT res, err := f.app.Query(f.ctx, 0, req) if err != nil { + return emptyResponse, err } castedRes, ok := res.(ResponseT) if !ok { return emptyResponse, fmt.Errorf("unexpected response type: %T", res) } return castedRes, nil } }
123-156: Enhance test coverage for balance queriesThe
TestQueryBalancefunction provides a good starting point for testing balance queries. To further improve test coverage, consider the following suggestions:
- Test querying balances for multiple accounts with different denominations.
- Test querying balances for non-existent accounts or denominations.
- Verify that the returned balance matches the expected balance after performing transactions (e.g., transfers, minting, burning).
These additional test cases will help ensure the robustness of the balance querying functionality.
301-325: Add assertions to TestQueryTotalSupplyOfThe
TestQueryTotalSupplyOffunction is well-structured, but it lacks specific assertions on the returned supply.Consider adding an assertion to verify the correctness of the returned supply:
testdata.DeterministicIterationsV2( - t, req, gasMeterFactory, queryFn, assertNonZeroGas, nil) + t, req, gasMeterFactory, queryFn, assertNonZeroGas, func(t *testing.T, res *banktypes.QuerySupplyOfResponse) { + require.Equal(t, coin.Amount, res.Amount.Amount, "supply of %s should match the minted amount", coin.Denom) + })This assertion will ensure that the supply returned by the query matches the minted amount for the specific denomination.
327-366: Add assertions to TestQueryParamsThe
TestQueryParamsfunction is well-structured, but it lacks specific assertions on the returned parameters.Consider adding assertions to verify the correctness of the returned parameters:
testdata.DeterministicIterationsV2( - t, req, gasMeterFactory, queryFn, assertNonZeroGas, nil) + t, req, gasMeterFactory, queryFn, assertNonZeroGas, func(t *testing.T, res *banktypes.QueryParamsResponse) { + require.Equal(t, params.DefaultSendEnabled, res.Params.DefaultSendEnabled, "DefaultSendEnabled should match") + require.Equal(t, len(params.SendEnabled), len(res.Params.SendEnabled), "SendEnabled length should match") + for i, sendEnabled := range params.SendEnabled { + require.Equal(t, sendEnabled.Denom, res.Params.SendEnabled[i].Denom, "SendEnabled denom should match") + require.Equal(t, sendEnabled.Enabled, res.Params.SendEnabled[i].Enabled, "SendEnabled status should match") + } + })These assertions will ensure that the parameters returned by the query match the expected values set in the test.
403-437: Enhance metadata-related test assertionsThe
TestDenomsMetadatafunction is well-implemented, but it could benefit from more specific assertions to ensure the correctness of the returned metadata.Consider adding the following assertion:
testdata.DeterministicIterationsV2( - t, req, gasMeterFactory, queryFn, assertNonZeroGas, nil) + t, req, gasMeterFactory, queryFn, assertNonZeroGas, func(t *testing.T, res *banktypes.QueryDenomsMetadataResponse) { + require.Equal(t, len(denomsMetadata), len(res.Metadatas), "number of metadata entries should match") + for _, expected := range denomsMetadata { + found := false + for _, actual := range res.Metadatas { + if expected.Base == actual.Base { + require.Equal(t, expected, actual, "metadata for denom %s should match", expected.Base) + found = true + break + } + } + require.True(t, found, "metadata for denom %s should be present in the response", expected.Base) + } + })This addition will ensure that the returned metadata values match the expected metadata set in the test.
439-465: Enhance metadata-related test assertionsThe
TestDenomMetadatafunction is well-implemented, but it could benefit from more specific assertions to ensure the correctness of the returned metadata.Consider adding the following assertion:
testdata.DeterministicIterationsV2( - t, req, gasMeterFactory, queryFn, assertNonZeroGas, nil) + t, req, gasMeterFactory, queryFn, assertNonZeroGas, func(t *testing.T, res *banktypes.QueryDenomMetadataResponse) { + require.Equal(t, denomMetadata[0], res.Metadata, "returned metadata should match the set metadata") + })This addition will ensure that the returned metadata value matches the expected metadata set in the test.
518-570: Enhance assertions in DenomOwners testThe
TestDenomOwnersfunction is well-implemented, but it could benefit from more specific assertions to ensure the correctness of the returned data.Consider adding the following assertion:
testdata.DeterministicIterationsV2( - t, req, gasMeterFactory, queryFn, assertNonZeroGas, nil) + t, req, gasMeterFactory, queryFn, assertNonZeroGas, func(t *testing.T, res *banktypes.QueryDenomOwnersResponse) { + require.Equal(t, len(denomOwners), len(res.DenomOwners), "number of DenomOwners should match") + for _, expected := range denomOwners { + found := false + for _, actual := range res.DenomOwners { + if expected.Address == actual.Address { + require.Equal(t, expected.Balance.Denom, actual.Balance.Denom, "denom should match for address %s", expected.Address) + require.True(t, expected.Balance.Amount.Equal(actual.Balance.Amount), "amount should match for address %s", expected.Address) + found = true + break + } + } + require.True(t, found, "DenomOwner for address %s should be present in the response", expected.Address) + } + })This addition will ensure that the returned DenomOwners data matches the expected values set in the test.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
tests/integration/v2/app.go(1 hunks)tests/integration/v2/bank/determinisitic_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/v2/app.go
🧰 Additional context used
📓 Path-based instructions (1)
tests/integration/v2/bank/determinisitic_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 (2)
tests/integration/v2/bank/determinisitic_test.go (2)
88-116: LGTM!
The initDeterministicFixture function is well-structured and sets up the necessary components for the deterministic fixture. It initializes the mock account keeper, configures the app, and returns the fixture with the required fields.
203-248: LGTM!
The TestQuerySpendableBalances function is well-implemented and tests the querying of spendable balances. It generates random test cases using rapid testing and also includes a fixed test case. The use of FundAccount and DeterministicIterationsV2 ensures thorough testing of the functionality.
| func TestQueryTotalSupply(t *testing.T) { | ||
| t.Parallel() | ||
| f := initDeterministicFixture(t) | ||
| f.ctx = f.app.StateLatestContext(t) | ||
| gasMeterFactory := integration.GasMeterFactory(f.ctx) | ||
| queryFn := queryFnFactory[*banktypes.QueryTotalSupplyRequest, *banktypes.QueryTotalSupplyResponse](f) | ||
|
|
||
| res, err := queryFn(&banktypes.QueryTotalSupplyRequest{}) | ||
| require.NoError(t, err) | ||
| initialSupply := res.GetSupply() | ||
|
|
||
| rapid.Check(t, func(rt *rapid.T) { | ||
| numCoins := rapid.IntRange(1, 3).Draw(rt, "num-count") | ||
| coins := make(sdk.Coins, 0, numCoins) | ||
|
|
||
| for i := 0; i < numCoins; i++ { | ||
| coin := sdk.NewCoin( | ||
| rapid.StringMatching(denomRegex).Draw(rt, "denom"), | ||
| math.NewInt(rapid.Int64Min(1).Draw(rt, "amount")), | ||
| ) | ||
|
|
||
| coins = coins.Add(coin) | ||
| } | ||
|
|
||
| require.NoError(t, f.bankKeeper.MintCoins(f.ctx, minttypes.ModuleName, coins)) | ||
|
|
||
| initialSupply = initialSupply.Add(coins...) | ||
|
|
||
| req := &banktypes.QueryTotalSupplyRequest{ | ||
| Pagination: testdata.PaginationGenerator(rt, uint64(len(initialSupply))).Draw(rt, "pagination"), | ||
| } | ||
|
|
||
| testdata.DeterministicIterationsV2(t, req, gasMeterFactory, queryFn, assertNonZeroGas, nil) | ||
| }) | ||
|
|
||
| f = initDeterministicFixture(t) // reset | ||
| f.ctx = f.app.StateLatestContext(t) | ||
| gasMeterFactory = integration.GasMeterFactory(f.ctx) | ||
| queryFn = queryFnFactory[*banktypes.QueryTotalSupplyRequest, *banktypes.QueryTotalSupplyResponse](f) | ||
|
|
||
| coins := sdk.NewCoins( | ||
| sdk.NewCoin("foo", math.NewInt(10)), | ||
| sdk.NewCoin("bar", math.NewInt(100)), | ||
| ) | ||
|
|
||
| require.NoError(t, f.bankKeeper.MintCoins(f.ctx, minttypes.ModuleName, coins)) | ||
|
|
||
| req := &banktypes.QueryTotalSupplyRequest{} | ||
| testdata.DeterministicIterationsV2(t, req, gasMeterFactory, queryFn, assertNonZeroGas, nil) | ||
| } |
There was a problem hiding this comment.
Reset state between iterations to prevent state leakage
In the TestQueryTotalSupply function, the initialSupply is accumulated across iterations, which can cause tests to depend on state from previous runs. This may lead to flaky tests or incorrect assertions.
Reset initialSupply at the beginning of each iteration:
rapid.Check(t, func(rt *rapid.T) {
+ res, err := queryFn(&banktypes.QueryTotalSupplyRequest{})
+ require.NoError(t, err)
+ initialSupply := res.GetSupply()
+
numCoins := rapid.IntRange(1, 3).Draw(rt, "num-count")
coins := make(sdk.Coins, 0, numCoins)
for i := 0; i < numCoins; i++ {
coin := sdk.NewCoin(
rapid.StringMatching(denomRegex).Draw(rt, "denom"),
math.NewInt(rapid.Int64Min(1).Draw(rt, "amount")),
)
coins = coins.Add(coin)
}
require.NoError(t, f.bankKeeper.MintCoins(f.ctx, minttypes.ModuleName, coins))
- initialSupply = initialSupply.Add(coins...)Committable suggestion skipped: line range outside the PR's diff.
| func TestQueryAllBalances(t *testing.T) { | ||
| t.Parallel() | ||
| f := initDeterministicFixture(t) | ||
| f.ctx = f.app.StateLatestContext(t) | ||
| gasMeterFactory := integration.GasMeterFactory(f.ctx) | ||
| addressCodec := codectestutil.CodecOptions{}.GetAddressCodec() | ||
| queryFn := queryFnFactory[*banktypes.QueryAllBalancesRequest, *banktypes.QueryAllBalancesResponse](f) | ||
|
|
||
| rapid.Check(t, func(rt *rapid.T) { | ||
| addr := testdata.AddressGenerator(rt).Draw(rt, "address") | ||
| numCoins := rapid.IntRange(1, 10).Draw(rt, "num-count") | ||
| coins := make(sdk.Coins, 0, numCoins) | ||
|
|
||
| addrStr, err := addressCodec.BytesToString(addr) | ||
| require.NoError(t, err) | ||
|
|
||
| for i := 0; i < numCoins; i++ { | ||
| coin := getCoin(rt) | ||
| if exists, _ := coins.Find(coin.Denom); exists { | ||
| t.Skip("duplicate denom") | ||
| } | ||
| // NewCoins sorts the denoms | ||
| coins = sdk.NewCoins(append(coins, coin)...) | ||
| } | ||
|
|
||
| fundAccount(f, addr, coins...) | ||
|
|
||
| req := banktypes.NewQueryAllBalancesRequest( | ||
| addrStr, testdata.PaginationGenerator(rt, uint64(numCoins)).Draw(rt, "pagination"), false) | ||
| testdata.DeterministicIterationsV2(t, req, gasMeterFactory, queryFn, assertNonZeroGas, nil) | ||
| }) | ||
|
|
||
| coins := sdk.NewCoins( | ||
| sdk.NewCoin("stake", math.NewInt(10)), | ||
| sdk.NewCoin("denom", math.NewInt(100)), | ||
| ) | ||
|
|
||
| fundAccount(f, addr1, coins...) | ||
| addr1Str, err := addressCodec.BytesToString(addr1) | ||
| require.NoError(t, err) | ||
|
|
||
| req := banktypes.NewQueryAllBalancesRequest(addr1Str, nil, false) | ||
| testdata.DeterministicIterationsV2(t, req, gasMeterFactory, queryFn, assertNonZeroGas, nil) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid skipping tests due to duplicate denominations
In the TestQueryAllBalances function, the test skips when a duplicate denomination is found. Skipping tests within deterministic iterations can reduce test coverage and may hide potential issues. Instead of skipping, consider regenerating the coin or ensuring denominations are unique from the start.
Refactor the coin generation to ensure uniqueness without needing to skip:
for i := 0; i < numCoins; i++ {
- coin := getCoin(rt)
- if exists, _ := coins.Find(coin.Denom); exists {
- t.Skip("duplicate denom")
- }
+ denom := rapid.StringMatching(denomRegex).Filter(func(d string) bool {
+ _, exists := coins.Find(d)
+ return !exists
+ }).Draw(rt, "denom")
+ coin := sdk.NewCoin(denom, math.NewInt(rapid.Int64Min(1).Draw(rt, "amount")))
// NewCoins sorts the denoms
coins = sdk.NewCoins(append(coins, coin)...)
}Committable suggestion skipped: line range outside the PR's diff.
| func TestSendEnabled(t *testing.T) { | ||
| t.Parallel() | ||
| f := initDeterministicFixture(t) | ||
| f.ctx = f.app.StateLatestContext(t) | ||
| gasMeterFactory := integration.GasMeterFactory(f.ctx) | ||
| queryFn := queryFnFactory[*banktypes.QuerySendEnabledRequest, *banktypes.QuerySendEnabledResponse](f) | ||
| allDenoms := []string{} | ||
|
|
||
| rapid.Check(t, func(rt *rapid.T) { | ||
| count := rapid.IntRange(0, 10).Draw(rt, "count") | ||
| denoms := make([]string, 0, count) | ||
|
|
||
| for i := 0; i < count; i++ { | ||
| coin := banktypes.SendEnabled{ | ||
| Denom: rapid.StringMatching(denomRegex).Draw(rt, "denom"), | ||
| Enabled: rapid.Bool().Draw(rt, "enabled-status"), | ||
| } | ||
|
|
||
| f.bankKeeper.SetSendEnabled(f.ctx, coin.Denom, coin.Enabled) | ||
| denoms = append(denoms, coin.Denom) | ||
| } | ||
|
|
||
| allDenoms = append(allDenoms, denoms...) | ||
|
|
||
| req := &banktypes.QuerySendEnabledRequest{ | ||
| Denoms: denoms, | ||
| // Pagination is only taken into account when `denoms` is an empty array | ||
| Pagination: testdata.PaginationGenerator(rt, uint64(len(allDenoms))).Draw(rt, "pagination"), | ||
| } | ||
| testdata.DeterministicIterationsV2(t, req, gasMeterFactory, queryFn, assertNonZeroGas, nil) | ||
| }) | ||
|
|
||
| coin1 := banktypes.SendEnabled{ | ||
| Denom: "falsecoin", | ||
| Enabled: false, | ||
| } | ||
| coin2 := banktypes.SendEnabled{ | ||
| Denom: "truecoin", | ||
| Enabled: true, | ||
| } | ||
|
|
||
| f.bankKeeper.SetSendEnabled(f.ctx, coin1.Denom, false) | ||
| f.bankKeeper.SetSendEnabled(f.ctx, coin2.Denom, true) | ||
|
|
||
| req := &banktypes.QuerySendEnabledRequest{ | ||
| Denoms: []string{coin1.GetDenom(), coin2.GetDenom()}, | ||
| } | ||
|
|
||
| testdata.DeterministicIterationsV2(t, req, gasMeterFactory, queryFn, assertNonZeroGas, nil) | ||
| } |
There was a problem hiding this comment.
Prevent race conditions by avoiding shared mutable state
In the TestSendEnabled function, the allDenoms slice is shared across parallel tests, which can lead to race conditions when appending to it concurrently.
Make allDenoms a local variable within the test function to prevent concurrent access:
func TestSendEnabled(t *testing.T) {
t.Parallel()
f := initDeterministicFixture(t)
f.ctx = f.app.StateLatestContext(t)
gasMeterFactory := integration.GasMeterFactory(f.ctx)
queryFn := queryFnFactory[*banktypes.QuerySendEnabledRequest, *banktypes.QuerySendEnabledResponse](f)
- allDenoms := []string{}
rapid.Check(t, func(rt *rapid.T) {
+ allDenoms := []string{}
count := rapid.IntRange(0, 10).Draw(rt, "count")
denoms := make([]string, 0, count)
for i := 0; i < count; i++ {
coin := banktypes.SendEnabled{
Denom: rapid.StringMatching(denomRegex).Draw(rt, "denom"),
Enabled: rapid.Bool().Draw(rt, "enabled-status"),
}
f.bankKeeper.SetSendEnabled(f.ctx, coin.Denom, coin.Enabled)
denoms = append(denoms, coin.Denom)
}
allDenoms = append(allDenoms, denoms...)Committable suggestion skipped: line range outside the PR's diff.
(cherry picked from commit b5993d6) # Conflicts: # server/v2/appmanager/appmanager.go # server/v2/stf/core_header_service.go # tests/go.mod
… (#22457) Co-authored-by: Matt Kocubinski <mkocubinski@gmail.com> Co-authored-by: Julien Robert <julien@rbrt.fr>
Description
This PR creates integration tests for x/bank in server/v2 architecture by doing the following:
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
New Features
Bug Fixes
Tests
Chores