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

chore!: Refactor x/bank CLI Tests #12706

Merged
merged 34 commits into from
Aug 3, 2022

Conversation

alexanderbez
Copy link
Contributor

@alexanderbez alexanderbez commented Jul 24, 2022

Changelog

  • Defined a new TendermintRPC interface and the Node methods on client.Context now operate and use this type instead of a concrete Tendermint RPC client.
  • Add --chain-id flag to AddTxFlagsToCmd
  • Define CreateExecuteContext to make tests CLI possible with a custom context
  • Remove x/bank/client/testutil/ package 🚀🚀🚀

ref: #12696


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
  • added ! to 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
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • 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.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@github-actions github-actions bot added the C:x/genutil genutil module issues label Jul 28, 2022
@alexanderbez alexanderbez marked this pull request as ready for review July 28, 2022 01:39
@alexanderbez alexanderbez requested a review from a team as a code owner July 28, 2022 01:39
network *network.Network
}

func NewIntegrationTestSuite(cfg network.Config) *IntegrationTestSuite {
Copy link
Contributor

@amaury1093 amaury1093 Jul 28, 2022

Choose a reason for hiding this comment

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

one reason this test suite was not in a _test.go was to actually make it public for external chains (we're using it in Regen, Provenance wanted that too, ref #6711 (comment)).

Idea is that we can pass our own NewRegenApp into that test suite, and it will run bank CLI tests againt the chain, making sure we didn't mess up NewRegenApp.

I think that's still valuable, so I'm reluctant to move these tests into a _test.go file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, the entire point of the EPIC is to make the CLI tests completely independent of simapp or any app for that matter and independent of Tendermint as well, so essentially they're just basic unit tests.

There is another EPIC that describes integration tests that should be more in the flavor of e2e tests. They would not exist in any module, but in tests/e2e.

Copy link
Member

Choose a reason for hiding this comment

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

Should we not then move these to tests/e2e instead of deleting them? There is already the cli_test.go moved there. So when we refactor end-to-end tests, then we don't start from 0.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK I understand now what you're doing in this PR, it makes sense to decouple CLI tests from simapp 👍

I guess I'm still confused what kind of tests tests/e2e will have: CLI? gRPC? (both of which are often closely tied to modules).

There is another EPIC that describes integration tests that should be more in the flavor of e2e tests

Which EPIC is that?

Copy link
Member

Choose a reason for hiding this comment

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

I guess I'm still confused what kind of tests tests/e2e will have: CLI? gRPC? (both of which are often closely tied to modules).

We moved the current integration/e2e tests there and talked how this is a short term fix to unblock app wiring work. We still need to have a discussion around e2e tests. Bez proposed two ways, umee and osmosis. We should evaluate those and others to see what approach we should take.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we not then move these to tests/e2e instead of deleting them? There is already the cli_test.go moved there. So when we refactor end-to-end tests, then we don't start from 0.

No because I think they should be completely redefined or at least defined on an entire app-basis, not module by module. See #12527

I guess I'm still confused what kind of tests tests/e2e will have: CLI? gRPC? (both of which are often closely tied to modules).

Most likely both -- gRPC and CLI

Which EPIC is that?

I can't seem to find it. I think it was actually a comment in a PR by @kocubinski. @marbar3778 should we make one for e2e tests?

Copy link
Member

Choose a reason for hiding this comment

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

I am also in favor of keeping (moving to tests/e2e) these tests until we have some consensus on levels of testing. In our call this morning it was decided an ADR would be made to concentrate feedback on the topic.

Copy link
Contributor Author

@alexanderbez alexanderbez Jul 29, 2022

Choose a reason for hiding this comment

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

These are not how E2E tests should be designed IMO. I linked references to how I think we should design our E2E tests and they should be docker-based, not use an in-process Tendermint node. But if there is team consensus to move these existing tests there, then I will do so 👍

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! I think we need to restore and CLI tests under tests/e2e/bank. Otherwise the tests are failing.

"github.com/cosmos/cosmos-sdk/x/bank/types"
)

func (s *IntegrationTestSuite) TestTotalSupplyGRPCHandler() {
Copy link
Member

Choose a reason for hiding this comment

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

Should we not keep these in tests/e2e/bank prior to refactoring, then? It's being used in tests/e2e/bank/cli_test.go

Copy link
Contributor Author

@alexanderbez alexanderbez Jul 29, 2022

Choose a reason for hiding this comment

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

No. E2E tests should not exist in modules at all IMO. There should be a single place, e.g. tests/e2e that tests an application's full e2e functionality such as CLI and gRPC.

Copy link
Member

@julienrbrt julienrbrt Jul 29, 2022

Choose a reason for hiding this comment

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

That's what I mean: to move temporarily these deleted tests files in /tests/e2e/bank until we refactor them properly :)
So they won't be in the module but we still have a sort of e2e tests until we improve them.

Otherwise you need to delete /tests/e2e/bank (see the build error)

var _ client.TendermintRPC = (*mockTendermintRPC)(nil)

type mockTendermintRPC struct {
rpcclientmock.Client
Copy link
Member

Choose a reason for hiding this comment

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

😍

@@ -26,7 +22,7 @@ import (
// handling and queries.
type Context struct {
FromAddress sdk.AccAddress
Client rpcclient.Client
Copy link
Member

Choose a reason for hiding this comment

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

😻

@codecov
Copy link

codecov bot commented Jul 30, 2022

Codecov Report

Merging #12706 (9235038) into main (7103587) will decrease coverage by 0.76%.
The diff coverage is 23.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #12706      +/-   ##
==========================================
- Coverage   57.01%   56.25%   -0.77%     
==========================================
  Files         661      648      -13     
  Lines       56088    55009    -1079     
==========================================
- Hits        31981    30947    -1034     
+ Misses      21588    21564      -24     
+ Partials     2519     2498      -21     
Impacted Files Coverage Δ
client/account_retriever.go 0.00% <0.00%> (ø)
client/context.go 56.28% <0.00%> (ø)
client/flags/flags.go 19.67% <0.00%> (-0.33%) ⬇️
client/grpc_query.go 0.00% <ø> (ø)
client/tx/factory.go 25.70% <ø> (+0.10%) ⬆️
simapp/simd/cmd/root.go 70.32% <ø> (-0.24%) ⬇️
x/auth/client/cli/tips.go 0.00% <0.00%> (ø)
x/auth/client/cli/tx_multisign.go 0.00% <ø> (ø)
x/auth/client/cli/tx_sign.go 0.00% <ø> (ø)
x/auth/client/cli/validate_sigs.go 0.00% <ø> (ø)
... and 32 more

client/query.go Outdated Show resolved Hide resolved
alexanderbez and others added 3 commits August 1, 2022 17:11
Co-authored-by: Marko <marbar3778@yahoo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants