Update error handling in cli-app

[KevinWMatthews]

The cli app will panic in ws_account_payments_incoming for several legitimate use cases:

• invalid URL
• invalid URL protocol/scheme
• ilp node not running

This extends the interpreter.rs module's Error enum, allowing the ? to gracefully report failures to the end user. Is this the style you prefer for error handling?

A few other notes:

• Refactored a nested match statement for readability, drawn from this clap example. The behavior should be the same.
• Is there a more descriptive name for ClientErr, say, RequestErr?
• Can socket.read_message().expect("Could not receive WebSocket message") occur in normal use?
• Is there is a reasonable way to spin up an ilp node for testing runtime failures? There doesn't seem to be anything that actually exercises NodeClient::ws_account_payments_incoming().

[bstrie]

Thanks for the PR! I'll get to reviewing this ASAP.

[bstrie]

Reviewed, but I forgot to click the "changes requested" button. :) Let me know what you think.

[KevinWMatthews]

I've added an error handler for catching errors from set_scheme but I haven't figured out how to actually cause the error.

set_scheme("ws") will fail on these parsable URLs:

• scheme:localhost
• scheme:/localhost

However, it seems like when url.parse() detects an http or https scheme it forces the URL to have a valid base. For example, url.parse() will:

• change http:localhost to http://localhost
• change http:/localhost to http://localhost
• reject http//localhost with Url::ParseError "relative URL without a base"

Once parse reformats the URL, set_scheme succeeds.

I don't see any harm in catching an error from set_scheme (up to you) but I haven't added a test because I haven't figured out what to test.

[bstrie]

I don't see any harm in catching an error from set_scheme (up to you) but I haven't added a test because I haven't figured out what to test.

Interesting that the error case may be unreachable thanks to the semantics of parse, but since those semantics are observed rather than explicitly documented I'd prefer to to err on the side of properly propagating the error. I'm fine with not having a test for that case; were you thinking of adding should-fail tests for the other error cases?

[KevinWMatthews]

Expecting CLI app tests to fail until the testnet comes back up:

test interface_tests::testnet_setup ... FAILED

failures:

---- interface_tests::testnet_setup stdout ----
thread 'interface_tests::testnet_setup' panicked at 'Got unexpected response from Xpring Testnet Signup API: Error(Json(Error("expected value", line: 1, column: 1)))', src/libcore/result.rs:1084:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.


failures:
    interface_tests::testnet_setup

test result: FAILED. 19 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

This does raise an interesting point - some unit tests will fail without an internet connection.

(Edit) this will be fixed by #487. A manual fixup should be done after both are merged but it shouldn't be difficult.

[KevinWMatthews]

were you thinking of adding should-fail tests for the other error cases?

I haven't thought of anything else to add - the new tests exercise the new error types at least to some extent (excepting a SchemeErr). We could dig up more failure cases if you think it's valuable.

It would be cool to spin up a node and add some runtime checks, but that might be best left as a separate endeavor?

[bstrie]

This does raise an interesting point - some unit tests will fail without an internet connection.

This is the goal of the interface_tests module (see the comment at the top of it); all failures should be 100% deterministic and errors from hitting the network should be entirely expected and disregarded.

[bstrie]

It would be cool to spin up a node and add some runtime checks, but that might be best left as a separate endeavor?

Yes, adding integration testing like this is unnecessary at this point (the existing tests for the HTTP API elsewhere in the repo should suffice, and if they don't that should be fixed elsewhere).

[bstrie]

Can you rebase on top of #487?

[KevinWMatthews]

Rebase finished, went smoothly.

[bstrie]

@KevinWMatthews I've pushed some changes, what do you think? I couldn't resist giving thiserror a spin (it's real nice, turns out), and on consideration I don't think we really need the should-fail tests for ProtocolErr and UsageErr.

[KevinWMatthews]

@bstrie Nice! I see what you mean about thiserror - it looks pretty slick.

That's fine if you want to omit tests for ProtocolErr and UsageErr. I added them to illustrate the failing behavior - easier than a lengthy explanation.

I see what you did in should_parse(): be explicit that ClientErr and WebsocketErr are ignored in the tests. Thanks!

[KevinWMatthews]

Rebased onto lastest master.
The previous tests failed in the ilp-node suite - not yet sure why.

[gakonst]

@KevinWMatthews we had some flakiness which should be fixed in master now.

[KevinWMatthews]

@gakonst Thanks!
@bstrie Nice, the SendErr and TestnetErr types are more descriptive than ClientErr. Looks good!

Do you prefer to rebase onto master before merging?

[bstrie]

@KevinWMatthews thanks again for your work here! Sorry that it took some time to merge, we've had various events and travel to prepare for that have been occupying much of our bandwidth.

[KevinWMatthews]

@bstrie No worries! I understand the need to prioritize.
Thanks for the ping!