Skip to content

Commit d88b8b1

Browse files
authored
Unrolled build for rust-lang#138531
Rollup merge of rust-lang#138531 - Kobzol:test-diff-try-build, r=marcoieni 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: ```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 493c38b + c9d3147 commit d88b8b1

File tree

6 files changed

+532
-472
lines changed

6 files changed

+532
-472
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

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

0 commit comments

Comments
 (0)