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.

Sign up for GitHub

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

Jump to bottom

Additional tests checking if files exists when using --database and --state-dir options #413

Merged
merged 7 commits into from
Jun 18, 2019
Merged

Additional tests checking if files exists when using --database and --state-dir options #413

merged 7 commits into from
Jun 18, 2019

Conversation

piotr-iohk
Copy link
Contributor

Issue Number

Overview

  • I have added few tests checking if files are created on filesystem when using
    • cardano-wallet server --database <filepath>
    • or cardano-wallet-launcher --state-dir <filepath>

Comments

cardano-wallet-launcher tests can be easily changed to cardano-wallet launcher after #368.

@piotr-iohk piotr-iohk requested review from rvl and KtorZ June 14, 2019 13:15
@piotr-iohk piotr-iohk self-assigned this Jun 14, 2019
@akegalj akegalj mentioned this pull request Jun 14, 2019
Closed
12 tasks
KtorZ
KtorZ reviewed Jun 14, 2019
View reviewed changes
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.

Nice that we test this. Some remark to make the code a bit more concise and digest 👍

lib/core/test/integration/Test/Integration/Scenario/CLI/Server.hs Outdated
describe "SERVER - cardano-wallet server" $ do
it "SERVER - Can start cardano-wallet server without --database" $ \_ -> do
let cardanoWalletServer = Command
"cardano-wallet"
Copy link
Member

Choose a reason for hiding this comment

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

Note that we should use stack -- exec cardano-wallet and not cardano-wallet as the latter requires the software to have been installed. Which isn't usually the case when tests are ran, so it's really easy to just run them on a wrong version.

lib/core/test/integration/Test/Integration/Scenario/CLI/Server.hs Outdated
let cardanoWalletServer = Command
"cardano-wallet"
[ "server"
, "--random-port"
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 remove the --random-port from the tests since it's completely unrelated and this is a totally optional argument.

lib/core/test/integration/Test/Integration/Scenario/CLI/Server.hs Outdated
Inherit
expectCmdStarts cardanoWalletServer

w1 <- doesFileExist dbFile
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
w1 <- doesFileExist dbFile
doesFileExist dbFile `shouldReturn` False

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that was my first choice but:

/home/piotr/wb/cardano-wallet/lib/http-bridge/test/integration/Cardano/LauncherSpec.hs:46:71: error:
    • Couldn't match expected type ‘IO Bool’ with actual type ‘Bool’
    • In the second argument of ‘shouldBe’, namely ‘True’
      In a stmt of a 'do' block:
        (doesFileExist (stateDir ++ "/wallet.db-wal")) `shouldBe` True
      In the second argument of ‘($)’, namely
        ‘do let cardanoWalletLauncher
                  = Command "cardano-wallet-launcher" ... (return ()) Inherit
            expectCmdStarts cardanoWalletLauncher
            d1 <- doesDirectoryExist stateDir
            d2 <- doesDirectoryExist (stateDir ++ "/testnet")
            ....’
   |        
46 |             (doesFileExist (stateDir ++ "/wallet.db-wal")) `shouldBe` True

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure to get your point here 😅 ? You example uses shouldBe, whereas I suggested shouldReturn (exactly because shouldBe works with raw / unwrapped values, but shouldReturn works with values inside a Monad).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ooh, gotcha! (note to self: don't respond to comments after midnight, or... at least, proof read the next day...)

lib/core/test/integration/Test/Integration/Scenario/CLI/Server.hs Outdated
w3 <- doesFileExist (dbFile ++ "-wal")
w1 `shouldBe` True
w2 `shouldBe` True
w3 `shouldBe` True
Copy link
Member

Choose a reason for hiding this comment

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

same remark as before 👍

lib/core/test/integration/Test/Integration/Scenario/CLI/Server.hs Outdated Show resolved Hide resolved
lib/http-bridge/test/integration/Cardano/LauncherSpec.hs Outdated
it "LAUNCHER - Can start launcher against testnet with --state-dir" $ do
let cardanoWalletLauncher = Command
"cardano-wallet-launcher"
[ "--http-bridge-port", "8080"
Copy link
Member

Choose a reason for hiding this comment

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

Depending on which of this one or #414 goes first, we'll have to update this to use the launch command instead.

lib/http-bridge/test/integration/Cardano/LauncherSpec.hs Outdated Show resolved Hide resolved
@rvl
Copy link
Contributor

rvl commented Jun 14, 2019

Oh! I also added CLI/Server.hs for the DaedalusIPC work.

@piotr-iohk
Copy link
Contributor Author

@KtorZ, @rvl - I rebased and pushed review remarks. @rvl - I must have rebased carelessly because I removed your contents of CLI/Server.hs (that was not intended though). I noticed however that I have the same tests in Cardano/LauncherSpec.hs (except for DaedalusIPC which I added there). I think that tests for the cardano-wallet launch fit better in Cardano/LauncherSpec.hs and CLI/Server.hs contains now tests for cardano-wallet serve.

KtorZ
KtorZ reviewed Jun 17, 2019
View reviewed changes
lib/core/test/integration/Test/Integration/Scenario/CLI/Server.hs Outdated
w2 `shouldBe` True
w3 `shouldBe` True
where
dbFile = "./test/data/test-DB-File"
Copy link
Member

Choose a reason for hiding this comment

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

@rvl's version was using temporary random directory which has the nice side effect of not cluttering the development environment and avoid file conflicts between multiple runs (because for some reasons / failures, the tear down code wouldn't run).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(because for some reasons / failures, the tear down code wouldn't run).

I suppose after_ tearDown should run no matter if test fails or passes?

Also @rvl's tests where for the launch and --state-dir not for serve and --database (https://github.com/input-output-hk/cardano-wallet/blob/master/lib/core/test/integration/Test/Integration/Scenario/CLI/Server.hs#L28-L39)

In any case I can bring temporary dir back.

Copy link
Member

Choose a reason for hiding this comment

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

I suppose after_ tearDown should run no matter if test fails or passes?

Provided the tear down code works as expected and removes files where they ought to be 😬 ... Perhaps files are already there and are created elsewhere, so the test doesn't test anything ? The good thing with taking a random file location everytime is that, we are "sure" nothing was there before, no matter what and we can eliminate false positives.

Although, that should also hold for the CI environment (which is supposedly, free of any build and tests artifacts), but it's not like we've never mistakenly committed test files and had them available in the integration environment 🙄 ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, added temp directory for launch and serve tests

KtorZ
KtorZ approved these changes Jun 17, 2019
View reviewed changes
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.

LGTM

@KtorZ KtorZ merged commit 43e49aa into master Jun 18, 2019
@KtorZ KtorZ deleted the piotr/testing branch June 18, 2019 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KtorZ KtorZ KtorZ approved these changes

@rvl rvl Awaiting requested review from rvl

Assignees

@piotr-iohk piotr-iohk

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@piotr-iohk @rvl @KtorZ