test(systemtest): fix cometbft client #22835
Conversation
julienrbrt
added
the
backport/v0.52.x
julienrbrt
requested review from
cool-develope,
kocubinski and
a team
as code owners
📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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 (1)
store/v2/root/store.go (1)
307-313: LGTM! Sequential execution improves reliability.The change from concurrent to sequential execution of WriteChangeset and Commit operations:
- Simplifies error handling
- Eliminates potential race conditions
- Makes the code more maintainable and easier to debug
Consider adding metrics or logging to track the performance impact of this change, especially for large changesets.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
store/v2/root/store.go(1 hunks)tests/systemtests/cometbft_client_test.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
store/v2/root/store.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
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"
🔇 Additional comments (3)
tests/systemtests/cometbft_client_test.go (3)
265-267: LGTM! Improved test stability.
The addition of chain reset and block await ensures a clean and stable state before running ABCI query tests, which improves test reliability.
30-35: LGTM! Version-specific query handling looks correct.
The conditional logic appropriately handles both v2 ("comet") and legacy status queries.
Let's verify the version-specific query paths:
✅ Verification successful
Version-specific query handling is consistently implemented across the codebase
The verification shows that IsV2() is used consistently throughout the codebase to handle version-specific differences:
- Query paths are properly prefixed with "comet" in v2 mode (e.g., in
cometbft_client_test.goandstaking_test.go) - Other v2-specific adjustments are also handled appropriately:
- Server configuration parameters in
testnet_init.go - Store and snapshot management in
snapshots_test.go - Group query paths in
group_test.go
- Server configuration parameters in
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other version-specific query handlers
rg -A 3 "IsV2\(\)"
Length of output: 4044
231-231: Verify the impact of removing 'Rest' from the test name.
The function rename from TestValidatorSetByHeight_GRPCRestGateway to TestValidatorSetByHeight_GRPCGateway suggests an architectural change.
Let's check for similar patterns and related changes:
✅ Verification successful
The test rename aligns with the codebase's consistent naming pattern
The removal of 'Rest' from the test name is consistent with the codebase's architecture, where all gRPC-gateway related components use GRPCGateway without the Rest prefix. This is evidenced by:
- All other test functions in the codebase use
GRPCGatewaynaming (e.g.,TestLatestValidatorSet_GRPCGateway,TestSimulateTx_GRPCGateway) - Module interfaces and implementations consistently use
GRPCGatewayin their naming (e.g.,HasGRPCGateway,RegisterGRPCGatewayRoutes) - Server and API components follow the same convention with
GRPCGatewayRouter
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other REST-related test names and usages
rg "GRPCRestGateway"
# Search for related architectural changes
rg "GRPC.*Gateway"
Length of output: 13316
(cherry picked from commit f266328) # Conflicts: # store/v2/root/store.go # tests/systemtests/cometbft_client_test.go
Co-authored-by: Julien Robert <[email protected]>
Description
Follow-up of #22834
ref: #22753
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
Tests