Add more complete async test coverage #1325

Activity

1. Jolah1 commented on Jul 27, 2026

   

   Jolah1

   on Jul 27, 2026

   Contributor

   More actions

   I'd like to pick this up, but flagging first that the gap has narrowed since this was opened — a fair bit of async load/close coverage has
   landed in the meantime:

   • Async load() is exercised by the replay_event_log_async tests (receive/v2/session.rs and send/v2/session.rs).
   • Async close() on replay failure is covered — both crates assert is_closed after a failed async replay (e.g. receive/v2/session.rs
     ~L668–683).
   • Async close() via a terminal transition is covered too — the terminate case in persist.rs::test_maybe_terminal_transition
     (is_closed: true) runs through save_async via the
     run_test_cases! macro.

   What does still look genuinely uncovered is the error paths. Both replay_event_log and replay_event_log_async map a failing load() or close() into InternalReplayError::PersistenceFailure, but the only persisters in the tree (InMemoryPersister / InMemoryAsyncPersister) use type InternalStorageError = Infallible, so they can never fail. those PersistenceFailure branches are never reached in any test (it appears only at the four map_err sites, never in an assertion).

   Proposed scope: add a fault-injecting persister (a real, non-Infallible storage error) that can error on load() and on close(), and assert that replay_event_log / replay_event_log_async surface PersistenceFailure for both branches, in both the receive and send
   crates.

   Happy to do this under #1325 or if you'd rather keep #1325 as-is and track the error-path coverage separately, I can open a focused follow-up issue instead. Which do you prefer?
2. xstoicunicornx commented on Sep 2, 2026

   

   xstoicunicornx

   on Sep 2, 2026

   Collaborator

   More actions

   @benalleng - can you confirm this stems from discussion around lack of FFI test coverage (and not the main payjoin crate)? Seems like all bindings now have save_async test coverage.

   As for .load() and .close() test coverage, wouldn't this depend on the implementation used? Do you think its necessary to add test coverage to the provided test utils persister (which is the only persister we provide)? I think I lean towards no but maybe I'm thinking about this wrong.
3. benalleng commented on Sep 8, 2026

   

   benalleng

   on Sep 8, 2026

   CollaboratorAuthor

   More actions

   I think we should limit what the goal is for this issue. I pointed out that there was a lack of load() and close() coverage though to get that coverage we should try to use existing methods and implementations and methods rather than trying to make a new dummy implementation. In practice this means looking to payjoin-cli and creating an integration test there to add coverage.

   👍React with 👍1Reacted by spacebear