refactor!: remove testutil/network #22392
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request involves the complete removal of the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)CHANGELOG.md (1)Pattern 🔇 Additional comments (4)CHANGELOG.md (4)Line range hint The CHANGELOG.md follows the standard format with clear sections for Features, Improvements, Bug Fixes etc. The organization makes it easy for users to understand changes between versions. Line range hint The changelog properly documents version changes following semantic versioning, with clear breaking changes sections that help users understand upgrade impacts. Line range hint The markdown formatting is consistent throughout the document, with proper heading levels, bullet points, and code blocks. This makes the document easy to read and parse. Line range hint Changes are well documented per module with clear explanations of the changes and their impacts. Breaking changes and deprecations are clearly marked. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| go func() { | ||
| fmt.Println("Press the Enter Key to terminate all validator processes") | ||
| if _, err := fmt.Scanln(); err == nil { | ||
| close(shutdownCh) // Signal shutdown | ||
| } | ||
| }() |
Check notice
Code scanning / CodeQL
Spawning a Go routine Note test
| go func() { | ||
| <-sigCh // Wait for Ctrl+C | ||
| fmt.Println("\nCtrl+C detected, terminating validator processes...") | ||
| close(shutdownCh) // Signal shutdown | ||
| }() |
Check notice
Code scanning / CodeQL
Spawning a Go routine Note test
|
If you attempt to migrate them to the integration v2 framework, does it work? |
julienrbrt
changed the title
julienrbrt
requested review from
facundomedica,
julienrbrt,
a team and
JulianToledano
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
tests/integration/auth/keeper/account_retriever_test.go (1)
7-21: Import Statements Not Ordered According togciTool.The import statements are not properly grouped and ordered according to the
gcitool guidelines. This can affect code readability and maintainability.Consider running the
gciformatting tool to automatically sort and group the imports as per the project standards.🧰 Tools
🪛 golangci-lint (1.62.2)
7-7: File is not
gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order(gci)
12-12: File is not
gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order(gci)
20-20: ST1019: package "github.com/cosmos/cosmos-sdk/x/auth/types" is being imported more than once
(stylecheck)
21-21: ST1019(related information): other import of "github.com/cosmos/cosmos-sdk/x/auth/types"
(stylecheck)
tests/integration/auth/keeper/fixture_test.go (1)
Line range hint
21-34: Import Statements Not Ordered According togciTool.The import statements are not properly grouped and ordered according to the
gcitool guidelines. This can affect code readability and maintainability.Please run the
gcitool to sort and group the imports correctly as per the project coding standards.🧰 Tools
🪛 golangci-lint (1.62.2)
21-21: File is not
gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order(gci)
tests/integration/genutil/init_test.go (1)
396-412: Improve function documentation and implementationWhile the function correctly implements the required functionality, here are some suggested improvements:
- The function comment should follow Go standards with a complete description
- The port conversion could be simplified
Consider these improvements:
-// Get a free address for a test CometBFT server -// protocol is either tcp, http, etc +// freeTCPAddr returns a free TCP address and port for testing purposes. +// It returns the full address string, port string, a function to close the listener, +// and any error encountered. func freeTCPAddr() (addr, port string, closeFn func() error, err error) { l, err := net.Listen("tcp", "127.0.0.1:0") if err != nil { return "", "", nil, err } closeFn = func() error { return l.Close() } portI := l.Addr().(*net.TCPAddr).Port - port = fmt.Sprintf("%d", portI) + port = strconv.Itoa(portI) addr = fmt.Sprintf("tcp://127.0.0.1:%s", port) return }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (19)
client/grpc/cmtservice/status_test.go(0 hunks)simapp/simd/cmd/testnet.go(14 hunks)simapp/test_helpers.go(0 hunks)simapp/testutil_network_test.go(0 hunks)tests/integration/auth/keeper/account_retriever_test.go(1 hunks)tests/integration/auth/keeper/fixture_test.go(3 hunks)tests/integration/distribution/cli_tx_test.go(0 hunks)tests/integration/genutil/init_test.go(3 hunks)tests/integration/server/grpc/out_of_gas_test.go(0 hunks)tests/integration/server/grpc/server_test.go(0 hunks)tests/integration/server/grpc_test.go(1 hunks)tests/integration/tx/benchmark/benchmarks_test.go(0 hunks)tests/systemtests/cometbft_client_test.go(1 hunks)testutil/cli/tx.go(0 hunks)testutil/network/doc.go(0 hunks)testutil/network/interface.go(0 hunks)testutil/network/network.go(0 hunks)testutil/network/util.go(0 hunks)testutil/network/validator.go(0 hunks)
💤 Files with no reviewable changes (13)
- tests/integration/distribution/cli_tx_test.go
- simapp/test_helpers.go
- testutil/network/doc.go
- testutil/network/interface.go
- testutil/network/validator.go
- client/grpc/cmtservice/status_test.go
- simapp/testutil_network_test.go
- tests/integration/tx/benchmark/benchmarks_test.go
- testutil/network/util.go
- testutil/cli/tx.go
- tests/integration/server/grpc/out_of_gas_test.go
- testutil/network/network.go
- tests/integration/server/grpc/server_test.go
🧰 Additional context used
📓 Path-based instructions (6)
tests/systemtests/cometbft_client_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/fixture_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/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/server/grpc_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/genutil/init_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"
simapp/simd/cmd/testnet.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 golangci-lint (1.62.2)
tests/integration/auth/keeper/fixture_test.go
21-21: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
34-34: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
tests/integration/auth/keeper/account_retriever_test.go
7-7: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
12-12: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
20-20: ST1019: package "github.com/cosmos/cosmos-sdk/x/auth/types" is being imported more than once
(stylecheck)
21-21: ST1019(related information): other import of "github.com/cosmos/cosmos-sdk/x/auth/types"
(stylecheck)
tests/integration/server/grpc_test.go
18-18: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
21-21: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
24-24: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
simapp/simd/cmd/testnet.go
[medium] 702-702: G204: Subprocess launched with a potential tainted input or cmd arguments
(gosec)
🔇 Additional comments (3)
tests/systemtests/cometbft_client_test.go (1)
25-34: Test TestQueryStatus Added Successfully.
The new test TestQueryStatus enhances the test coverage by verifying the node status. The implementation aligns with testing best practices.
tests/integration/genutil/init_test.go (2)
8-8: LGTM: Import addition is correct
The addition of the "net" package import is properly placed and necessary for the new TCP address functionality.
Line range hint 223-233: LGTM: Successfully migrated from network package
The test has been properly updated to use the local freeTCPAddr function while maintaining the same functionality and error handling. This change successfully removes the dependency on the testutil/network package as intended.
|
|
||
| // run start command | ||
| cmdArgs := []string{"start", fmt.Sprintf("--%s=%s", flags.FlagHome, nodeDir)} | ||
| runCmd := exec.Command(os.Args[0], cmdArgs...) // spawn new process |
There was a problem hiding this comment.
Security Concern: Potential Command Injection in exec.Command.
Using exec.Command(os.Args[0], cmdArgs...) where cmdArgs may include untrusted input can lead to command injection vulnerabilities. It's crucial to ensure that all inputs used in cmdArgs are properly sanitized and validated.
Consider validating or sanitizing cmdArgs to prevent potential security risks. Alternatively, use exec.CommandContext with a controlled set of arguments.
🧰 Tools
🪛 golangci-lint (1.62.2)
[medium] 702-702: G204: Subprocess launched with a potential tainted input or cmd arguments
(gosec)
| srvconfig "github.com/cosmos/cosmos-sdk/server/config" | ||
| servergrpc "github.com/cosmos/cosmos-sdk/server/grpc" | ||
| simtestutil "github.com/cosmos/cosmos-sdk/testutil/sims" | ||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
| "github.com/cosmos/cosmos-sdk/x/auth/keeper" | ||
| "github.com/cosmos/cosmos-sdk/x/auth/types" | ||
| authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate Import of github.com/cosmos/cosmos-sdk/x/auth/types.
The package github.com/cosmos/cosmos-sdk/x/auth/types is imported twice with different aliases (types and authtypes), which can cause confusion.
Consolidate the imports by removing one of the duplicates and using a single alias throughout the code to improve clarity.
🧰 Tools
🪛 golangci-lint (1.62.2)
20-20: ST1019: package "github.com/cosmos/cosmos-sdk/x/auth/types" is being imported more than once
(stylecheck)
21-21: ST1019(related information): other import of "github.com/cosmos/cosmos-sdk/x/auth/types"
(stylecheck)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
server/grpc/server.go (1)
23-23: Excellent interface design changeThe modification from concrete
types.Applicationto interface type follows the Interface Segregation Principle perfectly. This change:
- Reduces coupling by requiring only the necessary
RegisterGRPCServermethod- Increases flexibility for testing and implementation
- Aligns with the goal of removing testutil/network dependencies
This is a good example of API design in Go - requiring only the minimal interface needed rather than a larger concrete type. Consider applying similar patterns in other packages where concrete types are currently required.
CHANGELOG.md (2)
Line range hint
1-1: Consider adding a title header to the changelogThe file would benefit from having a clear title header like "# Cosmos SDK Changelog" at the top.
+ # Cosmos SDK Changelog
64-65: Fix inconsistent newline after bullet pointThere is an inconsistent newline after the bullet point. For consistency, remove the extra newline.
- * (testutil) [#22392](https://github.com/cosmos/cosmos-sdk/pull/22392) Remove `testutil/network` package. Use the integration framework or systemtests framework instead. -tests/integration/server/grpc_test.go (2)
202-211: Verify error handling inTestGRPCServer_BankBalance_OutOfGasThe test correctly checks for an out-of-gas error. However, consider verifying that the response
resisnilto prevent potential nil pointer dereferences.Apply this diff to enhance the test:
_, err := bankClient.Balance( context.Background(), &banktypes.QueryBalanceRequest{Address: s.address.String(), Denom: "stake"}, ) +s.Require().Nil(res) s.Require().ErrorContains(err, sdkerrors.ErrOutOfGas.Error())
265-265: Address the skipped test and associated TODOThe test
TestGRPCServer_InterfaceReflectionis currently skipped with a TODO to fix the test (#22825). Consider addressing this issue to ensure the test suite is comprehensive.Would you like assistance in generating a solution or opening a GitHub issue to track this task?
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)server/grpc/server.go(2 hunks)tests/integration/auth/keeper/account_retriever_test.go(1 hunks)tests/integration/auth/keeper/fixture_test.go(3 hunks)tests/integration/server/grpc_test.go(1 hunks)tests/systemtests/cometbft_client_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/systemtests/cometbft_client_test.go
- tests/integration/auth/keeper/account_retriever_test.go
🧰 Additional context used
📓 Path-based instructions (4)
tests/integration/auth/keeper/fixture_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"
server/grpc/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tests/integration/server/grpc_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"
CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🔇 Additional comments (10)
server/grpc/server.go (1)
8-8: LGTM: Clean import addition
The new import for gogoproto's gRPC package is properly aliased and necessary for the interface type change.
CHANGELOG.md (1)
Line range hint 1-2000: Overall documentation structure looks good
The changelog follows good practices with:
- Clear version headers and release dates
- Categorized sections (Features, Improvements, Bug Fixes etc.)
- Consistent formatting for PR references
- Detailed descriptions of changes
tests/integration/auth/keeper/fixture_test.go (4)
6-7: LGTM: Import additions are appropriate
The new imports are necessary for block finalization functionality and test assertions.
40-42: LGTM: Fixture struct additions are well-structured
The new fields provide necessary context and encoding configuration for integration tests, aligning with the migration away from testutil/network.
147-147: LGTM: Fixture initialization is complete and consistent
The initialization of new fields is properly handled, with appropriate context unwrapping and encoding configuration assignment.
Also applies to: 151-151
Line range hint 1-153: Verify test coverage after testutil/network removal
Let's ensure that the migration from testutil/network hasn't left any gaps in test coverage.
✅ Verification successful
Let me gather more specific information about test coverage.
Based on the analysis of the test files and patterns, I can now provide a final response.
Test coverage is properly maintained after testutil/network removal
The migration from testutil/network to the new integration test framework is well-covered:
- The new fixture pattern is consistently used across multiple integration test packages (auth, accounts, distribution, gov, etc.)
- The test file implements comprehensive test cases including:
- Account retrieval and management
- Module account initialization
- Account migration scenarios
- Async execution tests
- Benchmarking tests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining testutil/network usage and verify test coverage
# Check for any remaining testutil/network imports
echo "Checking for remaining testutil/network imports..."
rg -l "testutil/network" --type go
# Check for test files that might need migration
echo "Checking for potential test files that might need migration..."
fd -e go -e _test.go . | xargs rg -l "network"
# Look for potentially similar test patterns that might need updating
echo "Checking for similar test patterns..."
ast-grep --pattern 'func Test$_($_ *testing.T) {
$$$
network.$_
$$$
}'
Length of output: 1531
Script:
#!/bin/bash
# Check for test coverage in auth/keeper tests
echo "Checking auth/keeper test coverage..."
fd -e go -e _test.go . tests/integration/auth/keeper/ | xargs cat
# Check for any other integration tests that might be using the new pattern
echo "Checking for similar integration test patterns..."
ast-grep --pattern 'type fixture struct {
$$$
}'
# Look for other test files that might need similar updates
echo "Checking for potential similar test files..."
fd -e go -e _test.go . tests/integration/
Length of output: 37359
tests/integration/server/grpc_test.go (4)
3-49: Group and order imports according to 'gci' formatting rules
The imports are not properly grouped and ordered according to the project's import style guidelines enforced by the gci tool. Please run gci with the specified arguments to format the imports correctly.
179-184: Correct the function call to establish a gRPC client connection
The function grpc.NewClient does not exist in the google.golang.org/grpc package. The standard function to establish a client connection is grpc.Dial.
Apply this diff to correct the function call:
-s.conn, err = grpc.NewClient(
+s.conn, err = grpc.Dial(
grpcCfg.Address,
grpc.WithTransportCredentials(insecure.NewCredentials()),
grpc.WithDefaultCallOptions(grpc.ForceCodec(codec.NewProtoCodec(encodingCfg.InterfaceRegistry).GRPCCodec())),
)192-200: LGTM!
The test TestGRPCServer_TestService is well-implemented and follows best practices.
291-317: LGTM!
The test TestGRPCUnpacker effectively verifies the unpacking of interfaces for validators and handles cases where interfaces are not yet unpacked.
| // commit and finalize block | ||
| defer func() { | ||
| _, err := integrationApp.Commit() | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| }() | ||
| height := integrationApp.LastBlockHeight() + 1 | ||
| _, err = integrationApp.FinalizeBlock(&cmtabcitypes.FinalizeBlockRequest{Height: height, DecidedLastCommit: cmtabcitypes.CommitInfo{Votes: []cmtabcitypes.VoteInfo{{}}}}) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving error handling and cleanup in block finalization
The current implementation has several areas for improvement:
- Replace panic with test failure
- Make cleanup more explicit
- Use test assertions consistently
Consider this alternative implementation:
// commit and finalize block
defer func() {
- _, err := integrationApp.Commit()
- if err != nil {
- panic(err)
- }
+ _, err := integrationApp.Commit()
+ require.NoError(t, err, "failed to commit block")
}()
height := integrationApp.LastBlockHeight() + 1
_, err = integrationApp.FinalizeBlock(&cmtabcitypes.FinalizeBlockRequest{
Height: height,
DecidedLastCommit: cmtabcitypes.CommitInfo{Votes: []cmtabcitypes.VoteInfo{{}}},
})
require.NoError(t, err)📝 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.
| // commit and finalize block | |
| defer func() { | |
| _, err := integrationApp.Commit() | |
| if err != nil { | |
| panic(err) | |
| } | |
| }() | |
| height := integrationApp.LastBlockHeight() + 1 | |
| _, err = integrationApp.FinalizeBlock(&cmtabcitypes.FinalizeBlockRequest{Height: height, DecidedLastCommit: cmtabcitypes.CommitInfo{Votes: []cmtabcitypes.VoteInfo{{}}}}) | |
| require.NoError(t, err) | |
| // commit and finalize block | |
| defer func() { | |
| _, err := integrationApp.Commit() | |
| require.NoError(t, err, "failed to commit block") | |
| }() | |
| height := integrationApp.LastBlockHeight() + 1 | |
| _, err = integrationApp.FinalizeBlock(&cmtabcitypes.FinalizeBlockRequest{ | |
| Height: height, | |
| DecidedLastCommit: cmtabcitypes.CommitInfo{Votes: []cmtabcitypes.VoteInfo{{}}}, | |
| }) | |
| require.NoError(t, err) |
Description
Closes: #22384
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Chores
testutil/networkpackage, recommending the use of the integration framework or system tests framework instead.