diff --git a/site/src/comparison.rs b/site/src/comparison.rs index d9620e810..b92c76e90 100644 --- a/site/src/comparison.rs +++ b/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, @@ -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::>() - .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, + /// A map from benchmark name to an error which occured when building `b` but not `a`. pub new_errors: HashMap, } @@ -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, + category_map: HashMap, ) -> (Option, Option) { 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); } diff --git a/site/src/github.rs b/site/src/github.rs index 9326e912f..99c5767be 100644 --- a/site/src/github.rs +++ b/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::>() + .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