From 6f214c5bb3959dc8a646fac0dafdcaeaf7e025f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Thu, 13 Mar 2025 13:14:14 +0100 Subject: [PATCH 1/8] Fix pluralization of tests --- src/ci/citool/src/merge_report.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ci/citool/src/merge_report.rs b/src/ci/citool/src/merge_report.rs index 17e42d49286..2dbe383b912 100644 --- a/src/ci/citool/src/merge_report.rs +++ b/src/ci/citool/src/merge_report.rs @@ -242,7 +242,7 @@ fn report_test_changes(mut diffs: Vec) { println!(" - {}: {}", test.name, format_diff(&outcome_diff)); } if extra_tests > 0 { - println!(" - (and {extra_tests} additional {})", pluralize("tests", extra_tests)); + println!(" - (and {extra_tests} additional {})", pluralize("test", extra_tests)); } } From 208cef45fc963528a8959e3b153d3b238ca1e187 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Thu, 13 Mar 2025 13:15:38 +0100 Subject: [PATCH 2/8] Add note about the experimental status --- .github/workflows/post-merge.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/post-merge.yml b/.github/workflows/post-merge.yml index 31e075f45d6..da566a91b46 100644 --- a/.github/workflows/post-merge.yml +++ b/.github/workflows/post-merge.yml @@ -35,7 +35,7 @@ jobs: cd src/ci/citool - echo "Post-merge analysis result" > output.log + printf "*This is an experimental post-merge analysis result. You can ignore it.*\n\n" > output.log cargo run --release post-merge-report ${PARENT_COMMIT} ${{ github.sha }} >> output.log cat output.log From 2192d5c4e218aa137f7d38bc6c93e36af6149bf3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Thu, 13 Mar 2025 13:18:47 +0100 Subject: [PATCH 3/8] Print the compared SHAs --- src/ci/citool/src/merge_report.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/ci/citool/src/merge_report.rs b/src/ci/citool/src/merge_report.rs index 2dbe383b912..21b77299cdb 100644 --- a/src/ci/citool/src/merge_report.rs +++ b/src/ci/citool/src/merge_report.rs @@ -14,6 +14,8 @@ type JobName = String; pub fn post_merge_report(job_db: JobDatabase, parent: Sha, current: Sha) -> anyhow::Result<()> { let jobs = download_all_metrics(&job_db, &parent, ¤t)?; let diffs = aggregate_test_diffs(&jobs)?; + + println!("Comparing {parent} (base) -> {current} (this PR)\n"); report_test_changes(diffs); Ok(()) From 7ed913be3196ac43f6938ba5beb01e72464cc1cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Thu, 13 Mar 2025 13:26:45 +0100 Subject: [PATCH 4/8] Add cache for downloading job metrics To make it easier to experiment locally. --- src/ci/citool/src/merge_report.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/ci/citool/src/merge_report.rs b/src/ci/citool/src/merge_report.rs index 21b77299cdb..76b7d779abc 100644 --- a/src/ci/citool/src/merge_report.rs +++ b/src/ci/citool/src/merge_report.rs @@ -1,5 +1,6 @@ use std::cmp::Reverse; use std::collections::HashMap; +use std::path::PathBuf; use anyhow::Context; use build_helper::metrics::{JsonRoot, TestOutcome}; @@ -56,7 +57,16 @@ Maybe it was newly added?"#, Ok(jobs) } +/// Downloads job metrics of the given job for the given commit. +/// Caches the result on the local disk. fn download_job_metrics(job_name: &str, sha: &str) -> anyhow::Result { + let cache_path = PathBuf::from(".citool-cache").join(sha).join(job_name).join("metrics.json"); + if let Some(cache_entry) = + std::fs::read_to_string(&cache_path).ok().and_then(|data| serde_json::from_str(&data).ok()) + { + return Ok(cache_entry); + } + let url = get_metrics_url(job_name, sha); let mut response = ureq::get(&url).call()?; if !response.status().is_success() { @@ -70,6 +80,13 @@ fn download_job_metrics(job_name: &str, sha: &str) -> anyhow::Result { .body_mut() .read_json() .with_context(|| anyhow::anyhow!("cannot deserialize metrics from {url}"))?; + + // Ignore errors if cache cannot be created + if std::fs::create_dir_all(cache_path.parent().unwrap()).is_ok() { + if let Ok(serialized) = serde_json::to_string(&data) { + let _ = std::fs::write(&cache_path, &serialized); + } + } Ok(data) } From 5a7f227351dc12fe3217394c9ecb7e9643c6d67e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Thu, 13 Mar 2025 13:36:34 +0100 Subject: [PATCH 5/8] Collapse report in `
` --- .github/workflows/post-merge.yml | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/.github/workflows/post-merge.yml b/.github/workflows/post-merge.yml index da566a91b46..de31c28cc90 100644 --- a/.github/workflows/post-merge.yml +++ b/.github/workflows/post-merge.yml @@ -35,8 +35,13 @@ jobs: cd src/ci/citool - printf "*This is an experimental post-merge analysis result. You can ignore it.*\n\n" > output.log + printf "*This is an experimental post-merge analysis report. You can ignore it.*\n\n" > output.log + printf "
\nPost-merge report\n\n" >> output.log + cargo run --release post-merge-report ${PARENT_COMMIT} ${{ github.sha }} >> output.log + + printf "
\n" >> output.log + cat output.log gh pr comment ${HEAD_PR} -F output.log From d5d633d2460e45df25c679933ea678367b4d94a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Thu, 13 Mar 2025 13:55:03 +0100 Subject: [PATCH 6/8] Group diffs by tests, rather than job groups --- src/ci/citool/src/merge_report.rs | 186 ++++++++++++++++-------------- 1 file changed, 98 insertions(+), 88 deletions(-) diff --git a/src/ci/citool/src/merge_report.rs b/src/ci/citool/src/merge_report.rs index 76b7d779abc..918e9b92f1e 100644 --- a/src/ci/citool/src/merge_report.rs +++ b/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. pub fn post_merge_report(job_db: JobDatabase, parent: Sha, current: Sha) -> anyhow::Result<()> { let jobs = download_all_metrics(&job_db, &parent, ¤t)?; - let diffs = aggregate_test_diffs(&jobs)?; + let aggregated_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,72 +94,30 @@ 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)] +struct AggregatedTestDiffs { + diffs: HashMap>, +} + fn aggregate_test_diffs( jobs: &HashMap, -) -> anyhow::Result> { - let mut job_diffs = vec![]; +) -> anyhow::Result { + let mut diffs: HashMap> = HashMap::new(); // Aggregate test suites for (name, metrics) in jobs { if let Some(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 in calculate_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> = - 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()) -} - -fn calculate_test_diffs( - reference: TestSuiteData, - current: TestSuiteData, -) -> Vec<(Test, TestOutcomeDiff)> { - let mut diffs = vec![]; - for (test, outcome) in ¤t.tests { - match reference.tests.get(test) { - Some(before) => { - if before != outcome { - diffs.push(( - test.clone(), - TestOutcomeDiff::ChangeOutcome { - before: before.clone(), - after: outcome.clone(), - }, - )); - } - } - None => diffs.push((test.clone(), TestOutcomeDiff::Added(outcome.clone()))), - } - } - for (test, outcome) in &reference.tests { - if !current.tests.contains_key(test) { - diffs.push((test.clone(), TestOutcomeDiff::Missing { before: outcome.clone() })); - } - } - - diffs -} - -/// Represents a difference in the outcome of tests between a base and a current commit. -#[derive(Debug)] -struct AggregatedTestDiffs { - /// All jobs that had the exact same test diffs. - jobs: Vec, - test_diffs: Vec<(Test, TestOutcomeDiff)>, + Ok(AggregatedTestDiffs { diffs }) } #[derive(Eq, PartialEq, Hash, Debug)] @@ -170,6 +127,47 @@ enum TestOutcomeDiff { Added(TestOutcome), } +#[derive(Eq, PartialEq, Hash, Debug)] +struct TestDiff { + test: Test, + diff: TestOutcomeDiff, +} + +fn calculate_test_diffs(parent: TestSuiteData, current: TestSuiteData) -> HashSet { + let mut diffs = HashSet::new(); + for (test, outcome) in ¤t.tests { + match parent.tests.get(test) { + Some(before) => { + if before != outcome { + diffs.insert(TestDiff { + test: test.clone(), + diff: TestOutcomeDiff::ChangeOutcome { + before: before.clone(), + after: outcome.clone(), + }, + }); + } + } + None => { + diffs.insert(TestDiff { + test: test.clone(), + diff: TestOutcomeDiff::Added(outcome.clone()), + }); + } + } + } + for (test, outcome) in &parent.tests { + if !current.tests.contains_key(test) { + diffs.insert(TestDiff { + test: test.clone(), + diff: TestOutcomeDiff::Missing { before: outcome.clone() }, + }); + } + } + + diffs +} + /// Aggregates test suite executions from all bootstrap invocations in a given CI job. #[derive(Default)] struct TestSuiteData { @@ -200,16 +198,13 @@ fn normalize_test_name(name: &str) -> String { } /// Prints test changes in Markdown format to stdout. -fn report_test_changes(mut diffs: Vec) { +fn report_test_diffs(mut diff: AggregatedTestDiffs) { println!("## Test differences"); - if diffs.is_empty() { + if diff.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())); - fn format_outcome(outcome: &TestOutcome) -> String { match outcome { TestOutcome::Passed => "pass".to_string(), @@ -238,36 +233,51 @@ fn report_test_changes(mut diffs: Vec) { } } - let max_diff_count = 10; - let max_job_count = 5; - let max_test_count = 10; + // 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. + let mut grouped_diffs: Vec<(&TestDiff, u64)> = vec![]; + let mut job_list_to_group: HashMap<&[JobName], u64> = HashMap::new(); + let mut job_index: Vec<&[JobName]> = vec![]; - for diff in diffs.iter().take(max_diff_count) { - let mut jobs = diff.jobs.clone(); + for (_, jobs) in diff.diffs.iter_mut() { jobs.sort(); - - let jobs = jobs.iter().take(max_job_count).map(|j| format!("`{j}`")).collect::>(); - - 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() - }; - println!("- {}{suffix}", jobs.join(",")); - - 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)); - } } - let extra_diffs = diffs.len().saturating_sub(max_diff_count); + 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() as u64; + job_index.push(jobs); + job_list_to_group.insert(jobs, id); + id + } + }; + grouped_diffs.push((diff, job_group)); + } + + // 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 = 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(", ")); } } From f981a0a0cd266e88c4c504a7d3218d045cc8104e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Thu, 13 Mar 2025 14:31:23 +0100 Subject: [PATCH 7/8] Do not print doctest diffs in the report When they are moved around in code, their name changes, which produces too noisy diffs. --- src/ci/citool/src/merge_report.rs | 56 ++++++++++++++++++++++++------- 1 file changed, 44 insertions(+), 12 deletions(-) diff --git a/src/ci/citool/src/merge_report.rs b/src/ci/citool/src/merge_report.rs index 918e9b92f1e..be7246cfa5e 100644 --- a/src/ci/citool/src/merge_report.rs +++ b/src/ci/citool/src/merge_report.rs @@ -2,7 +2,7 @@ use std::collections::{HashMap, HashSet}; use std::path::PathBuf; use anyhow::Context; -use build_helper::metrics::{JsonRoot, TestOutcome}; +use build_helper::metrics::{JsonRoot, TestOutcome, TestSuiteMetadata}; use crate::jobs::JobDatabase; use crate::metrics::get_test_suites; @@ -177,6 +177,7 @@ struct TestSuiteData { #[derive(Hash, PartialEq, Eq, Debug, Clone)] struct Test { name: String, + is_doctest: bool, } /// Extracts all tests from the passed metrics and map them to their outcomes. @@ -185,7 +186,10 @@ fn aggregate_tests(metrics: &JsonRoot) -> TestSuiteData { let test_suites = get_test_suites(&metrics); for suite in test_suites { for test in &suite.tests { - let test_entry = Test { name: normalize_test_name(&test.name) }; + // Poor man's detection of doctests based on the "(line XYZ)" suffix + let is_doctest = matches!(suite.metadata, TestSuiteMetadata::CargoPackage { .. }) + && test.name.contains("(line"); + let test_entry = Test { name: normalize_test_name(&test.name), is_doctest }; tests.insert(test_entry, test.outcome.clone()); } } @@ -198,7 +202,7 @@ fn normalize_test_name(name: &str) -> String { } /// Prints test changes in Markdown format to stdout. -fn report_test_diffs(mut diff: AggregatedTestDiffs) { +fn report_test_diffs(diff: AggregatedTestDiffs) { println!("## Test differences"); if diff.diffs.is_empty() { println!("No test diffs found"); @@ -233,6 +237,10 @@ fn report_test_diffs(mut diff: AggregatedTestDiffs) { } } + fn format_job_group(group: u64) -> String { + format!("**J{group}**") + } + // 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 @@ -243,12 +251,20 @@ fn report_test_diffs(mut diff: AggregatedTestDiffs) { let mut job_list_to_group: HashMap<&[JobName], u64> = HashMap::new(); let mut job_index: Vec<&[JobName]> = vec![]; - for (_, jobs) in diff.diffs.iter_mut() { - jobs.sort(); - } + let original_diff_count = diff.diffs.len(); + let diffs = diff + .diffs + .into_iter() + .filter(|(diff, _)| !diff.test.is_doctest) + .map(|(diff, mut jobs)| { + jobs.sort(); + (diff, jobs) + }) + .collect::>(); + let doctest_count = original_diff_count.saturating_sub(diffs.len()); let max_diff_count = 100; - for (diff, jobs) in diff.diffs.iter().take(max_diff_count) { + for (diff, jobs) in diffs.iter().take(max_diff_count) { let jobs = &*jobs; let job_group = match job_list_to_group.get(jobs.as_slice()) { Some(id) => *id, @@ -266,18 +282,34 @@ fn report_test_diffs(mut diff: AggregatedTestDiffs) { 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)); + println!( + "- `{}`: {} ({})", + diff.test.name, + format_diff(&diff.diff), + format_job_group(job_group) + ); } - let extra_diffs = diff.diffs.len().saturating_sub(max_diff_count); + let extra_diffs = diffs.len().saturating_sub(max_diff_count); if extra_diffs > 0 { println!("\n(and {extra_diffs} additional {})", pluralize("test diff", extra_diffs)); } - // Now print the job index - println!("\n**Job index**\n"); + if doctest_count > 0 { + println!( + "\nAdditionally, {doctest_count} doctest {} were found.", + pluralize("diff", doctest_count) + ); + } + + // Now print the job group index + println!("\n**Job group index**\n"); for (group, jobs) in job_index.into_iter().enumerate() { - println!("- J{group}: `{}`", jobs.join(", ")); + println!( + "- {}: {}", + format_job_group(group as u64), + jobs.iter().map(|j| format!("`{j}`")).collect::>().join(", ") + ); } } From 6ef465ba126013fa6add41813cd8299072507df6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Fri, 14 Mar 2025 09:09:26 +0100 Subject: [PATCH 8/8] Add clarification about doctests --- src/ci/citool/src/merge_report.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ci/citool/src/merge_report.rs b/src/ci/citool/src/merge_report.rs index be7246cfa5e..62daa2e6853 100644 --- a/src/ci/citool/src/merge_report.rs +++ b/src/ci/citool/src/merge_report.rs @@ -297,7 +297,7 @@ fn report_test_diffs(diff: AggregatedTestDiffs) { if doctest_count > 0 { println!( - "\nAdditionally, {doctest_count} doctest {} were found.", + "\nAdditionally, {doctest_count} doctest {} were found. These are ignored, as they are noisy.", pluralize("diff", doctest_count) ); }