Add `transaction list` CLI command. #532

jonathanknowles · 2019-07-12T09:16:34Z

Issue Number

#467

Overview

I have:

added a transaction list command to the CLI, which takes optional --start TIME and --end TIME parameters.
added a CLI unit test to demonstrate that transaction list --help displays the expected help text.
added a CLI integration test to confirm that transaction list returns [] for a brand new wallet, regardless of what values are provided for --start or -end (as expected).

Outstanding work for future PRs

The listTransactions endpoint is not yet implemented, currently returning [].

Therefore, the tests introduced in this PR are limited in their scope.

Once the listTransactions endpoint is implemented, we can add the following tests in future PRs:

CLI integration tests to confirm that the full set of transactions is returned when neither --start nor --end are specified.
CLI integration tests to confirm that appropriate subsets of transactions are returned, in the correct order, when either --start or --end are specified (or both).

jonathanknowles · 2019-07-22T03:53:50Z

lib/core/test/integration/Test/Integration/Scenario/CLI/Transactions.hs

@@ -644,6 +651,17 @@ spec = do
            out `shouldBe` ""
            c `shouldBe` ExitFailure 1

+    it "TRANS_LIST_01 - Listing transactions for an empty wallet" $ \ctx ->
+        withMaxSuccess 10 $ property $ \mAfter mBefore -> do


This property is rather slow, so we only check it a few times.

Maybe integration tests layer is therefore not the best place for properties? I think that more value we can gain from adding some negative cases here (see my previous comment) and have properties like this on unit level...

Maybe integration tests layer is therefore not the best place for properties? I think that more value we can gain from adding some negative cases here (see my previous comment) and have properties like this on unit level...

Hmm, I wonder if there's a good reason to avoid using properties here?

My intent here was to cover all the cases for --after and --before. (Both of these parameters can be either present or absent.) The point is that the values can be anything, and the result should still be [].

It seemed sensible to generate these cases rather than write them manually. But perhaps there is a better way?

It seemed sensible to generate these cases rather than write them manually. But perhaps there is a better way?

I agree ^.

By saying:

Maybe integration tests layer is therefore not the best place for properties?

I was reffering to your comment:

This property is rather slow, so we only check it a few times.

I was thinking that maybe it just takes a lot of time on this level and that's why I though that bringing it to unit level would make it faster...

This property is rather slow, so we only check it a few times.

I was thinking that maybe it just takes a lot of time on this level and that's why I though that bringing it to unit level would make it faster...

Good point. I guess it depends on what we deem acceptable. On my system, with 10 cases, it takes around 1-2 seconds to run. Is this fast enough?

On my system, with 10 cases, it takes around 1-2 seconds to run. Is this fast enough?

Yeah, I think that's OK in general. (on the other hand that may be the time of 1000 tests on unit level ;) )

There's also this concept of testing pyramid (https://medium.com/@timothy.cochran/test-pyramid-the-key-to-good-automated-test-strategy-9f3d7e3c02d5), which I'm fond of, which basically says to have the most tests on the lowest level of the system and the least on the highest - end-to-end level. Following this concept we could have say, property doing 1000 examples on unit test level testing a lot of different values and verify that proper error is thrown and have just one integration test with single value (or just a few values) testing if everything works fine end-to-end (e.g. check the error message is presented correctly to the user etc.)

There's also this concept of testing pyramid (https://medium.com/@timothy.cochran/test-pyramid-the-key-to-good-automated-test-strategy-9f3d7e3c02d5), which I'm fond of, which basically says to have the most tests on the lowest level of the system and the least on the highest - end-to-end level.

Thanks for sharing this, the content makes a lot of sense.

For this case though, I'm guessing we really would like to test all the following cases:

Both --after and --before parameters present.

Only --after present.

Only --before present.

Neither --after nor --before present.

In addition, to be able to test these parameters with random rather than fixed values, is probably useful.

We could make an argument for writing the above four cases as separate unit tests, but since all cases should produce the same output (the empty list), wouldn't that mean an unnecessary duplication of code? (Also, given that we only generate 10 cases, I'm guessing it wouldn't save us much time on each test run.)

Though having said that, I don't have a particularly strong preference in this case! If you have a strong preference, I would be happy to rewrite this without properties.

We avoid property-based tests in integration tests because:

We want to keep them predictable. They're already sometimes quite tricky to debug, if we can't reproduce them exactly as we want, it becomes even harder.

They're slow, so we've to limit their execution to only a small number, and here, we don't have any guarantee that all cases are covered. Ideally, we would use a checkCoverage + cover to make sure we do cover all the cases we want, but the test would take minutes to run.

Instead, when tests a rather similar in their setup, execution and assertion, we use a test matrix to make it clear. See also: https://github.com/input-output-hk/cardano-wallet/blob/master/lib/core/test/integration/Test/Integration/Scenario/CLI/Addresses.hs#L123-L134

@KtorZ @piotr-iohk

Okay. I've:

changed the integration test to use a matrix of hand-picked time values (rather than using a property-based test).

changed the listTransactionsViaCLI function to take Maybe String for each time value.

Hopefully this should address the concerns raised above, but let me know if you'd like me to change anything about this.

Thanks again for looking at this PR. :)

piotr-iohk · 2019-07-22T06:19:24Z

lib/cli/test/unit/Cardano/CLISpec.hs

+            , "  --port INT               port used for serving the wallet"
+            , "                           API. (default: 8090)"
+            , "  --after TIME             specifies an earliest time"
+            , "  --before TIME            specifies a latest time"


It would be nice to add info about the time format (ISO-8601 UTC) and maybe exemplary value.

That's a good idea. I'll add it.

@piotr-iohk Added:

transaction list --help Available options: -h,--help Show this help text --port INT port used for serving the wallet API. (default: 8090) --after TIME specifies an earliest time (ISO 8601 format: basic or extended) --before TIME specifies a latest time (ISO 8601 format: basic or extended)

piotr-iohk · 2019-07-22T06:25:45Z

lib/core/test/integration/Test/Integration/Framework/DSL.hs

+    => s
+    -> ApiWallet
+    -> Maybe Iso8601Time
+    -> Maybe Iso8601Time


I think it would be better to have just a [String] as an argument of this method. This would allow for more flexibility when constructing tests, in particular we can add a lot of different negative tests (e.g. faulty wallet ID, typos in the parameter names, bad-formed parameter values etc.) which is not possible currently.

I think it would be better to have just a [String] as an argument of this method. This would allow for more flexibility when constructing tests, in particular we can add a lot of different negative tests (e.g. faulty wallet ID, typos in the parameter names, bad-formed parameter values etc.) which is not possible currently.

@piotr-iohk thanks again for looking at this.

I can sympathize with this argument. Though for this PR, I consciously avoided doing that due to feedback during previous reviews: for example: #519 (comment)

@KtorZ, would you be able to share your opinion on this?

Regarding the use of negative tests, I had some thoughts which I wanted to share here:

Regarding simple typos in parameter names: rather than writing lots of manual tests (which are likely to be rather repetitive), perhaps there is a better, automated way to generate CLI calls with mis-spelt parameter names? We could potentially generate these from the CLI definition.

Regarding badly-formed parameters that the CLI can detect due to parsing errors (i.e., fromText returning Left). Again, perhaps there is a way to generate these automatically? (Using QuickCheck, we can generate arbitrary values that we know will not parse with fromText, and pass these to the CLI, expecting failure.)

Regarding invalid parameters that parse correctly, whose invalidity can only be detected by the back-end. Because the CLI must first parse data from Text to well-typed values before sending it to the back end, I suspect this case also doesn't require the use of String types.

@piotr-iohk Would be interested to hear your opinion!

Thanks @jonathanknowles!

I think the way I'm coming from is just the past experience: In https://github.com/input-output-hk/cardano-wallet-legacy we actually had a strongly typed API client and it used to be quite limiting for me in terms of flexibility to create test cases (in particularly negative) (see examplary request -> https://github.com/input-output-hk/cardano-wallet-legacy/blob/develop/test/integration/Test/Integration/Scenario/Transactions.hs#L255-L262). As an aid there was this unsafeRequest added later which was less typed but it was giving more flexibility (e.g. https://github.com/input-output-hk/cardano-wallet-legacy/blob/develop/test/integration/Test/Integration/Scenario/EosWallets.hs#L59-L69). So in this iteration of wallet we kind of went for the second approach from the beggining.

In my mind the idea of automated integration test is just to have a tool to be able to make the same calls and get the same responses (and test them) as the end user would (and user can basically type anything ;))
Off course if there is a room for improving this, I'm all for it (in particular for generating all sort of negative test cases). The only thing that I would not want to achieve is that any additional layers of abstraction become something that limit us in testing.

Having a stringly-typed API makes for a quite poor documentation when reading function signatures. Yet, having said that, there's not much we do with the underlying data-type apart from "showing it". Plus, we do want to submit values that are not valid and therefore, that couldn't be represented as an Iso8601Range. Most of the integration tests work this way for this very reason as @piotr-iohk highlighted already.

Regarding the automated generation of false cases at the integration level: sounds like overkill and probably too complicated at this stage. We can already test all sort of things at the unit level with extensive properties and all sort of negative tests. Plus, integration tests are already quite complicated on their own, so we have to strive to avoid this complexity from exploding. Mixing property tests at this stage would require some careful thoughts and design for something we can already do very well at the unit level.

@KtorZ @piotr-iohk

Okay. I've:

changed the integration test to use a matrix of hand-picked time values (rather than using a property-based test).

changed the listTransactionsViaCLI function to take Maybe String for each time value.

Hopefully this should address the concerns raised above, but let me know if you'd like me to change anything about this.

@KtorZ wrote:

Regarding the automated generation of false cases at the integration level: sounds like overkill and probably too complicated at this stage.

My point is that "typos in the parameter names" was used as a justification for allowing parameters to be supplied as [String] in the listTransactionsViaCLI command:

Assuming we decide that it's valuable to test that parameter name misspellings are rejected, wouldn't it be better to generate such tests automatically from the CLI definition? (as part of a unit test suite.)

It seems like there would be countless permutations, especially when we take all commands and argument combinations into account.

Btw. another advantage of [String] is also that we can test different order of parameters (--start ... --end ... or --end ... --start ...)

piotr-iohk · 2019-07-22T06:30:40Z

lib/core/test/integration/Test/Integration/Scenario/CLI/Transactions.hs

@@ -644,6 +651,17 @@ spec = do
            out `shouldBe` ""
            c `shouldBe` ExitFailure 1

+    it "TRANS_LIST_01 - Listing transactions for an empty wallet" $ \ctx ->
+        withMaxSuccess 10 $ property $ \mAfter mBefore -> do


Maybe integration tests layer is therefore not the best place for properties? I think that more value we can gain from adding some negative cases here (see my previous comment) and have properties like this on unit level...

rvl

Looks good

jonathanknowles · 2019-07-23T09:49:29Z

I've changed the CLI time range arguments to --start and --end in line with the discussion here: #467 (comment)

piotr-iohk · 2019-07-23T10:01:36Z

lib/core/test/integration/Test/Integration/Framework/DSL.hs

-        , maybe [] (\t -> ["--start", showT t]) mTimeRangeStart
-        , maybe [] (\t -> ["--end", showT t]) mTimeRangeEnd
+        , maybe [] (\t -> ["--start", t]) mTimeRangeStart
+        , maybe [] (\t -> ["--end",   t]) mTimeRangeEnd


I'd still be after using just [String] rather than ApiWallet -> Maybe String -> Maybe String 😅 because:

we cannot provide faulty walletId.

also we're not able to provide invalid argument like --star...

(I know, I know... if you're developer, your eyes must hurt when you see this ;) )

I'd still be after using just [String] rather than ApiWallet -> Maybe String -> Maybe String sweat_smile because:
(I know, I know... if you're developer, your eyes must hurt when you see this ;) )

It's okay, I have a pair of special developer goggles. 😅

No problem, I've updated the listTransactionsViaCLI function as requested.

However, I do still have some reservations.

Given that this function is intended to be part of a DSL, it seems we've drawn the line rather arbitrarily on which parts of the command should be freeform, and which parts should be hard-coded. If we allow the arguments to be [String] (thus allowing invalid variations), then why hard-code the transaction list command prefix (thus disallowing invalid variations like transaction lost and so on)?

also we're not able to provide invalid argument like --star...

I would question whether we really want to be building integration tests that check misspelled CLI arguments are rejected. It seems like there would be countless permutations, especially when we take all commands and argument combinations into account.

Surely, if we decide that it's valuable to test for misspellings, wouldn't it be better to generate such tests automatically from the CLI definition? (as part of a unit test suite.)

Given that this function is intended to be part of a DSL, it seems we've drawn the line rather arbitrarily on which parts of the command should be freeform, and which parts should be hard-coded. If we allow the >arguments to be [String] (thus allowing invalid variations), then why hard-code the transaction list command prefix (thus disallowing invalid variations like transaction lost and so on)?

Indeed, it is like just a shortcut to test different cases of transaction list. Also there is actually a special test case that roughly covers cases like transaction lost -> https://github.com/input-output-hk/cardano-wallet/blob/master/lib/core/test/integration/Test/Integration/Scenario/CLI/Miscellaneous.hs#L38-L102, so it will surely land there.

I would question whether we really want to be building integration tests that check misspelled CLI arguments are rejected. It seems like there would be countless permutations, especially when we take all commands and argument combinations into account.

My opinion is that in integration test it is not necessary about providing all possible permutations but rather examples focusing on bonduary values and corner cases and having in mind equivalence partitioning. So --star and --startt would be probably enough to test misspelled --start,

they are sort of boundary values and kind of corner(ish) (very similar to --start yet different in length),

Also I might assume that if they give expected result than pretty much other misspells will, so I don't need to test all possible combinations (equivalence partitioning) [well, for sanity I would probably test also some weird strings ;)]

Btw. another advantage of [String] is also that we can test different order of parameters (--start ... --end ... or --end ... --start ...)

Surely, if we decide that it's valuable to test for misspellings, wouldn't it be better to generate such tests automatically from the CLI definition? (as part of a unit test suite.)

I think that testing misspellings on integration level is probably good enough, but perhaps that would be also a nice option.

In response to feedback from previous review: #532 (comment)

…y wallet.

…re` arguments. In the case of these arguments, we accept ISO 8601 format strings, either in basic format or extended format.

Use `--start` and `--end` instead of `--after` and `--before`. See: #467 (comment)

…on test.

In response to feedback from previous review: #532 (comment)

piotr-iohk

Thanks! 👍

jonathanknowles · 2019-07-24T06:57:50Z

Thanks! +1

@piotr-iohk thanks again for your thorough review, I appreciate our discussion.

jonathanknowles self-assigned this Jul 12, 2019

jonathanknowles force-pushed the jonathanknowles/cli-iso-8601-time branch 2 times, most recently from 0e1e728 to 49341e7 Compare July 15, 2019 08:31

jonathanknowles force-pushed the jonathanknowles/cli-list-transactions branch from 6f7b68a to 11e0c1b Compare July 15, 2019 08:31

jonathanknowles changed the base branch from jonathanknowles/cli-iso-8601-time to master July 16, 2019 01:28

jonathanknowles force-pushed the jonathanknowles/cli-list-transactions branch 2 times, most recently from 19d982b to 98e06cc Compare July 16, 2019 08:02

KtorZ mentioned this pull request Jul 16, 2019

Implement transaction list command in CLI #467

Closed

5 tasks

jonathanknowles force-pushed the jonathanknowles/cli-list-transactions branch 7 times, most recently from 5bfc600 to 4e9a90c Compare July 22, 2019 03:30

jonathanknowles changed the title ~~WIP: Add transaction list CLI command.~~ Add transaction list CLI command. Jul 22, 2019

jonathanknowles force-pushed the jonathanknowles/cli-list-transactions branch from 4e9a90c to 74884f2 Compare July 22, 2019 03:35

jonathanknowles marked this pull request as ready for review July 22, 2019 03:35

jonathanknowles commented Jul 22, 2019

View reviewed changes

piotr-iohk suggested changes Jul 22, 2019

View reviewed changes

rvl approved these changes Jul 22, 2019

View reviewed changes

jonathanknowles force-pushed the jonathanknowles/cli-list-transactions branch from 15e014c to 38c0bf7 Compare July 22, 2019 07:46

piotr-iohk reviewed Jul 23, 2019

View reviewed changes

jonathanknowles added a commit that referenced this pull request Jul 24, 2019

Make listTransactionsViaCLI completely freeform in its arguments.

2bcd588

In response to feedback from previous review: #532 (comment)

jonathanknowles added 5 commits July 24, 2019 02:35

Add listTransactions function to WalletClient.

a74c64f

Add transaction list CLI command.

2e0f2c1

Add CLI help test for "transaction list" command.

3794b30

Add listTransactionsViaCLI command.

4ca3359

Add CLI test to demonstrate that we can list transactions for an empt…

8578d23

…y wallet.

jonathanknowles added 4 commits July 24, 2019 02:40

Clearly state which format is acceptable for the--after and `--befo…

1f010a3

…re` arguments. In the case of these arguments, we accept ISO 8601 format strings, either in basic format or extended format.

Rename CLI transaction list time range options.

d1a506d

Use `--start` and `--end` instead of `--after` and `--before`. See: #467 (comment)

Use a matrix of time range values in transaction list CLI integrati…

0905dd6

…on test.

Make listTransactionsViaCLI completely freeform in its arguments.

52ad301

In response to feedback from previous review: #532 (comment)

jonathanknowles force-pushed the jonathanknowles/cli-list-transactions branch from 2bcd588 to 52ad301 Compare July 24, 2019 02:44

jonathanknowles requested review from piotr-iohk and KtorZ July 24, 2019 03:58

piotr-iohk approved these changes Jul 24, 2019

View reviewed changes

jonathanknowles merged commit a09e08a into master Jul 24, 2019

jonathanknowles deleted the jonathanknowles/cli-list-transactions branch July 24, 2019 06:57

KtorZ added this to the Finalize 'Transactions' API endpoints milestone Jul 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `transaction list` CLI command. #532

Add `transaction list` CLI command. #532

jonathanknowles commented Jul 12, 2019 •

edited

jonathanknowles Jul 22, 2019

piotr-iohk Jul 22, 2019

jonathanknowles Jul 22, 2019

piotr-iohk Jul 22, 2019

jonathanknowles Jul 22, 2019

piotr-iohk Jul 22, 2019

jonathanknowles Jul 23, 2019 •

edited

KtorZ Jul 23, 2019

jonathanknowles Jul 23, 2019

piotr-iohk Jul 22, 2019

jonathanknowles Jul 22, 2019

jonathanknowles Jul 22, 2019

piotr-iohk Jul 22, 2019

jonathanknowles Jul 22, 2019 •

edited

piotr-iohk Jul 22, 2019 •

edited

KtorZ Jul 23, 2019

jonathanknowles Jul 23, 2019

jonathanknowles Jul 24, 2019

piotr-iohk Jul 24, 2019 •

edited

piotr-iohk Jul 22, 2019

rvl left a comment

jonathanknowles commented Jul 23, 2019

piotr-iohk Jul 23, 2019

jonathanknowles Jul 24, 2019 •

edited

piotr-iohk Jul 24, 2019

piotr-iohk left a comment

jonathanknowles commented Jul 24, 2019

Add transaction list CLI command. #532

Add transaction list CLI command. #532

Conversation

jonathanknowles commented Jul 12, 2019 • edited

Issue Number

Overview

Outstanding work for future PRs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanknowles Jul 23, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanknowles Jul 22, 2019 • edited

Choose a reason for hiding this comment

piotr-iohk Jul 22, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piotr-iohk Jul 24, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvl left a comment

Choose a reason for hiding this comment

jonathanknowles commented Jul 23, 2019

Choose a reason for hiding this comment

jonathanknowles Jul 24, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piotr-iohk left a comment

Choose a reason for hiding this comment

jonathanknowles commented Jul 24, 2019

Add `transaction list` CLI command. #532

Add `transaction list` CLI command. #532

jonathanknowles commented Jul 12, 2019 •

edited

jonathanknowles Jul 23, 2019 •

edited

jonathanknowles Jul 22, 2019 •

edited

piotr-iohk Jul 22, 2019 •

edited

piotr-iohk Jul 24, 2019 •

edited

jonathanknowles Jul 24, 2019 •

edited