Enable Integration Tests for Jörmungandr (Part III: Command-Line tests) #496

KtorZ · 2019-07-02T16:33:04Z

Issue Number

#358

Overview

I have introduce a new class-constraints KnownCommand to allow abstracting the name of the command-line interface from the CLI DSL and scenarios
I have renamed cardano-wallet executable into cardano-wallet-http-bridge
I isolated the Server specs for http-bridge and created a specific one for Jormungandr
I have enabled most of the integration tests specs, except the Ports one (require a slightly more complex setup)

To be done:

Enable the Port tests
Review .travis.yaml deployment script

Comments

⚠️ Base branch is KtorZ/cli-optparse-applicative ⚠️

spoiler: Most of this PR is about @t.

KtorZ · 2019-07-02T16:35:13Z

lib/core/test/integration/Test/Integration/Scenario/API/Wallets.hs

-        Stdout mnemonics <- generateMnemonicsViaCLI []
-        let payldCrt = payloadWith "!st created" (T.words $ T.pack mnemonics)
+        mnemonics <- mnemonicToText @15 . entropyToMnemonic <$> genEntropy
+        let payldCrt = payloadWith "!st created" mnemonics


I've "inlined" this CLI call since there's no reason the API scenarios would require something from the CLI (and this complexifies calling these specs for now reasons). We do have dedicated CLI specs for testing mnemonic generation so, all good.

KtorZ · 2019-07-02T16:36:14Z

lib/http-bridge/test/integration/Main.hs

+instance KnownCommand (HttpBridge n) where
+    commandName = "cardano-wallet-http-bridge"
+
+main :: forall t. (t ~ HttpBridge 'Testnet) => IO ()


And this is where the magic happens and where we finally get to choose which CLI the specs should run on.

KtorZ · 2019-07-02T16:37:47Z

lib/core/test/integration/Test/Integration/Scenario/CLI/Transactions.hs

        err `shouldBe` "Please enter a passphrase: **************\nOk.\n"
        txJson <- expectValidJSON (Proxy @(ApiTransaction t)) out
        verify txJson
-            [ expectCliFieldBetween amount (feeMin + amt, feeMax + amt)
+            [ expectCliFieldBetween amount (feeMin + (2*amt), feeMax + (2*amt))


So, this was left unnoticed because the fee amounts are much bigger on the bridge scenarios and make the amount completely insignificant. This isn't the case with Jörmungandr where the amount and fee are more of less of the same order of magnitude!

rvl · 2019-07-03T09:42:59Z

lib/core/test/integration/Test/Integration/Framework/DSL.hs

-deleteWalletViaCLI :: (CmdResult r, HasType Port s) => s -> String -> IO r
-deleteWalletViaCLI ctx walId = cardanoWalletCLI
+deleteWalletViaCLI
+    :: forall t r s. (CmdResult r, KnownCommand t, HasType Port s)


Maybe we could use a constraint synonym to reduce noise.

Yeah, thought of it but some functions have slightly different constraints, and I thought that introducing sometimes an alias, and sometimes not will create more noise than help. In the end, it will mean one other type of constraint to load into our brain when reading :/

rvl · 2019-07-03T09:48:14Z

lib/jormungandr/test/integration/Test/Integration/Jormungandr/Scenario/CLI/Server.hs

+                let process = proc' (commandName @t) args
+                withCreateProcess process $ \_ _ _ ph -> do
+                    expectPathEventuallyExist db
+                    expectPathEventuallyExist (db <> "-shm")


I know this is copy&pasted from somewhere else, but will comment here anyway.

I don't think we need to assert that the -shm and -wal files exist. They are meant to be temporary files, and are a sqlite implementation detail. We just need to assert the db file was created.

Also if the -wal file exists after exit then that means we have not closed the database connection properly. Hmm.

I asked a question in the past about this on a review (about what where those -shm and -wal expected to be). I agree that we could reduce this to test whether the db file exists as the rest seems more like an "implementation detail" from SQLite & Persistent (as far as I understand...)

This allows for running the same scenarios using different command line interfaces

…implement 'KnownCommand' for it

Remains: the port tests which are a bit trickier. Will do in a separate commit

…tion

Rationale is that these files are rather internal to the way the database works. So, there's no reason we should be even considering them. As long as the db file exists, we are fine.

KtorZ requested review from jonathanknowles and piotr-iohk July 2, 2019 16:33

KtorZ self-assigned this Jul 2, 2019

KtorZ changed the base branch from master to KtorZ/cli-optparse-applicative July 2, 2019 16:33

KtorZ commented Jul 2, 2019

View reviewed changes

jonathanknowles changed the base branch from KtorZ/cli-optparse-applicative to master July 3, 2019 07:19

jonathanknowles force-pushed the KtorZ/cli-integration-tests-multiple-backend branch from b642f83 to d310442 Compare July 3, 2019 07:34

KtorZ force-pushed the KtorZ/cli-integration-tests-multiple-backend branch from e8e1e4e to 2631f81 Compare July 3, 2019 08:35

jonathanknowles mentioned this pull request Jul 3, 2019

Use a shared library for integration tests rather than a symlink. #499

Closed

2 tasks

rvl approved these changes Jul 3, 2019

View reviewed changes

KtorZ and others added 7 commits July 3, 2019 14:11

remove use of 'generateMnemonicsViaCLI' from API scenarios

600bcd5

define command-line name through a type constraint via 'KnownCommand'

44ab3dc

This allows for running the same scenarios using different command line interfaces

name 'cardano-wallet' executable to 'cardano-wallet-http-bridge' and …

335bbac

…implement 'KnownCommand' for it

enable (almost all) CLI integration tests for Jörmungandr

5f1dbe0

Remains: the port tests which are a bit trickier. Will do in a separate commit

fix oversight about amount comparison for multiple output CLI transac…

bbaef19

…tion

remove assertion on temporary db files

344ba7a

Rationale is that these files are rather internal to the way the database works. So, there's no reason we should be even considering them. As long as the db file exists, we are fine.

Make nix happy again.

9d929a4

KtorZ force-pushed the KtorZ/cli-integration-tests-multiple-backend branch from 2631f81 to 9d929a4 Compare July 3, 2019 12:13

KtorZ merged commit 52eb698 into master Jul 3, 2019

KtorZ deleted the KtorZ/cli-integration-tests-multiple-backend branch July 4, 2019 16:15

KtorZ mentioned this pull request Jul 4, 2019

Integration tests on Jörmungandr #358

Closed

3 tasks

KtorZ added this to the Jörmungandr Integration Testing milestone Jul 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Integration Tests for Jörmungandr (Part III: Command-Line tests) #496

Enable Integration Tests for Jörmungandr (Part III: Command-Line tests) #496

KtorZ commented Jul 2, 2019 •

edited

Loading

KtorZ Jul 2, 2019

KtorZ Jul 2, 2019 •

edited

Loading

KtorZ Jul 2, 2019

rvl Jul 3, 2019

KtorZ Jul 3, 2019

rvl Jul 3, 2019

KtorZ Jul 3, 2019 •

edited

Loading

Enable Integration Tests for Jörmungandr (Part III: Command-Line tests) #496

Enable Integration Tests for Jörmungandr (Part III: Command-Line tests) #496

Conversation

KtorZ commented Jul 2, 2019 • edited Loading

Issue Number

Overview

Comments

KtorZ Jul 2, 2019

Choose a reason for hiding this comment

KtorZ Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

KtorZ Jul 2, 2019

Choose a reason for hiding this comment

rvl Jul 3, 2019

Choose a reason for hiding this comment

KtorZ Jul 3, 2019

Choose a reason for hiding this comment

rvl Jul 3, 2019

Choose a reason for hiding this comment

KtorZ Jul 3, 2019 • edited Loading

Choose a reason for hiding this comment

KtorZ commented Jul 2, 2019 •

edited

Loading

KtorZ Jul 2, 2019 •

edited

Loading

KtorZ Jul 3, 2019 •

edited

Loading