Skip to content

send SIGINT instead of SIGKILL in e2e test#497

Merged
spacebear21 merged 1 commit intopayjoin:masterfrom
nothingmuch:shutdown-resume-with-sigint
Jan 20, 2025
Merged

send SIGINT instead of SIGKILL in e2e test#497
spacebear21 merged 1 commit intopayjoin:masterfrom
nothingmuch:shutdown-resume-with-sigint

Conversation

@nothingmuch
Copy link
Collaborator

When running a llvm-cov instrumented payjoin-cli in the e2e test, using calling .kill() on the tokio::process::Child sends a SIGKILL, which terminates the process immediately, causing the coverage measurements to not be written.

The payjoin-cli resume command already has an interrupt handler, so sending SIGINT instead makes it exit gracefully.

Closes #494

When running a llvm-cov instrumented payjoin-cli in the e2e test, using
calling .kill() on the tokio::process::Child sends a SIGKILL, which
terminates the process immediately, causing the coverage measurements to
not be written.

The payjoin-cli resume command already has an interrupt handler, so
sending SIGINT instead makes it exit gracefully.
@nothingmuch
Copy link
Collaborator Author

Note that Cargo-{minimal,recent}.lock are still identical after adding nix as a dev dependency, see also #454.

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 12858008331

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+11.8%) to 72.247%

Totals Coverage Status
Change from base Build 12832868611: 11.8%
Covered Lines: 3517
Relevant Lines: 4868

💛 - Coveralls

@nothingmuch nothingmuch marked this pull request as ready for review January 19, 2025 23:14
Copy link
Contributor

@DanGould DanGould left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK Confirmed that the coveralls build for this PR now includes coverage measurement for payoin-cli. Awesome catch.

Copy link
Collaborator

@spacebear21 spacebear21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I noticed is that payjoin-cli v1 is not covered, which is likely due to the missing _danger-local-https in contrib/coverage.sh for the v1 llvm-cov command. This can be addressed in a separate PR.

@spacebear21 spacebear21 merged commit d13d6e4 into payjoin:master Jan 20, 2025
6 checks passed
@spacebear21 spacebear21 mentioned this pull request Apr 2, 2025
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement graceful shutdown for payjoin-cli

4 participants