test: migrate e2e/authz to system tests #21819
Conversation
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request involves migrating end-to-end (E2E) tests for authorization from the Changes
Assessment against linked issues
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Outside diff range and nitpick comments (3)
tests/systemtests/test_runner.go (1)
140-152: LGTM: Well-implemented RunGRPCQueries function with a minor suggestionThe
RunGRPCQueriesfunction is well-implemented and aligns with the PR objective of migrating e2e tests to system tests. It effectively uses subtests and proper error handling.Consider adding a timeout mechanism for GRPC requests to prevent tests from hanging indefinitely in case of network issues. You could use a context with timeout:
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() resp, err := testutil.GetRequestWithContext(ctx, tc.url)This change would require updating the
testutil.GetRequestfunction to accept a context parameter.tests/systemtests/bank_test.go (1)
Line range hint
286-304: LGTM: Improved test case structure and executionThe refactoring of denom metadata test cases to use the
GRPCTestCasestruct is a good improvement. It enhances code organization and reusability. The use ofRunGRPCQueriesfor test execution likely simplifies the testing process.This change aligns well with the PR objective of rewriting the e2e test as a system test.
Consider adding a comment explaining the
GRPCTestCasestruct fields for better readability, especially for developers new to the codebase.tests/systemtests/authz_test.go (1)
5-13: Organize Import Statements According to Go StandardsEnsure that import statements are organized with standard library packages first, followed by third-party packages, and local packages last. Also, separate them with blank lines for better readability.
Organize imports as:
import ( "fmt" "testing" "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/tidwall/gjson" )
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (8)
- tests/e2e/authz/cli_test.go (0 hunks)
- tests/e2e/authz/grpc.go (0 hunks)
- tests/e2e/authz/tx.go (0 hunks)
- tests/systemtests/authz_test.go (1 hunks)
- tests/systemtests/bank_test.go (4 hunks)
- tests/systemtests/system.go (3 hunks)
- tests/systemtests/test_runner.go (2 hunks)
- x/authz/module/autocli.go (1 hunks)
Files not reviewed due to no reviewable changes (3)
- tests/e2e/authz/cli_test.go
- tests/e2e/authz/grpc.go
- tests/e2e/authz/tx.go
Additional context used
Path-based instructions (5)
tests/systemtests/authz_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tests/systemtests/bank_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"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/test_runner.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"x/authz/module/autocli.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (9)
x/authz/module/autocli.go (1)
59-59: LGTM! Consider verifying impact and updating docs.The removal of
Varargs: truefor themsgsargument aligns with the PR objectives of migrating e2e tests to system tests by potentially enforcing stricter input requirements. This change looks good and is likely to improve the robustness of the system tests.To ensure this change doesn't break existing functionality:
- Verify that all existing usages of this command in the codebase and tests have been updated to comply with the new input format.
- Update any relevant documentation or examples that may reference the old varargs behavior.
Here's a script to help verify the usage:
Please review the results of this script to ensure all usages are compatible with the new input format.
Verification successful
Verified! No issues found with the
authz execusage.The removal of
Varargs: truefor themsgsargument has been successfully validated against the existing codebase and tests. All current usages of theauthz execcommand conform to the new single-argument requirement, ensuring that the change aligns with the PR objectives without introducing regressions.
- No instances of multiple
msg-json-filearguments were found.- Deprecated
legacy-execcommands appropriately guide towards the updatedexecusage.- Tests are in place to support the new command structure.
Please proceed with merging this change and ensure that all relevant documentation is updated to reflect the enforced single argument requirement for the
authz execcommand.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of the 'authz exec' command in Go files and tests # Search in Go files echo "Searching in Go files:" rg --type go 'authz\s+exec' -C 3 # Search in test files echo "Searching in test files:" rg --type-add 'test:*.{go,t,sh}' --type test 'authz\s+exec' -C 3Length of output: 3625
tests/systemtests/test_runner.go (2)
134-138: LGTM: Well-structured GRPCTestCase structThe
GRPCTestCasestruct is well-defined and aligns with the PR objective of migrating e2e tests to system tests. The field names are clear and self-explanatory, making it easy to understand and use in test scenarios.
133-168: Summary: Effective implementation supporting e2e to system test migrationThe changes in this file effectively support the PR objective of migrating e2e tests to system tests. The new
GRPCTestCasestruct,RunGRPCQueriesfunction, andWriteToTempJSONFilefunction enhance the testing capabilities and provide a solid foundation for implementing system tests.These additions align well with the coding guidelines for test files and demonstrate good practices in test implementation, such as using subtests, proper error handling, and test isolation.
To ensure that these changes are properly integrated and utilized, please run the following command to check for any new system tests that use these new functions:
This will help verify that the new functionality is being used in actual system tests, furthering the PR's objective of migrating e2e tests to system tests.
Verification successful
Verification Successful: New Functions Utilized in System Tests
The shell script confirmed that both
RunGRPCQueriesandWriteToTempJSONFileare actively used intests/systemtests/bank_test.goandtests/systemtests/authz_test.go. This verifies that the new implementations are integrated effectively, supporting the migration from e2e tests to system tests as intended.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for new system tests using the new functions rg --type go 'RunGRPCQueries|WriteToTempJSONFile' tests/systemtestsLength of output: 1158
tests/systemtests/bank_test.go (3)
227-227: LGTM: Improved flexibility in obtaining API addressThe change to use
sut.APIAddress()for obtaining the base URL is a good improvement. It enhances flexibility and maintainability by using a method to retrieve the API address instead of hard-coding it.
Line range hint
1-328: Summary: Successful migration of bank GRPC queries to system testsThe changes in this file successfully achieve the PR objective of migrating e2e tests to system tests for bank GRPC queries. Key improvements include:
- Enhanced flexibility in obtaining the API address.
- Improved test case structure using the
GRPCTestCasestruct for both denom metadata and balance tests.- Consistent use of
RunGRPCQueriesfor test execution.These changes not only fulfill the migration requirement but also improve code organization, reusability, and maintainability. The refactoring aligns well with Go best practices.
To fully complete the PR objectives, ensure that:
- The old e2e test has been removed (as verified by the previous script).
- The CI process successfully runs these tests with both version 1 and version 2 of the software.
Line range hint
310-328: LGTM: Consistent improvement in test case structureThe refactoring of balance test cases to use the
GRPCTestCasestruct is consistent with the earlier changes and improves code organization. The use ofRunGRPCQueriesfor test execution maintains consistency across different test types.This change aligns well with the PR objective of rewriting the e2e test as a system test.
To ensure the completeness of the migration, let's verify that the old e2e test has been removed as mentioned in the PR objectives. Run the following script:
Verification successful
✅ Verification Successful: Old e2e test files have been removed
The executed shell script confirmed that no old e2e test files containing bank GRPC queries exist in the
tests/e2edirectory. This confirms that the migration to system tests is complete and aligns with the PR objectives.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the old e2e test file for bank queries has been removed # Search for any file in the e2e directory that might contain bank GRPC queries result=$(find tests/e2e -type f -exec grep -l "bank.*GRPC" {} +) if [ -n "$result" ]; then echo "Potential old e2e test file(s) found:" echo "$result" else echo "No potential old e2e test files found. Migration seems complete." fiLength of output: 224
tests/systemtests/system.go (3)
56-56: LGTM: Addition ofapiAddrfieldThe addition of the
apiAddrfield to theSystemUnderTeststruct is appropriate and well-placed. It's consistent with the structure's purpose and will allow for storing and accessing the API address.
626-628: LGTM: Addition ofAPIAddressmethodThe
APIAddressmethod is a well-implemented getter for theapiAddrfield. It follows Go conventions and provides a clean way to access the API address.
90-90: Verify theapiPortStartvariableThe initialization of
apiAddruses a variableapiPortStartwhich is not defined in the visible code. This could lead to compilation errors or unexpected behavior.Please ensure that
apiPortStartis properly defined and initialized before its use. If it's defined in another file, consider adding a comment to clarify its source. Alternatively, you might want to pass it as a parameter to theNewSystemUnderTestfunction.To verify this, you can run the following command:
tests/systemtests/test_runner.go
Outdated
| func WriteToTempJSONFile(tb testing.TB, s string) *os.File { | ||
| tb.Helper() | ||
|
|
||
| tmpFile, err := os.CreateTemp(tb.TempDir(), "test-*.json") | ||
| require.Nil(tb, err) | ||
| defer tmpFile.Close() | ||
|
|
||
| // Write to the temporary file | ||
| _, err = tmpFile.WriteString(s) | ||
| require.Nil(tb, err) | ||
|
|
||
| return tmpFile | ||
| } |
There was a problem hiding this comment.
LGTM: Well-implemented WriteToTempJSONFile function with a suggestion
The WriteToTempJSONFile function is well-implemented and provides a useful utility for creating temporary JSON files in tests. It properly uses the testing.TB interface and follows good practices for test isolation.
Consider adding an option to close the file before returning it. This would allow the caller to decide whether they need the file open or closed. You could modify the function signature and implementation as follows:
func WriteToTempJSONFile(tb testing.TB, s string, keepOpen bool) *os.File {
// ... existing implementation ...
if !keepOpen {
err = tmpFile.Close()
require.Nil(tb, err)
}
return tmpFile
}This change would provide more flexibility to the function users while maintaining the current functionality.
tests/systemtests/authz_test.go
Outdated
| bankTx := fmt.Sprintf(`{ | ||
| "@type": "%s", | ||
| "from_address": "%s", | ||
| "to_address": "%s", | ||
| "amount": [ | ||
| { | ||
| "denom": "%s", | ||
| "amount": "%d" | ||
| } | ||
| ] | ||
| }`, msgSendTypeURL, granterAddr, tc.toAddr, denom, tc.amount) |
There was a problem hiding this comment.
Use Structs and json.Marshal Instead of fmt.Sprintf for JSON Construction
Constructing JSON messages using fmt.Sprintf can lead to errors if the data contains special characters or unexpected formats. It's recommended to use Go structs and json.Marshal to build JSON messages, which ensures proper encoding and reduces the risk of invalid JSON.
Apply this refactor to construct the JSON message safely:
type BankTx struct {
Type string `json:"@type"`
FromAddress string `json:"from_address"`
ToAddress string `json:"to_address"`
Amount []struct {
Denom string `json:"denom"`
Amount string `json:"amount"`
} `json:"amount"`
}
bankTxStruct := BankTx{
Type: msgSendTypeURL,
FromAddress: granterAddr,
ToAddress: tc.toAddr,
Amount: []struct {
Denom string `json:"denom"`
Amount string `json:"amount"`
}{
{
Denom: denom,
Amount: fmt.Sprintf("%d", tc.amount),
},
},
}
bankTxBytes, err := json.Marshal(bankTxStruct)
require.NoError(t, err)
execMsg := WriteToTempJSONFile(t, string(bankTxBytes))
tests/systemtests/authz_test.go
Outdated
| bankTx := fmt.Sprintf(`{ | ||
| "@type": "%s", | ||
| "from_address": "%s", | ||
| "to_address": "%s", | ||
| "amount": [ | ||
| { | ||
| "denom": "%s", | ||
| "amount": "%d" | ||
| } | ||
| ] | ||
| }`, msgSendTypeURL, granterAddr, allowedAddr, denom, 10) |
There was a problem hiding this comment.
Use Structs and json.Marshal for Consistent JSON Message Construction
Similar to previous instances, constructing JSON using fmt.Sprintf can be error-prone. Using structs with json.Marshal enhances code safety and readability.
Refactor the code as suggested to utilize structs and json.Marshal.
tests/systemtests/authz_test.go
Outdated
| delegateTx := fmt.Sprintf(`{"@type":"%s","delegator_address":"%s","validator_address":"%s","amount":{"denom":"%s","amount":"%d"}}`, | ||
| msgDelegateTypeURL, granterAddr, tc.valAddr, denom, tc.amount) | ||
| execMsg := WriteToTempJSONFile(t, delegateTx) | ||
| defer execMsg.Close() | ||
|
|
||
| cmd := append(append(execCmdArgs, execMsg.Name()), "--from="+tc.grantee) |
There was a problem hiding this comment.
Prefer Using Structs and json.Marshal for Delegate Transactions
Constructing JSON payloads with fmt.Sprintf might introduce formatting errors. Leveraging Go structs and json.Marshal ensures the JSON is correctly formatted.
Implement the refactor using structs:
type DelegateTx struct {
Type string `json:"@type"`
DelegatorAddress string `json:"delegator_address"`
ValidatorAddress string `json:"validator_address"`
Amount struct {
Denom string `json:"denom"`
Amount string `json:"amount"`
} `json:"amount"`
}
delegateTxStruct := DelegateTx{
Type: msgDelegateTypeURL,
DelegatorAddress: granterAddr,
ValidatorAddress: tc.valAddr,
Amount: struct {
Denom string `json:"denom"`
Amount string `json:"amount"`
}{
Denom: denom,
Amount: fmt.Sprintf("%d", tc.amount),
},
}
delegateTxBytes, err := json.Marshal(delegateTxStruct)
require.NoError(t, err)
execMsg := WriteToTempJSONFile(t, string(delegateTxBytes))| require.Equal(t, initialAmount, allowedAddrBal) | ||
|
|
||
| var spendLimitAmount int64 = 1000 | ||
| expirationTime := time.Now().Add(time.Second * 10).Unix() |
There was a problem hiding this comment.
Avoid Using time.Now() Directly in Tests
Using time.Now() in tests can lead to flaky tests due to timing issues. Consider injecting a time provider or using a fixed timestamp to make tests deterministic and reliable.
Also applies to: 449-449
| } | ||
|
|
||
| // test grant expiry | ||
| time.Sleep(time.Second * 10) |
There was a problem hiding this comment.
Minimize Use of time.Sleep in Tests
Using time.Sleep can slow down tests and cause flakiness. It's better to mock time or adjust the logic to wait for events or conditions instead of fixed sleep durations.
Also applies to: 467-467
| valOperAddr := gjson.Get(rsp, "validators.#.operator_address").Array()[0].String() | ||
|
|
There was a problem hiding this comment.
Check for Empty Validator List Before Accessing Elements
Directly accessing the first element of the validators array without checking if it's empty can cause a panic if the array is empty. Ensure that the array has elements before accessing.
Modify the code to include a check:
validators := gjson.Get(rsp, "validators.#.operator_address").Array()
require.GreaterOrEqual(t, len(validators), 1)
valOperAddr := validators[0].String()
tests/systemtests/authz_test.go
Outdated
| true, | ||
| "decoding bech32 failed", | ||
| false, | ||
| }, | ||
| { | ||
| "redelegate authorization with invalid deny validator address", | ||
| grantee1Addr, | ||
| []string{"redelegate", "--deny-validators=invalid"}, | ||
| true, | ||
| "decoding bech32 failed", | ||
| false, | ||
| }, | ||
| { | ||
| "valid send authorization", | ||
| grantee1Addr, | ||
| []string{"send", "--spend-limit=1000stake"}, | ||
| false, | ||
| "", | ||
| false, | ||
| }, | ||
| { | ||
| "valid send authorization with expiration", | ||
| grantee2Addr, | ||
| []string{"send", "--spend-limit=1000stake", fmt.Sprintf("--expiration=%d", expirationTime)}, | ||
| false, | ||
| "", | ||
| false, | ||
| }, | ||
| { | ||
| "valid generic authorization", | ||
| grantee3Addr, | ||
| []string{"generic", "--msg-type=" + msgVoteTypeURL}, | ||
| false, | ||
| "", | ||
| false, | ||
| }, | ||
| { | ||
| "valid delegate authorization", | ||
| grantee4Addr, | ||
| []string{"delegate", "--allowed-validators=" + valOperAddr}, | ||
| false, | ||
| "", | ||
| false, | ||
| }, | ||
| { | ||
| "valid unbond authorization", | ||
| grantee5Addr, | ||
| []string{"unbond", "--deny-validators=" + valOperAddr}, | ||
| false, | ||
| "", | ||
| false, | ||
| }, | ||
| { | ||
| "valid redelegate authorization", | ||
| grantee6Addr, | ||
| []string{"redelegate", "--allowed-validators=" + valOperAddr}, | ||
| false, | ||
| "", | ||
| false, | ||
| }, | ||
| } | ||
|
|
||
| grantsCount := 0 | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| cmd := append(append(grantCmdArgs, tc.grantee), tc.cmdArgs...) | ||
| if tc.expectErr { | ||
| if tc.queryTx { | ||
| rsp := cli.Run(cmd...) | ||
| RequireTxFailure(t, rsp) | ||
| require.Contains(t, rsp, tc.expErrMsg) | ||
| } else { | ||
| assertErr := func(_ assert.TestingT, gotErr error, gotOutputs ...interface{}) bool { | ||
| require.Len(t, gotOutputs, 1) | ||
| output := gotOutputs[0].(string) | ||
| require.Contains(t, output, tc.expErrMsg) | ||
| return false | ||
| } | ||
| _ = cli.WithRunErrorMatcher(assertErr).Run(cmd...) | ||
| } | ||
| } else { | ||
| rsp := cli.RunAndWait(cmd...) | ||
| RequireTxSuccess(t, rsp) | ||
|
|
||
| // query granter-grantee grants | ||
| resp := cli.CustomQuery("q", "authz", "grants", granterAddr, tc.grantee) | ||
| grants := gjson.Get(resp, "grants").Array() | ||
| // check grants length equal to 1 to confirm grant created successfully | ||
| require.Len(t, grants, 1) | ||
| grantsCount++ | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| // query grants-by-granter | ||
| resp := cli.CustomQuery("q", "authz", "grants-by-granter", granterAddr) | ||
| grants := gjson.Get(resp, "grants").Array() | ||
| require.Len(t, grants, grantsCount) | ||
| } |
There was a problem hiding this comment.
Split Long Test Function into Smaller, Focused Tests
The TestAuthzGrantTxCmd function is lengthy and covers multiple scenarios. Consider splitting it into smaller test functions, each focusing on a specific aspect or scenario. This improves readability and makes maintenance easier.
tests/systemtests/authz_test.go
Outdated
| time.Sleep(time.Second * 10) | ||
| bankTx := fmt.Sprintf(`{ |
There was a problem hiding this comment.
Use Time Mocking Instead of time.Sleep for Expiration Tests
Relying on time.Sleep to test expiration can make tests slow and unreliable. Use a time mocking library or dependency injection to simulate time passage, making the tests faster and more stable.
Consider using a time mocking library like github.com/benbjohnson/clock to control time in tests.
Also applies to: 467-468
| if tc.queryTx { | ||
| rsp := cli.Run(cmd...) | ||
| RequireTxFailure(t, rsp) | ||
| require.Contains(t, rsp, tc.expErrMsg) | ||
| } else { | ||
| assertErr := func(_ assert.TestingT, gotErr error, gotOutputs ...interface{}) bool { | ||
| require.Len(t, gotOutputs, 1) | ||
| output := gotOutputs[0].(string) | ||
| require.Contains(t, output, tc.expErrMsg) | ||
| return false | ||
| } | ||
| _ = cli.WithRunErrorMatcher(assertErr).Run(cmd...) | ||
| } |
There was a problem hiding this comment.
Simplify Error Handling in Test Cases
The error handling logic within the test cases can be simplified for clarity. Instead of defining an anonymous function for error matching, consider using require.ErrorContains or similar assertions provided by the testify package.
Refactor the error assertions:
if tc.expectErr {
rsp := cli.Run(cmd...)
RequireTxFailure(t, rsp)
require.Contains(t, rsp, tc.expErrMsg)
} else {
rsp := cli.RunAndWait(cmd...)
RequireTxSuccess(t, rsp)
// Additional success assertions...
}
tests/systemtests/test_runner.go
Outdated
| @@ -127,3 +130,39 @@ func printResultFlag(ok bool) { | |||
| fmt.Println(failureFlag) | |||
| } | |||
| } | |||
|
|
|||
| type GRPCTestCase struct { | |||
There was a problem hiding this comment.
Why GRPC? Isn't this a REST api?
personal preference:
Adding more framework code for common operations makes sense. I am wondering if it would be more beneficial to add them to a new file. The type and method added in this PR could fit into some rest client helper type.
There was a problem hiding this comment.
Sure. Even though it's REST api, named it as GRPC as it runs using grpc gateway endpoints.
tests/systemtests/test_runner.go
Outdated
| t.Run(tc.name, func(t *testing.T) { | ||
| resp, err := testutil.GetRequest(tc.url) | ||
| require.NoError(t, err) | ||
| require.Contains(t, string(resp), tc.expOut) |
There was a problem hiding this comment.
This is a weak check. Would it make sense to use require.JSONEq( ) instead?
tests/systemtests/test_runner.go
Outdated
|
|
||
| // Write the given string to a new temporary json file. | ||
| // Returns an file for the test to use. | ||
| func WriteToTempJSONFile(tb testing.TB, s string) *os.File { |
There was a problem hiding this comment.
nit: the method makes no assumption on the file content. Why limit this to json only?
It is used in authz_test.go only. We can move it there to start with until a second use case pops up.
There was a problem hiding this comment.
We already have a method WriteToTempFile in testutil which writes given data to a temp file. But for authz exec command, it supports only json file, so I wrote a separate method for it. Will move this method to authz_test.go
tests/systemtests/test_runner.go
Outdated
| tb.Helper() | ||
|
|
||
| tmpFile, err := os.CreateTemp(tb.TempDir(), "test-*.json") | ||
| require.Nil(tb, err) |
There was a problem hiding this comment.
here and others: this give a better failure message
| require.Nil(tb, err) | |
| require.NoError(tb, err) |
| @@ -224,7 +224,7 @@ func TestBankGRPCQueries(t *testing.T) { | |||
|
|
|||
| // start chain | |||
| sut.StartChain(t) | |||
| baseurl := fmt.Sprintf("http://localhost:%d", apiPortStart) | |||
| baseurl := sut.APIAddress() | |||
There was a problem hiding this comment.
👍 to have this in a single place
tests/systemtests/authz_test.go
Outdated
| _ = cli.WithRunErrorMatcher(assertErr).Run(cmd...) | ||
| } | ||
| } else { | ||
| rsp := cli.RunAndWait(cmd...) |
There was a problem hiding this comment.
nit: you can use cli.RunAndWait(cmd...) for the happy path, now.
There was a problem hiding this comment.
Just to confirm, is this what is being used here?
tests/systemtests/authz_test.go
Outdated
| cli := NewCLIWrapper(t, sut, verbose) | ||
|
|
||
| // get validator address which will be used as granter | ||
| granterAddr := gjson.Get(cli.Keys("keys", "list"), "1.address").String() |
tests/systemtests/authz_test.go
Outdated
| ] | ||
| }`, msgSendTypeURL, granterAddr, tc.toAddr, denom, tc.amount) | ||
| execMsg := WriteToTempJSONFile(t, bankTx) | ||
| defer execMsg.Close() |
There was a problem hiding this comment.
nit: the file is closed already. No need to do it here again
| } | ||
|
|
||
| // test grant expiry | ||
| time.Sleep(time.Second * 10) |
There was a problem hiding this comment.
The time can not be mocked here but maybe test setups can be re-arranged to reduce the total sleep time. For example, first setup the 2 grants that will timeout, run some other tests, wait for the timeout to happen.
The SUT does not provide a CurrentBlockTime() method yet. But we could extend the block listener to also store the block time from the header. WDYT?
There was a problem hiding this comment.
Yeah, may be works.
| resp = cli.CustomQuery("q", "authz", "grants-by-granter", granterAddr) | ||
| grants = gjson.Get(resp, "grants").Array() | ||
| require.Len(t, grants, 0) | ||
| } |
There was a problem hiding this comment.
This was a very big test. Although I do understand that all the cases share the exec command, do you see some way to simplify this or split into multiple tests? You are covering send, generic, delegate, undelegate
as well as use cases like expired and and revoked. I would expect most of this is covered by unit tests already.
There was a problem hiding this comment.
I am trying to migrate tests already present in tests/e2e. So, I had implemented all those which were present. Will try to split them into multiple tests.
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (2)
tests/systemtests/rest_cli.go (2)
10-14: Consider adding comments to improve documentation.While the struct definition is clear, it would be beneficial to add comments describing the purpose of the
GRPCTestCasestruct and its fields. This will enhance code readability and maintainability.Consider adding comments like this:
// GRPCTestCase represents a test case for gRPC queries. type GRPCTestCase struct { name string // Name of the test case url string // URL for the gRPC request expOut string // Expected output of the request }
16-28: LGTM: Well-implemented test helper function.The
RunGRPCQueriesfunction is well-structured and follows Go conventions. It correctly usest.Helper()and provides proper error handling and assertions.Consider adding a brief comment for the
t.Runblock to explain its purpose:for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { // Run individual test case resp, err := testutil.GetRequest(tc.url) require.NoError(t, err) require.JSONEq(t, tc.expOut, string(resp)) }) }
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- tests/systemtests/authz_test.go (1 hunks)
- tests/systemtests/bank_test.go (6 hunks)
- tests/systemtests/rest_cli.go (1 hunks)
Additional context used
Path-based instructions (3)
tests/systemtests/authz_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tests/systemtests/bank_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"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
776-776: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Additional comments not posted (10)
tests/systemtests/rest_cli.go (2)
1-8: LGTM: Package declaration and imports are well-structured.The package name is appropriate, and imports are correctly grouped and used.
1-28: Ensure comprehensive test coverage in implementation.The
RunGRPCQueriesfunction provides a solid framework for running gRPC test cases, which is good for system testing. It integrates well with the cosmos-sdk testutil package and is flexible for various gRPC endpoints.To ensure comprehensive test coverage:
- Implement actual test cases using this framework in a separate file.
- Verify that all relevant gRPC endpoints are covered.
Run the following script to check for test implementations:
If no results are found, consider adding test implementations to utilize this framework effectively.
tests/systemtests/bank_test.go (5)
24-24: Improved validator address retrievalThe change from parsing
cli.Keys("keys", "list")output to usingcli.GetKeyAddr("node0")is a good improvement. It simplifies the code and reduces the potential for errors in parsing. This change aligns well with the PR objective of improving test efficiency without compromising test coverage.
Line range hint
157-187: Enhanced error handling and balance verification in TestBankMultiSendTxCmdThe changes in this section significantly improve the test quality:
- The error handling is now more precise, checking for specific error messages instead of a boolean flag.
- For successful transactions, detailed balance checks have been added for all involved accounts.
- The balance updates are tracked throughout the test, ensuring consistency.
These improvements enhance the test coverage and align well with the PR objective of improving test quality. The additional checks provide better verification of the multi-send functionality.
221-221: Improved API base URL retrievalThe change to use
sut.APIAddress()for obtaining the base URL is a good improvement. This approach makes the test more flexible and less prone to errors that could arise from hardcoded URLs. It aligns with best practices for system tests by using dynamic configuration, which can adapt to different test environments more easily.
280-298: Refactored denom metadata test casesThe changes in this section improve the structure and efficiency of the tests:
- Test cases now use the
GRPCTestCasestruct, enhancing code reusability and readability.- The loop for executing test cases has been replaced with a call to
RunGRPCQueries, which likely encapsulates common test execution logic.These improvements align well with the PR objective of enhancing test structure and efficiency. The use of a common struct and execution function reduces code duplication and makes it easier to add or modify test cases in the future.
Line range hint
304-322: Refactored balance query test casesThe changes in this section mirror the improvements made to the denom metadata test cases:
- Balance query test cases now use the
GRPCTestCasestruct, enhancing code reusability and readability.- The execution of test cases has been simplified by using the
RunGRPCQueriesfunction.These changes maintain consistency with the previous improvements and further contribute to the overall enhancement of test structure and efficiency. This refactoring makes the tests more maintainable and easier to extend in the future.
tests/systemtests/authz_test.go (3)
53-54: Check for Empty Validator List Before Accessing ElementsDirectly accessing the first element of the validators array without checking if it's empty can cause a panic if the array is empty. Ensure that the array has elements before accessing.
207-219: Simplify Error Handling in Test CasesThe error handling logic within the test cases can be simplified for clarity. Instead of defining an anonymous function for error matching, consider using
require.ErrorContainsor similar assertions provided by thetestifypackage.
401-401: Avoid Usingtime.Sleepin TestsUsing
time.Sleepcan slow down tests and cause flakiness. It's better to mock time or adjust the logic to wait for events or conditions instead of fixed sleep durations.Also applies to: 440-440
tests/systemtests/authz_test.go
Outdated
| granterAddr := cli.GetKeyAddr("node0") | ||
| require.NotEmpty(t, granterAddr) | ||
| granteeAddr := cli.GetKeyAddr("node1") | ||
| require.NotEmpty(t, granterAddr) |
There was a problem hiding this comment.
Fix Incorrect Variable in Validation
In the setupChain function, after retrieving granteeAddr, you're checking require.NotEmpty(t, granterAddr) again instead of granteeAddr. This appears to be a copy-paste error and may cause the test to miss a potential empty granteeAddr.
Apply this diff to correct the variable:
granteeAddr := cli.GetKeyAddr("node1")
-require.NotEmpty(t, granterAddr)
+require.NotEmpty(t, granteeAddr)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.
| granterAddr := cli.GetKeyAddr("node0") | |
| require.NotEmpty(t, granterAddr) | |
| granteeAddr := cli.GetKeyAddr("node1") | |
| require.NotEmpty(t, granterAddr) | |
| granterAddr := cli.GetKeyAddr("node0") | |
| require.NotEmpty(t, granterAddr) | |
| granteeAddr := cli.GetKeyAddr("node1") | |
| require.NotEmpty(t, granteeAddr) |
tests/systemtests/authz_test.go
Outdated
| tb.Helper() | ||
|
|
||
| tmpFile, err := os.CreateTemp(tb.TempDir(), "test-*.json") | ||
| require.NoError(tb, err) | ||
| defer tmpFile.Close() | ||
|
|
||
| // Write to the temporary file | ||
| _, err = tmpFile.WriteString(s) | ||
| require.NoError(tb, err) | ||
|
|
||
| return tmpFile |
There was a problem hiding this comment.
Avoid Closing the File Before Returning It
In the WriteToTempJSONFile function, you defer tmpFile.Close() but then return the *os.File. Closing the file before returning it may lead to unexpected behavior if the calling function tries to use the file. Consider closing the file after you are done using it or adjusting the function to return the file name instead.
Apply this diff to fix the issue:
func WriteToTempJSONFile(tb testing.TB, s string) *os.File {
tb.Helper()
tmpFile, err := os.CreateTemp(tb.TempDir(), "test-*.json")
require.NoError(tb, err)
- defer tmpFile.Close()
// Write to the temporary file
_, err = tmpFile.WriteString(s)
require.NoError(tb, err)
+ // Close the file after writing
+ err = tmpFile.Close()
+ require.NoError(tb, err)
+
return tmpFile
}And update the usage if necessary to accommodate the change.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
tests/systemtests/authz_test.go (1)
58-58: Avoid usingtime.Now()directly in testsUsing
time.Now()in tests can lead to non-deterministic behavior and flaky tests. Consider injecting a mock time or using a fixed timestamp to make the tests deterministic and reliable.Refactor to use a fixed timestamp:
-expirationTime := time.Now().Add(time.Hour).Unix() +fixedTime := time.Unix(1609459200, 0) // Fixed timestamp +expirationTime := fixedTime.Add(time.Hour).Unix()
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (4)
- tests/systemtests/authz_test.go (1 hunks)
- tests/systemtests/bank_test.go (6 hunks)
- tests/systemtests/rest_cli.go (1 hunks)
- tests/systemtests/system.go (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/systemtests/bank_test.go
- tests/systemtests/rest_cli.go
- tests/systemtests/system.go
🧰 Additional context used
📓 Path-based instructions (1)
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"
Gitleaks
tests/systemtests/authz_test.go
777-777: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments not posted (2)
tests/systemtests/authz_test.go (2)
493-497: Verify the expected error message for exceeding spend limitIn the test case "amount greater than spend limit", the expected error message is set to "negative coin amount". However, providing an amount greater than the spend limit might result in a different error, such as "requested amount is more than spend limit". Please verify that the error message matches the actual error produced when the amount exceeds the spend limit.
55-56:⚠️ Potential issueCheck for empty validator list before accessing elements
Directly accessing the first element of the validators array without checking if it's empty can cause a panic if the array is empty. Ensure that the array has elements before accessing to prevent potential runtime errors.
Modify the code to include a check:
rsp := cli.CustomQuery("q", "staking", "validators") +validators := gjson.Get(rsp, "validators.#.operator_address").Array() +require.GreaterOrEqual(t, len(validators), 1) -valOperAddr := gjson.Get(rsp, "validators.#.operator_address").Array()[0].String() +valOperAddr := validators[0].String()Likely invalid or redundant comment.
| expirationTime := time.Now().Add(time.Second * 5).Unix() | ||
| execSendCmd := msgSendExec(t, granterAddr, granteeAddr, allowedAddr, testDenom, 10) | ||
|
|
||
| // create generic authorization grant | ||
| rsp := cli.RunAndWait("tx", "authz", "grant", granteeAddr, "generic", | ||
| "--msg-type="+msgSendTypeURL, | ||
| "--expiration="+fmt.Sprintf("%d", expirationTime), | ||
| "--fees=1"+testDenom, | ||
| "--from", granterAddr) | ||
| RequireTxSuccess(t, rsp) | ||
| granterBal-- | ||
|
|
||
| rsp = cli.RunAndWait(execSendCmd...) | ||
| RequireTxSuccess(t, rsp) | ||
| // check granter balance equals to granterBal - transferredAmount | ||
| expGranterBal := granterBal - 10 | ||
| require.Equal(t, expGranterBal, cli.QueryBalance(granterAddr, testDenom)) | ||
|
|
||
| time.Sleep(time.Second * 5) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce dependency on real time in tests
Using real time functions like time.Now() and time.Sleep() can cause tests to be slow and flaky. Instead, mock the time or use a testing clock to simulate the passage of time, which will make the tests faster and more reliable.
Consider using a mocked time package or dependency injection for time.
| tmpFile, err := os.CreateTemp(tb.TempDir(), "test-*.json") | ||
| require.NoError(tb, err) | ||
|
|
||
| // Write to the temporary file | ||
| _, err = tmpFile.WriteString(s) | ||
| require.NoError(tb, err) | ||
|
|
||
| // Close the file after writing | ||
| err = tmpFile.Close() | ||
| require.NoError(tb, err) | ||
|
|
||
| return tmpFile |
There was a problem hiding this comment.
Handle file closure properly in WriteToTempJSONFile
Currently, the temporary file is closed before it is used by the calling function, which may lead to errors when the file is accessed later. Instead of closing the file within WriteToTempJSONFile, defer the closure or leave it to the caller to close the file after use.
Apply this change to defer closing the file:
func WriteToTempJSONFile(tb testing.TB, s string) *os.File {
tb.Helper()
tmpFile, err := os.CreateTemp(tb.TempDir(), "test-*.json")
require.NoError(tb, err)
+ // Caller is responsible for closing the file
- err = tmpFile.Close()
- require.NoError(tb, err)
// Write to the temporary file
_, err = tmpFile.WriteString(s)
require.NoError(tb, err)
- // Close the file after writing
- err = tmpFile.Close()
- require.NoError(tb, err)
return tmpFile
}📝 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.
| tmpFile, err := os.CreateTemp(tb.TempDir(), "test-*.json") | |
| require.NoError(tb, err) | |
| // Write to the temporary file | |
| _, err = tmpFile.WriteString(s) | |
| require.NoError(tb, err) | |
| // Close the file after writing | |
| err = tmpFile.Close() | |
| require.NoError(tb, err) | |
| return tmpFile | |
| func WriteToTempJSONFile(tb testing.TB, s string) *os.File { | |
| tb.Helper() | |
| tmpFile, err := os.CreateTemp(tb.TempDir(), "test-*.json") | |
| require.NoError(tb, err) | |
| // Caller is responsible for closing the file | |
| // Write to the temporary file | |
| _, err = tmpFile.WriteString(s) | |
| require.NoError(tb, err) | |
| return tmpFile | |
| } |
| time.Sleep(time.Second * 10) | ||
|
|
There was a problem hiding this comment.
Avoid using time.Sleep in tests
Using time.Sleep can make tests slower and lead to flakiness. Consider mocking time or restructuring the test to wait for specific events or conditions instead of a fixed sleep duration. This will improve the reliability and efficiency of the test.
Apply this refactor to remove time.Sleep:
-func TestAuthzExecSendAuthorization(t *testing.T) {
...
- // test grant expiry
- time.Sleep(time.Second * 10)
+ // test grant expiry
+ waitForGrantExpiry()
...
+func waitForGrantExpiry() {
+ // Implement logic to wait until the grant has expired without using time.Sleep
+}Committable suggestion was skipped due to low confidence.
x/authz/module/autocli.go
Outdated
| @@ -56,7 +56,7 @@ func (am AppModule) AutoCLIOptions() *autocliv1.ModuleOptions { | |||
| Short: "Execute tx on behalf of granter account", | |||
| Example: fmt.Sprintf("$ %s tx authz exec msg.json --from grantee\n $ %[1]s tx bank send [granter] [recipient] [amount] --generate-only | jq .body.messages > msg.json && %[1]s tx authz exec msg.json --from grantee", version.AppName), | |||
| PositionalArgs: []*autocliv1.PositionalArgDescriptor{ | |||
| {ProtoField: "msgs", Varargs: true}, | |||
| {ProtoField: "msgs"}, | |||
There was a problem hiding this comment.
I think it should be varargs, as it can take multiple msgs
julienrbrt
added
the
backport/v0.52.x
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
- tests/systemtests/authz_test.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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"
Gitleaks
tests/systemtests/authz_test.go
725-725: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments not posted (2)
tests/systemtests/authz_test.go (2)
27-233: Well-Structured Tests with Comprehensive CoverageThe
TestAuthzGrantTxCmdfunction is well-organized, utilizing table-driven tests to cover various scenarios effectively. It enhances readability and maintainability by systematically testing both valid and invalid authorization grants.
725-725: False Positive Alert from Static AnalysisThe static analysis hint regarding a Generic API Key on line 725 appears to be a false positive. The
next_keyin the pagination response is a base64-encoded string for pagination purposes and does not expose any sensitive information.🧰 Tools
Gitleaks
725-725: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
| sut.ModifyGenesisCLI(t, | ||
| []string{"genesis", "add-genesis-account", granteeAddr, initialBalance}, | ||
| []string{"genesis", "add-genesis-account", allowedAddr, initialBalance}, | ||
| []string{"genesis", "add-genesis-account", newAccount, initialBalance}, | ||
| ) | ||
| sut.StartChain(t) | ||
|
|
||
| // query balances | ||
| granterBal := cli.QueryBalance(granterAddr, testDenom) | ||
| granteeBal := cli.QueryBalance(granteeAddr, testDenom) | ||
| require.Equal(t, initialAmount, granteeBal) | ||
| allowedAddrBal := cli.QueryBalance(allowedAddr, testDenom) | ||
| require.Equal(t, initialAmount, allowedAddrBal) | ||
|
|
||
| var spendLimitAmount int64 = 1000 | ||
| expirationTime := time.Now().Add(time.Second * 10).Unix() | ||
|
|
||
| // test exec send authorization | ||
|
|
||
| // create send authorization grant | ||
| rsp := cli.RunAndWait("tx", "authz", "grant", granteeAddr, "send", | ||
| "--spend-limit="+fmt.Sprintf("%d%s", spendLimitAmount, testDenom), | ||
| "--allow-list="+allowedAddr, | ||
| "--expiration="+fmt.Sprintf("%d", expirationTime), | ||
| "--fees=1"+testDenom, | ||
| "--from", granterAddr) | ||
| RequireTxSuccess(t, rsp) | ||
| // reduce fees of above tx from granter balance | ||
| granterBal-- | ||
|
|
||
| testCases := []struct { | ||
| name string | ||
| grantee string | ||
| toAddr string | ||
| amount int64 | ||
| expErrMsg string | ||
| }{ | ||
| { | ||
| "valid exec transaction", | ||
| granteeAddr, | ||
| allowedAddr, | ||
| 20, | ||
| "", | ||
| }, | ||
| { | ||
| "send to not allowed address", | ||
| granteeAddr, | ||
| notAllowedAddr, | ||
| 10, | ||
| "cannot send to", | ||
| }, | ||
| { | ||
| "amount greater than spend limit", | ||
| granteeAddr, | ||
| allowedAddr, | ||
| spendLimitAmount + 5, | ||
| "requested amount is more than spend limit", | ||
| }, | ||
| { | ||
| "no grant found", | ||
| newAccount, | ||
| granteeAddr, | ||
| 20, | ||
| "authorization not found", | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| // msg send | ||
| cmd := msgSendExec(t, granterAddr, tc.grantee, tc.toAddr, testDenom, tc.amount) | ||
| if tc.expErrMsg != "" { | ||
| rsp := cli.Run(cmd...) | ||
| RequireTxFailure(t, rsp) | ||
| require.Contains(t, rsp, tc.expErrMsg) | ||
| return | ||
| } | ||
| rsp := cli.RunAndWait(cmd...) | ||
| RequireTxSuccess(t, rsp) | ||
|
|
||
| // check granter balance equals to granterBal - transferredAmount | ||
| expGranterBal := granterBal - tc.amount | ||
| require.Equal(t, expGranterBal, cli.QueryBalance(granterAddr, testDenom)) | ||
| granterBal = expGranterBal | ||
|
|
||
| // check allowed addr balance equals to allowedAddrBal + transferredAmount | ||
| expAllowAddrBal := allowedAddrBal + tc.amount | ||
| require.Equal(t, expAllowAddrBal, cli.QueryBalance(allowedAddr, testDenom)) | ||
| allowedAddrBal = expAllowAddrBal | ||
| }) | ||
| } | ||
|
|
||
| // test grant expiry | ||
| time.Sleep(time.Second * 10) | ||
|
|
||
| execSendCmd := msgSendExec(t, granterAddr, granteeAddr, allowedAddr, testDenom, 10) | ||
| rsp = cli.Run(execSendCmd...) | ||
| RequireTxFailure(t, rsp) | ||
| require.Contains(t, rsp, "authorization not found") | ||
| } |
There was a problem hiding this comment.
Consider Refactoring to Avoid time.Sleep for Better Test Reliability
The use of time.Sleep at line 351 can lead to slow and flaky tests. Consider refactoring the test to simulate time passage or wait for specific conditions without pausing execution. This will improve test reliability and speed.
To address this, you might use a time mocking library or adjust the test setup to avoid the need for waiting periods.
| func setupChain(t *testing.T) (*CLIWrapper, string, string) { | ||
| t.Helper() | ||
|
|
||
| sut.ResetChain(t) | ||
| cli := NewCLIWrapper(t, sut, verbose) | ||
|
|
||
| require.GreaterOrEqual(t, cli.nodesCount, 2) | ||
|
|
||
| // get validators' address which will be used as granter and grantee | ||
| granterAddr := cli.GetKeyAddr("node0") | ||
| require.NotEmpty(t, granterAddr) | ||
| granteeAddr := cli.GetKeyAddr("node1") | ||
| require.NotEmpty(t, granteeAddr) | ||
|
|
||
| sut.StartChain(t) | ||
|
|
||
| return cli, granterAddr, granteeAddr | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance Reusability by Refactoring setupChain Function
The setupChain function initializes the chain and retrieves addresses. Similar setup code appears in multiple test functions. Consider refactoring common initialization steps into setupChain to reduce duplication and enhance maintainability.
You can modify setupChain to accept parameters or return additional values as needed by different tests.
| t.Helper() | ||
|
|
||
| bankTx := fmt.Sprintf(`{ | ||
| "@type": "%s", | ||
| "from_address": "%s", | ||
| "to_address": "%s", | ||
| "amount": [ | ||
| { | ||
| "denom": "%s", | ||
| "amount": "%d" | ||
| } | ||
| ] | ||
| }`, msgSendTypeURL, granter, toAddr, denom, amount) | ||
| execMsg := WriteToTempJSONFile(t, bankTx) | ||
|
|
||
| execSendCmd := []string{"tx", "authz", "exec", execMsg.Name(), "--from=" + grantee} | ||
| return execSendCmd | ||
| } | ||
|
|
There was a problem hiding this comment.
Use Structs and json.Marshal Instead of fmt.Sprintf for JSON Construction
Constructing JSON messages using fmt.Sprintf can lead to errors if the data contains special characters or unexpected formats. It's recommended to use Go structs and json.Marshal to build JSON messages, which ensures proper encoding and reduces the risk of invalid JSON.
Apply this refactor to construct the JSON message safely:
type BankTx struct {
Type string `json:"@type"`
FromAddress string `json:"from_address"`
ToAddress string `json:"to_address"`
Amount []struct {
Denom string `json:"denom"`
Amount string `json:"amount"`
} `json:"amount"`
}
bankTxStruct := BankTx{
Type: msgSendTypeURL,
FromAddress: granter,
ToAddress: toAddr,
Amount: []struct {
Denom string `json:"denom"`
Amount string `json:"amount"`
}{
{
Denom: denom,
Amount: fmt.Sprintf("%d", amount),
},
},
}
bankTxBytes, err := json.Marshal(bankTxStruct)
require.NoError(t, err)
execMsg := WriteToTempJSONFile(t, string(bankTxBytes))This approach enhances code safety and readability.
| granterBal := cli.QueryBalance(granterAddr, testDenom) | ||
| granteeBal := cli.QueryBalance(granteeAddr, testDenom) | ||
| require.Equal(t, initialAmount, granteeBal) | ||
| allowedAddrBal := cli.QueryBalance(allowedAddr, testDenom) | ||
| require.Equal(t, initialAmount, allowedAddrBal) | ||
|
|
||
| var spendLimitAmount int64 = 1000 | ||
| expirationTime := time.Now().Add(time.Second * 10).Unix() | ||
|
|
||
| // test exec send authorization | ||
|
|
||
| // create send authorization grant | ||
| rsp := cli.RunAndWait("tx", "authz", "grant", granteeAddr, "send", | ||
| "--spend-limit="+fmt.Sprintf("%d%s", spendLimitAmount, testDenom), | ||
| "--allow-list="+allowedAddr, | ||
| "--expiration="+fmt.Sprintf("%d", expirationTime), | ||
| "--fees=1"+testDenom, | ||
| "--from", granterAddr) | ||
| RequireTxSuccess(t, rsp) | ||
| // reduce fees of above tx from granter balance | ||
| granterBal-- | ||
|
|
||
| testCases := []struct { | ||
| name string | ||
| grantee string | ||
| toAddr string | ||
| amount int64 | ||
| expErrMsg string | ||
| }{ | ||
| { | ||
| "valid exec transaction", | ||
| granteeAddr, | ||
| allowedAddr, | ||
| 20, | ||
| "", | ||
| }, | ||
| { | ||
| "send to not allowed address", | ||
| granteeAddr, | ||
| notAllowedAddr, | ||
| 10, | ||
| "cannot send to", | ||
| }, | ||
| { | ||
| "amount greater than spend limit", | ||
| granteeAddr, | ||
| allowedAddr, | ||
| spendLimitAmount + 5, | ||
| "requested amount is more than spend limit", | ||
| }, | ||
| { | ||
| "no grant found", | ||
| newAccount, | ||
| granteeAddr, | ||
| 20, | ||
| "authorization not found", | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| // msg send | ||
| cmd := msgSendExec(t, granterAddr, tc.grantee, tc.toAddr, testDenom, tc.amount) | ||
| if tc.expErrMsg != "" { | ||
| rsp := cli.Run(cmd...) | ||
| RequireTxFailure(t, rsp) | ||
| require.Contains(t, rsp, tc.expErrMsg) | ||
| return | ||
| } | ||
| rsp := cli.RunAndWait(cmd...) | ||
| RequireTxSuccess(t, rsp) | ||
|
|
||
| // check granter balance equals to granterBal - transferredAmount | ||
| expGranterBal := granterBal - tc.amount | ||
| require.Equal(t, expGranterBal, cli.QueryBalance(granterAddr, testDenom)) | ||
| granterBal = expGranterBal | ||
|
|
||
| // check allowed addr balance equals to allowedAddrBal + transferredAmount | ||
| expAllowAddrBal := allowedAddrBal + tc.amount | ||
| require.Equal(t, expAllowAddrBal, cli.QueryBalance(allowedAddr, testDenom)) | ||
| allowedAddrBal = expAllowAddrBal | ||
| }) |
There was a problem hiding this comment.
Ensure Test Isolation to Prevent Side Effects
The variable granterBal is updated across multiple test cases, which can lead to unintended side effects between tests. Consider resetting granterBal or re-initializing it within each test case to ensure test isolation and improve reliability.
Refactor the test cases as follows:
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
granterBal := initialGranterBal
// rest of the test code...
})
}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
tests/systemtests/rest_cli.go (4)
11-15: Consider improving field visibility and naming in GRPCTestCase.The
GRPCTestCasestructure is well-defined, but consider the following suggestions:
- If this structure is intended to be used outside this package, consider making the fields exported (capitalized).
- The
expOutfield name could be more explicit, e.g.,expectedOutputorexpectedResponse.Example:
type GRPCTestCase struct { Name string URL string ExpectedResponse string }
17-29: LGTM: Well-structured test function, but consider renaming.The
RunGRPCQueriesfunction is well-implemented, using subtests and proper error handling. However, the function name might be misleading as it's making HTTP requests, not gRPC calls. Consider renaming it toRunHTTPQueriesorRunRESTQueriesfor clarity.
31-36: Consider improving field visibility and naming in GRPCTestCaseWithHeaders.Similar to the
GRPCTestCasestructure, consider the following improvements:
- If this structure is intended to be used outside this package, consider making the fields exported (capitalized).
- The
expOutfield name could be more explicit, e.g.,expectedOutputorexpectedResponse.Example:
type GRPCTestCaseWithHeaders struct { Name string URL string Headers map[string]string ExpectedResponse string }
38-50: LGTM: Well-structured test function with headers, but consider renaming.The
RunGRPCQueriesWithHeadersfunction is well-implemented, following the same good practices asRunGRPCQueries. However, the function name might be misleading as it's making HTTP requests with headers, not gRPC calls. Consider renaming it toRunHTTPQueriesWithHeadersorRunRESTQueriesWithHeadersfor clarity.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
- tests/systemtests/bank_test.go (5 hunks)
- tests/systemtests/rest_cli.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/systemtests/bank_test.go
🧰 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 not posted (2)
tests/systemtests/rest_cli.go (2)
1-9: LGTM: Package declaration and imports are well-structured.The package name
systemtestsis appropriate for the content, and the imports are correctly organized according to the Uber Go Style Guide (standard library first, followed by external packages).
1-50: Overall assessment: Well-structured system test framework with minor improvements needed.This new file introduces a solid framework for running HTTP-based system tests, which aligns well with the PR objectives of migrating e2e tests to system tests. The code is well-structured, follows good Go practices, and provides reusable components for test cases with and without headers.
Key strengths:
- Clear separation of concerns with distinct structures for different test case types.
- Good use of subtests and error handling in test functions.
- Reusable design that can be easily extended for various system test scenarios.
Suggested improvements:
- Rename functions to accurately reflect that they're running HTTP/REST queries, not gRPC.
- Consider making structure fields exported for better reusability.
- Use more explicit names for some fields (e.g.,
expOuttoexpectedResponse).Overall, this code provides a good foundation for system testing and seems to offer sufficient coverage for the associated changes in the pull request.
akhilkumarpilli
force-pushed
the
akhil/authz-system-tests
branch
from
fb4dc68
to
2ce8237
Compare
Description
Closes: #21609
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
Refactor
Chores
SystemUnderTeststruct to include a new API address field for better configuration management.