Merge pull request #1299 from benjaminsavage/fix_test

Making this test less flaky, and good error reports

ipa-core/src/test_fixture/hybrid_event_gen.rs

@@ -158,7 +158,10 @@ impl<R: Rng> Iterator for EventGenerator<R> {
 
 #[cfg(all(test, unit_test))]
 mod tests {
-    use std::collections::{HashMap, HashSet};
+    use std::{
+        collections::{HashMap, HashSet},
+        iter::zip,
+    };
 
     use rand::thread_rng;
 
@@ -175,10 +178,30 @@ mod tests {
 
     #[test]
     fn default_config() {
+        // Since there is randomness, the actual number will be a bit different
+        // from the expected value.
+        // The "tolerance" is used to compute the allowable range of values.
+        // It is multiplied by the expected value. So a tolerance of 0.05 means
+        // we will accept a value within 5% of the expected value
+        const EXPECTED_HISTOGRAM_WITH_TOLERANCE: [(i32, f64); 12] = [
+            (0, 0.0),
+            (647_634, 0.01),
+            (137_626, 0.01),
+            (20_652, 0.02),
+            (3_085, 0.05),
+            (463, 0.12),
+            (70, 0.5),
+            (10, 1.0),
+            (2, 1.0),
+            (0, 1.0),
+            (0, 1.0),
+            (0, 1.0),
+        ];
+        const TEST_COUNT: usize = 1_000_000;
         let gen = EventGenerator::with_default_config(thread_rng());
         let max_convs_per_imp = gen.config.max_convs_per_imp.get();
         let mut match_key_to_event_count = HashMap::new();
-        for event in gen.take(10_000) {
+        for event in gen.take(TEST_COUNT) {
             match event {
                 TestHybridRecord::TestImpression { match_key, .. } => {
                     match_key_to_event_count
@@ -200,44 +223,29 @@ mod tests {
             histogram[count] += 1;
         }
 
-        assert!(
-            (6470 - histogram[1]).abs() < 200,
-            "expected {:?} unmatched events, got {:?}",
-            647,
-            histogram[1]
-        );
-
-        assert!(
-            (1370 - histogram[2]).abs() < 100,
-            "expected {:?} unmatched events, got {:?}",
-            137,
-            histogram[2]
-        );
-
-        assert!(
-            (200 - histogram[3]).abs() < 50,
-            "expected {:?} unmatched events, got {:?}",
-            20,
-            histogram[3]
-        );
-
-        assert!(
-            (30 - histogram[4]).abs() < 40,
-            "expected {:?} unmatched events, got {:?}",
-            3,
-            histogram[4]
-        );
-
-        assert!(
-            (0 - histogram[11]).abs() < 10,
-            "expected {:?} unmatched events, got {:?}",
-            0,
-            histogram[11]
-        );
+        for (actual, (expected, tolerance)) in
+            zip(histogram, EXPECTED_HISTOGRAM_WITH_TOLERANCE.iter())
+        {
+            // Adding a constant value of 10 is a way of dealing with the high variability small values
+            // which will vary a lot more (as a percent). Because 10 is an increasingly large percentage of
+            // A smaller and smaller expected value
+            let max_tolerance = f64::from(*expected) * tolerance + 10.0;
+            assert!(
+                f64::from((expected - actual).abs()) <= max_tolerance,
+                "{:?} is outside of the expected range: ({:?}..{:?})",
+                actual,
+                f64::from(*expected) - max_tolerance,
+                f64::from(*expected) + max_tolerance,
+            );
+        }
     }
 
     #[test]
     fn lots_of_repeat_conversions() {
+        const EXPECTED_HISTOGRAM: [i32; 12] = [
+            0, 299_296, 25_640, 20_542, 16_421, 13_133, 10_503, 8_417, 6_730, 5_391, 4_289, 17_206,
+        ];
+        const TEST_COUNT: usize = 1_000_000;
         const MAX_CONVS_PER_IMP: u32 = 10;
         const MAX_BREAKDOWN_KEY: u32 = 20;
         const MAX_VALUE: u32 = 3;
@@ -252,7 +260,7 @@ mod tests {
         );
         let max_convs_per_imp = gen.config.max_convs_per_imp.get();
         let mut match_key_to_event_count = HashMap::new();
-        for event in gen.take(100_000) {
+        for event in gen.take(TEST_COUNT) {
             match event {
                 TestHybridRecord::TestImpression {
                     match_key,
@@ -279,40 +287,16 @@ mod tests {
             histogram[count] += 1;
         }
 
-        assert!(
-            (30_032 - histogram[1]).abs() < 800,
-            "expected {:?} unmatched events, got {:?}",
-            30_032,
-            histogram[1]
-        );
-
-        assert!(
-            (2_572 - histogram[2]).abs() < 300,
-            "expected {:?} unmatched events, got {:?}",
-            2_572,
-            histogram[2]
-        );
-
-        assert!(
-            (2_048 - histogram[3]).abs() < 200,
-            "expected {:?} unmatched events, got {:?}",
-            2_048,
-            histogram[3]
-        );
-
-        assert!(
-            (1_650 - histogram[4]).abs() < 100,
-            "expected {:?} unmatched events, got {:?}",
-            1_650,
-            histogram[4]
-        );
-
-        assert!(
-            (1_718 - histogram[11]).abs() < 100,
-            "expected {:?} unmatched events, got {:?}",
-            1_718,
-            histogram[11]
-        );
+        for (expected, actual) in zip(EXPECTED_HISTOGRAM.iter(), histogram) {
+            let max_tolerance = f64::from(*expected) * 0.05 + 10.0;
+            assert!(
+                f64::from((expected - actual).abs()) <= max_tolerance,
+                "{:?} is outside of the expected range: ({:?}..{:?})",
+                actual,
+                f64::from(*expected) - max_tolerance,
+                f64::from(*expected) + max_tolerance,
+            );
+        }
     }
 
     #[test]