Expose test helpers

[spacebear21]

This is an effort to address #422.

Commonly-used test utilities are moved to a standalone payjoin-test-utils crate. This new crate also provides an opportunity to create downstream test fixtures in payjoin-ffi and language bindings downstream of it.

The new crate also introduces TestServices, which further facilitates testing by handling common operations such as initializing a payjoin directory and OHTTP relay, fetching OHTTP keys, etc.

[coveralls]

Pull Request Test Coverage Report for Build 12876605002

Details

• 110 of 115 (95.65%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.5%) to 72.66%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin-test-utils/src/lib.rs	110	115	95.65%

Totals
Change from base Build 12874640319:	0.5%
Covered Lines:	3641
Relevant Lines:	5011

---

💛 - Coveralls

[DanGould]

Hmm our extensive list of dev-dependencies is why I started doing putting the test utility functions into #405, thinking a _test-utils feature would just change the visibility of some internal types. We do already have _danger-local-https, which makes --all-features insecure, but I'm a bit more wary about adding all of these dev-dependencies as optional features.

I recognize that this is musing / complaining, so I want to make sure we're looking at all of the angles we could be approaching this from. @nothingmuch do you have any experience with test utilities? Any ideas of someone outside the org we might ask who has more experience? Maybe matt

[spacebear21]

I started doing putting the test utility functions into #405

Assuming you meant #425 - somehow I overlooked that PR when I started this one yesterday. I do share your concern about exposing too many optional dev-dependencies, so making a separate crate seems like a better approach. I'm open to other suggestions also.

[DanGould]

Yes I did mean #425 glad you found it.

[nothingmuch]

@nothingmuch do you have any experience with test utilities?

Yes, but not in rust... In other languages, my experience is that reusable code for testing should just be treated as reusable code, so naively I would guess the way to tackle this is the approach of #425

[spacebear21]

I took from #425 to move the test utils to a new crate.

Additionally, the last commit introduces TestServices based on @nothingmuch's earlier suggestion. I'd like to hear your thoughts (and @DanGould's) on the "take handle" approach I took for transferring ownership of the handles when needed in the tokio::select!.

I'm also considering whether it would be beneficial to add some other convenience methods to TestServices:

• Addhttp_agent: Arc<Client> and a corresponding getter since we initialize an agent in every V2 test
• Add a wait_for_all_services_ready helper since we also wait for both services to be ready in all tests.
• Then, the inner do_v*_to_v* test functions can just take a single services: &TestServices parameter instead of two individual URLs plus the cert.
• Maybe also a helper for fetching OHTTP keys?

[nothingmuch]

this is looking good

apart from my one comment, the only other suggestion i have is in the non draft version of this PR squashing the first and 3rd to last commits to eliminate the feature & dev-dependencies churn

[nothingmuch]

perhaps this pattern can be put in a test serivces method that takes an Fn for the do_ methods, and seelects on its own.

somehing like services.run_test_body(do_expiration_tests), and modify those helpers too take services as the single argument. i didn't check to see if that would work for all tests but even if it's only for a large chunk of them this could help simplify things further

[spacebear21]

I tried implementing this but couldn't satisfy the compiler.

EDITED for more context, the actual issue with this approach is:

error[E0308]: mismatched types
   --> payjoin/tests/integration.rs:232:13
    |
232 |             services.run_test_body(do_expiration_tests);
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ one type is more general than the other
    |
    = note: expected opaque type `impl for<'a> std::future::Future<Output = Result<(), Box<(dyn StdError + 'static)>>>`
               found opaque type `impl std::future::Future<Output = Result<(), Box<(dyn StdError + 'static)>>>`
    = help: consider `await`ing on both `Future`s
    = note: distinct uses of `impl Trait` result in different opaque types
note: the lifetime requirement is introduced here
   --> /Users/spacebear/Projects/rust-payjoin/payjoin-test-utils/src/lib.rs:82:25
    |
82  |         F: Fn(&Self) -> Fut,
    |                         ^^^

error: implementation of `Fn` is not general enough
   --> payjoin/tests/integration.rs:232:13
    |
232 |               services.run_test_body(do_expiration_tests);
    |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
   ::: /Users/spacebear/Projects/rust-payjoin/payjoin-test-utils/src/lib.rs:79:5
    |
79  | /     pub async fn run_test_body<Fut, F>(&mut self, test_body: F) -> ()
80  | |     where
81  | |         Fut: Future<Output = Result<(), BoxError>>,
82  | |         F: Fn(&Self) -> Fut,
    | |____________----------------- due to a where-clause on `TestServices::run_test_body`...
    |              |
    |              doesn't satisfy where-clause
    |
    = note: ...`for<'a> fn(&'a TestServices) -> impl std::future::Future<Output = Result<(), Box<(dyn StdError + 'static)>>> {do_expiration_tests}` must implement `Fn<(&TestServices,)>`
    = note: ...but it actually implements `Fn<(&'0 TestServices,)>`, for some specific lifetime `'0`

[nothingmuch]

Hmm, another thought, I was putting together a flake app to start regtest, the directory and the relay, and wrote a yucky shell script with an exit trap and background tasks, it's disgusting... and then realized I'm just replicating this PR but in bash. Anyway, what do you think about adding a main.rs to the test crate that effectively exposes TestServices, for manual testing purposes?

[nothingmuch]

in the PR for #472 i'd like to build on this, the majority of that change boilerplate in the integration tests

@spacebear21 i'm happy to add this as a commit and PR on your branch, or just depend on this branch and rebase as necessary, or just add the ugly function and clean it up later in the interest of getting BIP 77 related changes merged ASAP, please let me know what works best for you

[spacebear21]

Feel free to PR into this branch (or even push your commit directly to it if you have permissions). I may not be able to review tonight but can look tomorrow morning.

[spacebear21]

My latest push:

• Squashed 1st and 3rd commits for easier review per @nothingmuch's suggestion
• Adds http_agent, wait_for_services_ready, and fetch_ohttp_keys to TestServices
• Simplifies the inner test functions to just take services as a single argument

[spacebear21]

Some outstanding questions:

1. Are there any concerns re: the "take handle" approach I took for transferring ownership of the JoinHandles where needed in tokio::select!?
2. Am I missing a cleaner way of implementing Expose test helpers #484 (comment)?
3. Are there any other common test helpers that would make sense to add to payjoin-test-utils / TestServices, perhaps in follow-up PRs?
   • Maybe the redis docker container could be initialized in TestServices::initialize. I'm not sure if there are drawbacks associated with testcontainers and the Docker dependency, especially w.r.t payjoin-ffi/language bindings.
   • bitcoind sender/receiver?

[DanGould]

You've championed major upgrade in a part of the code that's easy to neglect. It's going to make maintaining all of our tests easier and provide a home for de-duplication. All of my comments are minor, including those addressing your questions.

Are there any concerns re: the "take handle" approach I took for transferring ownership of the JoinHandles where needed in tokio::select!?
Am I missing a cleaner way of implementing

Returning an Option<_> means every call I see unwrap() is called. I'd prefer a None return from take() to either panic or pass an error to clean these call sites in line with #487

Similar comment with the _url functions, remove the unwrap either with OnceCell static initialization or expect() to panic

i.e.

static OHTTP_RELAY_URL: OnceCell<Url> = OnceCell::new();

// fn initialize()
//...
        OHTTP_RELAY_URL.get_or_init(|| {
            Url::parse(&format!("http://localhost:{}", ohttp_relay.0)).expect("Invalid URL")
        });
// ...

pub fn ohttp_relay_url(&self) -> Url {
    OHTTP_RELAY_URL.get().expect("OHTTP Relay URL not initialized").clone()
}

I think this is a mere optimization that could be a follow up, and might even be overkill just to remove unwrap with limited perf upgrade.

Maybe the redis docker container could be initialized in TestServices::initialize. I'm not sure if there are drawbacks associated with testcontainers and the Docker dependency, especially w.r.t payjoin-ffi/language bindings.

Almost certainly redis could be initialized in TestServices::initialize. I'd like for us to figure out how to get a real ohttp-relay involved where we're using mock_ohttp_relay in both integration and e2e tests if possible. as well.

I think this would be fine to start up these services from a binary with bindings on top as long as docker were running. I reckon it would work the same, we'd just need to write bindings for payjoin-test-utils.

Are there any other common test helpers that would make sense to add to payjoin-test-utils / TestServices, perhaps in follow-up PRs?

There appears to be a lot of duplicated sender / receiver test setup in integration tests, but that might not belong in TestServices or test-utils. But I think an audit is in order as a follow up while we're doing mass cleanup.

bitcoind sender/receiver?

Unclear how to abstract / if it's necessary abstract individual or enumerated wallet initialization but I think this is ok, both sender and receiver appear to be used in every case.

[DanGould]

You didn't introduce it but this should be a docstring with /// prefix