chore!: Refactor x/bank CLI Tests #12706

alexanderbez · 2022-07-24T14:08:31Z

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

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

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)

…os/cosmos-sdk into bez/12696-x-bank-cli-tests-refactor

amaury1093 · 2022-07-28T06:54:15Z

x/bank/client/testutil/suite.go

-	network *network.Network
-}
-
-func NewIntegrationTestSuite(cfg network.Config) *IntegrationTestSuite {


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.

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.

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.

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?

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.

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?

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.

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 👍

x/bank/client/testutil/grpc.go

julienrbrt

lgtm! I think we need to restore and CLI tests under tests/e2e/bank. Otherwise the tests are failing.

julienrbrt · 2022-07-28T13:15:02Z

x/bank/client/testutil/grpc.go

-	"github.com/cosmos/cosmos-sdk/x/bank/types"
-)
-
-func (s *IntegrationTestSuite) TestTotalSupplyGRPCHandler() {


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

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.

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)

kocubinski · 2022-07-28T18:27:48Z

x/bank/client/cli/suite_test.go

+var _ client.TendermintRPC = (*mockTendermintRPC)(nil)
+
+type mockTendermintRPC struct {
+	rpcclientmock.Client


kocubinski · 2022-07-28T19:03:19Z

client/context.go

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


codecov · 2022-07-30T01:51:17Z

Codecov Report

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

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

Co-authored-by: Marko <marbar3778@yahoo.com>

updates

ef0afe1

alexanderbez mentioned this pull request Jul 24, 2022

EPIC: Rewrite and Simplify CLI Tests #12696

Closed

15 tasks

github-actions bot added the C:CLI label Jul 24, 2022

alexanderbez and others added 5 commits July 24, 2022 10:10

Merge branch 'main' into bez/12696-x-bank-cli-tests-refactor

136e0c9

updates

c4f5a6c

updates

c4d01b3

Merge branch 'main' into bez/12696-x-bank-cli-tests-refactor

045b142

updates

4b9b40b

github-actions bot added the C:x/bank label Jul 25, 2022

alexanderbez and others added 18 commits July 25, 2022 20:59

updates

28688f0

updates

5447530

updates

9d61e4c

updates

b229234

updates

dd6e1f4

updates

1484c8e

updates

5a209d1

Merge branch 'main' into bez/12696-x-bank-cli-tests-refactor

133d732

updates

b2364c2

Merge branch 'bez/12696-x-bank-cli-tests-refactor' of github.com:cosm…

3f94e2e

…os/cosmos-sdk into bez/12696-x-bank-cli-tests-refactor

updates

883e6da

updates

7350035

updates

a58abf2

Merge branch 'main' into bez/12696-x-bank-cli-tests-refactor

1ff43e4

updates

f5b8eb1

updates

fdb1974

updates

cfd48d5

updates

a3e3857

github-actions bot added the C:x/genutil genutil module issues label Jul 28, 2022

updates

0333304

github-actions bot added the C:x/auth label Jul 28, 2022

alexanderbez and others added 2 commits July 28, 2022 09:27

updates

1d51689

Merge branch 'main' into bez/12696-x-bank-cli-tests-refactor

004d979

alexanderbez marked this pull request as ready for review July 28, 2022 01:39

alexanderbez requested a review from a team as a code owner July 28, 2022 01:39

amaury1093 reviewed Jul 28, 2022

View reviewed changes

x/bank/client/testutil/grpc.go Outdated Show resolved Hide resolved

alexanderbez and others added 2 commits July 28, 2022 08:39

Merge branch 'main' into bez/12696-x-bank-cli-tests-refactor

d2b1844

updates

ae0f2be

alexanderbez requested review from amaury1093, tac0turtle and julienrbrt July 28, 2022 13:11

julienrbrt approved these changes Jul 28, 2022

View reviewed changes

kocubinski reviewed Jul 28, 2022

View reviewed changes

alexanderbez added 2 commits July 28, 2022 21:13

Merge branch 'main' into bez/12696-x-bank-cli-tests-refactor

f0d4ba8

Merge branch 'main' into bez/12696-x-bank-cli-tests-refactor

6e13bf3

tac0turtle reviewed Aug 1, 2022

View reviewed changes

client/query.go Outdated Show resolved Hide resolved

alexanderbez and others added 3 commits August 1, 2022 17:11

Update client/query.go

fc4f732

Co-authored-by: Marko <marbar3778@yahoo.com>

updates

f89840a

updates

9235038

amaury1093 assigned amaury1093 and JeancarloBarrios Aug 3, 2022

alexanderbez merged commit ccc8003 into main Aug 3, 2022

alexanderbez deleted the bez/12696-x-bank-cli-tests-refactor branch August 3, 2022 16:08

yihuang mentioned this pull request Aug 31, 2022

fix: call SetIAVLCacheSize with the configured value in simapp (backport #13107) #13108

Merged

larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023

chore!: Refactor x/bank CLI Tests (cosmos#12706)

cb9923b

srdtrk mentioned this pull request Jun 22, 2023

[Bug] Compatibility issues with SDK v0.47 queries strangelove-ventures/interchaintest#637

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore!: Refactor x/bank CLI Tests #12706

chore!: Refactor x/bank CLI Tests #12706

alexanderbez commented Jul 24, 2022 •

edited

Loading

amaury1093 Jul 28, 2022 •

edited

Loading

alexanderbez Jul 28, 2022

julienrbrt Jul 28, 2022

amaury1093 Jul 28, 2022

tac0turtle Jul 28, 2022

alexanderbez Jul 28, 2022

kocubinski Jul 28, 2022

alexanderbez Jul 29, 2022 •

edited

Loading

julienrbrt left a comment

julienrbrt Jul 28, 2022

alexanderbez Jul 29, 2022 •

edited

Loading

julienrbrt Jul 29, 2022 •

edited

Loading

kocubinski Jul 28, 2022

kocubinski Jul 28, 2022

codecov bot commented Jul 30, 2022 •

edited

Loading

chore!: Refactor x/bank CLI Tests #12706

chore!: Refactor x/bank CLI Tests #12706

Conversation

alexanderbez commented Jul 24, 2022 • edited Loading

Changelog

Author Checklist

Reviewers Checklist

amaury1093 Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez Jul 29, 2022 • edited Loading

Choose a reason for hiding this comment

julienrbrt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez Jul 29, 2022 • edited Loading

Choose a reason for hiding this comment

julienrbrt Jul 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 30, 2022 • edited Loading

Codecov Report

alexanderbez commented Jul 24, 2022 •

edited

Loading

amaury1093 Jul 28, 2022 •

edited

Loading

alexanderbez Jul 29, 2022 •

edited

Loading

alexanderbez Jul 29, 2022 •

edited

Loading

julienrbrt Jul 29, 2022 •

edited

Loading

codecov bot commented Jul 30, 2022 •

edited

Loading