From 019aeedeb4f834306b41c82f2421fdc1c0c16266 Mon Sep 17 00:00:00 2001 From: Scott Schafer Date: Mon, 27 Feb 2023 10:27:08 -0600 Subject: [PATCH] feat: Use test name for dir when running tests --- crates/cargo-test-macro/src/lib.rs | 30 +++++++++++--- crates/cargo-test-support/src/paths.rs | 54 ++++++++++++++++---------- src/doc/contrib/src/tests/writing.md | 11 +++--- tests/testsuite/main.rs | 48 +++++++++++++++++++++++ 4 files changed, 113 insertions(+), 30 deletions(-) diff --git a/crates/cargo-test-macro/src/lib.rs b/crates/cargo-test-macro/src/lib.rs index aa06f477de0..112c020ed9b 100644 --- a/crates/cargo-test-macro/src/lib.rs +++ b/crates/cargo-test-macro/src/lib.rs @@ -133,6 +133,9 @@ pub fn cargo_test(attr: TokenStream, item: TokenStream) -> TokenStream { add_attr(&mut ret, "ignore", reason); } + let mut test_name = None; + let mut num = 0; + // Find where the function body starts, and add the boilerplate at the start. for token in item { let group = match token { @@ -144,18 +147,35 @@ pub fn cargo_test(attr: TokenStream, item: TokenStream) -> TokenStream { continue; } } + TokenTree::Ident(i) => { + // The first time through it will be `fn` the second time is the + // name of the test. + if test_name.is_none() && num == 1 { + test_name = Some(i.to_string()) + } else { + num += 1; + } + ret.extend(Some(TokenTree::Ident(i))); + continue; + } other => { ret.extend(Some(other)); continue; } }; - let mut new_body = to_token_stream( - r#"let _test_guard = { + let name = &test_name + .clone() + .map(|n| n.split("::").next().unwrap().to_string()) + .unwrap(); + + let mut new_body = to_token_stream(&format!( + r#"let _test_guard = {{ let tmp_dir = option_env!("CARGO_TARGET_TMPDIR"); - cargo_test_support::paths::init_root(tmp_dir) - };"#, - ); + let test_dir = cargo_test_support::paths::test_dir(std::file!(), "{name}"); + cargo_test_support::paths::init_root(tmp_dir, test_dir) + }};"# + )); new_body.extend(group.stream()); ret.extend(Some(TokenTree::from(Group::new( diff --git a/crates/cargo-test-support/src/paths.rs b/crates/cargo-test-support/src/paths.rs index ef1fddb7003..f1fc1bc298f 100644 --- a/crates/cargo-test-support/src/paths.rs +++ b/crates/cargo-test-support/src/paths.rs @@ -7,7 +7,6 @@ use std::fs; use std::io::{self, ErrorKind}; use std::path::{Path, PathBuf}; use std::process::Command; -use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::Mutex; static CARGO_INTEGRATION_TEST_DIR: &str = "cit"; @@ -51,28 +50,20 @@ pub fn global_root() -> PathBuf { } } -// We need to give each test a unique id. The test name could serve this -// purpose, but the `test` crate doesn't have a way to obtain the current test -// name.[*] Instead, we used the `cargo-test-macro` crate to automatically -// insert an init function for each test that sets the test name in a thread -// local variable. -// -// [*] It does set the thread name, but only when running concurrently. If not -// running concurrently, all tests are run on the main thread. +// We need to give each test a unique id. The test name serve this +// purpose. We are able to get the test name by having the `cargo-test-macro` +// crate automatically insert an init function for each test that sets the +// test name in a thread local variable. thread_local! { - static TEST_ID: RefCell> = RefCell::new(None); + static TEST_NAME: RefCell> = RefCell::new(None); } pub struct TestIdGuard { _private: (), } -pub fn init_root(tmp_dir: Option<&'static str>) -> TestIdGuard { - static NEXT_ID: AtomicUsize = AtomicUsize::new(0); - - let id = NEXT_ID.fetch_add(1, Ordering::SeqCst); - TEST_ID.with(|n| *n.borrow_mut() = Some(id)); - +pub fn init_root(tmp_dir: Option<&'static str>, test_name: PathBuf) -> TestIdGuard { + TEST_NAME.with(|n| *n.borrow_mut() = Some(test_name)); let guard = TestIdGuard { _private: () }; set_global_root(tmp_dir); @@ -85,20 +76,20 @@ pub fn init_root(tmp_dir: Option<&'static str>) -> TestIdGuard { impl Drop for TestIdGuard { fn drop(&mut self) { - TEST_ID.with(|n| *n.borrow_mut() = None); + TEST_NAME.with(|n| *n.borrow_mut() = None); } } pub fn root() -> PathBuf { - let id = TEST_ID.with(|n| { - n.borrow().expect( + let test_name = TEST_NAME.with(|n| { + n.borrow().clone().expect( "Tests must use the `#[cargo_test]` attribute in \ order to be able to use the crate root.", ) }); let mut root = global_root(); - root.push(&format!("t{}", id)); + root.push(&test_name); root } @@ -345,3 +336,26 @@ pub fn windows_reserved_names_are_allowed() -> bool { true } } + +/// This takes the test location (std::file!() should be passed) and the test name +/// and outputs the location the test should be places in, inside of `target/tmp/cit` +/// +/// `path: tests/testsuite/workspaces.rs` +/// `name: `workspace_in_git +/// `output: "testsuite/workspaces/workspace_in_git` +pub fn test_dir(path: &str, name: &str) -> std::path::PathBuf { + let test_dir: std::path::PathBuf = std::path::PathBuf::from(path) + .components() + // Trim .rs from any files + .map(|c| c.as_os_str().to_str().unwrap().trim_end_matches(".rs")) + // We only want to take once we have reached `tests` or `src`. This helps when in a + // workspace: `workspace/more/src/...` would result in `src/...` + .skip_while(|c| c != &"tests" && c != &"src") + // We want to skip "tests" since it is taken in `skip_while`. + // "src" is fine since you could have test in "src" named the same as one in "tests" + // Skip "mod" since `snapbox` tests have a folder per test not a file and the files + // are named "mod.rs" + .filter(|c| c != &"tests" && c != &"mod") + .collect(); + test_dir.join(name) +} diff --git a/src/doc/contrib/src/tests/writing.md b/src/doc/contrib/src/tests/writing.md index a84dd5d6a41..dc4c51d8f5e 100644 --- a/src/doc/contrib/src/tests/writing.md +++ b/src/doc/contrib/src/tests/writing.md @@ -65,8 +65,9 @@ The [`#[cargo_test]` attribute](#cargo_test-attribute) is used in place of `#[te #### `#[cargo_test]` attribute The `#[cargo_test]` attribute injects code which does some setup before starting the test. -It will create a filesystem "sandbox" under the "cargo integration test" directory for each test, such as `/path/to/cargo/target/tmp/cit/t123/`. -The sandbox will contain a `home` directory that will be used instead of your normal home directory. +It will create a filesystem "sandbox" under the "cargo integration test" directory for each test, such as +`/path/to/cargo/target/tmp/cit/testsuite/bad_config/bad1`. The sandbox will contain a `home` directory that +will be used instead of your normal home directory. The `#[cargo_test]` attribute takes several options that will affect how the test is generated. They are listed in parentheses separated with commas, such as: @@ -200,7 +201,7 @@ Then populate - This attribute injects code which does some setup before starting the test, creating a filesystem "sandbox" under the "cargo integration test" directory for each test such as - `/path/to/cargo/target/cit/t123/` + `/path/to/cargo/target/tmp/cit/testsuite/cargo_add/add_basic` - The sandbox will contain a `home` directory that will be used instead of your normal home directory `Project`: @@ -267,9 +268,9 @@ environment. The general process is: `cargo test --test testsuite -- features2::inactivate_targets`. 2. In another terminal, head into the sandbox directory to inspect the files and run `cargo` directly. - 1. The sandbox directories start with `t0` for the first test. + 1. The sandbox directories match the format of `tests/testsuite`, just replace `tests` with `target/tmp/cit` - `cd target/tmp/cit/t0` + `cd target/tmp/cit/testsuite/features2/inactivate_target` 2. Set up the environment so that the sandbox configuration takes effect: `export CARGO_HOME=$(pwd)/home/.cargo` diff --git a/tests/testsuite/main.rs b/tests/testsuite/main.rs index 66914e4dfd8..fd16b86f9ba 100644 --- a/tests/testsuite/main.rs +++ b/tests/testsuite/main.rs @@ -143,3 +143,51 @@ fn aaa_trigger_cross_compile_disabled_check() { // This triggers the cross compile disabled check to run ASAP, see #5141 cargo_test_support::cross_compile::disabled(); } + +// This is placed here as running tests in `cargo-test-support` would rebuild it +#[cargo_test] +fn check_test_dir() { + let tests = vec![ + ( + "tests/testsuite/workspaces.rs", + "workspace_in_git", + "testsuite/workspaces/workspace_in_git", + ), + ( + "tests/testsuite/cargo_remove/invalid_arg/mod.rs", + "case", + "testsuite/cargo_remove/invalid_arg/case", + ), + ( + "tests/build-std/main.rs", + "cross_custom", + "build-std/main/cross_custom", + ), + ( + "src/tools/cargo/tests/testsuite/build.rs", + "cargo_compile_simple", + "src/tools/cargo/testsuite/build/cargo_compile_simple", + ), + ( + "src/tools/cargo/tests/testsuite/cargo_add/add_basic/mod.rs", + "case", + "src/tools/cargo/testsuite/cargo_add/add_basic/case", + ), + ( + "src/tools/cargo/tests/build-std/main.rs", + "cross_custom", + "src/tools/cargo/build-std/main/cross_custom", + ), + ( + "workspace/more/src/tools/cargo/tests/testsuite/build.rs", + "cargo_compile_simple", + "src/tools/cargo/testsuite/build/cargo_compile_simple", + ), + ]; + for (path, name, expected) in tests { + assert_eq!( + cargo_test_support::paths::test_dir(path, name), + std::path::PathBuf::from(expected) + ); + } +}