test: e2e/client to system tests #21981
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request involves the deletion of an end-to-end test suite for the gRPC service in the Cosmos SDK's cmtservice module and introduces new system tests for the gRPC client functionalities and the CometBFT client. Additionally, the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
|
JulianToledano
assigned JulianToledano, alpe and julienrbrt and unassigned JulianToledano
julienrbrt
added
the
backport/v0.52.x
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
tests/systemtests/rpc_client.go (1)
111-113: Refine the error handling inNewStreammethodThe
NewStreammethod currently returns a generic error message"not implemented". Consider returning a more specific gRPC status error to clearly indicate that streaming is not supported.Apply this diff to refine the error returned:
func (r RPCClient) NewStream(ctx context.Context, desc *grpc.StreamDesc, method string, opts ...grpc.CallOption) (grpc.ClientStream, error) { - return nil, errors.New("not implemented") + return nil, status.Errorf(codes.Unimplemented, "streaming is not supported") }Ensure that you import the necessary packages:
+ "google.golang.org/grpc/codes" + "google.golang.org/grpc/status"tests/systemtests/client_test.go (3)
27-32: Consider using more descriptive variable names for clarityIn accordance with the Uber Go Style Guide, using more descriptive variable names can improve code readability. Consider renaming
qctoclient,restonodeInfo, andvtocliVersion.Apply this diff to improve variable naming:
-qc := cmtservice.NewServiceClient(sut.RPCClient(t)) -res, err := qc.GetNodeInfo(context.Background(), &cmtservice.GetNodeInfoRequest{}) +client := cmtservice.NewServiceClient(sut.RPCClient(t)) +nodeInfo, err := client.GetNodeInfo(context.Background(), &cmtservice.GetNodeInfoRequest{}) ... -v := NewCLIWrapper(t, sut, true).Version() +cliVersion := NewCLIWrapper(t, sut, true).Version() -assert.Equal(t, res.ApplicationVersion.Version, v) +assert.Equal(t, nodeInfo.ApplicationVersion.Version, cliVersion)
99-99: Remove unnecessaryfmt.PrintfstatementThe
fmt.Printf("%v", res)statement on line 99 may be unnecessary in the test and can clutter the test output. Consider removing it or using proper logging if needed.Apply this diff to remove the unnecessary print statement:
- fmt.Printf("%v", res)
255-256: Consider removingexpectedCodefield if it's not usedThe
expectedCodefield in the test case struct is not set or used in the test cases. If it's not needed, consider removing it to simplify the test code.Apply this diff to remove the unused field:
type testCase struct { name string req *cmtservice.ABCIQueryRequest expectErr bool - expectedCode uint32 validQuery bool }And remove the corresponding assertion in line 320:
- assert.Equal(t, res.Code, tc.expectedCode)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
tests/systemtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
- tests/e2e/client/grpc/cmtservice/service_test.go (0 hunks)
- tests/systemtests/client_test.go (1 hunks)
- tests/systemtests/go.mod (1 hunks)
- tests/systemtests/rpc_client.go (2 hunks)
💤 Files with no reviewable changes (1)
- tests/e2e/client/grpc/cmtservice/service_test.go
🧰 Additional context used
📓 Path-based instructions (3)
tests/systemtests/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/systemtests/go.mod (1)
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"tests/systemtests/rpc_client.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (3)
tests/systemtests/go.mod (2)
29-29: LGTM: Addition of CometBFT API dependencyThe addition of
github.com/cometbft/cometbft/api v1.0.0-rc.1is appropriate for implementing e2e tests that interact with or simulate CometBFT components. This aligns well with the PR objective of introducing end-to-end tests for client to system interactions.
23-23: Verify direct usage of gRPC in system testsThe
google.golang.org/grpc v1.67.0dependency has been moved from indirect to direct. This suggests that the system tests now directly use gRPC, possibly for client-server communication in the e2e tests.Please ensure that the gRPC package is being used appropriately in the newly added or modified system tests. You can verify this by running the following command:
This will help confirm that the change in dependency status is justified by actual usage in the test code.
✅ Verification successful
Verification Successful: Direct gRPC usage confirmed in system tests
tests/systemtests/rpc_client.go: Utilizes gRPC for invoking methods and creating streams.tests/systemtests/system.go: Configures gRPC addresses for system components.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for gRPC usage in system test files rg --type go 'grpc\.' tests/systemtestsLength of output: 537
tests/systemtests/rpc_client.go (1)
47-109: Well-implementedInvokemethodThe
Invokemethod is implemented correctly, handling serialization, context metadata extraction, error checking, and response unmarshalling appropriately.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -2,11 +2,23 @@ | |||
|
|
|||
| import ( | |||
| "context" | |||
| "errors" | |||
| "reflect" | |||
Check notice
Code scanning / CodeQL
Sensitive package import Note test
tests/systemtests/client_test.go
Outdated
There was a problem hiding this comment.
let's rename this to cometbft_client_test.go
tests/systemtests/client_test.go
Outdated
| v := NewCLIWrapper(t, sut, true).Version() | ||
| assert.Equal(t, res.ApplicationVersion.Version, v) | ||
|
|
||
| restRes, err := testutil.GetRequest(fmt.Sprintf("%s/cosmos/base/tendermint/v1beta1/node_info", baseurl)) |
There was a problem hiding this comment.
For later, we should be adding a way to distinguish a v2 app with comet and one without.
Nothing to do here, but eventually we should skip some v2 system test depending on the consensus engine we want to test
There was a problem hiding this comment.
I'll add a todo
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
tests/systemtests/cometbft_client_test.go (3)
34-34: Address the TODO comment to distinguish version v2 in testsThe TODO comment indicates a need to add logic to distinguish version v2 and potentially skip certain system tests depending on the consensus engine being tested. Implementing this functionality will ensure that tests are run appropriately for different consensus engines.
Would you like assistance in implementing this logic or creating a GitHub issue to track this task?
49-51: Add assertions to validate response contentIn the
TestQuerySyncingfunction, the test checks that no error occurs when making the REST API request but does not assert the actual syncing status returned. Consider adding an assertion to verify that the REST API response syncing status matches the gRPC client's syncing status.
64-66: Add assertions to validate REST API response inTestQueryLatestBlockThe test checks for errors when requesting the latest block via REST API but does not verify the content of the response. Adding assertions to confirm that the REST API response contains the expected block data will improve test coverage.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
- tests/systemtests/cometbft_client_test.go (1 hunks)
- tests/systemtests/rpc_client.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/systemtests/rpc_client.go
🧰 Additional context used
📓 Path-based instructions (1)
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 (1)
tests/systemtests/cometbft_client_test.go (1)
260-266: Ensure error handling for marshaling inTestABCIQueryIn the test case "request with invalid path recursive," the
Datafield is set by marshaling anotherABCIQueryRequest. Ensure that any errors occurring during marshaling are properly handled, and that the test effectively validates the behavior when an invalid recursive path is provided.
| func TestLatestValidatorSet(t *testing.T) { | ||
| sut.ResetChain(t) | ||
| sut.StartChain(t) | ||
|
|
||
| vals := sut.RPCClient(t).Validators() | ||
|
|
||
| qc := cmtservice.NewServiceClient(sut.RPCClient(t)) | ||
| testCases := []struct { | ||
| name string | ||
| req *cmtservice.GetLatestValidatorSetRequest | ||
| expErr bool | ||
| expErrMsg string | ||
| }{ | ||
| {"nil request", nil, true, "cannot be nil"}, | ||
| {"no pagination", &cmtservice.GetLatestValidatorSetRequest{}, false, ""}, | ||
| {"with pagination", &cmtservice.GetLatestValidatorSetRequest{Pagination: &qtypes.PageRequest{Offset: 0, Limit: uint64(len(vals))}}, false, ""}, | ||
| } | ||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| res, err := qc.GetLatestValidatorSet(context.Background(), tc.req) | ||
| if tc.expErr { | ||
| assert.Error(t, err) | ||
| assert.Contains(t, err.Error(), tc.expErrMsg) | ||
| } else { | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, len(res.Validators), len(vals)) | ||
| content, ok := res.Validators[0].PubKey.GetCachedValue().(cryptotypes.PubKey) | ||
| assert.True(t, ok) | ||
| assert.Equal(t, content.Address(), vals[0].PubKey.Address()) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance assertions in TestLatestValidatorSet
The test cases check for various scenarios when querying the latest validator set. Consider adding additional assertions to validate the contents of the returned validators, such as checking individual validator fields or properties, to enhance test robustness.
(cherry picked from commit 91b47cb) # Conflicts: # tests/systemtests/go.mod # tests/systemtests/go.sum
* main: docs: amend docs for 52 changes (#21992) test: migrate e2e/authz to system tests (#21819) refactor(runtime/v2): use StoreBuilder (#21989) feat(schema): add API descriptors, struct, oneof & list types, and wire encoding spec (#21482) docs: add instructions to change DefaultGenesis (#21680) feat(x/staking)!: Add metadata field to validator info (#21315) chore(x/authz)!: Remove account keeper dependency (#21632) chore(contributing): delete link (#21990) test(gov): Migrate e2e to system test (#21927) test: e2e/client to system tests (#21981)
Co-authored-by: Julien Robert <[email protected]> Co-authored-by: Julian Toledano <[email protected]>
Description
Closes:
#21923
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
Chores