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

Add DB-Layer Unit Tests #88

Merged
merged 6 commits into from
Mar 21, 2019
Merged

Conversation

paweljakubas
Copy link
Contributor

Issue Number

#69

Overview

  • I have added db MVar implementation tests to unit tests suite

Comments

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.

I like the tests!
Some suggestions in naming and a few (important) remarks however.

@@ -26,7 +26,7 @@
module Cardano.Wallet
(
-- * Wallet
Wallet
Wallet(..)
Copy link
Member

Choose a reason for hiding this comment

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

Nope! That is, forbidden!
This would be violating all the guarantees this module provides us.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted the change

it "multiple sequential putCheckpoints work properly"
(checkCoverage dbMultiplePutsSeqProp)
it "multiple parallel putCheckpoints work properly"
(checkCoverage dbMultiplePutsParProp)
Copy link
Member

Choose a reason for hiding this comment

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

checkCoverage are misleading here as properties below don't compute any coverage via cover or coverTable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

replaced with property


instance Eq (Wallet Int) where
Wallet _ _ _ s1 == Wallet _ _ _ s2
= s1 == s2
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

derived Eq instance in Wallet.hs

= s1 == s2

instance Show (PrimaryKey WalletId) where
show (PrimaryKey wid) = show wid
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

derived Show

instance Arbitrary (PrimaryKey WalletId) where
-- No shrinking
arbitrary = do
fiftyInts <- vectorOf 50 $ choose (0 :: Int, 9)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@paweljakubas paweljakubas Mar 20, 2019

Choose a reason for hiding this comment

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

lowered 50 to 10 to avoid shrinking

:: (PrimaryKey WalletId, Int)
-> Property
dbReadCheckpointsProp (key, val) = monadicIO $ liftIO $ do
db <- newDBLayer
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

used before



dbReplaceValsProp
:: (PrimaryKey WalletId, Int, Int)
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest to create a newtype newtype DummyState = DummyState Int to makes signatures a bit clearer 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

db <- newDBLayer

mapConcurrently_ (\(key, val) -> putCheckpoints db key (toWalletState val)) keyValPairs
resFromDb <- Set.fromList <$> readWallets db
Copy link
Member

Choose a reason for hiding this comment

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

This makes me wonder if the DBLayer shouldn't just return a Set here in a first place 🤔 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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"
Copy link
Member

Choose a reason for hiding this comment

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

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"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@paweljakubas paweljakubas force-pushed the paweljakubas/69/add-db-layer-unit-tests branch from 23c66aa to 9b10a97 Compare March 20, 2019 20:01
@paweljakubas paweljakubas requested a review from KtorZ March 20, 2019 20:18
Copy link
Contributor

@rvl rvl left a comment

Choose a reason for hiding this comment

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

LGTM. The same properties could also be used for other DBLayer implementations.

@KtorZ KtorZ mentioned this pull request Mar 21, 2019
@paweljakubas paweljakubas force-pushed the paweljakubas/69/add-db-layer-unit-tests branch 2 times, most recently from df2c4dc to 5930605 Compare March 21, 2019 07:53
@KtorZ KtorZ force-pushed the paweljakubas/69/add-db-layer-unit-tests branch from 0e3d822 to 1cee09a Compare March 21, 2019 13:22
@KtorZ KtorZ changed the title [69] adding db unit tests Add DB-Layer Unit Tests Mar 21, 2019
@KtorZ KtorZ merged commit 7b96ef6 into master Mar 21, 2019
@KtorZ KtorZ deleted the paweljakubas/69/add-db-layer-unit-tests branch March 21, 2019 14:12
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