test: migrate e2e/distribution to system tests #21908
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request introduce modifications to the Changes
Assessment against linked issues
Possibly related PRs
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
|
akhilkumarpilli
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (4)
tests/systemtests/rest_cli.go (2)
34-60: New functionTestRestQueryIgnoreNumberslooks good with a minor suggestion.The implementation of
TestRestQueryIgnoreNumbersis well-structured and achieves its purpose of ignoring numeric values in JSON responses during comparison. It adheres to Go testing conventions and uses appropriate error handling.A minor suggestion for improvement:
Consider compiling the regex pattern once outside the loop to improve performance, especially if this function is called frequently with many test cases. Here's a suggested modification:
func TestRestQueryIgnoreNumbers(t *testing.T, testCases []RestTestCase) { t.Helper() + // compile the regex once + r, err := regexp.Compile(`"\d+(\.\d+)?"`) + require.NoError(t, err) for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { resp, err := testutil.GetRequest(tc.url) require.NoError(t, err) - // regular expression pattern to match any numeric value in the JSON - numberRegexPattern := `"\d+(\.\d+)?"` - - // compile the regex - r, err := regexp.Compile(numberRegexPattern) - require.NoError(t, err) // replace all numeric values in the above JSONs with `NUMBER` text expectedJSON := r.ReplaceAllString(tc.expOut, `"NUMBER"`) actualJSON := r.ReplaceAllString(string(resp), `"NUMBER"`) // compare two jsons require.JSONEq(t, expectedJSON, actualJSON) }) } }This change will compile the regex only once, potentially improving performance for multiple test cases.
E2e tests were not removed as part of the migration
The PR objective is to migrate e2e tests to system tests; however, no e2e tests have been removed from the
tests/e2edirectory. Please ensure that the old e2e tests are deleted to complete the migration process.
- No e2e tests were removed in
tests/e2edirectory.🔗 Analysis chain
Line range hint
1-60: Verify alignment with PR objectivesThe new
TestRestQueryIgnoreNumbersfunction enhances the system testing capabilities, which aligns with the PR objective of migrating e2e tests to system tests. However, to fully meet the PR objectives:
- Ensure that this new function is being used to replace a specific e2e test that was identified for migration.
- Verify that the old e2e test has been or will be deleted as part of this PR.
- Confirm that the migrated test works correctly with both version 1 and version 2 of the software, as mentioned in the PR objectives.
To help verify the migration process, you can run the following script:
This script will help identify removed e2e tests, verify the usage of the new function, and check for any comments related to version compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the migration of e2e tests to system tests # Test 1: Check for the removal of an e2e test echo "Checking for removed e2e tests:" git diff origin/main | grep -E "^-.*func.*Test.*" tests/e2e # Test 2: Verify the usage of the new TestRestQueryIgnoreNumbers function echo "Checking for usage of TestRestQueryIgnoreNumbers:" rg "TestRestQueryIgnoreNumbers\(" tests/systemtests # Test 3: Look for any TODOs or FIXMEs related to version compatibility echo "Checking for version compatibility comments:" rg -i "TODO|FIXME" -A 3 -B 3 | grep -i "version"Length of output: 3794
Script:
#!/bin/bash # Description: Verify the migration of e2e tests to system tests # Test 1: Check for the removal of e2e tests by identifying deleted test functions or files echo "Checking for removed e2e tests:" git diff origin/main --diff-filter=D --summary | grep " delete mode" | grep "tests/e2e" if [ $? -ne 0 ]; then echo "No e2e tests were removed." else echo "Some e2e tests have been removed." fi # Test 2: Verify the usage of the new TestRestQueryIgnoreNumbers function echo "Checking for usage of TestRestQueryIgnoreNumbers:" rg "TestRestQueryIgnoreNumbers\(" tests/systemtests # Test 3: Look for any TODOs or FIXMEs related to version compatibility echo "Checking for version compatibility comments:" rg -i "TODO|FIXME" -A 3 -B 3 | grep -i "version"Length of output: 3869
tests/systemtests/system.go (2)
135-145: LGTM! Consider enhancing error handling.The changes to the
SetupChainmethod look good. The addition of the expedited voting period and the improved error messages enhance the setup process and provide better clarity.Consider wrapping the error messages with
fmt.Errorfto include the original error. This would provide more context in case of failures. For example:- panic(fmt.Sprintf("failed to set block max gas: %s", err)) + panic(fmt.Errorf("failed to set block max gas: %w", err))This change would allow for better error tracing if needed in the future.
Line range hint
1-740: Summary: Improved setup process and loggingThe changes in this file are focused on enhancing the setup process and improving logging. These modifications, while minimal, contribute to better usability and easier debugging of the system tests. The addition of the expedited voting period in the genesis configuration and the clearer error messages in the
SetupChainmethod are particularly noteworthy improvements.Consider adding more comprehensive logging throughout the file, especially in critical methods like
ResetChain,ModifyGenesisJSON, andAddFullnode. This would further improve the debuggability of the system tests.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
- tests/systemtests/distribution_test.go (1 hunks)
- tests/systemtests/rest_cli.go (2 hunks)
- tests/systemtests/system.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
tests/systemtests/distribution_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/rest_cli.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"tests/systemtests/system.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 (9)
tests/systemtests/rest_cli.go (1)
6-6: New imports look good.The addition of
regexpandgithub.com/cosmos/cosmos-sdk/testutilimports is appropriate for the new functionality introduced in theTestRestQueryIgnoreNumbersfunction.Also applies to: 11-11
tests/systemtests/system.go (4)
Line range hint
180-181: LGTM! Improved logging enhances clarity.The enhancements to the logging in the
StartChainmethod are beneficial. Explicitly mentioning the start of a new block listener improves the clarity of the process and will be helpful for debugging and monitoring.
Line range hint
435-458: No changes in this segment.
Line range hint
516-578: No changes in this segment.
Line range hint
605-740: No changes in this segment.tests/systemtests/distribution_test.go (4)
18-101: Tests in 'TestWithdrawAllRewardsCmd' cover key scenarios effectivelyThe function
TestWithdrawAllRewardsCmdcomprehensively tests the withdraw-all-rewards command, including different--max-msgsvalues and verifies balances before and after withdrawals. The test is well-structured and adheres to best practices.
103-226: 'TestDistrValidatorGRPCQueries' provides thorough validation of gRPC endpointsThe test function effectively checks validator-related gRPC queries, handling both valid and invalid inputs, and ensures the endpoints return the expected responses. Error cases are appropriately tested.
228-349: 'TestDistrDelegatorGRPCQueries' thoroughly tests delegator gRPC queriesThis function covers a wide range of scenarios for delegator-related queries, including rewards, validators, and withdraw addresses. It tests both success and failure cases, ensuring robustness of the endpoints.
346-346: Ensure the correctness of the expected withdraw addressIn the test case for the withdraw address, verify that the expected address is correct and matches the one set in the test setup.
Run the following script to check if the withdraw address matches the delegator address:
| newAddr := cli.AddKey("newAddr") | ||
| require.NotEmpty(t, newAddr) | ||
|
|
||
| testDenom := "stake" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define 'testDenom' as a constant
The variable testDenom is a constant value throughout the tests. Defining it as a constant improves code readability and conveys that its value does not change.
Apply this diff:
-testDenom := "stake"
+const testDenom = "stake"📝 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.
| testDenom := "stake" | |
| const testDenom = "stake" |
| val1Addr := validators[0].String() | ||
| val2Addr := validators[1].String() | ||
|
|
||
| var delegationAmount int64 = 100000 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define 'delegationAmount' as a constant
The delegationAmount variable holds a constant value used in multiple places. Defining it as a constant will make the code more maintainable.
Apply this diff:
-var delegationAmount int64 = 100000
+const delegationAmount int64 = 100_000📝 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.
| var delegationAmount int64 = 100000 | |
| const delegationAmount int64 = 100_000 |
|
|
||
| testDenom := "stake" | ||
|
|
||
| var initialAmount int64 = 10000000 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define 'initialAmount' as a constant
Since initialAmount represents a fixed initial value, consider defining it as a constant to enhance code clarity.
Apply this diff:
-var initialAmount int64 = 10000000
+const initialAmount int64 = 10_000_000📝 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.
| var initialAmount int64 = 10000000 | |
| const initialAmount int64 = 10_000_000 |
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| assertGenOnlyOutput := func(_ assert.TestingT, gotErr error, gotOutputs ...interface{}) bool { | ||
| require.Len(t, gotOutputs, 1) | ||
| // gets output combining two objects without any space or new line | ||
| splitOutput := strings.Split(gotOutputs[0].(string), "}{") | ||
| require.Len(t, splitOutput, tc.expTxLen) | ||
| return false | ||
| } | ||
| cmd := append(withdrawCmdArgs, fmt.Sprintf("--max-msgs=%d", tc.maxMsgs), "--generate-only") | ||
| _ = cli.WithRunErrorMatcher(assertGenOnlyOutput).Run(cmd...) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the test case loop by extracting common logic
In the loop over test cases, the construction of cmd and the assertion function are repeated. Consider refactoring to reduce duplication and improve readability.
You can extract the assertion function and command construction outside the loop:
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
+ cmd := append(withdrawCmdArgs, fmt.Sprintf("--max-msgs=%d", tc.maxMsgs), "--generate-only")
assertGenOnlyOutput := func(_ assert.TestingT, gotErr error, gotOutputs ...interface{}) bool {
require.Len(t, gotOutputs, 1)
// gets output combining two objects without any space or new line
splitOutput := strings.Split(gotOutputs[0].(string), "}{")
require.Len(t, splitOutput, tc.expTxLen)
return false
}
- cmd := append(withdrawCmdArgs, fmt.Sprintf("--max-msgs=%d", tc.maxMsgs), "--generate-only")
_ = cli.WithRunErrorMatcher(assertGenOnlyOutput).Run(cmd...)
})
}📝 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.
| for _, tc := range testCases { | |
| t.Run(tc.name, func(t *testing.T) { | |
| assertGenOnlyOutput := func(_ assert.TestingT, gotErr error, gotOutputs ...interface{}) bool { | |
| require.Len(t, gotOutputs, 1) | |
| // gets output combining two objects without any space or new line | |
| splitOutput := strings.Split(gotOutputs[0].(string), "}{") | |
| require.Len(t, splitOutput, tc.expTxLen) | |
| return false | |
| } | |
| cmd := append(withdrawCmdArgs, fmt.Sprintf("--max-msgs=%d", tc.maxMsgs), "--generate-only") | |
| _ = cli.WithRunErrorMatcher(assertGenOnlyOutput).Run(cmd...) | |
| }) | |
| } | |
| for _, tc := range testCases { | |
| t.Run(tc.name, func(t *testing.T) { | |
| cmd := append(withdrawCmdArgs, fmt.Sprintf("--max-msgs=%d", tc.maxMsgs), "--generate-only") | |
| assertGenOnlyOutput := func(_ assert.TestingT, gotErr error, gotOutputs ...interface{}) bool { | |
| require.Len(t, gotOutputs, 1) | |
| // gets output combining two objects without any space or new line | |
| splitOutput := strings.Split(gotOutputs[0].(string), "}{") | |
| require.Len(t, splitOutput, tc.expTxLen) | |
| return false | |
| } | |
| _ = cli.WithRunErrorMatcher(assertGenOnlyOutput).Run(cmd...) | |
| }) | |
| } |
| // extract gentx path from above command output | ||
| re := regexp.MustCompile(`"(.*?\.json)"`) | ||
| match := re.FindStringSubmatch(rsp) | ||
| require.GreaterOrEqual(t, len(match), 1) | ||
|
|
||
| updatedGentx := filepath.Join(WorkDir, match[1]) | ||
| updatedGentxBz, err := os.ReadFile(updatedGentx) // #nosec G304 | ||
| require.NoError(t, err) | ||
|
|
||
| sut.ModifyGenesisJSON(t, func(genesis []byte) []byte { | ||
| state, err := sjson.SetRawBytes(genesis, "app_state.genutil.gen_txs.0", updatedGentxBz) | ||
| require.NoError(t, err) | ||
| return state | ||
| }) |
There was a problem hiding this comment.
Handle errors from 'regexp.MustCompile' and improve error checking
Using regexp.MustCompile will panic if the regex is invalid. Instead, use regexp.Compile and handle the error. Additionally, ensure all errors are checked for robustness.
Apply this diff:
-re := regexp.MustCompile(`"(.*?\.json)"`)
+re, err := regexp.Compile(`"(.*?\.json)"`)
+require.NoError(t, err)Committable suggestion was skipped due to low confidence.
| baseurl := sut.APIAddress() | ||
| decodingFailedOutput := `{"code":2, "message":"decoding bech32 failed: invalid separator index -1", "details":[]}` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reuse 'decodingFailedOutput' variable
The decodingFailedOutput variable is redefined in multiple test functions. Consider defining it once and reusing it across tests to avoid duplication.
Move decodingFailedOutput to a common location:
+const decodingFailedOutput = `{"code":2, "message":"decoding bech32 failed: invalid separator index -1", "details":[]}`
func TestDistrValidatorGRPCQueries(t *testing.T) {
// ...
- decodingFailedOutput := `{"code":2, "message":"decoding bech32 failed: invalid separator index -1", "details":[]}`
// ...
}
func TestDistrDelegatorGRPCQueries(t *testing.T) {
// ...
- decodingFailedOutput := `{"code":2, "message":"decoding bech32 failed: invalid separator index -1", "details":[]}`
// ...
}Committable suggestion was skipped due to low confidence.
| sut.AwaitNBlocks(t, 3) | ||
|
|
||
| baseurl := sut.APIAddress() | ||
| expectedAmountOutput := fmt.Sprintf(`{"denom":"%s","amount":"203.105000000000000000"}`, denom) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hardcoding amounts in expected outputs
The expectedAmountOutput variable contains hardcoded amounts, which may cause tests to fail if the underlying calculations change. Consider computing expected amounts dynamically.
Calculate the expected amount based on initial conditions:
-expectedAmountOutput := fmt.Sprintf(`{"denom":"%s","amount":"203.105000000000000000"}`, denom)
+commissionAmount := calculateExpectedCommissionAmount()
+expectedAmountOutput := fmt.Sprintf(`{"denom":"%s","amount":"%s"}`, denom, commissionAmount)Where calculateExpectedCommissionAmount() is a helper function that computes the expected commission.
Committable suggestion was skipped due to low confidence.
|
Could you delete the relevant tests? |
julienrbrt
added
the
backport/v0.52.x
| // test withdraw address grpc endpoint | ||
| withdrawAddrURL := baseurl + `/cosmos/distribution/v1beta1/delegators/%s/withdraw_address` | ||
| withdrawAddrTestCases := []RestTestCase{ | ||
| { |
There was a problem hiding this comment.
i think we can remove those cases, this is catched by unit tests
| delegatorValsURL := baseurl + `/cosmos/distribution/v1beta1/delegators/%s/validators` | ||
| valsTestCases := []RestTestCase{ | ||
| { | ||
| "invalid delegator validators gRPC request with wrong delegator address", |
| }, | ||
| { | ||
| "wrong validator address (specific validator rewards)", | ||
| fmt.Sprintf(delegatorRewardsURL+`/%s`, delAddr, "wrongvaladdress"), |
| rewardsOutput := fmt.Sprintf(`{"rewards":[{"validator_address":"%s","reward":[%s]}],"total":[%s]}`, valOperAddr, expectedAmountOutput, expectedAmountOutput) | ||
|
|
||
| delegatorRewardsTestCases := []RestTestCase{ | ||
| { |
|
|
||
| slashTestCases := []RestTestCase{ | ||
| { | ||
| "invalid slashes gRPC request with wrong validator address", |
(cherry picked from commit 489aaae) # Conflicts: # tests/systemtests/rest_cli.go # tests/systemtests/system.go
|
@Mergifyio backport release/v0.52.x |
✅ Backports have been created
|
) Co-authored-by: Akhil Kumar P <[email protected]>
Description
Closes: #21854
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
Refactor
Chores