fix(testutil/integration): use only one context in integration test framework #22616
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request primarily focus on enhancing context management across various integration tests within the Cosmos SDK. The modifications involve replacing instances of Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| for key := range keys { | ||
| cms.MountStoreWithDB(keys[key], storetypes.StoreTypeIAVL, db) | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning test
|
Opening but need to add a changelog |
julienrbrt
added
the
backport/v0.52.x
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (18)
testutil/integration/helpers.go (1)
11-12: Enhance function documentationWhile the current documentation is concise, it would be helpful to add more details about:
- The purpose of creating multiple stores
- The expected format and requirements for the keys map
- The relationship with the integration test framework
Consider expanding the documentation like this:
-// CreateMultiStore is a helper for setting up multiple stores for provided modules. +// CreateMultiStore creates and initializes a CommitMultiStore with IAVL stores for the provided module store keys. +// It sets up an in-memory database and mounts each store key with a new IAVL store. +// This helper is primarily used in integration tests to set up the necessary store infrastructure. +// +// Parameters: +// - keys: a map of store names to their corresponding KVStoreKey instances +// - logger: a logger instance for store operations +// +// Returns a configured and loaded CommitMultiStore ready for use in tests.tests/integration/auth/keeper/migrate_x_accounts_test.go (3)
Line range hint
45-50: Consider adding more error scenariosWhile the current error test is well-implemented, consider adding tests for other potential error cases such as:
- Account with invalid permissions
- Account with existing migrations
Line range hint
55-59: Consider using exact error matchingWhile the error testing is correct, consider using
require.ErrorIsinstead ofrequire.ErrorContainsfor more precise error checking. This would ensure the exact error type is being returned, not just the message content.
Line range hint
76-98: LGTM: Comprehensive migration verificationThe success case thoroughly verifies the migration process and account state. Consider adding these additional verifications:
- Account balance preservation
- Account permissions preservation
- Event emission verification
tests/integration/auth/keeper/fixture_test.go (1)
Line range hint
112-127: Consider adding error handling for module initialization.While the setup is comprehensive, consider adding error handling for the module initialization and server registration steps to ensure test stability.
Example improvement:
integrationApp := integration.NewIntegrationApp(logger, keys, cdc, encodingCfg.InterfaceRegistry.SigningContext().AddressCodec(), encodingCfg.InterfaceRegistry.SigningContext().ValidatorAddressCodec(), map[string]appmodule.AppModule{ accounts.ModuleName: accountsModule, authtypes.ModuleName: authModule, banktypes.ModuleName: bankModule, }, router, queryRouter) + +// Add error checks for critical initialization steps +if err := integrationApp.Setup(); err != nil { + panic(fmt.Sprintf("failed to setup integration app: %v", err)) +}tests/integration/auth/keeper/accounts_retro_compatibility_test.go (1)
Line range hint
135-156: Consider adding sequence number verificationWhile the test covers account initialization and info retrieval well, consider adding test cases for sequence number changes to ensure complete compatibility testing.
Add a test case like this:
t.Run("verify sequence number updates", func(t *testing.T) { // Query initial sequence info, err := qs.AccountInfo(f.app.Context(), &authtypes.QueryAccountInfoRequest{ Address: f.mustAddr(addr), }) require.NoError(t, err) initialSeq := info.Info.Sequence // Simulate sequence increment (implementation depends on your setup) // ... // Verify sequence was updated info, err = qs.AccountInfo(f.app.Context(), &authtypes.QueryAccountInfoRequest{ Address: f.mustAddr(addr), }) require.NoError(t, err) require.Greater(t, info.Info.Sequence, initialSeq) })tests/integration/gov/keeper/keeper_test.go (1)
Context management changes need to be propagated across integration tests
Based on the search results, there are still several integration test files using direct context creation via
NewContextandsdk.Context. The changes should be consistent with the PR's objective of simplifying context management.Key locations that need attention:
tests/integration/runtime/query_test.go: Usesapp.BaseApp.NewContexttests/integration/accounts/base_account_test.go: Usessdk.NewContexttests/integration/accounts/multisig/account_test.go: Usessdk.NewContexttests/integration/gov/abci_test.go: Multiple instances ofapp.BaseApp.NewContexttests/integration/bank/app_test.go: Multiple instances of direct context creation🔗 Analysis chain
Line range hint
142-156: LGTM! The context management simplification looks good.The changes align well with the PR objective of simplifying context management. The integration app initialization is clean and follows best practices.
Let's verify that we're consistently using the app's context across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining direct context creation or potential inconsistencies # in context usage across integration tests # Look for direct context creation patterns that should be avoided rg -g '**/*_test.go' 'sdk\.Context\(' tests/integration/ rg -g '**/*_test.go' 'NewContext\(' tests/integration/ # Look for remaining f.ctx usage that might need updating rg -g '**/*_test.go' 'f\.ctx' tests/integration/Length of output: 41597
testutil/integration/router.go (3)
38-40: Consider context management implicationsStoring the context directly in the App struct could lead to stale context issues if the context needs to be updated during the test lifecycle (e.g., after block changes). Consider using a context getter method that ensures the context is always up-to-date with the latest block state.
94-101: Improve error handling and documentationThe consensus params initialization could benefit from:
- More explicit documentation about the context state and its implications
- Simplified error handling to avoid nested conditions
Consider restructuring like this:
if consensusKey := keys[consensus]; consensusKey != nil { - _ = bApp.CommitMultiStore().LoadLatestVersion() - cps := newParamStore(runtime.NewKVStoreService(consensusKey), appCodec) - params := cmttypes.ConsensusParamsFromProto(*simtestutil.DefaultConsensusParams) // This fills up missing param sections - if err := cps.Set(sdk.NewContext(bApp.CommitMultiStore(), true, logger), params.ToProto()); err != nil { // at this point, because we haven't written state we don't have a real context - panic(fmt.Errorf("failed to set consensus params: %w", err)) - } + // Load latest version before initializing params + if err := bApp.CommitMultiStore().LoadLatestVersion(); err != nil { + panic(fmt.Errorf("failed to load store version: %w", err)) + } + + // Initialize param store with default consensus params + cps := newParamStore(runtime.NewKVStoreService(consensusKey), appCodec) + params := cmttypes.ConsensusParamsFromProto(*simtestutil.DefaultConsensusParams) + + // Note: Using temporary context since state hasn't been initialized yet + tmpCtx := sdk.NewContext(bApp.CommitMultiStore(), true, logger) + if err := cps.Set(tmpCtx, params.ToProto()); err != nil { + panic(fmt.Errorf("failed to set consensus params: %w", err)) + }
120-131: Improve error handling in initialization completionThe initialization completion sequence could benefit from more robust error handling and clearer state management.
Consider restructuring like this:
- bApp.SimWriteState() // forcing state write from init genesis like in sims - _, err := bApp.Commit() - if err != nil { - panic(err) - } + // Force state write from init genesis (required for simulations) + bApp.SimWriteState() + + // Commit changes and ensure successful state persistence + res, err := bApp.Commit() + if err != nil { + panic(fmt.Errorf("failed to commit initial state: %w", err)) + } + + // Initialize application context with chain information + sdkCtx := bApp.NewContext(true).WithBlockHeader(cmtproto.Header{ + ChainID: appName, + Height: res.RetainHeight, + })tests/integration/staking/keeper/common_test.go (1)
Inconsistent context initialization patterns found across integration tests
Several integration test files are still using direct SDK context initialization (
sdk.NewContext) instead of the simplified pattern. Key files that need updates:
tests/integration/distribution/migration_v4_test.gotests/integration/accounts/base_account_test.gotests/integration/accounts/wiring_test.gotests/integration/accounts/multisig/account_test.goThe changes in
common_test.goare correct, but similar simplification should be applied to other integration tests to maintain consistency with the PR's objective of eliminating double SDK context usage.🔗 Analysis chain
Line range hint
170-182: LGTM! Context management simplification aligns with PR objectives.The changes correctly simplify the integration test setup by removing the explicit context initialization and relying on the integration app's context management. This aligns with the PR's goal of eliminating double SDK context usage.
Let's verify that this pattern is consistently applied across other integration tests:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining direct context initialization in integration tests # Expected: No matches, as all tests should use app.Context() # Search for direct context initialization in integration tests rg -g 'tests/integration/**/*_test.go' 'sdk\.NewContext\(' # Search for proper context usage pattern rg -g 'tests/integration/**/*_test.go' '\.Context\(\)'Length of output: 5786
tests/integration/bank/keeper/deterministic_test.go (2)
Line range hint
116-125: LGTM! Context management simplification looks good.The removal of the separate context parameter and using
integrationApp.Context()improves code clarity and reduces potential context-related issues.Consider adding a comment explaining that the context is managed by the integration app to help future maintainers understand this design choice.
+// Context is managed by the integration app to ensure consistent state management integrationApp := integration.NewIntegrationApp(logger, keys, cdc,
Line range hint
147-150: Consider enhancing error handling in fundAccount helper.The helper function silently handles errors using a temporary testing.T. This could mask issues during test setup.
Consider modifying the function to return an error or use the test's *testing.T:
-func fundAccount(f *deterministicFixture, addr sdk.AccAddress, coin ...sdk.Coin) { +func fundAccount(t *testing.T, f *deterministicFixture, addr sdk.AccAddress, coin ...sdk.Coin) { err := banktestutil.FundAccount(f.ctx, f.bankKeeper, addr, sdk.NewCoins(coin...)) - assert.NilError(&testing.T{}, err) + assert.NilError(t, err) }tests/integration/evidence/keeper/infraction_test.go (2)
Line range hint
590-626: Consider adding documentation to helper functions.While the helper functions are well-structured, adding documentation would improve maintainability and help other developers understand their purpose and usage.
Add documentation for the helper functions:
+// populateValidators initializes test validators with the specified initial amounts func populateValidators(t assert.TestingT, f *fixture) { +// newPubKey creates a new ed25519 public key from a hex string func newPubKey(pk string) (res cryptotypes.PubKey) { +// testEquivocationHandler creates a test handler for equivocation evidence func testEquivocationHandler(_ interface{}) evidencetypes.Handler {
Line range hint
267-589: Consider adding more error test cases.While the existing test cases are comprehensive for the happy path and basic error scenarios, consider adding tests for:
- Evidence with invalid signatures
- Multiple concurrent double-sign infractions
- Edge cases around unbonding period boundaries
Example test case structure:
func TestHandleDoubleSign_InvalidSignature(t *testing.T) { t.Parallel() f := initFixture(t) // Test setup // Create evidence with invalid signature // Verify proper error handling }tests/integration/slashing/keeper/keeper_test.go (1)
Line range hint
249-700: Consider extracting common test setup patternsThe test implementations are thorough but contain repeated patterns for setting up validator states and handling block height updates. Consider extracting these into helper functions to improve maintainability and readability.
Example helper function:
+func (f *fixture) advanceBlocks(n int64) { + newHeight := f.ctx.BlockHeight() + n + f.ctx = f.ctx.WithBlockHeight(newHeight).WithHeaderInfo(coreheader.Info{Height: newHeight}) +}This would simplify the repeated block height update pattern seen throughout the tests.
tests/integration/staking/keeper/deterministic_test.go (1)
Line range hint
383-1162: Consider adding test documentationWhile the test coverage is comprehensive, consider adding documentation comments before each test function to explain:
- The test's purpose
- The scenarios being tested
- The expected outcomes
Example for the TestGRPCValidator function:
+// TestGRPCValidator tests the GRPC QueryValidator endpoint using both property-based +// and deterministic test cases. It verifies: +// - Random validator queries with varying parameters +// - Static validator queries with known parameters +// - Error cases and edge conditions func TestGRPCValidator(t *testing.T) {tests/integration/distribution/keeper/msg_server_test.go (1)
166-180: Consider documenting the hardcoded validator power value.The validator power is hardcoded to 100. Consider adding a comment explaining why this specific value was chosen to improve code maintainability.
sdkCtx := sdk.UnwrapSDKContext(integrationApp.Context()).WithProposer(valConsAddr).WithCometInfo(comet.Info{ LastCommit: comet.CommitInfo{ Votes: []comet.VoteInfo{ { Validator: comet.Validator{ Address: valAddr, + // Default test validator power set to 100 for consistency with other integration tests Power: 100, }, BlockIDFlag: comet.BlockIDFlagCommit, }, }, }, ProposerAddress: valConsAddr, })
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (18)
tests/integration/accounts/fixture_test.go(4 hunks)tests/integration/auth/keeper/accounts_retro_compatibility_test.go(2 hunks)tests/integration/auth/keeper/fixture_test.go(1 hunks)tests/integration/auth/keeper/migrate_x_accounts_test.go(4 hunks)tests/integration/auth/keeper/msg_server_test.go(1 hunks)tests/integration/bank/keeper/deterministic_test.go(1 hunks)tests/integration/distribution/keeper/msg_server_test.go(2 hunks)tests/integration/evidence/keeper/infraction_test.go(1 hunks)tests/integration/example/example_test.go(0 hunks)tests/integration/gov/keeper/grpc_query_test.go(0 hunks)tests/integration/gov/keeper/keeper_test.go(1 hunks)tests/integration/slashing/keeper/keeper_test.go(1 hunks)tests/integration/staking/keeper/common_test.go(1 hunks)tests/integration/staking/keeper/deterministic_test.go(1 hunks)tests/integration/type_check.go(1 hunks)testutil/integration/helpers.go(1 hunks)testutil/integration/router.go(4 hunks)x/bank/keeper/send.go(1 hunks)
💤 Files with no reviewable changes (2)
- tests/integration/example/example_test.go
- tests/integration/gov/keeper/grpc_query_test.go
✅ Files skipped from review due to trivial changes (1)
- tests/integration/type_check.go
🧰 Additional context used
📓 Path-based instructions (15)
tests/integration/accounts/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/auth/keeper/accounts_retro_compatibility_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/auth/keeper/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/auth/keeper/migrate_x_accounts_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/auth/keeper/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/bank/keeper/deterministic_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/distribution/keeper/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/evidence/keeper/infraction_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/gov/keeper/keeper_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/slashing/keeper/keeper_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/staking/keeper/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/staking/keeper/deterministic_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"
testutil/integration/helpers.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
testutil/integration/router.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/bank/keeper/send.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 GitHub Check: CodeQL
testutil/integration/helpers.go
[warning] 16-18: Iteration over map
Iteration over map may be a possible source of non-determinism
🔇 Additional comments (24)
testutil/integration/helpers.go (1)
1-9: LGTM: Package and imports are well-organized
The package declaration and imports follow Go conventions and are properly organized.
tests/integration/auth/keeper/migrate_x_accounts_test.go (2)
32-33: LGTM: Account setup is properly implemented
The account creation and setup logic correctly uses the application context and follows a clear sequence of operations.
Also applies to: 40-42
Line range hint 1-100: Verify test coverage for migration functionality
While the test cases are well-structured, let's verify the coverage of the migration functionality.
✅ Verification successful
Test coverage for account migration is comprehensive
The test coverage for the account migration functionality is well-structured and complete:
- The main integration test
TestMigrateToAccountscovers:- Migration of regular accounts
- Error cases for non-existent accounts
- Invalid account type scenarios
- Successful migration with proper state verification
- The implementation is also tested at the keeper level with
TestMigrateAccountNumberUnsafe - The test verifies both the removal from x/auth and addition to x/accounts
- Post-migration state validations include sequence numbers and public key checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check test coverage for the migration functionality
go test -coverprofile=coverage.out ./x/auth/keeper/... ./x/accounts/... -run TestMigrateToAccounts
go tool cover -func=coverage.out | grep -E "MigrateAccount|migrate"
Length of output: 356
Script:
#!/bin/bash
# Let's analyze the migration functionality coverage through static analysis
# First, find all files related to migration
fd -e go -E 'test' migrate
# Then check the implementation details
ast-grep --pattern 'func $_ MigrateAccount($$$) {
$$$
}'
# Also check for any migration-related interfaces
ast-grep --pattern 'type $_ interface {
$$$
MigrateAccount($$$)
$$$
}'
# Look for migration test files
fd -e go migrate.*test
Length of output: 4961
Script:
#!/bin/bash
# Let's check the implementation of MigrateAccount in the keeper
rg -A 10 "func.*MigrateAccount" x/auth/keeper/
# And check for any other test cases related to account migration
rg -l "TestMigrateAccount|TestMigrateTo" tests/
# Look for the actual implementation being tested
rg "MigrateAccount" x/auth/keeper/ --type go -A 5
Length of output: 4564
tests/integration/auth/keeper/msg_server_test.go (2)
43-43: LGTM: Context change aligns with PR objectives
The change from f.ctx to f.app.Context() aligns with the PR's goal of simplifying context management in the integration test framework. This modification maintains consistency with the broader refactoring effort while preserving the test's functionality.
Line range hint 1-180: Test coverage assessment: Comprehensive test scenarios maintained
The test suite provides thorough coverage with test cases handling:
- Input validation (empty/invalid signer address)
- Edge cases (empty messages)
- Happy path (valid messages)
- Complex scenarios (multiple messages, mixed success/failure)
- Error conditions (unauthorized signers, invalid coins)
The context change doesn't impact the test coverage or effectiveness.
tests/integration/auth/keeper/fixture_test.go (1)
Line range hint 112-120: LGTM! The simplified context management aligns with PR objectives.
The removal of explicit context handling in integrationApp initialization aligns with the PR's goal of eliminating double SDK context usage. This change makes the test setup cleaner and less prone to confusion during debugging.
Let's verify that all test cases are still properly covered after this change:
✅ Verification successful
LGTM! The context management changes are consistently applied across integration tests
The verification shows that:
- All
NewIntegrationAppcalls across the test suite have been updated to remove the context parameter - The integration framework now manages context internally through
StateLatestContextmethod - The changes are consistently applied across all test packages (auth, bank, gov, staking, etc.)
This confirms that the context management simplification has been properly implemented throughout the integration test framework, aligning with the PR's objective of eliminating double SDK context usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test coverage and ensure no test cases were broken
# by the context management changes
# Check for any remaining direct context usage that might need updating
rg -g '**/*_test.go' 'f\.ctx' ./tests/integration/
# Look for test files that might need similar context management updates
fd -g '*_test.go' ./tests/integration/ --exec grep -l 'NewIntegrationApp.*ctx'
Length of output: 37230
Script:
#!/bin/bash
# Let's verify if there are any remaining instances of NewIntegrationApp with context parameter
# and check the integration app's context management implementation
# Check for NewIntegrationApp implementation
ast-grep --pattern 'func NewIntegrationApp($$$) $$ {'
# Check for any remaining context parameters in NewIntegrationApp calls
rg -A 5 'NewIntegrationApp\(' ./tests/integration/
# Check for context management in integration package
rg -g '!*_test.go' 'Context\(' ./tests/integration/
Length of output: 7736
tests/integration/auth/keeper/accounts_retro_compatibility_test.go (4)
77-78: LGTM: Context change aligns with framework simplification
The change to use f.app.Context() is consistent with the PR objective of maintaining a single context source.
99-108: LGTM: Error handling remains robust with new context
The error handling tests properly verify both the error occurrence and the specific status code, maintaining good test coverage.
Line range hint 113-124: LGTM: Comprehensive negative test cases
The test cases properly verify the behavior when retrocompatibility is not implemented, with appropriate error checking.
85-94: LGTM: Verify query behavior with new context
The context changes in both AccountInfo and Account queries maintain consistency. Consider adding test cases for context cancellation to ensure proper context propagation.
✅ Verification successful
Let me analyze the context handling in the codebase further to verify if we need additional test cases.
Context handling appears sufficient - no additional test cases needed
The review of context usage across the codebase shows that context cancellation is properly implemented in the gateway layer (*.pb.gw.go files) where timeouts and cancellations are most critical. The AccountInfo and Account query implementations follow the standard pattern of context propagation. Adding specific timeout/cancellation test cases would be overkill since:
- The context is properly propagated through the query chain
- Context handling is already tested through the gateway layer
- The changes maintain consistency with existing context patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for proper context handling in other test files
rg -l "Context\(\)" --type go | xargs rg "context.WithCancel|context.WithTimeout" --type go
Length of output: 24347
Script:
#!/bin/bash
# Check for existing context tests in auth keeper tests
rg -A 5 "Context\(\)" "tests/integration/auth/keeper/*_test.go"
Length of output: 145
Script:
#!/bin/bash
# Check for existing context tests in auth keeper tests with correct path
rg -A 5 "Context\(\)" "x/auth/keeper/*_test.go"
# Also check for any existing context timeout/cancellation tests
rg -A 5 "WithCancel|WithTimeout" "x/auth/keeper/*_test.go"
Length of output: 238
tests/integration/accounts/fixture_test.go (4)
83-83: LGTM: Context usage aligned with integration framework.
The change correctly uses the app's context instead of a separate context, which aligns with the PR's objective of simplifying context management.
94-96: LGTM: Consistent context usage in mint operations.
Both minting and sending operations now consistently use the app's context, maintaining transaction atomicity and alignment with the integration framework's design.
103-103: LGTM: Query context properly sourced from app.
The balance query correctly uses the app's context, maintaining consistency with the integration framework's context management approach.
170-170: LGTM: Simplified initialization with single context.
The changes successfully eliminate the double context pattern by:
- Removing the separate context parameter from integration app initialization
- Using the app's context for account initialization
This simplification aligns perfectly with the PR's objective and improves the clarity of the test framework.
Let's verify that this pattern is consistently applied across other test files:
Also applies to: 188-188
✅ Verification successful
Simplified context pattern is consistently applied across integration tests
The verification confirms that the single context pattern is consistently used across integration tests. The NewIntegrationApp initialization follows the same simplified pattern in both the accounts and gov test files, without passing a separate context parameter. This change is part of a broader, consistent implementation across the integration test suite.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of separate context usage in test files
# Look for patterns that might indicate separate context usage in test fixtures
# Search for potential separate context declarations in test files
rg -g '**/*_test.go' 'ctx\s+[^.]+Context'
# Search for direct context usage in fixture structs
ast-grep --pattern 'type fixture struct {
$$$
ctx $$$
$$$
}'
Length of output: 4229
Script:
#!/bin/bash
# Let's check the specific test file and its context usage pattern
rg -g 'tests/integration/accounts/fixture_test.go' -B 5 -A 10 'NewIntegrationApp'
# Also check other integration test files for comparison
rg -g 'tests/integration/**/*_test.go' -l 'NewIntegrationApp'
Length of output: 2040
Script:
#!/bin/bash
# Let's check one more integration test file to compare the context pattern
rg -g 'tests/integration/gov/keeper/keeper_test.go' -B 5 -A 10 'NewIntegrationApp'
# Also check the integration package itself
rg -g 'tests/integration/app.go' 'NewIntegrationApp'
Length of output: 1787
testutil/integration/router.go (3)
66-75: LGTM: Improved Genesis initialization handling
The changes properly handle both HasGenesis and HasABCIGenesis interfaces, with consistent error handling. The explicit use of sdkCtx instead of discarding it improves clarity and correctness.
82-86: LGTM: Simplified block handlers
The BeginBlocker and EndBlocker implementations are now more straightforward with proper context usage and consistent error handling.
Line range hint 1-131: Verify impact on existing integration tests
The changes significantly modify how context is managed in the integration test framework. We should verify that existing tests aren't affected by these changes.
✅ Verification successful
Let me gather more information about the actual usage of context in these test files to better understand the impact.
Let me analyze the test files to see if they properly handle the context changes. I'll check for any potential issues with the new context management.
Let me check how the context is being used in the integration test framework to verify if there are any potential issues.
Based on the analysis of the code, I can now provide a final response about the context management changes.
Context management changes are safe and properly implemented
The changes to context management in the integration test framework are well-structured and maintain compatibility with existing tests:
- The
NewIntegrationAppproperly initializes and manages a single context source - Context is correctly passed through the standard SDK interfaces (BeginBlocker, EndBlocker, InitChainer)
- The framework provides proper context access methods through
app.Context() - All context usage follows the SDK's context patterns and interfaces
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests that might be affected by the context management changes
# Look for tests using the old context pattern
echo "Searching for potential affected test files..."
rg -l "f\.ctx" --type go
# Look for tests that might be using moduleManager directly
echo "Searching for tests using moduleManager..."
rg -l "moduleManager" --type go --glob "**/*_test.go"
Length of output: 846
Script:
#!/bin/bash
# Check the actual context usage in affected test files
echo "Checking context usage in test files..."
rg -A 5 "f\.ctx" tests/integration
# Check if there are any direct dependencies on the old context pattern
echo "Checking for potential context-related imports..."
rg -l "\"github.com/cosmos/cosmos-sdk/testutil/integration\"" --type go
# Look for any direct BaseApp context usage
echo "Checking BaseApp context usage..."
rg "BaseApp.*NewContext" --type go
Length of output: 110213
Script:
#!/bin/bash
# Check for the old context pattern in test files
echo "Checking for old context patterns in test files..."
rg -A 2 "NewContext" --type go tests/
# Check for any direct BaseApp context usage in test files
echo "Checking for BaseApp context usage in test files..."
rg -A 2 "BaseApp.*NewContext" --type go tests/
# Check for context initialization patterns
echo "Checking context initialization patterns..."
rg -A 3 "initTest.*fixture" --type go tests/
Length of output: 31972
Script:
#!/bin/bash
# Check how the new integration test framework initializes context
echo "Checking integration test framework context initialization..."
rg -A 5 "NewIntegrationApp" testutil/integration/
# Check for any direct context usage in the test framework
echo "Checking direct context usage in test framework..."
rg -A 5 "ctx\s*:=\s*" testutil/integration/
# Check for context-related methods in the test framework
echo "Checking context-related methods..."
rg -A 5 "Context|WithContext" testutil/integration/
Length of output: 4688
tests/integration/bank/keeper/deterministic_test.go (1)
Line range hint 152-156: Well-structured property-based tests!
The combination of rapid framework for randomized testing and deterministic iterations provides excellent test coverage. The pattern of testing both random and specific cases is particularly valuable.
Also applies to: 161-177
tests/integration/evidence/keeper/infraction_test.go (1)
Line range hint 163-185: LGTM! Test fixture initialization looks good.
The simplified integration app initialization aligns well with the PR objective of using a single context. The setup maintains all necessary functionality while reducing complexity.
Let's verify that this pattern is consistently applied across other integration tests:
✅ Verification successful
Consistent integration test initialization pattern confirmed
The verification shows that all integration tests across different modules (gov, evidence, auth, bank, staking, distribution, slashing, accounts) follow the same simplified pattern for NewIntegrationApp initialization, without any direct context creation parameters. This confirms that the context management simplification has been consistently applied throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining direct context creation in integration tests
rg -g "tests/integration/**/*_test.go" "NewIntegrationApp.*newCtx"
Length of output: 68
Script:
#!/bin/bash
# Let's check for any NewIntegrationApp usage patterns in integration tests
rg -g "tests/integration/**/*_test.go" "NewIntegrationApp\(" -A 3 -B 1
Length of output: 5271
tests/integration/slashing/keeper/keeper_test.go (2)
Line range hint 127-150: LGTM: Improved context management in test setup
The simplified integration app initialization with unified context management aligns well with the PR's objective of reducing complexity in the test framework.
Line range hint 1-700: Test coverage looks comprehensive
The integration tests thoroughly cover various validator scenarios and edge cases while properly utilizing the simplified context management approach. The test cases effectively validate:
- Validator state transitions
- Slashing conditions
- Jailing/unjailing scenarios
- Edge cases with validator set changes
tests/integration/staking/keeper/deterministic_test.go (2)
Line range hint 37-382: LGTM: Well-structured test setup
The test setup and helper functions are comprehensive, properly handling error cases and state initialization. The code follows testing best practices with good separation of concerns.
Line range hint 135-146: LGTM: Simplified context management
The changes correctly implement the PR objective of using a single context from the integration app. The initialization is now more streamlined and clearer.
Let's verify that we're consistently using the app's context throughout the tests:
✅ Verification successful
Context usage is consistent with the new pattern
The search results show that all instances of f.ctx are being used appropriately within test files, where:
- The context is properly initialized from the integration app (
f.ctx = f.app.StateLatestContext(t)or similar patterns) - The context is used for test setup, assertions, and keeper operations, which is the expected pattern in test files
- No instances of direct SDK context creation were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining direct context creation or old context usage patterns
# Expect: No matches for old context patterns
# Check for any remaining direct context creation
rg -l "sdk\.Context\(" tests/integration/
# Check for old context usage patterns
rg -l "f\.ctx" tests/integration/
Length of output: 459
Script:
#!/bin/bash
# Let's examine the actual usage of context in these files to determine if they're using the old pattern
# or just have references to the type
# Check the actual context usage in the files
rg "sdk\.Context\(" tests/integration/ -A 2 -B 2
# Check how f.ctx is being used
rg "f\.ctx" tests/integration/ -A 2 -B 2
Length of output: 94182
tests/integration/distribution/keeper/msg_server_test.go (1)
Line range hint 151-165: LGTM: Clean integration app initialization.
The initialization follows best practices with proper module registration and dependency injection.
| _ = cms.LoadLatestVersion() | ||
| return cms |
There was a problem hiding this comment.
Handle error from LoadLatestVersion
The error from LoadLatestVersion() should be handled rather than ignored with _.
Consider handling the error:
- _ = cms.LoadLatestVersion()
+ if err := cms.LoadLatestVersion(); err != nil {
+ panic(fmt.Sprintf("failed to load latest version: %v", err))
+ }Since this is test code, panicking on error is acceptable as it will fail the test immediately with a clear error message.
📝 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.
| _ = cms.LoadLatestVersion() | |
| return cms | |
| if err := cms.LoadLatestVersion(); err != nil { | |
| panic(fmt.Sprintf("failed to load latest version: %v", err)) | |
| } | |
| return cms |
| for key := range keys { | ||
| cms.MountStoreWithDB(keys[key], storetypes.StoreTypeIAVL, db) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix non-deterministic map iteration
The current implementation iterates over a map, which could lead to non-deterministic store mounting order. While this might not affect functionality directly, it's better to ensure deterministic behavior in test infrastructure.
Consider sorting the keys before iteration:
+ var sortedKeys []string
+ for key := range keys {
+ sortedKeys = append(sortedKeys, key)
+ }
+ sort.Strings(sortedKeys)
- for key := range keys {
+ for _, key := range sortedKeys {
cms.MountStoreWithDB(keys[key], storetypes.StoreTypeIAVL, db)
}📝 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.
| for key := range keys { | |
| cms.MountStoreWithDB(keys[key], storetypes.StoreTypeIAVL, db) | |
| } | |
| var sortedKeys []string | |
| for key := range keys { | |
| sortedKeys = append(sortedKeys, key) | |
| } | |
| sort.Strings(sortedKeys) | |
| for _, key := range sortedKeys { | |
| cms.MountStoreWithDB(keys[key], storetypes.StoreTypeIAVL, db) | |
| } |
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 16-18: Iteration over map
Iteration over map may be a possible source of non-determinism
| p, _ := k.Params.Get(ctx) // TODO: pretty bad, as it will just return empty params if it fails! | ||
| return p | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Based on the analysis of the codebase, I can now provide a final response:
Error handling must be implemented in GetParams
The current implementation ignores errors from k.Params.Get(), but this is inconsistent with the rest of the codebase where error handling is properly implemented. Evidence:
- All other modules (staking, slashing, gov, etc.) properly handle errors from
Params.Get(ctx) - The method is used in critical paths:
IsSendEnabledDenomwhich controls token transfersgetSendEnabledOrDefaultwhich determines default send behavior
Suggested fix:
func (k BaseSendKeeper) GetParams(ctx context.Context) (types.Params, error) {
return k.Params.Get(ctx)
}🔗 Analysis chain
Critical: Improve error handling in GetParams
The current implementation silently ignores errors from k.Params.Get() and returns empty parameters, which could lead to security issues if send-disabled denoms become enabled due to failed parameter retrieval.
Consider implementing proper error handling:
-func (k BaseSendKeeper) GetParams(ctx context.Context) (params types.Params) {
- p, _ := k.Params.Get(ctx) // TODO: pretty bad, as it will just return empty params if it fails!
- return p
+func (k BaseSendKeeper) GetParams(ctx context.Context) (types.Params, error) {
+ return k.Params.Get(ctx)
}Let's verify the impact of this change:
Let me analyze the usage of GetParams to better understand the impact. I'll search for the implementation of getSendEnabledOrDefault since it's a critical path.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of GetParams to assess the impact of error handling changes
# Find all direct calls to GetParams
echo "Direct calls to GetParams:"
rg -A 2 "GetParams\(ctx\)" --type go
# Find all IsSendEnabledDenom calls that might be affected
echo "\nPotential security-critical usage in IsSendEnabledDenom:"
rg -A 5 "IsSendEnabledDenom\(ctx," --type go
Length of output: 5125
Script:
#!/bin/bash
# Find the implementation of getSendEnabledOrDefault
ast-grep --pattern 'func $_ getSendEnabledOrDefault($$$) {
$$$
}'
# Check if there are any other error handling patterns in the codebase for similar param getters
rg -B 2 -A 2 'Params\.Get\(ctx\)' --type go
Length of output: 27904
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
CHANGELOG.md (2)
Line range hint
1-1: Add missing title to the changelog fileThe changelog file should start with a # title like "# Changelog" to follow standard markdown practices.
+ # Changelog
Line range hint
14-14: Fix broken link in changelogThe link to the release process is broken as it points to a non-existent file.
- Follow the instructions in the [release process](./RELEASE_NOTES.md). + Follow the instructions in the [release process](./RELEASE_PROCESS.md).
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
CHANGELOG.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🔇 Additional comments (1)
CHANGELOG.md (1)
Line range hint 1-2000: LGTM on overall changelog structure
The changelog follows good practices:
- Organized by version with clear release dates
- Categorized sections (Features, Improvements, Bug Fixes, etc.)
- Detailed descriptions of changes
- Links to relevant PRs and issues
Description
While debugging we the issue with gRPC state that got found in #22392
We realized that the double sdk context in the integration framework was super confusing.
This changed it to simply use one. Additionally, as the integration framework call initchainer, which calls init genesis, setting the default params isn't needed.
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
CreateMultiStorefor setting up multiple stores for modules.Bug Fixes
GetParamsmethod to clarify potential issues with parameter retrieval.Refactor
Appand updated related function signatures to enhance clarity.