test: migrate e2e/auth to system tests and fix sign-batch #22149
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request involves the deletion of end-to-end (E2E) tests for the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
akhilkumarpilli
changed the title
akhilkumarpilli
changed the title
akhilkumarpilli
requested review from
facundomedica,
testinginprod,
aaronc and
a team
as code owners
akhilkumarpilli
requested review from
alpe,
tac0turtle,
julienrbrt and
hieuvubk
and removed request for
aaronc,
facundomedica and
testinginprod
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
client/account_retriever.go (1)
16-18: Consider using a more descriptive field nameThe
mockAccountstruct is well-defined, but the field nameaddrcould be more descriptive. Consider renaming it toaddressfor better clarity.type mockAccount struct { - addr []byte + address []byte }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
- client/account_retriever.go (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
client/account_retriever.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (3)
client/account_retriever.go (3)
20-22: LGTM: GetAddress method implementationThe
GetAddressmethod is correctly implemented, returning the address field as required by theAccountinterface.
55-56: LGTM: Updated GetAccount methodThe
GetAccountmethod has been correctly updated to return amockAccountinstance initialized with the provided address. This change enhances the mock implementation, allowing for more realistic testing scenarios.
Line range hint
1-70: Summary of changes and their impactThe introduction of the
mockAccountstruct and the update to theMockAccountRetriever.GetAccountmethod enhance the mocking capabilities of theAccountRetrieverinterface. These changes allow for more realistic testing scenarios by providing a concrete mock implementation instead of returning nil.The changes are well-contained and don't introduce any inconsistencies in the existing code. The
MockAccountRetrievercontinues to correctly implement theAccountRetrieverinterface.Overall, these changes improve the testability of code that depends on the
AccountRetrieverinterface.
| func (m mockAccount) GetPubKey() cryptotypes.PubKey { | ||
| return nil | ||
| } | ||
|
|
||
| func (m mockAccount) GetAccountNumber() uint64 { | ||
| return 0 | ||
| } | ||
|
|
||
| func (m mockAccount) GetSequence() uint64 { | ||
| return 0 | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making mock values configurable
The implementation of GetPubKey, GetAccountNumber, and GetSequence methods is correct for a basic mock. However, to enhance flexibility in testing scenarios, consider making these values configurable. This would allow tests to set specific values when needed.
Here's a suggested implementation:
type mockAccount struct {
address []byte
pubKey cryptotypes.PubKey
accNum uint64
seq uint64
}
func (m mockAccount) GetPubKey() cryptotypes.PubKey {
return m.pubKey
}
func (m mockAccount) GetAccountNumber() uint64 {
return m.accNum
}
func (m mockAccount) GetSequence() uint64 {
return m.seq
}This change would require updating the MockAccountRetriever.GetAccount method to initialize these fields.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
tests/systemtests/auth_test.go (3)
70-75: Reminder: Address the TODO about multi-message support in v2There's a TODO comment indicating that the code block should be removed once version 2 supports multiple messages. To maintain code cleanliness and prevent outdated code from persisting, please ensure that this TODO is tracked and addressed when v2 supports multi-message transactions.
Would you like me to open a GitHub issue to track this task and ensure it's revisited in the future?
96-112: Enhance test coverage with additional casesThe test cases in
testSignTxBroadcastcover primary scenarios. To ensure robust testing, consider adding edge cases such as invalid account numbers, incorrect sequences, missing signatures, and unsupported sign modes. This will help catch potential issues and improve the reliability of the transaction signing process.
495-505: Ensure proper cleanup of temporary filesIn the
TestTxEncodeandDecodefunction, temporary files are created to store transaction data. Consider cleaning up these files after the test completes to prevent resource leaks and maintain a clean testing environment.You can defer the removal of the temporary file:
txFile := StoreTempFile(t, []byte(sendTx)) +defer os.Remove(txFile.Name())
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
- tests/systemtests/auth_test.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
tests/systemtests/auth_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"
| for _, tc := range txsTestCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| cmd := []string{"q", "txs", tc.query} | ||
| rsp = cli.CustomQuery(cmd...) | ||
| txs := gjson.Get(rsp, "txs").Array() | ||
| require.Equal(t, tc.expLen, len(txs)) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Fix variable capture in for-loop when using t.Run
In the loop starting at line 227, the variable tc is captured by reference within the closure passed to t.Run. This can lead to subtests using the last value of tc rather than the intended one, causing flaky or incorrect test results. To fix this, capture tc at each iteration by re-declaring it inside the loop.
Apply the following fix:
for _, tc := range txsTestCases {
+ tc := tc // capture loop variable
t.Run(tc.name, func(t *testing.T) {
// test code
})
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, tc := range txsTestCases { | |
| t.Run(tc.name, func(t *testing.T) { | |
| cmd := []string{"q", "txs", tc.query} | |
| rsp = cli.CustomQuery(cmd...) | |
| txs := gjson.Get(rsp, "txs").Array() | |
| require.Equal(t, tc.expLen, len(txs)) | |
| }) | |
| } | |
| for _, tc := range txsTestCases { | |
| tc := tc // capture loop variable | |
| t.Run(tc.name, func(t *testing.T) { | |
| cmd := []string{"q", "txs", tc.query} | |
| rsp = cli.CustomQuery(cmd...) | |
| txs := gjson.Get(rsp, "txs").Array() | |
| require.Equal(t, tc.expLen, len(txs)) | |
| }) | |
| } |
tests/systemtests/auth_test.go
Outdated
| assertTxOutput := func(_ assert.TestingT, gotErr error, gotOutputs ...interface{}) bool { | ||
| require.Len(t, gotOutputs, 1) | ||
| output := gotOutputs[0].(string) | ||
| if tc.expErrMsg != "" { | ||
| require.Contains(t, output, tc.expErrMsg) | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling the return value from assertTxOutput
In the TestAuxSigner function, the custom error matcher assertTxOutput returns a boolean value indicating the success of the assertion. Currently, the return value is being ignored. To improve test reliability and clarity, consider checking the return value or using assert.NoError if appropriate.
Modify the code to handle the return value:
_ = cli.WithRunErrorMatcher(assertTxOutput).Run(cli.withTXFlags(cmd...)...)
+// Optionally, you can check the return value to assert test success
+success := cli.WithRunErrorMatcher(assertTxOutput).Run(cli.withTXFlags(cmd...)...)
+require.True(t, success)Committable suggestion was skipped due to low confidence.
(cherry picked from commit 4733fc1)
…22149) (#22234) Co-authored-by: Akhil Kumar P <[email protected]>
Description
Closes: #21926
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
Chores