Catch new match arm cargo mutants by benalleng · Pull Request #686 · payjoin/rust-payjoin

benalleng · 2025-05-06T18:32:30Z

The newest version of cargo mutants v25.0.1 added a new type of mutation that attempts to delete entire match arms.

Mutants passing on 7b3ea49

259 mutants tested in 8m 49s: 163 caught, 96 unviable

Closes #671

coveralls · 2025-05-06T18:37:36Z

Pull Request Test Coverage Report for Build 14887651084

Details

23 of 23 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.3%) to 82.274%

Totals
Change from base Build 14868790478:	0.3%
Covered Lines:	5477
Relevant Lines:	6657

💛 - Coveralls

benalleng · 2025-05-06T18:50:21Z

payjoin/src/receive/optional_parameters.rs

                warn!("only one additional-fee parameter specified: {params:?}");
            }
-            _ => (),
+            (None, None) => (),


This is a notable change to prevent the mutant, seeing as it is the only remaining possibility I thought it was an ok change to make here.

DanGould · 2025-05-06T19:05:04Z

payjoin/src/receive/optional_parameters.rs

                            }
                        },
-                ("minfeerate", fee_rate) =>
+                ("minfeerate", fee_rate) => {


These bracket changes seem like a lint. Were these fixes autogen or is this your preferred syntax or something else?

definitely autogen, I have been struggling a bit after I reset my dev environment to try out nixos

payjoin/src/receive/optional_parameters.rs

payjoin/src/uri/mod.rs

payjoin/tests/integration.rs

spacebear21

tACK 8ef8b21

I ran cargo mutants on master then this branch and confirmed the 7 missed mutants are gone. I also got 3-4 timeouts on both branches that seem unrelated to this PR but may be worth a follow up if others can replicate.

benalleng · 2025-05-07T00:04:58Z

I ran cargo mutants on master then this branch and confirmed the 7 missed mutants are gone. I also got 3-4 timeouts on both branches that seem unrelated to this PR but may be worth a follow up if others can replicate.

Any traces for those timeouts?

DanGould

Would you pretty please change the error so that we can test the pattern (which is different from exact equality)?

We should be able to use this pattern elsewhere to get rid of the string checks, which, check out the following review, can be buggy for actually testing desired behavior

DanGould · 2025-05-07T15:31:44Z

payjoin/src/uri/mod.rs

+        assert_eq!(
+            pjuri.unwrap_err().to_string(),
+            bitcoin_uri::de::Error::Extras(PjParseError::from(
+                InternalPjParseError::DuplicateParams("pjos")
+            ))
+            .to_string()


This is a very weird assert, testing string equality but from a differently produced error. DuplicateParams("pjos") not DuplicateParams("pj"), which error matching would catch

IMO this test should be updated to

fn test_pj_duplicate_params() { let uri = "bitcoin:12c6DSiU4Rq3P4ZxziKxzrL5LmMBrzjrJX?pjos=1&pjos=1&pj=HTTPS://EXAMPLE.COM/\ %23OH1QYPM5JXYNS754Y4R45QWE336QFX6ZR8DQGVQCULVZTV20TFVEYDMFQC"; let pjuri = Uri::try_from(uri); assert!(matches!( pjuri, Err(bitcoin_uri::de::Error::Extras(PjParseError( InternalPjParseError::DuplicateParams("pjos") ))) )); let uri = "bitcoin:12c6DSiU4Rq3P4ZxziKxzrL5LmMBrzjrJX?pjos=1&pj=HTTPS://EXAMPLE.COM/\ %23OH1QYPM5JXYNS754Y4R45QWE336QFX6ZR8DQGVQCULVZTV20TFVEYDMFQC&pj=HTTPS://EXAMPLE.COM/\ %23OH1QYPM5JXYNS754Y4R45QWE336QFX6ZR8DQGVQCULVZTV20TFVEYDMFQC"; let pjuri = Uri::try_from(uri); assert!(matches!( pjuri, Err(bitcoin_uri::de::Error::Extras(PjParseError( InternalPjParseError::DuplicateParams("pj") ))) )); }

before merge, which requires one setup commit that changes visibility PjParseError(pub(crate) InternalPjParseError)

Wow, definitely an oversight on that mistake! Ok, I get it now, didn't quite understand the intention from the previous review, but I like this pattern.

fwiw I didn't quite understand the difference between equality and pattern matching until I dug in here, so my intention was unclear. I could just smell we weren't testing the right thing

To allow for better error pattern matching and not string comparison for errors it would be helpful to have internal error types at least public within the payjoin crate for comparison.

The newest version of cargo mutants v25.0.1 added a new type of mutation that attempts to delete entire match arms.

spacebear21 · 2025-05-07T16:40:53Z

I ran cargo mutants on master then this branch and confirmed the 7 missed mutants are gone. I also got 3-4 timeouts on both branches that seem unrelated to this PR but may be worth a follow up if others can replicate.

Any traces for those timeouts?

master:

❯ cargo mutants -j 2
Found 261 mutants to test
ok       Unmutated baseline in 13.8s build + 18.1s test
 INFO Auto-set test timeout to 1m 31s
MISSED   payjoin/src/uri/mod.rs:154:21: replace match guard with true in 9.4s build + 19.6s test
MISSED   payjoin/src/uri/mod.rs:168:13: delete match arm in 1.9s build + 17.2s test
TIMEOUT  payjoin/src/receive/v1/mod.rs:722:47: replace == with != in ProvisionalProposal::sender_input_indexes in 14.6s build + 91.0s test
MISSED   payjoin/src/receive/optional_parameters.rs:80:17: delete match arm in 1.9s build + 20.9s test
TIMEOUT  payjoin/src/receive/v1/mod.rs:456:33: replace > with == in WantsInputs::avoid_uih in 2.1s build + 91.0s test
TIMEOUT  payjoin/src/receive/v1/mod.rs:617:77: replace -= with += in ProvisionalProposal::apply_fee in 13.2s build + 91.0s test
test     payjoin/src/receive/v2/mod.rs:145:30: replace > with < in Receiver::extract_req ... 14.0s
MISSED   payjoin/src/receive/v2/mod.rs:337:13: delete match arm in 2.6s build + 30.4s test
MISSED   payjoin/src/receive/optional_parameters.rs:105:13: delete match arm in 2.3s build + 19.3s test
MISSED   payjoin/src/uri/mod.rs:177:13: delete match arm in 12.7s build + 22.2s test
MISSED   payjoin/src/uri/mod.rs:169:23: replace match guard with true in 2.0s build + 30.9s test
261 mutants tested in 16m 33s: 7 missed, 155 caught, 96 unviable, 3 timeouts

this branch:

❯ cargo mutants -j 2
Found 259 mutants to test
ok       Unmutated baseline in 15.3s build + 19.2s test
 INFO Auto-set test timeout to 1m 36s
TIMEOUT  payjoin/src/receive/v1/mod.rs:713:9: replace ProvisionalProposal::sender_input_indexes -> Vec<usize> with vec![0] in 2.8s build + 96.0s test
TIMEOUT  payjoin/src/receive/v1/mod.rs:522:9: replace WantsInputs::receiver_min_input_amount -> Amount with Default::default() in 2.4s build + 96.1s test
TIMEOUT  payjoin/src/receive/v1/mod.rs:456:33: replace > with < in WantsInputs::avoid_uih in 1.9s build + 96.1s test
TIMEOUT  payjoin/src/receive/v1/mod.rs:637:36: replace > with < in ProvisionalProposal::apply_fee in 7.9s build + 96.0s test
259 mutants tested in 16m 5s: 159 caught, 96 unviable, 4 timeouts

DanGould

ACK 7b3ea49

@spacebear21 must the timeouts you mentioned block merge or may we proceed with what we have here?

spacebear21 · 2025-05-07T19:53:58Z

They seem unrelated and I can't replicate reliably so this is fine to merge.

benalleng commented May 6, 2025

View reviewed changes

benalleng force-pushed the mutants-05-06 branch from 1be6568 to b036c5d Compare May 6, 2025 19:07

DanGould reviewed May 6, 2025

View reviewed changes

benalleng force-pushed the mutants-05-06 branch from b036c5d to b5e7c13 Compare May 6, 2025 20:13

spacebear21 reviewed May 6, 2025

View reviewed changes

payjoin/tests/integration.rs Show resolved Hide resolved

payjoin/tests/integration.rs Outdated Show resolved Hide resolved

payjoin/tests/integration.rs Show resolved Hide resolved

benalleng force-pushed the mutants-05-06 branch 4 times, most recently from 1f5c7c3 to 8ef8b21 Compare May 6, 2025 21:47

spacebear21 approved these changes May 6, 2025

View reviewed changes

DanGould requested changes May 7, 2025

View reviewed changes

benalleng added 2 commits May 7, 2025 11:47

Elevate InternalPjParse Error to be crate public

120e4e0

To allow for better error pattern matching and not string comparison for errors it would be helpful to have internal error types at least public within the payjoin crate for comparison.

Catch new match arm cargo mutants

7b3ea49

The newest version of cargo mutants v25.0.1 added a new type of mutation that attempts to delete entire match arms.

benalleng force-pushed the mutants-05-06 branch from 8ef8b21 to 7b3ea49 Compare May 7, 2025 15:48

DanGould approved these changes May 7, 2025

View reviewed changes

spacebear21 merged commit 7f27656 into payjoin:master May 7, 2025
7 checks passed

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Pull Request Test Coverage Report for Build 14887651084

Details

💛 - Coveralls

Uh oh!

benalleng May 6, 2025

Choose a reason for hiding this comment

Uh oh!

DanGould May 6, 2025

Choose a reason for hiding this comment

Uh oh!

benalleng May 6, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

spacebear21 left a comment

Choose a reason for hiding this comment

Uh oh!

benalleng commented May 7, 2025

Uh oh!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

DanGould May 7, 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 7, 2025

Choose a reason for hiding this comment

Uh oh!

DanGould May 7, 2025

Choose a reason for hiding this comment

Uh oh!

spacebear21 commented May 7, 2025

Uh oh!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

spacebear21 commented May 7, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

benalleng commented May 6, 2025 •

edited

Loading

coveralls commented May 6, 2025 •

edited

Loading

DanGould May 7, 2025 •

edited

Loading