Add DB-Layer Unit Tests #88
Conversation
src/Cardano/Wallet.hs
Outdated
| @@ -26,7 +26,7 @@ | |||
| module Cardano.Wallet | |||
| ( | |||
| -- * Wallet | |||
| Wallet | |||
| Wallet(..) | |||
There was a problem hiding this comment.
Nope! That is, forbidden!
This would be violating all the guarantees this module provides us.
There was a problem hiding this comment.
reverted the change
| it "multiple sequential putCheckpoints work properly" | ||
| (checkCoverage dbMultiplePutsSeqProp) | ||
| it "multiple parallel putCheckpoints work properly" | ||
| (checkCoverage dbMultiplePutsParProp) |
There was a problem hiding this comment.
checkCoverage are misleading here as properties below don't compute any coverage via cover or coverTable
There was a problem hiding this comment.
replaced with property
|
|
||
| instance Eq (Wallet Int) where | ||
| Wallet _ _ _ s1 == Wallet _ _ _ s2 | ||
| = s1 == s2 |
There was a problem hiding this comment.
This is dodgy. We can probably just force the wallet state s to have an Eq and Ord instance, and derive this constraint for any Wallet s in the primitive itself.
There was a problem hiding this comment.
derived Eq instance in Wallet.hs
| = s1 == s2 | ||
|
|
||
| instance Show (PrimaryKey WalletId) where | ||
| show (PrimaryKey wid) = show wid |
There was a problem hiding this comment.
Show instances should not be written by hand. We have one exception which is the instance for ShowFmt in the wallet primitive which gives you the ability to wrap types in a ShowFmt newtype in order to "swap" the underlying type Show instance with Buildable and produce a more readable output if that's the end goal.
| instance Arbitrary (PrimaryKey WalletId) where | ||
| -- No shrinking | ||
| arbitrary = do | ||
| fiftyInts <- vectorOf 50 $ choose (0 :: Int, 9) |
There was a problem hiding this comment.
That's a rather long name, with no shrinking. It would be slightly nicer if we could produce short names too, and shrink to a shorter name in case of error to get a nicer error message.
There was a problem hiding this comment.
lowered 50 to 10 to avoid shrinking
| :: (PrimaryKey WalletId, Int) | ||
| -> Property | ||
| dbReadCheckpointsProp (key, val) = monadicIO $ liftIO $ do | ||
| db <- newDBLayer |
There was a problem hiding this comment.
You could probably use a beforeEach from HSpec here to have the fixture done by HSpec and the db layer passed in as an argument.
|
|
||
|
|
||
| dbReplaceValsProp | ||
| :: (PrimaryKey WalletId, Int, Int) |
There was a problem hiding this comment.
I'd suggest to create a newtype newtype DummyState = DummyState Int to makes signatures a bit clearer 👍
| db <- newDBLayer | ||
|
|
||
| mapConcurrently_ (\(key, val) -> putCheckpoints db key (toWalletState val)) keyValPairs | ||
| resFromDb <- Set.fromList <$> readWallets db |
There was a problem hiding this comment.
This makes me wonder if the DBLayer shouldn't just return a Set here in a first place 🤔 ?
There was a problem hiding this comment.
for me using list suggests that the events/data are stored with "time order". For distributed systems with high throughput this is very high requirement, and usually it is better to start with was stored requirement (which implies Set). Probably, here at this point it does not matter. But to be honest, I do not know requirement here
There was a problem hiding this comment.
Hmmm.. That's a bit of a false assumption here. Especially because there's no particular ordering defined on the checkpoints and therefore, there's no guarantee that the DB layers will preserve the insertion order.
Yet, one thing a Set will convey is the absence of duplicate in the data, which is right.
| spec = return () | ||
| spec = do | ||
| describe "DB works as expected" $ do | ||
| it "readCheckpoints works properly" |
There was a problem hiding this comment.
Minor suggestion in the title here but, maybe we give some insights about what "works properly" means, and states the actual properties being checked like "readCheckpoints . putCheckpoints yields inserted checkpoints"
paweljakubas
force-pushed
the
paweljakubas/69/add-db-layer-unit-tests
branch
from
23c66aa
to
9b10a97
Compare
paweljakubas
force-pushed
the
paweljakubas/69/add-db-layer-unit-tests
branch
2 times, most recently
from
df2c4dc
to
5930605
Compare
Increase test coverage
Add more information to README.md
KtorZ
force-pushed
the
paweljakubas/69/add-db-layer-unit-tests
branch
from
0e3d822
to
1cee09a
Compare
Issue Number
#69
Overview
Comments