From 0ecc54956c792ba8a57ecf2a5eba30ff7c8a7cfb Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Mon, 4 Apr 2022 17:38:46 +0200 Subject: [PATCH 1/5] Optionally write footnotes for summary table --- site/src/comparison.rs | 34 ++++++++++++++++++---------------- site/src/github.rs | 2 +- 2 files changed, 19 insertions(+), 17 deletions(-) diff --git a/site/src/comparison.rs b/site/src/comparison.rs index d9620e810..8f226dc8d 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) } } } @@ -372,7 +372,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 +393,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 +402,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 +422,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 +434,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() )), @@ -511,13 +514,15 @@ pub fn write_summary_table( .unwrap(); } - writeln!( - result, - r#" + if with_footnotes { + writeln!( + result, + r#" [^1]: *number of relevant changes* [^2]: *the arithmetic mean of the percent change*"# - ) - .unwrap(); + ) + .unwrap(); + } } /// The amount of confidence we have that a comparison actually represents a real @@ -1275,16 +1280,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 +1489,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..47ed7dcee 100644 --- a/site/src/github.rs +++ b/site/src/github.rs @@ -700,7 +700,7 @@ 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); } write!(result, "\n{}", DISAGREEMENT).unwrap(); From f47640553b543d9a41c08b51ffc1b70abec619b7 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Mon, 4 Apr 2022 17:44:54 +0200 Subject: [PATCH 2/5] Only show errors in GitHub message and show also secondary benchmarks --- site/src/comparison.rs | 18 ++++-------------- site/src/github.rs | 18 ++++++++++++++++++ 2 files changed, 22 insertions(+), 14 deletions(-) diff --git a/site/src/comparison.rs b/site/src/comparison.rs index 8f226dc8d..ba336a998 100644 --- a/site/src/comparison.rs +++ b/site/src/comparison.rs @@ -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, @@ -500,20 +504,6 @@ pub fn write_summary_table( ) .unwrap(); - if !primary.errors_in.is_empty() { - write!( - result, - "\nThe following benchmark(s) failed to build:\n{}\n", - primary - .errors_in - .iter() - .map(|benchmark| format!("- {benchmark}")) - .collect::>() - .join("\n") - ) - .unwrap(); - } - if with_footnotes { writeln!( result, diff --git a/site/src/github.rs b/site/src/github.rs index 47ed7dcee..8f552604c 100644 --- a/site/src/github.rs +++ b/site/src/github.rs @@ -701,6 +701,24 @@ 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::>() + .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(); From 15fd0ddb7f8aa81d5754e220ff767f56a11724a6 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Mon, 4 Apr 2022 17:49:53 +0200 Subject: [PATCH 3/5] Tweak message about not finding relevant changes --- site/src/github.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/github.rs b/site/src/github.rs index 8f552604c..99c5767be 100644 --- a/site/src/github.rs +++ b/site/src/github.rs @@ -761,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 From a04bee4e814ccce08ebaa0bccf13110c80ed5be6 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Mon, 4 Apr 2022 18:43:56 +0200 Subject: [PATCH 4/5] Split up errors --- site/src/comparison.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/site/src/comparison.rs b/site/src/comparison.rs index ba336a998..45c724e5b 100644 --- a/site/src/comparison.rs +++ b/site/src/comparison.rs @@ -755,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 `a`. pub new_errors: HashMap, } @@ -790,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), From fdd2059caa191989236c75307dce35edd14675b6 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Tue, 5 Apr 2022 10:01:07 +0200 Subject: [PATCH 5/5] Fix typo --- site/src/comparison.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/comparison.rs b/site/src/comparison.rs index 45c724e5b..b92c76e90 100644 --- a/site/src/comparison.rs +++ b/site/src/comparison.rs @@ -755,7 +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 `a`. + /// A map from benchmark name to an error which occured when building `b` but not `a`. pub new_errors: HashMap, }