feat(test-support): Use test name for dir when running tests

[Muscraft]

In #11738, I made a test's "sandbox" folder based on its name instead of a generated number, which can change from run to run. #11812 reverted this change due to problems with path length limits on Windows. After getting frustrated with the generated folders while trying to debug a test recently, I decided that it would be a good idea to bring back name-based folders to platforms that support them.

[rustbot]

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[weihanglo]

I'd like to call out @ehuss's comment #11738 (comment), though personally fine with this PR.

[Muscraft]

Not sure how I missed this when I was going back through that PR. Given that concern, it might make sense for me to bring this up in the next team meeting.

[ehuss]

There is some documentation in https://github.com/rust-lang/cargo/blob/HEAD/src/doc/contrib/src/tests/writing.md that would need to be updated.

[ehuss]

@rustbot author

I think @Muscraft indicated he could add some kind of symlink to the first test.
There's also the docs to be updated.

[ehuss]

@Muscraft Is this ready?

[Muscraft]

Yes it's ready, I just forgot to mark it as such

@rustbot ready

[ehuss]

Some suggestions about removing some of the duplicate code and reducing the number of lines of code to try to simplify.

View changes since this review

[ehuss]

With an eye towards reducing the amount of duplicated code, I'm wondering if we can try to combine things a little more. So, for example, this could be:

Suggested change

	let mut new_body = if cfg!(windows) {
	to_token_stream(
	r#"let _test_guard = {
	let tmp_dir = option_env!("CARGO_TARGET_TMPDIR");
	cargo_test_support::paths::init_root(tmp_dir)
	};"#,
	)
	} else {
	to_token_stream(&format!(
	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)
	}};"#
	))
	};
	let mut new_body = to_token_stream(&format!(
	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)
	}};"#
	));

[ehuss]

Part 2 of merging. Also, changing the name of TEST_NAME to TEST_DIR, since it doesn't include just the test name, but the full path. That was a little confusing to me.

Suggested change

	#[cfg(windows)]
	static TEST_ID: RefCell<Option<usize>> = const { RefCell::new(None) };
	#[cfg(not(windows))]
	static TEST_NAME: RefCell<Option<PathBuf>> = const { RefCell::new(None) };
	static TEST_ID: RefCell<Option<usize>> = const { RefCell::new(None) };
	static TEST_DIR: RefCell<Option<PathBuf>> = const { RefCell::new(None) };

[ehuss]

Part 3 of merge. Most of the platform-specific behavior would be here.

Part of the change here is that TEST_DIR now has the same behavior on both platforms where it represents the (relative) path of the test directory.

This also adds a comment that explains what the symlink is doing.

Suggested change

	#[cfg(windows)]
	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);
	let r = root();
	r.rm_rf();
	r.mkdir_p();
	guard
	}
	/// For test harnesses like [`crate::cargo_test`]
	#[cfg(not(windows))]
	pub fn init_root(tmp_dir: Option<&'static str>, test_name: PathBuf) -> TestIdGuard {
	static NEXT_ID: AtomicUsize = AtomicUsize::new(0);
	let id = NEXT_ID.fetch_add(1, Ordering::SeqCst);
	TEST_NAME.with(|n| *n.borrow_mut() = Some(test_name));
	let guard = TestIdGuard { _private: () };
	set_global_root(tmp_dir);
	let r = root();
	r.rm_rf();
	r.mkdir_p();
	if id == 0 {
	use crate::SymlinkBuilder;
	let mut root = global_root();
	root.push(&format!("t{}", id));
	root.rm_rf();
	SymlinkBuilder::new_dir(r, root).mk();
	}
	guard
	}
	pub fn init_root(tmp_dir: Option<&'static str>, test_dir: PathBuf) -> 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));
	if cfg!(windows) {
	// Due to path-length limits, Windows doesn't use the full test name.
	TEST_DIR.with(|n| *n.borrow_mut() = Some(PathBuf::from(format!("t{id}"))));
	} else {
	TEST_DIR.with(|n| *n.borrow_mut() = Some(test_dir));
	}
	let guard = TestIdGuard { _private: () };
	set_global_root(tmp_dir);
	let r = root();
	r.rm_rf();
	r.mkdir_p();
	#[cfg(not(windows))]
	if id == 0 {
	// Create a symlink from `t0` to the first test to make it easier to
	// find and reuse when running a single test.
	use crate::SymlinkBuilder;
	let mut alias = global_root();
	alias.push("t0");
	alias.rm_rf();
	SymlinkBuilder::new_dir(r, alias).mk();
	}
	guard
	}

[ehuss]

Part 4 of removing the platform-specific code.

Suggested change

	#[cfg(windows)]
	TEST_ID.with(|n| *n.borrow_mut() = None);
	#[cfg(not(windows))]
	TEST_NAME.with(|n| *n.borrow_mut() = None);
	TEST_ID.with(|n| *n.borrow_mut() = None);
	TEST_DIR.with(|n| *n.borrow_mut() = None);

[ehuss]

Part 5 of removing the platform-specific code.

This also updates the doc-comment to mention both forms.

Suggested change

	/// Path to the test's filesystem scratchpad
	///
	/// ex: `$CARGO_TARGET_TMPDIR/cit/<integration test>/<module>/<fn name>/`
	/// or `$CARGO_TARGET_TMPDIR/cit/t0` on Windows
	pub fn root() -> PathBuf {
	let test_dir = TEST_DIR.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(&test_dir);
	root
	}

[ehuss]

A very minor nit. The previous text was written with an eye towards the test directory names being sequentially numbered.

Suggested change

	1. The first test's sandbox directory is called `t0`.

[Muscraft]

It looks like this change runs into annotate-snippets's automatic trimming of long lines. I hadn't considered that long test paths could run into it.

Ideally, we would just increase the term width for this test, but cargo currently sets the term width via auto-detection (defaulting to 140, IIRC). I could add support for configuring the term width, but that seems like something that would require a larger discussion.

[weihanglo]

#16391 (comment)

It makes sense to me to have a larger width by default and override when needed for Cargo tests. Otherwise it would be a bit painful when updating the Cargo submodule in rust-lang/rust

[ehuss]

There's still a problem here, even with #16403. I tried running the tests in rust-lang/rust, and got an error with implicit_minimum_version_req. You can check this with something like the following (replace the target dir appropriately):

CARGO_TARGET_DIR=/Users/eric/Temp/build/aarch64-apple-darwin/stage2-tools cargo test --test testsuite --target host-tuple -- implicit_minimum_version_req

I recommend raising __CARGO_TEST_TTY_WIDTH_DO_NOT_USE_THIS to 400. Or 1000. Something that we are confident won't be a problem.

[Muscraft]

I set this to 400 like you recommended and added a comment mentioning that path lengths can exceed 200 chars when Cargo's tests get ran in rust-lang/rust

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[ehuss]

Thanks!

View changes since this review