Uh oh!
There was an error while loading. Please reload this page.
test(dash-spv): add multi-wallet integration tests - #697
Conversation
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR introduces comprehensive multi-wallet SPV synchronization test coverage for the Dash SPV library. It adds shared test constants and helper functions, registers a new test module, refactors existing tests to use centralized test data, and implements four end-to-end test scenarios exploring runtime wallet additions, shared blocks, concurrent initialization, and rescan behavior during tip advancement. ChangesMulti-Wallet SPV Sync Test Coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## v0.42-dev #697 +/- ##
==========================================
Coverage 71.17% 71.17% ==========================================
Files 320 320 Lines 68602 68602 ==========================================
+ Hits 48827 48830 +3 + Misses 19775 19772 -3
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
bbf4de5 to
6514b12Compare6514b12 to
34c27d9Compare34c27d9 to
943eec3CompareThere was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dash-spv/tests/dashd_sync/tests_multi_wallet.rs`:
- Around line 50-51: Add the #[ignore] attribute to network-dependent
integration tests so they don't run by default: place #[ignore] immediately
above each #[tokio::test] declaration in this file (including the test function
test_sync_two_wallets_same_client and the other four network-dependent test
cases mentioned) so they are excluded from regular runs and can be executed
explicitly with -- --ignored in the dedicated integration job.
In `@dash-spv/tests/dashd_sync/tests_restart.rs`:
- Around line 212-213: Add the #[ignore] attribute to the test function
declaration for test_runtime_add_after_restart so the test is not run by
default; specifically, place #[ignore] above the existing #[tokio::test] on the
test_runtime_add_after_restart function so it will only run when invoked with --
--ignored.
- Around line 247-263: Mark the network-dependent test (tests_restart.rs) with
#[ignore] and change the sync-event draining/watch logic to explicitly handle
broadcast lag: when draining client_handle.sync_event_receiver use try_recv()
but detect and handle RecvError::Lagged (don't silently ignore it), and in the
watch loop match recv() errors so that Err(RecvError::Lagged) is handled
separately (e.g., fail or increment a spurious counter) rather than treating all
recv errors the same as a timeout/break; reference the
SyncEvent::FiltersSyncComplete variant, client_handle.sync_event_receiver,
try_recv, recv, and RecvError::Lagged, and consider starting the receiver
observation before returning from wait_for_sync()/the function that spawns the
upstream rescan to avoid missing early events.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b4412e3d-697d-4715-918c-f636c88383a5
📒 Files selected for processing (5)
dash-spv/tests/dashd_sync/helpers.rsdash-spv/tests/dashd_sync/main.rsdash-spv/tests/dashd_sync/tests_basic.rsdash-spv/tests/dashd_sync/tests_multi_wallet.rsdash-spv/tests/dashd_sync/tests_restart.rs
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
943eec3 to
348c570CompareThere was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dash-spv/tests/dashd_sync/helpers.rs`:
- Around line 116-118: The match arm that treats Ok(Err(_)) as a hard panic is
incorrect for Tokio broadcast::RecvError::Lagged; update the receiver logic (the
match handling the wallet event channel recv where `target` and
`wallet.synced_height` are used) to detect RecvError::Lagged specifically, and
when lagged: re-read the wallet's current `synced_height` (in case the target
was missed) and continue the polling loop instead of panicking; only panic for
terminal errors (e.g., RecvError::Closed or other non-lagged errors).
- Around line 116-118: The match arm that treats Ok(Err(_)) from
Receiver::recv() as a panic should distinguish RecvError::Lagged from
RecvError::Closed: in the block handling the receiver result (where you
currently have Ok(Err(_)) => panic!(...)), match the error enum explicitly and
on RecvError::Lagged(_) simply continue (or log and continue) because it is
recoverable, while still panicking (or failing the test) only on
RecvError::Closed; reference tokio::sync::broadcast::Receiver::recv(), the
RecvError::Lagged variant, and the existing Ok(Err(_)) panic site in helpers.rs
to locate and update the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7558f81c-62da-4a2d-b2a1-19cf8d0d7fbd
📒 Files selected for processing (1)
dash-spv/tests/dashd_sync/helpers.rs
Uh oh!
There was an error while loading. Please reload this page.
a6d0b45 to
2238aecCompareAdds `tests_multi_wallet.rs` plus extensions to `tests_basic.rs` and `tests_restart.rs` covering per-wallet sync behavior: - Two wallets in one `WalletManager` sync independently. - Runtime wallet-add with `birth_height` before, at, and beyond tip, asserting existing wallets' `synced_height` and `last_processed_height` never regress. - Deep historical rescan triggered by a `birth_height = 0` runtime add. - A block funding two wallets is attributed correctly to each after both are added at runtime. - Runtime add against a `Syncing`-state `FiltersManager`. - Runtime-add rescan composed with a live tip advance. - Restart of a fully-synced client emits no rescan, then runtime-adding a second wallet only catches up the new wallet.
2238aec to
7090009Compare
Adds
tests_multi_wallet.rsplus extensions totests_basic.rsandtests_restart.rscovering per-wallet sync behavior:WalletManagersync independently.birth_heightbefore, at, and beyond tip, asserting existing wallets'synced_heightandlast_processed_heightnever regress.birth_height = 0runtime add.Syncing-stateFiltersManager.Based on:
birth_heightinManagedWalletInfoctors #692birth_height#693Summary by CodeRabbit