-
Notifications
You must be signed in to change notification settings - Fork 630
[CBR-366] Adding test for partial getters #3469
[CBR-366] Adding test for partial getters #3469
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but just to confirm-- did you manage to have an account with a non-zero balance?
yes, in the integration test added the following is taken place:
|
You mean in |
yes this is the fragment responsible:
|
Ok, sounds good. Ideally we'd modify the wallet tests themselves to be able to do something similar, perhaps hook it up to our blockchain infrastructure, but for now, this will do just fine :) Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @paweljakubas 👍
randomNewAccount <- forM [1..4] $ \(_i :: Int) -> | ||
generate arbitrary :: IO NewAccount | ||
forM_ randomNewAccount $ \(rAcc :: NewAccount) -> | ||
postAccount wc (walId wallet) rAcc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a side note but this is a good use-case for >>=
. The 4 lines above can be expressed as:
forM_ [1..4] $ \_ -> generate arbitrary >>= postAccount wc (walId wallet)
balancesPartialUpdated <- | ||
mapM (\resp -> wrData <$> resp `shouldPrism` _Right) balancesPartialUpdatedResp' | ||
|
||
map (AccountBalance . accAmount) accsUpdated `shouldBe` balancesPartialUpdated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why 5 accounts and not only one 😄 ?
Left (WalletLayer.GetAccountError (V1 (Kernel.UnknownHdAccount _))) -> | ||
return () | ||
Left unexpectedErr -> | ||
fail $ "expecting different failure than " <> show unexpectedErr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
Right accs -> (map V1.accAddresses $ API.wrData accs) | ||
`shouldBe` | ||
(map (\(Right bal) -> (V1.acaAddresses . API.wrData) bal) partialAddresses) | ||
Left err -> fail (show err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^.^ .. That is maybe overkill / redundant with integration tests in the end. I think we can trust Servant here if our handlers function work as expected 😛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is patterned after @adinapoli-iohk 's tests, which do the same.
NOT MERGING THIS YET. Wait for #3462 to get in |
…d-tests-for-account-partial-getters [CBR-366] Adding test for partial getters
…hk/paweljakubas/CBR-366/add-tests-for-account-partial-getters [CBR-366] Adding test for partial getters
Description
Adding test for partial getters
Linked issue
https://iohk.myjetbrains.com/youtrack/issue/CBR-366
Type of change
Developer checklist
Testing checklist
QA Steps
Unit tests:
stack test --ta "-m GetAccountBalance" cardano-sl-wallet-new:wallet-unit-tests
stack test --ta "-m GetAccountAddresses" cardano-sl-wallet-new:wallet-unit-tests
Integration tests:
nix-build release.nix -A tests.walletIntegration
Screenshots (if available)