refactor(tests/integration): Migrate mint and protocolpool integration tests to server v2 #22859
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request involves the deletion of several configuration and test files related to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
tests/integration/v2/mint/module_test.go (1)
48-51: Consider adding more specific assertionsWhile the basic existence check is good, consider adding more specific assertions to verify:
- The correct account type (should be a ModuleAccount)
- The expected permissions for the mint module account
ctx := app.StateLatestContext(t) acc := accountKeeper.GetAccount(ctx, authtypes.NewModuleAddress(types.ModuleName)) require.NotNil(t, acc) + moduleAcc, ok := acc.(*authtypes.ModuleAccount) + require.True(t, ok) + require.Equal(t, types.ModuleName, moduleAcc.Name) + require.Equal(t, []string{"minter"}, moduleAcc.Permissions)tests/integration/v2/protocolpool/module_test.go (4)
62-62: Use a more descriptive variable name instead ofresThe variable
resis used to instantiate thefixturestruct. For improved readability, consider using a more descriptive name likefxorfixture.
111-111: Remove unnecessary print statementThe
fmt.Println(headerInfo)statement will print output during test execution, which is generally discouraged unless for debugging purposes. It can clutter the test output.Apply this diff to remove the print statement:
- fmt.Println(headerInfo)
123-123: Avoid using magic numbers; compute the expected balance dynamicallyUsing hard-coded values like
"11883031stake"makes the test less maintainable and harder to understand. Consider calculating the expected balance within the test or defining it as a constant with a descriptive name.
172-172: Redundant error check; variableerrnot updatedThe
require.NoError(t, err)at this line is redundant sinceerrhasn't been modified since the last check. This could cause confusion.Apply this diff to remove the unnecessary error check:
- require.NoError(t, err)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
tests/integration/mint/app_config.go(0 hunks)tests/integration/mint/module_test.go(0 hunks)tests/integration/protocolpool/app_config.go(0 hunks)tests/integration/protocolpool/module_test.go(0 hunks)tests/integration/v2/mint/module_test.go(1 hunks)tests/integration/v2/protocolpool/module_test.go(1 hunks)
💤 Files with no reviewable changes (4)
- tests/integration/protocolpool/module_test.go
- tests/integration/mint/module_test.go
- tests/integration/mint/app_config.go
- tests/integration/protocolpool/app_config.go
🧰 Additional context used
📓 Path-based instructions (2)
tests/integration/v2/mint/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/protocolpool/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"
🔇 Additional comments (1)
tests/integration/v2/mint/module_test.go (1)
1-24: LGTM! Well-organized imports section
The imports are properly organized and the blank imports are correctly used for app wiring in the Cosmos SDK context.
| ) | ||
|
|
||
| func TestItCreatesModuleAccountOnInitBlock(t *testing.T) { | ||
| t.Helper() |
There was a problem hiding this comment.
Remove unnecessary t.Helper() call
The t.Helper() function should only be used in test helper functions that are called by other tests, not in test functions themselves.
- t.Helper()
There was a problem hiding this comment.
This one should still be removed yes
There was a problem hiding this comment.
@julienrbrt Thank you for confirming! 🐰🎉
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| for i := 0; i < 30; i++ { | ||
| _, state := app.Deliver(t, ctx, nil) | ||
| _, err = app.Commit(state) | ||
| require.NoError(t, err) | ||
|
|
||
| headerInfo := integration.HeaderInfoFromContext(ctx) | ||
| headerInfo.Time = headerInfo.Time.Add(time.Minute) | ||
| ctx = integration.SetHeaderInfo(ctx, headerInfo) | ||
|
|
||
| // withdraw funds randomly, but it must always land on the same end balance | ||
| if rand.Intn(100) > 50 { | ||
| _, err = msgServer.WithdrawContinuousFund(ctx, &protocolpooltypes.MsgWithdrawContinuousFund{ | ||
| RecipientAddress: testAddr0Str, | ||
| }) | ||
| require.NoError(t, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid non-deterministic tests by eliminating randomness
The use of rand.Intn(100) > 50 introduces randomness into the test, which can lead to flaky tests and inconsistent results. Tests should be deterministic to ensure reliability.
Apply this diff to remove randomness:
- if rand.Intn(100) > 50 {
+ {Alternatively, if randomness is necessary, seed the random number generator at the start of the test to make the results reproducible.
Committable suggestion skipped: line range outside the PR's diff.
| ) | ||
|
|
||
| func TestItCreatesModuleAccountOnInitBlock(t *testing.T) { | ||
| t.Helper() |
There was a problem hiding this comment.
This one should still be removed yes
…cosmos/cosmos-sdk into son/port_mint_integration_tests
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/integration/v2/protocolpool/module_test.go (1)
101-106: 🛠️ Refactor suggestionAvoid non-deterministic behavior in tests
The use of
rand.Intn(100) > 50introduces randomness into the test, which can lead to flaky tests and inconsistent results. Tests should be deterministic to ensure reliability.Apply this diff to remove randomness:
- if rand.Intn(100) > 50 { + {
🧹 Nitpick comments (1)
tests/integration/v2/protocolpool/module_test.go (1)
61-89: Refactor common setup code into a helper functionBoth
TestWithdrawAnytimeandTestExpireInTheMiddlehave similar setup code for initializing the application and context. Consider refactoring this shared code into a helper function to reduce duplication and improve maintainability.As an example, you could create a helper function:
func setupTest(t *testing.T) (fixture, sdk.Context) { res := fixture{} startupCfg := integration.DefaultStartUpConfig(t) startupCfg.HeaderService = &integration.HeaderService{} app, err := integration.NewApp( depinject.Configs(configurator.NewAppV2Config(moduleConfigs...), depinject.Supply(log.NewNopLogger())), startupCfg, &res.accountKeeper, &res.protocolpoolKeeper, &res.bankKeeper, &res.stakingKeeper) require.NoError(t, err) ctx := app.StateLatestContext(t) return res, ctx }Then modify your test functions to use this helper:
-func TestWithdrawAnytime(t *testing.T) { - res := fixture{} - - startupCfg := integration.DefaultStartUpConfig(t) - startupCfg.HeaderService = &integration.HeaderService{} - - app, err := integration.NewApp( - depinject.Configs(configurator.NewAppV2Config(moduleConfigs...), depinject.Supply(log.NewNopLogger())), - startupCfg, &res.accountKeeper, &res.protocolpoolKeeper, &res.bankKeeper, &res.stakingKeeper) - require.NoError(t, err) - - ctx := app.StateLatestContext(t) +func TestWithdrawAnytime(t *testing.T) { + res, ctx := setupTest(t)Also applies to: 125-143
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/integration/v2/mint/module_test.go(1 hunks)tests/integration/v2/protocolpool/module_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/v2/mint/module_test.go
🧰 Additional context used
📓 Path-based instructions (1)
tests/integration/v2/protocolpool/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"
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
mintandprotocolpoolmodules, verifying module account creation and fund withdrawal behaviors.Bug Fixes
Chores