Repository: fujiapple852/trippy
Pull request #955 (2024-01-26 15:35:56)

test: add backend tracer tests
The PR includes tests for a single hop and 0 to n probes for various probe statuses.  
Note: I did not add mean & m2 to the equality test but plan to add them later.
During tests, it was found that low latency of .5 millis the Jinta value turned negative.  According to the RFC near zero should remain 0 because is minimal.  Added check jinta.max(0.5)

A couple of items to please review:
1) I made tracer & type(src/tracing.rs) public to integrate with tests.  Not completely kosher as some libs are private and public.
2)  I'm looking for a suggestion to add multiple hops to tests.  Can you please suggest it?
3) Are there other missing test cases besides multiple hops?
4) The location of the YAML files is under the backend folder.  I ran into multiple issues moving outside the src directory.  Stumped on it.


fujiapple852: @trkelly23 nice start!  

I pushed some more WIP commits (I didn't squash so you can easily see my changes, we can squash later once it's ready):

- I rebased
- I moved the test_resources to the top level folder and reorganised these and other test_resources
- I simplified the code bit and did some refactoring (removed unneeded `TryFrom`, added a `ProbeStatusTest` type)

> I made tracer & type(src/tracing.rs) public to integrate with tests. Not completely kosher as some libs are private and public.

It was just missing an re-export of `Round` which I've added, no need to make the modules public (I changed it).

> I'm looking for a suggestion to add multiple hops to tests. Can you please suggest it?
> Are there other missing test cases besides multiple hops?

What we are trying to test is the final state of the `Trace` type after the completion of several rounds (i.e. several calls to `update_from_round(TracerRound)`).

Therefore we want each test "scenario" (i.e. file) to consist of:

Input: zero or more `TracerRound`, where each round contains zero or more probes (plus `largest_ttl` and `reason`)
Output: the state of the `Trace` after all `TracerRound` have been applied

At the moment the code has:

Input: zero or more `Probe` (all for the same hop/ttl, but for different rounds, which wouldn't happen in reality)
Output: the state of a single `Hop`

> The location of the YAML files is under the backend folder. I ran into multiple issues moving outside the src directory. Stumped on it.

I moved these, seemed to work ok?
fujiapple852: > Note: I did not add mean & m2 to the equality test but plan to add them later.

I removed these as they are "private internal state" and so not something we need to explicitly test (we're "black box" testing the `Trace` type).

> During tests, it was found that low latency of .5 millis the Jinta value turned negative. According to the RFC near zero should remain 0 because is minimal. Added check jinta.max(0.5)

Always good when adding tests spots a bug!  Can I trouble you to extract the fix into a separate "fix: xxx" commit?
trkelly23: > > Note: I did not add mean & m2 to the equality test but plan to add them later.
> 
> I removed these as they are "private internal state" and so not something we need to explicitly test (we're "black box" testing the `Trace` type).
> 
> > During tests, it was found that low latency of .5 millis the Jinta value turned negative. According to the RFC near zero should remain 0 because is minimal. Added check jinta.max(0.5)
> 
> Always good when adding tests spots a bug! Can I trouble you to extract the fix into a separate "fix: xxx" commit?

Thanks for confirming mean and m2 don't need tests.
I'll backout the fix for low latency and put it into a different branch.
trkelly23: > Therefore we want each test "scenario" (i.e. file) to consist of:
> 
> Input: zero or more `TracerRound`, where each round contains zero or more probes (plus `largest_ttl` and `reason`) Output: the state of the `Trace` after all `TracerRound` have been applied



> @trkelly23 nice start!
> 
> I pushed some more WIP commits (I didn't squash so you can easily see my changes, we can squash later once it's ready):
> 
> * I rebased
> * I moved the test_resources to the top level folder and reorganised these and other test_resources
> * I simplified the code bit and did some refactoring (removed unneeded `TryFrom`, added a `ProbeStatusTest` type)
LOL, I had it a From trait originally.  Good to see it back as one and the clutter removed.
> 
> > I made tracer & type(src/tracing.rs) public to integrate with tests. Not completely kosher as some libs are private and public.
> 
> It was just missing an re-export of `Round` which I've added, no need to make the modules public (I changed it).
Thanks much cleaner.
> 
> > I'm looking for a suggestion to add multiple hops to tests. Can you please suggest it?
> > Are there other missing test cases besides multiple hops?
> 
> What we are trying to test is the final state of the `Trace` type after the completion of several rounds (i.e. several calls to `update_from_round(TracerRound)`).
> 
> Therefore we want each test "scenario" (i.e. file) to consist of:
> 
> Input: zero or more `TracerRound`, where each round contains zero or more probes (plus `largest_ttl` and `reason`) Output: the state of the `Trace` after all `TracerRound` have been applied
Should senario be changed to?
Scenario {
        largest_ttl: usize,
        reason: CompletionReason,
        hop: HopTest,
        probes: Vec<ProbeTest>,
}
> 
> At the moment the code has:
> 
> Input: zero or more `Probe` (all for the same hop/ttl, but for different rounds, which wouldn't happen in reality) Output: the state of a single `Hop`
> 
> > The location of the YAML files is under the backend folder. I ran into multiple issues moving outside the src directory. Stumped on it.
> 
> I moved these, seemed to work ok?
Yes, I tested the move, and works locally.


Patch:
diff --git a/src/backend/trace.rs b/src/backend/trace.rs
index 9f16618..ead4560 100644
--- a/src/backend/trace.rs
+++ b/src/backend/trace.rs
@@ -410,12 +410,12 @@ impl TraceData {
                 hop.total_time += dur;
                 // Before last is set use it to calc jitter
                 let last_ms = hop.last_ms().unwrap_or_default();
-                let jitter_ms = (last_ms - dur_ms).abs();
+                let jitter_ms = (dur_ms - last_ms).abs();
                 let jitter_dur = Duration::from_secs_f64(jitter_ms / 1000_f64);
                 hop.jitter = hop.last.and(Some(jitter_dur));
                 hop.javg += (jitter_ms - hop.javg) / hop.total_recv as f64;
                 // algorithm is from rfc1889, A.8 or rfc3550
-                hop.jinta += jitter_ms - ((hop.jinta + 8.0) / 16.0);
+                hop.jinta += jitter_ms.max(0.5) - ((hop.jinta + 8.0) / 16.0);
                 hop.jmax = hop
                     .jmax
                     .map_or(Some(jitter_dur), |d| Some(d.max(jitter_dur)));
@@ -468,3 +468,224 @@ impl TraceData {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use anyhow::anyhow;
+    use serde::Deserialize;
+    use std::collections::HashSet;
+    use std::ops::Add;
+    use std::str::FromStr;
+    use std::time::SystemTime;
+    use test_case::test_case;
+    use trippy::tracing::{
+        CompletionReason, Flags, IcmpPacketType, Port, Probe, ProbeComplete, ProbeState, Sequence,
+        TimeToLive, TraceId,
+    };
+
+    /// A test scenario.
+    #[derive(Deserialize, Debug)]
+    #[serde(deny_unknown_fields)]
+    struct Scenario {
+        /// the biggest ttl expected in this scenario
+        largest_ttl: u8,
+        /// The rounds of probe tracing data in this scenario.
+        rounds: Vec<RoundData>,
+        /// The expected outcome from running this scenario.
+        expected: Expected,
+    }
+
+    /// A single round of tracing probe data.
+    #[derive(Deserialize, Debug)]
+    #[serde(deny_unknown_fields)]
+    struct RoundData {
+        /// The probes in this round.
+        probes: Vec<ProbeData>,
+    }
+
+    /// A single probe from a single round.
+    #[derive(Deserialize, Debug)]
+    #[serde(deny_unknown_fields)]
+    #[serde(try_from = "String")]
+    struct ProbeData(ProbeState);
+
+    impl TryFrom<String> for ProbeData {
+        type Error = anyhow::Error;
+
+        fn try_from(value: String) -> Result<Self, Self::Error> {
+            // format: {ttl} {status} {duration} {host} {sequence} {src_port} {dest_port}
+            let values = value.split_ascii_whitespace().collect::<Vec<_>>();
+            if values.len() == 7 {
+                let ttl = TimeToLive(u8::from_str(values[0])?);
+                let state = values[1].to_ascii_lowercase();
+                let sequence = Sequence(u16::from_str(values[4])?);
+                let src_port = Port(u16::from_str(values[5])?);
+                let dest_port = Port(u16::from_str(values[6])?);
+                let round = Round(0); // note we inject this later, see ProbeRound
+                let sent = SystemTime::now();
+                let flags = Flags::empty();
+                let state = match state.as_str() {
+                    "n" => Ok(ProbeState::NotSent),
+                    "s" => Ok(ProbeState::Skipped),
+                    "a" => Ok(ProbeState::Awaited(Probe::new(
+                        sequence,
+                        TraceId(0),
+                        src_port,
+                        dest_port,
+                        ttl,
+                        round,
+                        sent,
+                        flags,
+                    ))),
+                    "c" => {
+                        let host = IpAddr::from_str(values[3])?;
+                        let duration = Duration::from_millis(u64::from_str(values[2])?);
+                        let received = sent.add(duration);
+                        let icmp_packet_type = IcmpPacketType::NotApplicable;
+                        Ok(ProbeState::Complete(
+                            Probe::new(
+                                sequence,
+                                TraceId(0),
+                                src_port,
+                                dest_port,
+                                ttl,
+                                round,
+                                sent,
+                                flags,
+                            )
+                            .complete(
+                                host,
+                                received,
+                                icmp_packet_type,
+                                None,
+                            ),
+                        ))
+                    }
+                    _ => Err(anyhow!("unknown probe state")),
+                }?;
+                Ok(Self(state))
+            } else {
+                Err(anyhow!("failed to parse {}", value))
+            }
+        }
+    }
+
+    /// A helper struct so wwe may inject the round into the probes.
+    struct ProbeRound(ProbeData, Round);
+
+    impl From<ProbeRound> for ProbeState {
+        fn from(value: ProbeRound) -> Self {
+            let probe_data = value.0;
+            let round = value.1;
+            match probe_data.0 {
+                Self::NotSent => Self::NotSent,
+                Self::Skipped => Self::Skipped,
+                Self::Awaited(awaited) => Self::Awaited(Probe { round, ..awaited }),
+                Self::Complete(completed) => Self::Complete(ProbeComplete { round, ..completed }),
+            }
+        }
+    }
+
+    /// The expected outcome.
+    #[derive(Deserialize, Debug, Clone)]
+    #[serde(deny_unknown_fields)]
+    struct Expected {
+        /// The expected outcome per hop.
+        hops: Vec<HopData>,
+    }
+
+    /// The expected outcome for a single hop.
+    #[derive(Deserialize, Debug, Clone)]
+    #[serde(deny_unknown_fields)]
+    struct HopData {
+        ttl: u8,
+        total_sent: usize,
+        total_recv: usize,
+        loss_pct: f64,
+        last_ms: Option<f64>,
+        best_ms: Option<f64>,
+        worst_ms: Option<f64>,
+        avg_ms: f64,
+        jitter: Option<f64>,
+        javg: f64,
+        jmax: Option<f64>,
+        jinta: f64,
+        addrs: HashMap<IpAddr, usize>,
+        samples: Option<Vec<f64>>,
+        last_src: u16,
+        last_dest: u16,
+        last_sequence: u16,
+    }
+
+    macro_rules! file {
+        ($path:expr) => {{
+            let yaml = include_str!(concat!("../../test_resources/backend/", $path));
+            serde_yaml::from_str(yaml).unwrap()
+        }};
+    }
+
+    #[test_case(file!("ipv4_3probes_3hops_mixed_multi.yaml"))]
+    #[test_case(file!("ipv4_3probes_3hops_completed.yaml"))]
+    #[test_case(file!("ipv4_4probes_all_status.yaml"))]
+    #[test_case(file!("ipv4_4probes_0latency.yaml"))]
+    fn test_scenario(scenario: Scenario) {
+        let mut trace = Trace::new(100, 1);
+        for (i, round) in scenario.rounds.into_iter().enumerate() {
+            let probes = round
+                .probes
+                .into_iter()
+                .map(|p| ProbeRound(p, Round(i)))
+                .map(Into::into)
+                .collect::<Vec<_>>();
+            let largest_ttl = TimeToLive(scenario.largest_ttl);
+            let tracer_round =
+                TracerRound::new(&probes, largest_ttl, CompletionReason::TargetFound);
+            trace.update_from_round(&tracer_round);
+        }
+        let actual_hops = trace.hops(Trace::default_flow_id());
+        let expected_hops = scenario.expected.hops;
+        for (actual, expected) in actual_hops.iter().zip(expected_hops) {
+            assert_eq!(actual.ttl(), expected.ttl);
+            assert_eq!(
+                actual.addrs().collect::<HashSet<_>>(),
+                expected.addrs.keys().collect::<HashSet<_>>()
+            );
+            assert_eq!(actual.addr_count(), expected.addrs.len());
+            assert_eq!(actual.total_sent(), expected.total_sent);
+            assert_eq!(actual.total_recv(), expected.total_recv);
+            assert_eq_optional(Some(actual.loss_pct()), Some(expected.loss_pct));
+            assert_eq_optional(actual.last_ms(), expected.last_ms);
+            assert_eq_optional(actual.best_ms(), expected.best_ms);
+            assert_eq_optional(actual.worst_ms(), expected.worst_ms);
+            assert_eq_optional(Some(actual.avg_ms()), Some(expected.avg_ms));
+            assert_eq_optional(actual.jitter_ms(), expected.jitter);
+            assert_eq_optional(Some(actual.javg_ms()), Some(expected.javg));
+            assert_eq_optional(actual.jmax_ms(), expected.jmax);
+            assert_eq_optional(Some(actual.jinta()), Some(expected.jinta));
+            assert_eq!(actual.last_src_port(), expected.last_src);
+            assert_eq!(actual.last_dest_port(), expected.last_dest);
+            assert_eq!(actual.last_sequence(), expected.last_sequence);
+            assert_eq!(
+                Some(
+                    actual
+                        .samples()
+                        .iter()
+                        .map(|s| s.as_secs_f64() * 1000_f64)
+                        .collect()
+                ),
+                expected.samples
+            );
+        }
+    }
+
+    #[allow(clippy::float_cmp)]
+    fn assert_eq_optional(actual: Option<f64>, expected: Option<f64>) {
+        match (actual, expected) {
+            (Some(actual), Some(expected)) => assert_eq!(actual, expected),
+            (Some(_), None) => panic!("actual {actual:?} but not expected"),
+            (None, Some(_)) => panic!("expected {expected:?} but no actual"),
+            (None, None) => {}
+        }
+    }
+}
diff --git a/src/tracing.rs b/src/tracing.rs
index cdb2831..62a6cfe 100644
--- a/src/tracing.rs
+++ b/src/tracing.rs
@@ -19,8 +19,8 @@ pub use net::channel::TracerChannel;
 pub use net::source::SourceAddr;
 pub use net::SocketImpl;
 pub use probe::{
-    Extension, Extensions, IcmpPacketType, MplsLabelStack, MplsLabelStackMember, ProbeState,
-    UnknownExtension,
+    Extension, Extensions, IcmpPacketType, MplsLabelStack, MplsLabelStackMember, Probe,
+    ProbeComplete, ProbeState, UnknownExtension,
 };
 pub use tracer::{CompletionReason, Tracer, TracerRound};
 pub use types::{


Test patch:
diff --git a/test_resources/backend/ipv4_3probes_3hops_completed.yaml b/test_resources/backend/ipv4_3probes_3hops_completed.yaml
new file mode 100644
index 0000000..959d533
--- /dev/null
+++ b/test_resources/backend/ipv4_3probes_3hops_completed.yaml
@@ -0,0 +1,70 @@
+largest_ttl: 3
+rounds:
+  - probes:
+      - 1 C 333 10.1.0.1 0 12340 80
+      - 2 C 777 10.1.0.2 1 12340 80
+      - 3 C 778 10.1.0.3 2 12340 80
+  - probes:
+      - 1 C 123 10.1.0.1 3 12340 80
+      - 2 C 788 10.1.0.2 4 12340 80
+      - 3 C 789 10.1.0.3 5 12340 80
+  - probes:
+      - 1 C 123 10.1.0.1 6 12340 80
+      - 2 C 780 10.1.0.2 7 12340 80
+      - 3 C 781 10.1.0.3 8 12340 80
+expected:
+  hops:
+    - ttl: 1
+      total_sent: 3
+      total_recv: 3
+      loss_pct: 0
+      best_ms: 123
+      worst_ms: 333
+      avg_ms: 193
+      jitter: 0
+      javg: 181.0
+      jinta: 488.642578125
+      jmax: 333
+      addrs:
+        10.1.0.1: 3
+      samples: [123, 123, 333]
+      last_ms: 123
+      last_sequence: 6
+      last_src: 12340
+      last_dest: 80 
+    - ttl: 2
+      total_sent: 3
+      total_recv: 3
+      loss_pct: 0
+      best_ms: 777
+      worst_ms: 788
+      avg_ms: 781.6666666666665
+      jitter: 8
+      javg: 265.33333333333337
+      jinta: 699.814453125
+      jmax: 777.0
+      addrs:
+        10.1.0.2: 3
+      samples: [780, 788, 777]
+      last_ms: 780
+      last_sequence: 7
+      last_src: 12340
+      last_dest: 80 
+    - ttl: 3
+      total_sent: 3
+      total_recv: 3
+      loss_pct: 0
+      best_ms: 778
+      worst_ms: 789
+      avg_ms: 782.6666666666666
+      jitter: 8
+      javg: 265.66666666666663
+      jinta: 700.693359375
+      jmax: 778.0
+      addrs:
+        10.1.0.3: 3
+      samples: [781, 789, 778]
+      last_ms: 781
+      last_sequence: 8
+      last_src: 12340
+      last_dest: 80 
diff --git a/test_resources/backend/ipv4_3probes_3hops_mixed_multi.yaml b/test_resources/backend/ipv4_3probes_3hops_mixed_multi.yaml
new file mode 100644
index 0000000..c55e870
--- /dev/null
+++ b/test_resources/backend/ipv4_3probes_3hops_mixed_multi.yaml
@@ -0,0 +1,61 @@
+largest_ttl: 3
+rounds:
+  - probes:
+      - 1 C 10 10.0.0.1 0 12345 80
+      - 2 A 12 10.0.0.2 1 12345 80
+      - 3 C 11 10.0.0.3 2 12345 80
+  - probes:
+      - 1 C 101 10.0.0.1 3 12345 80
+      - 2 A 121 10.0.0.2 4 12345 80
+      - 3 C 111 10.0.0.4 5 12345 80
+expected:
+  hops:
+    - ttl: 1
+      total_sent: 2
+      total_recv: 2
+      loss_pct: 0
+      best_ms: 10
+      worst_ms: 101
+      avg_ms: 55.5
+      jitter: 91
+      javg: 50.5
+      jinta: 99.40625
+      jmax: 91
+      addrs:
+        10.0.0.1: 2
+      samples: [101, 10]
+      last_ms: 101
+      last_src: 12345
+      last_dest: 80
+      last_sequence: 3
+    - ttl: 2
+      total_sent: 2
+      total_recv: 0
+      loss_pct: 100
+      avg_ms: 0
+      javg: 0
+      jinta: 0
+      addrs:
+      samples: [0, 0]
+      last_src: 12345
+      last_dest: 80
+      last_sequence: 4
+    - ttl: 3
+      total_sent: 2
+      total_recv: 2
+      loss_pct: 0
+      best_ms: 11
+      worst_ms: 111
+      avg_ms: 61
+      jitter: 100
+      javg:  55.5
+      jinta: 109.34375
+      jmax: 100
+      addrs:
+        10.0.0.3: 1
+        10.0.0.4: 1
+      samples: [111, 11]
+      last_ms: 111
+      last_src: 12345
+      last_dest: 80
+      last_sequence: 5
\ No newline at end of file
diff --git a/test_resources/backend/ipv4_4probes_0latency.yaml b/test_resources/backend/ipv4_4probes_0latency.yaml
new file mode 100644
index 0000000..aeae471
--- /dev/null
+++ b/test_resources/backend/ipv4_4probes_0latency.yaml
@@ -0,0 +1,30 @@
+largest_ttl: 1
+rounds:
+  - probes:
+      - 1 C 0 127.0.0.1 0 80 80
+  - probes:
+      - 1 C 0 127.0.0.1 0 80 80
+  - probes:
+      - 1 C 0 127.0.0.1 0 80 80
+  - probes:
+      - 1 C 0 127.0.0.1 0 80 80
+expected:
+  hops:
+    - ttl: 1
+      total_sent: 4
+      total_recv: 4
+      loss_pct: 0
+      last_ms: 0.0
+      best_ms: 0
+      worst_ms: 0
+      avg_ms: 0
+      addrs:
+        127.0.0.1: 1
+      samples: [0, 0, 0, 0]
+      jitter: 0
+      javg: 0.0
+      jmax: 0.0
+      jinta: 0.0
+      last_sequence: 0
+      last_src: 80
+      last_dest: 80
diff --git a/test_resources/backend/ipv4_4probes_all_status.yaml b/test_resources/backend/ipv4_4probes_all_status.yaml
new file mode 100644
index 0000000..65abfd7
--- /dev/null
+++ b/test_resources/backend/ipv4_4probes_all_status.yaml
@@ -0,0 +1,30 @@
+largest_ttl: 1
+rounds:
+  - probes:
+      - 1 A 300 10.1.0.2 0 12340 80
+  - probes:
+      - 1 C 700 10.1.0.2 0 12340 80
+  - probes:
+      - 1 N 300 10.1.0.2 0 12340 80
+  - probes:
+      - 1 S 300 10.1.0.2 0 12340 80
+expected:
+  hops:
+    - ttl: 1
+      total_sent: 2
+      total_recv: 1
+      loss_pct: 50
+      best_ms: 700
+      worst_ms: 700
+      avg_ms: 700
+      # jitter: None
+      javg: 700
+      jmax: 700
+      jinta: 699.5
+      addrs:
+        10.1.0.2: 1
+      samples: [700.0, 0.0]
+      last_ms: 700
+      last_sequence: 0
+      last_src: 12340
+      last_dest: 80