Add mutants for sender module by benalleng · Pull Request #717 · payjoin/rust-payjoin

benalleng · 2025-05-26T20:36:07Z

This adds the mutants for the mod file in the sender.

This is the first part of the sender to be covered by mutation testing. Once the the sender has complete mutation coverage we can look at finalizing #539

benalleng · 2025-05-26T20:39:42Z

No missed mutants as of 54c9110

Found 320 mutants to test
ok       Unmutated baseline in 29.5s build + 11.7s test
 INFO Auto-set test timeout to 59s
320 mutants tested in 9m 21s: 197 caught, 123 unviable

coveralls · 2025-05-26T20:40:39Z

Pull Request Test Coverage Report for Build 15424551466

Warning: 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.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

220 of 220 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.5%) to 84.835%

Totals
Change from base Build 15400978156:	0.5%
Covered Lines:	6618
Relevant Lines:	7801

💛 - Coveralls

benalleng · 2025-05-28T19:34:58Z

In reference to the fact that ensure!() is not caught by mutations as @spacebear21 pointed out first in #539 I added a commit that expands the mutation coverage by converting our ensure macros to plain function if statements. It's possible there is a cleaner way to add these into the mutation flow but with most of them being quite simple I thought just inverting the logic as an if statement made sense. Below is the new mutation coverage as of e484cd5.

Found 350 mutants to test
ok       Unmutated baseline in 28.4s build + 16.1s test
 INFO Auto-set test timeout to 1m 21s
350 mutants tested in 10m 36s: 222 caught, 128 unviable

I did have a mutation that was missed but I can no longer reproduce it and the logic indicates it shouldn't have happened but it did, I don't think we need to worry about this too much but something to point out if anyone else is able to reproduce it

MISSED   payjoin/src/send/mod.rs:275:24: replace match guard with true in 9.2s build + 21.9s test

DanGould · 2025-05-30T16:13:34Z

payjoin/src/send/mod.rs

+                    if self.output_substitution == OutputSubstitution::Disabled
+                        || (proposed_txout.script_pubkey == original_output.script_pubkey
+                            && proposed_txout.value < original_output.value)
+                    {
+                        return Err(InternalProposalError::DisallowedOutputSubstitution);
+                    };


I think this is more strict than the original, it errors if output substitution is disabled, regardless of the output

Good catch, I think I have the logic cleaned up here now.

spacebear21 · 2025-05-30T17:20:15Z

payjoin/src/send/mod.rs

-macro_rules! ensure {
-    ($cond:expr, $error:ident) => {
-        if !($cond) {
-            return Err(InternalProposalError::$error);
-        }
-    };
-}
-


I wonder, could this be replaced with a simple function?

fn ensure<T>(condition: bool, error: T) -> Result<(), T> { if !condition { return Err(error); } Ok(()) } ensure(original_fee <= proposed_fee, InternalProposalError::AbsoluteFeeDecreased)?;

In addition to keeping to DRY, it would make this diff easier to reason about and validate. The key thing to ensure with this approach is that mutants can still modify the operators in the condition.

So I don't think it catches everything, we lose 8 possible mutants when going about this approach, I will check what gets missed. I am going to go out on a limb and assume that the .is_empty() or .is_some() by themselves as params are not mutated while they are in a traditional if clause as the clause can just be replaced with true or false. operands like || && <= etc are still mutated here I'm assuming.

~~I am checking now what can no-longer be mutated.~~

I think actually these new mutants are just a byproduct of how the if clauses added a small amount of complexity into the catches, we can use this ensure function.

It was mostly the fact that >= has only one mutant; <
While the inverse > has two mutants; < and ==

Also having to check if (!is_empty()) adds a mutant with the negation vs the base ensure

benalleng · 2025-05-30T22:50:33Z

Mutation coverage as of e3b048f

Found 342 mutants to test
ok       Unmutated baseline in 24.5s build + 11.2s test
 INFO Auto-set test timeout to 56s
342 mutants tested in 9m 32s: 214 caught, 128 unviable

benalleng · 2025-05-30T23:04:33Z

I wanted to mention that these example psbts in test_find_change_index() are not perfectly modified psbts, as such I am not sure if they would be valid if the test for each specific assert did not exist its possible the psbt would still fail at some other point.

I had a hard time grokking what key values to change on the psbt specifically to get to each error there but I do think targeting the key values instead of just creating new psbt strings is a better way to do this.

I tried to find an exact example but I couldn't before I stepped off but I'm fairly certain these psbts would fail on more than just each assert it is testing for.

DanGould

ACK e3b048f

The function is so much better and it seems each instance is the same as before.

In the tests, did you try to make the PSBTs in rust rather than just with strings? It seems like a bit of a shortcut to read in strings, but if you're confident they contain what the comments state, and you did try to have semantic rust that was for some reason a PITA, I don't see a great reason to oppose letting this in as-is. How did you make the strings?

benalleng · 2025-06-02T22:33:50Z

I used bitcoin-cli createpsbt and https://bip174.org for some additional modification

benalleng · 2025-06-03T14:40:29Z

Sat down and managed to make them in rust after-all it was actually rather easy after realizing I had the psbt strings as reference to what should be changed 😅

benalleng · 2025-06-03T16:21:40Z

Tested again and confirmed no new mutants on 5e2bd4f

DanGould

I left a commit which should be squashed that's a shortcut for creating ScriptBuf from hex and a comment on string equalilty that we should replace with error matching (can squash/un-credit me). After those this should be good to go

DanGould · 2025-06-03T17:18:43Z

payjoin/src/send/mod.rs

+        assert_eq!(
+            ctx.clone().process_proposal(proposal.clone()).unwrap_err().to_string(),
+            InternalProposalError::DisallowedOutputSubstitution.to_string()
+        );


assert!(match!

Suggested change

assert_eq!(

ctx.clone().process_proposal(proposal.clone()).unwrap_err().to_string(),

InternalProposalError::DisallowedOutputSubstitution.to_string()

);

assert!(matches!(

ctx.clone().process_proposal(proposal.clone()).unwrap_err(),

InternalProposalError::DisallowedOutputSubstitution

));

DanGould · 2025-06-03T17:22:57Z

payjoin/src/send/mod.rs

+        let mut proposal: bitcoin::Psbt = PARSED_PAYJOIN_PROPOSAL.clone();
+
+        ctx.fee_contribution = None;
+        proposal.unsigned_tx.output.get_mut(0).unwrap().value =


In the future, especially for tests, I think output[0] would work here and still panic. The only time to use get_mut(0) is if you wanted to handle it without a panic.

DanGould · 2025-06-03T17:37:47Z

yeah if you use output[0] you don't even need the unwrap!

benalleng · 2025-06-03T17:51:46Z

I just need to bite the bullet and finally make a pre-commit hook 😅

This adds the mutants for the mod file in the sender. I am breaking the mutation coverage up for the sender file by file so there is not a huge PR touching everything as we can add coverage one file at a time.

By using plain if statements we can test these logic blocks with mutants ensuring some better coverage on the send mod.rs file.

DanGould · 2025-06-03T18:37:07Z

payjoin/src/send/mod.rs

+            Some((Amount::from_sat(1000), None)),
+            true,
+        );
+        assert_eq!(fee_contribution.err(), Some(InternalBuildSenderError::AmbiguousChangeOutput));


In the future I think this syntax is simpler:

Suggested change

assert_eq!(fee_contribution.err(), Some(InternalBuildSenderError::AmbiguousChangeOutput));

assert_eq!(fee_contribution, Err(InternalBuildSenderError::AmbiguousChangeOutput));

DanGould

ACK d8d7c1d

benalleng force-pushed the sender-mutants branch from 16ecc8f to 54c9110 Compare May 26, 2025 20:38

benalleng force-pushed the sender-mutants branch from 54c9110 to d6fdd80 Compare May 26, 2025 20:45

DanGould self-requested a review May 26, 2025 23:44

DanGould requested changes May 30, 2025

View reviewed changes

spacebear21 reviewed May 30, 2025

View reviewed changes

benalleng force-pushed the sender-mutants branch 4 times, most recently from 12ba639 to e3b048f Compare May 30, 2025 22:49

DanGould approved these changes Jun 2, 2025

View reviewed changes

benalleng force-pushed the sender-mutants branch from e3b048f to 9a81cb7 Compare June 3, 2025 14:39

benalleng force-pushed the sender-mutants branch 4 times, most recently from e4b0b5a to 5e2bd4f Compare June 3, 2025 15:31

DanGould requested changes Jun 3, 2025

View reviewed changes

benalleng force-pushed the sender-mutants branch from 734a494 to a3751b4 Compare June 3, 2025 17:34

benalleng force-pushed the sender-mutants branch 2 times, most recently from 790aa6b to 4fe9c78 Compare June 3, 2025 18:04

Add mutants for sender module

bc46a38

This adds the mutants for the mod file in the sender. I am breaking the mutation coverage up for the sender file by file so there is not a huge PR touching everything as we can add coverage one file at a time.

Use plain function logic instead of ensure checks

d8d7c1d

By using plain if statements we can test these logic blocks with mutants ensuring some better coverage on the send mod.rs file.

benalleng force-pushed the sender-mutants branch from 4fe9c78 to d8d7c1d Compare June 3, 2025 18:07

DanGould reviewed Jun 3, 2025

View reviewed changes

DanGould approved these changes Jun 3, 2025

View reviewed changes

DanGould merged commit 6975557 into payjoin:master Jun 3, 2025
7 checks passed

	assert_eq!(fee_contribution.err(), Some(InternalBuildSenderError::AmbiguousChangeOutput));
	assert_eq!(fee_contribution, Err(InternalBuildSenderError::AmbiguousChangeOutput));

Conversation

benalleng commented May 26, 2025

Uh oh!

benalleng commented May 26, 2025

Uh oh!

coveralls commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 15424551466

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

benalleng commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DanGould May 30, 2025

Choose a reason for hiding this comment

Uh oh!

benalleng May 30, 2025

Choose a reason for hiding this comment

Uh oh!

spacebear21 May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benalleng May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benalleng May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benalleng commented May 30, 2025

Uh oh!

benalleng commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DanGould left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benalleng commented Jun 2, 2025

Uh oh!

benalleng commented Jun 3, 2025

Uh oh!

benalleng commented Jun 3, 2025

Uh oh!

DanGould left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanGould Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

DanGould Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

DanGould commented Jun 3, 2025

Uh oh!

benalleng commented Jun 3, 2025

Uh oh!

DanGould Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented May 26, 2025 •

edited

Loading

benalleng commented May 28, 2025 •

edited

Loading

spacebear21 May 30, 2025 •

edited

Loading

benalleng May 30, 2025 •

edited

Loading

benalleng May 30, 2025 •

edited

Loading

benalleng commented May 30, 2025 •

edited

Loading

DanGould left a comment •

edited

Loading

DanGould left a comment •

edited

Loading