Skip to content

chore: remove flaky handshake_test.rs integration tests#574

Open
xdustinface wants to merge 1 commit intov0.42-devfrom
chore/flaky-handshake-timeout-test
Open

chore: remove flaky handshake_test.rs integration tests#574
xdustinface wants to merge 1 commit intov0.42-devfrom
chore/flaky-handshake-timeout-test

Conversation

@xdustinface
Copy link
Collaborator

@xdustinface xdustinface commented Mar 23, 2026

All three tests in this file were effectively no-ops in CI:

  • test_handshake_timeout used timing assertions on a connection to an RFC 5737 address. The lower bound (>= 1s) fails when the OS returns an immediate ICMP unreachable and the upper bound (< 5s) can fail under CI load. The remaining logic just tests tokio's timeout wrapper, not project code.
  • test_handshake_with_mainnet_peer and test_multiple_connect_disconnect_cycles silently pass when no Dash Core node is running at 127.0.0.1:9999.

Summary by CodeRabbit

  • Tests

    • Reorganized integration test structure for peer connection testing.
  • Documentation

    • Updated test documentation to reflect current test organization and command examples.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 23, 2026

📝 Walkthrough

Walkthrough

The pull request removes the handshake_test.rs integration test file and updates associated documentation to reference peer_test instead. Test plan documentation is simplified to remove references to the deleted test suite.

Changes

Cohort / File(s) Summary
Test File Removal
dash-spv/tests/handshake_test.rs
Entire integration test suite deleted, including tests for mainnet peer handshake, timeout handling, network manager lifecycle, and connect/disconnect cycles (112 lines).
Documentation Updates
dash-spv/CLAUDE.md, dash-spv/tests/test_plan.md
Updated Rust test invocation examples from handshake_test to peer_test and replaced test_handshake_with_mainnet_peer with test_peer_connection. Simplified test plan documentation by removing detailed progress tracking for the removed test file.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 Hop, hop—old tests take their final bow,
To peer_test we migrate now,
Handshakes fade, documentation's bright,
Consolidation feels just right!

🚥 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 accurately identifies the main change: removing flaky integration tests from handshake_test.rs. It is specific, concise, and clearly summarizes the primary changeset.
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 chore/flaky-handshake-timeout-test

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

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

All three tests in this file were effectively no-ops in CI:

- `test_handshake_timeout` used timing assertions on a connection to
  an RFC 5737 address. The lower bound (`>= 1s`) fails when the OS
  returns an immediate ICMP unreachable and the upper bound (`< 5s`)
  can fail under CI load. The remaining logic just tests tokio's
  `timeout` wrapper, not project code.
- `test_handshake_with_mainnet_peer` and
  `test_multiple_connect_disconnect_cycles` silently pass when no
  Dash Core node is running at 127.0.0.1:9999.
@xdustinface xdustinface force-pushed the chore/flaky-handshake-timeout-test branch from 73a0745 to 3d2a896 Compare March 23, 2026 03:19
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
dash-spv/CLAUDE.md (1)

127-132: ⚠️ Potential issue | 🟡 Minor

Update stale test-category summary to remove handshake/count claims.

The section still says “Network layer: Handshake ... (3/4 passing),” which is now outdated after removing handshake_test.rs. Please sync this summary with the current test plan state.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@dash-spv/CLAUDE.md` around lines 127 - 132, Update the stale "Test Categories
(from `tests/test_plan.md`)" summary: remove the obsolete "Handshake" mention
and the hard-coded "(3/4 passing)" claim from the "Network layer: Handshake,
connection management (3/4 passing)" line in CLAUDE.md, and replace it with an
accurate, current phrase (e.g., "Network layer: connection management") and an
updated pass/fail count that matches the live tests by syncing with
tests/test_plan.md; ensure the "Test Categories" section text reflects the
current test_plan.md contents.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@dash-spv/CLAUDE.md`:
- Around line 127-132: Update the stale "Test Categories (from
`tests/test_plan.md`)" summary: remove the obsolete "Handshake" mention and the
hard-coded "(3/4 passing)" claim from the "Network layer: Handshake, connection
management (3/4 passing)" line in CLAUDE.md, and replace it with an accurate,
current phrase (e.g., "Network layer: connection management") and an updated
pass/fail count that matches the live tests by syncing with tests/test_plan.md;
ensure the "Test Categories" section text reflects the current test_plan.md
contents.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 48dd95da-5dd1-44d8-a6d0-e1d696b353ff

📥 Commits

Reviewing files that changed from the base of the PR and between 19fb152 and 3d2a896.

📒 Files selected for processing (3)
  • dash-spv/CLAUDE.md
  • dash-spv/tests/handshake_test.rs
  • dash-spv/tests/test_plan.md
💤 Files with no reviewable changes (1)
  • dash-spv/tests/handshake_test.rs

@codecov
Copy link

codecov bot commented Mar 23, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.30%. Comparing base (19fb152) to head (3d2a896).

Additional details and impacted files
@@              Coverage Diff              @@
##           v0.42-dev     #574      +/-   ##
=============================================
- Coverage      66.32%   66.30%   -0.03%     
=============================================
  Files            311      311              
  Lines          64976    64976              
=============================================
- Hits           43097    43083      -14     
- Misses         21879    21893      +14     
Flag Coverage Δ
core 75.13% <ø> (ø)
ffi 36.82% <ø> (+0.02%) ⬆️
rpc 28.28% <ø> (ø)
spv 80.93% <ø> (-0.14%) ⬇️
wallet 66.27% <ø> (ø)
see 5 files with indirect coverage changes

@github-actions github-actions bot added the ready-for-review CodeRabbit has approved this PR label Mar 23, 2026
@xdustinface xdustinface requested a review from ZocoLini March 23, 2026 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-review CodeRabbit has approved this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant