test(server/v2/cometbft): Add Consensus query p2p, app, grpc
#22771
Conversation
hieuvubk
requested review from
facundomedica,
sontrinh16 and
a team
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request enhance the testing framework for consensus-related functionalities in the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
C:server/v2
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
server/v2/cometbft/abci_test.go (3)
21-22: Fix import order to comply with Go and project conventionsThe import statements are not properly grouped and ordered, as indicated by the
gcilinter. Standard library imports should be separated from third-party and project-specific imports. Please organize the imports to meet the project's import conventions.🧰 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)
38-40: Ensure imports are grouped and sorted correctlyThe imports are not grouped according to the conventions specified in the Uber Go Style Guide. Please group standard library, third-party, and project-specific imports separately and ensure they are alphabetically sorted within each group.
🧰 Tools
🪛 golangci-lint (1.62.2)
40-40: 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)
813-813: Format the code withgofumpt -extrafor consistencyThe code is not formatted according to the
gofumpt -extrastyle guidelines, as indicated by thegofumptlinter. Please format the code to improve readability and maintain consistency across the codebase.🧰 Tools
🪛 golangci-lint (1.62.2)
813-813: File is not
gofumpt-ed with-extra(gofumpt)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
server/v2/cometbft/abci_test.go(8 hunks)server/v2/cometbft/utils.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
server/v2/cometbft/utils.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/abci_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🪛 golangci-lint (1.62.2)
server/v2/cometbft/abci_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)
40-40: 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)
824-824: Error return value of queryRouterBuilder.RegisterHandler is not checked
(errcheck)
813-813: File is not gofumpt-ed with -extra
(gofumpt)
🔇 Additional comments (2)
server/v2/cometbft/abci_test.go (1)
654-709: Unit tests enhance coverage for new query functionalities
The new test functions TestConsensus_GRPCQuery, TestConsensus_P2PQuery, and TestConsensus_AppQuery appropriately test the added query functionalities, covering scenarios for valid and invalid requests, error handling, and response validation. These tests enhance the robustness of the testing suite.
Also applies to: 711-749, 750-790
server/v2/cometbft/utils.go (1)
193-197: Prevent potential nil pointer dereference in error handling
The added nil check for txRes.Error before accessing Error() ensures that there is no nil pointer dereference when constructing the error message. This change improves the robustness of the error handling logic.
| queryRouterBuilder.RegisterHandler( | ||
| proto.MessageName(&testdata.SayHelloRequest{}), | ||
| helloFooHandler, | ||
| ) |
There was a problem hiding this comment.
Check the error return value of RegisterHandler
The call to queryRouterBuilder.RegisterHandler does not check the returned error. Ignoring errors may lead to unexpected behavior or panics during runtime. Please handle the error appropriately.
Apply the following fix to handle the error:
+ err := queryRouterBuilder.RegisterHandler(
proto.MessageName(&testdata.SayHelloRequest{}),
helloFooHandler,
)
+ require.NoError(t, err)
- queryRouterBuilder.RegisterHandler(
- proto.MessageName(&testdata.SayHelloRequest{}),
- helloFooHandler,
- )Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 golangci-lint (1.62.2)
824-824: Error return value of queryRouterBuilder.RegisterHandler is not checked
(errcheck)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
server/v2/cometbft/abci_test.go (4)
21-22: Organize imports according to Go conventionsThe import statements are not properly grouped and ordered. According to the Uber Golang style guide, imports should be grouped in the following order:
- Standard library packages.
- Third-party packages.
- Local packages.
Apply this diff to organize the imports:
import ( + "context" + "crypto/sha256" + "encoding/json" + "errors" + "io" + "reflect" + "strings" + "sync" + "testing" + "time" abci "github.com/cometbft/cometbft/abci/types" abciproto "github.com/cometbft/cometbft/api/cometbft/abci/v1" v1 "github.com/cometbft/cometbft/api/cometbft/types/v1" "github.com/cosmos/gogoproto/proto" gogotypes "github.com/cosmos/gogoproto/types" "github.com/stretchr/testify/require" + "github.com/cosmos/cosmos-sdk/testutil/testdata" + sdk "github.com/cosmos/cosmos-sdk/types" + gogoproto "github.com/cosmos/gogoproto/proto" appmodulev2 "cosmossdk.io/core/appmodule/v2" "cosmossdk.io/core/server" "cosmossdk.io/core/store" "cosmossdk.io/core/transaction" "cosmossdk.io/log" "cosmossdk.io/server/v2/appmanager" "cosmossdk.io/server/v2/cometbft/handlers" cometmock "cosmossdk.io/server/v2/cometbft/internal/mock" "cosmossdk.io/server/v2/cometbft/mempool" "cosmossdk.io/server/v2/cometbft/oe" "cosmossdk.io/server/v2/cometbft/types" "cosmossdk.io/server/v2/stf" "cosmossdk.io/server/v2/stf/branch" "cosmossdk.io/server/v2/stf/mock" consensustypes "cosmossdk.io/x/consensus/types" )Also applies to: 38-40
🧰 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)
654-709: Enhance assertions inTestConsensus_GRPCQueryWhile the test covers basic success and failure cases, adding more detailed assertions can improve test robustness:
- Verify the exact contents of the successful response to ensure it matches expected values.
- Consider testing additional edge cases, such as malformed requests or unexpected data.
711-749: Expand error handling tests inTestConsensus_P2PQueryTo strengthen the test suite:
- Include test cases for invalid input scenarios, such as incorrect paths or missing parameters.
- Confirm that appropriate error codes and messages are returned for different failure cases.
813-813: Format code withgofumptThe code is not formatted according to
gofumptstandards. Formatting the code enhances readability and maintains consistency.Run the following command to format the code:
gofumpt -w ./server/v2/cometbft/abci_test.go🧰 Tools
🪛 golangci-lint (1.62.2)
813-813: File is not
gofumpt-ed with-extra(gofumpt)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
server/v2/cometbft/abci_test.go(8 hunks)server/v2/cometbft/utils.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
server/v2/cometbft/utils.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/abci_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🪛 golangci-lint (1.62.2)
server/v2/cometbft/abci_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)
40-40: 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)
824-824: Error return value of queryRouterBuilder.RegisterHandler is not checked
(errcheck)
813-813: File is not gofumpt-ed with -extra
(gofumpt)
🔇 Additional comments (3)
server/v2/cometbft/abci_test.go (2)
593-593: Ensure sufficient test coverage in TestConsensus_QueryStore
The renaming of TestConsensus_Query to TestConsensus_QueryStore improves clarity. Please verify that all scenarios related to store queries are adequately tested to maintain comprehensive coverage.
750-790:
Handle error return value of queryRouterBuilder.RegisterHandler
At line 824, the error returned by queryRouterBuilder.RegisterHandler is not checked. Ignoring errors can lead to unexpected behavior if the handler fails to register.
Apply this diff to handle the potential error:
queryRouterBuilder.RegisterHandler(
proto.MessageName(&testdata.SayHelloRequest{}),
helloFooHandler,
)
+err = queryRouterBuilder.RegisterHandler(
+ proto.MessageName(&testdata.SayHelloRequest{}),
+ helloFooHandler,
+)
+if err != nil {
+ t.Fatalf("failed to register handler: %v", err)
+}Likely invalid or redundant comment.
server/v2/cometbft/utils.go (1)
193-197: Prevent potential nil pointer dereference
Good enhancement in intoABCISimulationResponse function. By checking if txRes.Error is not nil before calling Error(), you prevent a possible nil pointer dereference, improving the robustness of the error handling.
julienrbrt
added
the
backport/v0.52.x
server/v2/cometbft/abci_test.go
Outdated
| @@ -18,6 +18,8 @@ import ( | |||
| gogotypes "github.com/cosmos/gogoproto/types" | |||
| "github.com/stretchr/testify/require" | |||
|
|
|||
| "reflect" | |||
There was a problem hiding this comment.
Could you run the linter? Weird that it doesn't fail, but the import sorting is wrong
julienrbrt
changed the title
p2p, app, grpcp2p, app, grpc
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
server/v2/cometbft/abci_test.go(9 hunks)server/v2/cometbft/utils.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/v2/cometbft/utils.go
🧰 Additional context used
📓 Path-based instructions (1)
server/v2/cometbft/abci_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🪛 golangci-lint (1.62.2)
server/v2/cometbft/abci_test.go
19-19: ST1019(related information): other import of "github.com/cosmos/gogoproto/proto"
(stylecheck)
824-824: Error return value of queryRouterBuilder.RegisterHandler is not checked
(errcheck)
🔇 Additional comments (6)
server/v2/cometbft/abci_test.go (6)
Line range hint 593-653: LGTM! Well-structured store query tests
The renamed test function provides clear separation of store-specific query tests with good coverage of both successful and error scenarios.
654-709: LGTM! Comprehensive GRPC query tests
The test covers:
- Empty request validation
- Invalid query handler scenarios
- Successful query execution with response validation
711-748: LGTM! Well-implemented P2P query tests
The test effectively validates P2P filtering functionality for both address and ID-based queries.
750-789: LGTM! Good coverage of App query scenarios
The test thoroughly covers:
- Transaction simulation
- Version query functionality
Line range hint 879-902: LGTM! Well-structured mock implementations
The setup includes well-implemented mock filters for P2P address and ID validation, along with proper version string handling.
824-827:
Check error return value from RegisterHandler
The error returned by queryRouterBuilder.RegisterHandler should be checked to ensure proper error handling.
Apply this fix:
-queryRouterBuilder.RegisterHandler(
+err := queryRouterBuilder.RegisterHandler(
proto.MessageName(&testdata.SayHelloRequest{}),
helloFooHandler,
)
+require.NoError(t, err)Likely invalid or redundant comment.
🧰 Tools
🪛 golangci-lint (1.62.2)
824-824: Error return value of queryRouterBuilder.RegisterHandler is not checked
(errcheck)
| @@ -15,6 +16,7 @@ import ( | |||
| abciproto "github.com/cometbft/cometbft/api/cometbft/abci/v1" | |||
| v1 "github.com/cometbft/cometbft/api/cometbft/types/v1" | |||
| "github.com/cosmos/gogoproto/proto" | |||
| gogoproto "github.com/cosmos/gogoproto/proto" | |||
There was a problem hiding this comment.
Remove duplicate import of gogoproto/proto
There's a duplicate import of github.com/cosmos/gogoproto/proto. This can lead to confusion and potential issues.
Apply this fix:
-gogoproto "github.com/cosmos/gogoproto/proto"🧰 Tools
🪛 golangci-lint (1.62.2)
19-19: ST1019(related information): other import of "github.com/cosmos/gogoproto/proto"
(stylecheck)
…ackport #22771) (#22773) Co-authored-by: Hieu Vu <[email protected]> Co-authored-by: Julien Robert <[email protected]>
Description
Closes: #21475
SimulationResponseAuthor 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