Skip to content
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit 34fd0c6

Browse files
committedMar 15, 2025
Auto merge of rust-lang#138531 - Kobzol:test-diff-try-build, r=<try>
Store test diffs in job summaries and improve analysis formatting This PR stores the test diffs that we already have in the post-merge workflow also into individual job summaries. This makes it easier to compare test (and later also other) diffs per job, which will be especially useful for try jobs, so that we can actually see the test diffs *before* we merge a given PR. As a drive-by, I also made a bunch of cleanups in `citool` and in the formatting of the summary and post-merge analyses. These changes are split into self-contained commits. The analysis can be tested locally with the following command (you can replace : ```bash $ curl https://ci-artifacts.rust-lang.org/rustc-builds/<current-sha>/metrics-<job-name>.json > metrics.json $ cargo run --manifest-path src/ci/citool/Cargo.toml postprocess-metrics metrics.json --job-name <job-name> --parent <parent-sha> > out.md ``` For example, for [this PR](rust-lang#138523): ```bash $ curl https://ci-artifacts.rust-lang.org/rustc-builds/282865097d138c7f0f7a7566db5b761312dd145c/metrics-aarch64-gnu.json > metrics.json $ cargo run --manifest-path src/ci/citool/Cargo.toml postprocess-metrics metrics.json --job-name aarch64-gnu --parent d9e5539 > out.md ``` Best reviewed commit by commit. r? `@marcoieni` try-job: aarch64-gnu try-job: dist-x86_64-linux
2 parents adea7cb + e845318 commit 34fd0c6

File tree

6 files changed

+530
-473
lines changed

6 files changed

+530
-473
lines changed
 

‎.github/workflows/ci.yml

+17-2
Original file line numberDiff line numberDiff line change
@@ -239,16 +239,31 @@ jobs:
239239
if: github.event_name == 'push' || env.DEPLOY == '1' || env.DEPLOY_ALT == '1'
240240

241241
- name: postprocess metrics into the summary
242+
# This step is not critical, and if some I/O problem happens, we don't want
243+
# to cancel the build.
244+
continue-on-error: true
242245
run: |
243246
if [ -f build/metrics.json ]; then
244-
./build/citool/debug/citool postprocess-metrics build/metrics.json ${GITHUB_STEP_SUMMARY}
247+
METRICS=build/metrics.json
245248
elif [ -f obj/build/metrics.json ]; then
246-
./build/citool/debug/citool postprocess-metrics obj/build/metrics.json ${GITHUB_STEP_SUMMARY}
249+
METRICS=obj/build/metrics.json
247250
else
248251
echo "No metrics.json found"
252+
exit 0
249253
fi
250254
255+
# Get closest bors merge commit
256+
PARENT_COMMIT=`git rev-list --author='bors <bors@rust-lang.org>' -n1 --first-parent HEAD^1`
257+
258+
./build/citool/debug/citool postprocess-metrics \
259+
--job-name ${CI_JOB_NAME} \
260+
--parent ${PARENT_COMMIT} \
261+
${METRICS} >> ${GITHUB_STEP_SUMMARY}
262+
251263
- name: upload job metrics to DataDog
264+
# This step is not critical, and if some I/O problem happens, we don't want
265+
# to cancel the build.
266+
continue-on-error: true
252267
if: needs.calculate_matrix.outputs.run_type != 'pr'
253268
env:
254269
DATADOG_API_KEY: ${{ secrets.DATADOG_API_KEY }}

‎src/ci/citool/src/analysis.rs

+360
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,360 @@
1+
use crate::metrics;
2+
use crate::metrics::{JobMetrics, JobName, get_test_suites};
3+
use crate::utils::{output_details, pluralize};
4+
use build_helper::metrics::{
5+
BuildStep, JsonRoot, TestOutcome, TestSuite, TestSuiteMetadata, format_build_steps,
6+
};
7+
use std::collections::{BTreeMap, HashMap, HashSet};
8+
9+
pub fn output_bootstrap_stats(metrics: &JsonRoot) {
10+
if !metrics.invocations.is_empty() {
11+
println!("# Bootstrap steps");
12+
record_bootstrap_step_durations(&metrics);
13+
record_test_suites(&metrics);
14+
}
15+
}
16+
17+
fn record_bootstrap_step_durations(metrics: &JsonRoot) {
18+
for invocation in &metrics.invocations {
19+
let step = BuildStep::from_invocation(invocation);
20+
let table = format_build_steps(&step);
21+
eprintln!("Step `{}`\n{table}\n", invocation.cmdline);
22+
output_details(&invocation.cmdline, || {
23+
println!("<pre><code>{table}</code></pre>");
24+
});
25+
}
26+
eprintln!("Recorded {} bootstrap invocation(s)", metrics.invocations.len());
27+
}
28+
29+
fn record_test_suites(metrics: &JsonRoot) {
30+
let suites = metrics::get_test_suites(&metrics);
31+
32+
if !suites.is_empty() {
33+
let aggregated = aggregate_test_suites(&suites);
34+
let table = render_table(aggregated);
35+
println!("\n# Test results\n");
36+
println!("{table}");
37+
} else {
38+
eprintln!("No test suites found in metrics");
39+
}
40+
}
41+
42+
fn render_table(suites: BTreeMap<String, TestSuiteRecord>) -> String {
43+
use std::fmt::Write;
44+
45+
let mut table = "| Test suite | Passed ✅ | Ignored 🚫 | Failed ❌ |\n".to_string();
46+
writeln!(table, "|:------|------:|------:|------:|").unwrap();
47+
48+
fn compute_pct(value: f64, total: f64) -> f64 {
49+
if total == 0.0 { 0.0 } else { value / total }
50+
}
51+
52+
fn write_row(
53+
buffer: &mut String,
54+
name: &str,
55+
record: &TestSuiteRecord,
56+
surround: &str,
57+
) -> std::fmt::Result {
58+
let TestSuiteRecord { passed, ignored, failed } = record;
59+
let total = (record.passed + record.ignored + record.failed) as f64;
60+
let passed_pct = compute_pct(*passed as f64, total) * 100.0;
61+
let ignored_pct = compute_pct(*ignored as f64, total) * 100.0;
62+
let failed_pct = compute_pct(*failed as f64, total) * 100.0;
63+
64+
write!(buffer, "| {surround}{name}{surround} |")?;
65+
write!(buffer, " {surround}{passed} ({passed_pct:.0}%){surround} |")?;
66+
write!(buffer, " {surround}{ignored} ({ignored_pct:.0}%){surround} |")?;
67+
writeln!(buffer, " {surround}{failed} ({failed_pct:.0}%){surround} |")?;
68+
69+
Ok(())
70+
}
71+
72+
let mut total = TestSuiteRecord::default();
73+
for (name, record) in suites {
74+
write_row(&mut table, &name, &record, "").unwrap();
75+
total.passed += record.passed;
76+
total.ignored += record.ignored;
77+
total.failed += record.failed;
78+
}
79+
write_row(&mut table, "Total", &total, "**").unwrap();
80+
table
81+
}
82+
83+
/// Computes a post merge CI analysis report of test differences
84+
/// between the `parent` and `current` commits.
85+
pub fn output_test_diffs(job_metrics: HashMap<JobName, JobMetrics>) {
86+
let aggregated_test_diffs = aggregate_test_diffs(&job_metrics);
87+
report_test_diffs(aggregated_test_diffs);
88+
}
89+
90+
#[derive(Default)]
91+
struct TestSuiteRecord {
92+
passed: u64,
93+
ignored: u64,
94+
failed: u64,
95+
}
96+
97+
fn test_metadata_name(metadata: &TestSuiteMetadata) -> String {
98+
match metadata {
99+
TestSuiteMetadata::CargoPackage { crates, stage, .. } => {
100+
format!("{} (stage {stage})", crates.join(", "))
101+
}
102+
TestSuiteMetadata::Compiletest { suite, stage, .. } => {
103+
format!("{suite} (stage {stage})")
104+
}
105+
}
106+
}
107+
108+
fn aggregate_test_suites(suites: &[&TestSuite]) -> BTreeMap<String, TestSuiteRecord> {
109+
let mut records: BTreeMap<String, TestSuiteRecord> = BTreeMap::new();
110+
for suite in suites {
111+
let name = test_metadata_name(&suite.metadata);
112+
let record = records.entry(name).or_default();
113+
for test in &suite.tests {
114+
match test.outcome {
115+
TestOutcome::Passed => {
116+
record.passed += 1;
117+
}
118+
TestOutcome::Failed => {
119+
record.failed += 1;
120+
}
121+
TestOutcome::Ignored { .. } => {
122+
record.ignored += 1;
123+
}
124+
}
125+
}
126+
}
127+
records
128+
}
129+
130+
/// Represents a difference in the outcome of tests between a base and a current commit.
131+
/// Maps test diffs to jobs that contained them.
132+
#[derive(Debug)]
133+
struct AggregatedTestDiffs {
134+
diffs: HashMap<TestDiff, Vec<JobName>>,
135+
}
136+
137+
fn aggregate_test_diffs(jobs: &HashMap<JobName, JobMetrics>) -> AggregatedTestDiffs {
138+
let mut diffs: HashMap<TestDiff, Vec<JobName>> = HashMap::new();
139+
140+
// Aggregate test suites
141+
for (name, metrics) in jobs {
142+
if let Some(parent) = &metrics.parent {
143+
let tests_parent = aggregate_tests(parent);
144+
let tests_current = aggregate_tests(&metrics.current);
145+
for diff in calculate_test_diffs(tests_parent, tests_current) {
146+
diffs.entry(diff).or_default().push(name.to_string());
147+
}
148+
}
149+
}
150+
151+
AggregatedTestDiffs { diffs }
152+
}
153+
154+
#[derive(Eq, PartialEq, Hash, Debug)]
155+
enum TestOutcomeDiff {
156+
ChangeOutcome { before: TestOutcome, after: TestOutcome },
157+
Missing { before: TestOutcome },
158+
Added(TestOutcome),
159+
}
160+
161+
#[derive(Eq, PartialEq, Hash, Debug)]
162+
struct TestDiff {
163+
test: Test,
164+
diff: TestOutcomeDiff,
165+
}
166+
167+
fn calculate_test_diffs(parent: TestSuiteData, current: TestSuiteData) -> HashSet<TestDiff> {
168+
let mut diffs = HashSet::new();
169+
for (test, outcome) in &current.tests {
170+
match parent.tests.get(test) {
171+
Some(before) => {
172+
if before != outcome {
173+
diffs.insert(TestDiff {
174+
test: test.clone(),
175+
diff: TestOutcomeDiff::ChangeOutcome {
176+
before: before.clone(),
177+
after: outcome.clone(),
178+
},
179+
});
180+
}
181+
}
182+
None => {
183+
diffs.insert(TestDiff {
184+
test: test.clone(),
185+
diff: TestOutcomeDiff::Added(outcome.clone()),
186+
});
187+
}
188+
}
189+
}
190+
for (test, outcome) in &parent.tests {
191+
if !current.tests.contains_key(test) {
192+
diffs.insert(TestDiff {
193+
test: test.clone(),
194+
diff: TestOutcomeDiff::Missing { before: outcome.clone() },
195+
});
196+
}
197+
}
198+
199+
diffs
200+
}
201+
202+
/// Aggregates test suite executions from all bootstrap invocations in a given CI job.
203+
#[derive(Default)]
204+
struct TestSuiteData {
205+
tests: HashMap<Test, TestOutcome>,
206+
}
207+
208+
#[derive(Hash, PartialEq, Eq, Debug, Clone)]
209+
struct Test {
210+
name: String,
211+
is_doctest: bool,
212+
}
213+
214+
/// Extracts all tests from the passed metrics and map them to their outcomes.
215+
fn aggregate_tests(metrics: &JsonRoot) -> TestSuiteData {
216+
let mut tests = HashMap::new();
217+
let test_suites = get_test_suites(&metrics);
218+
for suite in test_suites {
219+
for test in &suite.tests {
220+
// Poor man's detection of doctests based on the "(line XYZ)" suffix
221+
let is_doctest = matches!(suite.metadata, TestSuiteMetadata::CargoPackage { .. })
222+
&& test.name.contains("(line");
223+
let test_entry = Test { name: generate_test_name(&test.name, &suite), is_doctest };
224+
tests.insert(test_entry, test.outcome.clone());
225+
}
226+
}
227+
TestSuiteData { tests }
228+
}
229+
230+
/// Normalizes Windows-style path delimiters to Unix-style paths
231+
/// and adds suite metadata to the test name.
232+
fn generate_test_name(name: &str, suite: &TestSuite) -> String {
233+
let name = name.replace('\\', "/");
234+
let stage = match suite.metadata {
235+
TestSuiteMetadata::CargoPackage { stage, .. } => stage,
236+
TestSuiteMetadata::Compiletest { stage, .. } => stage,
237+
};
238+
239+
format!("{name} (stage {stage})")
240+
}
241+
242+
/// Prints test changes in Markdown format to stdout.
243+
fn report_test_diffs(diff: AggregatedTestDiffs) {
244+
println!("# Test differences");
245+
if diff.diffs.is_empty() {
246+
println!("No test diffs found");
247+
return;
248+
}
249+
250+
fn format_outcome(outcome: &TestOutcome) -> String {
251+
match outcome {
252+
TestOutcome::Passed => "pass".to_string(),
253+
TestOutcome::Failed => "fail".to_string(),
254+
TestOutcome::Ignored { ignore_reason } => {
255+
let reason = match ignore_reason {
256+
Some(reason) => format!(" ({reason})"),
257+
None => String::new(),
258+
};
259+
format!("ignore{reason}")
260+
}
261+
}
262+
}
263+
264+
fn format_diff(diff: &TestOutcomeDiff) -> String {
265+
match diff {
266+
TestOutcomeDiff::ChangeOutcome { before, after } => {
267+
format!("{} -> {}", format_outcome(before), format_outcome(after))
268+
}
269+
TestOutcomeDiff::Missing { before } => {
270+
format!("{} -> [missing]", format_outcome(before))
271+
}
272+
TestOutcomeDiff::Added(outcome) => {
273+
format!("[missing] -> {}", format_outcome(outcome))
274+
}
275+
}
276+
}
277+
278+
fn format_job_group(group: u64) -> String {
279+
format!("**J{group}**")
280+
}
281+
282+
// It would be quite noisy to repeat the jobs that contained the test changes after/next to
283+
// every test diff. At the same time, grouping the test diffs by
284+
// [unique set of jobs that contained them] also doesn't work well, because the test diffs
285+
// would have to be duplicated several times.
286+
// Instead, we create a set of unique job groups, and then print a job group after each test.
287+
// We then print the job groups at the end, as a sort of index.
288+
let mut grouped_diffs: Vec<(&TestDiff, u64)> = vec![];
289+
let mut job_list_to_group: HashMap<&[JobName], u64> = HashMap::new();
290+
let mut job_index: Vec<&[JobName]> = vec![];
291+
292+
let original_diff_count = diff.diffs.len();
293+
let diffs = diff
294+
.diffs
295+
.into_iter()
296+
.filter(|(diff, _)| !diff.test.is_doctest)
297+
.map(|(diff, mut jobs)| {
298+
jobs.sort();
299+
(diff, jobs)
300+
})
301+
.collect::<Vec<_>>();
302+
let doctest_count = original_diff_count.saturating_sub(diffs.len());
303+
304+
let max_diff_count = 100;
305+
for (diff, jobs) in diffs.iter().take(max_diff_count) {
306+
let jobs = &*jobs;
307+
let job_group = match job_list_to_group.get(jobs.as_slice()) {
308+
Some(id) => *id,
309+
None => {
310+
let id = job_index.len() as u64;
311+
job_index.push(jobs);
312+
job_list_to_group.insert(jobs, id);
313+
id
314+
}
315+
};
316+
grouped_diffs.push((diff, job_group));
317+
}
318+
319+
// Sort diffs by job group and test name
320+
grouped_diffs.sort_by(|(d1, g1), (d2, g2)| g1.cmp(&g2).then(d1.test.name.cmp(&d2.test.name)));
321+
322+
output_details(
323+
&format!("Show {} test {}\n", original_diff_count, pluralize("diff", original_diff_count)),
324+
|| {
325+
for (diff, job_group) in grouped_diffs {
326+
println!(
327+
"- `{}`: {} ({})",
328+
diff.test.name,
329+
format_diff(&diff.diff),
330+
format_job_group(job_group)
331+
);
332+
}
333+
334+
let extra_diffs = diffs.len().saturating_sub(max_diff_count);
335+
if extra_diffs > 0 {
336+
println!(
337+
"\n(and {extra_diffs} additional {})",
338+
pluralize("test diff", extra_diffs)
339+
);
340+
}
341+
342+
if doctest_count > 0 {
343+
println!(
344+
"\nAdditionally, {doctest_count} doctest {} were found. These are ignored, as they are noisy.",
345+
pluralize("diff", doctest_count)
346+
);
347+
}
348+
349+
// Now print the job group index
350+
println!("\n**Job group index**\n");
351+
for (group, jobs) in job_index.into_iter().enumerate() {
352+
println!(
353+
"- {}: {}",
354+
format_job_group(group as u64),
355+
jobs.iter().map(|j| format!("`{j}`")).collect::<Vec<_>>().join(", ")
356+
);
357+
}
358+
},
359+
);
360+
}
There was a problem loading the remainder of the diff.

0 commit comments

Comments
 (0)
Failed to load comments.