Pull Request Test Coverage Report for Build 12958602475

Details

• 10 of 10 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.04%) to 78.641%

Totals
Change from base Build 12957441139:	0.04%
Covered Lines:	3645
Relevant Lines:	4635

💛 - Coveralls

CI failing for MSRV due to testcontainers dependencies not supported upon upgrading to the latest version. @DanGould what's your process for this kind of thing? Should I just try downgrading testcontainers to the highest version that still compiles?

I look at crates.io/crates/redis to see which version is msrv compliant

Then I think I'd pin redis https://crates.io/crates/redis/0.23.3 with listed msrv 1.63 via Cargo-minimal.lock by cargo update -p redis --precise 0.23.3

I downgraded testcontainers to the highest version that still supported our MSRV. Unfortunately it doesn't support the cleaner Redis::start() syntax, so we still need to explicitly initialize docker. To prevent the docker client from being dropped during tests' execution we use Box::leak(), which looks kind of scary but should be fine when used only for testing.

re:

I noticed that we initialize a tracing subscriber in a every test but don't really seem to be using tracing much, instead using log::debug in most places. Is there a reason to keep using tracing in tests or should we just remove that dependency too and use log instead?

We can easily convert log::x to tracing::x to take advantage of tracing's additional context. There's also the tracing-log crate to have dependency log::x handled as tracing::x calls.

I feel like tracing calls would be especially useful for payment processors depending on payjoin, like bria, since they depend heavily on structured logging, but I have to admit I'm green to the ins-and-outs of a well maintained system of structured logging.

I thought we should take advantage of tracing's structured logs in payjoin-directory, ohttp-relay, and payjoin-cli to provide structured logging for downstream users. It gives way more context across closure boundaries and with async, and the logs can be filtered dynamically. I figured this would be good for debugging and uptime monitoring in particular, but I do wonder if it's at odds with our goals for privacy. We're certainly not taking advantage of it yet. tracing lets us provide the kind of polish I'd like our actual kit to have post-1.0. If we figure out how to do that I reckon it makes sense to do in tests too.

Is it overly complex in tests? We're definitely not using it to leverage what we print in CI vs local. hard to say. @0xBEEFCAF3 @nothingmuch any comment on where we should be using structured logs