Basic integration scenarios for CLI #264

piotr-iohk · 2019-05-15T12:52:16Z

Issue Number

Overview

I have added some basic integration tests for CLI (get, list, update, delete, mnemonics, version). Let's see if this approach works... 🤞 (However, for know, still don't know how to test prompt command line with this (wallet create))

Comments

KtorZ · 2019-05-15T13:06:58Z

lib/http-bridge/test/integration/Test/Integration/Scenario/CLISpec.hs

+
+    it "CLI - Can generate mnemonics with default size" $  do
+        Stdout out <- command [] "cardano-wallet" ["mnemonic", "generate"]
+        length (splitOn " " out) `shouldBe` 15


You're probably just looking for: https://hackage.haskell.org/package/base-4.11.1.0/docs/Data-List.html#v:words from the base package. No need for the extra split package here 🙏

KtorZ · 2019-05-15T13:10:36Z

lib/http-bridge/test/integration/Test/Integration/Scenario/CLISpec.hs

+    it "CLI - Shows version" $  do
+        Stdout out <- command [] "cardano-wallet" ["--version"]
+        cabal <- readFile "../../cardano-wallet.cabal"
+        cabal `shouldContain` out


That's somewhat unsatisfactory haha ^^" ... Perhaps we can better have the bits extracting the version number from the cardano-wallet.cabal available in the libraries, or instead, expect the version to match a known version. We'll have to update that version number of each version bump, but I think that's a fair price to pay :)

KtorZ · 2019-05-15T13:11:16Z

lib/http-bridge/test/integration/Test/Integration/Scenario/CLISpec.hs

+        forM_ [9, 12, 15, 18, 21, 24] $ \size -> it (show size) $ do
+            Stdout out <- command [] "cardano-wallet"
+                            ["mnemonic", "generate", "--size", show size]
+            length (splitOn " " out) `shouldBe` size


Would deserve some negative tests too I think (not saying this should be part of this PR, but for exhaustiveness)

KtorZ · 2019-05-15T13:15:47Z

lib/http-bridge/test/integration/Test/Integration/Scenario/CLISpec.hs

+        walId <- createWallet ctx "CLI Wallet" mnemonics15
+        Stdout out <- command [] "cardano-wallet"
+                        ["wallet", "delete", "--port", "1337", walId ]
+        out `shouldNotContain` "CLI Wallet"


Interesting approach. I think we could add some special assertion function too here to verify the output even more deeply. The CLI do output some Ok or Error on stderr and, if any, output responses on stdout (without any noise). So in practice, we could even verify that whatever gets printed on stdout is always a valid JSON object.

The CmdResult allows us to check both stderr and stdout, and also response codes! Which is kinda cool. So we can definitely go crazy here 😛

Side-note: variable indentation --> not cool.

Definitely there's room for going crazy and I had the same idea to check the somehow cast the output to JSON or maybe even ApiWallet (still the main intention here was to check if the approach works)

Side-note: variable indentation --> not cool.

it was not my intention :) it's just trying to comply with 80 chars rule...

piotr-iohk · 2019-05-16T08:34:20Z

lib/http-bridge/test/integration/Test/Integration/Scenario/CLISpec.hs

+    it "CLI - Shows help with --help" $  do
+        (Exit c, Stdout out) <- command [] "cardano-wallet" ["--help"]
+        out `shouldContain` "Cardano Wallet CLI"
+        c `shouldBe` ExitFailure 1


I wonder if this is expected...

No it's not. I noticed some days ago (and fixed but never pushed it actually :|) while I was trying to generate code-completion for the CLI. That's not right -> Let's have 0 here and a fix :)

…approach works.

akegalj

looks good so far

akegalj · 2019-05-16T16:46:07Z

exe/wallet/Main.hs

@@ -149,7 +150,7 @@ Options:
 Examples:
  # Create a transaction and send 22 lovelace from wallet-id to specified addres
  cardano-wallet transaction create \
-    --wallet-id 2512a00e9653fe49a44a5886202e24d77eeb998f \
+    2512a00e9653fe49a44a5886202e24d77eeb998f \


good catch, my mistake

akegalj · 2019-05-16T16:51:28Z

lib/http-bridge/test/integration/Test/Integration/Scenario/CLISpec.hs

+specWithCluster :: SpecWith Context
+specWithCluster = do
+    it "CLI - Can get a wallet" $ \ctx -> do
+        walId <- createWallet ctx "1st CLI Wallet" mnemonics15


why not use arbitrary name and mnemonics? (I can help with this if needed)

¯_(ツ)_/¯

One benefit in case of mnemonics is that they were all generated with https://iancoleman.io/bip39/ (i.e. using something that is outside of our codebase, which makes sense in terms of testing/verifying that they work)

akegalj · 2019-05-16T16:52:36Z

lib/http-bridge/test/integration/Test/Integration/Scenario/CLISpec.hs

+        let test size = it ("--size=" <> show size) $ do
+                (Exit c, Stdout out) <- command [] "cardano-wallet"
+                    ["mnemonic", "generate", "--size", show size]
+                length (words out) `shouldBe` size


we could even test is it valid bip39 here

hm, probably yes. But then we would be using the same code that generates the mnemonics to verify if they are valid (?) Then, not sure if there would be any additional value...

piotr-iohk requested a review from KtorZ May 15, 2019 12:52

piotr-iohk self-assigned this May 15, 2019

piotr-iohk force-pushed the piotr/96/testing branch from 1b91d3c to b9a78da Compare May 15, 2019 13:09

piotr-iohk mentioned this pull request May 15, 2019

Extend cardano Wallet CLI with selected commands #96

Closed

10 tasks

piotr-iohk force-pushed the piotr/96/testing branch from b9a78da to 558666e Compare May 15, 2019 13:14

KtorZ suggested changes May 15, 2019

View reviewed changes

piotr-iohk force-pushed the piotr/96/testing branch 2 times, most recently from 564a022 to 67e5dd6 Compare May 15, 2019 21:42

piotr-iohk commented May 16, 2019

View reviewed changes

Piotr Stachyra and others added 10 commits May 16, 2019 12:05

fix CLI example (wallet-id to be positional param)

226d48b

Few positive scenarios for wallet CLI. It is mainly to check if this …

614875e

…approach works.

check version returned by CLI is the same as in cabal file

9b88f10

add exit code verification

728db74

fix exit code when using --help in CLIs --> use 0 instead of 1

04eb7fc

review formatting in CLISpec.hs

60cff92

hard-code version number in CLISpec

92baa4f

Verify stderr output from the CLI too

ce5fc10

add 'expectValidJSON' to integration DSL and verifies CLI stdout

13d8d05

add negative test for generate mnemonic

e82e0e9

KtorZ approved these changes May 16, 2019

View reviewed changes

KtorZ force-pushed the piotr/96/testing branch from 3505283 to e82e0e9 Compare May 16, 2019 10:06

KtorZ merged commit bae73e2 into master May 16, 2019

iohk-bors bot deleted the piotr/96/testing branch May 16, 2019 11:59

akegalj reviewed May 16, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic integration scenarios for CLI #264

Basic integration scenarios for CLI #264

piotr-iohk commented May 15, 2019

KtorZ May 15, 2019

KtorZ May 15, 2019

KtorZ May 15, 2019

KtorZ May 15, 2019

piotr-iohk May 15, 2019

piotr-iohk May 16, 2019

KtorZ May 16, 2019

akegalj left a comment

akegalj May 16, 2019

akegalj May 16, 2019

piotr-iohk May 17, 2019

piotr-iohk May 17, 2019

akegalj May 16, 2019

piotr-iohk May 17, 2019

Basic integration scenarios for CLI #264

Basic integration scenarios for CLI #264

Conversation

piotr-iohk commented May 15, 2019

Issue Number

Overview

Comments

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

akegalj left a 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

Choose a reason for hiding this comment