Merge pull request #1276 from rylev/always-show-new-errors

Always show errors when present

Author: rylev

site/src/comparison.rs

@@ -124,7 +124,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,
@@ -171,8 +174,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 {
@@ -206,13 +207,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,
         })
     }
 
@@ -221,7 +219,6 @@ impl ComparisonSummary {
             comparisons: vec![],
             num_improvements: 0,
             num_regressions: 0,
-            errors_in: vec![],
         }
     }
 
@@ -318,10 +315,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>,
@@ -606,7 +599,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(),
     }))
 }
 
@@ -754,7 +747,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 {
@@ -797,21 +790,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),
@@ -1501,7 +1494,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,18 +666,28 @@ 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{benchmarks}\n")
+    } else {
+        String::new()
+    };
+
     let benchmark_map = ctxt.get_benchmark_category_map().await;
     let (primary, secondary) = comparison.summarize_by_category(benchmark_map);
 
     const DISAGREEMENT: &str = "If you disagree with this performance assessment, \
     please file an issue in [rust-lang/rustc-perf](https://github.com/rust-lang/rustc-perf/issues/new).";
+    let footer = format!("{DISAGREEMENT}{errors}");
 
     if primary.is_none() && secondary.is_none() {
         return (
-            format!(
-                "This benchmark run did not return any relevant results.\n\n{}",
-                DISAGREEMENT
-            ),
+            format!("This benchmark run did not return any relevant results.\n\n{footer}"),
             None,
         );
     }
@@ -701,27 +711,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{footer}").unwrap();
 
     let direction = primary_direction.or(secondary_direction);
     (result, direction)