test(sims): fix test cases feegrant #19863
Conversation
In the file tests/sims/feegrant/operation_test.go tests failed becasue test accounts are not added to the accoint keeper Fixed: cosmos#19862
WalkthroughThe change involves creating and setting up new accounts in the Changes
Assessment against linked issues
Related issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| acc := suite.accountKeeper.NewAccountWithAddress(suite.ctx, account.Address) | ||
| suite.accountKeeper.SetAccount(suite.ctx, acc) |
There was a problem hiding this comment.
The addition of NewAccountWithAddress and SetAccount methods correctly addresses the issue of test accounts not being added to the account keeper, which was causing nil pointer dereferences. This change ensures that test accounts are properly initialized and set in the account keeper before being used in tests, thereby preventing the previously observed runtime errors.
However, it's important to ensure that the NewAccountWithAddress method does not have side effects that could affect other tests or the overall test suite's behavior. Additionally, consider adding a check after setting the account to verify that it has been correctly added to the account keeper. This could further improve the robustness of the tests.
Would you like me to provide an example of how to verify that accounts have been correctly added to the account keeper?
There was a problem hiding this comment.
All tests in the test suite are affected, now they pass. Before that they all failed.
Account is properly added to the keeper because all the tests passes. Why I should check that when the production code will failed if it is not. If we follow that logic we should check that account is fund correctly or that the context is properly created and so on.
There was a problem hiding this comment.
@EmilGeorgiev, thank you for your insight. You make a valid point about the balance between ensuring robustness and avoiding redundancy in tests. Given that all tests in the suite now pass, indicating that the accounts are properly added to the account keeper, it's reasonable to rely on these outcomes as confirmation of the accounts being correctly set up. Your approach highlights the importance of practicality in testing, ensuring that we focus on what truly matters for the integrity of the code without overcomplicating the test suite. I appreciate your perspective on this matter.
julienrbrt
changed the title
Co-authored-by: son trinh <[email protected]>
In the file tests/sims/feegrant/operation_test.go tests failed because test accounts are not added to the account keeper
Fixed: #19862
Description
Closes: #19862
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.
I have...
Summary by CodeRabbit