Generate DBLayer tests with quickcheck-state-machine #259

rvl · 2019-05-14T08:33:02Z

Relates to #154.

Overview

We can generate our DBLayer tests with quickcheck.

I used Edsko's blog post as a guide.

Comments

Todo list:

DBLayer methods put/readPrivateKey
shrinking of TxHistory doesn't work
Some more interesting tests can be tagged
Testing of the SeqState wallet
Tweak weightings in the command generator.
~~Use QSM parallel testing~~
Run QSM tests on Sqlite ~~and MVar~~ DBLayers.

KtorZ

Interesting 🤔 .. I got to re-read Edsko's blog post on the matter. One thing that bother me is that I couldn't find where pre-conditions for commands are located in this implementation. I wouldn't expect a "removeWallet" to be a valid transition if there hasn't been any createWallet before...
so, I might be missing something from what is being tested here maybe?

KtorZ · 2019-05-14T16:10:13Z

lib/core/test/unit/Cardano/Wallet/DBSpec2.hs

+mReadPrivateKey :: MWid -> MockOp (Maybe MPrivKey)
+mReadPrivateKey wid m@(M cp _ _ pk _)
+    | wid `Map.member` cp = (Right (Map.lookup wid pk), m)
+    | otherwise           = (Left (NoSuchWallet wid), m)


Wouldn't it be possible to leverage our already existing MVar implementation as the Mock, instead of re-encoding everything here 🤔 ? We've already some good unit tests which make sure that the MVar implementation works rather okay.

rvl · 2019-05-15T07:27:02Z

The property in this PR checks that the DBLayer implementation matches the Model.

This Model is even simpler than the MVar DB because it doesn't use MVars and isn't wrapped up in a DBLayer type. Therefore the model value can be shown in counterexamples.

The idea about preconditions (e.g. valid wallet id) is that the QSM generator should also generate negative test cases. The QSM tests should have tags to ensure that certain scenarios are tested, and the generator should have weightings to ensure interesting paths are covered.

KtorZ · 2019-05-15T14:21:50Z

@rvl I had a deeper look into this but still haven't got my head fully around it.
One thing I noticed and that appeared weird to me was that the db connection was part of the state machine model. Maybe it was on purpose, however I did remove it for It appears to be more on the land of "implementation details" in my opinion. Instead, a db connection is given to the underlying semantics function in order to interpret the commands in IO.

As a result, things are slightly simpler, and I could remove the MWid wrapper without much problem (not sure why it was there actually?).
If you think that's a bad idea, feel free to revert the commit. I'd be curious to hear why however.

rvl · 2019-05-16T11:13:33Z

The MWid wrapper was not necesary, and I already had a note to remove it.

Yes I intentionally put DB connection as part of the state machine model. This "implementation detail" is something that I would like to test.

KtorZ · 2019-05-16T11:52:20Z

The MWid wrapper was not necessary, and I already had a note to remove it.

Indeed, that's why I removed it 😅 ... But I thought it was there to make the whole thing compiles somehow. Apparently not.

Yes I intentionally put DB connection as part of the state machine model. This "implementation detail" is something that I would like to test.

👍

4040: Batch Import Addresses to 1.5.0 r=disassembler a=KtorZ ## Description  Backporting cardano-foundation/cardano-wallet#259 to 1.5.0 ## Linked issue  Co-authored-by: KtorZ <matthias.benkort@gmail.com>

lib/core/cardano-wallet-core.cabal

lib/core/test/unit/Cardano/Wallet/DB/TestWallet.hs

paweljakubas · 2019-05-17T11:43:52Z

lib/core/test/unit/Cardano/Wallet/DB/TestWallet.hs

+instance TxId DummyTarget where
+    txId = Hash . B8.pack . show
+
+newtype DummyState = DummyState Int


DummyState probably should be like :

newtype DummyStateSqlite = DummyStateSqlite (UTxO, PendingTx) deriving (Show, Eq, NFData)

Or even better to prepare test machine to take any state, ie. use state type parameter and then check this both in MVarSpec and SqliteSpec

lib/core/test/unit/Cardano/Wallet/DBSpec2.hs

akegalj

I have to admit that I didn't understand around 50% of the mechanics here on a first skim although I was using this lib a year ago. I guess edsko did use it in a more advanced way than I did.

Would have to skim it once again after I read edsko's blog post

lib/core/test/unit/Cardano/Wallet/DBSpec2.hs

KtorZ

Getting some failures while running the parallel tests.

SQLite Parallel failure

       uncaught exception: SqliteException
       SQLite3 returned ErrorError while attempting to perform step: cannot start a transaction within a transaction
       (after 5 tests and 2 shrinks)
         ParallelCommands
           { prefix = Commands { unCommands = [] }
           , suffixes =
               [ Pair
                   { proj1 =
                       Commands
                         { unCommands =
                             [ Command
                                 (At (ReadPrivateKey (Val (MWid "c"))))
                                 (At (Resp (Right (PrivateKey Nothing))))
                                 []
                             ]
                         }
                   , proj2 =
                       Commands
                         { unCommands =
                             [ Command
                                 (At
                                    (PutWalletMeta
                                       (Val (MWid "c"))
                                       WalletMetadata
                                         { name = WalletName { getWalletName = "bulbazaur" }
                                         , passphraseInfo =
                                             Just
                                               WalletPassphraseInfo
                                                 { lastUpdatedAt =
                                                     (1864 - 05 - 11) (22 : 24 : 03.767284750574) UTC
                                                 }
                                         , status = Ready
                                         , delegation = NotDelegating
                                         }))
                                 (At (Resp (Left (NoSuchWallet (Reference (Symbolic (Var 0)))))))
                                 [ Var 0 ]
                             ]
                         }
                   }
               ]
           }

SQLite parallel failure (2)

uncaught exception: SqliteException
 SQLite3 returned ErrorError while attempting to perform step: not an error
 (after 4 tests)
   ParallelCommands
     { prefix = Commands { unCommands = [] }
     , suffixes =
         [ Pair
             { proj1 =
                 Commands
                   { unCommands =
                       [ Command
                           (At
                              (CreateWallet
                                 (MWid "c")
                                 (Wallet
                                    UTxO { getUTxO = fromList [] }
                                    (fromList [])
                                    SlotId { epochNumber = 0 , slotNumber = 0 }
                                    SeqState
                                      { internalPool =
                                          AddressPool
                                            { accountPubKey =
                                                Key
                                                  { getKey =
                                                      XPub
                                                        { xpubPublicKey =
                                                            "Q\170\GS\202\198\&2KA\203\CANN'X\154 \139\DEL\FS\148\FSb\SO\RS\r\DLEAL\151\153\137\167\194"
                                                        , xpubChaincode =
                                                            ChainCode
                                                              "\204\196\"I\225y\132\196L\243\128\180\137\246,W\248@\137\225P$[\244\156Cm\v\151\t\197\143"
                                                        }
                                                  }
                                            , gap = AddressPoolGap { getAddressPoolGap = 15 }
                                            , indexedAddresses =
                                                fromList [ ... ]
                                            }
                                      , externalPool =
                                          AddressPool
                                            { accountPubKey =
                                                Key
                                                  { getKey =
                                                      XPub
                                                        { xpubPublicKey =
                                                            "Q\170\GS\202\198\&2KA\203\CANN'X\154 \139\DEL\FS\148\FSb\SO\RS\r\DLEAL\151\153\137\167\194"
                                                        , xpubChaincode =
                                                            ChainCode
                                                              "\204\196\"I\225y\132\196L\243\128\180\137\246,W\248@\137\225P$[\244\156Cm\v\151\t\197\143"
                                                        }
                                                  }
                                            , gap = AddressPoolGap { getAddressPoolGap = 10 }
                                            , indexedAddresses =
                                                fromList [ ... ]
                                            }
                                      , pendingChangeIxs = PendingIxs { pendingIxsToList = [] }
                                      })
                                 WalletMetadata
                                   { name = WalletName { getWalletName = "bulbazaur" }
                                   , passphraseInfo =
                                       Just
                                         WalletPassphraseInfo
                                           { lastUpdatedAt =
                                               (1864 - 05 - 06) (09 : 58 : 17.690387360868) UTC
                                           }
                                   , status = Restoring (Quantity Percentage { getPercentage = 66 })
                                   , delegation = NotDelegating
                                   }))
                           (At (Resp (Right (NewWallet (Reference (Symbolic (Var 0)))))))
                           [ Var 0 ]
                       ]
                   }
             , proj2 =
                 Commands
                   { unCommands =
                       [ Command
                           (At (ReadWalletMeta (Val (MWid "b"))))
                           (At (Resp (Right (Metadata Nothing))))
                           []
                       ]
                   }
             }
         ]
     }

The parallel MVar tests are also failing sometimes:

MVar parallel failure

  1) Cardano.Wallet.DB.MVar, MVar state machine tests, Parallel
       *** Failed! (after 95 tests and 203 shrinks):
       Exception:
         insertConcrets: impossible.
         CallStack (from HasCallStack):
           error, called at src/Test/StateMachine/Types/Environment.hs:73:3 in quickcheck-state-machine-0.6.0-w8NmrpYN7DCoqRW8X5Sia:Test.StateMachine.Types.Environment

I am not sure whether this is due to our implementation, SQLite itself, or, due to the QSM library... I remember Edsko saying quite a few times that the parallel testing wasn't really ready for serious uses...

Beside, on parallel tests, some pre-conditions are rather hard to satisfy / meet. I believe this is / ought to be handled properly by the state-machine library. If I recall correctly, the theory behind doing parallel QSM testing is like running every possible interleaves of the commands sequentially. But for instance, what if you create a wallet in one worker, and simply query it with the other worker. If the first worker runs first, the second end up with a wallet, if the second runs first, it got nothing.

To be honest, I'd rather have us focusing on the sequential one in this PR to get moving already, instead of a never-ending stream of work which adds always more and more complexity. Let's try to keep the scope of PR well-defined and open new PRs for additions that are out-of-scope.

lib/core/test/unit/Cardano/Wallet/DB/StateMachine.hs

rvl · 2019-05-30T07:22:19Z

prop_parallel

OK, I have disabled the parallel tests in HSpec so we can close this. I ran prop_parallel against MVar for quite a while and couldn't get that "impossible" error. The Sqlite prop_parallel errors are fixed by #337. I think the parallel tests run the same interleaving with the model and compare the set of results, but am not totally sure. In any case, I think it's useful and interesting to try it, considering how little effort it is to go from prop_sequential to prop_parallel.

putTxHistory

You are right, changing the Arbitrary TxId instance to not collide was a bad idea, because we want to test error cases.

To handle rollback, we need to update the TxMeta SQL schema to reference the checkpoint. The unique constraint would have a checkpoint ID added to it. That should work fine.

Because TxId is a hash of inputs and outputs, we expect a transaction of a certain TxId to be immutable and unique. If it were a different transaction, then it would have a different ID.

I think throwing an ErrorConstraint exception was the wrong thing for the DB layer to do, because there may be multiple TxMeta entries for a single Tx (multiple checkpoints of multiple wallets). When adding a (Tx, TxMeta), the DBLayer needs to decide which of the "old" or "new" Tx to keep. But it doesn't matter because they are the same Tx. I will pick the "new" Tx.

It is a valid action to insert the exact same Tx twice (it may belong to multiple TxHistories). It's not a valid action to put a different Tx at the same ID. In that case, last Tx wins (an arbitrary choice). So I have updated the QSM model accordingly, and fixed the Sqlite DBLayer.

It might be a good consistency check to recalculate the TxId after reading it from the database (if that's not too expensive).

QSM test was failing with this: Open CreateWallet b CreateWallet a ListWallets PostconditionFailed "PredicateC (Resp (Right (WalletIds [MWid \"b\",MWid \"a\"])) :/= Resp (Right (WalletIds [MWid \"a\",MWid \"b\"])))" /= Ok

And put back the Arbitrary (Hash "Tx") instance.

The model of dbPropertyTests doesn't match how the relational database works.

jonathanknowles · 2019-05-31T04:12:12Z

lib/core/src/Cardano/Wallet/DB/Sqlite.hs

@@ -213,7 +215,8 @@ newDBLayer fp = do
                  Nothing -> pure $ Left $ ErrNoSuchWallet wid

        , listWallets = runQuery' $
-              map (PrimaryKey . unWalletKey) <$> selectKeysList [] []
+              map (PrimaryKey . unWalletKey) <$>
+              selectKeysList [] [Asc WalTableId]


Just wondering: is there any particular reason for this ordering?

There is no complex dependency between DBLayer commands, so the Expr type is not needed.

jonathanknowles · 2019-05-31T04:35:08Z

lib/core/test/unit/Cardano/Wallet/DB/StateMachine.hs

+-------------------------------------------------------------------------------}
+
+newtype At f r
+    = At (f (Reference WalletId r))


Would it be possible to add some kind of comment to describe what this type is for? For example, what do f and r refer to?

KtorZ · 2019-05-31T07:13:12Z

lib/core/src/Cardano/Wallet/DB/Sqlite.hs

+    insertState (wid, sl) _ = insert_ (SeqState wid sl)
+    selectState (wid, sl) = fmap (const DummyState) <$>
+        selectFirst [SeqStateTableWalletId ==. wid, SeqStateTableCheckpointSlot ==. sl] []
+    deleteState wid = deleteWhere [SeqStateTableWalletId ==. wid]


This shouldn't be in the source code IMO :/

KtorZ · 2019-05-31T07:13:44Z

lib/core/src/Cardano/Wallet/DB.hs

+
+-- | Clean a database by removing all wallets.
+cleanDB :: Monad m => DBLayer m s t -> m ()
+cleanDB db = listWallets db >>= mapM_ (runExceptT . removeWallet db)


I am not sure we do want to keep this in the source code. This is purely for testing.

KtorZ · 2019-05-31T07:15:35Z

lib/core/test/unit/Cardano/Wallet/DBSpec.hs

+data SkipTests
+    = RunAllTests
+    | SkipTxHistoryReplaceTest
+    deriving (Show, Eq)


This looks a bit dodgy 😶 . What exactly is the issue with this test? If there's an issue, let's disable it temporarily and open a bug ticket about fixing it instead?

KtorZ · 2019-05-31T07:17:57Z

lib/core/test/unit/Cardano/Wallet/DB/StateMachine.hs

+
+{-# OPTIONS_GHC -fno-warn-orphans #-}
+
+module Cardano.Wallet.DB.StateMachine


I just realized Edsko's blog post isn't mentioned anywhere in this file. It'll be nice to have a reference to it as a comment 👍

KtorZ · 2019-05-31T07:19:56Z

lib/core/test/unit/Cardano/Wallet/DB/StateMachine.hs

+unMockWid (MWid wid) = WalletId . hash . B8.pack $ wid
+
+-- | Represent (XPrv, Hash) as a string.
+type MPrivKey = String


I wonder why we do have this mock XPrv and Hash in a first place? The generator only takes keys among a fixed list of 3 elements anyway, so we could as well just go for the right type from the start 🤔 ?

KtorZ · 2019-05-31T07:21:26Z

lib/core/test/unit/Cardano/Wallet/DB/StateMachine.hs

+
+-- | Mock wallet ID -- simple and easy to read
+newtype MWid = MWid String
+    deriving (Show, Eq, Ord, Generic)


Same remark here, I don't think the extra indirection is helpful here. It adds another layer of indirection that has little value? Arbitrary wallet id are easy to generate (especially if just taken from a fixed list of elements) and, this does increase the cognitive load necessary to process this module. Don't you think?

Problem here was that our generator would generate inputs, outputs and transaction ids independently. While this didn't matter much with the MVar implementation, it does create weird inconsistency with the relational model because indeed, we expect inputs associated with a particular tx id to not differ when associated with the same tx id. Since a transaction id is actually a hash over the content of the transaction, changing the transaction should yield a different id. So now, we generate "fake" txid by computing them from the actual transaction content (as done in production, we simply don't hash it in the test as this has little value here but cost quite some computing power).

We use to create a fresh new DB layer before the run of each property because this was very cheap. Now that we do create a db layer only once before a given property, we need to make sure that the state of the DB is at least cleaned up between each property.

KtorZ

Merging as-is. We can iterate on a few remaining parts in future PRs.

Nice job on pulling that out @rvl
@jonathanknowles Feedback is still welcome afterwards.

I've also ran the sequential tests ~10000 times trying to catch something. Seems pretty stable now.

4041: Batch Import Addresses to 1.4.2 r=disassembler a=KtorZ ## Description  Backporting cardano-foundation/cardano-wallet#259 to 1.4.2 ## Linked issue  Co-authored-by: KtorZ <matthias.benkort@gmail.com>

rvl added this to the SQLite implementation for the DB Layer milestone May 14, 2019

rvl self-assigned this May 14, 2019

KtorZ reviewed May 14, 2019

View reviewed changes

KtorZ force-pushed the rvl/154/db-layer-qsm branch from 492dd74 to 07e79d5 Compare May 16, 2019 11:53

paweljakubas reviewed May 17, 2019

View reviewed changes

lib/core/cardano-wallet-core.cabal Outdated Show resolved Hide resolved

paweljakubas reviewed May 17, 2019

View reviewed changes

lib/core/test/unit/Cardano/Wallet/DB/TestWallet.hs Outdated Show resolved Hide resolved

paweljakubas reviewed May 17, 2019

View reviewed changes

lib/core/test/unit/Cardano/Wallet/DBSpec2.hs Outdated Show resolved Hide resolved

akegalj reviewed May 20, 2019

View reviewed changes

lib/core/test/unit/Cardano/Wallet/DBSpec2.hs Outdated Show resolved Hide resolved

lib/core/test/unit/Cardano/Wallet/DBSpec2.hs Outdated Show resolved Hide resolved

akegalj mentioned this pull request May 21, 2019

Extend the current DB layer with an SQLite interface #154

Closed

12 tasks

rvl force-pushed the rvl/154/db-layer-qsm branch 3 times, most recently from c56248a to a7a7b23 Compare May 27, 2019 05:41

rvl changed the title ~~wip: Generate DBLayer tests with quickcheck-state-machine~~ Generate DBLayer tests with quickcheck-state-machine May 28, 2019

rvl marked this pull request as ready for review May 28, 2019 06:50

KtorZ reviewed May 28, 2019

View reviewed changes

lib/core/test/unit/Cardano/Wallet/DBSpec2.hs Outdated Show resolved Hide resolved

KtorZ force-pushed the rvl/154/db-layer-qsm branch 3 times, most recently from 87d7633 to 5caaf21 Compare May 28, 2019 13:55

rvl force-pushed the rvl/154/db-layer-qsm branch from 1203861 to 1344933 Compare May 29, 2019 06:37

KtorZ reviewed May 29, 2019

View reviewed changes

lib/core/test/unit/Cardano/Wallet/DB/StateMachine.hs Outdated Show resolved Hide resolved

rvl force-pushed the rvl/154/db-layer-qsm branch 2 times, most recently from 1df0e8a to 75c0b6c Compare May 30, 2019 07:12

rvl force-pushed the rvl/154/db-layer-qsm branch from 75c0b6c to 0bc32ff Compare May 30, 2019 07:57

rvl and others added 9 commits May 31, 2019 12:31

Generate DBLayer tests with quickcheck-state-machine

10d9ee5

Move cleanDB to DBLayer

ad0c18e

run sqlite migration silently

ddfeb8c

Sqlite: Make listWallets ordering explicit

2691f0b

QSM test was failing with this: Open CreateWallet b CreateWallet a ListWallets PostconditionFailed "PredicateC (Resp (Right (WalletIds [MWid \"b\",MWid \"a\"])) :/= Resp (Right (WalletIds [MWid \"a\",MWid \"b\"])))" /= Ok

Sqlite: Transaction inputs need to be ordered

aa5e7a6

Disable QSM parallel tests until we're sure they work

e070326

QSM: Adjust the putTxHistory model to handle updating transactions

e6a456d

And put back the Arbitrary (Hash "Tx") instance.

Sqlite: Allow adding the same transaction with putTxHistory

e4b8064

SqliteSpec: Skip TxHistory put+read tests for Sqlite

bb7895b

The model of dbPropertyTests doesn't match how the relational database works.

rvl force-pushed the rvl/154/db-layer-qsm branch from 1ef96a0 to bb7895b Compare May 31, 2019 02:31

jonathanknowles reviewed May 31, 2019

View reviewed changes

rvl added 2 commits May 31, 2019 15:35

DBLayer QSM: Remove WalletId expressions

2ddf77d

There is no complex dependency between DBLayer commands, so the Expr type is not needed.

DBSpec: Fix comment on Arbitrary TxIn

575746d

jonathanknowles reviewed May 31, 2019

View reviewed changes

KtorZ reviewed May 31, 2019

View reviewed changes

KtorZ added 3 commits May 31, 2019 15:46

remove redundant constraints

8d973f2

KtorZ approved these changes May 31, 2019

View reviewed changes

KtorZ merged commit e679901 into master May 31, 2019

iohk-bors bot deleted the rvl/154/db-layer-qsm branch May 31, 2019 15:01

jonathanknowles mentioned this pull request Jun 4, 2019

Some small fixes to DB.StateMachine module. #349

Merged

KtorZ removed this from the SQLite implementation for the DB Layer milestone Jun 5, 2019

jonathanknowles mentioned this pull request Jun 6, 2019

QSM DB tests don't check that all tags are covered. #353

Closed

1 task

rvl mentioned this pull request Jun 6, 2019

Clean ups of Sqlite test code #378

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate DBLayer tests with quickcheck-state-machine #259

Generate DBLayer tests with quickcheck-state-machine #259

rvl commented May 14, 2019 •

edited

Loading

KtorZ left a comment

KtorZ May 14, 2019

rvl commented May 15, 2019

KtorZ commented May 15, 2019

rvl commented May 16, 2019

KtorZ commented May 16, 2019

paweljakubas May 17, 2019 •

edited

Loading

akegalj left a comment •

edited

Loading

KtorZ left a comment

rvl commented May 30, 2019

jonathanknowles May 31, 2019 •

edited

Loading

jonathanknowles May 31, 2019

KtorZ May 31, 2019

KtorZ May 31, 2019

KtorZ May 31, 2019

KtorZ May 31, 2019

KtorZ May 31, 2019

KtorZ May 31, 2019

KtorZ left a comment


		{-# OPTIONS_GHC -fno-warn-orphans #-}

		module Cardano.Wallet.DB.StateMachine

Generate DBLayer tests with quickcheck-state-machine #259

Generate DBLayer tests with quickcheck-state-machine #259

Conversation

rvl commented May 14, 2019 • edited Loading

Overview

Comments

KtorZ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvl commented May 15, 2019

KtorZ commented May 15, 2019

rvl commented May 16, 2019

KtorZ commented May 16, 2019

paweljakubas May 17, 2019 • edited Loading

Choose a reason for hiding this comment

akegalj left a comment • edited Loading

Choose a reason for hiding this comment

KtorZ left a comment

Choose a reason for hiding this comment

rvl commented May 30, 2019

prop_parallel

putTxHistory

jonathanknowles May 31, 2019 • edited Loading

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

KtorZ left a comment

Choose a reason for hiding this comment

rvl commented May 14, 2019 •

edited

Loading

paweljakubas May 17, 2019 •

edited

Loading

akegalj left a comment •

edited

Loading

jonathanknowles May 31, 2019 •

edited

Loading