From f3778f919324b1d7e020dfe0abeeef9d03234c5b Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Wed, 8 Mar 2023 15:05:09 +0000 Subject: [PATCH] Revert "#11738" - Use test name for dir when running tests This reverts commit 64b0e793cea8542b34504a881f9cfd9444ab5138, reversing changes made to 958078633ee9ae1af053fbab32ac70ae475b0e7f. --- 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, 30 insertions(+), 113 deletions(-) diff --git a/crates/cargo-test-macro/src/lib.rs b/crates/cargo-test-macro/src/lib.rs index 112c020ed..aa06f477d 100644 --- a/crates/cargo-test-macro/src/lib.rs +++ b/crates/cargo-test-macro/src/lib.rs @@ -133,9 +133,6 @@ 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 { @@ -147,35 +144,18 @@ 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 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 mut new_body = to_token_stream( + r#"let _test_guard = { let tmp_dir = option_env!("CARGO_TARGET_TMPDIR"); - let test_dir = cargo_test_support::paths::test_dir(std::file!(), "{name}"); - cargo_test_support::paths::init_root(tmp_dir, test_dir) - }};"# - )); + cargo_test_support::paths::init_root(tmp_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 f1fc1bc29..ef1fddb70 100644 --- a/crates/cargo-test-support/src/paths.rs +++ b/crates/cargo-test-support/src/paths.rs @@ -7,6 +7,7 @@ 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"; @@ -50,20 +51,28 @@ pub fn global_root() -> PathBuf { } } -// 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. +// 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. thread_local! { - static TEST_NAME: RefCell> = RefCell::new(None); + static TEST_ID: RefCell> = RefCell::new(None); } pub struct TestIdGuard { _private: (), } -pub fn init_root(tmp_dir: Option<&'static str>, test_name: PathBuf) -> TestIdGuard { - TEST_NAME.with(|n| *n.borrow_mut() = Some(test_name)); +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)); + let guard = TestIdGuard { _private: () }; set_global_root(tmp_dir); @@ -76,20 +85,20 @@ pub fn init_root(tmp_dir: Option<&'static str>, test_name: PathBuf) -> TestIdGua impl Drop for TestIdGuard { fn drop(&mut self) { - TEST_NAME.with(|n| *n.borrow_mut() = None); + TEST_ID.with(|n| *n.borrow_mut() = None); } } pub fn root() -> PathBuf { - let test_name = TEST_NAME.with(|n| { - n.borrow().clone().expect( + let id = TEST_ID.with(|n| { + n.borrow().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(&test_name); + root.push(&format!("t{}", id)); root } @@ -336,26 +345,3 @@ 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 dc4c51d8f..a84dd5d6a 100644 --- a/src/doc/contrib/src/tests/writing.md +++ b/src/doc/contrib/src/tests/writing.md @@ -65,9 +65,8 @@ 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/testsuite/bad_config/bad1`. 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/t123/`. +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: @@ -201,7 +200,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/tmp/cit/testsuite/cargo_add/add_basic` + `/path/to/cargo/target/cit/t123/` - The sandbox will contain a `home` directory that will be used instead of your normal home directory `Project`: @@ -268,9 +267,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 match the format of `tests/testsuite`, just replace `tests` with `target/tmp/cit` + 1. The sandbox directories start with `t0` for the first test. - `cd target/tmp/cit/testsuite/features2/inactivate_target` + `cd target/tmp/cit/t0` 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 7f7c97ad8..a1e293acd 100644 --- a/tests/testsuite/main.rs +++ b/tests/testsuite/main.rs @@ -144,51 +144,3 @@ 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) - ); - } -}