Cargo mutants version update

[benalleng]

Closes #872, closes #836, closes #771, closes #890

This addressed #778 for now but I still would like to create a test instead of excluding it
I also added the basic test that should address the mutation from #847 (comment)

The exclusions are catogerized as follows.

• timeouts
• mutations that allow code to simply skip or follow a code path that does not actually cause or prevent an error
• expiry time checks I found not worth creating a test for.
• TODO exclusions

[coveralls]

Pull Request Test Coverage Report for Build 16575062311

Details

• 51 of 54 (94.44%) changed or added relevant lines in 4 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.09%) to 85.873%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/core/send/mod.rs	6	7	85.71%
payjoin/src/core/receive/v1/mod.rs	23	25	92.0%

Totals
Change from base Build 16543289714:	0.09%
Covered Lines:	7908
Relevant Lines:	9209

---

💛 - Coveralls

[spacebear21]

I slightly dislike the mutants exclusions by name because they seem vulnerable to regressions (e.g. if the skipped functions get renamed as #884 is likely to do for some of these) or false positives (e.g. what was once a trivial mutation becomes serious after several refactors). Trivial mutations seem like a code smell that the underlying code could be re-written to eliminate ambiguity. I don't have specific suggestions regarding those excluded mutants, but I want to ensure that we're very deliberate about what we choose to exclude.

[spacebear21]

Suggested change

	panic!("Broadcast suitability check should fail due to being below the min fee rate or unexpected error type")
	panic!("Broadcast suitability check with appropriate min_fee_rate should succeed")

[spacebear21]

Actually, I'd probably re-write the whole match statement:

let min_fee_rate = proposal.psbt_fee_rate().expect("Feerate calculation should not fail");
let _ = proposal.clone().check_broadcast_suitability(Some(min_fee_rate), |_| Ok(true)).expect("Broadcast suitability check with appropriate min_fee_rate should succeed");
assert_eq!(proposal.clone().psbt_fee_rate().unwrap(), min_fee_rate);

Re-writing it like that, it's unclear what exactly this assert_eq is testing for? It seems tautological that min_fee_rate == proposal.psbt_fee_rate().unwrap()

[spacebear21]

Probably this match could also be re-written as an expect_err() instead of match/panic.

[spacebear21]

Suggested change

	"Not changing the receiver output amount should always be allowed"
	"Increasing the receiver output amount is always allowed"

[spacebear21]

Some explanatory comments on these new edge cases wouldn't hurt. I'm guessing this is testing the maximum allowed fee contribution based on the input value?

[spacebear21]

This comment looks obsolete now.

[spacebear21]

nit:

Suggested change

	# Async SystemTime comparison
	"replace > with >= in Sender<WithReplyKey>::extract_v2", # checking if the system time is equal to the expiry is difficult to reasonably test
	"replace < with <= in Receiver<Initialized>::apply_unchecked_from_payload", # checking if the system time is equal to the expiry is difficult to reasonably test
	"replace > with >= in Receiver<Initialized>::extract_req", # checking if the system time is equal to the expiry is difficult to reasonably test
	"replace > with >= in extract_err_req", # checking if the system time is equal to the expiry is difficult to reasonably test
	"replace > with >= in Receiver<Initialized>::create_poll_request", # checking if the system time is equal to the expiry is difficult to reasonably test
	"replace > with >= in Sender<WithReplyKey>::create_v2_post_request", # checking if the system time is equal to the expiry is difficult to reasonably test
	# Async SystemTime comparison
	# checking if the system time is equal to the expiry is difficult to reasonably test
	"replace > with >= in Sender<WithReplyKey>::extract_v2",
	"replace < with <= in Receiver<Initialized>::apply_unchecked_from_payload",
	"replace > with >= in Receiver<Initialized>::extract_req",
	"replace > with >= in extract_err_req",
	"replace > with >= in Receiver<Initialized>::create_poll_request",
	"replace > with >= in Sender<WithReplyKey>::create_v2_post_request",

[benalleng]

Tried to to a little further organization

[spacebear21]

We may want to figure out a way to mock SystemTime in tests. I've used freezegun in Python in the past, I wonder if a similar tool exists the Rust ecosystem. We may even be able to mock it with a Trait, per Claude:

use std::time::SystemTime;

trait TimeProvider {
    fn now(&self) -> SystemTime;
}

struct RealTimeProvider;

impl TimeProvider for RealTimeProvider {
    fn now(&self) -> SystemTime {
        SystemTime::now()
    }
}

struct MockTimeProvider {
    time: SystemTime,
}

impl TimeProvider for MockTimeProvider {
    fn now(&self) -> SystemTime {
        self.time
    }
}

// Your code uses the trait
fn process_with_timestamp<T: TimeProvider>(time_provider: &T) -> SystemTime {
    time_provider.now()
}

#[cfg(test)]
mod tests {
    use super::*;
    use std::time::{Duration, UNIX_EPOCH};

    #[test]
    fn test_with_mock_time() {
        let mock_time = UNIX_EPOCH + Duration::from_secs(1000);
        let provider = MockTimeProvider { time: mock_time };
        
        let result = process_with_timestamp(&provider);
        assert_eq!(result, mock_time);
    }
}

This seems reasonable to leave as a follow-up, maybe to be addressed alongside #893.

[benalleng]

Ahh interesting, I got hung up on trying to fit the tokio::test pause() method in but it just wasn't working.

[benalleng]

Thanks! as for those trivial mutants a good can of axe body spray by just making the code blocks follow the logic path in the inverted operand cases.

With the named exclusions I think we might want to consider using diff mutations https://github.com/sourcefrog/cargo-mutants/blob/1481918f377a2e0a86a66185cb536dd292157438/.github/workflows/tests.yml#L234-L277 perhaps we're not quite ready for it in our codebase but it would be nice to keep the mutant coverage more maintainable

[spacebear21]

utACK 7a21ab7

It would be great to address the exclusions in later follow-ups and remove them from the exclusion list, but these seem reasonable enough for now, and merging this PR will reduce the noise from automated mutants issues.