test: migrate e2e/bank to system tests #21607
Conversation
WalkthroughWalkthroughThe pull request introduces a new end-to-end test suite for the bank module within a Cosmos SDK application, enhancing the existing testing framework. It includes the creation of a structured test suite with setup and teardown methods, as well as comprehensive system tests for transaction functionalities and gRPC queries, ensuring robust validation of the bank module's operations. Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
Recent review detailsConfiguration used: .coderabbit.yml Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- tests/e2e/bank/grpc.go (1 hunks)
- tests/systemtests/bank_test.go (1 hunks)
Additional context used
Path-based instructions (2)
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/e2e/bank/grpc.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 (5)
tests/systemtests/bank_test.go (3)
1-13: Header and imports are correctly set up.The build tag and package declaration are correctly specified for system tests. The imports are appropriate for the test functions, including necessary libraries for assertions, JSON handling, and specific Cosmos SDK dependencies.
15-121: Comprehensive system test for bank send command.The
TestBankSendTxCmdfunction is well-structured and covers various scenarios including valid transactions, insufficient fees, and insufficient gas. The use of table-driven tests enhances maintainability and readability. The assertions are correctly used to validate the outcomes based on the expected results.
- Best Practices: The use of
assertandrequireis consistent and appropriate for the scenarios tested.- Error Handling: The test cases handle expected errors effectively, using custom error matchers to assert specific error codes and messages.
- Performance: The test setup and execution are optimized for performance by reusing the CLI wrapper and only resetting the chain once at the beginning.
Overall, the test function adheres to best practices for system testing in Go, ensuring that each scenario is isolated and thoroughly tested.
123-216: Detailed system test for bank multi-send command.The
TestBankMultiSendTxCmdfunction effectively tests the multi-send functionality of the bank module. It includes tests for valid transactions, argument validation, fee checks, and gas limits. The structure is similar toTestBankSendTxCmd, utilizing table-driven tests for clarity and ease of maintenance.
- Error Handling: The function includes comprehensive checks for various error conditions, ensuring that the system behaves as expected under different configurations.
- Code Quality: The use of structured tests and clear naming conventions contributes to the high readability and maintainability of the code.
- Functionality: The tests cover a broad range of inputs and settings, ensuring that the multi-send functionality is robust against various inputs and conditions.
This function demonstrates good testing practices, with clear separation of scenarios and thorough validation of expected outcomes.
tests/e2e/bank/grpc.go (2)
21-89: Well-structured end-to-end test suite setup.The
E2ETestSuitestruct and its methods (SetupSuiteandTearDownSuite) are well-implemented, providing a robust framework for end-to-end testing of the bank module. The setup method prepares the test environment thoroughly, including configuring network settings and initializing metadata for tokens.
- Initialization: The
SetupSuitemethod correctly initializes the genesis state with detailed metadata for tokens, ensuring that the test environment is consistent with expected configurations.- Cleanup: The
TearDownSuitemethod provides clean-up functionality, ensuring that resources are properly released after tests are completed.- Best Practices: The use of the
suitepackage fromtestifyallows for structured and organized tests, enhancing the readability and maintainability of the test code.This setup ensures that the test environment is correctly prepared and cleaned up, which is crucial for the reliability of end-to-end tests.
Line range hint
95-216: Comprehensive testing of GRPC handlers within the bank module.The test methods
TestTotalSupplyGRPCHandler,TestDenomMetadataGRPCHandler, andTestBalancesGRPCHandlerare well-crafted, covering a wide range of scenarios for the bank module's GRPC endpoints. Each test method uses structured test cases to validate the functionality of the endpoints.
- Coverage: The tests cover various scenarios, including querying total supply, metadata, and balances, ensuring comprehensive coverage of the bank module's functionalities.
- Error Handling: The tests appropriately handle potential errors and validate the responses against expected outcomes, ensuring that the GRPC handlers behave as expected under different conditions.
- Best Practices: The use of structured test cases and assertions for comparing expected and actual results ensures that the tests are both thorough and easy to understand.
These methods demonstrate effective testing practices, ensuring that the bank module's GRPC handlers are robust and function correctly across various scenarios.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- tests/systemtests/bank_test.go (1 hunks)
Additional context used
Path-based instructions (1)
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"
Additional comments not posted (3)
tests/systemtests/bank_test.go (3)
1-1: This comment is a duplicate of the existing comment by julienrbrt. The author has acknowledged the comment and agreed to add the suggested non-happy path test cases. Let's wait for the author to address this comment in a future commit.
13-139: Great test coverage!The
TestBankSendTxCmdfunction provides comprehensive test coverage for the bank send command. It covers various scenarios such as:
- Valid transactions
- Insufficient fees
- Insufficient gas
- Insufficient funds
- Unauthorized signatures
--generate-onlyflag--dry-runflagThe test cases are well-structured and follow best practices. The code is readable and maintainable.
141-234: Great test coverage!The
TestBankMultiSendTxCmdfunction provides comprehensive test coverage for the bank multi-send command. It covers various scenarios such as:
- Valid transactions
- Insufficient arguments
- Insufficient fees
- Insufficient gas
- Invalid flag combinations
The test cases are well-structured and follow best practices. The code is readable and maintainable.
tests/systemtests/bank_test.go
Outdated
| func TestBankGRPCQueries(t *testing.T) { | ||
| // scenario: test bank grpc gateway queries | ||
| // given a running chain | ||
|
|
||
| sut.ResetChain(t) | ||
| cli := NewCLIWrapper(t, sut, verbose) | ||
| // add genesis account with some tokens | ||
| account1Addr := cli.AddKey("account1") | ||
| account2Addr := cli.AddKey("account2") | ||
| require.NotEqual(t, account1Addr, account2Addr) | ||
| sut.ModifyGenesisCLI(t, | ||
| []string{"genesis", "add-genesis-account", account1Addr, "10000000stake"}, | ||
| ) | ||
|
|
||
| sut.StartChain(t) | ||
|
|
||
| } |
There was a problem hiding this comment.
Test cases for bank gRPC queries are missing.
The TestBankGRPCQueries function is empty and does not contain any test cases for bank gRPC queries. This is a critical gap in the test coverage.
Please add test cases to cover the bank gRPC queries and ensure that they are functioning as expected.
If you need any assistance in implementing the test cases, I'd be happy to help. Let me know if you want me to generate the test code or open a GitHub issue to track this task.
There was a problem hiding this comment.
Very nice start! 🏄
From a high level view, I would recommend to focus on the bank specific TX cases here and avoid generic TX scenarios like "not enough fees, insufficient gas, wrong chain ID,..." . They would not be hitting the bank module but are handled before. Unit or integration tests can give the same level of confidence. A generic TX system test can also define them once as extra test net when needed.
IMHO "unauthorized signature" though can be useful here as it covers a high risk scenario.
This test may be used as a template for other modules so let's try to keep it to the minimum required for bank.
fyi: the default genesis comes with some predefined accounts for the validators node0 to n. If possible, try re-using them. The ModifyGenesis sets the dirty state flag to revert everything in the next test run.
tests/systemtests/bank_test.go
Outdated
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| tc := tc |
There was a problem hiding this comment.
nit: this is not needed anymore since Go 1.22
tests/systemtests/bank_test.go
Outdated
| assertUnauthorizedErr := func(t assert.TestingT, gotErr error, gotOutputs ...interface{}) bool { | ||
| assert.Len(t, gotOutputs, 1) | ||
| code := gjson.Get(gotOutputs[0].(string), "code") | ||
| assert.True(t, code.Exists()) |
There was a problem hiding this comment.
nit: here and others. prefer require. instead of assert. when you have a guard to fail fast.
| ) | ||
|
|
||
| sut.StartChain(t) | ||
|
|
There was a problem hiding this comment.
There is no query. I assume this is still WIP
There was a problem hiding this comment.
Yes, they were added now.
tests/systemtests/bank_test.go
Outdated
| sut.ModifyGenesisCLI(t, | ||
| []string{"genesis", "add-genesis-account", account1Addr, initialBalance}, | ||
| []string{"genesis", "add-genesis-account", account2Addr, initialBalance}, | ||
| []string{"genesis", "add-genesis-account", account3Addr, initialBalance}, |
There was a problem hiding this comment.
would it make sense to not add account3 to genesis so that it would be created by the submission? Then this scenario is handled as well
| txResult, found := cli.AwaitTxCommitted(rsp) | ||
| assert.True(t, found) | ||
| RequireTxSuccess(t, txResult) | ||
| } |
There was a problem hiding this comment.
You could query the new balance of the accounts to ensure the expected behaviour
tests/systemtests/bank_test.go
Outdated
| rsp := cli.Run(cmdArgs...) | ||
| txResult, found := cli.AwaitTxCommitted(rsp) | ||
| assert.True(t, found) | ||
| RequireTxSuccess(t, txResult) |
There was a problem hiding this comment.
As below, please check the new balance. The cli has a helper method for the query
julienrbrt
added
the
backport/v0.52.x
(cherry picked from commit 0064ccb)
* main: test: fix sims (#21735) build: bump proto-builder (#21730) refactor(schema)!: rename IntegerStringKind and DecimalStringKind (#21694) feat(types/collections): add `LegacyDec` collection value (#21693) refactor(server): alias AppOptions to coreserver.DynamicConfig (#21711) refactor(simapp): simplify simapp di (#21718) feat: replace the cosmos-db usecases in the tests with `core/testing` (#21525) feat(runtime/v2): store loader on simappv2 (#21704) docs(x/auth): vesting (#21715) build(deps): Bump google.golang.org/grpc from 1.66.1 to 1.66.2 (#21670) refactor(systemtest): Add cli.RunAndWait for common operations (#21689) fix(runtime/v2): provide default factory options if unset in app builder (#21690) chore: remove duplicate proto files for the same proto file (#21648) feat(x/genutil): add better error messages for genesis validation (#21701) build(deps): Bump cosmossdk.io/core from 1.0.0-alpha.1 to 1.0.0-alpha.2 (#21698) test: migrate e2e/bank to system tests (#21607) chore: fix the gci lint issue in testutil (#21695) docs(x/authz): update grant docs (#21677)
Description
Closes: #21581
Migrated bank CLI command tests from tests/e2e/bank to tests/systemtests.
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
Summary by CodeRabbit
New Features
Bug Fixes