test: v2 services helpers and demo using x/bank
#23057
Conversation
| } | ||
|
|
||
| // WithGas sets the gas config and meter on a testing ctx and returns the updated ctx. | ||
| func (t TestContext) WithGas(gasConfig gas.GasConfig, gasMeter gas.Meter) TestContext { |
There was a problem hiding this comment.
Created the TestContext wrapper so that we can extend with methods to modify it like the ones here
core/testing/environment.go
Outdated
|
|
||
| type TestEnvironment struct { | ||
| env appmodulev2.Environment | ||
| memEventsService MemEventsService |
There was a problem hiding this comment.
Include the concrete types of our Mem services so that we can use the additional helper methods (such as GetEvents) in our tests
📝 WalkthroughWalkthroughThis pull request introduces significant enhancements to the testing framework within the Cosmos SDK. Key changes include the creation of a new Changes
Suggested labels
Suggested reviewers
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (13)
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
|
core/testing/router.go
Outdated
| var ErrNoHandler = errors.New("no handler") | ||
|
|
||
| // NewMsgRouterBuilder is a router that routes messages to their respective handlers. | ||
| func NewMsgRouterBuilder() *ReflectionRouterBuilder { |
There was a problem hiding this comment.
Core-only router builder that uses reflect instead of protoreflect for test pathfinding
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
aljo242
requested review from
a team,
julienrbrt,
hieuvubk,
akhilkumarpilli and
sontrinh16
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
testutil/queryclient/queryclient.go (1)
68-71: Improve error handling in NewStream method.Instead of using a panic, consider returning a proper error message indicating that streaming is not supported in the test utility.
-func (q *QueryHelper) NewStream(context.Context, *grpc.StreamDesc, string, ...grpc.CallOption) (grpc.ClientStream, error) { - panic("not implemented") +func (q *QueryHelper) NewStream(context.Context, *grpc.StreamDesc, string, ...grpc.CallOption) (grpc.ClientStream, error) { + return nil, fmt.Errorf("streaming is not supported in the test query helper") }schema/appdata/mux.go (1)
142-142: Fix code formatting.Remove trailing whitespace after
batch.apply(&listener).- err := batch.apply(&listener) + err := batch.apply(&listener)🧰 Tools
🪛 golangci-lint (1.62.2)
142-142: File is not
gofumpt-ed with-extra(gofumpt)
x/bank/keeper/keeper_test.go (2)
1390-1394: Improve event testing robustness.The test relies on magic numbers for event array indices (8 and 7). Consider creating helper functions to find specific events by type and attributes to make the tests more maintainable.
Example helper function:
func findEventByType(events []coreevent.Event, eventType string) (coreevent.Event, bool) { for _, event := range events { if event.Type == eventType { return event, true } } return coreevent.Event{}, false }
1445-1445: Add comments explaining event counts.The test expects specific event counts (10 and 25) but doesn't explain why these numbers are expected. Add comments explaining the event breakdown to improve test maintainability.
+// 10 events: 2 mint + 2 coin_spent + 2 coin_recv + 2 transfer + 2 message events events = suite.env.MemEventsService().GetEvents(suite.ctx) require.Equal(10, len(events)) +// 25 events: previous 10 + 3 additional funding operations (5 events each) events = suite.env.MemEventsService().GetEvents(suite.ctx) require.Equal(25, len(events))Also applies to: 1460-1460
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
core/testing/context.go(2 hunks)schema/appdata/mux.go(1 hunks)testutil/queryclient/queryclient.go(1 hunks)x/bank/keeper/grpc_query_test.go(17 hunks)x/bank/keeper/keeper_test.go(21 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/bank/keeper/grpc_query_test.go
🧰 Additional context used
📓 Path-based instructions (4)
schema/appdata/mux.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
core/testing/context.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/bank/keeper/keeper_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
testutil/queryclient/queryclient.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 golangci-lint (1.62.2)
schema/appdata/mux.go
142-142: File is not gofumpt-ed with -extra
(gofumpt)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: tests (00)
- GitHub Check: test-system-v2
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (1)
core/testing/context.go (1)
15-19: LGTM! Well-structured context wrapper.The
TestContextimplementation is clean and follows good practices with proper interface checks and clear method documentation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
core/testing/event.go (1)
16-21: Consider adding validation for empty moduleName.While the implementation is correct, it might be worth adding validation to prevent empty module names.
func NewTestEventService(ctx context.Context, moduleName string) TestEventService { + if moduleName == "" { + panic("moduleName cannot be empty") + } unwrap(ctx).events[moduleName] = nil unwrap(ctx).protoEvents[moduleName] = nil return TestEventService{moduleName: moduleName} }core/testing/transaction.go (1)
13-17: Consider documenting the default ExecMode behavior.While the implementation is correct, it would be helpful to document the default execution mode behavior for test scenarios.
+// ExecMode returns the execution mode from the test context. +// By default, it returns the mode set in the dummy context. func (m TestTransactionService) ExecMode(ctx context.Context) transaction.ExecMode { dummy := unwrap(ctx) return dummy.execMode }core/testing/gas.go (1)
13-23: Consider adding panic recovery for unwrap failures.The implementation is correct, but it might be worth adding panic recovery when unwrapping fails to provide better error messages in tests.
func (m TestGasService) GasMeter(ctx context.Context) gas.Meter { + defer func() { + if r := recover(); r != nil { + panic(fmt.Sprintf("failed to get gas meter: %v", r)) + } + }() dummy := unwrap(ctx) return dummy.gasMeter }core/testing/environment.go (1)
36-38: Document nil service implications.Some services are explicitly set to nil. Consider documenting the implications of these nil services for test scenarios.
type TestEnvironment struct { appmodulev2.Environment + // testEventService and testHeaderService are concrete implementations + // that provide additional testing capabilities testEventService TestEventService testHeaderService TestHeaderService }Also applies to: 44-44
core/testing/router.go (1)
85-92: Consider documenting non-deterministic behavior.The map iteration in the
Buildmethod may produce non-deterministic results. While this is acceptable in test code, it should be documented to avoid confusion.Add a comment like:
+// Note: The order of handler registration is non-deterministic due to map iteration. for msgType, handler := range b.handlers {
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
core/testing/environment.go(1 hunks)core/testing/event.go(1 hunks)core/testing/gas.go(1 hunks)core/testing/header.go(1 hunks)core/testing/router.go(1 hunks)core/testing/transaction.go(1 hunks)schema/appdata/mux.go(1 hunks)testutil/queryclient/queryclient.go(1 hunks)x/bank/keeper/keeper_test.go(21 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- core/testing/header.go
- testutil/queryclient/queryclient.go
🧰 Additional context used
📓 Path-based instructions (7)
core/testing/transaction.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
core/testing/gas.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
core/testing/environment.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
core/testing/event.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/bank/keeper/keeper_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
core/testing/router.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
schema/appdata/mux.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 golangci-lint (1.62.2)
schema/appdata/mux.go
[medium] 142-142: G601: Implicit memory aliasing in for loop.
(gosec)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: tests (00)
- GitHub Check: test-system-v2
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (10)
core/testing/event.go (2)
12-14: LGTM! Clean and focused type definition.The
TestEventServicetype is well-defined with a clear purpose and minimal state.
24-33: LGTM! Methods are concise and follow interface contract.The implementation of
EventManager,GetEvents, andGetProtoEventsis clean and follows the interface contract correctly.core/testing/transaction.go (1)
9-11: LGTM! Clean interface implementation.The type definition and interface compliance check are well-structured.
core/testing/gas.go (1)
9-11: LGTM! Clean interface implementation.The type definition and interface compliance check are well-structured.
core/testing/environment.go (3)
13-25: LGTM! Well-structured configuration types.The configuration and environment types are well-defined with clear separation of concerns.
55-65: LGTM! Clean service accessor methods.The service accessor methods are well-implemented and provide clear access to the underlying services.
27-53: 🛠️ Refactor suggestionConsider alternatives to context.WithValue for environment storage.
As noted in previous reviews, using
context.WithValuecan obscure type checking. Consider alternative approaches to store the environment reference.type TestContext struct { context.Context + environment appmodulev2.Environment } func NewTestEnvironment(cfg TestEnvironmentConfig) (TestContext, TestEnvironment) { ctx := Context() // ... existing setup code ... - ctx.Context = context.WithValue(ctx.Context, corecontext.EnvironmentContextKey, env.Environment) + ctx.environment = env.Environment return ctx, env }Likely invalid or redundant comment.
core/testing/router.go (1)
16-23: LGTM! Well-structured router implementation.The router implementation follows good practices with proper initialization and error handling.
x/bank/keeper/keeper_test.go (2)
1390-1394: LGTM! Event handling is properly updated.The event handling is correctly migrated to use the new environment service.
133-141: LGTM! Test environment setup is well-structured.The test environment configuration and initialization is properly implemented using the new
coretestingpackage.
| @@ -139,7 +139,7 @@ func ListenerMux(listeners ...Listener) Listener { | |||
|
|
|||
| mux.onBatch = func(batch PacketBatch) error { | |||
| for _, listener := range listeners { | |||
| err := batch.apply(&listener) //nolint:gosec // aliasing is safe here | |||
| err := batch.apply(&listener) | |||
There was a problem hiding this comment.
Address potential memory aliasing in loop iteration.
The loop variable listener is being passed by reference to batch.apply, which could lead to unexpected behavior if the batch processing modifies the listener state.
Apply this diff to create a local copy of the listener:
-err := batch.apply(&listener)
+listenerCopy := listener
+err := batch.apply(&listenerCopy)📝 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.
| err := batch.apply(&listener) | |
| listenerCopy := listener | |
| err := batch.apply(&listenerCopy) |
🧰 Tools
🪛 golangci-lint (1.62.2)
[medium] 142-142: G601: Implicit memory aliasing in for loop.
(gosec)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
x/bank/keeper/keeper_test.go (6)
154-162: LGTM! Consider adding environment validation.The test environment setup looks good and aligns with the new testing framework. The configuration and initialization are properly structured.
Consider adding validation for required environment fields before initialization:
testEnironmentConfig := coretesting.TestEnvironmentConfig{ ModuleName: banktypes.ModuleName, Logger: log.NewNopLogger(), MsgRouter: nil, QueryRouter: nil, + // Add validation + if testEnironmentConfig.ModuleName == "" { + return nil, nil, fmt.Errorf("module name is required") + } }
1411-1415: Improve test readability by explaining the event count.The magic number
8in the event count check makes it difficult to understand why exactly 8 events are expected.Consider adding a comment explaining the event breakdown or use constants:
+// Expected events: +// 1. EventTypeMint from funding +// 2-7. EventTypeCoinSpent and EventTypeCoinReceived from funding +// 8. EventTypeTransfer from the actual transfer require.Equal(8, len(events))
Line range hint
1456-1466: Improve consistency in event count handling.The test uses magic numbers (10, 25) for event count verification without explaining the breakdown of expected events.
Consider using constants or helper functions to make the event count expectations more maintainable:
+const ( + baseEventCount = 3 // mint + coin_spent + coin_recv + fundingEventCount = baseEventCount * 3 // for 3 funding operations + transferEventCount = 1 +) + -require.Equal(10, len(events)) +require.Equal(baseEventCount*3 + transferEventCount, len(events))Also applies to: 1481-1481
1572-1572: Clarify context handling in test.The comment about context reassignment could be more explicit about why it's necessary.
Consider adding a more descriptive comment:
-// Go back to the suite's context since mockFundAccount uses that; FundAccount would fail for bad mocking otherwise. +// Reset to suite's context to ensure consistent mocking behavior. +// mockFundAccount expects the suite's context for proper mock setup, +// and FundAccount would fail with the modified context. ctx = suite.ctx
1734-1734: Consider using explicit time handling.The test relies on the environment's header time, which is good for consistency but could be more explicit.
Consider using a helper function for time-related test setup:
+func (suite *KeeperTestSuite) getTestTime() time.Time { + return suite.env.HeaderService().HeaderInfo(suite.ctx).Time +} + -vacc, err := vesting.NewContinuousVestingAccount(acc0, origCoins, suite.env.HeaderService().HeaderInfo(suite.ctx).Time.Unix(), endTime.Unix()) +vacc, err := vesting.NewContinuousVestingAccount(acc0, origCoins, suite.getTestTime().Unix(), endTime.Unix())
136-137: Consider adding specific tests for TestContext and TestEnvironment.While the existing tests implicitly verify the new testing framework, explicit tests would be valuable.
Consider adding dedicated test cases:
func (suite *KeeperTestSuite) TestEnvironmentSetup() { require := suite.Require() // Test environment initialization require.NotNil(suite.env) require.Equal(banktypes.ModuleName, suite.env.ModuleName()) // Test context initialization require.NotNil(suite.ctx) require.NotZero(suite.ctx.HeaderInfo().Time) }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
x/bank/go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
x/bank/go.mod(2 hunks)x/bank/keeper/grpc_query_test.go(17 hunks)x/bank/keeper/keeper_test.go(21 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- x/bank/go.mod
- x/bank/keeper/grpc_query_test.go
🧰 Additional context used
📓 Path-based instructions (1)
x/bank/keeper/keeper_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: tests (00)
- GitHub Check: test-system-v2
- GitHub Check: Analyze
- GitHub Check: Summary
testutil/queryclient/queryclient.go
Outdated
|
|
||
| // Route returns the GRPCQueryHandler for a given query route path or nil | ||
| // if not found | ||
| func (qrt *QueryHelper) Route(path string) GRPCQueryHandler { |
There was a problem hiding this comment.
the receivers above use q and here it is qrt. maybe just do q for all
| @@ -165,7 +165,10 @@ require ( | |||
| golang.org/x/arch v0.12.0 // indirect | |||
| ) | |||
|
|
|||
| replace github.com/cosmos/cosmos-sdk => ../../. | |||
| replace ( | |||
| cosmossdk.io/core/testing => ../../core/testing | |||
There was a problem hiding this comment.
double checking if this is intended or just leftover from devving?
There was a problem hiding this comment.
yeah this is intended to get the latest core testing utils
| appmodulev2.RegisterMsgHandler(router, handlers.MsgUpdateParams) | ||
| appmodulev2.RegisterMsgHandler(router, handlers.MsgSend) | ||
| appmodulev2.RegisterMsgHandler(router, handlers.MsgMint) |
There was a problem hiding this comment.
was this the wrong spot for this registration logic? registration of services is usually handled in module.go, so curious if there was an issue with this
There was a problem hiding this comment.
this is just so you could do keeper testing where you register to a router at the keeper package level and don't have to import module, creating some kind of circular dep
Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
testutil/queryclient/queryclient.go (3)
22-42: Maintain consistent receiver naming.For consistency with other methods in this file that use
q, consider usingqinstead ofqhin the constructor.- qh := &QueryHelper{ + q := &QueryHelper{ cdc: cdc, routes: map[string]GRPCQueryHandler{}, } - reflection.RegisterReflectionServiceServer(qh, reflection.NewReflectionServiceServer(interfaceRegistry)) + reflection.RegisterReflectionServiceServer(q, reflection.NewReflectionServiceServer(interfaceRegistry)) - return qh + return q
68-71: Improve documentation for unimplemented method.The
NewStreammethod should include a comment explaining why it's not implemented and whether it's expected to be implemented in the future.// NewStream implements the grpc ClientConn.NewStream method +// TODO: Streaming is not supported in the current implementation. +// This method exists only to satisfy the grpc.ClientConn interface. func (q *QueryHelper) NewStream(context.Context, *grpc.StreamDesc, string, ...grpc.CallOption) (grpc.ClientStream, error) { panic("not implemented") }
108-124: Enhance error handling with wrapped errors.Consider wrapping errors with additional context to make debugging easier.
res, err := methodHandler(handler, ctx, func(i interface{}) error { - return q.cdc.Unmarshal(req.Data, i) + if err := q.cdc.Unmarshal(req.Data, i); err != nil { + return fmt.Errorf("failed to unmarshal request data: %w", err) + } + return nil }, nil) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to handle method %s: %w", fqName, err) } // proto marshal the result bytes var resBytes []byte resBytes, err = q.cdc.Marshal(res) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to marshal response: %w", err) }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
x/bank/go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
testutil/queryclient/queryclient.go(1 hunks)x/bank/go.mod(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/bank/go.mod
🧰 Additional context used
📓 Path-based instructions (1)
testutil/queryclient/queryclient.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: tests (00)
- GitHub Check: test-simapp-v2
- GitHub Check: test-system-v2
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (2)
testutil/queryclient/queryclient.go (2)
1-15: LGTM! Clean package structure and imports.The package name and import organization follow Go best practices.
17-20: LGTM! Clear interface implementations.The struct correctly implements both
gogogrpc.ClientConnandgogogrpc.Serverinterfaces.
Description
Part of: #22903
core testingpackage to return more services and aTestContextandTestEnvironmentwrapper that can be used throughout module and keeper-level testsx/banktest, removing all uses ofbaseappandsdk.ContextAuthor 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
Release Notes
New Features
QueryHelperfor building query clients in a testing context.Improvements
Testing
Refactoring
These changes primarily focus on improving the testing ecosystem and providing more flexible, modular testing infrastructure for the Cosmos SDK.