refactor(tests/integration) Migrate lockup and multisig integration tests to server v2 #22924
Conversation
📝 WalkthroughWalkthroughThe pull request involves a comprehensive refactoring of integration tests for lockup and multisig accounts in the Cosmos SDK. The changes primarily focus on migrating test suites to a new server v2 architecture, replacing direct 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-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| for addrStr, power := range membersPowers { | ||
| members = append(members, &v1.Member{Address: addrStr, Weight: power}) | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning test
![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
🧹 Nitpick comments (13)
tests/integration/v2/accounts/multisig/test_suite.go (1)
118-143: Factor out repeated message handlersMultiple custom router service handlers (bankSendHandler, accountsExeccHandler) are defined inline. Having them as separate functions or methods can reduce duplication and improve clarity, especially if additional message types are registered.
tests/integration/v2/accounts/lockup/utils.go (1)
98-108: Check for side effects in ProvideAllLockupAccountsProviding lockup accounts via dependency injection might introduce unexpected side effects if not carefully managed. Keep an eye out for possible collisions or overshadowed configurations in multi-user or multi-module scenarios.
tests/integration/v2/accounts/lockup/delayed_lockup_test_suite.go (3)
23-25: Use consistent time offsets for test reliabilityYou set the current time once, then modify it slightly in subsequent lines. Ensure these offsets (1 minute here, 61 seconds elsewhere) won't cause flakiness in slower or faster CI environments.
Line range hint
53-62: Improve error handling in negative test casesThe “error - execute ...” tests confirm that certain operations fail as expected. It may be helpful to assert specific error messages or codes (e.g., using require.Contains) instead of just checking for non-nil errors.
125-128: Confirm time progression consistency before EndBlockerAfter adjusting the context’s Time, confirm that any modules depending on
BlockTime(like staking, distribution) have consistent updates and no off-by-one second errors.tests/integration/v2/accounts/lockup/permanent_lockup_test_suite.go (1)
Line range hint
51-60: Provide custom error feedback in negative test casesSimilar to delayed lockup, refine negative test checks to confirm the system yields a clear reason for the error. This helps future maintainers quickly identify the cause and expected behavior.
tests/integration/v2/accounts/lockup/continous_lockup_test_suite.go (3)
Line range hint
1-32: Add test documentation to improve maintainability.Consider adding a comment block describing the test's purpose, setup requirements, and test flow. This will help other developers understand and maintain the test suite.
func (s *IntegrationTestSuite) TestContinuousLockingAccount() { + // TestContinuousLockingAccount verifies the behavior of continuous locking accounts by: + // 1. Setting up an account with a 1-minute locking period + // 2. Testing token release over time + // 3. Verifying staking operations with locked/unlocked tokens
32-45: Consider adding edge case tests for time boundaries.The test only verifies the basic case. Consider adding test cases for:
- Initialization at exact end time
- Initialization with zero duration
- Initialization with very long duration
Line range hint
82-141: Enhance staking operation validations.While the test covers the basic operations, consider adding validations for:
- Total balance consistency after each operation
- Correct event emissions
- Error scenarios with invalid amounts
tests/integration/v2/accounts/lockup/periodic_lockup_test_suite.go (2)
Line range hint
33-52: Add test cases for varying period lengths and amounts.The current test uses identical periods. Consider adding test cases for:
- Different amounts per period
- Different period lengths
- Overlapping periods
Line range hint
82-99: Add balance validations between periods.Consider adding checks for:
- Remaining locked balance after each unlock
- Total account balance consistency
- Failed attempts to spend still-locked tokens
tests/integration/v2/accounts/multisig/account_test.go (1)
229-231: Document time manipulation impact.The time manipulation for voting period could be clearer. Add a comment explaining the implications:
+ // Advance time past the voting period (120 seconds) to allow proposal execution headerInfo := integration.HeaderInfoFromContext(ctx) headerInfo.Time = headerInfo.Time.Add(time.Second * 121) ctx = integration.SetHeaderInfo(ctx, headerInfo)tests/integration/v2/services.go (1)
129-141: Add documentation to the new GetAttribute function.The implementation looks good and follows Go idioms. However, it would benefit from documentation explaining its purpose and return values.
Add documentation above the function:
+// GetAttribute returns the first attribute matching the given key from the event. +// If no matching attribute is found or if there's an error accessing the attributes, +// it returns an empty attribute and false. func GetAttribute(e event.Event, key string) (event.Attribute, bool) {
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
tests/integration/accounts/lockup/utils.go(0 hunks)tests/integration/accounts/multisig/test_suite.go(0 hunks)tests/integration/v2/accounts/lockup/continous_lockup_test_suite.go(6 hunks)tests/integration/v2/accounts/lockup/delayed_lockup_test_suite.go(6 hunks)tests/integration/v2/accounts/lockup/periodic_lockup_test_suite.go(7 hunks)tests/integration/v2/accounts/lockup/permanent_lockup_test_suite.go(6 hunks)tests/integration/v2/accounts/lockup/utils.go(1 hunks)tests/integration/v2/accounts/multisig/account_test.go(5 hunks)tests/integration/v2/accounts/multisig/test_suite.go(1 hunks)tests/integration/v2/services.go(1 hunks)
💤 Files with no reviewable changes (2)
- tests/integration/accounts/lockup/utils.go
- tests/integration/accounts/multisig/test_suite.go
🧰 Additional context used
📓 Path-based instructions (8)
tests/integration/v2/services.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/accounts/lockup/permanent_lockup_test_suite.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/accounts/lockup/periodic_lockup_test_suite.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/accounts/lockup/delayed_lockup_test_suite.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/accounts/multisig/account_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/accounts/lockup/continous_lockup_test_suite.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/accounts/lockup/utils.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/accounts/multisig/test_suite.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"
🔇 Additional comments (10)
tests/integration/v2/accounts/multisig/test_suite.go (3)
40-53: Validate field usage for parallel test runs
While embedding the suite.Suite and holding references to keepers and members is convenient for a single test instance, ensure that no mutable state shared among parallel tests leads to data races. Consider carefully if these fields need synchronization or if each test instance has separate state.
59-73: Confirm module ordering correctness in SetupSuite
The sequence of module initializations (accounts, auth, bank, etc.) is crucial for correct wiring in Cosmos-based applications. Conflicts might arise if dependencies are injected out of order. Validate that the selected order meets all dependencies and does not break under certain configurations.
182-184: Non-deterministic map iteration might affect test reproducibility
Iteration over the map 'membersPowers' may produce different orders in repeated runs, potentially causing minor nondeterminism.
tests/integration/v2/accounts/lockup/utils.go (2)
49-57: Validate concurrency safety for suite fields
The references to keepers stored on the suite struct have the potential for concurrent access if tests run in parallel. Confirm that test scenarios do not mutate global states in ways that can clash.
205-213: Clarify usage of nil transaction options
Current calls to Execute or Query pass a nil pointer for transaction options. If advanced features (like fees, gas limits, or memo fields) are needed, ensure the system gracefully handles nil. Otherwise, consider providing a default parameter.
tests/integration/v2/accounts/lockup/delayed_lockup_test_suite.go (1)
84-84: Double-check DelegatedLocking
At line 84, verifying that DelegatedLocking is updated may require ensuring no race conditions or leftover state from prior tests. Consider wrapping in a sub-test with isolation or verifying the exact prior / next states.
tests/integration/v2/accounts/lockup/permanent_lockup_test_suite.go (3)
23-25: Ensure permanent lock logic matches end-user expectations
The permanent lock implies tokens are non-transferable or have limited conditions to unlock. Validate in a production scenario that the chosen approach (setting times, calling EndBlocker) is well-documented so that operators and users understand the constraints.
82-82: Lockup account info usage
At line 82, verifying DelegatedLocking seems crucial to ensuring no partial unlock occurs. The permanent lock scenario may also require stronger invariants. Consider additional checks if the logic has more constraints.
121-121: Logic check for additional funding after lock
Since the account is added more funds at line 121, confirm that the permanent lock does not incorrectly allow unlocking this new portion. The test passes, but consider adding an explicit check or comment about partial vs. complete lock.
tests/integration/v2/services.go (1)
129-141: Verify usage patterns of GetAttribute vs GetAttributes.
The implementation is correct, but let's verify its usage to ensure consumers are using the appropriate function based on their needs.
✅ Verification successful
Let me analyze the usage patterns more deeply with additional scripts to check for potential optimization cases.
Usage patterns of GetAttribute and GetAttributes are appropriate
Based on the analysis of the codebase:
GetAttributeis primarily used in test files for single attribute lookupsGetAttributesis used when multiple attributes with the same key need to be retrieved- No instances of inefficient patterns (multiple sequential GetAttribute calls or GetAttribute within loops) were found
- The implementation and usage patterns align with their intended purposes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage patterns of GetAttribute vs GetAttributes to ensure correct function selection
# Find all usages of both functions
echo "=== GetAttribute usage ==="
rg "GetAttribute\(" --type go
echo -e "\n=== GetAttributes usage ==="
rg "GetAttributes\(" --type go
# Look for potential cases where GetAttributes might be more appropriate
echo -e "\n=== Potential GetAttributes candidates ==="
rg "attr, ok := GetAttribute\([^)]+\).*if ok \{.*GetAttribute\([^)]+\)" --type go
Length of output: 2985
Script:
#!/bin/bash
# Look for patterns where multiple GetAttribute calls are made in sequence
# This could indicate where GetAttributes might be more appropriate
echo "=== Multiple sequential GetAttribute calls ==="
# Using ast-grep to find sequential GetAttribute calls within the same function
ast-grep --pattern 'GetAttribute($_, $_)
$$$
GetAttribute($_, $_)'
echo -e "\n=== GetAttribute calls within loops ==="
# Find cases where GetAttribute is called within loops
ast-grep --pattern 'for $_ {
$$$
GetAttribute($_, $_)
$$$
}'
Length of output: 349
| events := integration.EventsFromContext(ctx) | ||
| for _, e := range events { | ||
| if e.Type == "proposal_tally" { | ||
| propID, found := integration.GetAttribute(e, "proposal_id") | ||
| s.True(found) | ||
|
|
||
| if propID.Value == "1" { | ||
| foundPropResult = true | ||
| status, found := v.GetAttribute("status") | ||
| status, found := integration.GetAttribute(e, "status") | ||
| s.True(found) | ||
| s.Equal(v1.ProposalStatus_PROPOSAL_STATUS_REJECTED.String(), status.Value) | ||
|
|
||
| // exec_err is nil because the proposal didn't execute | ||
| execErr, found := v.GetAttribute("exec_err") | ||
| execErr, found := integration.GetAttribute(e, "exec_err") | ||
| s.True(found) | ||
| s.Equal("<nil>", execErr.Value) | ||
|
|
||
| rejectErr, found := v.GetAttribute("reject_err") | ||
| rejectErr, found := integration.GetAttribute(e, "reject_err") | ||
| s.True(found) | ||
| s.Equal("threshold not reached", rejectErr.Value) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate event verification logic.
This section duplicates event verification logic. Use the suggested helper function here as well:
- events := integration.EventsFromContext(ctx)
- for _, e := range events {
- if e.Type == "proposal_tally" {
- // ... event verification ...
- }
- }
+ s.verifyProposalEvents(ctx, v1.ProposalStatus_PROPOSAL_STATUS_REJECTED, "0", "1")Committable suggestion skipped: line range outside the PR's diff.
| events := integration.EventsFromContext(ctx) | ||
| for _, e := range events { | ||
| if e.Type == "proposal_tally" { | ||
| foundPropResult = true | ||
| status, found := v.GetAttribute("status") | ||
| attr, found := integration.GetAttribute(e, "status") | ||
| s.True(found) | ||
| s.Equal(v1.ProposalStatus_PROPOSAL_STATUS_PASSED.String(), status.Value) | ||
| s.Equal(v1.ProposalStatus_PROPOSAL_STATUS_PASSED.String(), attr.Value) | ||
|
|
||
| yesVotes, found := v.GetAttribute("yes_votes") | ||
| yesVotes, found := integration.GetAttribute(e, "yes_votes") | ||
| s.True(found) | ||
| s.Equal("100", yesVotes.Value) | ||
|
|
||
| noVotes, found := v.GetAttribute("no_votes") | ||
| noVotes, found := integration.GetAttribute(e, "no_votes") | ||
| s.True(found) | ||
| s.Equal("0", noVotes.Value) | ||
|
|
||
| propID, found := v.GetAttribute("proposal_id") | ||
| propID, found := integration.GetAttribute(e, "proposal_id") | ||
| s.True(found) | ||
| s.Equal("0", propID.Value) | ||
|
|
||
| execErr, found := v.GetAttribute("exec_err") | ||
| execErr, found := integration.GetAttribute(e, "exec_err") | ||
| s.True(found) | ||
| s.Equal("<nil>", execErr.Value) | ||
|
|
||
| rejectErr, found := v.GetAttribute("reject_err") | ||
| rejectErr, found := integration.GetAttribute(e, "reject_err") | ||
| s.True(found) | ||
| s.Equal("<nil>", rejectErr.Value) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor event verification into a helper function.
The event verification logic is repeated across tests. Consider extracting it into a helper function:
+func (s *IntegrationTestSuite) verifyProposalEvents(ctx sdk.Context, expectedStatus v1.ProposalStatus, expectedYesVotes string, proposalID string) {
+ events := integration.EventsFromContext(ctx)
+ for _, e := range events {
+ if e.Type == "proposal_tally" {
+ s.verifyEventAttribute(e, "status", expectedStatus.String())
+ s.verifyEventAttribute(e, "yes_votes", expectedYesVotes)
+ s.verifyEventAttribute(e, "proposal_id", proposalID)
+ }
+ }
+}Committable suggestion skipped: line range outside the PR's diff.
sontrinh16
deleted the
son/port_lockup_and_multisig_integration_tests
branch
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
Bug Fixes
Refactor
Chores