From cde2068ee6699ff080422aec6d8e18f9bb69921b Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Sun, 16 Aug 2026 01:31:32 +0800 Subject: [PATCH] test: remove the scratch directories the fixtures leave behind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `env::temp_dir().join(format!("tty7-x-{pid}"))`, wiped on the way in and left behind on the way out, 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, and `cargo test` added about 37 more each time. Two modules account for 10,904 of those, and both build every fixture through one helper, so both are one edit: `ssh_config` (18 fixtures) and `cli_install` (19). They now go through `testutil::temp_root`, which hands back a guard that removes the tree when it drops — the pattern `core::git::log` already uses, rather than a fourth copy of it. Measured rather than assumed: a full bin-test run leaves both prefixes at exactly the count they started at (5217 and 5687), where they used to grow every run. The remaining 29 per run are the fixtures in modules not converted here. The guard is deliberately not `Clone` — copying something that deletes a directory on drop is not a copy — so three sites that wanted a plain path say `to_path_buf()`. Eight more chained `tmpdir(..).join(..)`, where the guard is a temporary that drops at the end of the statement and takes the directory with it before the test looks at it; the test suite caught that as a failure and they now bind the guard first. `pin_test_config_dir` keeps its uncleaned directory on purpose: it sets a process-global config dir, so no scope owns it. `view.rs` was spelling that function out by hand and now calls it. --- src/core/cli_install.rs | 54 +++++++++++++++++-------- src/core/ssh_config.rs | 10 +---- src/main.rs | 2 + src/terminal/view.rs | 4 +- src/testutil.rs | 89 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 131 insertions(+), 28 deletions(-) create mode 100644 src/testutil.rs 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"); + } +}