In Process Integration Tests

[aaronc]

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

• testnet framework (Test Network Testing 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).

• x/bank - x/bank: Refactor CLI & Tests #6525
• x/auth - x/auth: in-process test refactor #6573, x/auth: Finish CLI Refactor #6765
• x/crisis - x/crisis: In-Process CLI Integration Tests #6634
• x/genutil
• x/distribution - x/distribution: In-Process Testing & CLI Refactor #6652
• x/evidence Update x/{mint,slashing,evidence} cli to use gRPC query client #6704
• x/params - x/params: CLI Refactor & In-Process Tests #6720
• x/gov - CLI/Tests: Remove Fixtures #6799
• x/mint - CLI/Tests: Remove Fixtures #6799
• x/slashing - x/slashing: In-Process CLI Integration Tests #6702
• x/staking - CLI/Tests: Remove Fixtures #6799
• x/ibc - x/ibc: refactor CLI #6639
• x/ibc-transfer
• x/upgrade - CLI/Tests: Remove Fixtures #6799

---

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

[sahith-narahari]

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

[anilcse]

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.

[aaronc]

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

[alexanderbez]

@aaronc this does not belong in 0.39.

[aaronc]

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

[alessio]

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

[alexanderbez]

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.

[sahith-narahari]

Sharing here for future reference,

1. https://github.com/urfave/cli
2. https://github.com/mitchellh/cli

[alexanderbez]

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.
2. 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 Test Network Testing Framework #6489).
2. We test each command individually. Use SetErr, SetIn, etc.. as needed.

[alessio]

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

[alessio]

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

[alexanderbez]

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

[aaronc]

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?

[alexanderbez]

What's Init?

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

I don't know.

[aaronc]

What's Init?

The Init* methods on CLIContext.

[alexanderbez]

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.

[aaronc]

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

[alexanderbez]

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

[aaronc]

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

[jgimeno]

Sorry by the reassign, I messed it :D

[aaronc]

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.

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.

[alexanderbez]

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

[aaronc]

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

[alexanderbez]

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.