Group diffs by tests, rather than job groups

Author: Kobzol

src/ci/citool/src/merge_report.rs

@@ -1,5 +1,4 @@
-use std::cmp::Reverse;
-use std::collections::HashMap;
+use std::collections::{HashMap,HashSet};
 use std::path::PathBuf;
 
 use anyhow::Context;
@@ -14,10 +13,10 @@ type JobName = String;
 /// Computes a post merge CI analysis report between the `parent` and `current` commits.
 pubfnpost_merge_report(job_db:JobDatabase,parent:Sha,current:Sha) -> anyhow::Result<()>{
 let jobs = download_all_metrics(&job_db,&parent,&current)?;
-letdiffs = aggregate_test_diffs(&jobs)?;
+letaggregated_test_diffs = aggregate_test_diffs(&jobs)?;
 
 println!("Comparing {parent} (base) -> {current} (this PR)\n");
-report_test_changes(diffs);
+report_test_diffs(aggregated_test_diffs);
 
 Ok(())
 }
@@ -95,81 +94,80 @@ fn get_metrics_url(job_name: &str, sha: &str) -> String {
 format!("https://ci-artifacts.rust-lang.org/rustc-builds{suffix}/{sha}/metrics-{job_name}.json")
 }
 
+/// Represents a difference in the outcome of tests between a base and a current commit.
+/// Maps test diffs to jobs that contained them.
+#[derive(Debug)]
+structAggregatedTestDiffs{
+diffs:HashMap<TestDiff,Vec<JobName>>,
+}
+
 fnaggregate_test_diffs(
 jobs:&HashMap<JobName,JobMetrics>,
-) -> anyhow::Result<Vec<AggregatedTestDiffs>>{
-letmutjob_diffs = vec![];
+) -> anyhow::Result<AggregatedTestDiffs>{
+letmutdiffs:HashMap<TestDiff,Vec<JobName>> = HashMap::new();
 
 // Aggregate test suites
 for(name, metrics)in jobs {
 ifletSome(parent) = &metrics.parent{
 let tests_parent = aggregate_tests(parent);
 let tests_current = aggregate_tests(&metrics.current);
-let test_diffs = calculate_test_diffs(tests_parent, tests_current);
-if !test_diffs.is_empty(){
- job_diffs.push((name.clone(), test_diffs));
+for diff incalculate_test_diffs(tests_parent, tests_current){
+ diffs.entry(diff).or_default().push(name.to_string());
 }
 }
 }
 
-// Aggregate jobs with the same diff, as often the same diff will appear in many jobs
-let job_diffs:HashMap<Vec<(Test,TestOutcomeDiff)>,Vec<String>> =
- job_diffs.into_iter().fold(HashMap::new(), |mut acc,(job, diffs)| {
- acc.entry(diffs).or_default().push(job);
- acc
-});
-
-Ok(job_diffs
-.into_iter()
-.map(|(test_diffs, jobs)| AggregatedTestDiffs{ jobs, test_diffs })
-.collect())
+Ok(AggregatedTestDiffs{ diffs })
+}
+
+#[derive(Eq,PartialEq,Hash,Debug)]
+enumTestOutcomeDiff{
+ChangeOutcome{before:TestOutcome,after:TestOutcome},
+Missing{before:TestOutcome},
+Added(TestOutcome),
+}
+
+#[derive(Eq,PartialEq,Hash,Debug)]
+structTestDiff{
+test:Test,
+diff:TestOutcomeDiff,
 }
 
-fncalculate_test_diffs(
-reference:TestSuiteData,
-current:TestSuiteData,
-) -> Vec<(Test,TestOutcomeDiff)>{
-letmut diffs = vec![];
+fncalculate_test_diffs(parent:TestSuiteData,current:TestSuiteData) -> HashSet<TestDiff>{
+letmut diffs = HashSet::new();
 for(test, outcome)in&current.tests{
-matchreference.tests.get(test){
+matchparent.tests.get(test){
 Some(before) => {
 if before != outcome {
- diffs.push((
- test.clone(),
-TestOutcomeDiff::ChangeOutcome{
+ diffs.insert(TestDiff{
+test: test.clone(),
+diff:TestOutcomeDiff::ChangeOutcome{
 before: before.clone(),
 after: outcome.clone(),
 },
-));
+});
 }
 }
-None => diffs.push((test.clone(),TestOutcomeDiff::Added(outcome.clone()))),
+None => {
+ diffs.insert(TestDiff{
+test: test.clone(),
+diff:TestOutcomeDiff::Added(outcome.clone()),
+});
+}
 }
 }
-for(test, outcome)in&reference.tests{
+for(test, outcome)in&parent.tests{
 if !current.tests.contains_key(test){
- diffs.push((test.clone(),TestOutcomeDiff::Missing{before: outcome.clone()}));
+ diffs.insert(TestDiff{
+test: test.clone(),
+diff:TestOutcomeDiff::Missing{before: outcome.clone()},
+});
 }
 }
 
  diffs
 }
 
-/// Represents a difference in the outcome of tests between a base and a current commit.
-#[derive(Debug)]
-structAggregatedTestDiffs{
-/// All jobs that had the exact same test diffs.
-jobs:Vec<String>,
-test_diffs:Vec<(Test,TestOutcomeDiff)>,
-}
-
-#[derive(Eq,PartialEq,Hash,Debug)]
-enumTestOutcomeDiff{
-ChangeOutcome{before:TestOutcome,after:TestOutcome},
-Missing{before:TestOutcome},
-Added(TestOutcome),
-}
-
 /// Aggregates test suite executions from all bootstrap invocations in a given CI job.
 #[derive(Default)]
 structTestSuiteData{
@@ -200,16 +198,13 @@ fn normalize_test_name(name: &str) -> String {
 }
 
 /// Prints test changes in Markdown format to stdout.
-fnreport_test_changes(mutdiffs:Vec<AggregatedTestDiffs>){
+fnreport_test_diffs(mutdiff:AggregatedTestDiffs){
 println!("## Test differences");
-if diffs.is_empty(){
+ifdiff.diffs.is_empty(){
 println!("No test diffs found");
 return;
 }
 
-// Sort diffs in decreasing order by diff count
- diffs.sort_by_key(|entry| Reverse(entry.test_diffs.len()));
-
 fnformat_outcome(outcome:&TestOutcome) -> String{
 match outcome {
 TestOutcome::Passed => "pass".to_string(),
@@ -238,36 +233,51 @@ fn report_test_changes(mut diffs: Vec<AggregatedTestDiffs>) {
 }
 }
 
-let max_diff_count = 10;
-let max_job_count = 5;
-let max_test_count = 10;
-
-for diff in diffs.iter().take(max_diff_count){
-letmut jobs = diff.jobs.clone();
+// It would be quite noisy to repeat the jobs that contained the test changes after/next to
+// every test diff. At the same time, grouping the test diffs by
+// [unique set of jobs that contained them] also doesn't work well, because the test diffs
+// would have to be duplicated several times.
+// Instead, we create a set of unique job groups, and then print a job group after each test.
+// We then print the job groups at the end, as a sort of index.
+letmut grouped_diffs:Vec<(&TestDiff,u64)> = vec![];
+letmut job_list_to_group:HashMap<&[JobName],u64> = HashMap::new();
+letmut job_index:Vec<&[JobName]> = vec![];
+
+for(_, jobs)in diff.diffs.iter_mut(){
  jobs.sort();
+}
 
-let jobs = jobs.iter().take(max_job_count).map(|j| format!("`{j}`")).collect::<Vec<_>>();
-
-let extra_jobs = diff.jobs.len().saturating_sub(max_job_count);
-let suffix = if extra_jobs > 0{
-format!(" (and {extra_jobs} {})", pluralize("other", extra_jobs))
-}else{
-String::new()
+let max_diff_count = 100;
+for(diff, jobs)in diff.diffs.iter().take(max_diff_count){
+let jobs = &*jobs;
+let job_group = match job_list_to_group.get(jobs.as_slice()){
+Some(id) => *id,
+None => {
+let id = job_index.len()asu64;
+ job_index.push(jobs);
+ job_list_to_group.insert(jobs, id);
+ id
+}
 };
-println!("- {}{suffix}", jobs.join(","));
+ grouped_diffs.push((diff, job_group));
+}
 
-let extra_tests = diff.test_diffs.len().saturating_sub(max_test_count);
-for(test, outcome_diff)in diff.test_diffs.iter().take(max_test_count){
-println!(" - {}: {}", test.name, format_diff(&outcome_diff));
-}
-if extra_tests > 0{
-println!(" - (and {extra_tests} additional {})", pluralize("test", extra_tests));
-}
+// Sort diffs by job group and test name
+ grouped_diffs.sort_by(|(d1, g1),(d2, g2)| g1.cmp(&g2).then(d1.test.name.cmp(&d2.test.name)));
+
+for(diff, job_group)in grouped_diffs {
+println!("- `{}`: {} (*J{job_group}*)", diff.test.name, format_diff(&diff.diff));
 }
 
-let extra_diffs = diffs.len().saturating_sub(max_diff_count);
+let extra_diffs = diff.diffs.len().saturating_sub(max_diff_count);
 if extra_diffs > 0{
-println!("\n(and {extra_diffs} additional {})", pluralize("diff", extra_diffs));
+println!("\n(and {extra_diffs} additional {})", pluralize("test diff", extra_diffs));
+}
+
+// Now print the job index
+println!("\n**Job index**\n");
+for(group, jobs)in job_index.into_iter().enumerate(){
+println!("- J{group}: `{}`", jobs.join(", "));
 }
 }