diff --git a/src/analytics/mod.rs b/src/analytics/mod.rs index 235732a9..68f5a513 100644 --- a/src/analytics/mod.rs +++ b/src/analytics/mod.rs @@ -169,6 +169,12 @@ fn compute_finding_score(value: f64, threshold: f64, rule_score: f64) -> f64 { }; } + // Value being 100% less than the threshold should be treated the same as + // being 100% greater than the threshold + if value == 0.0 { + return 2.0 * rule_score; + } + let mut delta = value / threshold; if delta < 1.0 { delta = delta.recip(); @@ -253,3 +259,82 @@ macro_rules! multi_data_analytical_rules { // Register all multi-data-type rule templates here multi_data_analytical_rules!(); + +#[cfg(test)] +mod tests { + use super::*; + + // --- value == 0.0 cases --- + + #[test] + fn test_value_zero_positive_score() { + // value=0 with non-zero threshold should return 2.0 * rule_score + let result = compute_finding_score(0.0, 10.0, 5.0); + assert_eq!(result, 10.0); // 2.0 * 5.0 + } + + #[test] + fn test_value_zero_negative_score() { + let result = compute_finding_score(0.0, 50.0, -2.0); + assert_eq!(result, -4.0); // 2.0 * -2.0 + } + + #[test] + fn test_value_zero_zero_score() { + let result = compute_finding_score(0.0, 100.0, 0.0); + assert_eq!(result, 0.0); // 2.0 * 0.0 + } + + // --- threshold == 0.0 cases --- + + #[test] + fn test_threshold_zero_value_less_than_one() { + // threshold=0, value<1 => rule_score + let result = compute_finding_score(0.5, 0.0, 3.0); + assert_eq!(result, 3.0); + } + + #[test] + fn test_threshold_zero_value_greater_than_one() { + // threshold=0, value>=1 => (value - 1.0) * rule_score + let result = compute_finding_score(5.0, 0.0, 3.0); + assert_eq!(result, 12.0); // (5.0 - 1.0) * 3.0 + } + + #[test] + fn test_threshold_zero_value_exactly_one() { + // threshold=0, value==1.0 => (1.0 - 1.0) * rule_score = 0 + let result = compute_finding_score(1.0, 0.0, 3.0); + assert_eq!(result, 0.0); + } + + #[test] + fn test_both_value_and_threshold_zero() { + // threshold==0 branch is checked first; value=0.0 < 1.0 => rule_score + let result = compute_finding_score(0.0, 0.0, 7.0); + assert_eq!(result, 7.0); + } + + // --- normal delta cases --- + + #[test] + fn test_value_above_threshold() { + // delta = 10/5 = 2.0, >= 1 so no recip => 2.0 * 3.0 = 6.0 + let result = compute_finding_score(10.0, 5.0, 3.0); + assert_eq!(result, 6.0); + } + + #[test] + fn test_value_below_threshold() { + // delta = 2/10 = 0.2, < 1 so recip => 5.0 * 3.0 = 15.0 + let result = compute_finding_score(2.0, 10.0, 3.0); + assert_eq!(result, 15.0); + } + + #[test] + fn test_value_equals_threshold() { + // delta = 1.0, not < 1 => 1.0 * rule_score + let result = compute_finding_score(5.0, 5.0, 4.0); + assert_eq!(result, 4.0); + } +} diff --git a/src/lib.rs b/src/lib.rs index 8bad0495..3a469f2c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -18,7 +18,6 @@ use crate::data::common::processed_data_accessor::ProcessedDataAccessor; use crate::visualizer::DataVisualizer; use anyhow::Result; use chrono::prelude::*; -use data::common; use data::common::utils::get_data_name_from_type; use log::{debug, error, info}; use serde::{Deserialize, Serialize}; @@ -109,9 +108,6 @@ pub enum PDError { #[error("The report {0} already exists in current directory.")] ReportExists(String), - #[error("The directory within the archive does not have the same name as the archive: {0}")] - ArchiveDirectoryInvalidName(String), - #[error("Invalid directory {0:?}")] InvalidDirectory(PathBuf), @@ -457,18 +453,18 @@ impl VisualizationData { pub fn init_visualizers( &mut self, + run_name: String, run_data_dir: PathBuf, tmp_dir: &Path, report_dir: &Path, - ) -> Result { - let run_name = common::utils::no_tar_gz_file_name(&run_data_dir).unwrap(); + ) -> Result<()> { let visualizers_len = self.visualizers.len(); let mut error_count = 0; for data_visualizer in self.visualizers.values_mut() { if let Err(e) = data_visualizer.init_visualizer( - run_data_dir.clone(), run_name.clone(), + run_data_dir.clone(), tmp_dir, report_dir, ) { @@ -481,7 +477,8 @@ impl VisualizationData { if error_count == visualizers_len { return Err(PDError::InvalidRunData.into()); } - Ok(run_name.clone()) + + Ok(()) } pub fn add_visualizer(&mut self, data_visualizer: DataVisualizer) { @@ -489,9 +486,9 @@ impl VisualizationData { .insert(data_visualizer.data_name.to_string(), data_visualizer); } - pub fn process_raw_data(&mut self, name: String) -> Result<()> { + pub fn process_raw_data(&mut self) -> Result<()> { for data_visualizer in self.visualizers.values_mut() { - if let Err(e) = data_visualizer.process_raw_data(name.clone()) { + if let Err(e) = data_visualizer.process_raw_data() { error!( "Error while processing {} raw data: {:#?}", data_visualizer.data_name, e diff --git a/src/report.rs b/src/report.rs index dce4c58c..f335241f 100644 --- a/src/report.rs +++ b/src/report.rs @@ -3,13 +3,13 @@ use crate::data::common::processed_data_accessor::ProcessedDataAccessor; use crate::data::common::utils::no_tar_gz_file_name; use crate::data::JS_DIR; use crate::{data, PDError, VisualizationData}; -use anyhow::Result; +use anyhow::{Context, Result}; use chrono::Utc; use clap::Args; use flate2::{read::GzDecoder, write::GzEncoder, Compression}; -use log::info; +use log::{info, warn}; use serde::Serialize; -use std::collections::{HashMap, HashSet}; +use std::collections::HashMap; use std::fs::File; use std::io::Write; use std::path::PathBuf; @@ -35,10 +35,12 @@ impl VersionInfo { struct RunsInfo { /// The list of run names (the run dir/archive file name minus the file format) run_names: Vec, - /// The list of paths to the run archives (run data tar files) - run_archive_paths: Vec, - /// The list of paths to the run data directory (where data are available for read and processing) - run_dir_paths: Vec, + /// The index used to deduplicate each unique run name + run_name_dedup_indices: HashMap, + /// The map from run names to paths to the run archives (run data tar files) + run_archive_paths: HashMap, + /// The map from run names to paths to the run data directory (where data are available for read and processing) + run_dir_paths: HashMap, /// The specified start time of every run's time range per_run_from_time: HashMap, /// The specified end time of every run's time range @@ -56,17 +58,41 @@ impl RunsInfo { run_archive_path: PathBuf, run_dir_path: PathBuf, ) -> Result<()> { - if self.run_names.contains(&run_name) { - return Err(PDError::DuplicateRunNames(run_name).into()); - } - - self.run_names.push(run_name); - self.run_archive_paths.push(run_archive_path); - self.run_dir_paths.push(run_dir_path); + let deduped_run_name = self.deduplicate_run_name(run_name); + self.run_names.push(deduped_run_name.clone()); + self.run_archive_paths + .insert(deduped_run_name.clone(), run_archive_path); + self.run_dir_paths.insert(deduped_run_name, run_dir_path); Ok(()) } + fn deduplicate_run_name(&mut self, run_name: String) -> String { + if !self.run_name_dedup_indices.contains_key(&run_name) { + self.run_name_dedup_indices.insert(run_name.clone(), 1); + return run_name; + } + // Any duplicate run name will be deduped to original run name appended with the first + // unused index + loop { + let deduped_run_name = format!( + "{}_{}", + run_name, + self.run_name_dedup_indices.get(&run_name).unwrap() + ); + if self.run_name_dedup_indices.contains_key(&deduped_run_name) { + *self.run_name_dedup_indices.get_mut(&run_name).unwrap() += 1; + } else { + self.run_name_dedup_indices + .insert(deduped_run_name.clone(), 1); + warn!( + "Duplicate run names detected. Renaming run {run_name} to {deduped_run_name}." + ); + return deduped_run_name; + } + } + } + fn process_per_run_time_range( &mut self, run_time_ranges: &Vec<(String, Option, Option)>, @@ -180,8 +206,6 @@ pub fn report(report: &Report, tmp_dir: &PathBuf) -> Result<()> { return Err(PDError::ReportExists(report_path_str).into()); } - check_duplicate_runs(&report.run)?; - let mut runs_info = RunsInfo::new(); for run in &report.run { @@ -265,21 +289,6 @@ fn parse_time_range(s: &str) -> Result<(String, Option, Option), Strin Ok((run_name.to_string(), from, to)) } -/// Checks if the list of runs include duplicates -pub fn check_duplicate_runs(run_args: &Vec) -> Result<()> { - let mut unique_runs: HashSet = HashSet::new(); - - for run in run_args { - let run_name = no_tar_gz_file_name(&PathBuf::from(run)) - .ok_or(PDError::InvalidArchive(PathBuf::from(run)))?; - if !unique_runs.insert(run_name.clone()) { - return Err(PDError::DuplicateRunNames(run_name).into()); - } - } - - Ok(()) -} - /// Creates (tar) an archive in the temporary directory pub fn create_archive(dir_path: &PathBuf, tmp_dir: &PathBuf) -> Result { if !dir_path.is_dir() { @@ -307,17 +316,35 @@ pub fn extract_archive(archive_path: &PathBuf, tmp_dir: &PathBuf) -> Result dir_name, - None => return Err(PDError::InvalidArchive(archive_path.clone()).into()), - }; + let extracted_dir_path = tmp_dir.join(&dir_name); if !extracted_dir_path.exists() { - return Err(PDError::ArchiveDirectoryInvalidName(dir_name).into()); + return Err(PDError::InvalidDirectory(extracted_dir_path).into()); } Ok(extracted_dir_path) @@ -366,11 +393,11 @@ fn generate_report_files(report_dir: PathBuf, runs_info: RunsInfo, tmp_dir: &Pat let mut visualization_data = VisualizationData::new(); data::add_all_visualization_data(&mut visualization_data); /* Init visualizers */ - for run_data_dir in runs_info.run_dir_paths { - let run_name = visualization_data - .init_visualizers(run_data_dir.clone(), tmp_dir, &report_dir) + for (run_name, run_data_dir) in runs_info.run_dir_paths { + visualization_data + .init_visualizers(run_name.clone(), run_data_dir.clone(), tmp_dir, &report_dir) .unwrap(); - visualization_data.process_raw_data(run_name).unwrap(); + visualization_data.process_raw_data().unwrap(); } let mut processed_data_accessor = ProcessedDataAccessor::from_time_ranges( @@ -432,9 +459,8 @@ fn generate_report_files(report_dir: PathBuf, runs_info: RunsInfo, tmp_dir: &Pat /* Copy all run archives into the report's data/archives path */ let report_run_archives_dir = report_data_dir.join("archive"); - for run_archive_source_path in runs_info.run_archive_paths { - let run_archive_dest_path = - report_run_archives_dir.join(run_archive_source_path.file_name().unwrap()); + for (run_name, run_archive_source_path) in runs_info.run_archive_paths { + let run_archive_dest_path = report_run_archives_dir.join(format!("{run_name}.tar.gz")); info!("Copying archive to {:?}", run_archive_dest_path); fs::copy(run_archive_source_path, run_archive_dest_path).unwrap(); } diff --git a/src/visualizer.rs b/src/visualizer.rs index be05228a..863ee318 100644 --- a/src/visualizer.rs +++ b/src/visualizer.rs @@ -51,11 +51,12 @@ impl DataVisualizer { pub fn init_visualizer( &mut self, - run_data_dir: PathBuf, run_name: String, + run_data_dir: PathBuf, tmp_dir: &Path, report_dir: &Path, ) -> Result<()> { + self.report_params.run_name = run_name.clone(); let (file_path, file) = get_file(&run_data_dir, self.data_name.to_string()).or_else(|e| { // Backward compatibility: if file is not found using the data's name, @@ -78,19 +79,26 @@ impl DataVisualizer { self.report_params.data_dir = run_data_dir; self.report_params.tmp_dir = tmp_dir.to_path_buf(); self.report_params.report_dir = report_dir.to_path_buf(); - self.report_params.run_name = run_name.clone(); self.report_params.data_file_path = file_path; self.file_handle = Some(file); self.data_available.insert(run_name, true); Ok(()) } - pub fn process_raw_data(&mut self, run_name: String) -> Result<()> { - if !self.data_available.get(&run_name).unwrap() { + pub fn process_raw_data(&mut self) -> Result<()> { + debug!( + "Processing raw {} data in {}", + self.data_name, self.report_params.run_name + ); + + if !self + .data_available + .get(&self.report_params.run_name) + .unwrap() + { debug!("Raw data unavailable for: {}", self.data_name); return Ok(()); } - debug!("Processing raw {} data in {}", self.data_name, run_name); self.file_handle .as_ref() @@ -129,7 +137,9 @@ impl DataVisualizer { .data .process_raw_data(self.report_params.clone(), raw_data)?; self.processed_data.data_format = processed_data.get_format_name(); - self.processed_data.runs.insert(run_name, processed_data); + self.processed_data + .runs + .insert(self.report_params.run_name.clone(), processed_data); Ok(()) } diff --git a/tests/test_aperf.rs b/tests/test_aperf.rs index 2a9b8200..10a846c9 100644 --- a/tests/test_aperf.rs +++ b/tests/test_aperf.rs @@ -39,7 +39,6 @@ where } #[test] -#[serial] #[cfg(feature = "hotline")] fn test_hotline() { unsafe { @@ -49,7 +48,6 @@ fn test_hotline() { #[cfg(target_os = "linux")] #[test] -#[serial] fn test_record() { run_test(|work_dir, tmp_dir| { let run_name = @@ -80,7 +78,6 @@ fn test_record() { #[cfg(target_os = "linux")] #[test] -#[serial] fn test_record_dont_collect_some_data() { run_test(|work_dir, tmp_dir| { let dont_collect_data_names = vec![ @@ -132,7 +129,6 @@ fn test_record_dont_collect_some_data() { #[cfg(target_os = "linux")] #[test] -#[serial] fn test_record_collect_only_some_data() { run_test(|work_dir, tmp_dir| { let collect_only_data_names = vec![ @@ -190,7 +186,6 @@ fn test_record_collect_only_some_data() { #[cfg(target_os = "linux")] #[test] -#[serial] fn test_record_and_report() { run_test(|work_dir, tmp_dir| { let run_name = String::from("test_record_run"); @@ -221,7 +216,6 @@ fn test_record_and_report() { #[cfg(target_os = "linux")] #[test] -#[serial] fn test_record_and_report_dot_in_run_name() { run_test(|work_dir, tmp_dir| { let run_name = String::from("test.record.data"); @@ -251,7 +245,6 @@ fn test_record_and_report_dot_in_run_name() { } #[test] -#[serial] fn test_report_with_empty_data_bin() { run_test(|work_dir, tmp_dir| { let run_name = String::from("empty_data_bin"); @@ -300,7 +293,6 @@ fn test_report_with_empty_data_bin() { } #[test] -#[serial] fn test_report_single_run() { run_test(|work_dir, tmp_dir| { let run_name = String::from("test_run_1"); @@ -329,7 +321,6 @@ fn test_report_single_run() { } #[test] -#[serial] fn test_report_multiple_runs() { run_test(|work_dir, tmp_dir| { let run_name_1 = String::from("test_run_1"); @@ -366,7 +357,6 @@ fn test_report_multiple_runs() { } #[test] -#[serial] fn test_report_from_report() { run_test(|work_dir, tmp_dir| { let input_report_path = get_test_data_path("test_report.tar.gz"); @@ -407,7 +397,155 @@ fn test_report_from_report() { } #[test] -#[serial] +fn test_report_renamed_run() { + run_test(|work_dir, tmp_dir| { + let run_name = String::from("renamed_test_run"); + let run_path = get_test_data_path(format!("{}.tar.gz", run_name)); + + let report_name = String::from("renamed_run_report"); + let rep = Report { + run: vec![run_path.into_os_string().into_string().unwrap()], + name: Some( + work_dir + .join(&report_name) + .into_os_string() + .into_string() + .unwrap(), + ), + time_range: vec![], + }; + assert!(report(&rep, &tmp_dir).is_ok()); + + verify_report_structure(&work_dir, &report_name, vec![run_name]); + + clean_dir_and_archive(&work_dir, &report_name); + + Ok(()) + }) +} + +#[test] +fn test_duplicate_run_names() { + run_test(|work_dir, tmp_dir| { + let input_report_path = get_test_data_path("test_report.tar.gz"); + let duplicate_run_name = String::from("test_run_1"); + let duplicate_run_path = get_test_data_path(format!("{}.tar.gz", duplicate_run_name)); + + let report_name = String::from("test_report"); + let report_dir_path = work_dir.join(&report_name); + let rep = Report { + run: vec![ + input_report_path + .clone() + .into_os_string() + .into_string() + .unwrap(), + duplicate_run_path + .clone() + .into_os_string() + .into_string() + .unwrap(), + ], + name: Some( + report_dir_path + .clone() + .into_os_string() + .into_string() + .unwrap(), + ), + time_range: vec![], + }; + + assert!(report(&rep, &tmp_dir).is_ok()); + + verify_report_structure( + &work_dir, + &report_name, + vec![ + String::from("test_run_1"), + String::from("test_run_2"), + String::from("test_run_1_1"), + ], + ); + + let rev_report_name = String::from("test_report_rev"); + let rev_report_dir_path = work_dir.join(&rev_report_name); + let rev_rep = Report { + run: vec![ + duplicate_run_path + .clone() + .into_os_string() + .into_string() + .unwrap(), + input_report_path + .clone() + .into_os_string() + .into_string() + .unwrap(), + ], + name: Some( + rev_report_dir_path + .clone() + .into_os_string() + .into_string() + .unwrap(), + ), + time_range: vec![], + }; + + assert!(report(&rev_rep, &tmp_dir).is_ok()); + + verify_report_structure( + &work_dir, + &rev_report_name, + vec![ + String::from("test_run_1"), + String::from("test_run_1_1"), + String::from("test_run_2"), + ], + ); + + let combined_report_name = String::from("combined"); + let combined_report_dir_path = work_dir.join(&combined_report_name); + let combined_rep = Report { + run: vec![ + report_dir_path.into_os_string().into_string().unwrap(), + rev_report_dir_path.into_os_string().into_string().unwrap(), + ], + name: Some( + combined_report_dir_path + .clone() + .into_os_string() + .into_string() + .unwrap(), + ), + time_range: vec![], + }; + + assert!(report(&combined_rep, &tmp_dir).is_ok()); + + verify_report_structure( + &work_dir, + &combined_report_name, + vec![ + String::from("test_run_1"), + String::from("test_run_1_1"), + String::from("test_run_2"), + String::from("test_run_1_2"), + String::from("test_run_1_1_1"), + String::from("test_run_2_1"), + ], + ); + + clean_dir_and_archive(&work_dir, &report_name); + clean_dir_and_archive(&work_dir, &rev_report_name); + clean_dir_and_archive(&work_dir, &combined_report_name); + + Ok(()) + }) +} + +#[test] fn test_report_with_time_range() { run_test(|work_dir, tmp_dir| { let run_name = String::from("test_run_1"); @@ -451,7 +589,6 @@ fn test_report_with_time_range() { } #[test] -#[serial] fn test_report_already_exists() { run_test(|work_dir, tmp_dir| { let run_name = String::from("test_run_3"); @@ -494,7 +631,6 @@ fn test_report_already_exists() { } #[test] -#[serial] fn test_run_data_not_exists() { run_test(|work_dir, tmp_dir| { let run_name = String::from("fake_run"); @@ -528,92 +664,6 @@ fn test_run_data_not_exists() { } #[test] -#[serial] -fn test_duplicate_run_data() { - run_test(|work_dir, tmp_dir| { - let input_report_path = get_test_data_path("test_report.tar.gz"); - let duplicate_run_name = String::from("test_run_1"); - let duplicate_run_path = get_test_data_path(format!("{}.tar.gz", duplicate_run_name)); - - let report_name = String::from("report_with_duplicate_data"); - let report_dir_path = work_dir.join(&report_name); - let report_archive_path = work_dir.join(format!("{}.tar.gz", report_name)); - let rep = Report { - run: vec![ - input_report_path.into_os_string().into_string().unwrap(), - duplicate_run_path.into_os_string().into_string().unwrap(), - ], - name: Some( - report_dir_path - .clone() - .into_os_string() - .into_string() - .unwrap(), - ), - time_range: vec![], - }; - - let error = report(&rep, &tmp_dir).unwrap_err(); - assert_eq!( - error.to_string(), - format!("Multiple runs with the same name: {}", duplicate_run_name) - ); - - assert!(!report_dir_path.exists()); - assert!(!report_archive_path.exists()); - - Ok(()) - }) -} - -#[test] -#[serial] -fn test_duplicate_run_data_quick_fail() { - run_test(|work_dir, tmp_dir| { - let duplicate_run_name = String::from("test_run_2"); - let duplicate_run_path = get_test_data_path(format!("{}.tar.gz", duplicate_run_name)); - - let report_name = String::from("report_not_to_be_generated"); - let report_dir_path = work_dir.join(&report_name); - let report_archive_path = work_dir.join(format!("{}.tar.gz", report_name)); - let rep = Report { - run: vec![ - duplicate_run_path - .clone() - .into_os_string() - .into_string() - .unwrap(), - duplicate_run_path - .clone() - .into_os_string() - .into_string() - .unwrap(), - ], - name: Some( - report_dir_path - .clone() - .into_os_string() - .into_string() - .unwrap(), - ), - time_range: vec![], - }; - - let error = report(&rep, &tmp_dir).unwrap_err(); - assert_eq!( - error.to_string(), - format!("Multiple runs with the same name: {}", duplicate_run_name) - ); - - assert!(!report_dir_path.exists()); - assert!(!report_archive_path.exists()); - - Ok(()) - }) -} - -#[test] -#[serial] fn test_report_with_time_range_invalid_run_name() { run_test(|work_dir, tmp_dir| { let run_name = String::from("test_run_1"); @@ -651,7 +701,6 @@ fn test_report_with_time_range_invalid_run_name() { #[cfg(target_os = "linux")] #[test] -#[serial] fn test_report_with_time_range_from_greater_than_to() { run_test(|work_dir, tmp_dir| { let run_name = String::from("test_run_1"); diff --git a/tests/test_data/renamed_test_run.tar.gz b/tests/test_data/renamed_test_run.tar.gz new file mode 100644 index 00000000..12bb17b1 Binary files /dev/null and b/tests/test_data/renamed_test_run.tar.gz differ