Always show errors when present

Previously when there were no relevant changes to show, we didn't show
benchmark errors. However, it always makes sense to show new benchmark
errors.

This change makes it so that new benchmark errors are always shown when
present.

Author: rylev

site/src/comparison.rs

@@ -125,7 +125,10 @@ pub async fn handle_compare(
         })
         .collect();
 
-    let mut new_errors = comparison.new_errors.into_iter().collect::<Vec<_>>();
+    let mut new_errors = comparison
+        .newly_failed_benchmarks
+        .into_iter()
+        .collect::<Vec<_>>();
     new_errors.sort();
     Ok(api::comparison::Response {
         prev,
@@ -172,8 +175,6 @@ pub struct ComparisonSummary {
     num_improvements: usize,
     /// The cached number of comparisons that are regressions
     num_regressions: usize,
-    /// Which benchmarks had errors
-    errors_in: Vec<String>,
 }
 
 impl ComparisonSummary {
@@ -207,13 +208,10 @@ impl ComparisonSummary {
         };
         comparisons.sort_by(cmp);
 
-        let errors_in = comparison.new_errors.keys().cloned().collect::<Vec<_>>();
-
         Some(ComparisonSummary {
             comparisons,
             num_improvements,
             num_regressions,
-            errors_in,
         })
     }
 
@@ -222,7 +220,6 @@ impl ComparisonSummary {
             comparisons: vec![],
             num_improvements: 0,
             num_regressions: 0,
-            errors_in: vec![],
         }
     }
 
@@ -319,10 +316,6 @@ impl ComparisonSummary {
         self.comparisons.is_empty()
     }
 
-    pub fn errors_in(&self) -> &[String] {
-        &self.errors_in
-    }
-
     fn arithmetic_mean<'a>(
         &'a self,
         changes: impl Iterator<Item = &'a TestResultComparison>,
@@ -607,7 +600,7 @@ async fn compare_given_commits(
         a: ArtifactDescription::for_artifact(&*conn, a.clone(), master_commits).await,
         b: ArtifactDescription::for_artifact(&*conn, b.clone(), master_commits).await,
         statistics,
-        new_errors: errors_in_b.into_iter().collect(),
+        newly_failed_benchmarks: errors_in_b.into_iter().collect(),
     }))
 }
 
@@ -755,7 +748,7 @@ pub struct Comparison {
     /// Statistics based on test case
     pub statistics: HashSet<TestResultComparison>,
     /// A map from benchmark name to an error which occured when building `b` but not `a`.
-    pub new_errors: HashMap<String, String>,
+    pub newly_failed_benchmarks: HashMap<String, String>,
 }
 
 impl Comparison {
@@ -798,21 +791,21 @@ impl Comparison {
             .partition(|s| category_map.get(&s.benchmark()) == Some(&Category::Primary));
 
         let (primary_errors, secondary_errors) = self
-            .new_errors
+            .newly_failed_benchmarks
             .into_iter()
             .partition(|(b, _)| category_map.get(&b.as_str().into()) == Some(&Category::Primary));
 
         let primary = Comparison {
             a: self.a.clone(),
             b: self.b.clone(),
             statistics: primary,
-            new_errors: primary_errors,
+            newly_failed_benchmarks: primary_errors,
         };
         let secondary = Comparison {
             a: self.a,
             b: self.b,
             statistics: secondary,
-            new_errors: secondary_errors,
+            newly_failed_benchmarks: secondary_errors,
         };
         (
             ComparisonSummary::summarize_comparison(&primary),
@@ -1516,7 +1509,7 @@ mod tests {
                 bootstrap_total: 0,
             },
             statistics,
-            new_errors: Default::default(),
+            newly_failed_benchmarks: Default::default(),
         };
         ComparisonSummary::summarize_comparison(&comparison)
             .unwrap_or_else(|| ComparisonSummary::empty())

site/src/github.rs

@@ -666,6 +666,21 @@ async fn categorize_benchmark(
         _ => return (String::from("ERROR categorizing benchmark run!"), None),
     };
 
+    let errors = if !comparison.newly_failed_benchmarks.is_empty() {
+        let benchmarks = comparison
+            .newly_failed_benchmarks
+            .iter()
+            .map(|(benchmark, _)| format!("- {benchmark}"))
+            .collect::<Vec<_>>()
+            .join("\n");
+        format!(
+            "\n**Warning ⚠**: The following benchmark(s) failed to build:\n{}\n",
+            benchmarks,
+        )
+    } else {
+        String::new()
+    };
+
     let benchmark_map = ctxt.get_benchmark_category_map().await;
     let (primary, secondary) = comparison.summarize_by_category(benchmark_map);
 
@@ -675,8 +690,8 @@ async fn categorize_benchmark(
     if primary.is_none() && secondary.is_none() {
         return (
             format!(
-                "This benchmark run did not return any relevant results.\n\n{}",
-                DISAGREEMENT
+                "This benchmark run did not return any relevant results.\n\n{}{}",
+                DISAGREEMENT, errors
             ),
             None,
         );
@@ -701,27 +716,9 @@ async fn categorize_benchmark(
             secondary.unwrap_or_else(|| ComparisonSummary::empty()),
         );
         write_summary_table(&primary, &secondary, true, &mut result);
-
-        if !primary.errors_in().is_empty() || !secondary.errors_in().is_empty() {
-            let list_errored_benchmarks = |summary: ComparisonSummary| {
-                summary
-                    .errors_in()
-                    .iter()
-                    .map(|benchmark| format!("- {benchmark}"))
-                    .collect::<Vec<_>>()
-                    .join("\n")
-            };
-            write!(
-                result,
-                "\nThe following benchmark(s) failed to build:\n{}{}\n",
-                list_errored_benchmarks(primary),
-                list_errored_benchmarks(secondary)
-            )
-            .unwrap();
-        }
     }
 
-    write!(result, "\n{}", DISAGREEMENT).unwrap();
+    write!(result, "\n{} {}", DISAGREEMENT, errors).unwrap();
 
     let direction = primary_direction.or(secondary_direction);
     (result, direction)