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 test: Polling for wallet fixture & Better error handling #263

Merged
merged 4 commits into from
May 15, 2019

Conversation

KtorZ
Copy link
Member

@KtorZ KtorZ commented May 15, 2019

Issue Number

#220

Overview

  • I have changed the 'fixtureWallet' helper such that it polls the wallet until funds become available.
  • I have adjusted the request function such that it won't throw HttpExeptionRequest but instead, put them down as value in the Left branch (to facilitate handling of such errors)
  • I have removed the warm up delay for the cluster, and instead, used polling to wait for the relay node to be ready (waiting for the chain to be at least 1), this is more reliable and allow to wait for just the necessary amount of time locally and in CI, instead of having a big timeout (which would probably have been too small on @akegalj's machine anyway 😅)
  • I also made sure that the integration setup would fail early when the cluster fail to start. I noticed that, if one of the node failed to start, the cluster async wouldn't terminate and just hang there. After a while, I discovered that this was because we close the stderr and stdout output streams for the cluster. With some open handles, the node can successfully shut itself down.

Comments

@KtorZ KtorZ requested a review from piotr-iohk May 15, 2019 11:00
@KtorZ KtorZ self-assigned this May 15, 2019
, "--tlscert", "/dev/null"
, "--tlskey", "/dev/null"
, "--tlsca", "/dev/null"
, "--no-tls"
Copy link
Member Author

Choose a reason for hiding this comment

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

Apparently, even if we provide --no-tls, we still have to give all the --tls*** options to the node... undoubtedly nice CLI we have there.

start <-
formatTime defaultTimeLocale "%s" . addUTCTime 2 <$> getCurrentTime
[h0, h1, h2, h3] <- forM ["core0", "core1", "core2", "relay"] $ \x -> do
openFile ("/tmp/cardano-node-simple/" <> x) WriteMode
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 comment below, we have to redirect stderr & stdout somewhere and can't just use NoStream otherwise the node fails to ... fail and the processes never terminate on failure.

throwM e
HttpExceptionRequest _ e ->
return (status500, Left (HttpException e))

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes it possible to pattern match on HttpException failures, these occur when for instance, there's a connection failure because the target server isn't up there yet. It allows us to retry calls when polling.

@KtorZ KtorZ force-pushed the KtorZ/integration-test-error-handling branch from c9d15b2 to 8d95539 Compare May 15, 2019 12:38
@piotr-iohk
Copy link
Contributor

stylish haskell ¯_(ツ)_/¯

@KtorZ KtorZ force-pushed the KtorZ/integration-test-error-handling branch from 8d95539 to e96d7dd Compare May 15, 2019 13:03
@KtorZ
Copy link
Member Author

KtorZ commented May 15, 2019

🤷‍♂️

Copy link
Contributor

@piotr-iohk piotr-iohk left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@KtorZ KtorZ merged commit aecc07f into master May 15, 2019
@KtorZ KtorZ deleted the KtorZ/integration-test-error-handling branch May 15, 2019 13:48
alecalve pushed a commit to alecalve/cardano-sl that referenced this pull request May 16, 2019
alecalve pushed a commit to alecalve/cardano-sl that referenced this pull request May 16, 2019
4038: Remove redundant [] in backupPhrase doc example r=KtorZ a=Anviking

## Description

Mirroring cardano-foundation/cardano-wallet#263

## Linked issue

cardano-foundation/cardano-wallet#263



Co-authored-by: anviking <[email protected]>
Copy link
Contributor

@akegalj akegalj left a comment

Choose a reason for hiding this comment

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

lgtm

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