fix(test): avoid DNS discovery hang in SPV lifecycle tests

[lklimek]

Summary

• Set use_local_node(true) in the SPV test helper create_test_manager() so all lifecycle tests use the explicit localhost peer address (127.0.0.1:19999) instead of DNS seed discovery
• Previously, tests with no explicit peers triggered DNS seed discovery in the upstream dash-spv library, which hangs because DashSpvClient::run() doesn't check the cancellation token during its startup phase (upstream bug: dashpay/rust-dashcore#577)
• test_start_stop_clean_shutdown now completes in ~0.3s instead of timing out at 10s

User story

Imagine you are a developer running cargo test on the project. Previously, test_start_stop_clean_shutdown would hang for 10 seconds and then fail — every time. Now all 20 SPV tests pass in ~1.4 seconds.

Test plan

• cargo test --all-features spv::tests:: — all 20 tests pass (1.37s)
• test_start_stop_clean_shutdown — previously timed out, now passes in ~0.3s
• test_use_local_node_toggle — updated assertions match new default from test helper

_{🤖 Co-authored by Claudius the Magnificent AI Agent}

Summary by CodeRabbit

• Tests
  • Updated test infrastructure to establish consistent default behavior for local peer handling in test scenarios, ensuring reliable test execution and clearer assertion expectations.

[coderabbitai]

📝 Walkthrough

Walkthrough

Updated test helpers and assertions in the SPV test module to establish local node usage as the default behavior for test instances, changing peer-discovery initialization and aligning dependent test expectations accordingly.

Changes

Cohort / File(s)	Summary
Test Configuration and Behavior src/spv/tests.rs	Modified create_test_manager() to explicitly enable local node mode by default via set_use_local_node(true). Updated test_use_local_node_toggle() assertions to reflect the new default, now verifying initial state as true and testing toggle behavior to false.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 A local node now blooms by default, so true,
Our test helpers dance with behaviors anew,
No more DNS seeds for the test rabbit's crew,
Toggles flip swift, from true down to blue! 🔘✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly and clearly describes the main change: avoiding DNS discovery hangs in SPV lifecycle tests by setting use_local_node(true) in the test helper.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/spv-test-dns-hang

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Copilot]

Pull request overview

This PR updates the SPV lifecycle test harness to avoid DNS seed discovery during tests (which can hang due to an upstream startup cancellation issue), by forcing tests to use an explicit localhost peer instead.

Changes:

• Set use_local_node(true) in create_test_manager() so SPV tests don’t trigger DNS seed discovery.
• Update test_use_local_node_toggle assertions to reflect the helper’s new default.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

🧹 Nitpick comments (1)

src/spv/tests.rs (1)

156-161: Consider adding a test for the default use_local_node value.

The updated assertions correctly reflect the helper's new behavior. However, this test no longer verifies that SpvManager::new() initializes use_local_node to false by default — that behavior is now masked by the helper.

Since the live testnet test (which creates SpvManager directly) is marked #[ignore], you may want to add a small test that constructs SpvManager without the helper and asserts the default is false, ensuring the default initialization remains correct.

💡 Optional: Add default value test

/// Given a freshly constructed SpvManager (not via helper),
/// Then use_local_node defaults to false.
#[tokio::test(flavor = "multi_thread", worker_threads = 4)]
async fn test_use_local_node_default_is_false() {
    let config = Arc::new(RwLock::new(test_network_config()));
    let task_manager = Arc::new(TaskManager::new());
    let tmp_dir = tempfile::TempDir::new().expect("Failed to create temp dir");
    let manager = SpvManager::new(
        tmp_dir.path(),
        Network::Testnet,
        config,
        task_manager,
    )
    .expect("SpvManager::new should succeed");

    assert!(
        !manager.use_local_node(),
        "Default use_local_node should be false"
    );
}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/spv/tests.rs` around lines 156 - 161, The test currently exercises
use_local_node via the test helper which masks SpvManager's default; add a
focused unit test that constructs SpvManager directly (calling SpvManager::new
with a temp dir, Network::Testnet, test_network_config wrapped in Arc<RwLock>,
and a TaskManager) and then asserts that manager.use_local_node() is false to
ensure the default initialization remains false; locate the new test near
existing tests in src/spv/tests.rs and name it clearly (e.g.,
test_use_local_node_default_is_false).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/spv/tests.rs`:
- Around line 156-161: The test currently exercises use_local_node via the test
helper which masks SpvManager's default; add a focused unit test that constructs
SpvManager directly (calling SpvManager::new with a temp dir, Network::Testnet,
test_network_config wrapped in Arc<RwLock>, and a TaskManager) and then asserts
that manager.use_local_node() is false to ensure the default initialization
remains false; locate the new test near existing tests in src/spv/tests.rs and
name it clearly (e.g., test_use_local_node_default_is_false).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: efd55dda-4985-45e7-942f-0d7ef66509ed

📥 Commits

Reviewing files that changed from the base of the PR and between 6752f2c and 92475f1.

📒 Files selected for processing (1)

• src/spv/tests.rs