From 6fc683b8fab2b14e5b2bd1b7eef2ab7b7a808f22 Mon Sep 17 00:00:00 2001 From: Ben Savage Date: Fri, 20 Sep 2024 16:43:53 -0700 Subject: [PATCH 1/5] Making this test less flaky, and good error reports --- ipa-core/src/test_fixture/hybrid_event_gen.rs | 112 +++++++----------- 1 file changed, 41 insertions(+), 71 deletions(-) diff --git a/ipa-core/src/test_fixture/hybrid_event_gen.rs b/ipa-core/src/test_fixture/hybrid_event_gen.rs index 8d44d38b3..eb2ee616a 100644 --- a/ipa-core/src/test_fixture/hybrid_event_gen.rs +++ b/ipa-core/src/test_fixture/hybrid_event_gen.rs @@ -1,4 +1,4 @@ -use std::num::NonZeroU32; +use std::{iter::zip, num::NonZeroU32}; use rand::Rng; @@ -175,10 +175,25 @@ mod tests { #[test] fn default_config() { + const EXPECTED_HISTOGRAM_WITH_TOLERANCE: [(i32, f64); 12] = [ + (0, 0.0), + (647634, 0.01), + (137626, 0.01), + (20652, 0.02), + (3085, 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 +215,25 @@ 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()) + { + let max_tolerance = (*expected as f64) * tolerance + 10.0; + assert!( + (expected - actual).abs() as f64 <= max_tolerance, + "expected {:?} unmatched events, got {:?}", + expected, + actual, + ); + } } #[test] fn lots_of_repeat_conversions() { + const EXPECTED_HISTOGRAM: [i32; 12] = [ + 0, 299296, 25640, 20542, 16421, 13133, 10503, 8417, 6730, 5391, 4289, 17206, + ]; + 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 +248,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 +275,14 @@ 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) { + assert!( + (expected - actual).abs() <= expected / 20 + 10, + "expected {:?} unmatched events, got {:?}", + expected, + actual, + ); + } } #[test] From dc9fad53623488fb9c71769d9f0fd62bffde2b6d Mon Sep 17 00:00:00 2001 From: Ben Savage Date: Fri, 20 Sep 2024 17:03:29 -0700 Subject: [PATCH 2/5] Improving it --- ipa-core/src/test_fixture/hybrid_event_gen.rs | 21 ++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/ipa-core/src/test_fixture/hybrid_event_gen.rs b/ipa-core/src/test_fixture/hybrid_event_gen.rs index eb2ee616a..c332cde2b 100644 --- a/ipa-core/src/test_fixture/hybrid_event_gen.rs +++ b/ipa-core/src/test_fixture/hybrid_event_gen.rs @@ -175,6 +175,11 @@ 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), (647634, 0.01), @@ -218,12 +223,16 @@ mod tests { 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 = (*expected as f64) * tolerance + 10.0; assert!( (expected - actual).abs() as f64 <= max_tolerance, - "expected {:?} unmatched events, got {:?}", - expected, + "{:?} is outside of the expected range: ({:?}..{:?})", actual, + (*expected as f64) - max_tolerance, + (*expected as f64) + max_tolerance, ); } } @@ -276,11 +285,13 @@ mod tests { } for (expected, actual) in zip(EXPECTED_HISTOGRAM.iter(), histogram) { + let max_tolerance = (*expected as f64) * 0.05 + 10.0; assert!( - (expected - actual).abs() <= expected / 20 + 10, - "expected {:?} unmatched events, got {:?}", - expected, + (expected - actual).abs() as f64 <= max_tolerance, + "{:?} is outside of the expected range: ({:?}..{:?})", actual, + (*expected as f64) - max_tolerance, + (*expected as f64) + max_tolerance, ); } } From 797c39f415c08a5ec49cf7b86c0ad9174b1c35b9 Mon Sep 17 00:00:00 2001 From: Ben Savage Date: Fri, 20 Sep 2024 17:13:03 -0700 Subject: [PATCH 3/5] Fix import --- ipa-core/src/bin/in_the_clear.rs | 65 +++++++++++++++++++ ipa-core/src/test_fixture/hybrid_event_gen.rs | 13 ++-- 2 files changed, 73 insertions(+), 5 deletions(-) create mode 100644 ipa-core/src/bin/in_the_clear.rs diff --git a/ipa-core/src/bin/in_the_clear.rs b/ipa-core/src/bin/in_the_clear.rs new file mode 100644 index 000000000..e48fa77a4 --- /dev/null +++ b/ipa-core/src/bin/in_the_clear.rs @@ -0,0 +1,65 @@ +use std::{ + error::Error, + path::{Path, PathBuf}, +}; + +use clap::Parser; +use ipa_core::{ + cli::playbook::InputSource, + test_fixture::hybrid::{hybrid_in_the_clear, TestHybridRecord}, +}; + +#[derive(Debug, Parser)] +pub struct CommandInput { + #[arg( + long, + help = "Read the input from the provided file, instead of standard input" + )] + input_file: Option, +} + +impl From<&CommandInput> for InputSource { + fn from(source: &CommandInput) -> Self { + if let Some(ref file_name) = source.input_file { + InputSource::from_file(file_name) + } else { + InputSource::from_stdin() + } + } +} + +#[derive(Debug, Parser)] +#[clap(name = "rc", about = "Report Collector CLI")] +#[command(about)] +struct Args { + #[clap(flatten)] + input: CommandInput, + + /// The destination file for output. + #[arg(long, value_name = "OUTPUT_FILE")] + output_file: PathBuf, +} + +fn main() -> Result<(), Box> { + let args = Args::parse(); + + let input = InputSource::from(&args.input); + + let input_rows = input.iter::().collect::>(); + let expected = hybrid_in_the_clear(&input_rows, 10); + + let mut file = File::options() + .write(true) + .create_new(true) + .open(args.output_file) + .map_err(|e| { + format!( + "Failed to create output file {}: {e}", + args.output_file.display() + ) + })?; + + write!(file, "{}", serde_json::to_string_pretty(&expected)?)?; + + Ok(()) +} diff --git a/ipa-core/src/test_fixture/hybrid_event_gen.rs b/ipa-core/src/test_fixture/hybrid_event_gen.rs index c332cde2b..7eddb0ba4 100644 --- a/ipa-core/src/test_fixture/hybrid_event_gen.rs +++ b/ipa-core/src/test_fixture/hybrid_event_gen.rs @@ -1,4 +1,4 @@ -use std::{iter::zip, num::NonZeroU32}; +use std::num::NonZeroU32; use rand::Rng; @@ -158,7 +158,10 @@ impl Iterator for EventGenerator { #[cfg(all(test, unit_test))] mod tests { - use std::collections::{HashMap, HashSet}; + use std::{ + collections::{HashMap, HashSet}, + iter::zip, + }; use rand::thread_rng; @@ -181,9 +184,9 @@ mod tests { // 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), - (647634, 0.01), - (137626, 0.01), + (0, 0.0), + (647634, 0.01), + (137626, 0.01), (20652, 0.02), (3085, 0.05), (463, 0.12), From 43104a17808ff5a5439144df09e55e9184720d3c Mon Sep 17 00:00:00 2001 From: Ben Savage Date: Fri, 20 Sep 2024 17:15:14 -0700 Subject: [PATCH 4/5] whoops --- ipa-core/src/bin/in_the_clear.rs | 65 -------------------------------- 1 file changed, 65 deletions(-) delete mode 100644 ipa-core/src/bin/in_the_clear.rs diff --git a/ipa-core/src/bin/in_the_clear.rs b/ipa-core/src/bin/in_the_clear.rs deleted file mode 100644 index e48fa77a4..000000000 --- a/ipa-core/src/bin/in_the_clear.rs +++ /dev/null @@ -1,65 +0,0 @@ -use std::{ - error::Error, - path::{Path, PathBuf}, -}; - -use clap::Parser; -use ipa_core::{ - cli::playbook::InputSource, - test_fixture::hybrid::{hybrid_in_the_clear, TestHybridRecord}, -}; - -#[derive(Debug, Parser)] -pub struct CommandInput { - #[arg( - long, - help = "Read the input from the provided file, instead of standard input" - )] - input_file: Option, -} - -impl From<&CommandInput> for InputSource { - fn from(source: &CommandInput) -> Self { - if let Some(ref file_name) = source.input_file { - InputSource::from_file(file_name) - } else { - InputSource::from_stdin() - } - } -} - -#[derive(Debug, Parser)] -#[clap(name = "rc", about = "Report Collector CLI")] -#[command(about)] -struct Args { - #[clap(flatten)] - input: CommandInput, - - /// The destination file for output. - #[arg(long, value_name = "OUTPUT_FILE")] - output_file: PathBuf, -} - -fn main() -> Result<(), Box> { - let args = Args::parse(); - - let input = InputSource::from(&args.input); - - let input_rows = input.iter::().collect::>(); - let expected = hybrid_in_the_clear(&input_rows, 10); - - let mut file = File::options() - .write(true) - .create_new(true) - .open(args.output_file) - .map_err(|e| { - format!( - "Failed to create output file {}: {e}", - args.output_file.display() - ) - })?; - - write!(file, "{}", serde_json::to_string_pretty(&expected)?)?; - - Ok(()) -} From 7b9f9da6835488a845c15ed6a2ae7997e564bd81 Mon Sep 17 00:00:00 2001 From: Ben Savage Date: Sat, 21 Sep 2024 04:20:12 -0700 Subject: [PATCH 5/5] Clippy --- ipa-core/src/test_fixture/hybrid_event_gen.rs | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/ipa-core/src/test_fixture/hybrid_event_gen.rs b/ipa-core/src/test_fixture/hybrid_event_gen.rs index 7eddb0ba4..bc1a463dd 100644 --- a/ipa-core/src/test_fixture/hybrid_event_gen.rs +++ b/ipa-core/src/test_fixture/hybrid_event_gen.rs @@ -185,10 +185,10 @@ mod tests { // we will accept a value within 5% of the expected value const EXPECTED_HISTOGRAM_WITH_TOLERANCE: [(i32, f64); 12] = [ (0, 0.0), - (647634, 0.01), - (137626, 0.01), - (20652, 0.02), - (3085, 0.05), + (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), @@ -229,13 +229,13 @@ mod tests { // 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 = (*expected as f64) * tolerance + 10.0; + let max_tolerance = f64::from(*expected) * tolerance + 10.0; assert!( - (expected - actual).abs() as f64 <= max_tolerance, + f64::from((expected - actual).abs()) <= max_tolerance, "{:?} is outside of the expected range: ({:?}..{:?})", actual, - (*expected as f64) - max_tolerance, - (*expected as f64) + max_tolerance, + f64::from(*expected) - max_tolerance, + f64::from(*expected) + max_tolerance, ); } } @@ -243,7 +243,7 @@ mod tests { #[test] fn lots_of_repeat_conversions() { const EXPECTED_HISTOGRAM: [i32; 12] = [ - 0, 299296, 25640, 20542, 16421, 13133, 10503, 8417, 6730, 5391, 4289, 17206, + 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; @@ -288,13 +288,13 @@ mod tests { } for (expected, actual) in zip(EXPECTED_HISTOGRAM.iter(), histogram) { - let max_tolerance = (*expected as f64) * 0.05 + 10.0; + let max_tolerance = f64::from(*expected) * 0.05 + 10.0; assert!( - (expected - actual).abs() as f64 <= max_tolerance, + f64::from((expected - actual).abs()) <= max_tolerance, "{:?} is outside of the expected range: ({:?}..{:?})", actual, - (*expected as f64) - max_tolerance, - (*expected as f64) + max_tolerance, + f64::from(*expected) - max_tolerance, + f64::from(*expected) + max_tolerance, ); } }