test: migrate e2e/group to system tests #22278
Conversation
📝 WalkthroughWalkthroughThis pull request involves the removal of end-to-end (E2E) tests for 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
|
![coderabbitai[bot]](https://github.com/avatars/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 (2)
tests/systemtests/group_test.go (2)
59-59: Extract hardcoded 'voting_period' values into a constant.The
voting_periodis hardcoded as"30000s"in multiple places. Defining it as a constant will improve maintainability and make updates easier if the value changes in the future.Apply this diff to define a constant and use it:
+ const votingPeriod = "30000s" // Inside the loop at line 59 - policyFile := StoreTempFile(t, []byte(fmt.Sprintf(`{"@type":"/cosmos.group.v1.ThresholdDecisionPolicy", "threshold":"%d", "windows":{"voting_period":"30000s"}}`, threshold))) + policyFile := StoreTempFile(t, []byte(fmt.Sprintf(`{"@type":"/cosmos.group.v1.ThresholdDecisionPolicy", "threshold":"%d", "windows":{"voting_period":"%s"}}`, threshold, votingPeriod))) // At line 83 for the percentage policy - policyFile := StoreTempFile(t, []byte(fmt.Sprintf(`{"@type":"%s", "percentage":"%f", "windows":{"voting_period":"30000s"}}`, percentagePolicyType, 0.5))) + policyFile := StoreTempFile(t, []byte(fmt.Sprintf(`{"@type":"%s", "percentage":"%f", "windows":{"voting_period":"%s"}}`, percentagePolicyType, 0.5, votingPeriod)))Also applies to: 83-83
48-120: Consider renaming 'rsp' to 'response' for clarity.The variable
rspis used throughout the test to store command responses. Renamingrsptoresponsewill enhance readability and align with the Uber Go Style Guide, which recommends using descriptive variable names.Apply this diff to rename the variable:
- rsp := cli.RunAndWait(createGroupCmd...) + response := cli.RunAndWait(createGroupCmd...)Update subsequent references to
rspaccordingly.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
- tests/e2e/group/cli_test.go (0 hunks)
- tests/e2e/group/suite.go (0 hunks)
- tests/systemtests/group_test.go (1 hunks)
💤 Files with no reviewable changes (2)
- tests/e2e/group/cli_test.go
- tests/e2e/group/suite.go
🧰 Additional context used
📓 Path-based instructions (1)
tests/systemtests/group_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/group_test.go (1)
74-74:⚠️ Potential issueVerify the correctness of 'pagination.total' comparison with 'threshold'.
In the assertion
require.Equal(t, gjson.Get(groupPoliciesResp, "pagination.total").Int(), int64(threshold)), you're comparingpagination.totaltothreshold. Ensure thatpagination.totalrepresents the total number of group policies at the current state and that it aligns with the expectedthresholdvalue in each iteration. Ifpagination.totalis cumulative across iterations, this comparison may not behave as intended.Run the following script to verify the cumulative value of
pagination.total:
| baseurl := sut.APIAddress() | ||
|
|
||
| // test create group | ||
| memberWeight := "5" | ||
| validMembers := fmt.Sprintf(` | ||
| { | ||
| "members": [ | ||
| { | ||
| "address": "%s", | ||
| "weight": "%s", | ||
| "metadata": "%s" | ||
| } | ||
| ] | ||
| }`, valAddr, memberWeight, validMetadata) | ||
| validMembersFile := StoreTempFile(t, []byte(validMembers)) | ||
| createGroupCmd := []string{"tx", "group", "create-group", valAddr, validMetadata, validMembersFile.Name(), "--from=" + valAddr} | ||
| rsp := cli.RunAndWait(createGroupCmd...) | ||
| RequireTxSuccess(t, rsp) | ||
|
|
||
| // query groups by admin to confirm group creation | ||
| rsp = cli.CustomQuery("q", "group", "groups-by-admin", valAddr) | ||
| require.Len(t, gjson.Get(rsp, "groups").Array(), 1) | ||
| groupId := gjson.Get(rsp, "groups.0.id").String() | ||
|
|
||
| // test create group policies | ||
| for i := 0; i < 5; i++ { | ||
| threshold := i + 1 | ||
| policyFile := StoreTempFile(t, []byte(fmt.Sprintf(`{"@type":"/cosmos.group.v1.ThresholdDecisionPolicy", "threshold":"%d", "windows":{"voting_period":"30000s"}}`, threshold))) | ||
| policyCmd := []string{"tx", "group", "create-group-policy", valAddr, groupId, validMetadata, policyFile.Name(), "--from=" + valAddr} | ||
| rsp = cli.RunAndWait(policyCmd...) | ||
| RequireTxSuccess(t, rsp) | ||
|
|
||
| // TODO: remove isV2() check once v2 is integrated with grpc gateway | ||
| var groupPoliciesResp, policyAddrQuery string | ||
| if isV2() { | ||
| groupPoliciesResp = cli.CustomQuery("q", "group", "group-policies-by-group", groupId) | ||
| policyAddrQuery = fmt.Sprintf("group_policies.#(decision_policy.value.threshold==%d).address", threshold) | ||
| } else { | ||
| groupPoliciesResp = string(GetRequest(t, fmt.Sprintf("%s/cosmos/group/v1/group_policies_by_group/%s", baseurl, groupId))) | ||
| policyAddrQuery = fmt.Sprintf("group_policies.#(decision_policy.threshold==%d).address", threshold) | ||
| } | ||
| require.Equal(t, gjson.Get(groupPoliciesResp, "pagination.total").Int(), int64(threshold)) | ||
| policyAddr := gjson.Get(groupPoliciesResp, policyAddrQuery).String() | ||
| require.NotEmpty(t, policyAddr) | ||
|
|
||
| rsp = cli.RunCommandWithArgs(cli.withTXFlags("tx", "bank", "send", valAddr, policyAddr, "1000stake", "--generate-only")...) | ||
| require.Equal(t, policyAddr, gjson.Get(rsp, "body.messages.0.to_address").String()) | ||
| } | ||
|
|
||
| // test create group policy with percentage decision policy | ||
| percentagePolicyType := "/cosmos.group.v1.PercentageDecisionPolicy" | ||
| policyFile := StoreTempFile(t, []byte(fmt.Sprintf(`{"@type":"%s", "percentage":"%f", "windows":{"voting_period":"30000s"}}`, percentagePolicyType, 0.5))) | ||
| policyCmd := []string{"tx", "group", "create-group-policy", valAddr, groupId, validMetadata, policyFile.Name(), "--from=" + valAddr} | ||
| rsp = cli.RunAndWait(policyCmd...) | ||
| RequireTxSuccess(t, rsp) | ||
|
|
||
| groupPoliciesResp := cli.CustomQuery("q", "group", "group-policies-by-admin", valAddr) | ||
| require.Equal(t, gjson.Get(groupPoliciesResp, "pagination.total").Int(), int64(6)) | ||
| policyAddr := gjson.Get(groupPoliciesResp, fmt.Sprintf("group_policies.#(decision_policy.type==%s).address", percentagePolicyType)).String() | ||
| require.NotEmpty(t, policyAddr) | ||
|
|
||
| // test create proposal | ||
| proposalJSON := fmt.Sprintf(`{ | ||
| "group_policy_address": "%s", | ||
| "messages":[ | ||
| { | ||
| "@type": "/cosmos.bank.v1beta1.MsgSend", | ||
| "from_address": "%s", | ||
| "to_address": "%s", | ||
| "amount": [{"denom": "stake","amount": "100"}] | ||
| } | ||
| ], | ||
| "metadata": "%s", | ||
| "title": "My Proposal", | ||
| "summary": "Summary", | ||
| "proposers": ["%s"] | ||
| }`, policyAddr, policyAddr, valAddr, validMetadata, valAddr) | ||
| proposalFile := StoreTempFile(t, []byte(proposalJSON)) | ||
| rsp = cli.RunAndWait("tx", "group", "submit-proposal", proposalFile.Name()) | ||
| RequireTxSuccess(t, rsp) | ||
|
|
||
| // query proposals | ||
| rsp = cli.CustomQuery("q", "group", "proposals-by-group-policy", policyAddr) | ||
| require.Len(t, gjson.Get(rsp, "proposals").Array(), 1) | ||
| proposalId := gjson.Get(rsp, "proposals.0.id").String() | ||
|
|
||
| // test vote proposal | ||
| rsp = cli.RunAndWait("tx", "group", "vote", proposalId, valAddr, "yes", validMetadata) | ||
| RequireTxSuccess(t, rsp) | ||
|
|
||
| // query votes | ||
| // TODO: remove isV2() check once v2 is integrated with grpc gateway | ||
| var voteResp string | ||
| if isV2() { | ||
| voteResp = cli.CustomQuery("q", "group", "vote", proposalId, valAddr) | ||
| } else { | ||
| voteResp = string(GetRequest(t, fmt.Sprintf("%s/cosmos/group/v1/vote_by_proposal_voter/%s/%s", baseurl, proposalId, valAddr))) | ||
| } | ||
| require.Equal(t, "VOTE_OPTION_YES", gjson.Get(voteResp, "vote.option").String()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider splitting 'TestGroupCommands' into smaller, focused test functions.
The TestGroupCommands function is quite extensive and covers multiple scenarios including group creation, policy creation, proposal submission, and voting. According to the Uber Go Style Guide, it's recommended to keep functions small and focused on a single responsibility. Splitting this function into smaller test functions will improve readability, make maintenance easier, and help isolate test failures.
🛠️ Refactor suggestion
Enhance test coverage by adding error scenarios and edge cases.
The current tests cover the standard workflow for group commands. To ensure robustness, consider adding test cases for error scenarios such as:
- Attempting to create a group with invalid members.
- Submitting a proposal with invalid messages.
- Voting with an unauthorized address.
- Edge cases like zero thresholds or percentages outside the valid range.
Including these cases will improve test coverage and help catch potential issues early.
julienrbrt
added
the
backport/v0.52.x
(cherry picked from commit 553e110)
Description
Closes: #22219
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