refactor(tests): Port auth integration tests to server v2 #22554
Conversation
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant enhancements to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
…//github.com/cosmos/cosmos-sdk into son/port_auth_integration_tests_to_server_v2
sontrinh16
requested review from
a team,
facundomedica,
testinginprod,
aaronc,
julienrbrt and
hieuvubk
as code owners
…//github.com/cosmos/cosmos-sdk into son/port_auth_integration_tests_to_server_v2
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
runtime/v2/module.go (1)
290-293: Consider adding error handling for router service initializationThe router service factory initialization could benefit from error handling to gracefully handle initialization failures.
Consider wrapping the service initialization with error handling:
routerBuilder RouterServiceBuilder = routerBuilder{ - msgRouterServiceFactory: stf.NewMsgRouterService, + msgRouterServiceFactory: func(actor []byte) router.Service { + svc := stf.NewMsgRouterService(actor) + if svc == nil { + panic("failed to initialize message router service") + } + return svc + }, queryRouter: stf.NewQueryRouterService(), }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
runtime/v2/module.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
runtime/v2/module.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (2)
runtime/v2/module.go (2)
218-232: LGTM: Environment provisioning changes are well-integrated
The new services (branchService and routerBuilder) are properly integrated into the environment while maintaining backward compatibility.
246-268: LGTM: Clean RouterServiceBuilder implementation
The implementation is concise, follows Go idioms, and correctly implements the interface contract.
…//github.com/cosmos/cosmos-sdk into son/port_auth_integration_tests_to_server_v2
| queryRouter router.Service | ||
| } | ||
|
|
||
| func NewRouterBuilder( |
There was a problem hiding this comment.
Let's kill this as you aren't supposed to use construct that manually
There was a problem hiding this comment.
One Q, are we sure those integration tests run on CI? I cannot find the job in v2-test.yml so I do not think they run.
Could you please add it? (copying test-integration from test.yml to v2-test.yml and renaming it test-integration-v2 and changing what's necessary)
I'll mark it as required afterwards
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
tests/integration/v2/app.go (4)
57-58: Add documentation for the handler type.Consider adding a doc comment explaining the purpose and usage of the handler type for better code maintainability.
+// handler is a function type that processes a context and returns a transaction message and an error. type handler = func(ctx context.Context) (transaction.Msg, error)
94-98: Enhance documentation for new configuration fields.The documentation for BranchService and RouterServiceBuilder could be more descriptive about their purposes and default behaviors.
- // BranchService defines the custom branch service to be used in the app. + // BranchService defines the custom branch service to be used in the app. + // If not provided, a default stf.BranchService will be used. BranchService corebranch.Service - // RouterServiceBuilder defines the custom builder - // for msg router and query router service to be used in the app. + // RouterServiceBuilder defines the custom builder for message and query routing. + // It constructs services that handle message routing and query handling. + // If not provided, a default builder with stf router services will be used. RouterServiceBuilder runtime.RouterServiceBuilder
124-128: Consider making service initialization more configurable.The current implementation directly initializes services with specific implementations. Consider allowing for more flexibility by accepting service factories or builders as parameters.
return StartupConfig{ ValidatorSet: CreateRandomValidatorSet, GenesisBehavior: Genesis_COMMIT, GenesisAccounts: []GenesisAccount{ga}, HomeDir: homedir, - BranchService: stf.BranchService{}, - RouterServiceBuilder: runtime.NewRouterBuilder( - stf.NewMsgRouterService, stf.NewQueryRouterService(), - ), + BranchService: getDefaultBranchService(), + RouterServiceBuilder: getDefaultRouterBuilder(), }
131-143: Consider adding more configuration options.The RunMsgConfig currently only supports automatic commit. Consider adding more options that might be useful for testing scenarios, such as:
- Gas limit configuration
- Error handling behavior
- State verification callbacks
type RunMsgConfig struct { Commit bool + GasLimit uint64 + SkipErrorHandling bool + StateVerifier func(context.Context) error } // WithAutomaticCommit enables automatic commit. func WithAutomaticCommit() Option { return func(cfg *RunMsgConfig) { cfg.Commit = true } } +// WithGasLimit sets a custom gas limit for the message execution. +func WithGasLimit(limit uint64) Option { + return func(cfg *RunMsgConfig) { + cfg.GasLimit = limit + } +}tests/integration/v2/auth/app_test.go (1)
35-35: Remove unused fieldcdcinsuitestructThe field
cdcis declared but never used. Removing it will clean up the code.Apply this diff to remove the unused field:
type suite struct { app *integration.App - cdc codec.Codec ctx context.Context authKeeper authkeeper.AccountKeeper accountsKeeper accounts.Keeper bankKeeper bankkeeper.Keeper }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
tests/integration/v2/app.go(6 hunks)tests/integration/v2/auth/app_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
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"
🪛 golangci-lint (1.62.2)
tests/integration/v2/app.go
433-433: test helper function should start from t.Helper()
(thelper)
| // RunMsg runs the handler for a transaction message. | ||
| // It required the context to have the integration context. | ||
| // a new state is committed if the option WithAutomaticCommit is set in options. | ||
| func (app *App) RunMsg(t *testing.T, ctx context.Context, handler handler, option ...Option) (resp transaction.Msg, err error) { | ||
| // set options | ||
| cfg := &RunMsgConfig{} | ||
| for _, opt := range option { | ||
| opt(cfg) | ||
| } | ||
|
|
||
| // need to have integration context | ||
| integrationCtx, ok := ctx.Value(contextKey).(*integrationContext) | ||
| require.True(t, ok) | ||
|
|
||
| resp, err = handler(ctx) | ||
|
|
||
| if cfg.Commit { | ||
| app.lastHeight++ | ||
| _, err := app.Commit(integrationCtx.state) | ||
| require.NoError(t, err) | ||
| } | ||
|
|
||
| return resp, err | ||
| } |
There was a problem hiding this comment.
Improve error handling and test helper implementation.
- Add t.Helper() as this is a test helper function
- Replace require assertions with proper error handling
- Add input validation
func (app *App) RunMsg(t *testing.T, ctx context.Context, handler handler, option ...Option) (resp transaction.Msg, err error) {
+ t.Helper()
+
+ if handler == nil {
+ return nil, fmt.Errorf("handler cannot be nil")
+ }
+
// set options
cfg := &RunMsgConfig{}
for _, opt := range option {
opt(cfg)
}
// need to have integration context
integrationCtx, ok := ctx.Value(contextKey).(*integrationContext)
- require.True(t, ok)
+ if !ok {
+ return nil, fmt.Errorf("context must contain integration context")
+ }
resp, err = handler(ctx)
+ if err != nil {
+ return nil, fmt.Errorf("handler execution failed: %w", err)
+ }
if cfg.Commit {
app.lastHeight++
_, err := app.Commit(integrationCtx.state)
- require.NoError(t, err)
+ if err != nil {
+ return nil, fmt.Errorf("failed to commit state: %w", err)
+ }
}
- return resp, err
+ return resp, nil
}📝 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.
| // RunMsg runs the handler for a transaction message. | |
| // It required the context to have the integration context. | |
| // a new state is committed if the option WithAutomaticCommit is set in options. | |
| func (app *App) RunMsg(t *testing.T, ctx context.Context, handler handler, option ...Option) (resp transaction.Msg, err error) { | |
| // set options | |
| cfg := &RunMsgConfig{} | |
| for _, opt := range option { | |
| opt(cfg) | |
| } | |
| // need to have integration context | |
| integrationCtx, ok := ctx.Value(contextKey).(*integrationContext) | |
| require.True(t, ok) | |
| resp, err = handler(ctx) | |
| if cfg.Commit { | |
| app.lastHeight++ | |
| _, err := app.Commit(integrationCtx.state) | |
| require.NoError(t, err) | |
| } | |
| return resp, err | |
| } | |
| // RunMsg runs the handler for a transaction message. | |
| // It required the context to have the integration context. | |
| // a new state is committed if the option WithAutomaticCommit is set in options. | |
| func (app *App) RunMsg(t *testing.T, ctx context.Context, handler handler, option ...Option) (resp transaction.Msg, err error) { | |
| t.Helper() | |
| if handler == nil { | |
| return nil, fmt.Errorf("handler cannot be nil") | |
| } | |
| // set options | |
| cfg := &RunMsgConfig{} | |
| for _, opt := range option { | |
| opt(cfg) | |
| } | |
| // need to have integration context | |
| integrationCtx, ok := ctx.Value(contextKey).(*integrationContext) | |
| if !ok { | |
| return nil, fmt.Errorf("context must contain integration context") | |
| } | |
| resp, err = handler(ctx) | |
| if err != nil { | |
| return nil, fmt.Errorf("handler execution failed: %w", err) | |
| } | |
| if cfg.Commit { | |
| app.lastHeight++ | |
| _, err := app.Commit(integrationCtx.state) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to commit state: %w", err) | |
| } | |
| } | |
| return resp, nil | |
| } |
🧰 Tools
🪛 golangci-lint (1.62.2)
433-433: test helper function should start from t.Helper()
(thelper)
| func (s suite) mustAddr(address []byte) string { | ||
| str, _ := s.authKeeper.AddressCodec().BytesToString(address) | ||
| return str | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use pointer receiver for mustAddr method for consistency
The mustAddr method uses a value receiver, whereas other methods in the suite struct use pointer receivers. For consistency and to avoid unintended copies, consider changing it to a pointer receiver.
Apply this diff to change the receiver:
-func (s suite) mustAddr(address []byte) string {
+func (s *suite) mustAddr(address []byte) string {
str, _ := s.authKeeper.AddressCodec().BytesToString(address)
return str
}📝 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.
| func (s suite) mustAddr(address []byte) string { | |
| str, _ := s.authKeeper.AddressCodec().BytesToString(address) | |
| return str | |
| } | |
| func (s *suite) mustAddr(address []byte) string { | |
| str, _ := s.authKeeper.AddressCodec().BytesToString(address) | |
| return str | |
| } |
i think it run with the current integration test job |
ah okay, missed that 👍🏾 |
Description
ref: #20799
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
NonAtomicExecmessage handling in the auth module.Bug Fixes
Tests