test: migrate remaining e2e tests to integration tests #22364
Conversation
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request modifies the testing framework by removing the end-to-end (E2E) tests and transitioning existing E2E test suites to integration tests. Key changes include the deletion of the Changes
Assessment against linked issues
Possibly related PRs
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
|
akhilkumarpilli
changed the title
akhilkumarpilli
changed the title
akhilkumarpilli
requested review from
auricom,
julienrbrt,
tac0turtle and
a team
as code owners
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
|
|
||
| authTest "github.com/cosmos/cosmos-sdk/tests/integration/auth/keeper" |
There was a problem hiding this comment.
is there any reason for having AppConfig in a different package?? looks like some have it in the same package while others don't 🤷♂️
There was a problem hiding this comment.
It's in the same package. Tests are with keeper_test package name and app_config is with keeper package name.
julienrbrt
added
the
backport/v0.52.x
|
removed test-e2e from required job, so this should get merged now. |
There was a problem hiding this comment.
Can you remove this too:
cosmos-sdk/scripts/build/testing.mk
Line 14 in dfc6bf0
.github/workflows/test.yml
Outdated
There was a problem hiding this comment.
Can you remove the test-e2e from the needs?
![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 (19)
tests/integration/auth/keeper/module_test.go (2)
Line range hint
17-24: Consider enhancing test coverage with additional scenarios.While the basic initialization test is good, consider adding more test cases to verify:
- Error scenarios (e.g., invalid configurations)
- Different module account types
- Account permissions and capabilities
Would you like me to help generate additional test cases to improve coverage?
Line range hint
26-30: Verify account properties in the assertion.The current test only checks if the account is not nil. Consider strengthening the assertions by verifying specific properties of the created module account.
ctx := app.BaseApp.NewContext(false) acc := accountKeeper.GetAccount(ctx, types.NewModuleAddress(types.FeeCollectorName)) -require.NotNil(t, acc) +require.NotNil(t, acc) +moduleAcc, ok := acc.(types.ModuleAccountI) +require.True(t, ok) +require.Equal(t, types.FeeCollectorName, moduleAcc.GetName()) +require.True(t, moduleAcc.HasPermission(types.Burner))tests/integration/auth/keeper/account_retriever_test.go (1)
Line range hint
18-45: Consider adding edge cases to strengthen integration test coverageWhile the existing test covers the happy path well, consider adding test cases for:
- Account not found scenarios
- Invalid addresses
- Response to malformed requests
Example addition:
// Test account not found scenario _, err = ar.GetAccount(clientCtx, sdk.AccAddress([]byte("invalid"))) require.Error(t, err)tests/integration/auth/keeper/keeper_bench_test.go (1)
52-53: LGTM! Consider minor formatting improvement.The
authTest.AppConfigchange is correct. Consider moving the closing parenthesis to the next line for better readability:depinject.Configs( depinject.Supply(log.NewNopLogger()), authTest.AppConfig, - ), &accountKeeper) + ), + &accountKeeper, + )tests/integration/accounts/lockup/utils.go (2)
33-39: Consider enhancing suite lifecycle methods.While the renaming is correct, consider these improvements:
- Add documentation comments to describe the setup process
- Implement proper cleanup in TearDownSuite to prevent resource leaks
Consider adding these improvements:
+// SetupSuite initializes the integration test suite by creating a new SimApp instance +// and performing any necessary test environment setup func (s *IntegrationTestSuite) SetupSuite() { s.T().Log("setting up integration test suite") s.app = setupApp(s.T()) } +// TearDownSuite ensures proper cleanup of test resources func (s *IntegrationTestSuite) TearDownSuite() { s.T().Log("tearing down integration test suite") + // TODO: Add cleanup logic if needed + // Example: Close any open connections, clean up test data, etc. }
Line range hint
48-71: Add documentation and improve error context.The utility functions are well-structured but would benefit from proper documentation and more descriptive error handling.
Consider these improvements:
+// executeTx executes a transaction message on the specified account +// Returns an error if the execution fails func (s *IntegrationTestSuite) executeTx(ctx sdk.Context, msg sdk.Msg, app *simapp.SimApp, accAddr, sender []byte) error { _, err := app.AccountsKeeper.Execute(ctx, accAddr, sender, msg, nil) - return err + if err != nil { + return fmt.Errorf("failed to execute transaction: %w", err) + } + return nil } +// queryAcc performs a query on an account and returns the response message func (s *IntegrationTestSuite) queryAcc(ctx sdk.Context, req sdk.Msg, app *simapp.SimApp, accAddr []byte) (transaction.Msg, error) { resp, err := app.AccountsKeeper.Query(ctx, accAddr, req) return resp, err }tests/integration/accounts/multisig/test_suite.go (5)
21-25: Add documentation for the IntegrationTestSuite struct.Consider adding a doc comment describing the purpose of this test suite and its fields, following Go documentation conventions.
+// IntegrationTestSuite defines a test suite for multisig account integration tests. +// It provides necessary setup and helper methods for testing multisig account functionality. type IntegrationTestSuite struct { suite.Suite app *simapp.SimApp members []sdk.AccAddress membersAddr []string }
29-31: Add documentation for the constructor.Consider adding a doc comment for the constructor function.
+// NewIntegrationTestSuite creates a new instance of IntegrationTestSuite. func NewIntegrationTestSuite() *IntegrationTestSuite { return &IntegrationTestSuite{} }
46-46: Document or implement TearDownSuite.Consider either:
- Implementing cleanup logic in TearDownSuite
- Adding a comment explaining why cleanup is unnecessary
-func (s *IntegrationTestSuite) TearDownSuite() {} +// TearDownSuite implements suite.SetupTestSuite. No cleanup is required as the app state +// is discarded after each test suite run. +func (s *IntegrationTestSuite) TearDownSuite() {}
54-57: Add documentation for executeTx helper.Consider adding a doc comment explaining the purpose and parameters of this helper function.
+// executeTx executes a transaction message for the specified account using the given sender. +// It returns any error that occurs during execution. func (s *IntegrationTestSuite) executeTx(ctx context.Context, msg sdk.Msg, accAddr, sender []byte) error { _, err := s.app.AccountsKeeper.Execute(ctx, accAddr, sender, msg, nil) return err }
59-62: Add documentation for queryAcc helper.Consider adding a doc comment explaining the purpose and parameters of this helper function.
+// queryAcc queries the account state using the provided request message. +// It returns the response message and any error that occurs. func (s *IntegrationTestSuite) queryAcc(ctx context.Context, req sdk.Msg, accAddr []byte) (transaction.Msg, error) { resp, err := s.app.AccountsKeeper.Query(ctx, accAddr, req) return resp, err }tests/integration/accounts/lockup/permanent_lockup_test_suite.go (2)
Line range hint
19-143: Consider enhancing test isolation and readabilityWhile the test coverage is comprehensive, consider these improvements:
- Add cleanup between test cases to ensure complete isolation
- Use more descriptive test names that follow the pattern: "Given_When_Then" or "Should_ExpectedBehavior_When_Condition"
- Consider extracting common setup code into helper methods
Example of improved test naming:
- t.Run("error - execute message, wrong sender", func(t *testing.T) { + t.Run("Should_ReturnError_When_SenderIsNotOwner", func(t *testing.T) {
Line range hint
19-143: Enhance test documentation and assertionsThe integration test effectively verifies cross-module interactions, but could benefit from:
- Adding comments explaining the test's purpose and integration points
- More explicit assertions with descriptive error messages
- Documentation of test prerequisites and assumptions
Example improvement:
+ // TestPermanentLockingAccount verifies the integration between accounts, staking, and bank modules + // for permanent locking accounts. It ensures: + // 1. Only owners can execute operations + // 2. Delegation/undelegation updates both staking and lockup state + // 3. Proper tracking of locked funds across operations func (s *IntegrationTestSuite) TestPermanentLockingAccount() {tests/integration/tx/benchmark/benchmarks_test.go (3)
Line range hint
32-84: Consider enhancing benchmark documentation.The benchmark implementation is solid and follows best practices. However, the documentation could be enhanced by:
- Adding the benchmark environment specifications (CPU, RAM)
- Including the Go version used for the benchmarks
- Documenting the expected performance thresholds
Line range hint
85-153: Consider refactoring the suite constructor for better maintainability.While the implementation is functional, consider breaking down the constructor into smaller, focused helper methods:
- Separate network setup
- Isolate transaction creation
- Group verification logic
This would improve readability and make the test suite easier to maintain.
Example refactor:
func NewTxBenchmarkSuite(tb testing.TB) *TxBenchmarkSuite { s := new(TxBenchmarkSuite) s.setupNetwork(tb) s.setupQueryClient() s.createInitialTransactions(tb) return s } func (s *TxBenchmarkSuite) setupNetwork(tb testing.TB) { // Network setup logic } func (s *TxBenchmarkSuite) setupQueryClient() { // Query client setup logic } func (s *TxBenchmarkSuite) createInitialTransactions(tb testing.TB) { // Transaction creation and verification logic }
Line range hint
158-191: Consider adding input validation.The helper function is well-implemented but could benefit from additional validation:
- Verify that the network has at least one validator
- Validate that the fee amount is positive
- Ensure the gas limit is within acceptable bounds
Example validation:
func mkTxBuilder(tb testing.TB, s *TxBenchmarkSuite) client.TxBuilder { tb.Helper() validators := s.network.GetValidators() if len(validators) == 0 { tb.Fatal("no validators in network") } val := validators[0] feeAmount := sdk.Coins{sdk.NewInt64Coin(s.cfg.BondDenom, 10)} if !feeAmount.IsValid() || feeAmount.IsZero() { tb.Fatal("invalid fee amount") } // ... rest of the implementation }tests/integration/accounts/lockup/delayed_lockup_test_suite.go (2)
Line range hint
19-177: Consider restructuring the test for better maintainability.While the test coverage is comprehensive, the test function is quite long and handles multiple scenarios. Consider breaking it down into smaller, focused test functions:
- TestDelayedLockingAccountErrors
- TestDelayedLockingAccountDelegation
- TestDelayedLockingAccountWithdrawals
Example structure:
func (s *IntegrationTestSuite) TestDelayedLockingAccountErrors() { // Setup common test state // Test error cases } func (s *IntegrationTestSuite) TestDelayedLockingAccountDelegation() { // Setup common test state // Test delegation operations } func (s *IntegrationTestSuite) TestDelayedLockingAccountWithdrawals() { // Setup common test state // Test withdrawal operations }
Line range hint
19-177: Enhance state validation in integration tests.While the test validates core functionality, consider adding more explicit state checks:
- Validate account state after each operation
- Add balance checks before and after operations
- Verify delegation constraints
Example enhancement:
// Before delegation initialState := s.queryLockupAccInfo(ctx, app, accountAddr) // After delegation finalState := s.queryLockupAccInfo(ctx, app, accountAddr) require.Equal(t, initialState.DelegatedLocking.Add(delegationAmount), finalState.DelegatedLocking)tests/integration/accounts/lockup/continous_lockup_test_suite.go (1)
Line range hint
19-196: Consider enhancing test readability and maintainabilityWhile the test structure is solid, consider these improvements:
- Extract magic numbers into named constants (e.g.,
releaseTimeInterval := time.Second * 12)- Consider using table-driven tests for similar test cases (e.g., error scenarios)
- Add comments explaining the time-based calculations for token release
Example for constants:
const ( initialBalance = 1000000 releaseInterval = 12 * time.Second // 1/5 of a minute lockupDuration = 1 * time.Minute delegationAmount = 100 )
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (15)
- .github/workflows/test.yml (0 hunks)
- tests/Makefile (0 hunks)
- tests/e2e/accounts/lockup/lockup_account_test.go (0 hunks)
- tests/integration/accounts/lockup/continous_lockup_test_suite.go (1 hunks)
- tests/integration/accounts/lockup/delayed_lockup_test_suite.go (1 hunks)
- tests/integration/accounts/lockup/lockup_account_test.go (1 hunks)
- tests/integration/accounts/lockup/periodic_lockup_test_suite.go (1 hunks)
- tests/integration/accounts/lockup/permanent_lockup_test_suite.go (1 hunks)
- tests/integration/accounts/lockup/utils.go (2 hunks)
- tests/integration/accounts/multisig/account_test.go (2 hunks)
- tests/integration/accounts/multisig/test_suite.go (4 hunks)
- tests/integration/auth/keeper/account_retriever_test.go (1 hunks)
- tests/integration/auth/keeper/keeper_bench_test.go (4 hunks)
- tests/integration/auth/keeper/module_test.go (3 hunks)
- tests/integration/tx/benchmark/benchmarks_test.go (5 hunks)
💤 Files with no reviewable changes (3)
- .github/workflows/test.yml
- tests/Makefile
- tests/e2e/accounts/lockup/lockup_account_test.go
✅ Files skipped from review due to trivial changes (1)
- tests/integration/accounts/lockup/periodic_lockup_test_suite.go
🧰 Additional context used
📓 Path-based instructions (11)
tests/integration/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/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/accounts/lockup/lockup_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/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/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/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/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"tests/integration/auth/keeper/account_retriever_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/auth/keeper/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/auth/keeper/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/tx/benchmark/benchmarks_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 (20)
tests/integration/accounts/lockup/lockup_account_test.go (1)
1-11: LGTM! Clean implementation of the integration test suite setup.The code follows Go testing conventions and properly implements the test suite initialization using testify. The naming correctly reflects the migration from e2e to integration tests.
Let's verify that the corresponding e2e test file has been removed and all references have been updated:
✅ Verification successful
Migration to integration tests verified successfully
The verification confirms:
- No e2e test directory exists in the tests folder
- No references to lockup e2e tests were found in the codebase
- The integration test suite is properly implemented with the correct structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify complete migration from e2e to integration tests # Test 1: Ensure the old e2e test file is removed echo "Checking for old e2e test file..." if rg -l "tests/e2e/accounts/lockup/lockup_account_test.go"; then echo "Warning: Old e2e test file still exists" fi # Test 2: Check for any remaining E2ETestSuite references in lockup tests echo "Checking for remaining E2ETestSuite references..." rg "E2ETestSuite" "tests/integration/accounts/lockup/"Length of output: 295
Script:
#!/bin/bash # Let's try a broader search to verify the migration # Check if the old e2e test directory exists echo "Checking if e2e test directory exists..." fd -t d "e2e" tests/ # Look for any lockup test files in e2e directory echo "Looking for lockup test files in e2e directory..." fd -t f "lockup.*test" tests/e2e # Check for any references to lockup e2e tests in the codebase echo "Checking for references to lockup e2e tests..." rg -i "lockup.*e2e" tests/ # Check the integration test suite implementation echo "Checking integration test suite implementation..." cat tests/integration/accounts/lockup/lockup_account_test.goLength of output: 822
tests/integration/auth/keeper/module_test.go (2)
1-1: LGTM: Package naming follows Go conventions.The change from
package keepertopackage keeper_testaligns with Go's testing best practices, providing better package separation for tests.
11-11: LGTM: Import alias improves code clarity.The new import alias
authTestclearly indicates the test-specific nature of the imported package and maintains good separation between test and implementation code.tests/integration/auth/keeper/account_retriever_test.go (2)
1-11: LGTM: Package and import changes align with testing best practicesThe change from
keepertokeeper_testproperly implements black-box testing, which is the recommended approach for integration tests in Go.
14-17: Verify test coverage after configuration changesWhile the configuration change is appropriate, let's ensure the test coverage remains comprehensive after migrating from e2e to integration tests.
✅ Verification successful
Test coverage is comprehensive after configuration changes
The verification shows that:
- Multiple account-related test cases exist in the auth keeper integration tests
- The configuration pattern is consistently used across integration tests
- The test setup maintains proper coverage with tests for:
- Account retrieval
- Module account initialization
- Account migration
- Backwards compatibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any e2e tests that might have been missed in the migration rg -g "**/*_test.go" "E2ETestSuite" . # Look for any remaining e2e test configurations that should be migrated rg -g "**/*.go" "test-e2e" .Length of output: 66
Script:
#!/bin/bash # Check test coverage for auth keeper tests rg -g "**/*_test.go" "func Test.*Account.*" tests/integration/auth/keeper/ # Look for any test setup patterns similar to the current one ast-grep --pattern 'cfg, err := network.DefaultConfigWithAppConfig($_)' # Check for other test configurations in auth keeper tests rg -g "**/*_test.go" "DefaultConfig" tests/integration/auth/keeper/Length of output: 1119
tests/integration/auth/keeper/keeper_bench_test.go (2)
Line range hint
1-14: LGTM! Package and import changes follow Go best practices.The package rename to
keeper_testproperly implements black-box testing, and the import alias is clear and well-structured.
23-23: Verify AppConfig migration completeness.The change to
authTest.AppConfigis correct, but let's ensure all AppConfig references have been migrated consistently.✅ Verification successful
Let me verify if the AppConfig references are consistent by checking the imports and usage patterns.
AppConfig migration is correctly implemented
The usage of
authTest.AppConfigis consistent across all auth test files, and the change in the benchmark test aligns with the module's testing patterns. All auth-related test files are using the same import and configuration approach.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining direct AppConfig references that might need migration rg -g '**/*_test.go' 'AppConfig[^.]' tests/Length of output: 2333
Script:
#!/bin/bash # Check the imports and usage patterns in auth tests rg -g '**/*_test.go' -B 3 -A 1 'authTest.AppConfig' tests/integration/auth/Length of output: 1663
tests/integration/accounts/lockup/utils.go (1)
23-30: LGTM! Verify test suite references.The renaming from E2ETestSuite to IntegrationTestSuite is consistent with the PR objectives. The structure follows testing best practices.
Let's verify that all references to this test suite have been updated:
tests/integration/accounts/multisig/test_suite.go (3)
Line range hint
33-44: LGTM! Comprehensive test setup.The setup properly initializes the test environment with necessary test accounts and error handling.
Line range hint
98-116: LGTM! Well-structured proposal management helpers.The createProposal and executeProposal functions provide clear abstractions for testing multisig functionality.
Line range hint
21-116: Verify test coverage after e2e to integration migration.The test suite has been successfully migrated while maintaining comprehensive test coverage for multisig functionality. The integration test setup properly initializes the test environment and provides necessary helpers for thorough testing.
✅ Verification successful
Test coverage successfully verified after e2e to integration migration
The verification confirms that:
- All e2e test files have been properly migrated, with no remaining e2e test files in the multisig package
- The integration tests maintain comprehensive coverage with key test cases:
TestSimpleSendProposal: Tests basic proposal functionalityTestConfigUpdate: Tests configuration management- The test infrastructure is properly set up with the test suite pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that no e2e test files remain in the multisig package fd -e go -E '*_test.go' . | rg 'e2e.*multisig' # Verify that all test cases were migrated rg -l "func.*Test.*" tests/e2e/accounts/multisig/ rg -l "func.*Test.*" tests/integration/accounts/multisig/Length of output: 331
Script:
#!/bin/bash # Check test cases in the integration test files rg "func.*Test.*" tests/integration/accounts/multisig/account_test.go -A 2 # Check if there are any remaining e2e test files in the entire repository fd -e go -E '*_test.go' . | rg 'e2e' # Look for any test cases that might have been missed in the migration rg -l "TestMultisig" .Length of output: 581
tests/integration/accounts/lockup/permanent_lockup_test_suite.go (1)
19-19: LGTM: Test suite renamed appropriatelyThe change from
E2ETestSuitetoIntegrationTestSuitealigns with the PR objective of migrating e2e tests to integration tests.tests/integration/tx/benchmark/benchmarks_test.go (2)
1-1: LGTM: Package and struct changes align with integration test migration.The package name and struct renaming changes appropriately reflect the transition from e2e to integration tests while maintaining clear semantics about the benchmark's purpose.
Also applies to: 24-29
154-156: LGTM: Clean and focused cleanup method.The Close method properly handles resource cleanup.
tests/integration/accounts/lockup/delayed_lockup_test_suite.go (1)
19-19: LGTM: Test suite rename aligns with integration testing strategy.The rename from
E2ETestSuitetoIntegrationTestSuiteappropriately reflects the migration from e2e to integration tests.tests/integration/accounts/lockup/continous_lockup_test_suite.go (2)
19-19: LGTM: Test suite successfully migrated from E2E to IntegrationThe rename from
E2ETestSuitetoIntegrationTestSuitealigns with the PR objective of migrating e2e tests to integration tests.
Line range hint
19-196: LGTM: Comprehensive test coverage maintained after migrationThe test suite maintains excellent coverage across various scenarios:
- Error handling for invalid operations
- Time-based token release verification
- Core operations (send, withdraw, delegate, undelegate)
- State validation after each operation
tests/integration/accounts/multisig/account_test.go (3)
21-22: LGTM: Test suite initialization properly renamedThe renaming from E2E to Integration test suite aligns with the PR objectives while maintaining proper Go testing conventions.
113-113: LGTM: Well-structured integration test for complex multisig configurationThe test thoroughly validates complex multisig features including member management, threshold updates, and time-based voting periods. It properly handles both successful and failed scenarios, making it a valuable integration test for verifying component interactions.
26-26: LGTM: Comprehensive integration test for multisig workflowThe test appropriately validates the complete multisig workflow including account creation, proposal management, voting, execution, and balance verification. This fits well as an integration test since it verifies the interaction between multiple components (auth, bank, accounts) in a controlled environment.
Let's verify that all e2e test references have been properly migrated:
✅ Verification successful
Based on the git history and codebase analysis, I can now provide a final response:
Migration from E2ETestSuite to IntegrationTestSuite is complete and consistent
The git history shows a clean migration where all references to
E2ETestSuitewere properly renamed toIntegrationTestSuite. The changes were part of a deliberate effort to "migrate remaining e2e tests to integration tests" as shown in the commit message. The test logic remains unchanged, and no other references toE2ETestSuiteexist in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining E2ETestSuite references in the codebase rg "E2ETestSuite" --type goLength of output: 27
Script:
#!/bin/bash # Let's try a broader search to understand the test suite structure rg "type.*TestSuite struct" --type go -A 3Length of output: 17554
Script:
#!/bin/bash # Search for test files in the accounts/multisig directory fd ".*test.*\.go$" tests/integration/accounts/multisig/Length of output: 159
Script:
#!/bin/bash # Check git history for the specific file to see the migration git log -p tests/integration/accounts/multisig/account_test.goLength of output: 9443
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (19)
tests/integration/auth/keeper/module_test.go (1)
Line range hint
16-30: Consider enhancing test coverage with edge cases.While the test effectively validates the basic module account creation scenario, consider adding test cases for:
- Multiple module account creation
- Error scenarios when initialization fails
- Validation of account properties and permissions
Would you like me to help generate additional test cases to improve coverage?
tests/integration/auth/keeper/keeper_bench_test.go (1)
Line range hint
45-67: LGTM: Benchmark follows consistent patternsThe benchmark maintains consistency with the previous test while effectively measuring account creation and storage performance.
Consider extracting the common address generation logic into a helper function since it's used in both benchmarks:
+func generateTestAddress(i int) sdk.AccAddress { + arr := []byte{byte((i & 0xFF0000) >> 16), byte((i & 0xFF00) >> 8), byte(i & 0xFF)} + return sdk.AccAddress(arr) +}tests/integration/accounts/lockup/utils.go (1)
Line range hint
48-73: Add documentation to utility functionsThese utility functions are well-structured but lack documentation. Consider adding GoDoc comments to describe:
- Function purpose
- Parameter descriptions
- Return value explanations
- Example usage
Example enhancement:
+// executeTx executes a transaction message for the specified account +// Parameters: +// - ctx: SDK Context +// - msg: Message to execute +// - app: SimApp instance +// - accAddr: Account address +// - sender: Sender address +// Returns error if execution fails func (s *IntegrationTestSuite) executeTx(ctx sdk.Context, msg sdk.Msg, app *simapp.SimApp, accAddr, sender []byte) error { _, err := app.AccountsKeeper.Execute(ctx, accAddr, sender, msg, nil) + if err != nil { + return fmt.Errorf("failed to execute transaction: %w", err) + } return err }tests/integration/accounts/multisig/test_suite.go (4)
Line range hint
33-44: Consider adding error handling for member generation.While the setup is comprehensive, consider handling potential errors during member generation more gracefully.
func (s *IntegrationTestSuite) SetupSuite() { s.app = setupApp(s.T()) + s.T().Helper() s.members = []sdk.AccAddress{} for i := 0; i < 10; i++ { addr := secp256k1.GenPrivKey().PubKey().Address() + if addr == nil { + s.T().Fatal("failed to generate member address") + } addrStr, err := s.app.AuthKeeper.AddressCodec().BytesToString(addr) require.NoError(s.T(), err) s.membersAddr = append(s.membersAddr, addrStr) s.members = append(s.members, sdk.AccAddress(addr)) } }
54-57: Add documentation for helper methods.The
executeTxandqueryAccmethods would benefit from godoc comments explaining their purpose and parameters.+// executeTx executes a transaction message on behalf of a sender through an account func (s *IntegrationTestSuite) executeTx(ctx context.Context, msg sdk.Msg, accAddr, sender []byte) error { _, err := s.app.AccountsKeeper.Execute(ctx, accAddr, sender, msg, nil) return err } +// queryAcc queries an account's state using the provided message func (s *IntegrationTestSuite) queryAcc(ctx context.Context, req sdk.Msg, accAddr []byte) (transaction.Msg, error) { resp, err := s.app.AccountsKeeper.Query(ctx, accAddr, req) return resp, err }Also applies to: 59-62
Line range hint
98-108: Add validation for proposal messages.The
createProposalmethod should validate that at least one message is provided.func (s *IntegrationTestSuite) createProposal(ctx context.Context, accAddr, sender []byte, msgs ...*codectypes.Any) { + if len(msgs) == 0 { + s.T().Fatal("at least one message is required for proposal creation") + } propReq := &v1.MsgCreateProposal{ Proposal: &v1.Proposal{ Title: "test", Summary: "test", Messages: msgs, }, } err := s.executeTx(ctx, propReq, accAddr, sender) s.NoError(err) }
Line range hint
110-116: Consider adding proposal existence check.The
executeProposalmethod could benefit from a validation step to check if the proposal exists before execution.func (s *IntegrationTestSuite) executeProposal(ctx context.Context, accAddr, sender []byte, proposalID uint64) error { + // Query proposal first to ensure it exists + queryReq := &v1.MsgQueryProposal{ProposalId: proposalID} + _, err := s.queryAcc(ctx, queryReq, accAddr) + if err != nil { + return fmt.Errorf("proposal %d does not exist: %w", proposalID, err) + } execReq := &v1.MsgExecuteProposal{ ProposalId: proposalID, } return s.executeTx(ctx, execReq, accAddr, sender) }tests/integration/accounts/lockup/permanent_lockup_test_suite.go (2)
Line range hint
47-57: Consider enhancing error assertions with descriptive messagesThe error assertions could be more descriptive to aid debugging. Consider adding custom error messages:
-require.NotNil(t, err) +require.Error(t, err, "should fail when sender is not the owner")
Line range hint
22-26: Consider enhancing test setup with constants and explicit setup/teardownThe test setup could be improved by:
- Defining constants for magic numbers (e.g., initial balances)
- Adding explicit setup/teardown methods for each subtest
- Using test fixtures for common test data
This would make the tests more maintainable and easier to understand.
Example improvement:
const ( initialBalance = 1000000 testAmount = 100 ) func (s *IntegrationTestSuite) setupTestState(t *testing.T) (*app.App, sdk.Context) { app := setupApp(t) ctx := sdk.NewContext(app.CommitMultiStore(), false, app.Logger()). WithHeaderInfo(header.Info{Time: time.Now()}) // ... rest of setup return app, ctx }tests/integration/tx/benchmark/benchmarks_test.go (3)
Line range hint
32-84: Consider enhancing benchmark results documentation.While the benchmark comparison is helpful, consider adding more context:
- Hardware specifications for both benchmarks
- Key performance factors that led to the improvement
- Methodology used for comparison
Line range hint
85-153: Consider improving test isolation.The constructor is well-implemented with proper error handling and verification. However, consider extracting the test transaction creation into separate helper functions to improve readability and maintainability.
+ func createTestTx(val network.ValidatorI, cfg network.Config, opts cli.TestTxConfig) ([]byte, error) { + msgSend := &banktypes.MsgSend{ + FromAddress: val.GetAddress().String(), + ToAddress: val.GetAddress().String(), + Amount: sdk.NewCoins(sdk.NewCoin(cfg.BondDenom, sdkmath.NewInt(10))), + } + return cli.SubmitTestTx(val.GetClientCtx(), msgSend, val.GetAddress(), opts) + }
Line range hint
158-192: Consider defining test constants.The helper function is well-implemented. Consider extracting magic numbers into named constants for better maintainability:
+ const ( + testFeeAmount = 10 + testSendAmount = 10 + testMemo = "foobar" + )tests/integration/accounts/lockup/delayed_lockup_test_suite.go (1)
Line range hint
20-32: Consider enhancing test setup documentation.While the test setup is comprehensive, consider adding a comment block describing the test's purpose and the delayed locking account's expected behavior. This would help maintainers understand the test scenarios better.
Add a comment block like this before the test function:
+// TestDelayedLockingAccount verifies the behavior of delayed locking accounts: +// - Tests account initialization with a time-based lock +// - Validates transaction restrictions during the lock period +// - Confirms proper behavior after lock expiration func (s *IntegrationTestSuite) TestDelayedLockingAccount() {tests/integration/accounts/lockup/continous_lockup_test_suite.go (2)
Line range hint
19-191: Consider adding edge case test scenarios.While the test coverage is comprehensive, consider adding these scenarios to strengthen the test suite:
- Maximum token amount validations
- Multiple concurrent operations (e.g., simultaneous delegate and send)
- Edge cases around time boundaries (e.g., exactly at end time)
Line range hint
19-191: Consider refactoring for improved maintainability.While the test structure is good, consider these improvements:
- Extract the large test function into smaller, focused test functions for each operation type (send, withdraw, delegate).
- Replace magic numbers with named constants:
- time.Second * 12 // 12 sec = 1/5 of a minute + const releaseTimeInterval = 12 * time.Second // 1/5 of a minutetests/integration/accounts/multisig/account_test.go (2)
Line range hint
26-112: Enhance test documentation while approving the migration.The test comprehensively covers the multisig workflow and properly follows integration testing patterns by verifying state changes and events. Consider enhancing the test documentation to:
- Document the test's prerequisites
- Outline the specific integration points being tested
- Explain the expected state changes and event emissions
Example documentation improvement:
// TestSimpleSendProposal verifies the integration between multisig account and bank module by: // Prerequisites: // - A multisig account with 1 member // - Initial balance setup // Test flow: // 1. Creates and submits a bank send proposal // 2. Verifies proposal voting mechanism // 3. Ensures proper execution and balance updates // 4. Validates event emissions and proposal status
Line range hint
113-251: Consider splitting the test for better isolation while approving the migration.The test effectively verifies the integration between multisig account and configuration management. However, it combines multiple test scenarios that could be isolated for better maintainability:
- Member addition scenario
- Threshold update scenario
- Failed member removal scenario
Consider splitting into focused test cases:
func (s *IntegrationTestSuite) TestConfigUpdate_MemberAddition() { // Test adding new members } func (s *IntegrationTestSuite) TestConfigUpdate_ThresholdChange() { // Test threshold updates } func (s *IntegrationTestSuite) TestConfigUpdate_FailedMemberRemoval() { // Test failed member removal }This separation would:
- Improve test failure isolation
- Make the test scenarios more discoverable
- Simplify maintenance and updates
tests/integration/accounts/lockup/periodic_lockup_test_suite.go (2)
Line range hint
64-69: Consider using more descriptive test case names.Test case names like "error - execute message, wrong sender" could be more specific. Consider following the pattern:
TestCase_Scenario_ExpectedOutcomeExample:
-t.Run("error - execute message, wrong sender", func(t *testing.T) { +t.Run("TestSend_UnauthorizedSender_ReturnsError", func(t *testing.T) {Also applies to: 89-94
Line range hint
122-122: Fix typo in test case name.-t.Run("oke - execute withdraw message", func(t *testing.T) { +t.Run("ok - execute withdraw message", func(t *testing.T) {
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (16)
- .github/workflows/test.yml (1 hunks)
- scripts/build/testing.mk (1 hunks)
- tests/Makefile (0 hunks)
- tests/e2e/accounts/lockup/lockup_account_test.go (0 hunks)
- tests/integration/accounts/lockup/continous_lockup_test_suite.go (1 hunks)
- tests/integration/accounts/lockup/delayed_lockup_test_suite.go (1 hunks)
- tests/integration/accounts/lockup/lockup_account_test.go (1 hunks)
- tests/integration/accounts/lockup/periodic_lockup_test_suite.go (1 hunks)
- tests/integration/accounts/lockup/permanent_lockup_test_suite.go (1 hunks)
- tests/integration/accounts/lockup/utils.go (2 hunks)
- tests/integration/accounts/multisig/account_test.go (2 hunks)
- tests/integration/accounts/multisig/test_suite.go (4 hunks)
- tests/integration/auth/keeper/account_retriever_test.go (1 hunks)
- tests/integration/auth/keeper/keeper_bench_test.go (4 hunks)
- tests/integration/auth/keeper/module_test.go (3 hunks)
- tests/integration/tx/benchmark/benchmarks_test.go (5 hunks)
💤 Files with no reviewable changes (2)
- tests/Makefile
- tests/e2e/accounts/lockup/lockup_account_test.go
🧰 Additional context used
📓 Path-based instructions (12)
tests/integration/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/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/accounts/lockup/lockup_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/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/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/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/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/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"tests/integration/auth/keeper/account_retriever_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/auth/keeper/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/auth/keeper/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/tx/benchmark/benchmarks_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 (26)
tests/integration/accounts/lockup/lockup_account_test.go (2)
1-11: LGTM! The test suite setup looks good.The implementation follows Go testing conventions and properly integrates with the testify/suite framework.
9-11: Verify test coverage after e2e migration.Let's ensure all test cases from the original e2e suite are covered in the integration tests.
tests/integration/auth/keeper/module_test.go (2)
1-1: LGTM! Package naming follows Go testing conventions.The change from
package keepertopackage keeper_testfollows Go testing best practices by using a separate package for black-box testing.
11-11: Verify the new import path across the codebase.The addition of the authTest import aligns with the migration from e2e to integration tests.
✅ Verification successful
Let me verify the import path usage more thoroughly by checking the actual implementation and import statements.
Let me check the specific usage of the import to ensure it's properly aligned across the codebase.
Import path verification completed successfully
The authTest import path
github.com/cosmos/cosmos-sdk/tests/integration/auth/keeperis consistently used across the integration test files where needed:
- tests/integration/auth/keeper/keeper_bench_test.go
- tests/integration/auth/keeper/account_retriever_test.go
- tests/integration/auth/keeper/module_test.go
The migration from e2e to integration tests is properly reflected in the import paths, with only documentation and legacy references remaining in non-test files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistency of authTest import usage across integration tests # and ensure no remaining e2e test references exist # Check for any remaining e2e test references echo "Checking for remaining e2e test references..." rg -l "e2e.*test" tests/ # Verify consistent usage of authTest import echo "Verifying authTest import usage..." rg -l "authTest.*AppConfig" tests/integration/auth/Length of output: 412
Script:
#!/bin/bash # Check the actual import statements and usage in the files echo "Checking import statements in the auth integration test files:" rg -A 2 "^import \(" tests/integration/auth/keeper/ # Check for any potential old import paths echo "Checking for any old import paths:" rg -l "cosmos-sdk/testutil" tests/integration/auth/Length of output: 2224
Script:
#!/bin/bash # Check the specific authTest import usage and its context echo "Checking authTest import usage and context:" rg -B 2 -A 2 'authTest "github.com/cosmos/cosmos-sdk/tests/integration/auth/keeper"' tests/ # Check if there are any remaining old test paths echo "Checking for old test paths:" rg -l "tests/e2e" .Length of output: 1722
tests/integration/auth/keeper/account_retriever_test.go (3)
1-1: LGTM: Package name change follows Go testing conventionsThe change to
keeper_testpackage name is appropriate for black-box testing, ensuring tests are written from a consumer's perspective.
8-8: LGTM: Clear import alias for AppConfig accessThe alias import provides clear access to AppConfig while avoiding naming conflicts.
14-14: LGTM: Consistent AppConfig usage with package changesThe change to use
authTest.AppConfigaligns with the package restructuring while maintaining the test's functionality.tests/integration/auth/keeper/keeper_bench_test.go (2)
Line range hint
1-11: LGTM: Package rename follows Go testing conventionsThe change from
keepertokeeper_testproperly implements black-box testing, which is the recommended approach for integration tests in Go.
Line range hint
17-43: LGTM: Benchmark implementation is robustThe benchmark effectively tests account retrieval performance with:
- Proper allocation reporting
- Realistic test data generation
- Clear setup and teardown phases
tests/integration/accounts/lockup/utils.go (2)
23-30: LGTM: Test suite structure properly migratedThe renaming from E2ETestSuite to IntegrationTestSuite is consistent with the PR objectives, and the structure follows testing best practices using the testify suite package.
Line range hint
1-73: Verify completeness of test utilitiesLet's ensure all necessary test utilities are available for integration testing.
✅ Verification successful
Let me gather more information about the test utilities usage to ensure completeness.
Test utilities are complete and well-structured
The test utilities provide all necessary functionality for the lockup integration tests:
- Core setup/teardown methods for test suite initialization
- Transaction execution and query helpers
- Account funding utility
- Specific lockup account query helper
- Used consistently across all lockup test types (permanent, periodic, delayed, continuous)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for other test files that might need these utilities echo "Searching for test files that use these utilities..." rg -l "IntegrationTestSuite" --type go # Check for any remaining e2e test references echo -e "\nChecking for any remaining e2e test references..." rg -i "e2e.*test" --type goLength of output: 1128
Script:
#!/bin/bash # Check how these utilities are used in the lockup tests echo "Checking lockup test implementations..." rg "IntegrationTestSuite" -A 5 tests/integration/accounts/lockup/lockup_account_test.go rg "IntegrationTestSuite" -A 5 tests/integration/accounts/lockup/permanent_lockup_test_suite.go rg "IntegrationTestSuite" -A 5 tests/integration/accounts/lockup/periodic_lockup_test_suite.go rg "IntegrationTestSuite" -A 5 tests/integration/accounts/lockup/delayed_lockup_test_suite.go rg "IntegrationTestSuite" -A 5 tests/integration/accounts/lockup/continous_lockup_test_suite.go # Check for any additional utility methods in these test files echo -e "\nChecking for additional utility methods..." rg "func.*\(s \*IntegrationTestSuite\)" tests/integration/accounts/lockup/Length of output: 3155
scripts/build/testing.mk (1)
21-21: LGTM! The test-all target has been correctly updated.The removal of the e2e test dependency from the test-all target aligns with the PR objective of migrating away from e2e tests.
Let's verify that no e2e related targets remain in the Makefile:
✅ Verification successful
Since no matches were found for e2e references in Makefiles or e2e directories, I can confidently verify that the e2e test dependencies have been completely removed from the build system.
✓ Confirmed: All e2e test references have been properly removed
The verification shows that:
- No Makefiles contain any remaining
test-e2etargets or references- No e2e directories exist in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining e2e related targets or references # Expected: No matches as e2e tests should be completely removed echo "Checking for any remaining e2e references in Makefiles..." rg -l 'test-e2e' --type make echo "Checking for any remaining e2e directory references..." fd -t d 'e2e'Length of output: 286
tests/integration/accounts/multisig/test_suite.go (2)
21-26: LGTM! Struct renaming aligns with integration test migration.The renaming from
E2ETestSuitetoIntegrationTestSuitecorrectly reflects the test suite's new purpose.
46-46: Verify if cleanup is needed in TearDownSuite.The empty TearDownSuite method might need to clean up any resources or state created during tests.
✅ Verification successful
Let me check the test setup to see if there are any resources that need cleanup.
Let me check if there are any persistent resources or database connections that need cleanup.
Let me check the test helper functions and app setup to understand resource management.
Empty TearDownSuite is appropriate - no cleanup needed
Based on the code analysis, the empty TearDownSuite is correct because:
- The test suite only uses in-memory resources through simapp.Setup
- All resources (app, members, membersAddr) are scoped to the test suite and will be garbage collected
- No persistent connections, file handles, or external resources are created
- The simapp is configured with
falseparameter which means no persistence🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for resource allocation or state modifications in test methods rg -t go "s\.(app|members|membersAddr)" tests/integration/accounts/multisig/Length of output: 3999
Script:
#!/bin/bash # Check test setup and initialization rg -t go "func.*SetupTest|func.*SetupSuite" tests/integration/accounts/multisig/test_suite.go -A 10Length of output: 490
Script:
#!/bin/bash # Check for database connections, file handles, or other resources that might need cleanup rg -t go "NewKVStoreKey|NewTransientStoreKey|NewMemoryStoreKey|db\." tests/integration/accounts/multisig/Length of output: 107
Script:
#!/bin/bash # Check setupApp and other test helper functions cat tests/integration/accounts/multisig/test_suite.goLength of output: 3375
tests/integration/accounts/lockup/permanent_lockup_test_suite.go (2)
19-19: LGTM: Test suite rename aligns with migration objectivesThe rename from
E2ETestSuitetoIntegrationTestSuitecorrectly implements the migration from e2e to integration tests while preserving the test functionality.
Line range hint
19-143: Comprehensive test coverage maintained during migrationThe test suite maintains excellent coverage across critical scenarios:
- Error handling (wrong sender, insufficient funds)
- Core operations (delegation, withdrawal, undelegation)
- State validation after operations
- Balance tracking
This ensures the migration to integration tests preserves the testing rigor.
tests/integration/tx/benchmark/benchmarks_test.go (3)
Line range hint
1-23: Package name change improves clarity.The package name change from
tx_testtobenchmark_testbetter reflects the purpose of these tests as benchmarks.
24-31: Struct rename aligns with integration test migration.The rename from
E2EBenchmarkSuitetoTxBenchmarkSuitemaintains the same functionality while properly reflecting its role in the integration test suite.
154-157: LGTM!The Close method properly handles cleanup of network resources.
tests/integration/accounts/lockup/delayed_lockup_test_suite.go (2)
19-19: LGTM: Test suite migration completed correctly.The test suite has been properly renamed from
E2ETestSuitetoIntegrationTestSuite, aligning with the PR objective of migrating e2e tests to integration tests.
Line range hint
34-180: Verify test coverage completeness after migration.The test cases are well-structured and cover essential scenarios. However, as this is part of a migration from e2e to integration tests, we should verify that no critical test scenarios were lost in the transition.
Let's check the git history to ensure all test cases were preserved:
Consider adding edge case tests.
While the current test cases cover the main functionality, consider adding tests for these edge cases:
- Attempting operations exactly at the endTime
- Testing with maximum token amounts
- Testing with minimum token amounts
Would you like me to provide example implementations for these additional test cases?
✅ Verification successful
Test coverage is comprehensive for delayed locking account functionality
Based on the analysis of both the implementation and test cases:
The current test suite covers all critical functionality of DelayedLockingAccount:
- Locking period validation (before/after EndTime)
- Token operations (send, delegate, undelegate)
- Withdrawal mechanisms
- Error cases for wrong sender and insufficient funds
- State tracking for delegated and locked tokens
The test cases properly verify the core struct fields:
- Owner permissions
- EndTime functionality
- OriginalLocking, DelegatedFree, DelegatedLocking tracking
- WithdrawedCoins accounting
The migration from e2e to integration tests has preserved all essential test scenarios, with the test structure remaining intact and only the test suite name being updated from E2ETestSuite to IntegrationTestSuite.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check git history for any removed test cases during migration # Look for removed test cases in the file's history git log -p tests/integration/accounts/lockup/delayed_lockup_test_suite.go | grep -B 5 -A 5 "^-.*func.*Test"Length of output: 661
Script:
#!/bin/bash # Let's try a different approach to find test cases # First, let's check if there are any e2e test files that might contain the original tests fd -e go -e ts -e js -e py "lockup.*test" -t f # Then let's search for test function definitions in the current integration test file rg "func.*Test.*\(" tests/integration/accounts/lockup/delayed_lockup_test_suite.go # Also search for t.Run patterns to see all test cases rg "t\.Run\(" tests/integration/accounts/lockup/delayed_lockup_test_suite.goLength of output: 1174
Script:
#!/bin/bash # Let's check the test cases in the original e2e tests if they exist in git history git log --all -p | rg -A 10 -B 10 "func.*TestDelayedLockingAccount" # Also let's check the test coverage by looking at the implementation rg "type DelayedLockingAccount struct" -A 10Length of output: 7143
Script:
#!/bin/bash # Let's check the implementation of DelayedLockingAccount to ensure test coverage ast-grep --pattern 'type DelayedLockingAccount struct { $$$ }' # And search for its methods ast-grep --pattern 'func ($VAR *DelayedLockingAccount) $FUNC($$$) { $$$ }'Length of output: 367
Script:
#!/bin/bash # Let's check the BaseLockup implementation since DelayedLockingAccount embeds it ast-grep --pattern 'type BaseLockup struct { $$$ }' # And check its methods to ensure test coverage ast-grep --pattern 'func ($VAR *BaseLockup) $FUNC($$$) { $$$ }' # Also check for any specific validation or locking logic rg "func.*Validate|func.*Lock|func.*Unlock" x/accounts/defaults/lockup/Length of output: 983
tests/integration/accounts/lockup/continous_lockup_test_suite.go (1)
19-19: LGTM: Test suite rename aligns with migration objectives.The rename from
E2ETestSuitetoIntegrationTestSuitecorrectly implements the migration from e2e to integration tests while preserving the test functionality.tests/integration/accounts/multisig/account_test.go (1)
21-22: LGTM: Test suite initialization properly renamed.The renaming from
TestE2ETestSuitetoTestIntegrationTestSuitealigns with the PR objective of migrating e2e tests to integration tests.tests/integration/accounts/lockup/periodic_lockup_test_suite.go (2)
19-19: LGTM: Test suite rename aligns with migration objectives.The rename from
E2ETestSuitetoIntegrationTestSuitecorrectly reflects the migration from e2e to integration tests.
Line range hint
19-241: LGTM: Successful migration from e2e to integration test.The test maintains comprehensive coverage while properly transitioning to an integration test:
- Preserves all test scenarios and assertions
- Maintains proper time-based testing
- Includes both positive and negative test cases
- Validates state changes appropriately
.github/workflows/test.yml (1)
158-158: LGTM: Successfully removed e2e test dependency.The removal of
test-e2efrom theneedsarray aligns with the PR objectives of migrating e2e tests to integration tests. This change streamlines the testing workflow while maintaining test coverage through integration tests.
| func (s *IntegrationTestSuite) SetupSuite() { | ||
| s.T().Log("setting up integration test suite") | ||
| s.app = setupApp(s.T()) | ||
| } | ||
|
|
||
| func (s *E2ETestSuite) TearDownSuite() { | ||
| s.T().Log("tearing down e2e test suite") | ||
| func (s *IntegrationTestSuite) TearDownSuite() { | ||
| s.T().Log("tearing down integration test suite") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing setup and teardown methods
While the basic structure is in place, consider these improvements:
- Add validation checks in SetupSuite to ensure the app is properly initialized
- Implement proper cleanup in TearDownSuite to prevent potential test interference
- Add logging of important test configuration parameters
Example enhancement:
func (s *IntegrationTestSuite) SetupSuite() {
s.T().Log("setting up integration test suite")
s.app = setupApp(s.T())
+ // Validate app initialization
+ require.NotNil(s.T(), s.app.AccountsKeeper)
+ require.NotNil(s.T(), s.app.BankKeeper)
+ s.T().Log("app initialized with account keeper and bank keeper")
}
func (s *IntegrationTestSuite) TearDownSuite() {
s.T().Log("tearing down integration test suite")
+ // Clean up any remaining state
+ s.app = nil
}Committable suggestion was skipped due to low confidence.
| func NewIntegrationTestSuite() *IntegrationTestSuite { | ||
| return &IntegrationTestSuite{} | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider initializing fields in the constructor.
The constructor could be enhanced to initialize the members slice and potentially the app instance, making the test suite setup more explicit.
func NewIntegrationTestSuite() *IntegrationTestSuite {
- return &IntegrationTestSuite{}
+ return &IntegrationTestSuite{
+ members: make([]sdk.AccAddress, 0),
+ membersAddr: make([]string, 0),
+ }
}📝 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 NewIntegrationTestSuite() *IntegrationTestSuite { | |
| return &IntegrationTestSuite{} | |
| } | |
| func NewIntegrationTestSuite() *IntegrationTestSuite { | |
| return &IntegrationTestSuite{ | |
| members: make([]sdk.AccAddress, 0), | |
| membersAddr: make([]string, 0), | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (18)
tests/integration/auth/keeper/keeper_bench_test.go (1)
Line range hint
45-53: LGTM! Benchmark suite maintains comprehensive coverage.The benchmark pair effectively tests both account retrieval and setting operations, maintaining good coverage after the migration to integration tests.
Consider adding a benchmark for account deletion to complete the CRUD operation coverage.
tests/integration/accounts/lockup/utils.go (1)
33-39: Consider enhancing setup/teardown loggingWhile the basic setup and teardown are correct, consider adding more detailed logging to help with test debugging.
- s.T().Log("setting up integration test suite") + s.T().Log("setting up integration test suite for lockup account tests") - s.T().Log("tearing down integration test suite") + s.T().Log("tearing down integration test suite for lockup account tests")tests/integration/accounts/multisig/test_suite.go (3)
Line range hint
33-46: Consider implementing cleanup in TearDownSuite.While the setup is comprehensive, the empty TearDownSuite method might miss opportunities for proper test cleanup. Consider adding cleanup logic to ensure test isolation.
Example implementation:
-func (s *IntegrationTestSuite) TearDownSuite() {} +func (s *IntegrationTestSuite) TearDownSuite() { + // Reset any modified state + s.app = nil + s.members = nil + s.membersAddr = nil +}
Line range hint
54-116: Enhance method documentation for better test maintainability.While the helper methods are well-structured, they would benefit from more detailed documentation explaining their testing purpose and expected outcomes.
Example improvements:
-func (s *IntegrationTestSuite) executeTx(ctx context.Context, msg sdk.Msg, accAddr, sender []byte) error { +// executeTx executes a transaction message for the given account and sender +// Returns error if the execution fails +// Used in integration tests to verify transaction handling in multisig accounts +func (s *IntegrationTestSuite) executeTx(ctx context.Context, msg sdk.Msg, accAddr, sender []byte) error {Similar documentation improvements should be added to other helper methods.
Line range hint
21-116: Consider adding integration test scenarios for edge cases.While the test suite structure is solid, consider adding test cases for:
- Threshold boundary conditions in multisig operations
- Concurrent proposal creation and execution
- Error scenarios in message execution
This would ensure comprehensive coverage during the e2e to integration test migration.
tests/integration/accounts/lockup/permanent_lockup_test_suite.go (2)
Line range hint
19-143: Consider adding edge cases to strengthen test coverageWhile the test suite provides good coverage of core functionality, consider adding these scenarios:
- Attempting to delegate more tokens than available
- Multiple delegations to different validators
- Concurrent undelegation and delegation operations
- Zero amount operations
Would you like me to help implement these additional test cases?
Line range hint
21-24: Enhance test isolation and time handlingConsider these improvements:
- Reset the app state between test runs to ensure complete isolation
- Use a deterministic time source for better test reproducibility
Here's a suggested improvement for the context setup:
- currentTime := time.Now() + currentTime := time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC) ctx := sdk.NewContext(app.CommitMultiStore(), false, app.Logger()).WithHeaderInfo(header.Info{ Time: currentTime, })tests/integration/tx/benchmark/benchmarks_test.go (3)
Line range hint
32-83: Consider enhancing event validation documentation.While the code correctly validates the 10 expected events, the inline comment could be more structured for better maintainability.
Consider restructuring the events comment as follows:
-// The 10 events are: -// - Sending Fee to the pool (3 events): coin_spent, coin_received, transfer -// - tx.* events (3 events): tx.fee, tx.acc_seq, tx.signature -// - Sending Amount to recipient (3 events): coin_spent, coin_received, transfer and message.sender=<val1> -// - Msg events (1 event): message.module=bank, message.action=/cosmos.bank.v1beta1.MsgSend and message.sender=<val1> (all in one event) +// Expected events (total: 10): +// 1. Fee pool events (3): +// - coin_spent +// - coin_received +// - transfer +// 2. Transaction events (3): +// - tx.fee +// - tx.acc_seq +// - tx.signature +// 3. Amount transfer events (3): +// - coin_spent +// - coin_received +// - transfer (with message.sender=<val1>) +// 4. Message events (1): +// - Combined event containing: +// * message.module=bank +// * message.action=/cosmos.bank.v1beta1.MsgSend +// * message.sender=<val1>
Line range hint
85-152: Consider extracting magic numbers into named constants.The function contains several magic numbers that could be made more maintainable by extracting them into named constants.
Consider applying this refactor:
+const ( + defaultNumValidators = 1 + defaultMsgSendAmount = 10 + defaultMsgSendAmount1 = 1 + defaultTxSequence = 2 +) func NewTxBenchmarkSuite(tb testing.TB) *TxBenchmarkSuite { tb.Helper() s := new(TxBenchmarkSuite) cfg := network.DefaultConfig(simapp.NewTestNetworkFixture) - cfg.NumValidators = 1 + cfg.NumValidators = defaultNumValidators s.cfg = cfg // ... rest of the code ... msgSend := &banktypes.MsgSend{ FromAddress: val.GetAddress().String(), ToAddress: val.GetAddress().String(), - Amount: sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdkmath.NewInt(10))), + Amount: sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdkmath.NewInt(defaultMsgSendAmount))), } // ... rest of the code ... msgSend1 := &banktypes.MsgSend{ FromAddress: val.GetAddress().String(), ToAddress: val.GetAddress().String(), - Amount: sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdkmath.NewInt(1))), + Amount: sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdkmath.NewInt(defaultMsgSendAmount1))), }
Line range hint
158-193: Consider extracting magic numbers into named constants.Similar to NewTxBenchmarkSuite, this function contains magic numbers that could be made more maintainable.
Consider applying this refactor:
+const ( + defaultFeeAmount = 10 + defaultMsgAmount = 10 +) func mkTxBuilder(tb testing.TB, s *TxBenchmarkSuite) client.TxBuilder { // ... existing code ... - feeAmount := sdk.Coins{sdk.NewInt64Coin(s.cfg.BondDenom, 10)} + feeAmount := sdk.Coins{sdk.NewInt64Coin(s.cfg.BondDenom, defaultFeeAmount)} gasLimit := testdata.NewTestGasLimit() assert.NilError(tb, txBuilder.SetMsgs(&banktypes.MsgSend{ FromAddress: val.GetAddress().String(), ToAddress: val.GetAddress().String(), - Amount: sdk.Coins{sdk.NewInt64Coin(s.cfg.BondDenom, 10)}, + Amount: sdk.Coins{sdk.NewInt64Coin(s.cfg.BondDenom, defaultMsgAmount)}, }), )tests/integration/accounts/lockup/delayed_lockup_test_suite.go (2)
Line range hint
20-33: Consider enhancing test setup documentation.While the test setup is comprehensive, consider adding comments to document:
- The purpose of the delayed locking account test
- The significance of the time-based setup
- The roles of different test accounts (owner, random, withdraw)
func (s *IntegrationTestSuite) TestDelayedLockingAccount() { + // TestDelayedLockingAccount verifies the behavior of delayed locking accounts + // by testing various operations before and after the unlock time. + // It sets up: + // - An owner account with initial funds + // - A delayed locking account with 1-minute lock period + // - Additional accounts for testing withdrawals and transfers t := s.T() app := setupApp(t)
Balance checks needed for delegation operations
The test suite currently lacks balance assertions for delegation operations. Add the following balance checks:
- Before delegation: verify initial account balance
- After delegation: verify account balance is reduced by delegated amount
- After undelegation: verify account balance is restored
- After reward withdrawal: verify reward amount is added to account balance
Example locations needing balance checks:
- Line 66-77: Before and after delegation operation
- Line 108-127: Before and after undelegation operation
- Line 98-106: After reward withdrawal operation
🔗 Analysis chain
Line range hint
66-127: Consider adding balance assertions for delegation operations.While the delegation operations are well tested, consider adding balance checks after each operation to ensure:
- Account balance is reduced after delegation
- Rewards are correctly tracked
- Balance is restored after undelegation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if similar balance assertions exist in other integration tests rg -A 2 "app.BankKeeper.GetBalance.*delegate" tests/integration/Length of output: 66
Script:
#!/bin/bash # Let's try a broader search for balance checks in tests rg -A 2 "BankKeeper.*Balance" tests/integration/ # Also search for specific balance assertions rg -A 2 "Balance.*Equal" tests/integration/ # Check for any GetBalance usage rg -A 2 "GetBalance" tests/integration/Length of output: 26956
Script:
#!/bin/bash # Let's check for any balance checks in staking-related tests rg -A 2 "BankKeeper.*Balance.*delegate" tests/integration/staking/ # Let's also check for any balance assertions in the test file under review rg -A 2 "BankKeeper.*Balance" tests/integration/accounts/lockup/delayed_lockup_test_suite.goLength of output: 418
tests/integration/accounts/lockup/continous_lockup_test_suite.go (2)
Line range hint
19-191: Consider adding edge cases to strengthen test coverageWhile the test suite provides good coverage of the main functionality, consider adding these edge cases:
- Attempt operations exactly at the boundary conditions (start_time and end_time)
- Test with maximum possible lockup duration
- Test with minimum possible amounts
- Test concurrent operations near unlock times
Would you like me to help implement these additional test cases?
Line range hint
44-85: Enhance test maintainability with more descriptive names and consistent error checksConsider these improvements to align with integration testing best practices:
- Use more descriptive test case names that include the expected behavior:
-"error - execute message, wrong sender" +"error - should reject transaction when sender is not the owner"
- Add explicit error message assertions:
-require.NotNil(t, err) +require.ErrorContains(t, err, "expected error message")tests/integration/accounts/multisig/account_test.go (2)
Line range hint
26-111: Consider adding more test cases for edge scenariosWhile the test comprehensively covers the happy path for proposal creation, voting, and execution, consider adding test cases for:
- Invalid vote submissions
- Proposal expiration scenarios
- Voting period boundaries
This would enhance the test coverage and ensure robust handling of edge cases.
Line range hint
113-252: Consider refactoring to table-driven testsThe test thoroughly covers configuration updates and voting scenarios. Consider refactoring to use table-driven tests for different threshold and member configurations to make the test more maintainable and comprehensive.
Example structure:
testCases := []struct { name string initialMembers map[string]uint64 updateMembers []*v1.Member threshold int64 expectedResult v1.ProposalStatus expectedError string }{ // test cases here }This would make it easier to add new test scenarios and maintain the test suite.
tests/integration/accounts/lockup/periodic_lockup_test_suite.go (2)
Line range hint
19-196: Consider enhancing integration test isolation.While the test cases are well-structured, consider improving test isolation by:
- Resetting the app state between major test scenarios
- Using test-specific accounts for each test case
- Adding helper functions for common setup operations
This would make the tests more maintainable and prevent potential state leakage between test cases.
Example helper function:
func (s *IntegrationTestSuite) setupTestAccount(t *testing.T, app *App, ctx sdk.Context, initialBalance math.Int) (sdk.AccAddress, string) { acc := sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()) s.fundAccount(app, ctx, acc, sdk.Coins{sdk.NewCoin("stake", initialBalance)}) accStr, err := app.AuthKeeper.AddressCodec().BytesToString(acc) require.NoError(t, err) return acc, accStr }
Line range hint
19-196: Add integration test documentation.Since this is part of a larger migration from E2E to integration tests, consider adding documentation that explains:
- The scope of these integration tests
- Key differences from the previous E2E tests
- Test environment setup requirements
Add the following documentation:
// TestPeriodicLockingAccount verifies the periodic locking account functionality through integration tests. // It covers: // - Account initialization with periodic locking periods // - Token sending restrictions based on locking periods // - Withdrawal functionality after unlock periods // - Delegation and undelegation operations // - Reward withdrawal // // Unlike E2E tests, these integration tests: // - Directly interact with keeper methods // - Use in-memory app setup // - Simulate time progression without actual waiting
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (15)
- .github/workflows/test.yml (0 hunks)
- tests/Makefile (0 hunks)
- tests/e2e/accounts/lockup/lockup_account_test.go (0 hunks)
- tests/integration/accounts/lockup/continous_lockup_test_suite.go (1 hunks)
- tests/integration/accounts/lockup/delayed_lockup_test_suite.go (1 hunks)
- tests/integration/accounts/lockup/lockup_account_test.go (1 hunks)
- tests/integration/accounts/lockup/periodic_lockup_test_suite.go (1 hunks)
- tests/integration/accounts/lockup/permanent_lockup_test_suite.go (1 hunks)
- tests/integration/accounts/lockup/utils.go (2 hunks)
- tests/integration/accounts/multisig/account_test.go (2 hunks)
- tests/integration/accounts/multisig/test_suite.go (4 hunks)
- tests/integration/auth/keeper/account_retriever_test.go (1 hunks)
- tests/integration/auth/keeper/keeper_bench_test.go (4 hunks)
- tests/integration/auth/keeper/module_test.go (3 hunks)
- tests/integration/tx/benchmark/benchmarks_test.go (5 hunks)
💤 Files with no reviewable changes (3)
- .github/workflows/test.yml
- tests/Makefile
- tests/e2e/accounts/lockup/lockup_account_test.go
🧰 Additional context used
📓 Path-based instructions (12)
tests/integration/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/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/accounts/lockup/lockup_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/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/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/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/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/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"tests/integration/auth/keeper/account_retriever_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/auth/keeper/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/auth/keeper/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/tx/benchmark/benchmarks_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 (27)
tests/integration/accounts/lockup/lockup_account_test.go (3)
1-2: LGTM: Package declaration follows Go conventionsPackage name matches the directory structure and follows Go naming conventions.
3-8: LGTM: Import block is well-organizedImports are properly grouped with standard library packages separated from external packages.
9-11: LGTM: Test suite setup follows best practicesThe test suite implementation follows the standard testify/suite pattern and aligns with the PR objective of migrating from E2E to integration tests.
Let's verify the test coverage and implementation of the integration test suite:
✅ Verification successful
Let me gather more information about the test methods in the lockup test suite files.
Test suite implementation is comprehensive and well-structured
The integration test suite follows best practices with:
- A properly structured base test suite in
utils.gowith required app setup- Comprehensive test coverage across different lockup scenarios:
- Periodic lockup tests
- Permanent lockup tests
- Delayed lockup tests
- Continuous lockup tests
Each test file contains dedicated test methods that focus on specific lockup account behaviors, providing good separation of concerns and maintainability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration test suite implementation and coverage # Check for the integration test suite implementation echo "Checking for IntegrationTestSuite implementation..." ast-grep --pattern 'type IntegrationTestSuite struct { $$$ }' # Look for test methods in the suite echo "Checking for test methods in the suite..." rg -l 'func \(.*IntegrationTestSuite\) Test' tests/integration/accounts/lockup/Length of output: 3282
Script:
#!/bin/bash # Check the actual test methods in the lockup test files echo "Checking test methods in periodic lockup tests..." rg "func \(.*IntegrationTestSuite\) Test" tests/integration/accounts/lockup/periodic_lockup_test_suite.go -A 1 echo -e "\nChecking test methods in permanent lockup tests..." rg "func \(.*IntegrationTestSuite\) Test" tests/integration/accounts/lockup/permanent_lockup_test_suite.go -A 1 echo -e "\nChecking test methods in delayed lockup tests..." rg "func \(.*IntegrationTestSuite\) Test" tests/integration/accounts/lockup/delayed_lockup_test_suite.go -A 1 echo -e "\nChecking test methods in continuous lockup tests..." rg "func \(.*IntegrationTestSuite\) Test" tests/integration/accounts/lockup/continous_lockup_test_suite.go -A 1Length of output: 1208
tests/integration/auth/keeper/module_test.go (4)
1-1: LGTM: Package name change improves test isolationThe change from
package keepertopackage keeper_testfollows Go best practices by implementing black-box testing, which helps ensure the tests only use the package's public API.
11-11: LGTM: Clear import aliasThe addition of the
authTestalias provides clear namespace separation for the test configuration.
Line range hint
26-29: LGTM: Test assertions are comprehensiveThe test properly verifies the module account creation with clear assertions.
Line range hint
16-24: Verify test coverage during migrationWhile the test logic remains sound, as part of the e2e to integration test migration:
✅ Verification successful
Test coverage appears complete after e2e to integration migration
Based on the verification results:
- No e2e test files remain in the
tests/e2edirectory- No references to
E2ETestSuitewere found- The auth module has comprehensive test coverage with:
- 8 integration test files under
tests/integration/auth/- System tests in
tests/systemtests/auth_test.go- All critical functionality including module initialization, account management, and CLI operations is tested
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining e2e tests that might need migration # and verify test coverage hasn't decreased # Check for any remaining e2e test files echo "Checking for remaining e2e tests..." fd -t f . 'tests/e2e' # Look for any remaining E2ETestSuite references echo "Checking for E2ETestSuite references..." rg "E2ETestSuite" -g '*.go'Length of output: 316
Script:
#!/bin/bash # Let's verify test coverage by: # 1. Finding all test files in the auth module before this change # 2. Checking test patterns and coverage in the current state # Check all test files in auth module echo "Current test files in auth module:" fd -e go -t f . 'tests' | grep -i 'auth' # Look for test patterns to ensure comprehensive coverage echo -e "\nTest patterns in auth module:" rg "func Test" -g '*_test.go' | grep -i 'auth' # Check for any integration test setup patterns echo -e "\nIntegration test setup patterns:" rg "simtestutil.Setup" -g '*_test.go' -A 2Length of output: 18260
tests/integration/auth/keeper/account_retriever_test.go (3)
1-1: LGTM: Package name change follows Go's testing conventionsThe change to
package keeper_testproperly implements black-box testing, which is the recommended approach for integration tests in Go.
14-14: LGTM: Test configuration maintains comprehensive coverageThe modified configuration setup preserves thorough testing of account retrieval functionality, including:
- Basic account retrieval
- Height-specific queries
- Account existence verification
- Account number and sequence retrieval
This provides sufficient coverage for an integration test.
8-8: Let me check where this specific AppConfig is defined.#!/bin/bash # Find the definition of AppConfig in auth test package rg -g 'tests/integration/auth/**/*.go' -B 2 -A 2 'AppConfig.*=' # Check if there are other similar test configurations fd -e go -e go.mod -x grep -l "AppConfig" {} tests/integration/auth/tests/integration/auth/keeper/keeper_bench_test.go (2)
Line range hint
1-11: LGTM! Package changes follow Go best practices.The package rename to
keeper_testand the addition of theauthTestalias improve test isolation and maintain clear dependencies.
Line range hint
17-24: Verify integration test configuration maintains benchmark accuracy.While the switch to
authTest.AppConfigaligns with the migration to integration tests, we should ensure it provides equivalent setup for benchmarking.✅ Verification successful
Integration test configuration is appropriate for benchmarks
The
authTest.AppConfigprovides a complete setup with all necessary modules (Auth, Bank, Staking, etc.) required for account operations. The benchmark's core logic remains unchanged, and the new configuration ensures proper dependency injection for accurate performance measurements.
- The AppConfig includes all essential modules: Auth, Bank, Staking, Accounts, and supporting modules
- The benchmark was newly added as part of the e2e to integration test migration, not modified from a previous version
- The test setup maintains proper isolation and dependencies for account operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Compare the old e2e and new integration test configurations # to ensure benchmark environment remains consistent # Search for AppConfig definitions echo "Searching for AppConfig definitions:" rg -A 10 "AppConfig.*=.*depinject\.Configs" tests/Length of output: 135
Script:
#!/bin/bash # Let's try a broader search for AppConfig in test files echo "Searching for AppConfig in test files:" rg "AppConfig" tests/ # Also search for the specific test file and its imports echo -e "\nChecking the benchmark test file and its imports:" cat tests/integration/auth/keeper/keeper_bench_test.go # Look for related test setup configurations echo -e "\nSearching for related test configurations:" rg "Setup.*depinject" tests/Length of output: 5224
Script:
#!/bin/bash # Let's examine the auth test AppConfig implementation echo "Checking auth test AppConfig implementation:" cat tests/integration/auth/keeper/app_config.go # Look for previous benchmark configuration echo -e "\nSearching for previous benchmark configuration:" git log -p tests/integration/auth/keeper/keeper_bench_test.goLength of output: 3561
tests/integration/accounts/lockup/utils.go (2)
23-30: LGTM: Test suite structure properly migratedThe renaming from E2ETestSuite to IntegrationTestSuite is consistent with the PR objectives and follows Go naming conventions.
Line range hint
62-73: LGTM: Well-structured query helperThe queryLockupAccInfo method has good error handling and type assertions. The use of require statements ensures test failures are clear and informative.
tests/integration/accounts/multisig/test_suite.go (1)
21-30: LGTM! Verify test suite registration.The struct and constructor are correctly renamed from E2ETestSuite to IntegrationTestSuite. The structure maintains all necessary fields for integration testing.
Let's verify the test suite is properly registered:
✅ Verification successful
Test suite registration is properly implemented
The test suite is correctly registered in
account_test.gousingsuite.Run(t, NewIntegrationTestSuite())in theTestIntegrationTestSuitefunction, which follows Go testing conventions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the test suite is registered with suite.Run rg -l 'suite\.Run\(.*IntegrationTestSuite.*\)' tests/integration/accounts/multisig/Length of output: 137
Script:
#!/bin/bash # Let's verify the actual test suite registration in the found file rg -A 2 'suite\.Run\(.*IntegrationTestSuite.*\)' tests/integration/accounts/multisig/account_test.goLength of output: 136493
Script:
#!/bin/bash # Let's check the content of account_test.go to verify the test suite registration cat tests/integration/accounts/multisig/account_test.goLength of output: 7291
tests/integration/accounts/lockup/permanent_lockup_test_suite.go (2)
19-19: LGTM: Test suite successfully migrated to integration testsThe renaming from E2ETestSuite to IntegrationTestSuite aligns with the PR objectives of migrating remaining e2e tests to integration tests.
Line range hint
19-143: Verify test coverage metricsLet's ensure the test coverage hasn't decreased after migration from e2e to integration tests.
✅ Verification successful
Test coverage is comprehensive after migration to integration tests
The migration from e2e to integration tests maintains strong test coverage:
Integration tests cover all key operations with 13 assertions:
- Account initialization
- Message execution with wrong sender
- Insufficient funds handling
- Delegation operations
- Reward withdrawal
- Undelegation flows
- Send operations
Unit tests provide additional coverage with 9 assertions:
- Core account functionality
- Delegation/undelegation state tracking
- Coin sending restrictions
The new integration test suite actually provides more comprehensive coverage by testing the full interaction flow with other modules (StakingKeeper, BankKeeper) while maintaining all the critical test scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Compare test coverage between the base branch and current changes # Get the coverage for the permanent lockup account implementation go test -coverprofile=coverage.out ./x/accounts/defaults/lockup/... # Display the coverage report go tool cover -func=coverage.out | grep -A 1 "permanent_lockup"Length of output: 273
Script:
#!/bin/bash # Let's try a different approach to analyze test coverage by examining the test file structure and assertions # First, find all test files related to permanent lockup fd -e go -E 'vendor/*' permanent.*test # Then examine the test implementations to understand coverage rg -l "func.*Test.*Permanent" # Look for test assertions in permanent lockup tests rg "require\.(No)?Error|assert\." -A 1 -B 1 "permanent.*test"Length of output: 446
Script:
#!/bin/bash # Let's examine both test files to analyze test coverage # Check the content of both test files echo "=== Integration Test Content ===" cat tests/integration/accounts/lockup/permanent_lockup_test_suite.go echo -e "\n=== Unit Test Content ===" cat x/accounts/defaults/lockup/permanent_locking_account_test.go # Count assertions in both files echo -e "\n=== Assertion Count in Integration Tests ===" rg "require\.(No)?Error|assert\." tests/integration/accounts/lockup/permanent_lockup_test_suite.go | wc -l echo -e "\n=== Assertion Count in Unit Tests ===" rg "require\.(No)?Error|assert\." x/accounts/defaults/lockup/permanent_locking_account_test.go | wc -lLength of output: 7919
tests/integration/tx/benchmark/benchmarks_test.go (3)
Line range hint
1-22: LGTM: Package name change aligns with the benchmark focus.The package rename from
tx_testtobenchmark_testbetter reflects the specific purpose of these tests.
24-30: LGTM: Struct rename aligns with integration test migration.The rename from
E2EBenchmarkSuitetoTxBenchmarkSuiteappropriately reflects its specific focus on transaction benchmarks and aligns with the migration from e2e to integration tests.
154-156: LGTM: Clean and straightforward cleanup method.tests/integration/accounts/lockup/delayed_lockup_test_suite.go (3)
19-19: LGTM: Test suite successfully migrated from E2E to Integration.The test suite has been correctly renamed from
E2ETestSuitetoIntegrationTestSuite, aligning with the PR objective of migrating e2e tests to integration tests.
Line range hint
35-65: LGTM: Comprehensive error case coverage.The error test cases effectively validate:
- Unauthorized sender attempts
- Insufficient funds scenarios
- Invalid withdrawal attempts
This provides good coverage of negative scenarios.
Line range hint
129-171: LGTM: Post-unlock scenarios thoroughly tested.The test cases effectively verify:
- Token sendability after unlock period
- Withdrawal functionality with correct balance calculations
- Time-based unlock mechanism
tests/integration/accounts/lockup/continous_lockup_test_suite.go (1)
19-19: LGTM: Successful migration from E2E to Integration test suiteThe rename from
E2ETestSuitetoIntegrationTestSuitealigns with the PR objectives of migrating remaining e2e tests to integration tests.tests/integration/accounts/multisig/account_test.go (1)
21-22: LGTM: Test suite initialization properly renamedThe renaming from E2E to Integration test suite aligns with the PR objectives while maintaining the correct test setup pattern.
tests/integration/accounts/lockup/periodic_lockup_test_suite.go (2)
19-19: Successfully migrated from E2E to Integration test suite.The test suite has been properly migrated while maintaining comprehensive coverage of the periodic locking account functionality.
Line range hint
19-196: Verify test coverage compared to removed E2E tests.Let's ensure no test coverage was lost during the migration from E2E to integration tests.
✅ Verification successful
Test coverage appears comprehensive with expanded integration test suite
The test coverage has been maintained and improved through a comprehensive set of integration tests. The current implementation includes:
- Multiple specialized test suites covering different lockup scenarios:
- Periodic lockup (current file)
- Continuous lockup
- Delayed lockup
- Permanent lockup
The periodic lockup tests specifically cover:
- Error cases (wrong sender, insufficient funds, no withdrawable tokens)
- Token unlocking periods
- Account operations (send, withdraw, delegate, undelegate)
- Reward management
- Balance verification
- State transitions
The migration from E2E to integration tests has resulted in a more focused and maintainable test structure while maintaining thorough coverage of the lockup account functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Compare test coverage between old E2E tests and new integration tests # Find the old E2E test file in git history git log --all -p --full-history -- "**/e2e/**/lockup_account_test.go" > /tmp/old_test.txt # Compare test function names and assertions echo "=== Old E2E test functions ===" rg "func.*Test.*\(" /tmp/old_test.txt echo "=== New integration test assertions ===" rg "require\." tests/integration/accounts/lockup/periodic_lockup_test_suite.goLength of output: 300
Script:
#!/bin/bash # Let's try a different approach to find test coverage # First, let's check if there are any other test files related to lockup accounts fd ".*lockup.*test.*\.go$" # Then let's examine the test assertions in the current test file echo "=== Current test assertions ===" rg "t\.Run|require\." tests/integration/accounts/lockup/periodic_lockup_test_suite.go # Let's also search for any other related test files in the current branch echo "=== Related test files ===" rg -l "lockup.*account" -g "*.go" tests/Length of output: 2549
| func (s *IntegrationTestSuite) executeTx(ctx sdk.Context, msg sdk.Msg, app *simapp.SimApp, accAddr, sender []byte) error { | ||
| _, err := app.AccountsKeeper.Execute(ctx, accAddr, sender, msg, nil) | ||
| return err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in executeTx
Consider adding context to the error return to help with debugging test failures.
func (s *IntegrationTestSuite) executeTx(ctx sdk.Context, msg sdk.Msg, app *simapp.SimApp, accAddr, sender []byte) error {
_, err := app.AccountsKeeper.Execute(ctx, accAddr, sender, msg, nil)
- return err
+ if err != nil {
+ return fmt.Errorf("failed to execute transaction: %w", err)
+ }
+ return 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.
| func (s *IntegrationTestSuite) executeTx(ctx sdk.Context, msg sdk.Msg, app *simapp.SimApp, accAddr, sender []byte) error { | |
| _, err := app.AccountsKeeper.Execute(ctx, accAddr, sender, msg, nil) | |
| return err | |
| } | |
| func (s *IntegrationTestSuite) executeTx(ctx sdk.Context, msg sdk.Msg, app *simapp.SimApp, accAddr, sender []byte) error { | |
| _, err := app.AccountsKeeper.Execute(ctx, accAddr, sender, msg, nil) | |
| if err != nil { | |
| return fmt.Errorf("failed to execute transaction: %w", err) | |
| } | |
| return nil | |
| } |
| func (s *IntegrationTestSuite) queryAcc(ctx sdk.Context, req sdk.Msg, app *simapp.SimApp, accAddr []byte) (transaction.Msg, error) { | ||
| resp, err := app.AccountsKeeper.Query(ctx, accAddr, req) | ||
| return resp, err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety to queryAcc method
The method could benefit from generic type parameters to ensure type safety at compile time.
-func (s *IntegrationTestSuite) queryAcc(ctx sdk.Context, req sdk.Msg, app *simapp.SimApp, accAddr []byte) (transaction.Msg, error) {
+func (s *IntegrationTestSuite) queryAcc[T transaction.Msg](ctx sdk.Context, req sdk.Msg, app *simapp.SimApp, accAddr []byte) (T, error) {
resp, err := app.AccountsKeeper.Query(ctx, accAddr, req)
- return resp, err
+ if err != nil {
+ var zero T
+ return zero, fmt.Errorf("query failed: %w", err)
+ }
+ result, ok := resp.(T)
+ if !ok {
+ var zero T
+ return zero, fmt.Errorf("unexpected response type: got %T, want %T", resp, zero)
+ }
+ return result, 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.
| func (s *IntegrationTestSuite) queryAcc(ctx sdk.Context, req sdk.Msg, app *simapp.SimApp, accAddr []byte) (transaction.Msg, error) { | |
| resp, err := app.AccountsKeeper.Query(ctx, accAddr, req) | |
| return resp, err | |
| } | |
| func (s *IntegrationTestSuite) queryAcc[T transaction.Msg](ctx sdk.Context, req sdk.Msg, app *simapp.SimApp, accAddr []byte) (T, error) { | |
| resp, err := app.AccountsKeeper.Query(ctx, accAddr, req) | |
| if err != nil { | |
| var zero T | |
| return zero, fmt.Errorf("query failed: %w", err) | |
| } | |
| result, ok := resp.(T) | |
| if !ok { | |
| var zero T | |
| return zero, fmt.Errorf("unexpected response type: got %T, want %T", resp, zero) | |
| } | |
| return result, nil | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.github/workflows/test.yml (1)
Line range hint
196-199: Consider removing the e2e coverage artifact download step.Since e2e tests are being migrated to integration tests, we can remove this step that attempts to download e2e coverage artifacts.
- - uses: actions/download-artifact@v3 - if: env.GIT_DIFF - with: - name: "${{ github.sha }}-e2e-coverage" - continue-on-error: true
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
- .github/workflows/test.yml (1 hunks)
- scripts/build/testing.mk (1 hunks)
🔇 Additional comments (3)
scripts/build/testing.mk (2)
Line range hint
1-90: Update documentation to reflect e2e test removal.Since e2e tests are being completely removed, ensure that any testing documentation is updated to reflect this change in testing strategy.
Let's check for documentation that might need updates:
#!/bin/bash # Description: Find documentation files that might reference e2e tests # Expected: Identify files that need updating # Search for e2e test references in markdown files and docs rg -t md -t txt "e2e test" --glob '!{CHANGELOG,LICENSE}*'
21-21: LGTM! Verify no other targets depend on e2e tests.The removal of e2e tests from the
test-alltarget aligns with the PR objectives.Let's verify no other targets depend on the removed e2e tests:
✅ Verification successful
No remaining dependencies on e2e tests found
The verification shows that there are no remaining references to
test-e2etargets in any Makefiles or.mkfiles. This aligns with the ADR-059 documentation which indicates the planned migration of e2e tests to integration tests. The removal of the e2e test dependency from thetest-alltarget is safe.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any remaining references to e2e tests in make targets # Expected: No references to test-e2e in any make targets or dependencies # Search in all Makefiles fd Makefile --type f --exec rg -A 5 'test-e2e'Length of output: 864
Script:
#!/bin/bash # Let's check all Makefiles specifically for any remaining test-e2e targets or dependencies fd Makefile --type f --exec grep -l "test-e2e" {} \; | while read -r file; do echo "=== $file ===" cat "$file" | grep -A 2 -B 2 "test-e2e" done # Also check for any e2e-related make targets fd "\.mk$" --type f --exec grep -l "test-e2e" {} \; | while read -r file; do echo "=== $file ===" cat "$file" | grep -A 2 -B 2 "test-e2e" doneLength of output: 140
.github/workflows/test.yml (1)
158-158: LGTM! Removal of test-e2e dependency.The removal of
test-e2efrom theneedsarray aligns with the PR's objective of migrating away from e2e tests.
Co-authored-by: Julien Robert <[email protected]> Co-authored-by: Marko <[email protected]> (cherry picked from commit e37d71a) # Conflicts: # tests/integration/tx/benchmark/service_test.go # tests/integration/tx/benchmark/testdata/tx_amino1.bin # tests/integration/tx/benchmark/testdata/tx_amino1.json
…) (#22381) Co-authored-by: Akhil Kumar P <[email protected]> Co-authored-by: Julien Robert <[email protected]>
There was a problem hiding this comment.
looks like those was pulled in with build failures
There was a problem hiding this comment.
These tests required utils functions which were deleted earlier with this commit, which got unnoticed. Will raise a PR with fix.
Description
Closes: #22358
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
Documentation
Chores