tests: rework the actually useful tests to live in the integration crate

crates/irtt-stats/src/ipdv.rs

@@ -185,65 +185,13 @@ mod tests {
         }
     }
 
-    #[test]
-    fn ipdv_tracker_late_wrapped_previous_completes_pair() {
-        let mut tracker = IpdvTracker::new(None);
-        assert!(tracker.insert(ipdv_sample(0, 14)).is_empty());
-
-        let pairs = tracker.insert(ipdv_sample(u32::MAX, 10));
-
-        assert_eq!(
-            pairs,
-            vec![CompletedIpdvPair {
-                previous_seq: u32::MAX,
-                current_seq: 0,
-                rtt_ipdv_ns: 4,
-                send_ipdv_ns: None,
-                receive_ipdv_ns: None,
-            }]
-        );
-    }
-
-    #[test]
-    fn ipdv_tracker_bounded_mode_limits_sequence_state() {
-        let limit = 4;
-        let mut tracker = IpdvTracker::new(Some(limit));
-
-        // Far more inserts than the limit, so eviction runs repeatedly. Every
-        // per-sequence collection must stay bounded by the limit rather than
-        // growing with the number of samples seen, and eviction must not stop
-        // adjacent sequences from still completing a pair.
-        for seq in 0..512_u32 {
-            let pairs = tracker.insert(ipdv_sample(seq, i128::from(seq)));
-
-            assert_eq!(pairs.len(), usize::from(seq > 0), "seq {seq}");
-            assert!(tracker.samples.len() <= limit, "seq {seq}");
-            assert_eq!(
-                tracker.sample_order.len(),
-                tracker.samples.len(),
-                "seq {seq}"
-            );
-            assert!(tracker.completed_pairs.len() <= limit, "seq {seq}");
-        }
-    }
-
     mod properties {
         use super::*;
         use proptest::prelude::*;
 
         proptest! {
-            /// The hand-written `ipdv_tracker_bounded_mode_limits_sequence_state`
-            /// test above only exercises a strictly increasing `0..512`
-            /// sequence. Real traffic can duplicate, skip, or otherwise not
-            /// strictly increase sequence numbers (see the client's
-            /// `LateReply`/`DuplicateReply` handling), so this generates
-            /// arbitrary, possibly-repeated, possibly-non-monotonic sequence
-            /// streams and checks the same bounds still hold after every
-            /// insert: `samples` and `sample_order` never exceed the
-            /// configured limit and stay in lockstep, and `completed_pairs`
-            /// never exceeds `samples.len()` (a successful entry can only
-            /// persist once both its endpoint samples are present, see
-            /// `try_pair`, so it is always a subset of the live sample keys).
+            // Hidden retention state must stay bounded even with duplicates,
+            // gaps and reordered replies; snapshots cannot expose a leak here.
             #[test]
             fn bounded_tracker_state_never_exceeds_the_limit(
                 limit in 1usize..8,
@@ -251,34 +199,26 @@ mod tests {
             ) {
                 let mut tracker = IpdvTracker::new(Some(limit));
                 for seq in seqs {
-                    tracker.insert(ipdv_sample(seq, i128::from(seq)));
+                    let completed_before = tracker.completed_pairs.clone();
+                    let pairs = tracker.insert(ipdv_sample(seq, i128::from(seq)));
+                    let emitted: HashSet<_> = pairs.iter().map(|pair| pair.current_seq).collect();
+                    prop_assert_eq!(emitted.len(), pairs.len());
+                    prop_assert!(emitted.is_disjoint(&completed_before), "a retained pair must not be emitted twice");
                     prop_assert!(tracker.samples.len() <= limit);
+                    prop_assert!(tracker.sample_order.len() <= limit);
+                    prop_assert!(tracker.completed_pairs.len() <= limit);
+                    // Eviction must remove stale pair markers as well as samples;
+                    // otherwise retained state can prevent future adjacent pairs.
                     prop_assert_eq!(tracker.sample_order.len(), tracker.samples.len());
-                    prop_assert!(tracker.completed_pairs.len() <= tracker.samples.len());
+                    prop_assert_eq!(tracker.sample_order.iter().collect::<HashSet<_>>().len(), tracker.samples.len());
+                    prop_assert!(tracker.sample_order.iter().all(|seq| tracker.samples.contains_key(seq)));
+                    prop_assert!(tracker.completed_pairs.iter().all(|seq| {
+                        tracker.samples.contains_key(seq)
+                            && tracker.samples.contains_key(&seq.wrapping_sub(1))
+                    }), "completed pairs must retain both sequence endpoints");
                 }
             }
 
-            /// A pair for a given adjacent `(seq - 1, seq)` boundary is only
-            /// ever emitted once, however many times `insert` is called
-            /// (including for already-seen or unrelated sequences in
-            /// between). This is the unbounded case, so eviction cannot be
-            /// the reason a pair fails to repeat.
-            #[test]
-            fn each_adjacent_pair_is_emitted_at_most_once(
-                seqs in prop::collection::vec(0u32..12, 0..200),
-            ) {
-                let mut tracker = IpdvTracker::new(None);
-                let mut seen_pairs = std::collections::HashSet::new();
-                for seq in seqs {
-                    for pair in tracker.insert(ipdv_sample(seq, i128::from(seq))) {
-                        let key = (pair.previous_seq, pair.current_seq);
-                        prop_assert!(
-                            seen_pairs.insert(key),
-                            "pair {key:?} emitted more than once"
-                        );
-                    }
-                }
-            }
         }
     }
 }

crates/irtt-stats/src/normalization.rs

@@ -218,43 +218,3 @@ fn duration_ns_i128(duration: Duration) -> i128 {
 fn signed_duration_ns(duration: SignedDuration) -> i128 {
     duration.as_nanos()
 }
-
-#[cfg(test)]
-mod tests {
-    use super::*;
-
-    #[test]
-    fn system_time_before_epoch_is_supported() {
-        let before = UNIX_EPOCH - Duration::from_nanos(7);
-        assert_eq!(system_time_ns(before), Some(-7));
-        let after = UNIX_EPOCH + Duration::from_nanos(7);
-        assert_eq!(system_time_ns(after), Some(7));
-        let now = SystemTime::now();
-        assert!(system_time_ns(now).is_some());
-    }
-
-    #[test]
-    fn loss_windowing_timestamp_is_clamped_to_its_own_sent_at() {
-        let base = Instant::now();
-        let sent_at = ClientTimestamp {
-            mono: base + Duration::from_millis(50),
-            wall: UNIX_EPOCH + Duration::from_millis(50),
-        };
-        // The pre-send-anchored `timeout_at` can precede this same probe's
-        // own post-send `sent_at` when probe_timeout is small relative to
-        // the send-call duration (see irtt-client's AGENTS.md). Rolling-
-        // window eviction assumes non-decreasing event timestamps, so the
-        // windowing `at()` must not regress behind the probe's own Sent
-        // event even though the public `timeout_at` field legitimately can.
-        let timeout_at = base + Duration::from_millis(10);
-
-        let event = ClientEvent::EchoLoss {
-            seq: 0,
-            sent_at,
-            timeout_at,
-        };
-
-        let normalized = normalize_event(&event).expect("loss event normalizes");
-        assert_eq!(normalized.at(), sent_at.mono);
-    }
-}

crates/irtt-stats/tests/ipdv.rs

@@ -0,0 +1,112 @@
+use std::time::{Duration, Instant, UNIX_EPOCH};
+
+use irtt_client::{
+    ClientEvent, ClientTimestamp, PacketMeta, RttSample, ServerTiming, SignedDuration,
+};
+use irtt_stats::{StatsCollector, StatsConfig};
+
+fn reply(
+    base: Instant,
+    seq: u32,
+    send_ms: u64,
+    receive_ms: u64,
+    timing: ServerTiming,
+) -> ClientEvent {
+    let raw = Duration::from_millis(receive_ms - send_ms);
+    ClientEvent::EchoReply {
+        seq,
+        remote: "127.0.0.1:2112".parse().unwrap(),
+        sent_at: ClientTimestamp {
+            mono: base + Duration::from_millis(send_ms),
+            wall: UNIX_EPOCH + Duration::from_millis(200 + u64::from(seq) * 10),
+        },
+        received_at: ClientTimestamp {
+            mono: base + Duration::from_millis(receive_ms),
+            wall: UNIX_EPOCH + Duration::from_millis(240 + u64::from(seq) * 20),
+        },
+        rtt: RttSample {
+            raw,
+            adjusted: None,
+            effective: SignedDuration::from_duration(raw),
+        },
+        server_timing: Some(timing),
+        one_way: None,
+        received_stats: None,
+        bytes: 64,
+        packet_meta: PacketMeta::default(),
+    }
+}
+
+#[test]
+fn directional_ipdv_uses_matching_clock_domains_and_preserves_zero_and_absence() {
+    let first = ServerTiming {
+        receive_mono_ns: Some(100_000_000),
+        send_mono_ns: Some(101_000_000),
+        receive_wall_ns: Some(1_000_000_000),
+        send_wall_ns: Some(1_001_000_000),
+        midpoint_mono_ns: None,
+        midpoint_wall_ns: None,
+        processing: None,
+    };
+    let second = ServerTiming {
+        receive_mono_ns: Some(105_000_000),
+        send_mono_ns: Some(108_000_000),
+        receive_wall_ns: Some(1_020_000_000),
+        send_wall_ns: Some(1_026_000_000),
+        ..first
+    };
+    // Wall and monotonic deltas deliberately disagree: availability alone
+    // cannot prove the formulas or which clock domain wins.
+    let mut wall_only = second;
+    wall_only.receive_mono_ns = None;
+    wall_only.send_mono_ns = None;
+    let mut no_send = wall_only;
+    no_send.receive_wall_ns = None;
+    let mut no_receive = wall_only;
+    no_receive.send_wall_ns = None;
+    let zero = ServerTiming {
+        receive_mono_ns: Some(110_000_000),
+        send_mono_ns: Some(111_000_000),
+        ..second
+    };
+    for (label, current, send_ms, expected_send_ms, expected_receive_ms) in [
+        ("monotonic preferred", second, 30, Some(5), Some(3)),
+        ("wall fallback", wall_only, 30, Some(10), Some(5)),
+        ("missing send endpoint", no_send, 30, None, Some(5)),
+        ("missing receive endpoint", no_receive, 30, Some(10), None),
+        ("reordered client send times", second, 10, Some(15), Some(3)),
+        ("zero variation", zero, 30, Some(0), Some(0)),
+    ] {
+        let base = Instant::now();
+        let mut collector = StatsCollector::new(StatsConfig::finite());
+        assert!(collector
+            .process(&reply(base, 0, 20, 40, first))
+            .ipdv_pairs
+            .is_empty());
+        let update = collector.process(&reply(base, 1, send_ms, 50, current));
+        assert_eq!(update.ipdv_pairs.len(), 1, "{label}");
+        let pair = &update.ipdv_pairs[0];
+        assert_eq!(
+            pair.send_ipdv,
+            expected_send_ms.map(Duration::from_millis),
+            "{label}"
+        );
+        assert_eq!(
+            pair.receive_ipdv,
+            expected_receive_ms.map(Duration::from_millis),
+            "{label}"
+        );
+        let snapshot = collector.snapshot();
+        for (expected, actual) in [
+            (expected_send_ms, snapshot.ipdv.send),
+            (expected_receive_ms, snapshot.ipdv.receive),
+        ] {
+            assert_eq!(actual.count, u64::from(expected.is_some()), "{label}");
+            assert_eq!(
+                actual.total_ns,
+                i128::from(expected.unwrap_or(0)) * 1_000_000,
+                "{label}"
+            );
+        }
+    }
+}

crates/irtt-stats/tests/rolling_properties.rs

@@ -1,44 +1,6 @@
-//! Property tests for the count-based and time-based rolling windows.
-//!
-//! [`RollingEvents`] (in `src/rolling.rs`) keeps two independent `VecDeque`s:
-//! one bounded by [`StatsConfig::rolling_count`] and one bounded by
-//! [`StatsConfig::rolling_time`]. They are pushed to together but evicted
-//! independently, and each exposes its own snapshot
-//! ([`StatsCollector::rolling_count`] / [`StatsCollector::rolling_time`]).
-//! Reading `rolling.rs` and the existing `rolling_count_eviction_recomputes_*`
-//! / `rolling_time_eviction_uses_event_timestamps` tests in `tests/stats.rs`
-//! confirms there is no single window bounded by *both* limits at once: the
-//! two bounds do not compose as AND or OR on shared storage, they are simply
-//! two separate parallel windows over the same input stream. This file proves
-//! that independence generatively: with both bounds configured
-//! simultaneously, each window must behave exactly as if the *other* bound
-//! did not exist.
-//!
-//! The reference model is a plain `Vec<(ClientEvent, at_ms)>` history that is
-//! re-filtered from scratch after every operation (once by count, once by
-//! time), then replayed through a fresh [`StatsCollector`] to obtain a
-//! [`Snapshot`]. Because [`StatsCollector::rolling_count`] /
-//! [`StatsCollector::rolling_time`] are themselves computed by replaying the
-//! retained events through a fresh `CoreStats` on every call (see
-//! `rolling.rs`'s `snapshot_window`), and because both the production window
-//! and this reference model process the *same* `ClientEvent` values in the
-//! *same* order through the *same* pure, deterministic normalization and
-//! aggregation code, the two snapshots are expected to be bit-for-bit equal,
-//! not merely close. `Snapshot` derives `PartialEq`, so the comparison below
-//! is exact `assert_eq!` with no floating-point tolerance: there is no
-//! independent numerical computation on the reference side to accumulate
-//! error against, both sides run the identical arithmetic in the identical
-//! order.
-//!
-//! The reference model's time-window recomputation (filter the whole history
-//! by `at_ms >= latest_at_ms - window_ms`) is only equivalent to the
-//! production incremental sliding-eviction (which pops from the front using
-//! the cutoff computed at each push) when event timestamps are pushed in
-//! non-decreasing order, exactly as `normalization.rs` documents ("Rolling-
-//! window eviction assumes events are pushed in non-decreasing `at()`
-//! order"). The operation generator below enforces that by construction: a
-//! shared clock only ever moves forward, and every generated event borrows
-//! its `at()` timestamp from the clock's current value.
+//! Independent count and time windows must match a filtered replay through
+//! StatsCollector, while cumulative accounting retains the complete history.
+//! Generated events have non-decreasing normalized window timestamps.
 
 use std::time::{Duration, Instant, UNIX_EPOCH};
 
@@ -208,7 +170,7 @@ fn build_event(op: &Op, seq: u32, clock_ms: u64, base: Instant) -> ClientEvent {
         Op::Loss => ClientEvent::EchoLoss {
             seq,
             sent_at: ts_at(base, clock_ms),
-            timeout_at: ts_at(base, clock_ms).mono,
+            timeout_at: ts_at(base, clock_ms).mono - Duration::from_millis(10),
         },
         Op::Warning => ClientEvent::Warning {
             kind: irtt_client::WarningKind::UntrackedReply,
@@ -218,11 +180,7 @@ fn build_event(op: &Op, seq: u32, clock_ms: u64, base: Instant) -> ClientEvent {
     }
 }
 
-/// Replays `events` in order through a fresh collector using the rolling
-/// windows' own semantics (running statistics only, same late-reply policy),
-/// mirroring exactly what `rolling.rs`'s private `snapshot_window` does for
-/// the production windows. This is the "recompute from scratch" reference
-/// primitive both the count and time reference windows below are built on.
+/// Public reference: collect exactly the events retained by one window.
 fn replay(events: &[ClientEvent], late_replies: LateReplyMode) -> Snapshot {
     let mut collector = StatsCollector::new(StatsConfig {
         samples: SampleMode::RunningOnly,
@@ -275,6 +233,11 @@ proptest! {
         // scratch after every step.
         let mut history: Vec<(ClientEvent, u64)> = Vec::new();
 
+        // Force a loss whose clamped time evicts the initial send, while its
+        // raw pre-send deadline would retain it. Every generated case must
+        // therefore distinguish the two normalization choices.
+        let ops = [Op::Send, Op::Advance(u16::try_from(time_limit_ms + 5).unwrap()), Op::Loss, Op::Advance(10), Op::Warning]
+            .into_iter().chain(ops).collect::<Vec<_>>();
         for (idx, op) in ops.iter().enumerate() {
             if let Op::Advance(delta) = op {
                 clock_ms += u64::from(*delta);

crates/irtt-stats/tests/stats.rs

@@ -319,6 +319,19 @@ fn ipdv_wraparound_sequence_is_adjacent() {
     assert!(wrapped.contributed_sample);
     assert_one_ipdv_pair(&wrapped, u32::MAX, 0, Duration::from_millis(4));
 
+    // The client classifies duplicate datagrams before publishing events;
+    // their accounting must not emit an adjacent pair a second time.
+    for seq in [u32::MAX, 0] {
+        let duplicate = collector.process(&ClientEvent::DuplicateReply {
+            seq,
+            remote: "127.0.0.1:2112".parse().unwrap(),
+            received_at: ts(30),
+            bytes: 64,
+        });
+        assert!(!duplicate.contributed_sample);
+        assert_no_ipdv_pairs(&duplicate);
+    }
+
     let snapshot = collector.snapshot();
     assert_eq!(snapshot.ipdv.round_trip.count, 1);
     assert_eq!(snapshot.ipdv.round_trip.total_ns, 4_000_000);
@@ -355,19 +368,6 @@ fn late_reply_can_complete_ipdv_pair() {
     assert_eq!(snapshot.ipdv.round_trip.total_ns, 10_000_000);
 }
 
-#[test]
-fn update_exposes_directional_ipdv_when_available() {
-    let mut collector = StatsCollector::new(StatsConfig::finite());
-    collector.process(&unadjusted_reply(0, 10));
-    let update = collector.process(&unadjusted_reply(1, 13));
-
-    assert!(update.contributed_sample);
-
-    let pair = assert_one_ipdv_pair(&update, 0, 1, Duration::from_millis(3));
-    assert!(pair.send_ipdv.is_some());
-    assert!(pair.receive_ipdv.is_some());
-}
-
 #[test]
 fn gap_fill_update_exposes_both_completed_ipdv_pairs() {
     let mut collector = StatsCollector::new(StatsConfig::finite());