Integration Test Framework Improvement

[xyephy]

Context

After reviewing the integration test framework in sv2-apps, specifically sniffer.rs, message_aggregator.rs, mock_roles.rs, and the test files, I discovered several structural issues that affect test reliability. These issues can cause false positives (tests pass when code is broken), false negatives (tests fail when code is correct), and slow test execution.

Note: The framework relies on time-based synchronization (fixed sleeps, polling intervals) instead of event-based synchronization (ready signals, notifications). This makes tests fragile, slow, and non-deterministic.

---

Issues Discovered

Sniffer Returns Before Ready

The start() function in sniffer.rs:81-113 spawns an async task and returns immediately, before the sniffer is actually listening for connections.

// sniffer.rs:81-113
pub fn start(&self) {
    tokio::spawn(async move {
        // ... setup connections ...
    });
    // Returns IMMEDIATELY - sniffer may not be listening yet
}

Tests compensate with arbitrary sleeps, as seen in mod.rs:152:

// mod.rs:152
sniffer.start();
tokio::time::sleep(Duration::from_secs(1)).await;  // Hope 1 second is enough

If the sniffer takes longer than the sleep to bind its port, downstream connections fail and tests crash.

---

Instant Negative Assertions

The assert_message_not_present() function in sniffer.rs:205-219 checks the queue at a single point in time and returns immediately.

// sniffer.rs:205-219
pub async fn assert_message_not_present(&self, direction, message_type) -> bool {
    let has_message_type = /* check queue RIGHT NOW */;
    !has_message_type  // Returns instantly
}

Tests use this with a preceding sleep, as seen in pool_integration.rs:314:

// pool_integration.rs:314
tokio::time::sleep(Duration::from_secs(1)).await;
assert!(sniffer.assert_message_not_present(...));

If the message arrives at 1.001 seconds, the test passes but the code is broken. The assertion checks a snapshot in time, not a guarantee that the message will never arrive.

---

Coarse Polling Interval

Message polling in sniffer.rs:198 uses a 1-second interval, which significantly slows down test execution.

// sniffer.rs:198
loop {
    if has_message_type { return; }
    tokio::time::sleep(Duration::from_secs(1)).await;  // 1 SECOND
}

A message arriving at 10ms is not detected until 1000ms. A test waiting for 5 messages takes a minimum of 5 seconds regardless of how fast the system responds.

The same 1-second polling exists in sv1_sniffer.rs:118 and sv1_sniffer.rs:154.

---

Weak Assertions

Current assertions in test files like pool_integration.rs only verify message type, not message content.

// pool_integration.rs
sniffer.wait_for_message_type(
    MessageDirection::ToDownstream,
    MESSAGE_TYPE_SETUP_CONNECTION_SUCCESS
);
assert_common_message!(&sniffer.next_message_from_upstream(), SetupConnectionSuccess);

This does not verify that used_version, flags, or any other fields contain correct values. A bug could send SetupConnectionSuccess { used_version: 0, flags: 0 } and tests would still pass.

---

Arbitrary Sleep Values

Hardcoded sleep durations are scattered throughout the codebase with no relation to actual timing requirements.

File	Duration	Purpose
template_provider.rs:346	3 sec	Wait for sv2-tp
template_provider.rs:180	2 sec	Wait for Bitcoin Core
mod.rs:152	1 sec	After sniffer start
pool_integration.rs:314	1 sec	Before negative assertion
pool_integration.rs:341	5 sec	Wait for job declaration
jd_integration.rs:44	2 sec	Wait for setup
jdc_block_propagation.rs:42	1 sec	Wait for block propagation

This is not exhaustive. These values work on fast machines but can cause flaky failures on slower CI systems.

---

Disabled Message Verification

Most tests disable check_on_drop, which would otherwise verify that all captured messages were consumed.

// Common pattern across test files
let (sniffer, _) = start_sniffer("test", addr, false, vec![], None);
//                                       ^^^^^ check_on_drop = false

Unconsumed messages could indicate unexpected protocol behavior or missing test assertions.

---

Proposed Improvements

The solution is to shift from time-based to event-based synchronization wherever possible.

Issue	Improvement
Sniffer returns before ready	Return a oneshot::Receiver that signals when the sniffer is listening
Instant negative assertions	Continuously check throughout the timeout duration before declaring absent
Coarse polling interval	Reduce polling from 1 second to 100ms
Weak assertions	Add field-level assertions for critical message values
Arbitrary sleep values	Replace fixed sleeps with ready signals and event notifications
Disabled message verification	Enable check_on_drop by default

These are incremental fixes to the existing codebase, not a rewrite.

Note: Some work has already started. PR #156 added sleep to connection retry loops in Sniffer, MockDownstream, and SnifferSV1. PR #191 adds implicit SetupConnection handling to MockDownstream and MockUpstream. These can serve as a foundation for the remaining improvements.

---

Discussion

• Are there additional issues that I missed that are worth addressing?
• What should be the priority order for these improvements?
• Should we establish guidelines for new tests (e.g., no arbitrary sleeps, field-level assertions required)?

[jbesraa]

It’s great to see the effort to improve the integration tests — that’s definitely appreciated.

Sniffer Returns Before Ready

There’s a bit of a chicken-and-egg situation here. start_sniffer returns the sniffer’s listening_address, which downstream role need in order to connect. Only once that happens does wait_for_client actually proceed.

Because of that, I’m not sure the framing of “before the sniffer is actually listening for connections” is entirely accurate. That said, it is true that the function returns before the sniffer is fully connected to both upstream and downstream.

One possible approach(what you are suggesting) could be to return the receiver side of a channel and have the test wait on it, which would allow us to remove the sleep. That might be a bit overengineered for testing purposes, though. If we do go in that direction, I think it should remain an internal mechanism rather than something each test has to handle explicitly.

Weak Assertions

I understand the concern here, but asserting every property of every message could make the tests very verbose and somewhat painful to write and maintain. For example, if a test is specifically about validating the RequestTransactionData flow, it’s not clear that asserting all prior messages adds much value. Focusing assertions on what the test is actually exercising might strike a better balance.