Skip to content

Commit

Permalink
Auto merge of #11738 - Muscraft:name-tests, r=epage
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
bors committed Feb 28, 2023
2 parents e7b11e7 + 835117d commit c06fb5f
Show file tree
Hide file tree
Showing 4 changed files with 113 additions and 30 deletions.
30 changes: 25 additions & 5 deletions crates/cargo-test-macro/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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(
Expand Down
54 changes: 34 additions & 20 deletions crates/cargo-test-support/src/paths.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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);
Expand All @@ -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
}

Expand Down Expand Up @@ -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)
}
11 changes: 6 additions & 5 deletions src/doc/contrib/src/tests/writing.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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`:
Expand Down Expand Up @@ -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`
Expand Down
48 changes: 48 additions & 0 deletions tests/testsuite/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
);
}
}

0 comments on commit c06fb5f

Please sign in to comment.