From 21a020881da6e7bbf3851971e1ec39f844616436 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Fri, 18 Oct 2024 11:32:49 +1100 Subject: [PATCH 1/5] Consolidate test collection state in one place --- src/tools/compiletest/src/lib.rs | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/src/tools/compiletest/src/lib.rs b/src/tools/compiletest/src/lib.rs index 2e66c084dd7..7abdfadbfa8 100644 --- a/src/tools/compiletest/src/lib.rs +++ b/src/tools/compiletest/src/lib.rs @@ -464,9 +464,7 @@ pub fn run_tests(config: Arc) { // 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())); @@ -550,27 +548,26 @@ pub fn test_opts(config: &Config) -> test::TestOpts { /// 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, - tests: &mut Vec, - found_paths: &mut HashSet, -) { +pub fn collect_and_make_tests(config: Arc) -> Vec { debug!("making tests from {:?}", config.src_base.display()); let inputs = 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 tests = vec![]; + let mut found_paths = HashSet::new(); let mut poisoned = false; + collect_tests_from_dir( config.clone(), &cache, &config.src_base, &PathBuf::new(), &inputs, - tests, - found_paths, + &mut tests, + &mut found_paths, &modified_tests, &mut poisoned, ) @@ -582,6 +579,10 @@ pub fn make_tests( eprintln!(); panic!("there are errors in tests"); } + + check_overlapping_tests(&found_paths); + + tests } /// Returns a stamp constructed from input files common to all test cases. From 932e9f01b1f60370b3ffcb370cdbf3b62ac2e718 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Fri, 18 Oct 2024 11:32:49 +1100 Subject: [PATCH 2/5] Store test collection context/state in two structs --- src/tools/compiletest/src/lib.rs | 157 ++++++++++++++----------------- 1 file changed, 72 insertions(+), 85 deletions(-) diff --git a/src/tools/compiletest/src/lib.rs b/src/tools/compiletest/src/lib.rs index 7abdfadbfa8..acf4860cc63 100644 --- a/src/tools/compiletest/src/lib.rs +++ b/src/tools/compiletest/src/lib.rs @@ -543,6 +543,21 @@ pub fn test_opts(config: &Config) -> test::TestOpts { } } +/// Read-only context data used during test collection. +struct TestCollectorCx { + config: Arc, + cache: HeadersCache, + inputs: Stamp, + modified_tests: Vec, +} + +/// Mutable state used during test collection. +struct TestCollector { + tests: Vec, + found_paths: HashSet, + 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), @@ -556,24 +571,16 @@ pub fn collect_and_make_tests(config: Arc) -> Vec { }); let cache = HeadersCache::load(&config); - let mut tests = vec![]; - let mut found_paths = HashSet::new(); - let mut poisoned = false; + let cx = TestCollectorCx { config, cache, inputs, modified_tests }; + let mut collector = + TestCollector { tests: vec![], found_paths: HashSet::new(), poisoned: false }; - collect_tests_from_dir( - config.clone(), - &cache, - &config.src_base, - &PathBuf::new(), - &inputs, - &mut tests, - &mut found_paths, - &modified_tests, - &mut poisoned, - ) - .unwrap_or_else(|reason| { - panic!("Could not read tests from {}: {reason}", config.src_base.display()) - }); + 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_paths, poisoned } = collector; if poisoned { eprintln!(); @@ -663,15 +670,10 @@ fn modified_tests(config: &Config, dir: &Path) -> Result, 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, - cache: &HeadersCache, + cx: &TestCollectorCx, + collector: &mut TestCollector, dir: &Path, relative_dir_path: &Path, - inputs: &Stamp, - tests: &mut Vec, - found_paths: &mut HashSet, - modified_tests: &Vec, - poisoned: &mut bool, ) -> io::Result<()> { // Ignore directories that contain a file named `compiletest-ignore-dir`. if dir.join("compiletest-ignore-dir").exists() { @@ -680,7 +682,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`", @@ -692,7 +694,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(()); } @@ -704,7 +706,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 @@ -716,33 +718,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_paths.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()); @@ -766,17 +760,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, - cache: &HeadersCache, - testpaths: &TestPaths, - inputs: &Stamp, - poisoned: &mut bool, -) -> Vec { +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`"); } @@ -793,7 +781,7 @@ 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 @@ -801,44 +789,44 @@ fn make_test( // - 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 @@ -892,13 +880,12 @@ 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_name = stamp(&cx.config, testpaths, revision); // Check the config hash inside the stamp file. let contents = match fs::read_to_string(&stamp_name) { Ok(f) => f, @@ -906,7 +893,7 @@ fn is_up_to_date( // 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. @@ -915,8 +902,8 @@ 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) { + let mut inputs = cx.inputs.clone(); + for path in files_related_to_test(&cx.config, testpaths, props, revision) { inputs.add_path(&path); } From c4c62a5591bdaa061ef6bab7fc18b29ff02a00c4 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Fri, 18 Oct 2024 13:30:31 +1100 Subject: [PATCH 3/5] Rename `inputs` to `common_inputs_stamp` The new name makes it clearer that this is a timestamp, and is collected from input files considered common to all tests. --- src/tools/compiletest/src/lib.rs | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/tools/compiletest/src/lib.rs b/src/tools/compiletest/src/lib.rs index acf4860cc63..5c148e37143 100644 --- a/src/tools/compiletest/src/lib.rs +++ b/src/tools/compiletest/src/lib.rs @@ -547,7 +547,7 @@ pub fn test_opts(config: &Config) -> test::TestOpts { struct TestCollectorCx { config: Arc, cache: HeadersCache, - inputs: Stamp, + common_inputs_stamp: Stamp, modified_tests: Vec, } @@ -565,13 +565,13 @@ struct TestCollector { /// because filtering is handled later by libtest. pub fn collect_and_make_tests(config: Arc) -> Vec { 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 cx = TestCollectorCx { config, cache, inputs, modified_tests }; + let cx = TestCollectorCx { config, cache, common_inputs_stamp, modified_tests }; let mut collector = TestCollector { tests: vec![], found_paths: HashSet::new(), poisoned: false }; @@ -592,7 +592,13 @@ pub fn collect_and_make_tests(config: Arc) -> Vec { 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"); @@ -902,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 = cx.inputs.clone(); + let mut inputs_stamp = cx.common_inputs_stamp.clone(); for path in files_related_to_test(&cx.config, testpaths, props, revision) { - inputs.add_path(&path); + 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_name) } /// The maximum of a set of file-modified timestamps. From 3f8d87beda3f615f1df19504f908fbe9e5824b83 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Fri, 18 Oct 2024 13:32:29 +1100 Subject: [PATCH 4/5] Rename `found_paths` to `found_path_stems` --- src/tools/compiletest/src/lib.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/tools/compiletest/src/lib.rs b/src/tools/compiletest/src/lib.rs index 5c148e37143..9962487ef81 100644 --- a/src/tools/compiletest/src/lib.rs +++ b/src/tools/compiletest/src/lib.rs @@ -554,7 +554,7 @@ struct TestCollectorCx { /// Mutable state used during test collection. struct TestCollector { tests: Vec, - found_paths: HashSet, + found_path_stems: HashSet, poisoned: bool, } @@ -573,21 +573,21 @@ pub fn collect_and_make_tests(config: Arc) -> Vec { let cx = TestCollectorCx { config, cache, common_inputs_stamp, modified_tests }; let mut collector = - TestCollector { tests: vec![], found_paths: HashSet::new(), poisoned: false }; + 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_paths, poisoned } = collector; + let TestCollector { tests, found_path_stems, poisoned } = collector; if poisoned { eprintln!(); panic!("there are errors in tests"); } - check_overlapping_tests(&found_paths); + check_for_overlapping_test_paths(&found_path_stems); tests } @@ -732,7 +732,7 @@ fn collect_tests_from_dir( // 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()); - collector.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() }; @@ -1023,11 +1023,11 @@ fn make_test_closure( /// To avoid problems, we forbid test names from overlapping in this way. /// /// See for more context. -fn check_overlapping_tests(found_paths: &HashSet) { +fn check_for_overlapping_test_paths(found_path_stems: &HashSet) { 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)); } } From 554097678a990255d580078e9d7f6b1d81d78752 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Fri, 18 Oct 2024 16:11:15 +1100 Subject: [PATCH 5/5] Rename `stamp` to `stamp_file_path` --- src/tools/compiletest/src/lib.rs | 8 ++++---- src/tools/compiletest/src/runtest.rs | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/tools/compiletest/src/lib.rs b/src/tools/compiletest/src/lib.rs index 9962487ef81..d045c6fe053 100644 --- a/src/tools/compiletest/src/lib.rs +++ b/src/tools/compiletest/src/lib.rs @@ -837,7 +837,7 @@ fn make_test(cx: &TestCollectorCx, collector: &mut TestCollector, testpaths: &Te /// 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,9 +891,9 @@ fn is_up_to_date( props: &EarlyProps, revision: Option<&str>, ) -> bool { - let stamp_name = stamp(&cx.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. @@ -915,7 +915,7 @@ fn is_up_to_date( // If no relevant files have been modified since the stamp file was last // written, the test is up-to-date. - inputs_stamp < Stamp::from_path(&stamp_name) + inputs_stamp < Stamp::from_path(&stamp_file_path) } /// The maximum of a set of file-modified timestamps. diff --git a/src/tools/compiletest/src/runtest.rs b/src/tools/compiletest/src/runtest.rs index 69a47fcd0fb..f0452008304 100644 --- 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) {