Rollup merge of #111936 - ferrocene:pa-test-suite-metadata, r=jyn514

GuillaumeGomez · web-flow · commit 859068c628cb · 2023-05-27T13:38:30.000+02:00
Include test suite metadata in the build metrics

This PR enhances the build metadata to include structured information about the test suites being executed, allowing external tools consuming the metadata to understand what was being tested.

The included metadata is:

* Target triple
* Host triple
* Stage number
* For compiletest tests:
  * Suite name
  * Mode
  * Comparing mode
* For crate tests:
  * List of crate names

This is implemented by replacing the `test` JSON node with a new `test_suite` node, which contains the metadata and the list of tests. This change also improves the handling of multiple test suites executed in the same step (for example in compiletest tests with a compare mode), as the multiple test suite executions will now be tracked in separate `test_suite` nodes.

This included a breaking change in the build metrics metadata format. To better handle this, in the second commit this PR introduces the `metadata_version` top-level field. The old version is considered to be `0`, while the new one `1`. Bootstrap will also gracefully handle existing metadata of a different version.

r? `@jyn514`
diff --git a/src/bootstrap/metrics.rs b/src/bootstrap/metrics.rs
@@ -14,6 +14,25 @@ use std::io::BufWriter;
 use std::time::{Duration, Instant, SystemTime};
 use sysinfo::{CpuExt, System, SystemExt};
 
+// Update this number whenever a breaking change is made to the build metrics.
+//
+// The output format is versioned for two reasons:
+//
+// - The metadata is intended to be consumed by external tooling, and exposing a format version
+//   helps the tools determine whether they're compatible with a metrics file.
+//
+// - If a developer enables build metrics in their local checkout, making a breaking change to the
+//   metrics format would result in a hard-to-diagnose error message when an existing metrics file
+//   is not compatible with the new changes. With a format version number, bootstrap can discard
+//   incompatible metrics files instead of appending metrics to them.
+//
+// Version changelog:
+//
+// - v0: initial version
+// - v1: replaced JsonNode::Test with JsonNode::TestSuite
+//
+const CURRENT_FORMAT_VERSION: usize = 1;
+
 pub(crate) struct BuildMetrics {
     state: RefCell<MetricsState>,
 }
@@ -57,7 +76,7 @@ impl BuildMetrics {
             duration_excluding_children_sec: Duration::ZERO,
 
             children: Vec::new(),
-            tests: Vec::new(),
+            test_suites: Vec::new(),
         });
     }
 
@@ -84,19 +103,31 @@ impl BuildMetrics {
         }
     }
 
+    pub(crate) fn begin_test_suite(&self, metadata: TestSuiteMetadata, builder: &Builder<'_>) {
+        // Do not record dry runs, as they'd be duplicates of the actual steps.
+        if builder.config.dry_run() {
+            return;
+        }
+
+        let mut state = self.state.borrow_mut();
+        let step = state.running_steps.last_mut().unwrap();
+        step.test_suites.push(TestSuite { metadata, tests: Vec::new() });
+    }
+
     pub(crate) fn record_test(&self, name: &str, outcome: TestOutcome, builder: &Builder<'_>) {
         // Do not record dry runs, as they'd be duplicates of the actual steps.
         if builder.config.dry_run() {
             return;
         }
 
         let mut state = self.state.borrow_mut();
-        state
-            .running_steps
-            .last_mut()
-            .unwrap()
-            .tests
-            .push(Test { name: name.to_string(), outcome });
+        let step = state.running_steps.last_mut().unwrap();
+
+        if let Some(test_suite) = step.test_suites.last_mut() {
+            test_suite.tests.push(Test { name: name.to_string(), outcome });
+        } else {
+            panic!("metrics.record_test() called without calling metrics.begin_test_suite() first");
+        }
     }
 
     fn collect_stats(&self, state: &mut MetricsState) {
@@ -131,7 +162,20 @@ impl BuildMetrics {
         // Some of our CI builds consist of multiple independent CI invocations. Ensure all the
         // previous invocations are still present in the resulting file.
         let mut invocations = match std::fs::read(&dest) {
-            Ok(contents) => t!(serde_json::from_slice::<JsonRoot>(&contents)).invocations,
+            Ok(contents) => {
+                // We first parse just the format_version field to have the check succeed even if
+                // the rest of the contents are not valid anymore.
+                let version: OnlyFormatVersion = t!(serde_json::from_slice(&contents));
+                if version.format_version == CURRENT_FORMAT_VERSION {
+                    t!(serde_json::from_slice::<JsonRoot>(&contents)).invocations
+                } else {
+                    println!(
+                        "warning: overriding existing build/metrics.json, as it's not \
+                         compatible with build metrics format version {CURRENT_FORMAT_VERSION}."
+                    );
+                    Vec::new()
+                }
+            }
             Err(err) => {
                 if err.kind() != std::io::ErrorKind::NotFound {
                     panic!("failed to open existing metrics file at {}: {err}", dest.display());
@@ -149,7 +193,7 @@ impl BuildMetrics {
             children: steps.into_iter().map(|step| self.prepare_json_step(step)).collect(),
         });
 
-        let json = JsonRoot { system_stats, invocations };
+        let json = JsonRoot { format_version: CURRENT_FORMAT_VERSION, system_stats, invocations };
 
         t!(std::fs::create_dir_all(dest.parent().unwrap()));
         let mut file = BufWriter::new(t!(File::create(&dest)));
@@ -159,11 +203,7 @@ impl BuildMetrics {
     fn prepare_json_step(&self, step: StepMetrics) -> JsonNode {
         let mut children = Vec::new();
         children.extend(step.children.into_iter().map(|child| self.prepare_json_step(child)));
-        children.extend(
-            step.tests
-                .into_iter()
-                .map(|test| JsonNode::Test { name: test.name, outcome: test.outcome }),
-        );
+        children.extend(step.test_suites.into_iter().map(JsonNode::TestSuite));
 
         JsonNode::RustbuildStep {
             type_: step.type_,
@@ -198,17 +238,14 @@ struct StepMetrics {
     duration_excluding_children_sec: Duration,
 
     children: Vec<StepMetrics>,
-    tests: Vec<Test>,
-}
-
-struct Test {
-    name: String,
-    outcome: TestOutcome,
+    test_suites: Vec<TestSuite>,
 }
 
 #[derive(Serialize, Deserialize)]
 #[serde(rename_all = "snake_case")]
 struct JsonRoot {
+    #[serde(default)] // For version 0 the field was not present.
+    format_version: usize,
     system_stats: JsonInvocationSystemStats,
     invocations: Vec<JsonInvocation>,
 }
@@ -237,13 +274,41 @@ enum JsonNode {
 
         children: Vec<JsonNode>,
     },
-    Test {
-        name: String,
-        #[serde(flatten)]
-        outcome: TestOutcome,
+    TestSuite(TestSuite),
+}
+
+#[derive(Serialize, Deserialize)]
+struct TestSuite {
+    metadata: TestSuiteMetadata,
+    tests: Vec<Test>,
+}
+
+#[derive(Serialize, Deserialize)]
+#[serde(tag = "kind", rename_all = "snake_case")]
+pub(crate) enum TestSuiteMetadata {
+    CargoPackage {
+        crates: Vec<String>,
+        target: String,
+        host: String,
+        stage: u32,
+    },
+    Compiletest {
+        suite: String,
+        mode: String,
+        compare_mode: Option<String>,
+        target: String,
+        host: String,
+        stage: u32,
     },
 }
 
+#[derive(Serialize, Deserialize)]
+pub(crate) struct Test {
+    name: String,
+    #[serde(flatten)]
+    outcome: TestOutcome,
+}
+
 #[derive(Serialize, Deserialize)]
 #[serde(tag = "outcome", rename_all = "snake_case")]
 pub(crate) enum TestOutcome {
@@ -266,3 +331,9 @@ struct JsonInvocationSystemStats {
 struct JsonStepSystemStats {
     cpu_utilization_percent: f64,
 }
+
+#[derive(Deserialize)]
+struct OnlyFormatVersion {
+    #[serde(default)] // For version 0 the field was not present.
+    format_version: usize,
+}
diff --git a/src/bootstrap/test.rs b/src/bootstrap/test.rs
@@ -317,6 +317,17 @@ impl Step for Cargo {
         cargo.env("CARGO_TEST_DISABLE_NIGHTLY", "1");
         cargo.env("PATH", &path_for_cargo(builder, compiler));
 
+        #[cfg(feature = "build-metrics")]
+        builder.metrics.begin_test_suite(
+            crate::metrics::TestSuiteMetadata::CargoPackage {
+                crates: vec!["cargo".into()],
+                target: self.host.triple.to_string(),
+                host: self.host.triple.to_string(),
+                stage: self.stage,
+            },
+            builder,
+        );
+
         let _time = util::timeit(&builder);
         add_flags_and_try_run_tests(builder, &mut cargo);
     }
@@ -1699,6 +1710,19 @@ note: if you're sure you want to do this, please open an issue as to why. In the
 
         builder.ci_env.force_coloring_in_ci(&mut cmd);
 
+        #[cfg(feature = "build-metrics")]
+        builder.metrics.begin_test_suite(
+            crate::metrics::TestSuiteMetadata::Compiletest {
+                suite: suite.into(),
+                mode: mode.into(),
+                compare_mode: None,
+                target: self.target.triple.to_string(),
+                host: self.compiler.host.triple.to_string(),
+                stage: self.compiler.stage,
+            },
+            builder,
+        );
+
         builder.info(&format!(
             "Check compiletest suite={} mode={} ({} -> {})",
             suite, mode, &compiler.host, target
@@ -1708,6 +1732,20 @@ note: if you're sure you want to do this, please open an issue as to why. In the
 
         if let Some(compare_mode) = compare_mode {
             cmd.arg("--compare-mode").arg(compare_mode);
+
+            #[cfg(feature = "build-metrics")]
+            builder.metrics.begin_test_suite(
+                crate::metrics::TestSuiteMetadata::Compiletest {
+                    suite: suite.into(),
+                    mode: mode.into(),
+                    compare_mode: Some(compare_mode.into()),
+                    target: self.target.triple.to_string(),
+                    host: self.compiler.host.triple.to_string(),
+                    stage: self.compiler.stage,
+                },
+                builder,
+            );
+
             builder.info(&format!(
                 "Check compiletest suite={} mode={} compare_mode={} ({} -> {})",
                 suite, mode, compare_mode, &compiler.host, target
@@ -2034,6 +2072,17 @@ fn run_cargo_test(
     let mut cargo =
         prepare_cargo_test(cargo, libtest_args, crates, primary_crate, compiler, target, builder);
     let _time = util::timeit(&builder);
+
+    #[cfg(feature = "build-metrics")]
+    builder.metrics.begin_test_suite(
+        crate::metrics::TestSuiteMetadata::CargoPackage {
+            crates: crates.iter().map(|c| c.to_string()).collect(),
+            target: target.triple.to_string(),
+            host: compiler.host.triple.to_string(),
+            stage: compiler.stage,
+        },
+        builder,
+    );
     add_flags_and_try_run_tests(builder, &mut cargo)
 }
 
