diff --git a/src/agent/onefuzz-task/src/test_utils.rs b/src/agent/onefuzz-task/src/config_test_utils.rs similarity index 61% rename from src/agent/onefuzz-task/src/test_utils.rs rename to src/agent/onefuzz-task/src/config_test_utils.rs index 29f8ce16d..22c83e60d 100644 --- a/src/agent/onefuzz-task/src/test_utils.rs +++ b/src/agent/onefuzz-task/src/config_test_utils.rs @@ -1,3 +1,13 @@ +use onefuzz::expand::{GetExpand, PlaceHolder}; + +// Moving this trait method into the GetExpand trait--and returning `Vec<(PlaceHolder, Box)>` instead +// would let us use define a default implementation for `get_expand()` while also coupling the expand values we +// test with those we give to the expander. +// It seems to me like a non-trivial (and perhaps bad) design change though. +pub trait GetExpandFields: GetExpand { + fn get_expand_fields(&self) -> Vec<(PlaceHolder, String)>; +} + pub mod arbitraries { use std::path::PathBuf; @@ -7,7 +17,7 @@ pub mod arbitraries { use reqwest::Url; use uuid::Uuid; - use crate::tasks::config::CommonConfig; + use crate::tasks::{config::CommonConfig, analysis}; prop_compose! { fn arb_uuid()( @@ -85,9 +95,20 @@ pub mod arbitraries { } } } - + prop_compose! { - fn arb_common_config(tag_limit: usize)( + fn arb_string_vec_no_vars()( + // I don't know how to figure out the expected value of the target options if they could contain variables (e.g. {machine_id}) + // This should be fine since this isn't used to test nested expansion + options in prop::collection::vec("[^{}]*", 10), + ) -> Vec { + options + } + } + + + prop_compose! { + fn arb_common_config()( job_id in arb_uuid(), task_id in arb_uuid(), instance_id in arb_uuid(), @@ -101,7 +122,7 @@ pub mod arbitraries { extra_output in option::of(arb_synced_dir()), min_available_memory_mb in any::(), machine_identity in arb_machine_identity(), - tags in prop::collection::hash_map(".*", ".*", tag_limit), + tags in prop::collection::hash_map(".*", ".*", 10), from_agent_to_task_endpoint in ".*", from_task_to_agent_endpoint in ".*", ) -> CommonConfig { @@ -131,12 +152,50 @@ pub mod arbitraries { type Strategy = BoxedStrategy; fn arbitrary_with(_args: Self::Parameters) -> Self::Strategy { - arb_common_config(10).boxed() + arb_common_config().boxed() } } - - // Make a trait out of this and add it to a common test module - impl CommonConfig { - // Get all the fields from the type that are passed to the expander + + prop_compose! { + fn arb_analysis_config()( + analyzer_exe in Just("src/lib.rs".to_string()), + analyzer_options in arb_string_vec_no_vars(), + analyzer_env in prop::collection::hash_map(".*", ".*", 10), + target_exe in arb_pathbuf(), + target_options in arb_string_vec_no_vars(), + input_queue in Just(None), + crashes in option::of(arb_synced_dir()), + analysis in arb_synced_dir(), + tools in option::of(arb_synced_dir()), + reports in option::of(arb_synced_dir()), + unique_reports in option::of(arb_synced_dir()), + no_repro in option::of(arb_synced_dir()), + common in arb_common_config(), + ) -> analysis::generic::Config { + analysis::generic::Config { + analyzer_exe, + analyzer_options, + analyzer_env, + target_exe, + target_options, + input_queue, + crashes, + analysis, + tools, + reports, + unique_reports, + no_repro, + common, + } + } + } + + impl Arbitrary for analysis::generic::Config { + type Parameters = (); + type Strategy = BoxedStrategy; + + fn arbitrary_with(_args: Self::Parameters) -> Self::Strategy { + arb_analysis_config().boxed() + } } } diff --git a/src/agent/onefuzz-task/src/lib.rs b/src/agent/onefuzz-task/src/lib.rs index 51eb0c321..9e01e5e04 100644 --- a/src/agent/onefuzz-task/src/lib.rs +++ b/src/agent/onefuzz-task/src/lib.rs @@ -8,4 +8,4 @@ extern crate onefuzz_telemetry; pub mod local; pub mod tasks; #[cfg(test)] -pub mod test_utils; +pub mod config_test_utils; diff --git a/src/agent/onefuzz-task/src/tasks/analysis/generic.rs b/src/agent/onefuzz-task/src/tasks/analysis/generic.rs index 161849029..4ed14a432 100644 --- a/src/agent/onefuzz-task/src/tasks/analysis/generic.rs +++ b/src/agent/onefuzz-task/src/tasks/analysis/generic.rs @@ -268,3 +268,55 @@ pub async fn run_tool( .with_context(|| format!("analyzer failed to run: {analyzer_path}"))?; Ok(()) } + +#[cfg(test)] +mod tests { + use proptest::prelude::*; + use onefuzz::expand::{GetExpand, PlaceHolder}; + + use crate::config_test_utils::GetExpandFields; + + use super::Config; + + impl GetExpandFields for Config { + fn get_expand_fields(&self) -> Vec<(PlaceHolder, String)> { + let mut params = self.common.get_expand_fields(); + params.push((PlaceHolder::AnalyzerExe, dunce::canonicalize(&self.analyzer_exe).unwrap().to_string_lossy().to_string())); + params.push((PlaceHolder::AnalyzerOptions, self.analyzer_options.join(" "))); + params.push((PlaceHolder::TargetExe, dunce::canonicalize(&self.target_exe).unwrap().to_string_lossy().to_string())); + params.push((PlaceHolder::TargetOptions, self.target_options.join(" "))); + params.push((PlaceHolder::OutputDir, dunce::canonicalize(&self.analysis.local_path).unwrap().to_string_lossy().to_string())); + if let Some(tools) = &self.tools { + params.push((PlaceHolder::ToolsDir, dunce::canonicalize(&tools.local_path).unwrap().to_string_lossy().to_string())); + } + if let Some(reports) = &self.reports { + params.push((PlaceHolder::ReportsDir, dunce::canonicalize(&reports.local_path).unwrap().to_string_lossy().to_string())); + } + if let Some(crashes) = &self.crashes { + if let Some(account) = crashes.remote_path.clone().and_then(|u| u.account()) { + params.push((PlaceHolder::CrashesAccount, account)); + } + if let Some(container) = crashes.remote_path.clone().and_then(|u| u.container()) { + params.push((PlaceHolder::CrashesContainer, container)); + } + } + + params + } + } + + proptest! { + #[test] + fn test_get_expand_values_match_config( + config in any::(), + ) { + let expand = config.get_expand(); + let params = config.get_expand_fields(); + + for (param, expected) in params.iter() { + let evaluated = expand.evaluate_value(format!("{}", param.get_string())).unwrap(); + assert_eq!(evaluated, *expected, "placeholder {} did not match expected value", param.get_string()); + } + } + } +} diff --git a/src/agent/onefuzz-task/src/tasks/config.rs b/src/agent/onefuzz-task/src/tasks/config.rs index 4ee71d935..cfbb9fa09 100644 --- a/src/agent/onefuzz-task/src/tasks/config.rs +++ b/src/agent/onefuzz-task/src/tasks/config.rs @@ -131,6 +131,7 @@ impl GetExpand for CommonConfig { .machine_id() .job_id(&self.job_id) .task_id(&self.task_id) + .setup_dir(&self.setup_dir) .set_optional_ref( &self.instance_telemetry_key, Expand::instance_telemetry_key @@ -139,7 +140,6 @@ impl GetExpand for CommonConfig { &self.microsoft_telemetry_key, Expand::microsoft_telemetry_key ) - .setup_dir(&self.setup_dir) .set_optional_ref( &self.extra_setup_dir, Expand::extra_setup_dir @@ -393,43 +393,47 @@ impl Config { #[cfg(test)] mod tests { use proptest::prelude::*; - use onefuzz::expand::GetExpand; - use std::collections::HashMap; + use onefuzz::expand::{GetExpand, PlaceHolder}; + + use crate::config_test_utils::GetExpandFields; use super::CommonConfig; + impl GetExpandFields for CommonConfig { + fn get_expand_fields(&self) -> Vec<(PlaceHolder, String)> { + let mut params = vec![ + (PlaceHolder::MachineId, self.machine_identity.machine_id.to_string()), + (PlaceHolder::JobId, self.job_id.to_string()), + (PlaceHolder::TaskId, self.task_id.to_string()), + (PlaceHolder::SetupDir, dunce::canonicalize(&self.setup_dir).unwrap().to_string_lossy().to_string()), + ]; + if let Some(key) = &self.instance_telemetry_key { + params.push((PlaceHolder::InstanceTelemetryKey, key.to_string())); + } + if let Some(key) = &self.microsoft_telemetry_key { + params.push((PlaceHolder::MicrosoftTelemetryKey, key.clone().to_string())); + } + if let Some(dir) = &self.extra_setup_dir { + params.push((PlaceHolder::ExtraSetupDir, dunce::canonicalize(dir).unwrap().to_string_lossy().to_string())); + } + if let Some(dir) = &self.extra_output { + params.push((PlaceHolder::ExtraOutputDir, dunce::canonicalize(&dir.local_path).unwrap().to_string_lossy().to_string())); + } + + params + } + } + proptest! { - // generate an arbitrary config - // map the expanded values from the config to their expander names - // verify that the get_expand() result has all the same values as are in the config #[test] fn test_get_expand_values_match_config( config in any::(), ) { let expand = config.get_expand(); - - // for now, use a hardcoded list of parameter names that the config supplies - let mut params = HashMap::from([ - ("machine_id", config.machine_identity.machine_id.to_string()), - ("job_id", config.job_id.to_string()), - ("task_id", config.task_id.to_string()), - ("setup_dir", dunce::canonicalize(config.setup_dir.clone()).unwrap().to_string_lossy().to_string()), - ]); - if let Some(key) = &config.instance_telemetry_key { - params.insert("instance_telemetry_key", key.to_string()); - } - if let Some(key) = &config.microsoft_telemetry_key { - params.insert("microsoft_telemetry_key", key.clone().to_string()); - } - if let Some(dir) = &config.extra_setup_dir { - params.insert("extra_setup_dir", dunce::canonicalize(dir).unwrap().to_string_lossy().to_string()); - } - if let Some(dir) = &config.extra_output { - params.insert("extra_output_dir", dunce::canonicalize(&dir.local_path).unwrap().to_string_lossy().to_string()); - } + let params = config.get_expand_fields(); for (param, expected) in params.iter() { - let evaluated = expand.evaluate_value(format!("{{{param}}}")).unwrap(); + let evaluated = expand.evaluate_value(format!("{}", param.get_string())).unwrap(); assert_eq!(evaluated, *expected); } }