Add createWallet and getWallet tests #82

paweljakubas · 2019-03-18T21:11:42Z

Issue Number

Overview

I have added unit tests for getWallet and createWallet
I have added CreateWalletError

Comments

I have added CreateWalletError as a way to forbid and inform about it when the same newWallet is passed through more than once in order to create wallet. As a consequence, I changed createWallet type to : NewWallet -> ExceptT CreateWalletError m WalletId

KtorZ

Tests looks good 👍
Some remarks on the overall structure though and writing spec file in general.

test/unit/Cardano/WalletLayerSpec.hs

KtorZ · 2019-03-19T08:03:24Z

test/unit/Cardano/WalletLayerSpec.hs

+            resFromDb `shouldSatisfy` isJust
+            --the same wallet creation second time
+            secondTrial <- runExceptT $ createWallet wl newWallet
+            secondTrial `shouldSatisfy` isLeft


I'd advise to make two separate tests for this. One test that focuses on creating wallets with various parameters from NewWallet, and one test that specifically test that a wallet can't be inserted twice. It's good to keep tests focused to one and one thing such that, when a test fail, you know exactly why it fails.

now, setupFixture prepares db, walletLayer and creates wallet giving its walletId

seperated this test into two as suggested

KtorZ · 2019-03-19T08:06:00Z

test/unit/Cardano/WalletLayerSpec.hs

+    res <- runExceptT $ createWallet wl newWallet
+    case res of
+        Left e ->
+            fail $ show e


I'd say that the fixture should take care of creating the wallet here. If the test is about getting a wallet, we should prepare the state to make the test able to perform what it needs. Ideally, tests are always divided in three parts (or four) parts:

The Setup / Fixture (preparing whatever is needed for the next step)

The Action (actual tests)

The Assertion (verifying the outcome of the previous step)
(- optionally, The Teardown / Cleanup (when it makes sense, releasing resources or wiping out some data created by the fixture and the action).

Each test now composes of three separated parts : setup, actions, verification of expectations

KtorZ · 2019-03-19T08:08:38Z

test/unit/Cardano/NetworkLayer/HttpBridgeSpec.hs

@@ -1,5 +1,7 @@
 module Cardano.NetworkLayer.HttpBridgeSpec
    ( spec
+    , mockNetworkLayer
+    , noLog


This feels wrong. Tests shouldn't depend on each others. I'd rather have a bit of duplication than shared code like this between tests. The rational being that the mockNetworkLayer was designed for the HttpBridgeSpec (and it goes the same for Orphan instances). Having each ***Spec file independent allows for test to be very easy to follow and maintain in the long-run. Making changes to one won't impact the others.

Especially because here, you don't need the "power" granted by the mockNetworkLayer from the HttpBridge. You could have a dummy network layer that would yield empty list of blocks working just fine for your use-case.

KtorZ · 2019-03-19T08:09:59Z

test/unit/Cardano/WalletLayerSpec.hs

+import Cardano.Wallet.AddressDiscoverySpec
+    ()
+import Cardano.Wallet.MnemonicSpec
+    ()


Same here. In practice, Arbitrary instances are local to a test because generators are tailored to the properties they test. I'd advise to re-define the instances here in this spec file, and goes for generic arbitrary as a default if it's enough to cover the need for this tests.

now, no other module arbitrary imports

akegalj · 2019-03-19T10:57:00Z

app/server/Main.hs

-        , gap = minBound
-        }
-    watchWallet wallet wid
+    widE <- runExceptT $ createWallet wallet NewWallet


why have we decided to start a wallet server with a default wallet compared to starting a wallet server without one where wallet is created on demand by calling wallet server endpoint ?

cannot answer here -> just refactor this snippet just in order to compile after changing createWallet

@akegalj there's no default wallet. Instead, there's just a very basic CLI that will "monitor" a wallet associated to some mnemonic that are given. This is merely for testing and to be able to play with our code instead of waiting 4 months to actually integrate with anything.

There's no web server at the moment, and, although we'll probably implement the beginning of this in the upcoming days, at this stage, it's still quite useful to be able to connect all the pieces together and already perform some tests to make sure that we are doing something sensible with the primitives.

The server/Main.hs is really not in its final stage.

akegalj · 2019-03-19T11:40:15Z

test/unit/Cardano/WalletLayerSpec.hs

+    , _fixtureWalletLayer :: WalletLayer IO SeqState
+    }
+
+prepareWalletLayerFixture :: IO WalletLayerFixture


Now that we are designing tests from start - do we really want to do the same test design as we did before? Defining fixtures like this, which promote generating data within function body (instead of generating fixture and other arguments as function arguments) breaks quickchecks shrink functionality.

Maybe we should reconsider this approach and generate everything within function arguments so that property body is as deterministic as possible. In this specific example everything works correctly as mockNetworkLayer is pure deterministic function. But I am afraid that this design encourages of someone in future to add

somethingArbitrary <- pick arbitrary

and to add somethingArbitrary to WalletLayerFixture to use later which would break quickchecks shrink functionality.

Maybe this is a good oportunity to have a chat with Edsko about https://iohk.io/blog/an-in-depth-look-at-quickcheck-state-machine/

Note that state machine approach won't eliminate fixture part - but instead it would become just one state transition (first one) in a chain of state transitions

I feel that the current fixture and test setup is very simple. setupFixture prepares db, network, walletLayer and creates wallet returning its db, walletLayer and corresponding walletId. For every test, the same fixture is instantiated. No complex transitions are occurring here. Maybe address state machine concept in separate PR and refactor then? Nevertheless,I think you raised very important issue here @akegalj !!!

@akegalj Your remark is sound and, note that we aren't going for the same approach as before. Here, the arbitrary generation happens via QuickCheck in the functions arguments. The fixture doesn't generate anything (and I agree, it OUGHT NOT), but rather, setup a state given some already generated data.

It will be quite tricky to do differently as, some I/O needs to happen for the property to happen, and the property is intrinsically monadic. It would be wrong to shove in any pick or generate call in the property. Yet, as they stand, the properties are totally deterministic (modulo the DB failure).

Beside, state machine testing is another stage in the testing process. They won't replace those basic unit-tests, but rather come as a complement. The nice thing with the DBLayer as it stands right now makes it possible to test the "MVar DBLayer" well, and then, use it as a model for state machine testing, comparing a more complex implementation (like an SQLite DBLayer) to the simple MVar one.

KtorZ · 2019-03-19T14:20:29Z

@paweljakubas , looking at the code coverage on those tests, we can see one interesting thing: most of the code is never evaluated. Which is rather problematic. An easy solution to this (which may also prevent some memory leaks in the meantime) would be to deepseq whatever is stored in the MVar DB layer.

import Control.DeepSeq ( deepseq )

newDBLayer :: forall s. IO (DBLayer IO s)
newDBLayer = do
    wallets <- newMVar mempty
    return $ DBLayer
        { putCheckpoints = \key cps ->
        cps `deepseq` (modifyMVar_ wallets (return . Map.insert key cps))

otherwise, unfortunately, tests are passing without executing most of the underlying code :/ ...
I'll recommend to always have a look at the code coverage when adding tests, to see if you're missing anything from the code execution (using stack test --coverage)

paweljakubas · 2019-03-19T14:44:36Z

indeed, before :

Generating coverage report for cardano-wallet's test-suite "unit"
 73% expressions used (1506/2037) 
 51% boolean coverage (14/27)     
      47% guards (10/21), 9 always True, 2 always False
      66% 'if' conditions (4/6), 1 always True, 1 always False
     100% qualifiers (0/0)        
 63% alternatives used (61/96)    
 73% local declarations used (53/72)
 53% top-level declarations used (227/421)

after the improvement @KtorZ suggested :

 Generating coverage report for cardano-wallet's test-suite "unit"
 76% expressions used (1565/2039) 
 51% boolean coverage (14/27)     
      47% guards (10/21), 9 always True, 2 always False
      66% 'if' conditions (4/6), 1 always True, 1 always False
     100% qualifiers (0/0)        
 63% alternatives used (61/96)    
 81% local declarations used (59/72)
 55% top-level declarations used (233/421)

paweljakubas · 2019-03-19T14:46:15Z

adopted the improvement ^^^ , rebased and squashed

KtorZ

LGTM

[23] Add mockup unit test [23] Add create/get wallet tests [23] fix last test [23] enhance test suite [23] Turn on FlexibleContexts [23] make sure putCheckpoints is evaluated [23] remove unneeded import from cabal

piotr-iohk · 2019-03-19T17:34:03Z

test/unit/Cardano/WalletLayerSpec.hs

+spec = do
+    describe "WalletLayer works as expected" $ do
+        it "Wallet upon creation is written down in db"
+            (checkCoverage walletCreationProp)


Does it makes sense to use checkCoverage when there is no cover or coverTable used in the property?
(Just asking, as I simply don't know :)
cc @paweljakubas , @KtorZ

@piotr-iohk Nope it doesn't :)

Without any cover or coverTable, it's equivalent to doing just property.

paweljakubas requested review from Anviking and KtorZ March 18, 2019 21:11

paweljakubas force-pushed the paweljakubas/23/create-get-wallet-impl branch 2 times, most recently from fc23a5f to 39a094d Compare March 19, 2019 07:34

KtorZ suggested changes Mar 19, 2019

View reviewed changes

KtorZ assigned paweljakubas Mar 19, 2019

akegalj reviewed Mar 19, 2019

View reviewed changes

paweljakubas force-pushed the paweljakubas/23/create-get-wallet-impl branch 2 times, most recently from 950888b to a943936 Compare March 19, 2019 13:14

paweljakubas requested a review from KtorZ March 19, 2019 13:34

paweljakubas force-pushed the paweljakubas/23/create-get-wallet-impl branch from a943936 to 1469c47 Compare March 19, 2019 14:45

KtorZ approved these changes Mar 19, 2019

View reviewed changes

[23] add createWallet

3908982

[23] Add mockup unit test [23] Add create/get wallet tests [23] fix last test [23] enhance test suite [23] Turn on FlexibleContexts [23] make sure putCheckpoints is evaluated [23] remove unneeded import from cabal

paweljakubas force-pushed the paweljakubas/23/create-get-wallet-impl branch from 1469c47 to 3908982 Compare March 19, 2019 15:20

KtorZ merged commit 9f2d97f into master Mar 19, 2019

KtorZ deleted the paweljakubas/23/create-get-wallet-impl branch March 19, 2019 15:35

paweljakubas mentioned this pull request Mar 19, 2019

Sketch wallet layer to fetch and create a wallet #23

Closed

2 tasks

KtorZ mentioned this pull request Mar 19, 2019

Connect the wallet primitive to the chain producer through the wallet layer #69

Closed

7 tasks

piotr-iohk reviewed Mar 19, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add createWallet and getWallet tests #82

Add createWallet and getWallet tests #82

paweljakubas commented Mar 18, 2019 •

edited

KtorZ left a comment

KtorZ Mar 19, 2019

paweljakubas Mar 19, 2019

paweljakubas Mar 19, 2019

KtorZ Mar 19, 2019

paweljakubas Mar 19, 2019

KtorZ Mar 19, 2019

paweljakubas Mar 19, 2019

KtorZ Mar 19, 2019

paweljakubas Mar 19, 2019

akegalj Mar 19, 2019

paweljakubas Mar 19, 2019 •

edited

KtorZ Mar 19, 2019 •

edited

akegalj Mar 19, 2019 •

edited

paweljakubas Mar 19, 2019 •

edited

KtorZ Mar 19, 2019

akegalj Mar 19, 2019

KtorZ commented Mar 19, 2019

paweljakubas commented Mar 19, 2019

paweljakubas commented Mar 19, 2019

KtorZ left a comment

piotr-iohk Mar 19, 2019 •

edited

KtorZ Mar 19, 2019

KtorZ Mar 19, 2019

Add createWallet and getWallet tests #82

Add createWallet and getWallet tests #82

Conversation

paweljakubas commented Mar 18, 2019 • edited

Issue Number

Overview

Comments

KtorZ 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

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

paweljakubas Mar 19, 2019 • edited

Choose a reason for hiding this comment

KtorZ Mar 19, 2019 • edited

Choose a reason for hiding this comment

akegalj Mar 19, 2019 • edited

Choose a reason for hiding this comment

paweljakubas Mar 19, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KtorZ commented Mar 19, 2019

paweljakubas commented Mar 19, 2019

paweljakubas commented Mar 19, 2019

KtorZ left a comment

Choose a reason for hiding this comment

piotr-iohk Mar 19, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas commented Mar 18, 2019 •

edited

paweljakubas Mar 19, 2019 •

edited

KtorZ Mar 19, 2019 •

edited

akegalj Mar 19, 2019 •

edited

paweljakubas Mar 19, 2019 •

edited

piotr-iohk Mar 19, 2019 •

edited