Merge pull request #191 from RReverser/master

Fix directory name escaping

Author: bheisler

src/html/mod.rs

@@ -264,25 +264,27 @@ impl Report for Html {
             .collect::<Vec<_>>();
 
         let mut all_plots = vec![];
-        let group_id = &all_ids[0].group_id;
+        let group_id = all_ids[0].group_id.clone();
+
+        let data: Vec<(BenchmarkId, Vec<f64>)> =
+            self.load_summary_data(&context.output_directory, &all_ids);
 
         let mut function_ids = BTreeSet::new();
-        for id in &all_ids {
-            if let Some(ref function_id) = id.function_id {
+        for id in all_ids {
+            if let Some(function_id) = id.function_id {
                 function_ids.insert(function_id);
             }
         }
 
-        let data: Vec<(BenchmarkId, Vec<f64>)> =
-            self.load_summary_data(&context.output_directory, &all_ids);
-
         for function_id in function_ids {
             let samples_with_function: Vec<_> = data.iter()
                 .by_ref()
-                .filter(|&&(ref id, _)| id.function_id.as_ref() == Some(function_id))
+                .filter(|&&(ref id, _)| id.function_id.as_ref() == Some(&function_id))
                 .collect();
+
             if samples_with_function.len() > 1 {
-                let subgroup_id = format!("{}/{}", group_id, function_id);
+                let subgroup_id = BenchmarkId::new(group_id.clone(), Some(function_id), None, None);
+
                 all_plots.extend(self.generate_summary(
                     &subgroup_id,
                     &*samples_with_function,
@@ -293,7 +295,7 @@ impl Report for Html {
         }
 
         all_plots.extend(self.generate_summary(
-            group_id,
+            &BenchmarkId::new(group_id, None, None, None),
             &*(data.iter().by_ref().collect::<Vec<_>>()),
             context,
             true,
@@ -586,7 +588,7 @@ impl Html {
 
     fn generate_summary(
         &self,
-        group_id: &str,
+        id: &BenchmarkId,
         data: &[&(BenchmarkId, Vec<f64>)],
         report_context: &ReportContext,
         full_summary: bool,
@@ -596,17 +598,19 @@ impl Html {
         try_else_return!(
             fs::mkdirp(&format!(
                 "{}/{}/report/",
-                report_context.output_directory, group_id
+                report_context.output_directory,
+                id.as_directory_name()
             )),
             || gnuplots
         );
 
         let violin_path = format!(
             "{}/{}/report/violin.svg",
-            report_context.output_directory, group_id
+            report_context.output_directory,
+            id.as_directory_name()
         );
         gnuplots.push(plot::summary::violin(
-            group_id,
+            id.id(),
             data,
             &violin_path,
             report_context.plot_config.summary_scale,
@@ -622,11 +626,12 @@ impl Html {
             if let Some(value_type) = value_types[0] {
                 let path = format!(
                     "{}/{}/report/lines.svg",
-                    report_context.output_directory, group_id
+                    report_context.output_directory,
+                    id.as_directory_name()
                 );
 
                 gnuplots.push(plot::summary::line_comparison(
-                    group_id,
+                    id.id(),
                     data,
                     &path,
                     value_type,
@@ -643,7 +648,7 @@ impl Html {
             .collect();
 
         let context = SummaryContext {
-            group_id: group_id.to_owned(),
+            group_id: id.id().to_owned(),
 
             thumbnail_width: THUMBNAIL_SIZE.0,
             thumbnail_height: THUMBNAIL_SIZE.1,
@@ -662,7 +667,8 @@ impl Html {
                 &text,
                 &format!(
                     "{}/{}/report/index.html",
-                    report_context.output_directory, group_id
+                    report_context.output_directory,
+                    id.as_directory_name()
                 ),
             ),
             || gnuplots

src/plot/summary.rs

@@ -79,7 +79,6 @@ pub fn line_comparison(
         .group_by(|&&&(ref id, _)| &id.function_id)
     {
         let mut tuples: Vec<_> = group
-            .into_iter()
             .map(|&&(ref id, ref sample)| {
                 // Unwrap is fine here because it will only fail if the assumptions above are not true
                 // ie. programmer error.

src/report.rs

@@ -75,7 +75,6 @@ impl BenchmarkId {
                 .replace("\"", "_")
                 .replace("/", "_")
                 .replace("\\", "_")
-                .replace(".", "_")
                 .replace("*", "_")
         }
 
@@ -92,7 +91,7 @@ impl BenchmarkId {
             (&None, &Some(ref val)) => {
                 format!("{}/{}", directory_safe(&group_id), directory_safe(val))
             }
-            (&None, &None) => group_id.clone(),
+            (&None, &None) => directory_safe(&group_id),
         };
 
         BenchmarkId {

tests/criterion_tests.rs

@@ -407,15 +407,15 @@ fn test_output_files() {
             "test_output",
             Benchmark::new("output_1", |b| b.iter(|| 10))
                 .with_function("output_2", |b| b.iter(|| 20))
-                .with_function("output_\\/.*\"?", |b| b.iter(|| 30)),
+                .with_function("output_\\/*\"?", |b| b.iter(|| 30)),
         );
     }
 
     // For each benchmark, assert that the expected files are present.
     for x in 0..3 {
         let dir = if x == 2 {
             // Check that certain special characters are replaced with underscores
-            tempdir.path().join(format!("test_output/output_______"))
+            tempdir.path().join(format!("test_output/output______"))
         } else {
             tempdir.path().join(format!("test_output/output_{}", x + 1))
         };