refactor(testutil): Removing testutil integration #23091
Conversation
📝 WalkthroughWalkthroughThis pull request involves a comprehensive reorganization and cleanup of integration tests within the Cosmos SDK, specifically focusing on the authentication and testing utility modules. The changes include removing several existing test files from the Changes
Suggested labels
Suggested reviewers
Possibly related PRs
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: 1
🧹 Nitpick comments (6)
tests/integration/v2/auth/module_test.go (1)
11-16: Consider validating additional properties of the created module account.Currently, the test only verifies that the account is not nil. For more comprehensive coverage, consider adding assertions that verify the account address or other critical fields, ensuring the module account is truly initialized as expected.
tests/integration/v2/auth/keeper_bench_test.go (3)
18-68: Consider factoring out the repeated setup logic.The code in lines 21-31 (module configuration) and lines 33-50 (keeper and app initialization) is almost identical to the setup logic in the second benchmark starting at lines 73-83 and 85-102. You could reduce duplication and improve maintainability by extracting this into a shared helper function.
- moduleConfigs := []configurator.ModuleOption{ - ... - } - var accountKeeper keeper.AccountKeeper - startupCfg := integration.DefaultStartUpConfig(b) - app, err := integration.NewApp( - ... - &accountKeeper) - require.NoError(b, err) - ctx := app.StateLatestContext(b) + app, ctx, accountKeeper, err := createTestApp(b) + require.NoError(b, err) ... // Reuse in both BenchmarkAccountMapperGetAccountFound and BenchmarkAccountMapperSetAccount
55-61: Optimize benchmark reliability.Currently, we're setting accounts in the same iteration as we plan to retrieve them. This approach works for the benchmark but might over-inflate the cost. Consider isolating the setup of accounts in a separate phase to more accurately measure read performance only.
70-116: Document the <224 assumption.**The assumption that
b.N < 2**24appears again for the second benchmark. Document or justify this limit to future maintainers who might need to raise it for larger-scale testing.tests/integration/v2/example/example_test.go (2)
24-25: Avoid using panic in test code.
Usingpaniccan obscure errors during test runs. For clarity and improved diagnostics, consider usingrequireorassertmethods fromtestifyto handle errors and unexpected conditions.- panic(err) + require.NoError(t, err, "unexpected error while updating params")🧰 Tools
🪛 golangci-lint (1.62.2)
25-25: tests: Example should be niladic
(govet)
28-28: Apply gofumpt formatting if desired.
A static analysis tool suggests that the file is not formatted usinggofumpt -extra. While not strictly mandatory, you may apply it to ensure consistent styling across the codebase.🧰 Tools
🪛 golangci-lint (1.62.2)
28-28: File is not
gofumpt-ed with-extra(gofumpt)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
tests/integration/auth/keeper/account_retriever_test.go(0 hunks)tests/integration/auth/keeper/accounts_retro_compatibility_test.go(0 hunks)tests/integration/auth/keeper/app_config.go(0 hunks)tests/integration/auth/keeper/fixture_test.go(0 hunks)tests/integration/auth/keeper/keeper_bench_test.go(0 hunks)tests/integration/auth/keeper/migrate_x_accounts_test.go(0 hunks)tests/integration/auth/keeper/module_test.go(0 hunks)tests/integration/auth/keeper/msg_server_test.go(0 hunks)tests/integration/bank/keeper/deterministic_test.go(0 hunks)tests/integration/example/example_test.go(0 hunks)tests/integration/v2/auth/keeper_bench_test.go(1 hunks)tests/integration/v2/auth/module_test.go(1 hunks)tests/integration/v2/example/example_test.go(1 hunks)testutil/integration/doc.go(0 hunks)testutil/integration/helpers.go(0 hunks)testutil/integration/options.go(0 hunks)testutil/integration/router.go(0 hunks)
💤 Files with no reviewable changes (14)
- testutil/integration/doc.go
- testutil/integration/helpers.go
- tests/integration/auth/keeper/app_config.go
- tests/integration/auth/keeper/module_test.go
- testutil/integration/options.go
- tests/integration/bank/keeper/deterministic_test.go
- tests/integration/example/example_test.go
- tests/integration/auth/keeper/accounts_retro_compatibility_test.go
- tests/integration/auth/keeper/fixture_test.go
- tests/integration/auth/keeper/keeper_bench_test.go
- tests/integration/auth/keeper/migrate_x_accounts_test.go
- tests/integration/auth/keeper/account_retriever_test.go
- tests/integration/auth/keeper/msg_server_test.go
- testutil/integration/router.go
🧰 Additional context used
📓 Path-based instructions (3)
tests/integration/v2/auth/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/auth/keeper_bench_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/example/example_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/example/example_test.go
28-28: File is not gofumpt-ed with -extra
(gofumpt)
25-25: tests: Example should be niladic
(govet)
🔇 Additional comments (4)
tests/integration/v2/auth/module_test.go (2)
1-1: Good package naming.
Keeping the package name the same as the module under test aligns with Go best practices, facilitating clarity in the test structure and discoverability.
3-9: Imports look good.
External dependencies are properly scoped to the specific assertions (testify/require) and module types (cosmos-sdk/x/auth/types), maintaining clarity and minimalism in import usage.
tests/integration/v2/auth/keeper_bench_test.go (1)
1-2: Good package naming.
The file structure and package naming look consistent with the new v2 integration framework.
tests/integration/v2/example/example_test.go (1)
91-91: Ensure console prints align with your testing goals.
In typical Go tests, any printed output is captured but not displayed unless the test fails. If this print statement is purely illustrative for an example, it’s fine; otherwise, consider removing or converting to a test assertion.
|
|
||
| // Example shows how to use the integration test framework to test the integration of SDK modules. | ||
| // Panics are used in this example, but in a real test case, you should use the testing.T object and assertions. | ||
| func Example(t *testing.T) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider renaming or making the function niladic.
In Go, Example functions are expected to be niladic for the testing framework to parse their output as examples. If you intend to provide a classic test, rename it (e.g., TestMintIntegration) and keep a *testing.T parameter. If you genuinely want an example test recognized by Go’s toolchain, remove the *testing.T parameter and rely on output comparison to validate correctness.
-func Example(t *testing.T) {
+func TestMintIntegration(t *testing.T) {📝 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 Example(t *testing.T) { | |
| func TestMintIntegration(t *testing.T) { |
🧰 Tools
🪛 golangci-lint (1.62.2)
25-25: tests: Example should be niladic
(govet)
julienrbrt
changed the title
Description
ref: #22904
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
Bug Fixes
New Features
Chores