Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sqlite: enable db property tests and fix failures #300

Merged
merged 10 commits into from
May 24, 2019

Conversation

rvl
Copy link
Contributor

@rvl rvl commented May 22, 2019

Relates to issue #154

Overview

  • Runs the dbPropertyTests in SqliteSpec.
  • Adds fixes for the failures.

@paweljakubas
Copy link
Contributor

@rvl These are my conclusions (hope will help you hammer out the solution) :

  1. They are deterministic and strange. For example, when I am running :
$ stack test --ta '-m "put . read yields a result"' cardano-wallet-core:test:unit

the test fails for "put . read yields a result" -> Tx History always at 63/100 test and this is irrespective of what WalletId was chosen.
2. The tests fail because of ErrWalletAlreadyExists. I investigated what happens. So in passing test we have something like that :

cleanDB wal BEFORE : [PrimaryKey (WalletId {getWalletId = 854fed8387aabb4ed709147b98b3f0c4e4804691})]
cleanDB wal AFTER : []
wal=[]
key=PrimaryKey (WalletId {getWalletId = ad54f8e826744f5fa76a1730bdf9b65771a6e976})

so we have cleanDB doing its job, as a result when the key is chosen for the test case (by createWallet) the wal=[] (this is listWallet). I would say this is as expected. In case of failure we have,

wal=[PrimaryKey (WalletId {getWalletId = ad54f8e826744f5fa76a1730bdf9b65771a6e976})]
      Tx History FAILED [1]
cleanDB wal BEFORE : [PrimaryKey (WalletId {getWalletId = ad54f8e826744f5fa76a1730bdf9b65771a6e976})]
cleanDB wal AFTER : []

ie., wal is not empty, test fails. cleanDB is missing. It comes only when the next test case runs.
This is very strange. as we use beforeAll in a predictable way we have situation that in 63/100 test the call to cleanDB is missing. If we run the same command as above we have different WalletIds.
3. Why I get to the conclusion that cleanDB is missing. It comes only when the next test case runs. So I introduced MVar into the code. In cleanDB I putMVar and in the tests (in the beginning) I takeMVar. And I got stucked :

cleanDB wal BEFORE : [PrimaryKey (WalletId {getWalletId =6645b53521f8eaf97e3d6404385b1835da0bb135})]
cleanDB wal AFTER : []
wal=[]
key=PrimaryKey (WalletId {getWalletId = ad54f8e826744f5fa76a1730bdf9b65771a6e976})
cleanDB wal BEFORE : [PrimaryKey (WalletId {getWalletId = ad54f8e826744f5fa76a1730bdf9b65771a6e976})]
cleanDB wal AFTER : []
wal=[]
key=PrimaryKey (WalletId {getWalletId = 8adfe5c94630e8ae4b47d6e4c9363ef31f5fa36b})
cleanDB wal BEFORE : [PrimaryKey (WalletId {getWalletId = 8adfe5c94630e8ae4b47d6e4c9363ef31f5fa36b})]
cleanDB wal AFTER : []
wal=[]
key=PrimaryKey (WalletId {getWalletId = 1d05f4517d2decf9b118b7a48e46a534b42f67a4})
^C

@rvl rvl force-pushed the rvl/154/dblayer-properties branch 2 times, most recently from d08f5f5 to ee6ad17 Compare May 24, 2019 06:55
@rvl rvl marked this pull request as ready for review May 24, 2019 06:55
@rvl
Copy link
Contributor Author

rvl commented May 24, 2019

@paweljakubas Thanks for the debugging and fixes.

@rvl rvl force-pushed the rvl/154/dblayer-properties branch from ee6ad17 to 689cf6e Compare May 24, 2019 07:20
, Wallet (..)
, migrateAll
, unWalletKey
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay ^.^ , I had kept this one separate as an "exception", but I admit this is more consistent and aligned with our style.

insertCheckpoint wid cp
{ createWallet = \(PrimaryKey wid) cp meta -> withWriteLock $
ExceptT $ runQuery conn $ do
res <- handleConstraint (ErrWalletAlreadyExists wid) $
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure that any constraint errors are actually because of already existing wallet id?

res <- handleConstraint (ErrWalletAlreadyExists wid) $
insert_ (mkWalletEntity wid meta)
when (isRight res) $
insertCheckpoint wid cp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, in case a wallet already exists (isLeft res), this returns a success 🤔 ?

Okay actually not. I was confused initially because we aren't in an 'Either' or 'ExceptT' monad here.

@@ -182,7 +179,7 @@ instance Arbitrary TxMeta where
arbitrary = TxMeta
<$> elements [Pending, InLedger, Invalidated]
<*> elements [Incoming, Outgoing]
<*> (SlotId <$> arbitrary <*> choose (0, 21600))
<*> (SlotId <$> choose (0, 1000) <*> choose (0, 21599))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

arbitrary = initWallet <$> arbitrary

instance Eq (SeqState DummyTarget) where
_ == _ = True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the rationale for this instance? And what is the justification for it to be always true ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe (cannot say in the name of Rodney) it is because it is transparent in the context of tests we deliver here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels somewhat wrong though. Why would any SeqState be equal? I wonder where this enters into play.

Copy link
Contributor

@paweljakubas paweljakubas May 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, we need to add :

191 instance Eq (SeqState DummyTarget) where                                                                                                                                                  
192     (SeqState int1 ext1 pen1) == (SeqState int2 ext2 pen2) =                                                                                                                              
193         (int1 == int2) && (ext1 == ext2) && (pen1 == pen2) 

or just :

deriving instance Eq (SeqState DummyTarget)

@@ -13,6 +13,7 @@

module Cardano.Wallet.Primitive.AddressDiscoverySpec
( spec
, DummyTarget
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm... Not really sure about this. The test and instance from AddressDiscovery are quite specific and tailored for the properties of that module. They only use a single account key for instance. I guess it's okay for the DBSpec, but I'd rather not do that yet, and stick with our current approach of not sharing instances between spec files. This is an open-gate for messiness later.

We may discuss how we can better share generators between spec files without making spec files depend on each others. It becomes quite hard then to follow which arbitrary instance comes from where....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved to DBspec needed bits, and here export DummyTarget to MVarSpec and SliteSpec.

-- | Delete transactions that belong to a wallet and aren't referred to by
-- either Pending or TxMeta.
deleteLooseTransactions
-- | Delete unused TxMeta values for a wallet.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment is a bit of a lie here 😬 ... the function doesn't care much about whether the meta is used or unused, it just cleans them all.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed. also take care of indenting in this file. Some functions had 2-character indents in this file

@paweljakubas paweljakubas force-pushed the rvl/154/dblayer-properties branch from 689cf6e to 5b66760 Compare May 24, 2019 11:48
Copy link
Contributor

@paweljakubas paweljakubas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of work and effort was put into this PR. LGTM!

@paweljakubas paweljakubas force-pushed the rvl/154/dblayer-properties branch from 5b66760 to ce69389 Compare May 24, 2019 12:55
Copy link
Member

@KtorZ KtorZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍
Nice investigation work.

@KtorZ KtorZ merged commit e73b111 into master May 24, 2019
@iohk-bors iohk-bors bot deleted the rvl/154/dblayer-properties branch May 24, 2019 13:33
@KtorZ KtorZ removed this from the SQLite implementation for the DB Layer milestone Jun 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants