Remove unwrap() in integration/e2e tests by spacebear21 · Pull Request #512 · payjoin/rust-payjoin

spacebear21 · 2025-01-26T23:47:08Z

Partially address #487.

There are other places to clean up, notably unit tests, that can be addressed in later PRs.

The commit messages might be a little repetitive but I wanted to keep each commit bite-sized for easier review. I can squash them if preferred.

coveralls · 2025-01-26T23:49:39Z

Pull Request Test Coverage Report for Build 13057514318

Details

19 of 19 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.005%) to 78.544%

Totals
Change from base Build 13057114932:	0.005%
Covered Lines:	3646
Relevant Lines:	4642

💛 - Coveralls

nothingmuch

cACK, partial review with a few comments.

Main takeaway is I'm not sure about when .expect() is preferred over .map_err() with format! and bubbling up...

ISTM that the benefits, in descending priority order are:

eliminate unwrap() to avoid vague failures without context (though ? doesn't materially improve that), and to make any snippets copied from tests be better behaved in whatever context they end up in (where ? does make a material difference)
simplifying types for things that should never fail
concision helping readability

is that correct?

nothingmuch · 2025-01-27T02:29:32Z

payjoin-cli/tests/e2e.rs

-        fn find_free_port() -> u16 {
-            let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap();
-            listener.local_addr().unwrap().port()
+        assert!(payjoin_sent??.unwrap_or(false), "Payjoin send was not detected");


I'm confused by .unwrap().unwrap_or(Some(false)).unwrap() becoming payjoin_sent??.unwrap_or(false).

It's a Result<Result<Option<bool>, Elapsed>, JoinError>, where the Option originates from the mpsc's recv(), the `bool is the value sent through the channel.

So isn't this equivalent to payjoin_sent.unwrap().unwrap().unwrap_or(false) instead, i.e. that a timeout bubbles up instead of becoming an assertion error? isn't it more appropriate for the assertion to fail in case of a timeout in which case .unwrap_or(Some(false)).ok_or(...)? would be closer in spirit to what it was, with i guess an anyhow error in the ... indicating the mpsc recv should have worked

also, using await? on the task seems like it reduce some of the noise... if you agree with this suggestion and the previous paragraph, i guess this implies the unwrap_or(Some(false).ok_or() is better done inside the task, so that await?? can bubble the task joining error if any, followed by the ok_or, and leaving payjoin_sent as just a bool?

Great catch! I applied your suggestion, with expect instead of ok_or since the rx channel should never close early.

nothingmuch · 2025-01-27T02:35:58Z

payjoin-test-utils/src/lib.rs

-    pub fn take_directory_handle(&mut self) -> Option<JoinHandle<Result<(), BoxSendSyncError>>> {
-        self.directory.1.take()
+    pub fn take_directory_handle(&mut self) -> JoinHandle<Result<(), BoxSendSyncError>> {
+        self.directory.1.take().expect("directory handle not found")


hmm, i prefer the simpler type here over expecting the caller to bubble this up, since this kind of thing should never fail a test but indicates the test environment is not as expected, having an expect message puts that kind of failure into context...

but i want to make sure i'm not missing anything about tests returning Result, is it equivalent to the test runner doing .unwrap() for us? i found this mentioned in rust by example, which links to a seemingly stale doc link that leads back to rust by example discussing this in the context of main (https://doc.rust-lang.org/edition-guide/rust-2018/error-handling-and-panics/question-mark-in-main-and-tests.html) and ? is ~impossible to websearch for

Yes, when an Error result bubbles up all the way to the test runner it fails the test and prints a debug representation of the error. The way it's displayed is slightly cleaner than when calling unwrap directly, e.g.:

unwrap:

---- integration::v1::v1_to_v1_p2pkh stdout ---- thread 'integration::v1::v1_to_v1_p2pkh' panicked at payjoin/tests/integration.rs:998:51: called `Result::unwrap()` on an `Err` value: PsbtEncoding(ConsensusEncoding(Io(Error { kind: UnexpectedEof, error: None }))) note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

?:

---- integration::v1::v1_to_v1_p2pkh stdout ---- Error: PsbtEncoding(ConsensusEncoding(Io(Error { kind: UnexpectedEof, error: None })))

nothingmuch · 2025-01-27T02:40:09Z

payjoin-test-utils/src/lib.rs

        .danger_accept_invalid_certs(true)
        .use_rustls_tls()
-        .add_root_certificate(reqwest::tls::Certificate::from_der(cert_der.as_slice()).unwrap()))
+        .add_root_certificate(reqwest::tls::Certificate::from_der(cert_der.as_slice())?))


in line with the remark on take_... variants, is this perhaps better handled with .expect()?

spacebear21 · 2025-01-28T20:22:30Z

Main takeaway is I'm not sure about when .expect() is preferred over .map_err() with format! and bubbling up...

ISTM that the benefits, in descending priority order are:
1. eliminate unwrap() to avoid vague failures without context (though ? doesn't materially improve that), and to make any snippets copied from tests be better behaved in whatever context they end up in (where ? does make a material difference)
2. simplifying types for things that should never fail
3. concision helping readability
is that correct?

Yes - I'd add another benefit which is that expect() panics immediately whereas bubbling up with map_err()? allows us to handle errors appropriately, which helps towards #421 and also opens the door for testing things we expect to fail with specific errors, e.g. #339.

DanGould

ACK 988ded4

I left some comments on further cleanups, but I think everything here is pointed in the right direction i.e. cleans up tech debt rather than takes new on and don't see big reasons not to merge and address the concerns in a follow up referencing the issues stated here.

DanGould · 2025-01-29T23:34:30Z

payjoin-cli/tests/e2e.rs

    #[cfg(feature = "v2")]
    #[tokio::test(flavor = "multi_thread", worker_threads = 4)]
-    async fn send_receive_payjoin() {
+    async fn send_receive_payjoin() -> Result<(), Box<dyn std::error::Error + Send + Sync>> {


nit: Why not use BoxSendSyncError here?

It would have to be imported in mod e2e but that results in a "unused" warning when the v2 feature is disabled. I suppose we could feature-gate the import, or put the v1 and v2 tests in separate modules like we do in integration.rs

That's a good reason. It would also be more organized to separate v1, v2 modules (and even separate files that use super::* but is not a necessity.

payjoin-test-utils/src/lib.rs

payjoin/tests/integration.rs

payjoin-cli/tests/e2e.rs

spacebear21 · 2025-01-30T17:22:01Z

My latest push addresses the minor issues Dan brought up, and should be good to merge as-is.

DanGould

ACK 2f14846

DanGould · 2025-01-30T18:31:41Z

payjoin-cli/tests/e2e.rs

    #[cfg(feature = "v2")]
    #[tokio::test(flavor = "multi_thread", worker_threads = 4)]
-    async fn send_receive_payjoin() {
+    async fn send_receive_payjoin() -> Result<(), Box<dyn std::error::Error + Send + Sync>> {


That's a good reason. It would also be more organized to separate v1, v2 modules (and even separate files that use super::* but is not a necessity.

spacebear21 added 5 commits January 26, 2025 18:40

Don't unwrap when parsing URIs

7297bbb

Don't unwrap when initializing TestServices

eb7bb34

Don't unwrap when taking JoinHandles

79ebbe7

Don't unwrap in try_request_with_bad_keys

bc4dd05

Don't unwrap responses in integration tests

ae8b822

spacebear21 requested a review from DanGould January 26, 2025 23:47

nothingmuch reviewed Jan 27, 2025

View reviewed changes

spacebear21 force-pushed the no-unwrap-in-tests branch 2 times, most recently from 5ca026e to 988ded4 Compare January 28, 2025 20:12

DanGould approved these changes Jan 29, 2025

View reviewed changes

spacebear21 added 3 commits January 30, 2025 12:08

Don't unwrap in proposal handlers

f62d48e

Don't unwrap in test utils

30b2803

Don't unwrap in e2e tests

2f14846

spacebear21 force-pushed the no-unwrap-in-tests branch from 988ded4 to 2f14846 Compare January 30, 2025 17:11

spacebear21 mentioned this pull request Jan 30, 2025

Implement std::error::Error for Uri errors payjoin/bitcoin_uri#6

Open

DanGould approved these changes Jan 30, 2025

View reviewed changes

DanGould merged commit 01107a8 into payjoin:master Jan 30, 2025
6 checks passed

DanGould mentioned this pull request Mar 17, 2025

Remove .unwrap(), even in Tests #487

Closed

Conversation

spacebear21 commented Jan 26, 2025

Uh oh!

coveralls commented Jan 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 13057514318

Details

💛 - Coveralls

Uh oh!

nothingmuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacebear21 commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

spacebear21 commented Jan 30, 2025

Uh oh!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Jan 26, 2025 •

edited

Loading

spacebear21 commented Jan 28, 2025 •

edited

Loading