Auto merge of rust-lang#11738 - Muscraft:name-tests, r=epage

feat: Use test name for dir when running tests

Tests for Cargo currently put their generated data in `target/tmp/cit/t{}`. This can make it very hard to debug tests if multiple are falling. Having the tests be named would make this much easier when debugging.

[This comment](https://github.com/rust-lang/cargo/blob/eff8d693b99077b0b7f70e4bc414fdca53597085/crates/cargo-test-support/src/paths.rs#L54-L64) explains why cargo used a number instead of the test name for the unique identifier. I found a way to get the test name to be the unique identifier. I did this by having `cargo-test-macro` get the name of the test when the macro is looking for where the function body starts. It passes the name of the test and the file that the test comes from into the boilerplate it was already adding.

It uses the file and the test name as the directory to place the tests generated data data into, i.e, `target/tmp/cit/testsuite/workspaces/workspace_in_git`.

![Screenshot 2023-02-20 at 20 55 30](https://user-images.githubusercontent.com/23045215/220236036-e1dcbe53-033e-4db7-a6a3-a689eae97386.png)

Note: I also found `t{}` to be frustrating when trying to get the size of a test. There is no good way to get the size of a test within `target/tmp/cit`, without running the tests one at a time and checking the size like so:
```python
for test in tests:
    subprocess.run(["cargo", "test", "-p", "cargo", test, "--", "--exact"])
    test_size = subprocess.run(["du", "-sk", "target/tmp/cit/t0"], stdout=subprocess.PIPE, text=True)
    size_dict[test] = int(test_size.stdout.split()[0])
```
It made it very hard to try and find which tests were causing most of the size of the `target` dir.

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(

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<Option<usize>> = RefCell::new(None);
+    static TEST_NAME: RefCell<Option<PathBuf>> = 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)
+}

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`

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)
+        );
+    }
+}