Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: migrate e2e/bank to system tests #21607

Merged
merged 8 commits into from
Sep 13, 2024
Merged

Conversation

akhilkumarpilli
Copy link
Contributor

@akhilkumarpilli akhilkumarpilli commented Sep 9, 2024

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...

  • included the correct type prefix in the PR title, you can find examples of the prefixes below:
  • confirmed ! in the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • updated the relevant documentation or specification, including comments for documenting Go code
  • confirmed all CI checks have passed

Reviewers 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...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Introduced an end-to-end test suite for the bank module, enhancing testing capabilities.
    • Added comprehensive system tests for sending and multi-sending transactions, as well as querying bank-related data via gRPC.
  • Bug Fixes

    • Improved reliability and correctness of banking functionalities through extensive testing scenarios.

Copy link
Contributor

coderabbitai bot commented Sep 9, 2024

Walkthrough

Walkthrough

The 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

File Change Summary
tests/e2e/bank/grpc.go Added E2ETestSuite struct, constructor, SetupSuite, and TearDownSuite methods.
tests/systemtests/bank_test.go Introduced system tests TestBankSendTxCmd, TestBankMultiSendTxCmd, and TestBankGRPCQueries for transaction and query validation.

Assessment against linked issues

Objective Addressed Explanation
Rewrite e2e test as a system test and verify it runs (#[21581])
Delete the old e2e test after migration (#[21581]) The old e2e test has not been deleted.

Possibly related issues

  • Issue server/v2: e2e/system tests #20800: The changes in this PR involve migrating e2e tests to system tests, which aligns with the goal of evaluating similar migrations in this related issue.

Possibly related PRs

Suggested labels

C:x/bank


Recent review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 08ccb0d and fa336bf.

Files selected for processing (1)
  • tests/systemtests/bank_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • tests/systemtests/bank_test.go

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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@akhilkumarpilli akhilkumarpilli marked this pull request as ready for review September 9, 2024 11:48
@akhilkumarpilli akhilkumarpilli requested a review from a team as a code owner September 9, 2024 11:48
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between ac53e33 and f2b5903.

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 TestBankSendTxCmd function 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 assert and require is 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 TestBankMultiSendTxCmd function 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 to TestBankSendTxCmd, 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 E2ETestSuite struct and its methods (SetupSuite and TearDownSuite) 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 SetupSuite method correctly initializes the genesis state with detailed metadata for tokens, ensuring that the test environment is consistent with expected configurations.
  • Cleanup: The TearDownSuite method provides clean-up functionality, ensuring that resources are properly released after tests are completed.
  • Best Practices: The use of the suite package from testify allows 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, and TestBalancesGRPCHandler are 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.

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! However, I think we should add more cases

tests/e2e/bank/grpc.go Outdated Show resolved Hide resolved
tests/systemtests/bank_test.go Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between f2b5903 and 7c9611c.

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 TestBankSendTxCmd function 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-only flag
  • --dry-run flag

The test cases are well-structured and follow best practices. The code is readable and maintainable.


141-234: Great test coverage!

The TestBankMultiSendTxCmd function 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.

Comment on lines 236 to 252
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)

}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@akhilkumarpilli akhilkumarpilli marked this pull request as draft September 10, 2024 11:49
Copy link
Contributor

@alpe alpe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

for _, tc := range testCases {
tc := tc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this is not needed anymore since Go 1.22

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())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: here and others. prefer require. instead of assert. when you have a guard to fail fast.

)

sut.StartChain(t)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no query. I assume this is still WIP

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, they were added now.

sut.ModifyGenesisCLI(t,
[]string{"genesis", "add-genesis-account", account1Addr, initialBalance},
[]string{"genesis", "add-genesis-account", account2Addr, initialBalance},
[]string{"genesis", "add-genesis-account", account3Addr, initialBalance},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could query the new balance of the accounts to ensure the expected behaviour

rsp := cli.Run(cmdArgs...)
txResult, found := cli.AwaitTxCommitted(rsp)
assert.True(t, found)
RequireTxSuccess(t, txResult)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As below, please check the new balance. The cli has a helper method for the query

@akhilkumarpilli akhilkumarpilli marked this pull request as ready for review September 11, 2024 10:21
Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 👏🏾

@julienrbrt julienrbrt assigned hieuvubk and unassigned tac0turtle Sep 12, 2024
@julienrbrt julienrbrt added this pull request to the merge queue Sep 13, 2024
@julienrbrt julienrbrt added the backport/v0.52.x PR scheduled for inclusion in the v0.52's next stable release label Sep 13, 2024
Merged via the queue into main with commit 0064ccb Sep 13, 2024
71 of 72 checks passed
@julienrbrt julienrbrt deleted the akhil/bank-system-tests branch September 13, 2024 07:02
mergify bot pushed a commit that referenced this pull request Sep 13, 2024
julienrbrt added a commit that referenced this pull request Sep 13, 2024
Co-authored-by: Akhil Kumar P <36399231+akhilkumarpilli@users.noreply.github.com>
Co-authored-by: Julien Robert <julien@rbrt.fr>
alpe added a commit that referenced this pull request Sep 16, 2024
* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/v0.52.x PR scheduled for inclusion in the v0.52's next stable release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tests/e2e/bank to system tests
5 participants