Fix payjoin-cli v1 coverage#532
Conversation
The v1 e2e test is feature-gated behind `not(feature = "v2")`, so it doesn't run when --all-features is enabled. This change ensures v1 coverage is reported.
Exiting cleanly allows other processes to complete (e.g. coverage reports).
Ensure that the child process completes its handling of the interrupt signal. This prevents the child process from panicking with `failed printing to stdout: Broken pipe (os error 32)` when it tries to print to stdout in the interrupt handler.
There is no obvious reason to spawn new tasks only for the purpose of reading and writing from/to stdout. More importantly, this behavior previously caused the payjoin-cli interrupt handlers to panic with `failed printing to stdout: Broken pipe (os error 32)` due to `stdout` getting dropped by the time sigint is called. This ensures `stdout` lives long enough for the interrupt message to print.
Avoid repetition by introducing `wait_for_stdout_match` which reads from stdout until a match is found against the given pattern. Note: it's important that `child_stdout` lives long enough, until `sigint` is called on the child process. Otherwise the interrupt handler would try to `println!` to a dropped stdout and panic.
Pull Request Test Coverage Report for Build 13203104215Details
💛 - Coveralls |
nothingmuch
left a comment
nothingmuch
left a comment
There was a problem hiding this comment.
ACK. This was very easy to review, great commit messages, clear changes, couldn't ask for a better PR. One tiny nit, sigint now no longer just sends a signint, maybe it should be renamed to terminate or something more descriptive?
I was also kind of surprised at the coverage report comment (not so much the report itself) since comparing the actual new & base reports, v1.rs is indeed included whereas before it wasn't, but the summary doesn't seem to take this into account.
nothingmuch
left a comment
nothingmuch
left a comment
There was a problem hiding this comment.
oops, meant to approve
It no longer just sends a sigint, so rename it to something more descriptive.
That is surprising 🤔 it even shows v1.rs as "new coverage" in the report itself. |
3 participants
Fix #499
This required a few changes:
llvm-covcommand for running the payjoin-cli v1 e2e test, as it is not currently covered by--all-features.siginttest function to wait until the child process completely exits, so that the interrupt handlers can actually print to stdout without panicking (failed printing to stdout: Broken pipe (os error 32)). This issue also affected the payjoin-cli v2 test, so this fix reduces the noise there as well (see Fixpayjoin-cliv1 coverage #499 (comment)).tokio::spawns in tests which movedcli.stdoutout of the main loop, also causing panics like the former in interrupt handlers.