Cosmos-sdk: In Process Integration Tests

Created on 12 Jun 2020  路  27Comments  路  Source: cosmos/cosmos-sdk

Summary

Running integration tests in a single process will improve code coverage metrics and make debugging easier.

Problem Definition

Currently CLI integration tests work by building CLI binaries and then calling the CLI binaries as sub-processes. While this is a somewhat realistic simulation of actual usage, it has the following downsides:

  • integration test coverage isn't reflected in code coverage metrics, therefore we can't

    • account for code coverage which is actually achieved by integration tests, and

    • have no way of knowing which CLI/REST/etc. code isn't covered

  • debugging is hard - there's no way to set breakpoints for failure either in the simd or simcli binaries

Proposal

  • [x] testnet framework (#6489)

Use the new testutil package to test command instances directly and in-process. As we're refactoring existing tests, we should be careful to construct/replace clean auxiliary functions for common operations (e.g. MsgSend).

Original Proposal (for future reference)

1. Make *cobra.Cmds respect stdin, stdout, & args overrides

All CLI methods are implemented using *cobra.Cmd which has SetIn, SetOut, SetArgs, etc. methods to override the default os.StdIn, os.Stdout, os.Args. As long as Cmds and the code they call respect these (which so far seems to be generally the case), Cmds can be called in the same process as test code. Only minimal changes to CLI test code would be required, mainly the fixtures.

2. Expose rootCmd's to integration test fixture

  • Expose GetRootCmd func's for simd/simcli
  • Setup build flags for cli_test_in_process to either use binaries as we currently do -or- use simd/simcli GetRootCmds and run in process
  • Adjust cli test fixture accordingly

3. Make every cli test suite a testify Suite that can be re-used by other apps

Each CLI test file should be restructured into a Suite that takes a config object. The config object will define how tests are run and will allow other apps building on the SDK to inject their own app config's and run the same suites for their apps.

4. Run all CLI tests in the SDK from a single simapp/cli_test.go file

This single simapp/cli_test.go file should import the suites from each module, populate them with the simapp config and run them one by one. This file would serve as an example for other apps as to how to setup their CLI tests.

For Admin Use

  • [ ] Not duplicate issue
  • [ ] Appropriate labels applied
  • [ ] Appropriate contributors tagged
  • [ ] Contributor assigned/self-assigned
tests
Source
picture aaronc
👍3

Most helpful comment

Assigning this to me and @alessio. I have ideas in my head on how to approach this. I will take a stab at a PoC over the coming days. However, I will take this on full-time after we release 0.39.

picture alexanderbez on 19 Jun 2020
❤4 👍1

All 27 comments

I feel using a testify Suite would be better and clean. This should also help us eliminate cli_tests from gaia and other applications using sdk.
Any thoughts @anilCSE

sahith-narahari picture sahith-narahari on 18 Jun 2020

Thanks @aaronc for the proposal. I had a feeling that in-process might not work best for cli tests but looks easy with the mentined approaches.

I vote for proposal 4 and 3. Especially 4 looks clean and simple. 2 looks flexible as it allows to chose binaries or GetRootCmd to run in-process but I don't see any advantage with that.

anilCSE picture anilCSE on 18 Jun 2020

Actually @anilCSE, these aren't separate proposals as it's currently written, they're just steps towards implementing. Maybe I shouldn't have numbered them and that makes it more confusing...

aaronc picture aaronc on 18 Jun 2020

@aaronc this does not belong in 0.39.

alexanderbez picture alexanderbez on 18 Jun 2020

I know this is somewhat involved @alexanderbez but we may want to consider this as part of battle testing 0.39. If we have accurate coverage metrics of client-facing code, we can improve coverage and potentially prevent bugs from getting into the release. (I would note that we only need to do steps 1 & 2 to get this benefit...)

aaronc picture aaronc on 18 Jun 2020
👍2

I support this (this may involve forking spf13/cobra though)

alessio picture alessio on 19 Jun 2020

Assigning this to me and @alessio. I have ideas in my head on how to approach this. I will take a stab at a PoC over the coming days. However, I will take this on full-time after we release 0.39.

alexanderbez picture alexanderbez on 19 Jun 2020
❤4 👍1

Sharing here for future reference,
1) https://github.com/urfave/cli
2) https://github.com/mitchellh/cli

sahith-narahari picture sahith-narahari on 20 Jun 2020

1a. Thinking about it more, we should not switch to a new lib unless we have very strong reasons to do so.
1b. Consider sticking with Cobra and refactor the way we build and expose commands.

  1. I propose we do not go with (4) in the issue body. CLI commands and API handlers should be tested locally to their definition.

The concrete proposal:

  1. Each cli package creates and uses a Suite, where it creates a testing framework (a la https://github.com/cosmos/cosmos-sdk/pull/6489).
  2. We test each command individually. Use SetErr, SetIn, etc.. as needed.
alexanderbez picture alexanderbez on 23 Jun 2020
👍2

I agree with @alexanderbez's comment. It all seems an excellent tradeoff between pragmatism and correctness.

alessio picture alessio on 23 Jun 2020

Plus, replacing cobra/viper with other libs would be a terribly painful process for both us and SDK clients at this stage.

alessio picture alessio on 23 Jun 2020
👍1

@alessio we need to rid ourselves of Viper here in order to take the cleanest path of least resistance.

Stuff like this has to go:

clientCtx = clientCtx.InitWithInputAndFrom(cmd.InOrStdin(), args[0])

We need to solely rely on args and cmd.Flags()

alexanderbez picture alexanderbez on 26 Jun 2020 
clientCtx = clientCtx.InitWithInputAndFrom(cmd.InOrStdin(), args[0])

So should we be actually passing a *cobra.Cmd instance to Init?

How will we deal with the config file support that viper provides?

aaronc picture aaronc on 26 Jun 2020

What's Init?

How will we deal with the config file support that viper provides?

I don't know.

alexanderbez picture alexanderbez on 26 Jun 2020

What's Init?

The Init* methods on CLIContext.

aaronc picture aaronc on 26 Jun 2020

That looks like a codesmell. If we stick solely to args, its the cleanest and safest way. @alessio and @jgimeno have a good path forward.

alexanderbez picture alexanderbez on 26 Jun 2020

FYI Viper alternatives:
https://github.com/lalamove/konfig
https://github.com/knadh/koanf

aaronc picture aaronc on 26 Jun 2020

@aaronc I don't think we need an alternative to Viper since viper can be used in a non-singleton way (i.e. you can create literal vipers). We just need to remove the use of viper from CLI and use it only where needed preferably as a literal.

Over the weekend, I'll pick a module and refactor it to demonstrate this. I'm hoping @jgimeno can pick use that as a reference and take up the other modules. Eventually, we'll remove the fixtures stuff as well.

alexanderbez picture alexanderbez on 26 Jun 2020

@aaronc I don't think we need an alternative to Viper since viper can be used in a non-singleton way (i.e. you can create literal vipers). We just need to remove the use of viper from CLI and use it only where needed preferably as a literal.

Ah I see... Maybe we could even just store a literal viper on client.Context then which would make it more amenable to testing.

Over the weekend, I'll pick a module and refactor it to demonstrate this. I'm hoping @jgimeno can pick use that as a reference and take up the other modules. Eventually, we'll remove the fixtures stuff as well.

Great!

aaronc picture aaronc on 26 Jun 2020
👍2

I've started x/bank => https://github.com/cosmos/cosmos-sdk/pull/6525

I think the scope of client.Context will change -- I'm hoping we can remove a lot of cruft and things we don't need from there. For query commands, all we need a context to have at a minimum is a client, marshaler, and verifier.

alexanderbez picture alexanderbez on 26 Jun 2020
🚀2

@alexanderbez I wonder if it's better to wait till you finish this module to use same approach for others or I can take next one?

EDIT: Just saw your comment, will check your update and I will take from there. I don't know if maybe you can take some ideas on how I solved the testing for the export params test.

jgimeno picture jgimeno on 27 Jun 2020

@jgimeno l think it would be useful if you add tests for the REST issue you had with gaia.

aaronc picture aaronc on 27 Jun 2020

Sorry by the reassign, I messed it :D

jgimeno picture jgimeno on 2 Jul 2020

So there are a couple things I would like to do differently with these in process integration tests. I tried addressing this in the architecture call, but the conclusion was more or less that I had to show the approach with PRs and documentation. So I'm making an attempt as a starting point to explain what I would do differently here. Also, my original proposal was removed from the issue body and that describde in pretty good detail how I was hoping this would be handled. I've copied it below so that it's visible.

Basically, I would like tests to allow for two things:

  • customizing the Config that goes into the test suite so that it can be used from different apps
  • not using locally scoped command constructors like bankcli.NewSendTxCmd() directly in tests but instead allowing a top-level *cobra.Command to be passed in so that we can test the actual full CLI command that an app uses

This is documented in the original proposal below. I will try to explain it more clearly with a PR.

I also never advocated getting rid of option to use the current out-of-process tests although maybe it's too late to change course now. My original proposal allowed for either type of test to be selected.

For any of these issues I would have appreciated having a discussion that allowed the pros and cons to be considered because I feel the approach that is being taken now is less valuable for developers writing app's outside of the SDK. But I don't know, maybe I just need to make a PR that does what I'm saying....

Original Proposal

Summary

Running integration tests in a single process will improve code coverage metrics and make debugging easier.

Problem Definition

Currently CLI integration tests work by building CLI binaries and then calling the CLI binaries as sub-processes. While this is a somewhat realistic simulation of actual usage, it has the following downsides:

integration test coverage isn't reflected in code coverage metrics, therefore we can't
account for code coverage which is actually achieved by integration tests, and
have no way of knowing which CLI/REST/etc. code isn't covered
debugging is hard - there's no way to set breakpoints for failure either in the simd or simcli binaries
Proposal

  1. Make *cobra.Cmds respect stdin, stdout, & args overrides

All CLI methods are implemented using *cobra.Cmd which has SetIn, SetOut, SetArgs, etc. methods to override the default os.StdIn, os.Stdout, os.Args. As long as Cmds and the code they call respect these (which so far seems to be generally the case), Cmds can be called in the same process as test code. Only minimal changes to CLI test code would be required, mainly the fixtures.

  1. Expose rootCmd's to integration test fixture

Expose GetRootCmd func's for simd/simcli
Setup build flags for cli_test_in_process to either use binaries as we currently do -or- use simd/simcli GetRootCmds and run in process
Adjust cli test fixture accordingly

  1. Make every cli test suite a testify Suite that can be re-used by other apps

Each CLI test file should be restructured into a Suite that takes a config object. The config object will define how tests are run and will allow other apps building on the SDK to inject their own app config's and run the same suites for their apps.

  1. Run all CLI tests in the SDK from a single simapp/cli_test.go file

This single simapp/cli_test.go file should import the suites from each module, populate them with the simapp config and run them one by one. This file would serve as an example for other apps as to how to setup their CLI tests.

aaronc picture aaronc on 13 Jul 2020
👍3

@aaronc your original proposal wasn't removed -- it's still in the body and commented out. I commented it out because what you're describing is of a different frame. What we have been working on thus far is needed and I believe the correct approach. What you're describing is, I believe, ancillary to test coverage and integration testing.

By all means, we should have a top-down, fully bootstrapped integration test suite (i.e. SimApp CLI and REST) that would live in the simapp pkg, which compliments the current tests being added. As I mentioned before, documentation isn't necessarily required. When you have the time and resources available, I recommend taking on this work 馃憤

alexanderbez picture alexanderbez on 13 Jul 2020
👍1

6711 is a small PR which makes these suites more reusable by apps.

I had wanted to allow the cmd's themselves to be based off of the root cli cmd too, but that's more involved and maybe the way it is actually okay. I need to think about it a bit more.

aaronc picture aaronc on 13 Jul 2020

Going to close this issue. Although not all modules have 100% test coverage of CLI commands, we're very close and all modules have been used to the revised CLI structure.

alexanderbez picture alexanderbez on 21 Jul 2020
🎉1
Was this page helpful?
0 / 5 - 0 ratings

Related issues

ValarDragon picture ValarDragon  路  3Comments
faboweb picture faboweb  路  3Comments
rigelrozanski picture rigelrozanski  路  3Comments
rigelrozanski picture rigelrozanski  路  3Comments
jackzampolin picture jackzampolin  路  3Comments