From a4a395ac998fcd7b7fdea6ef3aaef6c87d6e8962 Mon Sep 17 00:00:00 2001 From: Zane Duffield Date: Sun, 21 Apr 2024 23:03:06 +1000 Subject: [PATCH 1/2] Avoid iterating files when `--exact` is passed in This fixes a quadratic performance issue in which the test directories are iterated for every test case when run under nextest. --- src/runner.rs | 70 +++++++++++++++++++++++++++---- src/utils.rs | 73 +++++++++++++++++++++++++++++++++ tests/files/::colon::dir/::.txt | 1 + tests/files/::colon::dir/a.txt | 1 + 4 files changed, 136 insertions(+), 9 deletions(-) create mode 100644 tests/files/::colon::dir/::.txt create mode 100644 tests/files/::colon::dir/a.txt diff --git a/src/runner.rs b/src/runner.rs index 1582cf0bb..593d424bf 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -11,8 +11,7 @@ use libtest_mimic::{Arguments, Trial}; pub fn runner(requirements: &[Requirements]) -> ExitCode { let args = Arguments::from_args(); - let mut tests: Vec<_> = requirements.iter().flat_map(|req| req.expand()).collect(); - tests.sort_unstable_by(|a, b| a.name().cmp(b.name())); + let tests = find_tests(&args, requirements); let conclusion = libtest_mimic::run(&args, tests); @@ -25,6 +24,50 @@ pub fn runner(requirements: &[Requirements]) -> ExitCode { conclusion.exit_code() } +fn find_tests(args: &Arguments, requirements: &[Requirements]) -> Vec { + let tests: Vec<_> = if let Some(exact_filter) = exact_filter(args) { + let exact_tests: Vec<_> = requirements + .iter() + .flat_map(|req| req.exact(exact_filter)) + .collect(); + + if is_nextest() { + if exact_tests.is_empty() { + panic!("Failed to find exact match for filter {exact_filter}"); + } else if exact_tests.len() > 1 { + panic!( + "Only expected one but found {} exact matches for filter {exact_filter}", + exact_tests.len() + ); + } + } + exact_tests + } else if is_full_scan_forbidden(args) { + panic!("Exact filter was expected to be used"); + } else { + let mut tests: Vec<_> = requirements.iter().flat_map(|req| req.expand()).collect(); + tests.sort_unstable_by(|a, b| a.name().cmp(b.name())); + tests + }; + tests +} + +fn is_nextest() -> bool { + std::env::var("NEXTEST").as_deref() == Ok("1") +} + +fn is_full_scan_forbidden(args: &Arguments) -> bool { + !args.list && std::env::var("__DATATEST_FULL_SCAN_FORBIDDEN").as_deref() == Ok("1") +} + +fn exact_filter(args: &Arguments) -> Option<&str> { + if args.exact && args.skip.is_empty() { + args.filter.as_deref() + } else { + None + } +} + #[doc(hidden)] pub struct Requirements { test: TestFn, @@ -49,6 +92,21 @@ impl Requirements { } } + fn trial(&self, path: Utf8PathBuf) -> Trial { + let testfn = self.test; + let name = utils::derive_test_name(&self.root, &path, &self.test_name); + Trial::test(name, move || { + testfn + .call(&path) + .map_err(|err| format!("{:?}", err).into()) + }) + } + + fn exact(&self, filter: &str) -> Option { + let path = utils::derive_test_path(&self.root, filter, &self.test_name)?; + path.exists().then(|| self.trial(path)) + } + /// Scans all files in a given directory, finds matching ones and generates a test descriptor /// for each of them. fn expand(&self) -> Vec { @@ -66,13 +124,7 @@ impl Requirements { error ) }) { - let testfn = self.test; - let name = utils::derive_test_name(&self.root, &path, &self.test_name); - Some(Trial::test(name, move || { - testfn - .call(&path) - .map_err(|err| format!("{:?}", err).into()) - })) + Some(self.trial(path)) } else { None } diff --git a/src/utils.rs b/src/utils.rs index bcd64789a..11a8a5ffa 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -33,3 +33,76 @@ pub fn derive_test_name(root: &Utf8Path, path: &Utf8Path, test_name: &str) -> St format!("{}::{}", test_name, relative) } + +pub fn derive_test_path(root: &Utf8Path, filter: &str, test_name: &str) -> Option { + let relative = filter.strip_prefix(test_name)?.strip_prefix("::")?; + Some(root.join(relative)) +} + +#[cfg(test)] +mod tests { + use super::*; + + mod derive_path { + use super::*; + + #[test] + fn missing_test_name() { + assert_eq!(derive_test_path("root".into(), "file", "test_name"), None); + } + + #[test] + fn missing_colons() { + assert_eq!( + derive_test_path("root".into(), "test_name", "test_name"), + None + ); + } + + #[test] + fn is_relative_to_root() { + assert_eq!( + derive_test_path("root".into(), "test_name::file", "test_name"), + Some("root/file".into()) + ); + assert_eq!( + derive_test_path("root2".into(), "test_name::file", "test_name"), + Some("root2/file".into()) + ); + } + + #[test] + fn nested_dirs() { + assert_eq!( + derive_test_path("root".into(), "test_name::dir/dir2/file", "test_name"), + Some("root/dir/dir2/file".into()) + ); + } + + #[test] + fn subsequent_module_separators_remain() { + assert_eq!( + derive_test_path("root".into(), "test_name::mod::file", "test_name"), + Some("root/mod::file".into()) + ); + } + + #[test] + fn inverse_of_derive_test_name() { + let root: Utf8PathBuf = "root".into(); + for (path, test_name) in [ + (root.join("foo/bar.txt"), "test_name"), + (root.join("foo::bar.txt"), "test_name"), + (root.join("foo/bar/baz"), "test_name"), + (root.join("foo"), "test_name::mod"), + (root.join("🦀"), "🚀::🚀"), + ] { + let derived_test_name = derive_test_name(&root, &path, test_name); + assert_eq!( + derive_test_path(&root, &derived_test_name, test_name), + Some(path) + ); + } + } + } +} diff --git a/tests/files/::colon::dir/::.txt b/tests/files/::colon::dir/::.txt new file mode 100644 index 000000000..eaabfa772 --- /dev/null +++ b/tests/files/::colon::dir/::.txt @@ -0,0 +1 @@ +floop \ No newline at end of file diff --git a/tests/files/::colon::dir/a.txt b/tests/files/::colon::dir/a.txt new file mode 100644 index 000000000..ca2fb7494 --- /dev/null +++ b/tests/files/::colon::dir/a.txt @@ -0,0 +1 @@ +flarp \ No newline at end of file From 32942ad0d2ef43aaaf0d854d653ab491c5fda552 Mon Sep 17 00:00:00 2001 From: Zane Duffield Date: Wed, 24 Apr 2024 23:28:19 +1000 Subject: [PATCH 2/2] Add integration test for `cargo nextest run` on the example test harness --- Cargo.toml | 4 ++++ tests/run_example.rs | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) create mode 100644 tests/run_example.rs diff --git a/Cargo.toml b/Cargo.toml index 4df086fa4..e4e029f1c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -20,3 +20,7 @@ walkdir = "2.5.0" [[test]] name = "example" harness = false + +[[test]] +name = "run_example" +harness = true diff --git a/tests/run_example.rs b/tests/run_example.rs new file mode 100644 index 000000000..b884d0de0 --- /dev/null +++ b/tests/run_example.rs @@ -0,0 +1,42 @@ +// Copyright (c) The datatest-stable Contributors +// SPDX-License-Identifier: MIT OR Apache-2.0 + +#[test] +fn run_example() { + let output = std::process::Command::new("cargo") + .args(["nextest", "run", "--test=example", "--color=never"]) + .env("__DATATEST_FULL_SCAN_FORBIDDEN", "1") + .output() + .expect("Failed to run `cargo nextest`"); + + // It's a pain to make assertions on byte slices (even a subslice check isn't easy) + // and it's also difficult to print nice error messages. So we just assume all + // nextest output will be utf8 and convert it. + let stderr = std::str::from_utf8(&output.stderr).expect("cargo nextest stderr should be utf-8"); + + assert!( + output.status.success(), + "Command failed (exit status: {}, stderr: {stderr})", + output.status + ); + + let lines: &[&str] = &[ + "datatest-stable::example test_artifact::::colon::dir/::.txt", + "datatest-stable::example test_artifact::::colon::dir/a.txt", + "datatest-stable::example test_artifact::a.txt", + "datatest-stable::example test_artifact_utf8::::colon::dir/::.txt", + "datatest-stable::example test_artifact::b.txt", + "datatest-stable::example test_artifact_utf8::::colon::dir/a.txt", + "datatest-stable::example test_artifact_utf8::a.txt", + "datatest-stable::example test_artifact_utf8::c.skip.txt", + "datatest-stable::example test_artifact_utf8::b.txt", + "9 tests run: 9 passed, 0 skipped", + ]; + + for line in lines { + assert!( + stderr.contains(line), + "Expected to find substring\n {line}\nin stderr\n {stderr}", + ); + } +}