Session persister mutants by benalleng · Pull Request #791 · payjoin/rust-payjoin

benalleng · 2025-06-23T18:35:12Z

Closes #786

@0xBEEFCAF3 curious if you have any initial thoughts on catching all of the MaybeFatalTransition::transient(e) mutants. Currently all of those match arms are untested at each stage of the receiver.

coveralls · 2025-06-23T18:39:25Z

Pull Request Test Coverage Report for Build 16004747729

Details

146 of 163 (89.57%) changed or added relevant lines in 3 files are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.4%) to 85.897%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/core/receive/v2/session.rs	47	48	97.92%
payjoin/src/core/persist.rs	0	4	0.0%
payjoin/src/core/receive/v2/mod.rs	99	111	89.19%

Files with Coverage Reduction	New Missed Lines	%
payjoin/src/core/persist.rs	2	96.08%

Totals
Change from base Build 15983582431:	0.4%
Covered Lines:	7388
Relevant Lines:	8601

💛 - Coveralls

arminsabouri · 2025-06-23T19:34:59Z

@0xBEEFCAF3 curious if you have any initial thoughts on catching all of the MaybeFatalTransition::transient(e) mutants. Currently all of those match arms are untested at each stage of the receiver.

Are any of the fatal ones being caught?

benalleng · 2025-06-23T23:44:35Z

Yes, all of the mutants are the transient match arms, the fatal arms are all caught.

payjoin/src/receive/v2/mod.rs:289:21: delete match arm
payjoin/src/receive/v2/mod.rs:468:21: delete match arm
payjoin/src/receive/v2/mod.rs:527:17: delete match arm
payjoin/src/receive/v2/mod.rs:573:17: delete match arm
payjoin/src/receive/v2/mod.rs:618:17: delete match arm

Not sure if these are worthy of skipping as to the purpose of these transient errors.
As an aside i think its more likely that the fatal match arms are marked as unviable as opposed to all of them being explicitly caught as the fatal arms are all the catch-all arm. Removing _ => would just cause a linter to catch it.

arminsabouri · 2025-06-24T12:58:22Z

Not sure if these are worthy of skipping as to the purpose of these transient errors.

I agree. The transient errors should be caught and preferably in unit tests. Ideally we unit test each path a state transition method could take. As a first step we may want to develop some test tooling first for this first. i.e get_statex_from_test_vector(). Currently the way get into a particular state is by progressing through the entire state machine

benalleng · 2025-06-25T17:17:25Z

Ok, I have created a test for all levels of the state machine that have these transient errors, except for WithContext.
Since that is just taking in a response body and an ohttp::ClientResponse in process_response() what do you think the best approach is for getting that test in?

arminsabouri · 2025-06-26T14:55:16Z

Ok, I have created a test for all levels of the state machine that have these transient errors, except for WithContext. Since that is just taking in a response body and an ohttp::ClientResponse in process_response() what do you think the best approach is for getting that test in?

We talked about this in discord. The We realized that the withContext state transition should never throw a implementation error and that transient error branch can be removed. This is addressed here

benalleng · 2025-06-26T15:28:05Z

Added this note to the commit message so it will be in the history.

arminsabouri

Approach ACK. Just had a couple nits and I think we should move the unit tests added in persist.rs to the v2::receiver. Thanks for picking this up!

payjoin/src/persist.rs

payjoin/src/receive/v2/mod.rs

arminsabouri · 2025-06-26T19:08:56Z

payjoin/src/send/v2/session.rs


        let v2_get_context = V2GetContext {
-            endpoint,
+            endpoint: endpoint.clone(),


I dont see other changes that would warrant this clone? Do you remenber why we needed this?

This was a holdout from something I reverted, I had some tests before you added some tests in #805 it can be removed

payjoin/src/receive/v2/session.rs

arminsabouri · 2025-06-26T19:14:48Z

payjoin/src/receive/v2/session.rs

+        let session_context = SHARED_CONTEXT.clone();
+        let events = vec![SessionEvent::Created(session_context.clone())];
+
+        let uri = SessionHistory { events }.pj_uri().expect("Could not get session history uri");


personal nit: with expects I think its idomatic to explain why we expect a certain condition to be true rather than an error message. i.e expect("session creation event is present")

benalleng · 2025-07-01T13:11:25Z

I added a commit to make all of the relevant fields public, do you think that is ok or should they still be limited to the crate level?

arminsabouri · 2025-07-01T13:18:31Z

I added a commit to make all of the relevant fields public, do you think that is ok or should they still be limited to the crate level?

If they can be, they should be pub(crate)'d

A few new mutants were introduced with the push updating our session persistence. These catch the mutants by adding some new test coverage along that new code path.

The WithContext state transition should never throw an implementation error so that transient error branch should not exist and can be safely removed. This also obviously addresses the final missing mutant here as this code is not required.

arminsabouri

utACK 9bd4193
Just had a small nit. This is good to go as is

arminsabouri · 2025-07-07T13:26:37Z

payjoin/src/core/receive/v2/mod.rs

+        let receiver = Receiver {
+            state: UncheckedProposal {
+                v1: crate::receive::v1::test::unchecked_proposal_from_test_vector(),
+                context: context.clone(),
+            },
+        };


Nit: can we use the helper method defined above? (unchecked_proposal_v2_from_test_vector)

No, the expiry time now was changed in the context above. so we need to build the receiver here.

arminsabouri · 2025-07-07T13:48:48Z

payjoin/src/core/receive/v2/mod.rs

-                        ),
-                };
-            }
+            Err(e) =>


Just noting this so I don't forget. Removing transient errors from this state transition does make sense right now given that we are implicitly treating all the errors as fatal. However, there are certainly transient errors here that could be resolved based on some external events. e.g changing the ohttp relays, waiting for the dir to be operational again, etc...

benalleng force-pushed the mutants-06-23-25 branch 4 times, most recently from 8cc726e to 9539e4b Compare June 25, 2025 17:15

benalleng force-pushed the mutants-06-23-25 branch 2 times, most recently from e7031ec to 391e057 Compare June 26, 2025 14:35

benalleng force-pushed the mutants-06-23-25 branch from 391e057 to 284873c Compare June 26, 2025 15:24

benalleng changed the title ~~[WIP] session persister mutants~~ Session persister mutants Jun 26, 2025

benalleng marked this pull request as ready for review June 26, 2025 15:29

benalleng requested a review from arminsabouri June 26, 2025 15:29

benalleng force-pushed the mutants-06-23-25 branch from 284873c to 91bf47e Compare June 26, 2025 18:06

arminsabouri requested changes Jun 26, 2025

View reviewed changes

benalleng force-pushed the mutants-06-23-25 branch 4 times, most recently from 2b6a662 to 24faaa3 Compare July 1, 2025 13:04

benalleng force-pushed the mutants-06-23-25 branch from 24faaa3 to 7d273da Compare July 1, 2025 13:57

benalleng added 2 commits July 1, 2025 12:18

Catch session persister mutants

38fb1d6

A few new mutants were introduced with the push updating our session persistence. These catch the mutants by adding some new test coverage along that new code path.

benalleng force-pushed the mutants-06-23-25 branch from 7d273da to 9bd4193 Compare July 1, 2025 16:18

benalleng requested a review from arminsabouri July 3, 2025 18:11

arminsabouri approved these changes Jul 7, 2025

View reviewed changes

arminsabouri merged commit 22e59cc into payjoin:master Jul 7, 2025
7 checks passed

Conversation

benalleng commented Jun 23, 2025

Uh oh!

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

Uh oh!

Pull Request Test Coverage Report for Build 16004747729

Details

💛 - Coveralls

Uh oh!

arminsabouri commented Jun 23, 2025

Uh oh!

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

Uh oh!

Uh oh!

arminsabouri commented Jun 24, 2025

Uh oh!

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

Uh oh!

Uh oh!

arminsabouri commented Jun 26, 2025

Uh oh!

benalleng commented Jun 26, 2025

Uh oh!

arminsabouri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arminsabouri Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

benalleng Jun 27, 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!

Uh oh!

Uh oh!

arminsabouri Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

benalleng commented Jul 1, 2025

Uh oh!

arminsabouri commented Jul 1, 2025

Uh oh!

arminsabouri left a comment

Choose a reason for hiding this comment

Uh oh!

arminsabouri Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

benalleng Jul 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!

arminsabouri Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented Jun 23, 2025 •

edited

Loading

benalleng commented Jun 23, 2025 •

edited

Loading

benalleng commented Jun 25, 2025 •

edited

Loading

benalleng Jun 27, 2025 •

edited

Loading

benalleng Jul 7, 2025 •

edited

Loading