Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

compiletest: Store test collection context/state in two structs #131870

Merged
merged 5 commits into from
Oct 18, 2024
Merged

compiletest: Store test collection context/state in two structs #131870

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
21a0208
Consolidate test collection state in one place
Zalathar Oct 18, 2024
932e9f0
Store test collection context/state in two structs
Zalathar Oct 18, 2024
c4c62a5
Rename `inputs` to `common_inputs_stamp`
Zalathar Oct 18, 2024
3f8d87b
Rename `found_paths` to `found_path_stems`
Zalathar Oct 18, 2024
5540976
Rename `stamp` to `stamp_file_path`
Zalathar Oct 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
196 changes: 95 additions & 101 deletions src/tools/compiletest/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
Expand Down Expand Up @@ -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>,
jieyouxu marked this conversation as resolved.
Show resolved Hide resolved
poisoned: bool,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remark: I want to delete this poison thing and eventually just store a vec of errors/diagnostics, but anyway. Possibly still account for --fail-fast, but yeah.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I had an earlier draft where I tried to get rid of poisoned entirely, but in the end I decided to make this PR mostly faithful to the existing behaviour.

At some point in the future, having a list of errors instead of a boolean flag would be great.

}

/// 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");

Expand Down Expand Up @@ -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() {
Expand All @@ -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`",
Expand All @@ -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(());
}
Expand All @@ -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
Expand All @@ -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());
Expand All @@ -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`");
}
Expand All @@ -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")
}

Expand Down Expand Up @@ -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.
Expand All @@ -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.
Expand Down Expand Up @@ -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));
}
}
Expand Down
6 changes: 3 additions & 3 deletions src/tools/compiletest/src/runtest.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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) {
Expand Down
Loading