CLI golden tests for common command usage

[KtorZ]

Issue Number

#357

Overview

• I have transformed the comments in the CLI module about usage into executable specifications

Comments

The idea is to get some sort of visual golden tests to help us keep track of the CLI looks like and if it's not getting out of hands.

[jonathanknowles]

This looks like a big improvement. 👍

Are you planning to make similar tests for exe/wallet/{http-bridge,jormungandr}/Main.hs?

I only noticed one thing: individual command functions still have comments. For example:

-- | cardano-wallet mnemonic generate [--size=INT]
cmdMnemonicGenerate :: Mod CommandFields (IO ())    

Are these necessary, now that we have tests? Perhaps they should also be removed.

[jonathanknowles]

Do you plan to also remove the comments on functions like the one below:

-- | cardano-wallet mnemonic generate [--size=INT]
cmdMnemonicGenerate :: Mod CommandFields (IO ())         

It seems these are now superfluous, as we have:

["mnemonic", "generate", "--help"] `shouldShowUsage`  
    [ "Usage:  mnemonic generate [--size INT]"
    , "  Generate English BIP-0039 compatible mnemonic words."

[rvl]

I agree with the goal "don't let the CLI get out of control", but not fond of the mechanism used here.

Maintaining the golden strings in this way is going to be a pain.

Golden tests ensure that the help text won't change -- not even a single whitespace character is allowed to change. This is not what we want. We want a qualitative judgement about whether the CLI is sensible (i.e. "not getting out of hands").

Why don't we generate a man page style document from the CLI and upload it to the web site through CI. There are multiple ways of doing this, and the solution doesn't need to be highly sophisticated. Then QA can compare the man page between releases and assess whether it is suitable.

[jonathanknowles]

@rvl wrote:

Maintaining the golden strings in this way is going to be a pain.

If we think it's a pain to maintain strings this way, then perhaps we can follow the approach used in hspec-golden-aeson, where expected results are stored in version-controlled files?

When running the tests, we compare each expected output with the contents of the appropriate file. If a given output is unexpected, then the test suite overwrites the appropriate file. On test failure, we can check trivially to see whether it's a "real" failure. If not, then we can commit the new versions and carry on.

What do you think?

[KtorZ]

@rvl @jonathanknowles

I tend to agree with @rvl on that. This is going to be cumbersome to maintain, and we need to make sure that we keep an eye on it actually. I already imagine people updating things without much considerations (by lack of time or priority ...).

Note that we already have an command-line guide on the wiki:

https://github.com/input-output-hk/cardano-wallet/wiki/Wallet-command-line-interface

It is not automatically generated, but we do maintain it and update it on every release, with QA validation on top. Perhaps this is enough in the end.

[KtorZ]

If we think it's a pain to maintain strings this way, then perhaps we can follow the approach used in hspec-golden-aeson, where expected results are stored in version-controlled files?

@jonathanknowles I don't think this solves our issue here, unless we do indeed manually check those files (which don't really happen with the API golden tests..). We do want to keep an eye on the output and make sure the CLI remain in-control. If we tests against auto-generated strings, it's not going to help much as we could simply auto-generate "gibberish" and validate against it.

[piotr-iohk]

@rvl, @jonathanknowles, @KtorZ

Maintaining the golden strings in this way is going to be a pain.

Isn't it just the matter of adjusting the test whenever you change something? In that case it shouldn't be a lot of additional work, or maybe I miss something?

I generally like the idea. In my mind such tests can give feedback immediately (on every pr or merge to master) in case something is wrong. Off course this would not throw away manual verification of the CLI however such verification is not done every day, (it's done rather opportunistically every once in a while or just before the release). Such tests would give potenital feedback automatically and much more often.

When running the tests, we compare each expected output with the contents of the appropriate file. If a given output is unexpected, then the test suite overwrites the appropriate file.

It sounds a bit unsafe to me as we can get some unwanted content to get merged and then test against it...

[jonathanknowles]

@KtorZ

(which don't really happen with the API golden tests..)

I was under the impression that the tests fail if the output is different from the expected output? Perhaps I was mistaken? (Runs away to check.)

[piotr-iohk]

@KtorZ

I tend to agree with @rvl on that. This is going to be cumbersome to maintain, and we need to make sure that we keep an eye on it actually. I already imagine people updating things without much considerations (by lack of time or priority ...).

I think that when you have a test that you also need to look at and adjust when making a change it gives you additional opportunity to consider the change actually... ¯_(ツ)_/¯

[KtorZ]

I was under the impression that the tests fail if the output is different from the expected output? Perhaps I was mistaken? (Runs away to check.)

They do, and it happens when we change API types. Then usually, golden samples get re-generated and merged. I believe people usually look at the generated golden tests when this happens and give a justification for the change. But, I haven't actually checked them in a while. There's no manual review on the output there which is ... okay for API tests since they really are there for alerting us in case we were to change the representation of an API type in a non-compatible way.

The CLI tests here have a rather different purpose: we want to be sure that our little optparse-applicative pieces, once composed together, look reasonable and / or consistent. In that sense, we are much more interested into the output and the way it looks.

[KtorZ]

What do we do about this one?

@rvl @piotr-iohk @jonathan.knowles?

[jonathanknowles]

What do we do about this one?

@rvl @piotr-iohk @jonathan.knowles?

While the solution used in this PR has some drawbacks, I still think it's an improvement over the status quo.

With the status quo, we have our specification in comments that are checked by neither the compiler nor our test suite.

With this, at least our specification is checked, even if it's not encoded in the most efficient fashion.

How about we go ahead with this PR, and then in future if we really find that it's cumbersome to update, then perhaps we can look at the problem again?

Done 👍

[piotr-iohk]

👍