refactor: limit breaking changes for next version #22972
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request involves a refactoring of testing utilities across multiple files in the Cosmos SDK project. The primary change is the migration of the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (2)
💤 Files with no reviewable changes (2)
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
|
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: 1
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
client/grpc_query_test.go(1 hunks)tests/integration/distribution/migration_v4_test.go(1 hunks)testutil/integration/helpers.go(1 hunks)types/module/testutil/helpers.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
types/module/testutil/helpers.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/grpc_query_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"
tests/integration/distribution/migration_v4_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.
🔇 Additional comments (4)
testutil/integration/helpers.go (1)
8-9: LGTM! Clear deprecation notice with migration path.
The deprecation notice clearly indicates where to find the new implementation, which helps with the migration process.
client/grpc_query_test.go (1)
40-40: LGTM! Successful migration to new CreateMultiStore.
The change correctly uses the new moduletestutil.CreateMultiStore while maintaining the same functionality.
types/module/testutil/helpers.go (1)
89-99: LGTM! Clean implementation with proper store initialization.
The implementation correctly:
- Uses in-memory database for testing
- Properly initializes the commit multi-store with logging and metrics
- Correctly mounts all provided stores
- Loads the latest version as required
Let's verify that all usages of the old CreateMultiStore have been migrated:
✅ Verification successful
All usages of CreateMultiStore are using the new implementation
The verification shows that all references to CreateMultiStore in the codebase are using the new implementation from moduletestutil package. The function is being used correctly in test files:
tests/integration/distribution/migration_v4_test.goclient/grpc_query_test.go
No references to the old integration.CreateMultiStore were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old integration.CreateMultiStore
rg -l "integration.CreateMultiStore"
Length of output: 36
Script:
#!/bin/bash
# Let's search for any usage of CreateMultiStore function to ensure all references are using the new implementation
rg "CreateMultiStore\(" -A 2
Length of output: 1008
tests/integration/distribution/migration_v4_test.go (1)
Line range hint 33-170: LGTM: Comprehensive test coverage for v4 migration.
The tests thoroughly cover both the data migration (previous proposer) and funds migration aspects, with proper setup, mocks, and assertions.
| @@ -75,7 +74,7 @@ func TestFundsMigration(t *testing.T) { | |||
| authtypes.StoreKey, banktypes.StoreKey, disttypes.StoreKey, | |||
| ) | |||
| logger := log.NewTestLogger(t) | |||
| cms := integration.CreateMultiStore(keys, logger) | |||
| cms := moduletestutil.CreateMultiStore(keys, logger) | |||
There was a problem hiding this comment.
💡 Codebase verification
The CreateMultiStore change is correct but part of a larger migration
The verification shows that the change in tests/integration/distribution/migration_v4_test.go is correct, as CreateMultiStore is already aliased in testutil/integration/helpers.go to point to moduletestutil.CreateMultiStore. However, this change is part of a larger migration effort since:
- The files still importing the old package are using other integration utilities like
NewIntegrationApp,WithAutomaticFinalizeBlock, andWithAutomaticCommit - The old import path is still referenced in multiple test files
- The
testutil/integration/helpers.gofile showsCreateMultiStoreis marked as deprecated with instructions to use the new path directly
🔗 Analysis chain
LGTM: Successful migration of CreateMultiStore usage.
The change aligns with the PR objectives to preserve helper functions while removing the testutil/integration module.
Let's verify that all usages of CreateMultiStore have been migrated consistently:
Migration of CreateMultiStore requires additional changes
The verification revealed that there are still several files importing the old testutil/integration package, and the migration is incomplete. Key findings:
- Files still using the old import:
- tests/integration/server/grpc_test.go
- tests/integration/example/example_test.go
- tests/integration/bank/keeper/deterministic_test.go
- tests/integration/auth/keeper/msg_server_test.go
- tests/integration/auth/keeper/fixture_test.go
Let's verify if these files also use CreateMultiStore and need similar migration:
Let's check what other functions these files are using from the integration package to better understand the migration scope:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to integration.CreateMultiStore
# and verify consistent usage of moduletestutil.CreateMultiStore
# Check for any remaining references to the old import
rg -l "github.com/cosmos/cosmos-sdk/testutil/integration"
# Check for any remaining references to integration.CreateMultiStore
rg "integration\.CreateMultiStore"
# Verify consistent usage of the new import
rg -l "github.com/cosmos/cosmos-sdk/types/module/testutil"
# Verify consistent usage of moduletestutil.CreateMultiStore
rg "moduletestutil\.CreateMultiStore"
Length of output: 4520
Script:
#!/bin/bash
# Check the content of files still using the old import to see if they use CreateMultiStore
for file in tests/integration/server/grpc_test.go tests/integration/example/example_test.go tests/integration/bank/keeper/deterministic_test.go tests/integration/auth/keeper/msg_server_test.go tests/integration/auth/keeper/fixture_test.go; do
echo "=== $file ==="
rg "CreateMultiStore" "$file" -C 2
done
Length of output: 2145
Script:
#!/bin/bash
# Check what other integration package functions these files are using
for file in tests/integration/server/grpc_test.go tests/integration/example/example_test.go tests/integration/bank/keeper/deterministic_test.go tests/integration/auth/keeper/msg_server_test.go tests/integration/auth/keeper/fixture_test.go; do
echo "=== $file ==="
# Get a few lines around any line that uses the integration package
rg -A 2 -B 2 "integration\." "$file"
done
# Also check the helpers file to understand the integration package usage
echo "=== testutil/integration/helpers.go ==="
cat testutil/integration/helpers.go
Length of output: 4404
(cherry picked from commit ca55998)
…22974) Co-authored-by: Julien Robert <[email protected]>
Description
With the removal of testutil/integration happening on main soon: #22904
This makes sure some helpers that are still useful do not get deleted, nor change place between main (0.54) and 0.52
Additionally, check-compat script has been supersede by https://github.com/cosmos/nightly-stack/blob/main/.github/workflows/build-applications.yaml
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
CreateMultiStorefunction to enhance testing capabilities for store management.CreateMultiStorefunction, ensuring compatibility and improved functionality.CreateMultiStorefunction as deprecated, redirecting usage to the updated utility for better organization.