Merge pull request #1274 from rylev/improve-table

Improve summary table and GitHub messaging

Author: rylev

site/src/comparison.rs

@@ -155,7 +155,7 @@ async fn populate_report(
                     .entry(confidence.is_definitely_relevant().then(|| direction))
                     .or_default();
 
-                entry.push(write_summary(ctxt, comparison).await)
+                entry.push(write_triage_summary(ctxt, comparison).await)
             }
         }
     }
@@ -320,6 +320,10 @@ 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>,
@@ -372,7 +376,7 @@ impl ComparisonSummary {
     }
 }
 
-async fn write_summary(ctxt: &SiteCtxt, comparison: &Comparison) -> String {
+async fn write_triage_summary(ctxt: &SiteCtxt, comparison: &Comparison) -> String {
     use std::fmt::Write;
     let mut result = if let Some(pr) = comparison.b.pr {
         let title = github::pr_title(pr).await;
@@ -393,7 +397,7 @@ async fn write_summary(ctxt: &SiteCtxt, comparison: &Comparison) -> String {
     let primary = primary.unwrap_or_else(ComparisonSummary::empty);
     let secondary = secondary.unwrap_or_else(ComparisonSummary::empty);
 
-    write_summary_table(&primary, &secondary, &mut result);
+    write_summary_table(&primary, &secondary, false, &mut result);
 
     result
 }
@@ -402,6 +406,7 @@ async fn write_summary(ctxt: &SiteCtxt, comparison: &Comparison) -> String {
 pub fn write_summary_table(
     primary: &ComparisonSummary,
     secondary: &ComparisonSummary,
+    with_footnotes: bool,
     result: &mut String,
 ) {
     use std::fmt::Write;
@@ -421,7 +426,8 @@ pub fn write_summary_table(
     .unwrap();
     writeln!(
         result,
-        "| count[^1] | {} | {} | {} | {} | {} |",
+        "| count{} | {} | {} | {} | {} | {} |",
+        if with_footnotes { "[^1]" } else { "" },
         primary.num_regressions,
         secondary.num_regressions,
         primary.num_improvements,
@@ -432,7 +438,8 @@ pub fn write_summary_table(
 
     writeln!(
         result,
-        "| mean[^2] | {} | {} | {} | {} | {} |",
+        "| mean{} | {} | {} | {} | {} | {} |",
+        if with_footnotes { "[^2]" } else { "" },
         render_stat(primary.num_regressions, || Some(
             primary.arithmetic_mean_of_regressions()
         )),
@@ -497,27 +504,15 @@ pub fn write_summary_table(
     )
     .unwrap();
 
-    if !primary.errors_in.is_empty() {
-        write!(
+    if with_footnotes {
+        writeln!(
             result,
-            "\nThe following benchmark(s) failed to build:\n{}\n",
-            primary
-                .errors_in
-                .iter()
-                .map(|benchmark| format!("- {benchmark}"))
-                .collect::<Vec<_>>()
-                .join("\n")
+            r#"
+[^1]: *number of relevant changes*
+[^2]: *the arithmetic mean of the percent change*"#
         )
         .unwrap();
     }
-
-    writeln!(
-        result,
-        r#"
-[^1]: *number of relevant changes*
-[^2]: *the arithmetic mean of the percent change*"#
-    )
-    .unwrap();
 }
 
 /// The amount of confidence we have that a comparison actually represents a real
@@ -760,6 +755,7 @@ pub struct Comparison {
     pub b: ArtifactDescription,
     /// 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>,
 }
 
@@ -795,24 +791,29 @@ impl Comparison {
     /// Splits comparison into primary and secondary summaries based on benchmark category
     pub fn summarize_by_category(
         self,
-        map: HashMap<Benchmark, Category>,
+        category_map: HashMap<Benchmark, Category>,
     ) -> (Option<ComparisonSummary>, Option<ComparisonSummary>) {
         let (primary, secondary) = self
             .statistics
             .into_iter()
-            .partition(|s| map.get(&s.benchmark()) == Some(&Category::Primary));
+            .partition(|s| category_map.get(&s.benchmark()) == Some(&Category::Primary));
+
+        let (primary_errors, secondary_errors) = self
+            .new_errors
+            .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: self.new_errors.clone(),
+            new_errors: primary_errors,
         };
         let secondary = Comparison {
             a: self.a,
             b: self.b,
             statistics: secondary,
-            new_errors: self.new_errors,
+            new_errors: secondary_errors,
         };
         (
             ComparisonSummary::summarize_comparison(&primary),
@@ -1275,16 +1276,13 @@ fn compare_link(start: &ArtifactId, end: &ArtifactId) -> String {
 
 #[cfg(test)]
 mod tests {
+    use super::*;
+
     use collector::category::Category;
     use std::collections::HashSet;
 
     use database::{ArtifactId, Profile, Scenario};
 
-    use crate::comparison::{
-        write_summary_table, ArtifactDescription, Comparison, ComparisonSummary,
-        TestResultComparison,
-    };
-
     #[test]
     fn summary_table_only_regressions_primary() {
         check_table(
@@ -1487,7 +1485,7 @@ mod tests {
         let secondary = create_summary(secondary_statistics);
 
         let mut result = String::new();
-        write_summary_table(&primary, &secondary, &mut result);
+        write_summary_table(&primary, &secondary, true, &mut result);
         assert_eq!(result, expected);
     }
 

site/src/github.rs

@@ -700,7 +700,25 @@ async fn categorize_benchmark(
             primary.unwrap_or_else(|| ComparisonSummary::empty()),
             secondary.unwrap_or_else(|| ComparisonSummary::empty()),
         );
-        write_summary_table(&primary, &secondary, &mut result);
+        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();
@@ -743,7 +761,7 @@ fn generate_short_summary(
             } else {
                 (
                     format!(
-                        "no relevant changes found. {} results were found to be statistically significant but too small to be relevant.",
+                        "changes not relevant. {} results were found to be statistically significant but the changes were too small to be relevant.",
                         summary.num_significant_changes(),
                     ),
                     None