Rollup merge of #131870 - Zalathar:test-collector, r=jieyouxu

jieyouxu · web-flow · commit 43e50932f2f8 · 2024-10-18T14:52:26.000+01:00
compiletest: Store test collection context/state in two structs

This is another incremental cleanup that untangles some of the parameter passing during test collection, making it easier to see which pieces of context information are read-only, and making it easier to find where each field is used.
diff --git a/src/tools/compiletest/src/lib.rs b/src/tools/compiletest/src/lib.rs
@@ -464,9 +464,7 @@ pub fn run_tests(config: Arc<Config>) {
     // structure for each test (or each revision of a multi-revision test).
     let mut tests = Vec::new();
     for c in configs {
-        let mut found_paths = HashSet::new();
-        make_tests(c, &mut tests, &mut found_paths);
-        check_overlapping_tests(&found_paths);
+        tests.extend(collect_and_make_tests(c));
     }
 
     tests.sort_by(|a, b| a.desc.name.as_slice().cmp(&b.desc.name.as_slice()));
@@ -545,46 +543,62 @@ pub fn test_opts(config: &Config) -> test::TestOpts {
     }
 }
 
+/// Read-only context data used during test collection.
+struct TestCollectorCx {
+    config: Arc<Config>,
+    cache: HeadersCache,
+    common_inputs_stamp: Stamp,
+    modified_tests: Vec<PathBuf>,
+}
+
+/// Mutable state used during test collection.
+struct TestCollector {
+    tests: Vec<test::TestDescAndFn>,
+    found_path_stems: HashSet<PathBuf>,
+    poisoned: bool,
+}
+
 /// Creates libtest structures for every test/revision in the test suite directory.
 ///
 /// This always inspects _all_ test files in the suite (e.g. all 17k+ ui tests),
 /// regardless of whether any filters/tests were specified on the command-line,
 /// because filtering is handled later by libtest.
-pub fn make_tests(
-    config: Arc<Config>,
-    tests: &mut Vec<test::TestDescAndFn>,
-    found_paths: &mut HashSet<PathBuf>,
-) {
+pub fn collect_and_make_tests(config: Arc<Config>) -> Vec<test::TestDescAndFn> {
     debug!("making tests from {:?}", config.src_base.display());
-    let inputs = common_inputs_stamp(&config);
+    let common_inputs_stamp = common_inputs_stamp(&config);
     let modified_tests = modified_tests(&config, &config.src_base).unwrap_or_else(|err| {
         panic!("modified_tests got error from dir: {}, error: {}", config.src_base.display(), err)
     });
-
     let cache = HeadersCache::load(&config);
-    let mut poisoned = false;
-    collect_tests_from_dir(
-        config.clone(),
-        &cache,
-        &config.src_base,
-        &PathBuf::new(),
-        &inputs,
-        tests,
-        found_paths,
-        &modified_tests,
-        &mut poisoned,
-    )
-    .unwrap_or_else(|reason| {
-        panic!("Could not read tests from {}: {reason}", config.src_base.display())
-    });
+
+    let cx = TestCollectorCx { config, cache, common_inputs_stamp, modified_tests };
+    let mut collector =
+        TestCollector { tests: vec![], found_path_stems: HashSet::new(), poisoned: false };
+
+    collect_tests_from_dir(&cx, &mut collector, &cx.config.src_base, &PathBuf::new())
+        .unwrap_or_else(|reason| {
+            panic!("Could not read tests from {}: {reason}", cx.config.src_base.display())
+        });
+
+    let TestCollector { tests, found_path_stems, poisoned } = collector;
 
     if poisoned {
         eprintln!();
         panic!("there are errors in tests");
     }
+
+    check_for_overlapping_test_paths(&found_path_stems);
+
+    tests
 }
 
-/// Returns a stamp constructed from input files common to all test cases.
+/// Returns the most recent last-modified timestamp from among the input files
+/// that are considered relevant to all tests (e.g. the compiler, std, and
+/// compiletest itself).
+///
+/// (Some of these inputs aren't actually relevant to _all_ tests, but they are
+/// common to some subset of tests, and are hopefully unlikely to be modified
+/// while working on other tests.)
 fn common_inputs_stamp(config: &Config) -> Stamp {
     let rust_src_dir = config.find_rust_src_root().expect("Could not find Rust source root");
 
@@ -662,15 +676,10 @@ fn modified_tests(config: &Config, dir: &Path) -> Result<Vec<PathBuf>, String> {
 /// Recursively scans a directory to find test files and create test structures
 /// that will be handed over to libtest.
 fn collect_tests_from_dir(
-    config: Arc<Config>,
-    cache: &HeadersCache,
+    cx: &TestCollectorCx,
+    collector: &mut TestCollector,
     dir: &Path,
     relative_dir_path: &Path,
-    inputs: &Stamp,
-    tests: &mut Vec<test::TestDescAndFn>,
-    found_paths: &mut HashSet<PathBuf>,
-    modified_tests: &Vec<PathBuf>,
-    poisoned: &mut bool,
 ) -> io::Result<()> {
     // Ignore directories that contain a file named `compiletest-ignore-dir`.
     if dir.join("compiletest-ignore-dir").exists() {
@@ -679,7 +688,7 @@ fn collect_tests_from_dir(
 
     // For run-make tests, a "test file" is actually a directory that contains
     // an `rmake.rs` or `Makefile`"
-    if config.mode == Mode::RunMake {
+    if cx.config.mode == Mode::RunMake {
         if dir.join("Makefile").exists() && dir.join("rmake.rs").exists() {
             return Err(io::Error::other(
                 "run-make tests cannot have both `Makefile` and `rmake.rs`",
@@ -691,7 +700,7 @@ fn collect_tests_from_dir(
                 file: dir.to_path_buf(),
                 relative_dir: relative_dir_path.parent().unwrap().to_path_buf(),
             };
-            tests.extend(make_test(config, cache, &paths, inputs, poisoned));
+            make_test(cx, collector, &paths);
             // This directory is a test, so don't try to find other tests inside it.
             return Ok(());
         }
@@ -703,7 +712,7 @@ fn collect_tests_from_dir(
     // sequential loop because otherwise, if we do it in the
     // tests themselves, they race for the privilege of
     // creating the directories and sometimes fail randomly.
-    let build_dir = output_relative_path(&config, relative_dir_path);
+    let build_dir = output_relative_path(&cx.config, relative_dir_path);
     fs::create_dir_all(&build_dir).unwrap();
 
     // Add each `.rs` file as a test, and recurse further on any
@@ -715,33 +724,25 @@ fn collect_tests_from_dir(
         let file_path = file.path();
         let file_name = file.file_name();
 
-        if is_test(&file_name) && (!config.only_modified || modified_tests.contains(&file_path)) {
+        if is_test(&file_name)
+            && (!cx.config.only_modified || cx.modified_tests.contains(&file_path))
+        {
             // We found a test file, so create the corresponding libtest structures.
             debug!("found test file: {:?}", file_path.display());
 
             // Record the stem of the test file, to check for overlaps later.
             let rel_test_path = relative_dir_path.join(file_path.file_stem().unwrap());
-            found_paths.insert(rel_test_path);
+            collector.found_path_stems.insert(rel_test_path);
 
             let paths =
                 TestPaths { file: file_path, relative_dir: relative_dir_path.to_path_buf() };
-            tests.extend(make_test(config.clone(), cache, &paths, inputs, poisoned))
+            make_test(cx, collector, &paths);
         } else if file_path.is_dir() {
             // Recurse to find more tests in a subdirectory.
             let relative_file_path = relative_dir_path.join(file.file_name());
             if &file_name != "auxiliary" {
                 debug!("found directory: {:?}", file_path.display());
-                collect_tests_from_dir(
-                    config.clone(),
-                    cache,
-                    &file_path,
-                    &relative_file_path,
-                    inputs,
-                    tests,
-                    found_paths,
-                    modified_tests,
-                    poisoned,
-                )?;
+                collect_tests_from_dir(cx, collector, &file_path, &relative_file_path)?;
             }
         } else {
             debug!("found other file/directory: {:?}", file_path.display());
@@ -765,17 +766,11 @@ pub fn is_test(file_name: &OsString) -> bool {
 
 /// For a single test file, creates one or more test structures (one per revision)
 /// that can be handed over to libtest to run, possibly in parallel.
-fn make_test(
-    config: Arc<Config>,
-    cache: &HeadersCache,
-    testpaths: &TestPaths,
-    inputs: &Stamp,
-    poisoned: &mut bool,
-) -> Vec<test::TestDescAndFn> {
+fn make_test(cx: &TestCollectorCx, collector: &mut TestCollector, testpaths: &TestPaths) {
     // For run-make tests, each "test file" is actually a _directory_ containing
     // an `rmake.rs` or `Makefile`. But for the purposes of directive parsing,
     // we want to look at that recipe file, not the directory itself.
-    let test_path = if config.mode == Mode::RunMake {
+    let test_path = if cx.config.mode == Mode::RunMake {
         if testpaths.file.join("rmake.rs").exists() && testpaths.file.join("Makefile").exists() {
             panic!("run-make tests cannot have both `rmake.rs` and `Makefile`");
         }
@@ -792,57 +787,57 @@ fn make_test(
     };
 
     // Scan the test file to discover its revisions, if any.
-    let early_props = EarlyProps::from_file(&config, &test_path);
+    let early_props = EarlyProps::from_file(&cx.config, &test_path);
 
     // Normally we create one libtest structure per revision, with two exceptions:
     // - If a test doesn't use revisions, create a dummy revision (None) so that
     //   the test can still run.
     // - Incremental tests inherently can't run their revisions in parallel, so
     //   we treat them like non-revisioned tests here. Incremental revisions are
     //   handled internally by `runtest::run` instead.
-    let revisions = if early_props.revisions.is_empty() || config.mode == Mode::Incremental {
+    let revisions = if early_props.revisions.is_empty() || cx.config.mode == Mode::Incremental {
         vec![None]
     } else {
         early_props.revisions.iter().map(|r| Some(r.as_str())).collect()
     };
 
-    // For each revision (or the sole dummy revision), create and return a
+    // For each revision (or the sole dummy revision), create and append a
     // `test::TestDescAndFn` that can be handed over to libtest.
-    revisions
-        .into_iter()
-        .map(|revision| {
-            // Create a test name and description to hand over to libtest.
-            let src_file =
-                std::fs::File::open(&test_path).expect("open test file to parse ignores");
-            let test_name = crate::make_test_name(&config, testpaths, revision);
-            // Create a libtest description for the test/revision.
-            // This is where `ignore-*`/`only-*`/`needs-*` directives are handled,
-            // because they need to set the libtest ignored flag.
-            let mut desc = make_test_description(
-                &config, cache, test_name, &test_path, src_file, revision, poisoned,
-            );
+    collector.tests.extend(revisions.into_iter().map(|revision| {
+        // Create a test name and description to hand over to libtest.
+        let src_file = fs::File::open(&test_path).expect("open test file to parse ignores");
+        let test_name = make_test_name(&cx.config, testpaths, revision);
+        // Create a libtest description for the test/revision.
+        // This is where `ignore-*`/`only-*`/`needs-*` directives are handled,
+        // because they need to set the libtest ignored flag.
+        let mut desc = make_test_description(
+            &cx.config,
+            &cx.cache,
+            test_name,
+            &test_path,
+            src_file,
+            revision,
+            &mut collector.poisoned,
+        );
 
-            // If a test's inputs haven't changed since the last time it ran,
-            // mark it as ignored so that libtest will skip it.
-            if !config.force_rerun
-                && is_up_to_date(&config, testpaths, &early_props, revision, inputs)
-            {
-                desc.ignore = true;
-                // Keep this in sync with the "up-to-date" message detected by bootstrap.
-                desc.ignore_message = Some("up-to-date");
-            }
+        // If a test's inputs haven't changed since the last time it ran,
+        // mark it as ignored so that libtest will skip it.
+        if !cx.config.force_rerun && is_up_to_date(cx, testpaths, &early_props, revision) {
+            desc.ignore = true;
+            // Keep this in sync with the "up-to-date" message detected by bootstrap.
+            desc.ignore_message = Some("up-to-date");
+        }
 
-            // Create the callback that will run this test/revision when libtest calls it.
-            let testfn = make_test_closure(config.clone(), testpaths, revision);
+        // Create the callback that will run this test/revision when libtest calls it.
+        let testfn = make_test_closure(Arc::clone(&cx.config), testpaths, revision);
 
-            test::TestDescAndFn { desc, testfn }
-        })
-        .collect()
+        test::TestDescAndFn { desc, testfn }
+    }));
 }
 
 /// The path of the `stamp` file that gets created or updated whenever a
 /// particular test completes successfully.
-fn stamp(config: &Config, testpaths: &TestPaths, revision: Option<&str>) -> PathBuf {
+fn stamp_file_path(config: &Config, testpaths: &TestPaths, revision: Option<&str>) -> PathBuf {
     output_base_dir(config, testpaths, revision).join("stamp")
 }
 
@@ -891,21 +886,20 @@ fn files_related_to_test(
 /// (This is not very reliable in some circumstances, so the `--force-rerun`
 /// flag can be used to ignore up-to-date checking and always re-run tests.)
 fn is_up_to_date(
-    config: &Config,
+    cx: &TestCollectorCx,
     testpaths: &TestPaths,
     props: &EarlyProps,
     revision: Option<&str>,
-    inputs: &Stamp, // Last-modified timestamp of the compiler, compiletest etc
 ) -> bool {
-    let stamp_name = stamp(config, testpaths, revision);
+    let stamp_file_path = stamp_file_path(&cx.config, testpaths, revision);
     // Check the config hash inside the stamp file.
-    let contents = match fs::read_to_string(&stamp_name) {
+    let contents = match fs::read_to_string(&stamp_file_path) {
         Ok(f) => f,
         Err(ref e) if e.kind() == ErrorKind::InvalidData => panic!("Can't read stamp contents"),
         // The test hasn't succeeded yet, so it is not up-to-date.
         Err(_) => return false,
     };
-    let expected_hash = runtest::compute_stamp_hash(config);
+    let expected_hash = runtest::compute_stamp_hash(&cx.config);
     if contents != expected_hash {
         // Some part of compiletest configuration has changed since the test
         // last succeeded, so it is not up-to-date.
@@ -914,14 +908,14 @@ fn is_up_to_date(
 
     // Check the timestamp of the stamp file against the last modified time
     // of all files known to be relevant to the test.
-    let mut inputs = inputs.clone();
-    for path in files_related_to_test(config, testpaths, props, revision) {
-        inputs.add_path(&path);
+    let mut inputs_stamp = cx.common_inputs_stamp.clone();
+    for path in files_related_to_test(&cx.config, testpaths, props, revision) {
+        inputs_stamp.add_path(&path);
     }
 
     // If no relevant files have been modified since the stamp file was last
     // written, the test is up-to-date.
-    inputs < Stamp::from_path(&stamp_name)
+    inputs_stamp < Stamp::from_path(&stamp_file_path)
 }
 
 /// The maximum of a set of file-modified timestamps.
@@ -1029,11 +1023,11 @@ fn make_test_closure(
 /// To avoid problems, we forbid test names from overlapping in this way.
 ///
 /// See <https://github.com/rust-lang/rust/pull/109509> for more context.
-fn check_overlapping_tests(found_paths: &HashSet<PathBuf>) {
+fn check_for_overlapping_test_paths(found_path_stems: &HashSet<PathBuf>) {
     let mut collisions = Vec::new();
-    for path in found_paths {
+    for path in found_path_stems {
         for ancestor in path.ancestors().skip(1) {
-            if found_paths.contains(ancestor) {
+            if found_path_stems.contains(ancestor) {
                 collisions.push((path, ancestor));
             }
         }
diff --git a/src/tools/compiletest/src/runtest.rs b/src/tools/compiletest/src/runtest.rs
@@ -29,7 +29,7 @@ use crate::errors::{self, Error, ErrorKind};
 use crate::header::TestProps;
 use crate::read2::{Truncated, read2_abbreviated};
 use crate::util::{PathBufExt, add_dylib_path, logv, static_regex};
-use crate::{ColorConfig, json};
+use crate::{ColorConfig, json, stamp_file_path};
 
 mod debugger;
 
@@ -2595,8 +2595,8 @@ impl<'test> TestCx<'test> {
     }
 
     fn create_stamp(&self) {
-        let stamp = crate::stamp(&self.config, self.testpaths, self.revision);
-        fs::write(&stamp, compute_stamp_hash(&self.config)).unwrap();
+        let stamp_file_path = stamp_file_path(&self.config, self.testpaths, self.revision);
+        fs::write(&stamp_file_path, compute_stamp_hash(&self.config)).unwrap();
     }
 
     fn init_incremental_test(&self) {
