refactor(tests/integration): Port distribution integration tests to server v2 #22667
Conversation
…//github.com/cosmos/cosmos-sdk into son/port_auth_integration_tests_to_server_v2
…//github.com/cosmos/cosmos-sdk into son/port_auth_integration_tests_to_server_v2
…//github.com/cosmos/cosmos-sdk into son/port_auth_integration_tests_to_server_v2
📝 WalkthroughWalkthroughThe changes in this pull request enhance the application's integration testing framework by introducing a new Changes
Suggested labels
Possibly related PRs
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
|
…://github.com/cosmos/cosmos-sdk into son/port_distr_integration_tests_to_server_v2
sontrinh16
requested review from
facundomedica,
testinginprod,
JulianToledano,
ziscky and
a team
as code owners
sontrinh16
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (10)
tests/integration/v2/distribution/grpc_query_test.go (3)
345-345: Useassert.NilError(t, err)instead ofassert.Assert(t, err == nil)For consistency and clarity in error checking, replace
assert.Assert(t, err == nil)withassert.NilError(t, err).Apply this change:
- assert.Assert(t, err == nil) + assert.NilError(t, err)
208-211: Declare variables in the narrowest possible scopeVariables
reqandexpResare declared at a package level. For better readability and to prevent unintended side effects, declare these variables within the test cases or themalleatefunctions where they are used.Apply this change:
func TestGRPCValidatorSlashes(t *testing.T) { t.Parallel() f := createTestFixture(t) - var ( - req *types.QueryValidatorSlashesRequest - expRes *types.QueryValidatorSlashesResponse - ) testCases := []struct { name string malleate func() expPass bool expErrMsg string }{ { name: "empty request", malleate: func() { + var req *types.QueryValidatorSlashesRequest + var expRes *types.QueryValidatorSlashesResponse req = &types.QueryValidatorSlashesRequest{} expRes = &types.QueryValidatorSlashesResponse{} }, expPass: false, expErrMsg: "empty validator address", }, // ... other test cases ... } // ... rest of the function ... }Repeat similar changes in
TestGRPCCommunityPool:func TestGRPCCommunityPool(t *testing.T) { t.Parallel() f := createTestFixture(t) - var ( - req *types.QueryCommunityPoolRequest - expPool *types.QueryCommunityPoolResponse - ) testCases := []struct { name string malleate func() }{ { name: "valid request empty community pool", malleate: func() { + var req *types.QueryCommunityPoolRequest + var expPool *types.QueryCommunityPoolResponse req = &types.QueryCommunityPoolRequest{} expPool = &types.QueryCommunityPoolResponse{} }, }, // ... other test cases ... } // ... rest of the function ... }Also applies to: 386-389
345-345: Consistent error handling usingassert.NilErrorFor consistency and clarity in error assertions, replace
assert.Assert(t, err == nil)withassert.NilError(t, err).Apply this change:
- assert.Assert(t, err == nil) + assert.NilError(t, err)tests/integration/v2/distribution/msg_server_test.go (1)
206-279: Refactor repeatedpreRunfunctions to reduce code duplicationThe
preRunfunctions in your test cases repeatedly set theWithdrawAddrEnabledparameter. Consider extracting this repeated code into a helper function to reduce duplication and improve maintainability.tests/integration/v2/distribution/module_test.go (1)
11-15: Add test documentation and strengthen assertionsWhile the test function is well-structured, consider the following improvements:
- Add a function comment explaining the test's purpose and expectations
- Strengthen the assertion to verify the specific type and properties of the module account
Consider updating the test like this:
+// TestItCreatesModuleAccountOnInitBlock ensures that the distribution module account +// is properly created during block initialization with the expected permissions. func TestItCreatesModuleAccountOnInitBlock(t *testing.T) { f := createTestFixture(t) acc := f.authKeeper.GetAccount(f.ctx, authtypes.NewModuleAddress(types.ModuleName)) - assert.Assert(t, acc != nil) + assert.Assert(t, acc != nil, "distribution module account should exist") + moduleAcc, ok := acc.(authtypes.ModuleAccountI) + assert.Assert(t, ok, "account should be a module account") + assert.Equal(t, types.ModuleName, moduleAcc.GetName()) }tests/integration/v2/distribution/common_test.go (1)
16-16: Add function documentationPlease add a doc comment explaining the function's purpose and parameters. This will improve test maintainability and help other developers understand the test setup.
Example:
+// setupValidatorWithCommission initializes a validator with specific commission rates for testing. +// Parameters: +// - t: Testing context +// - f: Test fixture containing keepers and context +// - valAddr: Validator's address +// - initialStake: Initial staking amount for the validator func setupValidatorWithCommission(t *testing.T, f *fixture, valAddr sdk.ValAddress, initialStake int64) {tests/integration/v2/auth/app_test.go (1)
Line range hint
41-103: Consider adding HeaderService-specific test casesWhile the test suite structure is solid, consider adding specific test cases to verify the HeaderService integration, such as:
- Header information retrieval
- State transitions with header updates
- Error scenarios for header-related operations
This would ensure comprehensive coverage of the new v2 server integration.
tests/integration/v2/distribution/fixture_test.go (1)
70-140: Consider breaking down the createTestFixture function.While the function is working correctly, it's handling multiple responsibilities. Consider extracting the following into separate helper functions for better maintainability:
- Module configuration setup (lines 74-86)
- Router service setup (lines 91-101)
- Context setup with comet info (lines 122-135)
Example refactor:
func createModuleConfigs() []configurator.ModuleOption { return []configurator.ModuleOption{ configurator.AccountsModule(), // ... other modules } } func setupRouterServices(f *fixture) (runtime.RouterServiceFactory, router.Service) { msgRouterService := integration.NewRouterService() f.registerMsgRouterService(msgRouterService) // ... rest of router setup return routerFactory, queryRouterService } func createCometContext(ctx context.Context, addr sdk.AccAddress, valAddr sdk.ValAddress, valConsAddr sdk.ConsAddress) context.Context { return context.WithValue(ctx, corecontext.CometInfoKey, comet.Info{ // ... comet info setup }) }tests/integration/v2/app.go (1)
338-338: Fix typo in commentChange "heigh" to "height" in the comment.
- // update block heigh if integeration context is present + // update block height if integration context is presenttestutil/configurator/configurator.go (1)
181-191: LGTM! Consider adding documentationThe implementation of
AuthModuleWithMaccPermsfollows good practices and maintains consistency with the existing codebase. It provides a flexible way to configure module account permissions for testing scenarios.Consider adding a doc comment to explain the purpose and usage of this function:
+// AuthModuleWithMaccPerms returns a ModuleOption that configures the auth module +// with custom module account permissions. This is particularly useful for testing +// different permission configurations. func AuthModuleWithMaccPerms(maccPerms []*authmodulev1.ModuleAccountPermission) ModuleOption {
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (10)
tests/integration/v2/app.go(6 hunks)tests/integration/v2/auth/app_test.go(2 hunks)tests/integration/v2/distribution/common_test.go(1 hunks)tests/integration/v2/distribution/fixture_test.go(1 hunks)tests/integration/v2/distribution/grpc_query_test.go(1 hunks)tests/integration/v2/distribution/module_test.go(1 hunks)tests/integration/v2/distribution/msg_server_test.go(1 hunks)tests/integration/v2/services.go(3 hunks)testutil/configurator/configurator.go(2 hunks)x/staking/testutil/helpers.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
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/auth/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/distribution/common_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/distribution/fixture_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/distribution/grpc_query_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/distribution/module_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/distribution/msg_server_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/services.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"
testutil/configurator/configurator.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/staking/testutil/helpers.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (22)
tests/integration/v2/distribution/module_test.go (2)
1-9: LGTM! Package and imports are well-organized.
The package name and import organization follow Go best practices and the Uber style guide.
1-15: Enhance test coverage for distribution module integration
The current test file only covers the basic module account creation. Consider adding tests for:
- Core distribution functionality (rewards distribution, parameters, etc.)
- Error cases and edge conditions
- Integration with other modules
- State persistence across blocks
Let's check for existing distribution tests that might need porting:
Consider organizing the tests into separate files based on functionality:
module_test.go- Basic module setup and initializationdistribution_test.go- Core distribution functionalityparams_test.go- Parameter handlingintegration_test.go- Cross-module interactions
tests/integration/v2/distribution/common_test.go (2)
1-14: LGTM: Package and imports are well-organized
The package name and import organization follow the Uber Go Style Guide with proper grouping and spacing.
18-29: Improve code clarity and maintainability
Consider the following improvements:
- Define constants for magic numbers (1000)
- Ensure
valConsPk0is properly defined or imported
Example:
+const (
+ // DefaultTestPower represents the default consensus power used in tests
+ DefaultTestPower = 1000
+)
func setupValidatorWithCommission(t *testing.T, f *fixture, valAddr sdk.ValAddress, initialStake int64) {
t.Helper()
- initTokens := f.stakingKeeper.TokensFromConsensusPower(f.ctx, int64(1000))
+ initTokens := f.stakingKeeper.TokensFromConsensusPower(f.ctx, int64(DefaultTestPower))Let's verify the valConsPk0 definition:
tests/integration/v2/auth/app_test.go (2)
14-15: LGTM: Import statements are properly organized
The new imports for services and stf packages are correctly placed and necessary for the HeaderService integration.
84-84: Verify HeaderService integration with server v2
The HeaderService configuration looks correct for server v2 integration. Let's verify that this is consistent with other test files.
✅ Verification successful
HeaderService configuration is correctly implemented for server v2
The verification shows that:
- The HeaderService is consistently configured across v2 tests using
services.NewGenesisHeaderService - The implementation in
tests/integration/v2/services.goproperly implements theheader.Serviceinterface - The configuration aligns with the production code in
runtime/v2/module.go - There are no conflicting v1 implementations in the v2 test files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if HeaderService is consistently configured across v2 test files
# and verify no v1 implementations remain
# Check for HeaderService usage in v2 tests
rg -g "tests/integration/v2/**/*.go" "HeaderService.*=.*services\.NewGenesisHeaderService" -A 2
# Verify no v1 header service implementations remain
rg -g "tests/integration/v2/**/*.go" "HeaderService.*=.*(?<!services\.NewGenesis)HeaderService"
Length of output: 707
Script:
#!/bin/bash
# Let's try a different approach to verify HeaderService usage
# Check all HeaderService configurations in v2 tests
rg -g "tests/integration/v2/**/*.go" "HeaderService.*=" -A 2
# Check for any direct HeaderService struct usage
rg -g "tests/integration/v2/**/*.go" "type.*HeaderService"
# Look for HeaderService interface or implementation imports
rg -g "tests/integration/v2/**/*.go" "import.*HeaderService"
Length of output: 727
Script:
#!/bin/bash
# Let's check the implementation details and usage patterns
# Check HeaderService implementation in v2/services.go
rg -g "tests/integration/v2/services.go" "HeaderService" -B 2 -A 5
# Look for NewGenesisHeaderService usage and implementation
rg "NewGenesisHeaderService" -A 3
# Check for any methods defined on HeaderService
ast-grep --pattern 'func (*HeaderService) $_ ($_) $_'
Length of output: 2212
tests/integration/v2/services.go (3)
14-14: LGTM: Import addition follows style guidelines
The new header package import is properly grouped with other core imports.
73-73: LGTM: Context field addition is well-structured
The header field addition to integrationContext follows the existing pattern and maintains proper alignment.
201-212: LGTM: HeaderService implementation is robust and consistent
The implementation:
- Properly checks interface compliance
- Follows established patterns for context handling
- Has appropriate error handling
Let's verify the consistency of context handling across services:
✅ Verification successful
HeaderService implementation follows consistent context handling patterns
The implementation is verified to follow the same context handling pattern used across other services in the codebase:
- All services consistently check for integration context using
ctx.Value(contextKey).(*integrationContext) - Error handling follows the established pattern of returning zero/empty values when context is invalid
- The context key type and integration context are properly shared across services
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent context handling patterns across services
# Check for context handling patterns in other services
ast-grep --pattern 'func ($x *$service) $method(ctx context.Context) $ret {
$iCtx, ok := ctx.Value(contextKey).(*integrationContext)
if !ok {
$$$
}
$$$
}'
Length of output: 168
Script:
#!/bin/bash
# Let's try a different approach to verify context handling patterns
# First, let's find all service files
fd -e go . tests/integration/v2/ --exec rg -l "context.Context"
# Then, let's look for context handling patterns specifically
rg -U "ctx.Value\(contextKey\).*integrationContext.*if !ok" tests/integration/v2/
Length of output: 506
Script:
#!/bin/bash
# Let's examine the services.go file more closely to understand the context handling pattern
rg -A 5 "type \w+Service struct" tests/integration/v2/services.go
# And look for all methods that use context
rg -B 2 -A 5 "func.*\(ctx context\.Context\)" tests/integration/v2/services.go
Length of output: 2254
tests/integration/v2/distribution/fixture_test.go (5)
3-39: LGTM! Well-organized imports.
The imports are properly structured with clear separation between standard library, core SDK, and external packages. Blank imports are correctly used for module registration.
52-68: LGTM! Well-structured fixture type.
The fixture struct is well-designed with all necessary components for integration testing, including app instance, context, keepers, and test addresses.
157-160: Verify if QueryRouterService implementation is needed.
The registerQueryRouterService function is empty. If query routing is required for the tests, consider implementing it. If not needed, consider adding a comment explaining why it's empty.
✅ Verification successful
Based on the search results, I can generate the final response as we have enough context. The search shows that in tests/integration/v2/auth/app_test.go, there's an implementation of registerQueryRouterService that handles account number queries, while in the distribution test file it's empty.
Empty QueryRouterService implementation is acceptable if no query routing is needed
The empty implementation in the distribution test fixture appears intentional as query routing is only implemented where specific query handling is required (as seen in the auth tests where it's used for account number queries). If the distribution tests don't need to handle any specific queries, it's fine to leave it empty.
- Consider adding a comment like
// No query routing needed for distribution teststo make the intention explicit
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if query router registration is implemented in similar test files
rg "registerQueryRouterService" --type go -A 10
Length of output: 3129
41-44: Initialize empty address variables.
The emptyDelAddr and emptyValAddr variables are declared but not initialized. Consider initializing them with zero values or removing if unused.
1-160: Verify test coverage using this fixture.
Since this is a test fixture file for distribution integration tests, ensure that:
- All the initialized components (keepers, routers, etc.) are utilized in the actual test cases
- The fixture provides sufficient setup for testing all distribution module functionalities
✅ Verification successful
Test coverage is comprehensive and properly utilizes the fixture
The test fixture is well utilized across multiple test files with thorough coverage:
-
msg_server_test.go: Tests all message handlers
- WithdrawDelegatorReward
- SetWithdrawAddress
- WithdrawValidatorCommission
- FundCommunityPool
- UpdateParams
- CommunityPoolSpend
- DepositValidatorRewardsPool
-
grpc_query_test.go: Tests all query endpoints
- Params
- ValidatorOutstandingRewards
- ValidatorCommission
- ValidatorSlashes
- DelegatorWithdrawAddress
- CommunityPool
- DelegationRewards
-
module_test.go: Tests module initialization
The fixture provides all necessary components (keepers, routers, codecs) and proper setup for comprehensive distribution module testing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find test files that use this fixture
rg "createTestFixture" --type go
# Look for distribution module test coverage
fd -e go -x grep -l "TestDistribution" {} \;
Length of output: 1374
Script:
#!/bin/bash
# Let's check the actual test files content to verify coverage
echo "=== msg_server_test.go ==="
cat tests/integration/v2/distribution/msg_server_test.go
echo -e "\n=== module_test.go ==="
cat tests/integration/v2/distribution/module_test.go
echo -e "\n=== grpc_query_test.go ==="
cat tests/integration/v2/distribution/grpc_query_test.go
Length of output: 44702
x/staking/testutil/helpers.go (2)
33-33: LGTM: Constructor properly updated
The constructor signature change aligns with the struct field update and maintains proper test helper functionality.
26-26: Verify the impact of context type change
The change from sdk.Context to context.Context is a breaking change that might affect test implementations using this Helper struct.
✅ Verification successful
Context type change is safe to proceed
The change from sdk.Context to context.Context in the Helper struct is safe because:
- The Helper struct is only used in test files
- The context is properly handled in the NewHelper constructor
- All usages found in integration tests create the Helper instance through NewHelper function, which already accepts context.Context
- No direct access to Helper.Ctx field was found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any direct usage of the Helper.Ctx field that might be affected
rg -A 2 "Helper.*\.Ctx\." --type go
Length of output: 37
Script:
#!/bin/bash
# Let's try a broader search for Helper struct usage
rg -l "Helper\s*{" --type go
# Also search for any test files that might use this helper
rg -l "testutil.*Helper" --type go
Length of output: 505
Script:
#!/bin/bash
# Fix the regex and search for Helper struct usage
rg -l "Helper {" --type go
# Let's examine the content of these test files to see how Helper is used
rg -A 5 "testutil.*Helper" tests/integration/v2/distribution/common_test.go tests/integration/v2/distribution/msg_server_test.go tests/integration/slashing/abci_test.go
Length of output: 1929
Script:
#!/bin/bash
# Let's check the actual Helper struct definition and its usage
rg -A 10 "type Helper struct" --type go
# And check how the NewHelper function is implemented
rg -A 10 "func NewHelper" --type go
Length of output: 1425
tests/integration/v2/app.go (5)
100-101: LGTM: Well-structured HeaderService addition
The HeaderService field is properly added to StartupConfig with clear documentation and correct type usage.
131-131: LGTM: Consistent HeaderService initialization
The HeaderService is properly initialized using the standard pattern, consistent with other service initializations in the default configuration.
195-195: LGTM: Proper HeaderService dependency injection
The HeaderService is correctly injected into the dependency supply chain, following the established pattern for service injection.
320-322: LGTM: Clean LastBlockHeight implementation
The LastBlockHeight method is properly implemented as a getter method, following Go conventions and using appropriate types.
338-342: Consider potential block height overflow
The conversion from uint64 (lastHeight) to int64 (header.Height) could potentially cause overflow for very large block heights. Consider adding a check to prevent this issue.
iCtx, ok := ctx.Value(contextKey).(*integrationContext)
if ok {
+ if a.lastHeight > uint64(math.MaxInt64) {
+ panic("block height overflow")
+ }
iCtx.header.Height = int64(a.lastHeight)
}testutil/configurator/configurator.go (1)
166-166: Verify security implications of granting minter permission
The addition of "minter" permission to the distribution module is a significant change that could have security implications. Please ensure this aligns with the module's intended functionality in server v2.
|
|
||
| func TestGRPCCommunityPool(t *testing.T) { | ||
| t.Parallel() | ||
| f := createTestFixture(t) | ||
|
|
||
| var ( | ||
| req *types.QueryCommunityPoolRequest //nolint:staticcheck // we're using a deprecated call | ||
| expPool *types.QueryCommunityPoolResponse //nolint:staticcheck // we're using a deprecated call | ||
| ) | ||
|
|
||
| testCases := []struct { | ||
| name string | ||
| malleate func() | ||
| }{ | ||
| { | ||
| name: "valid request empty community pool", | ||
| malleate: func() { | ||
| req = &types.QueryCommunityPoolRequest{} //nolint:staticcheck // we're using a deprecated call | ||
| expPool = &types.QueryCommunityPoolResponse{} //nolint:staticcheck // we're using a deprecated call | ||
| }, | ||
| }, | ||
| { | ||
| name: "valid request", | ||
| malleate: func() { | ||
| amount := sdk.NewCoins(sdk.NewInt64Coin(sdk.DefaultBondDenom, 100)) | ||
| assert.NilError(t, f.bankKeeper.MintCoins(f.ctx, types.ModuleName, amount)) | ||
| assert.NilError(t, f.bankKeeper.SendCoinsFromModuleToAccount(f.ctx, types.ModuleName, f.addr, amount)) | ||
|
|
||
| err := f.poolKeeper.FundCommunityPool(f.ctx, amount, f.addr) | ||
| assert.Assert(t, err == nil) | ||
| req = &types.QueryCommunityPoolRequest{} //nolint:staticcheck // we're using a deprecated call | ||
|
|
||
| expPool = &types.QueryCommunityPoolResponse{Pool: sdk.NewDecCoinsFromCoins(amount...)} //nolint:staticcheck // we're using a deprecated call | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| for _, testCase := range testCases { | ||
| tc := testCase | ||
| t.Run(fmt.Sprintf("Case %s", tc.name), func(t *testing.T) { | ||
| testCase.malleate() | ||
|
|
||
| pool, err := f.queryClient.CommunityPool(f.ctx, req) //nolint:staticcheck // we're using a deprecated call | ||
|
|
||
| assert.NilError(t, err) | ||
| assert.DeepEqual(t, expPool, pool) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid overusing //nolint directives
The use of multiple //nolint:staticcheck directives suppresses important linter warnings. It's better to address the underlying issues rather than suppressing them. Update the code to remove deprecated calls and comply with current standards.
Refer to the previous refactor suggestion to update deprecated API usage in TestGRPCCommunityPool.
| var ( | ||
| req *types.QueryCommunityPoolRequest //nolint:staticcheck // we're using a deprecated call | ||
| expPool *types.QueryCommunityPoolResponse //nolint:staticcheck // we're using a deprecated call | ||
| ) | ||
|
|
||
| testCases := []struct { | ||
| name string | ||
| malleate func() | ||
| }{ | ||
| { | ||
| name: "valid request empty community pool", | ||
| malleate: func() { | ||
| req = &types.QueryCommunityPoolRequest{} //nolint:staticcheck // we're using a deprecated call | ||
| expPool = &types.QueryCommunityPoolResponse{} //nolint:staticcheck // we're using a deprecated call | ||
| }, | ||
| }, | ||
| { | ||
| name: "valid request", | ||
| malleate: func() { | ||
| amount := sdk.NewCoins(sdk.NewInt64Coin(sdk.DefaultBondDenom, 100)) | ||
| assert.NilError(t, f.bankKeeper.MintCoins(f.ctx, types.ModuleName, amount)) | ||
| assert.NilError(t, f.bankKeeper.SendCoinsFromModuleToAccount(f.ctx, types.ModuleName, f.addr, amount)) | ||
|
|
||
| err := f.poolKeeper.FundCommunityPool(f.ctx, amount, f.addr) | ||
| assert.Assert(t, err == nil) | ||
| req = &types.QueryCommunityPoolRequest{} //nolint:staticcheck // we're using a deprecated call | ||
|
|
||
| expPool = &types.QueryCommunityPoolResponse{Pool: sdk.NewDecCoinsFromCoins(amount...)} //nolint:staticcheck // we're using a deprecated call | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| for _, testCase := range testCases { | ||
| tc := testCase | ||
| t.Run(fmt.Sprintf("Case %s", tc.name), func(t *testing.T) { | ||
| testCase.malleate() | ||
|
|
||
| pool, err := f.queryClient.CommunityPool(f.ctx, req) //nolint:staticcheck // we're using a deprecated call | ||
|
|
||
| assert.NilError(t, err) | ||
| assert.DeepEqual(t, expPool, pool) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update tests to use non-deprecated API methods
The TestGRPCCommunityPool function uses deprecated API calls (QueryCommunityPoolRequest and QueryCommunityPoolResponse) and suppresses linter warnings with //nolint:staticcheck. It's important to update the test to use the current APIs to maintain code quality and future compatibility.
Apply this refactor to use the updated Query/QueryClient methods:
func TestGRPCCommunityPool(t *testing.T) {
t.Parallel()
f := createTestFixture(t)
- var (
- req *types.QueryCommunityPoolRequest //nolint:staticcheck // we're using a deprecated call
- expPool *types.QueryCommunityPoolResponse //nolint:staticcheck // we're using a deprecated call
- )
+ req := &types.QueryCommunityPoolRequest{}
+ var expPool *types.QueryCommunityPoolResponse
testCases := []struct {
name string
malleate func()
}{
{
name: "valid request empty community pool",
malleate: func() {
- req = &types.QueryCommunityPoolRequest{} //nolint:staticcheck // we're using a deprecated call
- expPool = &types.QueryCommunityPoolResponse{} //nolint:staticcheck // we're using a deprecated call
+ expPool = &types.QueryCommunityPoolResponse{}
},
},
{
name: "valid request",
malleate: func() {
amount := sdk.NewCoins(sdk.NewInt64Coin(sdk.DefaultBondDenom, 100))
assert.NilError(t, f.bankKeeper.MintCoins(f.ctx, types.ModuleName, amount))
assert.NilError(t, f.bankKeeper.SendCoinsFromModuleToAccount(f.ctx, types.ModuleName, f.addr, amount))
err := f.poolKeeper.FundCommunityPool(f.ctx, amount, f.addr)
- assert.Assert(t, err == nil)
- req = &types.QueryCommunityPoolRequest{} //nolint:staticcheck // we're using a deprecated call
+ assert.NilError(t, err)
expPool = &types.QueryCommunityPoolResponse{Pool: sdk.NewDecCoinsFromCoins(amount...)}
},
},
}
for _, testCase := range testCases {
tc := testCase
t.Run(fmt.Sprintf("Case %s", tc.name), func(t *testing.T) {
testCase.malleate()
- pool, err := f.queryClient.CommunityPool(f.ctx, req) //nolint:staticcheck // we're using a deprecated call
+ pool, err := f.queryClient.CommunityPool(f.ctx, req)
assert.NilError(t, err)
assert.DeepEqual(t, expPool, pool)
})
}
}Ensure that you're importing any necessary packages for the updated APIs.
Committable suggestion skipped: line range outside the PR's diff.
| msg *distrtypes.MsgFundCommunityPool //nolint:staticcheck // we're using a deprecated call | ||
| expErr bool | ||
| expErrMsg string | ||
| }{ | ||
| { | ||
| name: "no depositor address", | ||
| msg: &distrtypes.MsgFundCommunityPool{ //nolint:staticcheck // we're using a deprecated call | ||
| Amount: sdk.NewCoins(sdk.NewCoin("stake", math.NewInt(100))), | ||
| Depositor: emptyDelAddr.String(), | ||
| }, | ||
| expErr: true, | ||
| expErrMsg: "invalid depositor address", | ||
| }, | ||
| { | ||
| name: "invalid coin", | ||
| msg: &distrtypes.MsgFundCommunityPool{ //nolint:staticcheck // we're using a deprecated call | ||
| Amount: sdk.Coins{sdk.NewInt64Coin("stake", 10), sdk.NewInt64Coin("stake", 10)}, | ||
| Depositor: addr.String(), | ||
| }, | ||
| expErr: true, | ||
| expErrMsg: "10stake,10stake: invalid coins", | ||
| }, | ||
| { | ||
| name: "depositor address with no funds", | ||
| msg: &distrtypes.MsgFundCommunityPool{ //nolint:staticcheck // we're using a deprecated call | ||
| Amount: sdk.NewCoins(sdk.NewCoin("stake", math.NewInt(100))), | ||
| Depositor: addr2.String(), | ||
| }, | ||
| expErr: true, | ||
| expErrMsg: "insufficient funds", | ||
| }, | ||
| { | ||
| name: "valid message", | ||
| msg: &distrtypes.MsgFundCommunityPool{ //nolint:staticcheck // we're using a deprecated call | ||
| Amount: sdk.NewCoins(sdk.NewCoin("stake", math.NewInt(100))), | ||
| Depositor: addr.String(), | ||
| }, | ||
| expErr: false, | ||
| }, | ||
| } | ||
|
|
||
| msgServer := distrkeeper.NewMsgServerImpl(f.distrKeeper) | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| res, err := f.app.RunMsg( | ||
| t, | ||
| f.ctx, | ||
| func(ctx context.Context) (transaction.Msg, error) { | ||
| res, err := msgServer.FundCommunityPool(ctx, tc.msg) //nolint:staticcheck // we're using a deprecated call | ||
| return res, err | ||
| }, | ||
| integration.WithAutomaticCommit(), | ||
| ) | ||
| if tc.expErr { | ||
| assert.ErrorContains(t, err, tc.expErrMsg) | ||
| } else { | ||
| assert.NilError(t, err) | ||
| assert.Assert(t, res != nil) | ||
|
|
||
| // check the result | ||
| _, ok := res.(*distrtypes.MsgFundCommunityPoolResponse) //nolint:staticcheck // we're using a deprecated call | ||
| assert.Assert(t, ok, true) | ||
|
|
||
| // query the community pool funds | ||
| poolBal := f.bankKeeper.GetAllBalances(f.ctx, poolAcc.GetAddress()) | ||
| assert.Assert(t, poolBal.Equal(amount)) | ||
|
|
||
| assert.Assert(t, f.bankKeeper.GetAllBalances(f.ctx, addr).Empty()) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update deprecated methods to use the latest API
The use of deprecated methods like MsgFundCommunityPool may lead to maintenance issues in the future. Consider updating the code to use the latest methods provided by the SDK to ensure compatibility and benefit from improvements.
| msg *distrtypes.MsgCommunityPoolSpend //nolint:staticcheck // we're using a deprecated call | ||
| expErr bool | ||
| expErrMsg string | ||
| }{ | ||
| { | ||
| name: "invalid authority", | ||
| msg: &distrtypes.MsgCommunityPoolSpend{ //nolint:staticcheck // we're using a deprecated call | ||
| Authority: "invalid", | ||
| Recipient: recipient.String(), | ||
| Amount: sdk.NewCoins(sdk.NewCoin("stake", math.NewInt(100))), | ||
| }, | ||
| expErr: true, | ||
| expErrMsg: "invalid authority", | ||
| }, | ||
| { | ||
| name: "invalid recipient", | ||
| msg: &distrtypes.MsgCommunityPoolSpend{ //nolint:staticcheck // we're using a deprecated call | ||
| Authority: f.distrKeeper.GetAuthority(), | ||
| Recipient: "invalid", | ||
| Amount: sdk.NewCoins(sdk.NewCoin("stake", math.NewInt(100))), | ||
| }, | ||
| expErr: true, | ||
| expErrMsg: "decoding bech32 failed", | ||
| }, | ||
| { | ||
| name: "valid message", | ||
| msg: &distrtypes.MsgCommunityPoolSpend{ //nolint:staticcheck // we're using a deprecated call | ||
| Authority: f.distrKeeper.GetAuthority(), | ||
| Recipient: recipient.String(), | ||
| Amount: sdk.NewCoins(sdk.NewCoin("stake", math.NewInt(100))), | ||
| }, | ||
| expErr: false, | ||
| }, | ||
| } | ||
|
|
||
| msgServer := distrkeeper.NewMsgServerImpl(f.distrKeeper) | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| res, err := f.app.RunMsg( | ||
| t, | ||
| f.ctx, | ||
| func(ctx context.Context) (transaction.Msg, error) { | ||
| res, e := msgServer.CommunityPoolSpend(ctx, tc.msg) //nolint:staticcheck // we're using a deprecated call | ||
| return res, e | ||
| }, | ||
| integration.WithAutomaticCommit(), | ||
| ) | ||
| if tc.expErr { | ||
| assert.ErrorContains(t, err, tc.expErrMsg) | ||
| } else { | ||
| assert.NilError(t, err) | ||
| assert.Assert(t, res != nil) | ||
|
|
||
| // check the result | ||
| _, ok := res.(*distrtypes.MsgCommunityPoolSpendResponse) //nolint:staticcheck // we're using a deprecated call | ||
| assert.Assert(t, ok, true) | ||
|
|
||
| // query the community pool to verify it has been updated | ||
| poolBal := f.bankKeeper.GetAllBalances(f.ctx, poolAcc.GetAddress()) | ||
| assert.Assert(t, poolBal.Empty()) | ||
|
|
||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update deprecated methods to use the latest API
The use of deprecated methods like MsgCommunityPoolSpend may lead to maintenance issues in the future. It's recommended to update the code to use the latest available methods provided by the SDK.
| assert.NilError(t, f.stakingKeeper.SetValidator(f.ctx, validator)) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use require.NoError instead of assert.NilError for critical errors
When setting up the test environment, it's important to fail the test immediately if a critical error occurs. Replace assert.NilError with require.NoError to ensure the test stops upon failure.
Apply this diff to replace assert.NilError with require.NoError:
-assert.NilError(t, f.stakingKeeper.SetValidator(f.ctx, validator))
+require.NoError(t, f.stakingKeeper.SetValidator(f.ctx, validator))📝 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.
| assert.NilError(t, f.stakingKeeper.SetValidator(f.ctx, validator)) | |
| require.NoError(t, f.stakingKeeper.SetValidator(f.ctx, validator)) |
| if f.app.LastBlockHeight() != height { | ||
| panic(fmt.Errorf("expected block height to be %d, got %d", height, f.app.LastBlockHeight())) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace panic with assertion in test code
Using panic in tests can cause the entire test suite to halt unexpectedly. It's better to use assertions to handle test failures gracefully.
Apply this diff to replace the panic with an assertion:
-if f.app.LastBlockHeight() != height {
- panic(fmt.Errorf("expected block height to be %d, got %d", height, f.app.LastBlockHeight()))
-}
+require.Equal(t, height, f.app.LastBlockHeight(), "unexpected block height")Committable suggestion skipped: line range outside the PR's diff.
| sdkCtx := sdk.UnwrapSDKContext(sh.Ctx) | ||
| sh.Ctx = sdkCtx.WithHeaderInfo(header.Info{Time: sdkCtx.HeaderInfo().Time.Add(diff)}) | ||
| _, err := sh.k.EndBlocker(sh.Ctx) | ||
| require.NoError(sh.t, err) | ||
| return sh.Ctx | ||
| return sdkCtx |
There was a problem hiding this comment.
Fix context inconsistency in TurnBlockTimeDiff
Similar to TurnBlock, this method has a context inconsistency where it updates sh.Ctx but returns the old unwrapped context.
Consider this fix:
func (sh *Helper) TurnBlockTimeDiff(diff time.Duration) sdk.Context {
sdkCtx := sdk.UnwrapSDKContext(sh.Ctx)
- sh.Ctx = sdkCtx.WithHeaderInfo(header.Info{Time: sdkCtx.HeaderInfo().Time.Add(diff)})
+ newSDKCtx := sdkCtx.WithHeaderInfo(header.Info{Time: sdkCtx.HeaderInfo().Time.Add(diff)})
+ sh.Ctx = newSDKCtx
_, err := sh.k.EndBlocker(sh.Ctx)
require.NoError(sh.t, err)
- return sdkCtx
+ return newSDKCtx
}📝 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.
| sdkCtx := sdk.UnwrapSDKContext(sh.Ctx) | |
| sh.Ctx = sdkCtx.WithHeaderInfo(header.Info{Time: sdkCtx.HeaderInfo().Time.Add(diff)}) | |
| _, err := sh.k.EndBlocker(sh.Ctx) | |
| require.NoError(sh.t, err) | |
| return sh.Ctx | |
| return sdkCtx | |
| sdkCtx := sdk.UnwrapSDKContext(sh.Ctx) | |
| newSDKCtx := sdkCtx.WithHeaderInfo(header.Info{Time: sdkCtx.HeaderInfo().Time.Add(diff)}) | |
| sh.Ctx = newSDKCtx | |
| _, err := sh.k.EndBlocker(sh.Ctx) | |
| require.NoError(sh.t, err) | |
| return newSDKCtx |
| sdkCtx := sdk.UnwrapSDKContext(sh.Ctx) | ||
| sh.Ctx = sdkCtx.WithHeaderInfo(header.Info{Time: newTime}) | ||
| _, err := sh.k.EndBlocker(sh.Ctx) | ||
| require.NoError(sh.t, err) | ||
| return sh.Ctx | ||
| return sdkCtx |
There was a problem hiding this comment.
Fix context inconsistency in TurnBlock
The method updates sh.Ctx with a new context but returns the old unwrapped context. This could lead to inconsistencies if the returned context is used elsewhere.
Consider this fix:
func (sh *Helper) TurnBlock(newTime time.Time) sdk.Context {
sdkCtx := sdk.UnwrapSDKContext(sh.Ctx)
- sh.Ctx = sdkCtx.WithHeaderInfo(header.Info{Time: newTime})
+ newSDKCtx := sdkCtx.WithHeaderInfo(header.Info{Time: newTime})
+ sh.Ctx = newSDKCtx
_, err := sh.k.EndBlocker(sh.Ctx)
require.NoError(sh.t, err)
- return sdkCtx
+ return newSDKCtx
}📝 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.
| sdkCtx := sdk.UnwrapSDKContext(sh.Ctx) | |
| sh.Ctx = sdkCtx.WithHeaderInfo(header.Info{Time: newTime}) | |
| _, err := sh.k.EndBlocker(sh.Ctx) | |
| require.NoError(sh.t, err) | |
| return sh.Ctx | |
| return sdkCtx | |
| sdkCtx := sdk.UnwrapSDKContext(sh.Ctx) | |
| newSDKCtx := sdkCtx.WithHeaderInfo(header.Info{Time: newTime}) | |
| sh.Ctx = newSDKCtx | |
| _, err := sh.k.EndBlocker(sh.Ctx) | |
| require.NoError(sh.t, err) | |
| return newSDKCtx |
julienrbrt
changed the title
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.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
HeaderServiceto enhance application configuration.Bug Fixes
Tests
Documentation
AuthModuleto include custom module account permissions.