test(systemtests): fix failing tests #22145
Conversation
📝 WalkthroughWalkthroughThe pull request introduces several enhancements to the test suite for querying node information and validator sets within the Changes
Possibly related 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
|
alpe
force-pushed
the
alex/system_TestQueryLatestValidatorSet
branch
from
9a47afa to
56cb095
Compare
julienrbrt
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: 2
🧹 Outside diff range and nitpick comments (6)
tests/systemtests/rest_cli.go (1)
84-85: Approved: Enhanced error reporting with response body inclusionThis change improves the error reporting by including the response body when the status code doesn't match the expected value. This additional context will be valuable for debugging failing tests, which aligns well with the PR's objective of fixing failing system tests.
Consider truncating very large response bodies to avoid overwhelming error messages. You could implement this by adding a helper function to limit the response body to a reasonable length (e.g., 1000 characters) if it exceeds that limit. For example:
func truncateBody(body []byte, maxLen int) string { if len(body) <= maxLen { return string(body) } return string(body[:maxLen]) + "... (truncated)" }Then use it in the error message:
require.Equal(t, expCode, res.StatusCode, "status code should be %d, got: %d, %s", expCode, res.StatusCode, truncateBody(body, 1000))This would ensure that the error messages remain informative without becoming excessively long.
tests/systemtests/upgrade_test.go (2)
20-22: LGTM. Consider creating a tracking issue for re-enabling the test.The skip message clearly explains why the test is currently disabled. This prevents false negatives in the test suite.
To ensure this test doesn't remain skipped indefinitely, would you like me to create a GitHub issue to track the re-enabling of this test once the conditions are met?
Line range hint
1-124: Overall, the changes look good but require some follow-up actions.The updates to version numbers and upgrade names are consistent with the new versioning scheme. However, to ensure the integrity of the test suite, please:
- Create a tracking issue for re-enabling this test once the conditions mentioned in the skip message are met.
- Verify the availability of the v0.52 binary as suggested in the review comments.
- Confirm the consistency of the upgrade name across the codebase, particularly in simapp/upgrades.go.
These steps will help maintain the reliability and effectiveness of the upgrade test.
tests/systemtests/system.go (1)
769-772: LGTM! Consider adding a brief comment.The
NodesCount()method is a well-implemented getter for thenodesCountfield. It follows Go conventions and provides a clean way to access this information.Consider adding a brief comment to explain the purpose of this method, for example:
// NodesCount returns the number of node instances currently in use by the system under test. func (s *SystemUnderTest) NodesCount() int { return s.nodesCount }tests/systemtests/cometbft_client_test.go (2)
213-213: Ensure consistent acronym casing in function namesIn Go, acronyms should be consistently capitalized. Consider renaming the function to
TestValidatorSetByHeight_GRPCRESTGatewayto accurately represent the acronymsGRPCandREST.
222-226: Consistent capitalization of acronyms in struct field namesAcronyms in struct field names should be fully capitalized. Rename
expHttpCodetoexpHTTPCodeto maintain consistent acronym casing.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (5)
- tests/systemtests/cometbft_client_test.go (6 hunks)
- tests/systemtests/rest_cli.go (1 hunks)
- tests/systemtests/system.go (1 hunks)
- tests/systemtests/testnet_init.go (1 hunks)
- tests/systemtests/upgrade_test.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
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"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"tests/systemtests/testnet_init.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/upgrade_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 (9)
tests/systemtests/upgrade_test.go (3)
34-34: LGTM. Good practice to use the legacy binary for initialization.Setting the testnet initializer with the legacy binary ensures consistency between initialization and execution. This is a good practice that helps prevent version mismatch issues.
41-41: Verify consistency of upgrade name across the codebase.The update to the upgrade name aligns with the new versioning scheme. However, we should ensure that this name is consistent with the UpgradeName in simapp/upgrades.go as mentioned in the comment.
Let's verify the consistency of the upgrade name:
#!/bin/bash # Description: Check if the upgrade name is consistent in simapp/upgrades.go # Test: Search for the upgrade name in simapp/upgrades.go grep -n "v052-to-v054" $(fd -t f -e go upgrades.go) if [ $? -eq 0 ]; then echo "Upgrade name is consistent in simapp/upgrades.go" else echo "Upgrade name is not found or inconsistent in simapp/upgrades.go" fi
29-29: Verify the availability of v0.52 binary.The update to use the v0.52 binary aligns with the new versioning scheme. However, we should ensure that this version is actually available and compatible with the test setup.
Let's verify the availability of the v0.52 binary:
tests/systemtests/testnet_init.go (2)
54-66: LGTM! Enhancement to testnet initialization.The new
InitializerWithBinaryfunction is a valuable addition that allows for more flexible testnet initialization by specifying a custom binary. The implementation is clean, follows Go conventions, and correctly utilizes the existingNewSingleHostTestnetCmdInitializerconstructor.
Line range hint
1-66: File Review: Enhancing testnet initialization capabilitiesThe addition of the
InitializerWithBinaryfunction is the only change in this file. It integrates seamlessly with the existing code and provides a valuable enhancement to the testnet initialization process. The implementation adheres to Go best practices and the Uber Go Style Guide.As per the coding guidelines for
tests/**/*, this change contributes to improving the test coverage by allowing more flexible initialization of the testnet, which can lead to more comprehensive system tests.tests/systemtests/cometbft_client_test.go (4)
8-8: Correctly importednet/httppackageThe addition of the
net/httpimport is necessary for referencing HTTP status codes likehttp.StatusBadRequest.
170-176: Enhanced error handling in testsThe use of
GetRequestWithHeadersalong with expected HTTP status codes improves the robustness of the tests by explicitly checking for expected error conditions.
235-241: Improved test validation with expected HTTP status codesIncluding
expHTTPCodein the test cases and using it inGetRequestWithHeadersenhances test accuracy by validating the expected HTTP responses.
320-320: Assertion onres.Codestrengthens test checksAdding
assert.Equal(t, tc.expectedCode, res.Code)ensures that the response code matches the expected value, enhancing the test's effectiveness.
| @@ -103,7 +108,7 @@ func TestQueryLatestValidatorSet(t *testing.T) { | |||
| assert.NoError(t, err) | |||
| assert.Equal(t, len(res.Validators), 2) | |||
|
|
|||
| restRes := GetRequest(t, mustV(url.JoinPath(baseurl, "/cosmos/base/tendermint/v1beta1/validatorsets/latest?pagination.offset=0&pagination.limit=2"))) | |||
| restRes := GetRequest(t, fmt.Sprintf("%s/cosmos/base/tendermint/v1beta1/validatorsets/latest?pagination.offset=%d&pagination.limit=%d", baseurl, 0, 2)) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use net/url package to construct URLs safely
Constructing URLs using fmt.Sprintf may lead to issues if parameters contain special characters. Consider using the net/url package to properly encode query parameters and build the URL safely.
Refactor the URL construction as follows:
u, err := url.Parse(fmt.Sprintf("%s/cosmos/base/tendermint/v1beta1/validatorsets/latest", baseurl))
if err != nil {
t.Fatal(err)
}
q := u.Query()
q.Set("pagination.offset", "0")
q.Set("pagination.limit", "2")
u.RawQuery = q.Encode()
restRes := GetRequest(t, u.String())| if sut.NodesCount() < 2 { | ||
| t.Skip("not enough nodes") | ||
| return | ||
| } |
There was a problem hiding this comment.
Remove redundant return statement after t.Skip()
After calling t.Skip("not enough nodes"), the test execution is halted, so the return statement on line 88 is unnecessary and can be removed.
Apply this diff to remove the unnecessary return:
if sut.NodesCount() < 2 {
t.Skip("not enough nodes")
- return
}📝 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.
| if sut.NodesCount() < 2 { | |
| t.Skip("not enough nodes") | |
| return | |
| } | |
| if sut.NodesCount() < 2 { | |
| t.Skip("not enough nodes") | |
| } |
julienrbrt
added
the
backport/v0.52.x
| @@ -17,25 +17,28 @@ import ( | |||
| ) | |||
|
|
|||
| func TestChainUpgrade(t *testing.T) { | |||
| // err> panic: failed to load latest version: failed to load store: initial version set to 22, but found earlier version 1 [cosmossdk.io/[email protected]/rootmulti/store.go:256] | |||
There was a problem hiding this comment.
this probably needs to be reverted in the 0.52 backport
(cherry picked from commit b33484a) # Conflicts: # tests/systemtests/cometbft_client_test.go
Co-authored-by: Alexander Peters <[email protected]> Co-authored-by: Julien Robert <[email protected]>
Description
Fixes system tests
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
Tests