bjoernager/rust
bjoernager/rust
1
Fork 0
Code Issues Pull requests Activity

Rollup merge of #138454 - Kobzol:post-merge-workflow-fixes, r=jieyouxu

Browse source 
Improve post-merge workflow

Contains various fixes for the post-merge workflow implemented in https://github.com/rust-lang/rust/pull/138013, which were suggested on Zulip. This PR changes the grouping of test diffs and ignores doctests, as they are too noisy.

I'll post an example output (before/after this PR) in comments below.

r? ```@jieyouxu```
This commit is contained in:
León Orell Valerian Liehr 2025-03-15 00:18:23 +01:00 • committed by GitHub
commit 756b17975f
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 153 additions and 87 deletions
Download patch file Download diff file Expand all files Collapse all files

7
.github/workflows/post-merge.yml vendored
View file

@ -35,8 +35,13 @@ jobs:
cd src/ci/citool
echo "Post-merge analysis result" > output.log
printf "*This is an experimental post-merge analysis report. You can ignore it.*\n\n" > output.log
printf "<details>\n<summary>Post-merge report</summary>\n\n" >> output.log
cargo run --release post-merge-report ${PARENT_COMMIT} ${{ github.sha }} >> output.log
printf "</details>\n" >> output.log
cat output.log
gh pr comment ${HEAD_PR} -F output.log

233
src/ci/citool/src/merge_report.rs
View file

@ -1,8 +1,8 @@
use std::cmp::Reverse;
use std::collections::HashMap;
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;
@ -13,8 +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, &current)?;
let diffs = aggregate_test_diffs(&jobs)?;
report_test_changes(diffs);
let aggregated_test_diffs = aggregate_test_diffs(&jobs)?;
println!("Comparing {parent} (base) -> {current} (this PR)\n");
report_test_diffs(aggregated_test_diffs);
Ok(())
}
@ -54,7 +56,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<JsonRoot> {
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() {
@ -68,6 +79,13 @@ fn download_job_metrics(job_name: &str, sha: &str) -> anyhow::Result<JsonRoot> {
.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)
}
@ -76,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<TestDiff, Vec<JobName>>,
}
fn aggregate_test_diffs(
jobs: &HashMap<JobName, JobMetrics>,
) -> anyhow::Result<Vec<AggregatedTestDiffs>> {
let mut job_diffs = vec![];
) -> anyhow::Result<AggregatedTestDiffs> {
let mut diffs: HashMap<TestDiff, Vec<JobName>> = 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<(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())
}
fn calculate_test_diffs(
reference: TestSuiteData,
current: TestSuiteData,
) -> Vec<(Test, TestOutcomeDiff)> {
let mut diffs = vec![];
for (test, outcome) in &current.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<String>,
test_diffs: Vec<(Test, TestOutcomeDiff)>,
Ok(AggregatedTestDiffs { diffs })
}
#[derive(Eq, PartialEq, Hash, Debug)]
@ -151,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<TestDiff> {
let mut diffs = HashSet::new();
for (test, outcome) in &current.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 {
@ -160,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.
@ -168,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());
}
}
@ -181,16 +202,13 @@ fn normalize_test_name(name: &str) -> String {
}
/// Prints test changes in Markdown format to stdout.
fn report_test_changes(mut diffs: Vec<AggregatedTestDiffs>) {
fn report_test_diffs(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(),
@ -219,36 +237,79 @@ fn report_test_changes(mut diffs: Vec<AggregatedTestDiffs>) {
}
}
let max_diff_count = 10;
let max_job_count = 5;
let max_test_count = 10;
fn format_job_group(group: u64) -> String {
format!("**J{group}**")
}
for diff in diffs.iter().take(max_diff_count) {
let mut jobs = diff.jobs.clone();
jobs.sort();
// 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![];
let jobs = jobs.iter().take(max_job_count).map(|j| format!("`{j}`")).collect::<Vec<_>>();
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::<Vec<_>>();
let doctest_count = original_diff_count.saturating_sub(diffs.len());
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 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
}
};
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("tests", 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!(
"- `{}`: {} ({})",
diff.test.name,
format_diff(&diff.diff),
format_job_group(job_group)
);
}
let extra_diffs = 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));
}
if doctest_count > 0 {
println!(
"\nAdditionally, {doctest_count} doctest {} were found. These are ignored, as they are noisy.",
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!(
"- {}: {}",
format_job_group(group as u64),
jobs.iter().map(|j| format!("`{j}`")).collect::<Vec<_>>().join(", ")
);
}
}