Skip to content

feat(persist_test_utils)!: Add async suite to test AsyncWalletPersister - #343

Closed
ValuedMammal wants to merge 2 commits into
bitcoindevkit:masterfrom
ValuedMammal:feat/persist_test_utils_async
Closed

feat(persist_test_utils)!: Add async suite to test AsyncWalletPersister#343
ValuedMammal wants to merge 2 commits into
bitcoindevkit:masterfrom
ValuedMammal:feat/persist_test_utils_async

Conversation

@ValuedMammal

@ValuedMammal ValuedMammal commented Nov 7, 2025

Copy link
Copy Markdown
Collaborator

Description

Refactors the persist_test_utils module and extends it with an async test suite for AsyncWalletPersister.

903bd84: Refactor WalletPersister suite + update deps

The public test functions previously took a filename: &str and a path-based closure, managed tempfile::tempdir() internally, and panicked on failure. This redesigns the API so callers supply a FnOnce() -> Result<P, P::Error> closure that owns the store directly, and all functions return Result<(), PersistError> instead of panicking. A new PersistError enum exposes failures as structured values with ChangeSetMismatch { got, expected } and Persister(Box<dyn core::error::Error>) variants. persist_single_keychain is removed, and its coverage is absorbed into persist_keychains, which now runs two rounds: external descriptor only (the single-keychain case), then external plus change descriptor with merge verification. anyhow and tempfile are removed from the test-utils feature's optional dependencies and kept only as dev-dependencies.

6dc8fe5: Add async suite to test AsyncWalletPersister

Adds async counterparts to every public function in the WalletPersister test suite so implementors of AsyncWalletPersister can exercise the same scenarios: persist_wallet_changeset_async, persist_keychains_async, and persist_network_async. Two private helpers, init_async_wallet_persister and persist_changeset_verify_async, mirror the sync suite API. All entry points use the F: AsyncFnOnce() -> Result<P, P::Error> bound, consistent with the sync API pattern.

Potential follow-ups:

  • Add async counterpart for persist_multiple_wallet_changesets

Changelog notice

Added

  • persist_wallet_changeset_async, persist_keychains_async, persist_network_async to persist_test_utils for testing AsyncWalletPersister implementations

Changed

  • Refactored persist_test_utils public API: callers own store construction; functions now return Result<(), PersistError> instead of panicking
  • Added PersistError enum with ChangeSetMismatch and Persister variants
  • persist_keychains now covers the single-keychain case; persist_single_keychain removed
  • Removed optional anyhow and tempfile dependencies from the test-utils feature

Checklists

All Submissions:

  • I ran just p before pushing

New Features:

  • This pull request breaks the existing API
  • I've added docs for the new feature

@codecov

codecov Bot commented Nov 7, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.85%. Comparing base (e6e3ddf) to head (ae2030c).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #343      +/-   ##
==========================================
+ Coverage   81.79%   81.85%   +0.06%     
==========================================
  Files          25       25              
  Lines        6482     6482              
  Branches      296      296              
==========================================
+ Hits         5302     5306       +4     
+ Misses       1081     1078       -3     
+ Partials       99       98       -1     
Flag Coverage Δ
rust 81.85% <ø> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ValuedMammal
ValuedMammal force-pushed the feat/persist_test_utils_async branch 2 times, most recently from 15149b5 to bf0057f Compare February 7, 2026 05:49
Comment thread src/persist_test_utils.rs
@ValuedMammal
ValuedMammal force-pushed the feat/persist_test_utils_async branch 2 times, most recently from 7e53c18 to 8be4c86 Compare February 26, 2026 04:36
@ValuedMammal
ValuedMammal force-pushed the feat/persist_test_utils_async branch from 8be4c86 to 2c011f1 Compare March 18, 2026 02:31
@ValuedMammal ValuedMammal changed the title wip,draft: Add async support to persist_test_utils Add async support to persist_test_utils Mar 18, 2026
@ValuedMammal
ValuedMammal force-pushed the feat/persist_test_utils_async branch from 2c011f1 to 9c62a96 Compare March 18, 2026 03:01
@ValuedMammal ValuedMammal changed the title Add async support to persist_test_utils feat!: Add async support to persist_test_utils Mar 18, 2026
@ValuedMammal
ValuedMammal force-pushed the feat/persist_test_utils_async branch 2 times, most recently from 940e7c1 to 8059762 Compare March 30, 2026 17:16
@ValuedMammal
ValuedMammal marked this pull request as ready for review March 30, 2026 17:17
@ValuedMammal
ValuedMammal force-pushed the feat/persist_test_utils_async branch from 8059762 to 12e1fcd Compare March 30, 2026 17:26
@ValuedMammal ValuedMammal added this to the Wallet 4.0.0 milestone Mar 30, 2026
@ValuedMammal ValuedMammal self-assigned this Mar 30, 2026
@ValuedMammal ValuedMammal added api A breaking API change tests New or improved tests labels Mar 30, 2026
@ValuedMammal ValuedMammal moved this to In Progress in BDK Wallet Mar 30, 2026
@ValuedMammal
ValuedMammal force-pushed the feat/persist_test_utils_async branch from 12e1fcd to 2d46064 Compare April 13, 2026 13:42
Comment thread tests/persisted_wallet.rs Outdated
@ValuedMammal
ValuedMammal force-pushed the feat/persist_test_utils_async branch from 2d46064 to 0547ea1 Compare April 16, 2026 18:58
@KrishnaShuk

Copy link
Copy Markdown

Concept ACK

The sync side has 5 test utilities but only persist_wallet_changeset got an async equivalent.
persist_network, persist_keychains, persist_single_keychain, and persist_multiple_wallet_changesets are missing async versions.
A backend could pass the full changeset round-trip but fail on partial writes. Is this intentional or planned for follow-up?

@ValuedMammal
ValuedMammal force-pushed the feat/persist_test_utils_async branch from 0547ea1 to 6dc8fe5 Compare May 3, 2026 14:52
@ValuedMammal ValuedMammal changed the title feat!: Add async support to persist_test_utils feat(persist_test_utils)!: Add async suite to test AsyncWalletPersister May 3, 2026
@ValuedMammal ValuedMammal moved this from In Progress to Needs Review in BDK Wallet May 3, 2026
@ValuedMammal

Copy link
Copy Markdown
Collaborator Author

The sync side has 5 test utilities but only persist_wallet_changeset got an async equivalent.
persist_network, persist_keychains, persist_single_keychain, and persist_multiple_wallet_changesets are missing async versions.

Added persist_network_async. persist_keychains_async also covers the single keychain case.

…e deps

Redesign the API so callers supply a `FnOnce` closure that returns
the persister directly. All functions return
`Result<(), PersistError>` instead of panicking. Add `PersistError`
enum with `Persister` and `ChangeSetMismatch` variants.

Factor common logic into internal helpers `init_wallet_persister` and
`persist_changeset_verify` for reuse by each of the public functions.
Remove `persist_single_keychain`. Single-keychain persistence is
covered by `persist_keychains`.

Remove `anyhow` and `tempfile` from the test-utils feature's optional
dependencies.
Add async counterparts to the test suite to exercise implementations
of AsyncWalletPersister. All entry points use the
`F: AsyncFnOnce() -> Result<P, P::Error>` bound, consistent with
the sync API pattern.

`persist_wallet_changeset_async` tests a full changeset round-trip
followed by a second changeset to verify merge semantics.
`persist_keychains_async` runs the two-round descriptor persistence
check. `persist_network_async` verifies the network field round-trips
correctly.

Add private helpers `init_async_wallet_persister` and
`persist_changeset_verify_async` to mirror the sync suite.
@ValuedMammal
ValuedMammal force-pushed the feat/persist_test_utils_async branch from abd0cb9 to ae2030c Compare July 23, 2026 16:28
ValuedMammal added a commit that referenced this pull request Jul 27, 2026
…alletPersister

d138ebb feat(persist_test_utils): Add async suite to test AsyncWalletPersister (Listener)
3d611d4 refactor(persist_test_utils)!: Refactor WalletPersister suite + update deps (Listener)

Pull request description:

  ### Description

  Picks up the work started on #343 by extending the `persist_test_utils` module with an async test suite for `AsyncWalletPersister`.

  The public test functions previously took a `filename: &str` and a path-based closure, managed `tempfile::tempdir()` internally, and panicked on failure. This redesigns the API so callers supply a `FnOnce` closure that owns the store directly, and all functions return `Result<(), PersistError>` instead of panicking. A new `PersistError` enum exposes failures as structured values with `ChangeSetMismatch` and `Persister` variants. `persist_single_keychain` is removed, and its coverage is absorbed into `persist_keychains`, which now runs two rounds: external descriptor only, then external plus change descriptor with merge verification. `anyhow` and `tempfile` are removed from the `test-utils` feature's optional dependencies and kept only as `dev-dependencies`.

  Adds async counterparts to every public function in the `WalletPersister` test suite so implementors of `AsyncWalletPersister` can exercise the same scenarios: `persist_wallet_changeset_async`, `persist_keychains_async`, and `persist_network_async`. Two private helpers, `init_async_wallet_persister` and `check_changest_is_persisted_async`, mirror the sync suite API.

  ### Changelog notice

  Added

  - `persist-test-utils`: Add public functions `persist_wallet_changeset_async`, `persist_keychains_async`, and `persist_network_async` for testing `AsyncWalletPersister`
  - Added `PersistError` enum with `ChangeSetMismatch` and `Persister` variants

  Changed

  - Refactored `persist_test_utils` public API. callers own store construction; functions now return `Result<(), PersistError>`
  - deps: Removed optional `anyhow` and `tempfile` dependencies from the `test-utils` feature

  Removed

  - Removed `persist_single_keychain`; `persist_keychains` now covers the single-keychain case

  ### Before submitting

  - [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk_wallet/blob/master/CONTRIBUTING.md)
  - [x] This PR breaks the existing API

Top commit has no ACKs.

Tree-SHA512: be27a511b856c40fc09ecdea2b4acd28c485dcdf48ff82468c3581bad11a9b2e497f20cfd682ee6c3c21f1efc9a9e7f1df0115fe8ed5d06dade0de040199e9bc
@bladed-operative

Copy link
Copy Markdown
Contributor

Replaced

@ValuedMammal
ValuedMammal deleted the feat/persist_test_utils_async branch July 27, 2026 14:52
@github-project-automation github-project-automation Bot moved this from Needs Review to Done in BDK Wallet Jul 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api A breaking API change tests New or improved tests

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants