Expose test helpers #484
Expose test helpers #484
Conversation
This adds a test_utils module, gated behind the `_test_utils` feature. It contains internal test utilities shared between payjoin integration tests and payjoin-cli e2e tests. As written currently, those common functions depend on many `dev_dependencies` which must now be brought into the main `dependencies` and marked as optional, and are enabled by the `_test_utils` feature. I'm unsure if this is a best practice.
Pull Request Test Coverage Report for Build 12838560831Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
|
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 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 |
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. |
|
Yes I did mean #425 glad you found it. |
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 |
Feature-gating behind _test_utils poses several concerns, notably making bloating --all-features and requiring adding a bunch of optional dependencies due to our extensive list of dev-dependencies. Instead, expose payjoin-test-utils as a standalone crate. This new crate also provides an opportunity to create downstream testfixtures in payjoin-ffi and language bindings downstream of it. Co-authored-by: DanGould <[email protected]>
TestServices is a helper struct that initializes a Payjoin directory and OHTTP relay to facilitate v2 integration tests. It holds onto the running process handles until ownership is taken away explicitly with `take_*_handle`.
|
I took from #425 to move the test utils to a new crate. Additionally, the last commit introduces I'm also considering whether it would be beneficial to add some other convenience methods to
|
| tokio::select!( | ||
| err = ohttp_relay_handle => panic!("Ohttp relay exited early: {:?}", err), | ||
| err = directory_handle => panic!("Directory server exited early: {:?}", err), | ||
| res = do_expiration_tests(ohttp_relay, directory, cert) => assert!(res.is_ok(), "v2 send receive failed: {:#?}", res) | ||
| err = services.take_ohttp_relay_handle().unwrap() => panic!("Ohttp relay exited early: {:?}", err), | ||
| err = services.take_directory_handle().unwrap() => panic!("Directory server exited early: {:?}", err), | ||
| res = do_expiration_tests(services.ohttp_relay_url(), services.directory_url(), services.cert()) => assert!(res.is_ok(), "v2 send receive failed: {:#?}", res) |
There was a problem hiding this comment.
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
|
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? |
This is an effort to address #422.
It adds a test_utils module, gated behind the
_test_utilsfeature. So far I only extracted common functions and made no attempt to re-architect anything (e.g. by exposing aTestServicesstruct or similar).As written currently, those common functions depend on many
dev_dependencies. To satisfy the compiler I brought these into the maindependenciesand marked them as optional, so the_test_utilsfeature can enable them. I'm not sure if this is a best practice but it seems to be what rust-lightning does (albeit with way less dependencies).