Preliminary integration tests between Jormungandr <-> Network Layer (`networkTip`) #400

KtorZ · 2019-06-11T17:53:35Z

Issue Number

Overview

I have slightly changed the behavior of waitForConnection to also wait for the network tip to become available.
I have added happy & negative test paths for networkTip
I have fixed a few things along the way, including configuration and API definition (cf commits)

Comments

KtorZ · 2019-06-11T18:07:31Z

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

@@ -63,8 +61,6 @@ instance Exception ErrNetworkUnreachable
 data ErrNetworkTip
    = ErrNetworkTipNetworkUnreachable ErrNetworkUnreachable
    | ErrNetworkTipNotFound
-    | ErrNetworkTipBlockNotFound (Hash "BlockHeader")
-    -- ^ The tip-block wasn't found. This would be surprising.


Not needed. This is in the end, an implementation details which can be seen as NetworkTipNotFound from the external perspective.

🤔 They are different though. ErrNetworkTipNotFound is expected to be thrown when the http-bridge is started without the chain. ErrNetworkTipBlockNotFound means "the impossible just happened".

But they pretty much means the same for us in the end: there's no tip. We won't do anything special when the corresponding block isn't found bit treat that as if there was no tip.

…let-jormungandr'

…ation tests with Jormungandr

The prefix is part of the node's configuration file, so it can be changed at will and therefore, hasn't its place in the API definition. Instead, it should be passed alongside the 'BaseUrl' given to the Jormungandr manager

Anviking

lgtm — most pressingly wondering if we shouldn't run jormungandr in-memory

I am also (still?) suspicious/confused about NetworkLayer error types, but that's not specifically related to this pr

Anviking · 2019-06-12T02:34:59Z

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

@@ -63,8 +61,6 @@ instance Exception ErrNetworkUnreachable
 data ErrNetworkTip
    = ErrNetworkTipNetworkUnreachable ErrNetworkUnreachable
    | ErrNetworkTipNotFound
-    | ErrNetworkTipBlockNotFound (Hash "BlockHeader")
-    -- ^ The tip-block wasn't found. This would be surprising.


🤔 They are different though. ErrNetworkTipNotFound is expected to be thrown when the http-bridge is started without the chain. ErrNetworkTipBlockNotFound means "the impossible just happened".

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Network.hs

Anviking · 2019-06-12T04:23:30Z

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


+-- | A default 'RetryPolicy' with a constant delay, but no longer than 20


Seems confusing to say that the "constant delay" "is limited". Lazy suggestion:

Suggested change

-- | A default 'RetryPolicy' with a constant delay, but no longer than 20

-- | A default 'RetryPolicy' with a constant delay, but retries for no longer than 20

Anviking · 2019-06-12T04:28:34Z

lib/jormungandr/test/integration/Cardano/LauncherSpec.hs

 import Test.Hspec
    ( Spec, describe, expectationFailure, it )

 spec :: Spec
 spec = describe "cardano-wallet-launcher" $ do
    it "Can start launcher against testnet" $ do
+        removePathForcibly "/tmp/cardano-wallet-jormungandr"


Should we remove the storage from the Jörmungandr config, to use in-memory instead?

Hmmmm. No. Ideally integration tests reproduce the production setup as close as possible. This means, among othert things, using file based storage.

lib/jormungandr/test/integration/Cardano/Wallet/Jormungandr/NetworkSpec.hs

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

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Api.hs

paweljakubas

LGTM! I am also in favour of using only ErrNetworkTipNotFound and ignoring ErrNetworkTipBlockNotFound (Hash "BlockHeader") provided we know for sure the error I write below is eliminated. To my understand we get header hash of tip of block so having the tip means we have header hash of the tip. Nevertheless, basing on my experience when playing extensively with diffusion layer of cardano-sl, something else can pop out. Very rarely, but quite enough to detect it in the tests I encountered the error when calling requestTip :

BlockNetLogicException: DialogUnexpected "peer sent more than one tip"

Presumably, in new Shelly haskell node it is addressed but if not, we should remember about it when having obscure errors.

KtorZ · 2019-06-12T07:20:39Z

@pawel.jakubas @Anviking

This ErrNetworkTipBlockNotFound is a nice example of warning log severity IMO. Instead of returning a new error type that we are going to treat like the absence of tip anyway, I'd suggest to have a warning log line mentioning this bit still, returning a network tip not found error.

KtorZ · 2019-06-12T08:44:17Z

@Anviking Merging without the comment suggestion (will do in another PR)

KtorZ self-assigned this Jun 11, 2019

KtorZ requested a review from Anviking June 11, 2019 17:53

KtorZ mentioned this pull request Jun 11, 2019

Jörmungandr REST-API / Network Layer #219

Closed

12 tasks

KtorZ commented Jun 11, 2019

View reviewed changes

KtorZ added 6 commits June 11, 2019 20:11

remove 'ErrNetworkTipBlockNotFound' error & fix retrying code

bfd40e3

change destination folder for Jormungandr's data to '/tmp/cardano-wal…

441c988

…let-jormungandr'

add 'newNetworkLayer' and preliminary specs skeleton for basic integr…

876a635

…ation tests with Jormungandr

add negative test paths for network layer network tip

8406d52

extend jormungandr dot cabal with NetworkSpec

37d95bb

KtorZ force-pushed the KtorZ/jormungandr-network-tip-tests branch from af06b91 to 37d95bb Compare June 11, 2019 18:14

Anviking approved these changes Jun 12, 2019

View reviewed changes

paweljakubas approved these changes Jun 12, 2019

View reviewed changes

KtorZ merged commit 7548ec5 into master Jun 12, 2019

KtorZ deleted the KtorZ/jormungandr-network-tip-tests branch June 12, 2019 08:44

KtorZ mentioned this pull request Jun 12, 2019

Change wallet currentTip from SlotId to BlockHeader #384

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preliminary integration tests between Jormungandr <-> Network Layer (`networkTip`) #400

Preliminary integration tests between Jormungandr <-> Network Layer (`networkTip`) #400

KtorZ commented Jun 11, 2019

KtorZ Jun 11, 2019

Anviking Jun 12, 2019

KtorZ Jun 12, 2019

Anviking left a comment •

edited

Loading

Anviking Jun 12, 2019

Anviking Jun 12, 2019

Anviking Jun 12, 2019

KtorZ Jun 12, 2019

paweljakubas left a comment •

edited

Loading

KtorZ commented Jun 12, 2019

KtorZ commented Jun 12, 2019


		-- \| A default 'RetryPolicy' with a constant delay, but no longer than 20

Preliminary integration tests between Jormungandr <-> Network Layer (networkTip) #400

Preliminary integration tests between Jormungandr <-> Network Layer (networkTip) #400

Conversation

KtorZ commented Jun 11, 2019

Issue Number

Overview

Comments

KtorZ Jun 11, 2019

Choose a reason for hiding this comment

Anviking Jun 12, 2019

Choose a reason for hiding this comment

KtorZ Jun 12, 2019

Choose a reason for hiding this comment

Anviking left a comment • edited Loading

Choose a reason for hiding this comment

Anviking Jun 12, 2019

Choose a reason for hiding this comment

Anviking Jun 12, 2019

Choose a reason for hiding this comment

Anviking Jun 12, 2019

Choose a reason for hiding this comment

KtorZ Jun 12, 2019

Choose a reason for hiding this comment

paweljakubas left a comment • edited Loading

Choose a reason for hiding this comment

KtorZ commented Jun 12, 2019

KtorZ commented Jun 12, 2019

Preliminary integration tests between Jormungandr <-> Network Layer (`networkTip`) #400

Preliminary integration tests between Jormungandr <-> Network Layer (`networkTip`) #400

Anviking left a comment •

edited

Loading

paweljakubas left a comment •

edited

Loading