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

Make integration test suites reusable by apps #6711

Merged
merged 49 commits into from
Apr 23, 2021
Merged

Conversation

aaronc
Copy link
Member

@aaronc aaronc commented Jul 13, 2020

This is a relatively small refactor of the work done so far on #6423 to make in-process integration tests easily reusable by apps.

For the existing test suites (bank, crisis and distribution) the following changes were made:

  • move IntegrationTestSuite into client/testutil/suite.go files
  • add a NewIntegrationTestSuite constructor to the suites which takes a single Config parameter

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@codecov
Copy link

codecov bot commented Jul 13, 2020

Codecov Report

Merging #6711 (dcccac7) into master (1a15412) will increase coverage by 0.81%.
The diff coverage is 98.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6711      +/-   ##
==========================================
+ Coverage   58.89%   59.70%   +0.81%     
==========================================
  Files         585      595      +10     
  Lines       32801    37315    +4514     
==========================================
+ Hits        19318    22279    +2961     
- Misses      11199    13057    +1858     
+ Partials     2284     1979     -305     
Impacted Files Coverage Δ
x/bank/client/testutil/suite.go 97.71% <95.00%> (ø)
types/decimal.go 70.49% <100.00%> (+0.19%) ⬆️
x/auth/client/testutil/suite.go 95.41% <100.00%> (ø)
x/auth/vesting/client/testutil/suite.go 100.00% <100.00%> (ø)
x/authz/client/testutil/query.go 100.00% <100.00%> (ø)
x/authz/client/testutil/tx.go 97.63% <100.00%> (ø)
x/crisis/client/testsuite/suite.go 100.00% <100.00%> (ø)
x/distribution/client/testutil/suite.go 100.00% <100.00%> (ø)
x/evidence/client/testutil/suite.go 100.00% <100.00%> (ø)
x/feegrant/client/testutil/suite.go 99.13% <100.00%> (ø)
... and 62 more

@aaronc aaronc marked this pull request as ready for review July 13, 2020 21:44
@aaronc aaronc mentioned this pull request Jul 13, 2020
19 tasks
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

I just don't personally see the benefit to this approach. I don't want to block on my review, however. SDK developers at large should appropriately review this and merge if there is alignment.

@iramiller
Copy link
Contributor

For my team we are very interested in the efforts to make the test suites accessible for integration with the tests of our own modules. There is a lot of benefit from our perspective to ensuring that the application we have built on the SDK continues to support base behaviors correctly as new modules are added. This is especially important in cases such as a module that create extensions on BaseAccount.

My view is that all of the features contained in the resulting app need to be fully exercised regardless of if they were inherited or directly implemented in order to achieve a comprehensive integration test.

fedekunze
fedekunze previously approved these changes Jul 17, 2020
Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

good from my end too. Pending lint and conflict fixes

@@ -275,7 +272,7 @@ func (s *IntegrationTestSuite) TestNewSendTxCmd() {
ctx := context.Background()
ctx = context.WithValue(ctx, client.ClientContextKey, &clientCtx)
Copy link
Collaborator

Choose a reason for hiding this comment

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

ineffassign

@@ -543,7 +543,7 @@ func (s *IntegrationTestSuite) TestNewWithdrawRewardsCmd() {
ctx := context.Background()
ctx = context.WithValue(ctx, client.ClientContextKey, &clientCtx)
Copy link
Collaborator

Choose a reason for hiding this comment

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

ineffassign

@alexanderbez
Copy link
Contributor

alexanderbez commented Jul 17, 2020

For my team we are very interested in the efforts to make the test suites accessible for integration with the tests of our own modules. There is a lot of benefit from our perspective to ensuring that the application we have built on the SDK continues to support base behaviors correctly as new modules are added. This is especially important in cases such as a module that create extensions on BaseAccount.

My view is that all of the features contained in the resulting app need to be fully exercised regardless of if they were inherited or directly implemented in order to achieve a comprehensive integration test.

How does what is contained in this PR aid in this regard? Is there a PR or code you can point me to that references this? Module CLI integration tests are self-contained and isolated. How does accessing, say x/bank integration test suite, help you test your module?

I've stated above I'm very reluctant to merging this PR.

@aaronc
Copy link
Member Author

aaronc commented Jul 17, 2020

For my team we are very interested in the efforts to make the test suites accessible for integration with the tests of our own modules. There is a lot of benefit from our perspective to ensuring that the application we have built on the SDK continues to support base behaviors correctly as new modules are added. This is especially important in cases such as a module that create extensions on BaseAccount.
My view is that all of the features contained in the resulting app need to be fully exercised regardless of if they were inherited or directly implemented in order to achieve a comprehensive integration test.

How does what is contained in this PR aid in this regard? Is there a PR or code you can point me to that references this? Module CLI integration tests are self-contained and isolated. How does accessing, say x/bank integration test suite, help you test your module?

I've stated above I'm very reluctant to merging this PR.

Module CLI integration tests are not self-contained and isolated, they are running against simapp. Simapp is not what zone developers are deploying. I want to be able to run these integration test suites against the wired together app that I am building. I believe that is @iramiller's use case as well.

I am actually in favor of still running these suites as unit tests scoped to their modules - I might update this PR to keep those in place.

But I also want to be able to wire up these suites in Regen's code base to run against Regen's app.

@iramiller
Copy link
Contributor

I want to be able to run these integration test suites against the wired together app that I am building. I believe that is @iramiller's use case as well.

Indeed. I am interested in any changes that could be leveraged for exercising the built in SDK unit/integration type tests against my team's app. For reference when I tested the cli_tests from gaia (which required a great deal of tweaking to run in our environment) I encountered several errors where we had made changes that broke expected behavior. As a specific example we have patched our command flags to set some default/provide some structure that is appropriate for our use. During the run of the cli_tests it became clear that we had inadvertently wired a piece incorrectly. While our app behaved as expected an end user trying to follow SDK examples and use some of the extra flags would have found them to not work as expected.

This experience has made me very aware that we have a significant number of crucial pieces in our system that are inherited from the SDK and that they are not entirely isolated and safe from accidental breakage. From my perspective this is very solvable with the simulations, unit, and related integration tests that exist today in the SDK so my hope is that we will see changes that make these more accessible to SDK consumers for comprehensive testing of their own apps.

@alexanderbez
Copy link
Contributor

Module CLI integration tests are not self-contained and isolated, they are running against simapp.

Ok, then maybe we have different definitions of self-contained. Just because a suite uses simapp, doesn't mean its not self-contained and isolated. e.g. x/bank CLI integration tests are running x/bank CLI commands and x/bank only -- it doesn't rely on anything else and runs in isolation from other modules integration test suites.

@alexanderbez
Copy link
Contributor

I'm still not quite following @iramiller. How Gaia tests the CLI is radically broken, outdated, poorly designed, and most importantly, different than how we perform integration testing now (take a look at x/bank for example).

@iramiller
Copy link
Contributor

My point was not that the specific implementation used in gaia cli_test is the ideal... rather it is the ability to exercise tests built into the SDK for testing SDK functionality even when incorporated into an end user application that is important. Any group that builds with the SDK is taking a risk if they release software that has not verified the base features are still present and working correctly.

@sahith-narahari sahith-narahari marked this pull request as draft July 21, 2020 15:09
@github-actions
Copy link
Contributor

github-actions bot commented Sep 5, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Sep 5, 2020
@sahith-narahari
Copy link
Contributor

@aaronc is this still in path for stargate

cc @clevinson

@amaury1093
Copy link
Contributor

I think the proposed changes won't break that - we will need to run/select tests through command line

I'd love that. Could you write the command to run a single test in this PR? If we have that, I'm approving this PR.

@robert-zaremba
Copy link
Collaborator

I'd love that. Could you write the command to run a single test in this PR? If we have that, I'm approving this PR.

@AmauryM , here is an example with a full log:

cosmos/cosmos-sdk/x/auth/client/testutil @b18f3310* ❯ go test -tags norace -testify.m TestCLIValidateSignatures -v                               20s 16:45:43
=== RUN   TestIntegrationTestSuite
    suite.go:49: setting up integration test suite
    network.go:171: acquiring test network lock
    network.go:176: created temporary directory: /tmp/TestIntegrationTestSuite1178717921/001/chain-byZajI145057107
    network.go:185: preparing test network...
    network.go:373: starting test network...
    network.go:378: started test network
=== RUN   TestIntegrationTestSuite/TestCLIValidateSignatures
=== CONT  TestIntegrationTestSuite
    suite.go:72: tearing down integration test suite
    network.go:465: cleaning up test network...
    network.go:488: finished cleaning up test network
    network.go:462: released test network lock
--- PASS: TestIntegrationTestSuite (17.26s)
    --- PASS: TestIntegrationTestSuite/TestCLIValidateSignatures (0.09s)
PASS
ok      github.com/cosmos/cosmos-sdk/x/auth/client/testutil     17.315s

You can remove -v to surpass the suite logs.

@robert-zaremba
Copy link
Collaborator

BTW, @alessio - it's possible to use only the go test standard flags (instead of -testify.*, to filter the tests eg:

go test -tags norace -test.run TestIntegrationTestSuite/TestCLIValidateSignatures  -v

@amaury1093
Copy link
Contributor

@robert-zaremba thanks, that works 👍 . I'm good with this. @atheeshp could you fix the conflicts?

@aaronc
Copy link
Member Author

aaronc commented Apr 14, 2021

I didn't read through all changes. Could you roughly summarize, why do we need to move tests into subpackges?

Just to reiterate, the main goal of this refactoring is so that other apps can run these integration test suites against their app instead of simapp.

Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

lgtm

@amaury1093 amaury1093 requested a review from cyberbono3 April 16, 2021 13:44
@atheeshp
Copy link
Contributor

Any further actions on this PR? @clevinson , @aaronc

@aaronc
Copy link
Member Author

aaronc commented Apr 23, 2021

I'm going to move forward with merging this. It improves the testability of the SDK by apps and we will start leveraging this ASAP for additional QA in gaia, regen and other apps.

@shahankhatch, someone on our team can walk you through using this in gaia

@aaronc aaronc merged commit 3c65c3d into master Apr 23, 2021
@aaronc aaronc deleted the aaronc/6423-refactor branch April 23, 2021 18:49
@aaronc aaronc removed the backlog label Apr 23, 2021
@amaury1093 amaury1093 mentioned this pull request Jul 28, 2022
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.