Fix issues raised by the tests, improve coverage

Author: posborne

crates/analysis/src/keys.rs

@@ -82,7 +82,7 @@ impl KeyBuilder {
                 arch: if self.arch { Some(m.arch) } else { None },
                 engine: if self.engine { Some(m.engine) } else { None },
                 engine_flags: if self.engine_flags {
-                    m.engine_flags
+                    Some(m.engine_flags)
                 } else {
                     None
                 },
@@ -96,28 +96,28 @@ impl KeyBuilder {
 }
 
 /// A key for grouping measurements together.
-#[derive(PartialOrd, Ord, PartialEq, Eq, Hash)]
+#[derive(PartialOrd, Ord, PartialEq, Eq, Hash, Debug)]
 pub struct Key<'a> {
     pub arch: Option<Cow<'a, str>>,
     pub engine: Option<Cow<'a, str>>,
+    pub engine_flags: Option<Option<Cow<'a, str>>>,
     pub wasm: Option<Cow<'a, str>>,
     pub phase: Option<Phase>,
     pub event: Option<Cow<'a, str>>,
-    pub engine_flags: Option<Cow<'a, str>>,
 }
 
 impl Key<'_> {
     /// Does the given measurement match this key?
     pub fn matches(&self, m: &Measurement) -> bool {
         self.arch.as_ref().is_none_or(|x| *x == m.arch)
             && self.engine.as_ref().is_none_or(|x| *x == m.engine)
-            && self.wasm.as_ref().is_none_or(|x| *x == m.wasm)
-            && self.phase.as_ref().is_none_or(|x| *x == m.phase)
-            && self.event.as_ref().is_none_or(|x| *x == m.event)
             && self
                 .engine_flags
                 .as_ref()
-                .is_none_or(|x| Some(x) == m.engine_flags.as_ref())
+                .is_none_or(|x| *x == m.engine_flags)
+            && self.wasm.as_ref().is_none_or(|x| *x == m.wasm)
+            && self.phase.as_ref().is_none_or(|x| *x == m.phase)
+            && self.event.as_ref().is_none_or(|x| *x == m.event)
     }
 }
 
@@ -134,7 +134,7 @@ mod tests {
             wasm: Some("bench.wasm".into()),
             phase: Some(Phase::Compilation),
             event: Some("cycles".into()),
-            engine_flags: Some("-Wfoo=bar".into()),
+            engine_flags: Some(Some("-Wfoo=bar".into())),
         };
 
         // More test cases are needed, but this provides a sanity check for the matched key and

crates/analysis/src/summarize.rs

@@ -15,7 +15,7 @@ pub fn calculate<'a>(measurements: &[Measurement<'a>]) -> Vec<Summary<'a>> {
         summaries.push(Summary {
             arch: k.arch.unwrap(),
             engine: k.engine.unwrap(),
-            engine_flags: k.engine_flags,
+            engine_flags: k.engine_flags.unwrap(),
             wasm: k.wasm.unwrap(),
             phase: k.phase.unwrap(),
             event: k.event.unwrap(),
@@ -123,6 +123,7 @@ mod tests {
             Measurement {
                 arch: "x86".into(),
                 engine: "wasmtime".into(),
+                engine_flags: None,
                 wasm: "bench.wasm".into(),
                 process: 42,
                 iteration: 0,
@@ -158,6 +159,7 @@ mod tests {
             Measurement {
                 arch: "x86".into(),
                 engine: "wasmtime".into(),
+                engine_flags: None,
                 wasm: "bench.wasm".into(),
                 process: 42,
                 iteration: 0,
@@ -174,4 +176,31 @@ mod tests {
 
         assert_eq!(calculate(&measurements).len(), 2);
     }
+
+    #[test]
+    fn differing_engine_flags() {
+        use std::borrow::Cow;
+
+        fn measurement<'a>(flags: Option<Cow<'a, str>>, count: u64) -> Measurement<'a> {
+            Measurement {
+                arch: "x86".into(),
+                engine: "wasmtime".into(),
+                engine_flags: flags,
+                wasm: "bench.wasm".into(),
+                process: 42,
+                iteration: 0,
+                phase: Phase::Execution,
+                event: "cycles".into(),
+                count,
+            }
+        }
+        let measurements = vec![
+            measurement(Some("-Wfoo=bar".into()), 0),
+            measurement(Some("-Wfoo=bar".into()), 1),
+            measurement(Some("-Wdead=beeef".into()), 2),
+            measurement(None, 3),
+        ];
+
+        assert_eq!(calculate(&measurements).len(), 3);
+    }
 }

crates/data/tests/serialize.rs

@@ -12,8 +12,10 @@ fn effect_size_serialized_to_csv() {
             phase: Phase::Execution,
             event: "cycles".into(),
             a_engine: "control.so".into(),
+            a_engine_flags: None,
             a_mean: 100.0,
             b_engine: "feature.so".into(),
+            b_engine_flags: Some("-Wfoo=bar".into()),
             b_mean: 110.0,
             significance_level: 0.05,
             half_width_confidence_interval: 1.3,
@@ -23,7 +25,7 @@ fn effect_size_serialized_to_csv() {
     let csv = String::from_utf8(csv).unwrap();
     assert_eq!(
         csv,
-        "arch,wasm,phase,event,a_engine,a_mean,b_engine,b_mean,significance_level,half_width_confidence_interval\n\
-         x86_64,benchmark.wasm,Execution,cycles,control.so,100.0,feature.so,110.0,0.05,1.3\n"
+        "arch,wasm,phase,event,a_engine,a_engine_flags,a_mean,b_engine,b_engine_flags,b_mean,significance_level,half_width_confidence_interval\n\
+         x86_64,benchmark.wasm,Execution,cycles,control.so,,100.0,feature.so,-Wfoo=bar,110.0,0.05,1.3\n"
     );
 }