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

Integration Tests Http Bridge #80

Merged
merged 4 commits into from
Mar 19, 2019
Merged

Conversation

KtorZ
Copy link
Member

@KtorZ KtorZ commented Mar 18, 2019

Issue Number

#8

Overview

  • I have added some integration tests which automatically test our API to fetch blocks from the http bridge
  • I have removed unused code from the http bridge (according to the coverage report)
  • I have added an invariant to the http bridge in case an invalid slot id is passed (I noticed that this will cause the algorithm to fetch all blocks from the starting epoch down to the initial one, which is quite bad).

Comments

@KtorZ KtorZ requested a review from paweljakubas March 18, 2019 16:57
@paweljakubas
Copy link
Contributor

paweljakubas commented Mar 19, 2019

hmm, why it is all the time running? Because it was out-of-date at the time of pushing?

@KtorZ
Copy link
Member Author

KtorZ commented Mar 19, 2019

It seems that Travis failed to report the status to GitHub somehow. And, I realized I forgot to actually disable the integration tests for now in CI since we'll need a bit of caching first (or to run a local cluster)
😅

@KtorZ KtorZ self-assigned this Mar 19, 2019
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.

I like the simplification adopted based on invariant and the plethora of tests added. The PR needs rebase and transformers only usage. Also it would be nice to be green in CI (I bet this is because it was not up-ti-date at the moment of push)

, cardano-wallet
, fmt
, hspec
, mtl
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be replaced with transformers

port = 1337

spec :: Spec
spec = do
Copy link
Contributor

@paweljakubas paweljakubas Mar 19, 2019

Choose a reason for hiding this comment

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

Do we test (or it is reasonable to test) scenario when the tip refers to boundary block (ie., not main, but genesis). When talking with Byron nodes, one should take that into consideration. The consumers were aware of both types of blocks, handled two types of header hashes. But the blocks were dealt differently in two places of the code, in chain code (if I remember well) all blocks were downloaded and then filtered by consumer chain, some values form genesis block were used like time, ect.. Block coming from when listener mechanism (in the form of undos) were filtered upstream and the consumer (eg. wallet) were delivered only main blocks - it give rise to the need of that consumer to get additional data from the nodes (like time from slots, etc)

Copy link
Member Author

Choose a reason for hiding this comment

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

We do not because this is handled by our network layer. We do test fetching a whole epoch (so, when specifying a slot start at slot_number = 0). So, in contrary with the legacy wallet, we won't have to handle epoch boundaries blocks in the wallet logic (at this stage at least), since they are filtered out by the network layer.
When dealing with Haskell Shelley node, we'll have to reconsider some of that and will most probably have to apply some of the ledger rules to the EBB in order to keep track of a few protocol parameters.

( async, cancel )
import Control.Exception.Base
( ErrorCall )
import Control.Monad.Except
Copy link
Contributor

Choose a reason for hiding this comment

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

use rather import Control.Monad.Trans.Except

Copy link
Contributor

Choose a reason for hiding this comment

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

@paweljakubas why is this so? Are we trying to use most explicit module in the import which exports the function?

Copy link
Member Author

Choose a reason for hiding this comment

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

cf: #81

@KtorZ KtorZ force-pushed the KtorZ/integration-tests-http-bridge branch from ff7b2d4 to 7cb36b1 Compare March 19, 2019 13:08
@paweljakubas paweljakubas self-requested a review March 19, 2019 14:04
@@ -86,6 +86,9 @@ jobs:
name: "Tests"
script:
- tar xzf $STACK_WORK_CACHE
- travis_retry curl -L -o hermes-testnet.tar.gz https://github.com/input-output-hk/cardano-wallet/raw/data-integration-testing/hermes-testnet.tar.gz
Copy link
Contributor

Choose a reason for hiding this comment

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

why it is called hermes? ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

No idea. Ask the rust team 😅

@KtorZ KtorZ mentioned this pull request Mar 19, 2019
7 tasks
@KtorZ KtorZ force-pushed the KtorZ/integration-tests-http-bridge branch from 32e8daa to 5c598e4 Compare March 19, 2019 15:41
@KtorZ KtorZ force-pushed the KtorZ/integration-tests-http-bridge branch from 5c598e4 to 5ee68db Compare March 19, 2019 15:53
@KtorZ KtorZ merged commit 5de6221 into master Mar 19, 2019
@KtorZ KtorZ deleted the KtorZ/integration-tests-http-bridge branch March 19, 2019 16:11
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.

None yet

4 participants