diff --git a/src/core/cli_install.rs b/src/core/cli_install.rs index b8b05fe6..42c89e1f 100644 --- a/src/core/cli_install.rs +++ b/src/core/cli_install.rs @@ -730,12 +730,8 @@ mod unix_tests { std::fs::write(p, b"#!/bin/sh\n").unwrap(); } - fn tmpdir(tag: &str) -> PathBuf { - let dir = - std::env::temp_dir().join(format!("tty7-cli-install-{tag}-{}", std::process::id())); - let _ = std::fs::remove_dir_all(&dir); - std::fs::create_dir_all(&dir).unwrap(); - dir + fn tmpdir(tag: &str) -> crate::testutil::TempRoot { + crate::testutil::temp_root(&format!("cli-install-{tag}")) } #[test] @@ -754,7 +750,10 @@ mod unix_tests { #[test] fn an_unrelated_binary_named_tty7_is_left_alone() { let dir = tmpdir("occupied"); - let bin = tmpdir("occupied-src").join("tty7"); + // Bound rather than chained: the guard removes the directory when it + // drops, and a temporary would drop before bin was used. + let occupied_src_dir = tmpdir("occupied-src"); + let bin = occupied_src_dir.join("tty7"); touch(&bin); // Someone's own build, installed by hand. touch(&dir.join("tty7")); @@ -769,8 +768,14 @@ mod unix_tests { #[test] fn our_own_link_is_recognised_and_then_repointed_on_upgrade() { let dir = tmpdir("relink"); - let v1 = tmpdir("relink-v1").join("tty7"); - let v2 = tmpdir("relink-v2").join("tty7"); + // Bound rather than chained: the guard removes the directory when it + // drops, and a temporary would drop before v1 was used. + let relink_v1_dir = tmpdir("relink-v1"); + let v1 = relink_v1_dir.join("tty7"); + // Bound rather than chained: the guard removes the directory when it + // drops, and a temporary would drop before v2 was used. + let relink_v2_dir = tmpdir("relink-v2"); + let v2 = relink_v2_dir.join("tty7"); touch(&v1); touch(&v2); @@ -789,9 +794,15 @@ mod unix_tests { #[test] fn a_link_aimed_somewhere_deliberate_is_not_hijacked() { let dir = tmpdir("deliberate"); - let bin = tmpdir("deliberate-src").join("tty7"); + // Bound rather than chained: the guard removes the directory when it + // drops, and a temporary would drop before bin was used. + let deliberate_src_dir = tmpdir("deliberate-src"); + let bin = deliberate_src_dir.join("tty7"); touch(&bin); - let elsewhere = tmpdir("deliberate-other").join("my-terminal"); + // Bound rather than chained: the guard removes the directory when it + // drops, and a temporary would drop before elsewhere was used. + let deliberate_other_dir = tmpdir("deliberate-other"); + let elsewhere = deliberate_other_dir.join("my-terminal"); touch(&elsewhere); std::os::unix::fs::symlink(&elsewhere, dir.join("tty7")).unwrap(); @@ -804,8 +815,14 @@ mod unix_tests { #[test] fn a_copy_we_made_stays_ours_after_the_user_moves_off_the_appimage() { let dir = tmpdir("appimage-migrate"); - let v1 = tmpdir("appimage-mount-1").join("tty7"); - let v2 = tmpdir("appimage-mount-2").join("tty7"); + // Bound rather than chained: the guard removes the directory when it + // drops, and a temporary would drop before v1 was used. + let appimage_mount_1_dir = tmpdir("appimage-mount-1"); + let v1 = appimage_mount_1_dir.join("tty7"); + // Bound rather than chained: the guard removes the directory when it + // drops, and a temporary would drop before v2 was used. + let appimage_mount_2_dir = tmpdir("appimage-mount-2"); + let v2 = appimage_mount_2_dir.join("tty7"); touch(&v1); std::fs::write(&v2, b"#!/bin/sh\n# a later build\n").unwrap(); @@ -846,7 +863,10 @@ mod unix_tests { fn an_occupied_directory_does_not_end_the_search() { let taken = tmpdir("scan-taken"); let free = tmpdir("scan-free"); - let bin = tmpdir("scan-src").join("tty7"); + // Bound rather than chained: the guard removes the directory when it + // drops, and a temporary would drop before bin was used. + let scan_src_dir = tmpdir("scan-src"); + let bin = scan_src_dir.join("tty7"); touch(&bin); touch(&taken.join("tty7")); @@ -869,11 +889,11 @@ mod unix_tests { touch(&early.join("tty7")); touch(&ours.join("tty7")); - let path = vec![early.clone(), ours.clone()]; + let path = vec![early.to_path_buf(), ours.to_path_buf()]; assert_eq!(first_cli_on(&path), Some(early.join("tty7"))); // Our own directory first: no shadow. assert_eq!( - first_cli_on(&[ours.clone(), early.clone()]), + first_cli_on(&[ours.to_path_buf(), early.to_path_buf()]), Some(ours.join("tty7")) ); @@ -881,7 +901,7 @@ mod unix_tests { let dangling = tmpdir("shadow-dangling"); std::os::unix::fs::symlink(dangling.join("gone"), dangling.join("tty7")).unwrap(); assert_eq!( - first_cli_on(&[dangling, ours.clone()]), + first_cli_on(&[dangling.to_path_buf(), ours.to_path_buf()]), Some(ours.join("tty7")) ); } diff --git a/src/core/ssh_config.rs b/src/core/ssh_config.rs index c76644ca..86bbfbb7 100644 --- a/src/core/ssh_config.rs +++ b/src/core/ssh_config.rs @@ -1443,13 +1443,7 @@ mod tests { f.set_modified(ahead).unwrap(); } - fn temp_root(name: &str) -> PathBuf { - let dir = std::env::temp_dir().join(format!( - "tty7-ssh-config-test-{name}-{}", - std::process::id() - )); - let _ = std::fs::remove_dir_all(&dir); - std::fs::create_dir_all(&dir).unwrap(); - dir + fn temp_root(name: &str) -> crate::testutil::TempRoot { + crate::testutil::temp_root(&format!("ssh-config-test-{name}")) } } diff --git a/src/main.rs b/src/main.rs index fd018576..b58e921c 100644 --- a/src/main.rs +++ b/src/main.rs @@ -6,6 +6,8 @@ mod core; mod daemon; mod terminal; +#[cfg(test)] +mod testutil; mod ui; use crate::core::config::Config; diff --git a/src/terminal/view.rs b/src/terminal/view.rs index 4cce94b8..8b3af50f 100644 --- a/src/terminal/view.rs +++ b/src/terminal/view.rs @@ -9802,9 +9802,7 @@ mod gpui_tests { #[gpui::test] fn insert_newline_action_extends_the_line_and_enter_submits_it(cx: &mut TestAppContext) { - let dir = std::env::temp_dir().join(format!("tty7-covtest-{}", std::process::id())); - std::fs::create_dir_all(&dir).ok(); - crate::core::config::set_config_dir(dir); + crate::core::config::pin_test_config_dir(); let (window, mut daemon) = harness(cx); prompt_ready(&window, cx, &mut daemon); diff --git a/src/testutil.rs b/src/testutil.rs new file mode 100644 index 00000000..875638e3 --- /dev/null +++ b/src/testutil.rs @@ -0,0 +1,89 @@ +//! Scratch directories for tests, removed when the test ends. +//! +//! Every fixture here used to be `env::temp_dir().join(format!("tty7-x-{pid}"))`, +//! wiped on the way *in* and left behind on the way out. That reads as +//! self-cleaning and is not: the pid is in the name so two concurrent +//! `cargo test` runs cannot share a fixture, which also means a run never +//! finds the previous run's directory to wipe. One directory per fixture per +//! run, kept forever — this working copy had 25,639 of them. +//! +//! Dropping the guard is what removes the tree, so a test that panics still +//! cleans up: unwinding runs destructors, and the assertion failure is the +//! output that matters rather than the litter left beside it. + +use std::path::{Path, PathBuf}; + +/// A directory under the system temp dir that removes itself when dropped. +/// +/// Derefs to [`Path`], so it stands in for the `PathBuf` these fixtures used +/// to hand back: `root.join("x")` and `&root` both keep working. +/// +/// Bind it to a name — `let root = temp_root("x")` — rather than calling a +/// method on it directly. A temporary would be dropped at the end of the +/// statement that made it, taking the directory with it before the test +/// looked at it. +pub struct TempRoot(PathBuf); + +impl Drop for TempRoot { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } +} + +impl std::ops::Deref for TempRoot { + type Target = Path; + + fn deref(&self) -> &Path { + &self.0 + } +} + +impl AsRef for TempRoot { + fn as_ref(&self) -> &Path { + &self.0 + } +} + +/// A fresh, empty `tty7--` directory that lasts as long as the +/// returned guard. +/// +/// The pid stays in the name: it is what keeps two concurrent `cargo test` +/// runs off each other's fixture. It is the missing *removal*, not the +/// unique name, that made these accumulate. +pub fn temp_root(name: &str) -> TempRoot { + let dir = std::env::temp_dir().join(format!("tty7-{name}-{}", std::process::id())); + // Still wiped on the way in: a previous run that was killed hard enough + // to skip its destructors must not leave a fixture behind that this one + // would then read as its own. + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + TempRoot(dir) +} + +#[cfg(test)] +mod tests { + use super::temp_root; + + #[test] + fn a_scratch_directory_is_gone_once_its_guard_is() { + let path = { + let root = temp_root("testutil-selfcheck"); + let path = root.to_path_buf(); + assert!(path.is_dir(), "the fixture exists while the guard is held"); + std::fs::write(root.join("f"), b"x").unwrap(); + path + }; + assert!(!path.exists(), "and is removed with the guard: {path:?}"); + } + + #[test] + fn a_second_call_starts_from_an_empty_directory() { + let first = temp_root("testutil-reuse"); + std::fs::write(first.join("stale"), b"x").unwrap(); + let path = first.to_path_buf(); + std::mem::forget(first); // simulate a run that never dropped its guard + let again = temp_root("testutil-reuse"); + assert_eq!(*again, *path, "same name, same pid, same directory"); + assert!(!again.join("stale").exists(), "wiped on the way in"); + } +}