test(systemtests): fix gRPC tests for v1 & v2 #22774
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request primarily involve enhancements to error handling and testing frameworks across multiple files. The Changes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml ⛔ Files ignored due to path filters (1)
📒 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
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| for key, value := range headers { | ||
| req.Header.Set(key, value) | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
julienrbrt
added
the
backport/v0.52.x
github-actions
bot
added
C:server/v2
julienrbrt
requested review from
a team,
testinginprod and
facundomedica
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
systemtests/rest_support.go (1)
14-14: Use 'URL' instead of 'Url' for consistent namingAccording to Go naming conventions, acronyms should be capitalized. Consider renaming the
Urlfield toURLfor consistency.- Url string + URL stringtests/systemtests/authz_test.go (1)
729-756: Consider improving test case readability.While the test cases provide good coverage of pagination functionality, consider extracting the expected JSON responses into constants or helper functions to improve readability and maintainability.
Example refactor:
+const ( + singleGrantResponse = `{"grants":[{"authorization":...}],"pagination":{"next_key":null,"total":"1"}}` + multipleGrantsResponse = `{"grants":[{"authorization":...},{"authorization":...}],"pagination":{"next_key":null,"total":"2"}}` +) grantsTestCases := []systest.RestTestCase{ { Name: "expect single grant", Url: fmt.Sprintf(grantsURL, granterAddr, grantee1Addr), ExpCode: http.StatusOK, - ExpOut: fmt.Sprintf(`{"grants":[{%s}],"pagination":{"next_key":null,"total":"1"}}`, grant1), + ExpOut: singleGrantResponse, },🧰 Tools
🪛 Gitleaks (8.21.2)
744-744: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
tests/systemtests/distribution_test.go (2)
132-135: Consider adding error test cases for the params endpoint.The current test only covers the success scenario. Consider adding test cases for:
- Invalid block height
- Network errors
- Malformed requests
146-149: Add validation for malformed validator addresses.Consider adding test cases for:
- Malformed validator addresses
- Non-existent validator addresses
- Invalid address format
Also applies to: 159-162
tests/systemtests/bank_test.go (2)
283-283: Consider using exact status code matching where appropriate.While
GetRequestWithHeadersGreaterThanOrEqualis useful for v1/v2 compatibility, some test cases (like line 268's BadRequest) should expect exact status codes. Consider using conditional logic based on the API version being tested.
319-334: Enhance balance endpoint test coverage.Consider adding test cases for:
- Accounts with zero balance
- Very large balance numbers
- Multiple denominations in a single query
- Pagination with large sets of denominations
📜 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 (7)
server/v2/appmanager/appmanager.go(1 hunks)systemtests/CHANGELOG.md(1 hunks)systemtests/rest_support.go(3 hunks)tests/systemtests/authz_test.go(2 hunks)tests/systemtests/bank_test.go(4 hunks)tests/systemtests/distribution_test.go(4 hunks)tests/systemtests/go.mod(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- systemtests/CHANGELOG.md
🧰 Additional context used
📓 Path-based instructions (6)
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"
server/v2/appmanager/appmanager.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
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/authz_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/bank_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"
systemtests/rest_support.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 Gitleaks (8.21.2)
tests/systemtests/authz_test.go
744-744: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (7)
systemtests/rest_support.go (1)
131-133: Avoid non-determinism in map iteration over HTTP headers
Iteration over a map in Go can result in non-deterministic ordering. To ensure consistent behavior when setting headers, consider sorting the map keys before iteration.
server/v2/appmanager/appmanager.go (1)
233-233: Improved error wrapping enhances clarity
Wrapping the error with additional context in fmt.Errorf("invalid height: %w", err) provides clearer error messages and aids in debugging.
tests/systemtests/authz_test.go (3)
685-718: LGTM! Well-structured test cases for grant query endpoint.
The test cases provide good coverage of edge cases including invalid addresses, empty addresses, and invalid message types. The expected outputs are well-defined and match the expected behavior.
771-786: LGTM! Comprehensive test cases for grants by granter endpoint.
The test cases effectively cover error scenarios and successful queries, with clear expected outputs for each case.
798-813: LGTM! Well-structured test cases for grants by grantee endpoint.
The test cases provide good coverage of error scenarios and successful queries, maintaining consistency with other test sections.
tests/systemtests/distribution_test.go (1)
186-201: LGTM! Comprehensive test coverage for slashes endpoint.
The test cases effectively cover:
- Invalid start height
- Invalid end height
- Valid request scenario
tests/systemtests/bank_test.go (1)
268-268: LGTM! Status code change aligns with v2 error handling.
The change from StatusInternalServerError to StatusBadRequest correctly implements the improved error handling in v2, where user errors return appropriate 4xx status codes instead of 500.
tests/systemtests/go.mod
Outdated
| @@ -2,6 +2,8 @@ module github.com/cosmos/cosmos-sdk/tests/systemtests | |||
|
|
|||
| go 1.23 | |||
|
|
|||
| replace cosmossdk.io/systemtests => ../../systemtests | |||
There was a problem hiding this comment.
Avoid committing local 'replace' directives in go.mod
The replace directive with a local path (../../systemtests) can cause build issues for others who do not have the same directory structure. It's recommended to remove local replacements before committing or use versioned modules.
There was a problem hiding this comment.
will replace by a commit from here yeah
There was a problem hiding this comment.
@julienrbrt, great! Thank you for addressing this.
(cherry picked from commit 4caac04) # Conflicts: # server/v2/appmanager/appmanager.go # systemtests/CHANGELOG.md # systemtests/rest_support.go # tests/systemtests/go.mod # tests/systemtests/go.sum
Co-authored-by: Julien Robert <[email protected]>
Description
ref: #22753
Fixes
TestAuthzGRPCQueries& TestBankGRPCQueriesThe fix updates the systemtests framework to support error codes greater than equal.
We've decided to not keep the same behavior in v1 & v2, as the behavior in v2 is supperior.
v1 returns 500 for each failing queries, while v2 returns bad request when it is an user error.
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
Documentation
v1.0.0-rc.3.Refactor