From d269c8fdeb773ab4eeb40b7c45669fb0322299a1 Mon Sep 17 00:00:00 2001 From: "@permadeath.com" Date: Wed, 19 Aug 2026 15:34:51 -0400 Subject: [PATCH] test(temp): take throwaway directories from tempfile Replaces eighteen hand-rolled temp-directory helpers, most of which disambiguated by pid and so assumed one test process at a time. The five standing in for ~/.config/atgc are pinned to 0700: tempfile defaults a directory to 0o777 & ~umask, which would leave the mode assertions in those modules weaker than the thing they check. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.lock | 14 ++++++ Cargo.toml | 7 +++ src/clients/atproto/oauth/store.rs | 24 ++++++---- src/clients/git/run.rs | 4 +- src/cmd/doctor.rs | 23 +++++---- src/cmd/images.rs | 32 +++++++------ src/cmd/logs/oauth.rs | 7 ++- src/config/dir.rs | 60 +++++++++++++---------- src/config/lock.rs | 30 ++++++++---- src/logging/file.rs | 12 ++--- src/logging/oauth.rs | 30 ++++++------ src/testutil.rs | 77 ++++++++++++++++-------------- tests/exit_status.rs | 57 ++++++++++------------ tests/git_log.rs | 32 +++++++------ tests/support/mod.rs | 31 +++++------- 15 files changed, 238 insertions(+), 202 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 84bd5a4..551a4f9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -148,6 +148,7 @@ dependencies = [ "sha2", "sigpipe", "tangled-lexicon", + "tempfile", "terminal_size", "tokio", "userinput-lexicon", @@ -3440,6 +3441,19 @@ dependencies = [ "serde", ] +[[package]] +name = "tempfile" +version = "3.27.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "32497e9a4c7b38532efcdebeef879707aa9f794296a4f0244f6f69e9bc8574bd" +dependencies = [ + "fastrand", + "getrandom 0.4.3", + "once_cell", + "rustix", + "windows-sys 0.61.2", +] + [[package]] name = "tendril" version = "0.4.3" diff --git a/Cargo.toml b/Cargo.toml index 3e97afa..519c3e6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -128,6 +128,13 @@ userinput-lexicon = { path = "vendor/userinput-lexicon" } http-body-util = "0.1.4" hyper = { version = "1.11", features = ["http1", "server"] } hyper-util = { version = "0.1.20", features = ["tokio"] } +# Throwaway directories for the tests, replacing eighteen hand-rolled ones that +# disambiguated by pid — an assumption about how the suite runs, and one +# `cargo test` on threads does not honour. Dev-only on purpose: it never ships, +# and production `write_atomic` (src/config/dir.rs) keeps its own temp path +# because it sets the mode on the `open(2)` call, which `NamedTempFile::persist` +# would regress. +tempfile = "3.24" # Vendored for three fixes, all worth reporting upstream: # diff --git a/src/clients/atproto/oauth/store.rs b/src/clients/atproto/oauth/store.rs index e1cc47a..4bffb16 100644 --- a/src/clients/atproto/oauth/store.rs +++ b/src/clients/atproto/oauth/store.rs @@ -239,12 +239,20 @@ impl ClientAuthStore for SessionStore { mod tests { use super::*; - fn store_dir(label: &str) -> PathBuf { - let dir = - std::env::temp_dir().join(format!("atgc-sessionstore-{label}-{}", std::process::id())); - let _ = std::fs::remove_dir_all(&dir); - std::fs::create_dir_all(&dir).unwrap(); - dir + /// A throwaway store directory, named for the test so a leftover from a + /// killed run says which one made it. + fn store_dir(label: &str) -> tempfile::TempDir { + tempfile::Builder::new() + .prefix(&format!("atgc-sessionstore-{label}-")) + // 0o700, because these stand in for ~/.config/atgc, which + // production creates owner-only. `tempfile` defaults a + // directory to 0o777 & ~umask, which would quietly make + // every mode assertion below weaker than the real thing. + .permissions( + ::from_mode(0o700), + ) + .tempdir() + .unwrap() } /// The keys this writes have to stay the ones the rest of the tree @@ -269,7 +277,7 @@ mod tests { #[tokio::test] async fn a_pretty_printed_store_from_an_older_atgc_still_reads() { let dir = store_dir("pretty-compat"); - let path = dir.join("sessions.json"); + let path = dir.path().join("sessions.json"); let store = SessionStore::new(path.clone()); let session = crate::testutil::oauth_session("token"); @@ -326,7 +334,7 @@ mod tests { use std::os::unix::fs::PermissionsExt; let dir = store_dir("atomic-inode"); - let path = dir.join("sessions.json"); + let path = dir.path().join("sessions.json"); let store = SessionStore::new(path.clone()); let session = crate::testutil::oauth_session("first"); diff --git a/src/clients/git/run.rs b/src/clients/git/run.rs index 8c2c80c..76e8bdf 100644 --- a/src/clients/git/run.rs +++ b/src/clients/git/run.rs @@ -692,8 +692,8 @@ mod tests { let second = repo.git(&["rev-parse", "HEAD"]); repo.git(&["checkout", "-q", "main"]); - let log = std::env::temp_dir().join(format!("atgc-gitlog-{}.jsonl", std::process::id())); - let _ = std::fs::remove_file(&log); + let logs = tempfile::tempdir().expect("a temp directory"); + let log = logs.path().join("git.jsonl"); crate::logging::git::LOG.open_at(&log); // `status_in` is the runner `pr checkout` moves a branch with. diff --git a/src/cmd/doctor.rs b/src/cmd/doctor.rs index 1db74c0..6c595a3 100644 --- a/src/cmd/doctor.rs +++ b/src/cmd/doctor.rs @@ -1656,25 +1656,24 @@ mod tests { fn the_config_row_reports_a_store_other_users_can_read() { use std::os::unix::fs::PermissionsExt; - let dir = std::env::temp_dir().join(format!("atgc-doctor-perms-{}", std::process::id())); - let _ = std::fs::remove_dir_all(&dir); - std::fs::create_dir_all(&dir).unwrap(); + let dir = tempfile::tempdir().unwrap(); + let dir = dir.path(); let store = dir.join("sessions.json"); std::fs::write(&store, "{}").unwrap(); let chmod = |path: &Path, m: u32| { std::fs::set_permissions(path, Permissions::from_mode(m)).unwrap() }; - chmod(&dir, 0o700); + chmod(dir, 0o700); chmod(&store, 0o600); - let healthy = permissions_finding(&dir, None); + let healthy = permissions_finding(dir, None); assert_eq!(healthy.status, Status::Ok, "{}", healthy.detail); assert!(healthy.detail.contains("0700"), "{}", healthy.detail); assert!(healthy.remedy.is_none()); // Found wide and narrowed by this very command: still worth a row, // and it has to carry the mode it was found at. - let repaired = permissions_finding(&dir, Some(0o775)); + let repaired = permissions_finding(dir, Some(0o775)); assert_eq!(repaired.status, Status::Warn); assert!(repaired.detail.contains("0775"), "{}", repaired.detail); @@ -1682,7 +1681,7 @@ mod tests { // `ATGC_OAUTH_LOG` pointed at an existing file leaves, since // `create(true)` ignores the mode it is given. chmod(&store, 0o644); - let widened = permissions_finding(&dir, None); + let widened = permissions_finding(dir, None); assert_eq!(widened.status, Status::Warn); assert!( widened.detail.contains("sessions.json"), @@ -1693,7 +1692,7 @@ mod tests { // A repair does not make the files right, and the row has to carry // both facts: a widened file beside a directory this command found // wide is the ordinary state of an install made before either fix. - let both_ways = permissions_finding(&dir, Some(0o775)); + let both_ways = permissions_finding(dir, Some(0o775)); assert!(both_ways.detail.contains("0644"), "{}", both_ways.detail); assert!(both_ways.detail.contains("0775"), "{}", both_ways.detail); // One command, whatever the row found — and it has to be a command, @@ -1703,8 +1702,8 @@ mod tests { assert!(fix.contains("sessions.json"), "{fix}"); // The directory itself, which is the bug this all started from. - chmod(&dir, 0o775); - let both = permissions_finding(&dir, None); + chmod(dir, 0o775); + let both = permissions_finding(dir, None); assert_eq!(both.status, Status::Warn); assert!( both.detail.contains("the directory itself"), @@ -1717,8 +1716,8 @@ mod tests { // A directory that is not there at all is `n/a`: this command has to // report rather than fail, whatever state the machine is in. - std::fs::remove_dir_all(&dir).ok(); - assert_eq!(permissions_finding(&dir, None).status, Status::Na); + std::fs::remove_dir_all(dir).ok(); + assert_eq!(permissions_finding(dir, None).status, Status::Na); assert_eq!( config_check(&Err(anyhow::anyhow!("HOME is not set")), None).status, Status::Na diff --git a/src/cmd/images.rs b/src/cmd/images.rs index 8423375..70ad807 100644 --- a/src/cmd/images.rs +++ b/src/cmd/images.rs @@ -617,31 +617,33 @@ mod tests { /// but writing a whole minimal file keeps the fixture honest. const PNG: &[u8] = b"\x89PNG\r\n\x1a\n0000"; - struct TempDir(PathBuf); + /// A throwaway directory to plant image files in, named for the test so + /// a leftover from a killed run says which one made it. + struct TempDir(tempfile::TempDir); impl TempDir { fn new(name: &str) -> Self { - let dir = - std::env::temp_dir().join(format!("atgc-images-{name}-{}", std::process::id())); - std::fs::create_dir_all(&dir).unwrap(); - Self(dir) + Self( + tempfile::Builder::new() + .prefix(&format!("atgc-images-{name}-")) + .tempdir() + .unwrap(), + ) + } + + fn path(&self) -> PathBuf { + self.0.path().to_path_buf() } fn write(&self, name: &str, bytes: &[u8]) -> PathBuf { - let path = self.0.join(name); + let path = self.0.path().join(name); std::fs::write(&path, bytes).unwrap(); path } } - impl Drop for TempDir { - fn drop(&mut self) { - let _ = std::fs::remove_dir_all(&self.0); - } - } - fn scan(body: &str, root: &TempDir) -> anyhow::Result { - scan_with_roots(body, std::slice::from_ref(&root.0)) + scan_with_roots(body, &[root.path()]) } #[test] @@ -849,7 +851,7 @@ mod tests { #[test] fn an_absolute_path_that_is_a_directory_is_refused() { let dir = TempDir::new("absdir"); - let sub = dir.0.join("shots.png"); + let sub = dir.path().join("shots.png"); std::fs::create_dir_all(&sub).unwrap(); let err = scan(&format!("![x]({})", sub.display()), &dir) .unwrap_err() @@ -860,7 +862,7 @@ mod tests { #[test] fn a_relative_path_that_is_a_directory_is_refused_as_such() { let dir = TempDir::new("reldir"); - std::fs::create_dir_all(dir.0.join("shots.png")).unwrap(); + std::fs::create_dir_all(dir.path().join("shots.png")).unwrap(); let err = scan("![x](shots.png)", &dir).unwrap_err().to_string(); assert!( err.contains("not a regular file"), diff --git a/src/cmd/logs/oauth.rs b/src/cmd/logs/oauth.rs index ab292cf..7c05a37 100644 --- a/src/cmd/logs/oauth.rs +++ b/src/cmd/logs/oauth.rs @@ -1782,14 +1782,13 @@ mod tests { #[test] fn generations_are_read_oldest_first() { - let dir = std::env::temp_dir().join(format!("atgc-oauthlog-read-{}", std::process::id())); - std::fs::create_dir_all(&dir).unwrap(); - let live = oauthlog::LOG.path_in(&dir); + let dir = tempfile::tempdir().unwrap(); + let dir = dir.path(); + let live = oauthlog::LOG.path_in(dir); std::fs::write(&live, b"").unwrap(); assert_eq!(generations(&live), vec![live.clone()]); let old = dir.join("oauth.jsonl.1"); std::fs::write(&old, b"").unwrap(); assert_eq!(generations(&live), vec![old, live]); - std::fs::remove_dir_all(&dir).ok(); } } diff --git a/src/config/dir.rs b/src/config/dir.rs index 44b20f5..47d26fc 100644 --- a/src/config/dir.rs +++ b/src/config/dir.rs @@ -473,12 +473,21 @@ mod tests { } /// A throwaway directory this module may chmod at will, named for the - /// test so two of them cannot collide. - fn temp_dir(label: &str) -> PathBuf { - let dir = - std::env::temp_dir().join(format!("atgc-config-dir-{label}-{}", std::process::id())); - let _ = std::fs::remove_dir_all(&dir); - dir + /// test so a leftover from a killed run says which one made it. It is + /// deleted when the returned value drops, so hold it for the length of + /// the test. + fn temp_dir(label: &str) -> tempfile::TempDir { + tempfile::Builder::new() + .prefix(&format!("atgc-config-dir-{label}-")) + // 0o700, because these stand in for ~/.config/atgc, which + // production creates owner-only. `tempfile` defaults a + // directory to 0o777 & ~umask, which would quietly make + // every mode assertion below weaker than the real thing. + .permissions( + ::from_mode(0o700), + ) + .tempdir() + .unwrap() } /// A store found world-readable is narrowed; one already private is not @@ -493,7 +502,7 @@ mod tests { fn a_store_file_is_narrowed_only_when_it_is_wide() { use std::os::unix::fs::PermissionsExt; let dir = temp_dir("narrow-file"); - std::fs::create_dir_all(&dir).unwrap(); + let dir = dir.path(); let mode_of = |p: &Path| std::fs::metadata(p).unwrap().permissions().mode() & 0o7777; let plant = |name: &str, mode: u32| { let path = dir.join(name); @@ -524,7 +533,7 @@ mod tests { // login. narrow_file_if_wide(&dir.join("absent.json")); - std::fs::remove_dir_all(&dir).ok(); + std::fs::remove_dir_all(dir).ok(); } /// The rule, in the order it is applied: an absolute `XDG_CONFIG_HOME` @@ -610,7 +619,7 @@ mod tests { use std::os::unix::fs::PermissionsExt; let root = temp_dir("create"); - let dir = root.join("nested/atgc"); + let dir = root.path().join("nested/atgc"); create_private(&dir).expect("create it"); let mode = |p: &Path| std::fs::metadata(p).unwrap().permissions().mode() & 0o777; assert_eq!(mode(&dir), 0o700, "the state directory is not owner-only"); @@ -628,8 +637,6 @@ mod tests { std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o775)).unwrap(); create_private(&dir).expect("create it again"); assert_eq!(mode(&dir), 0o775, "create_private narrowed on its own"); - - std::fs::remove_dir_all(&root).ok(); } /// The repair, which is the half that reaches an install that already @@ -644,17 +651,22 @@ mod tests { fn an_existing_directory_is_narrowed_but_never_widened() { use std::os::unix::fs::PermissionsExt; - let dir = temp_dir("narrow"); - create_private(&dir).expect("create it"); - let mode = || std::fs::metadata(&dir).unwrap().permissions().mode() & 0o777; + // The directory must not exist yet: `create_private` applies its + // mode only to a directory it creates, and `tempfile` hands back one + // that already exists at 0o777 & ~umask. Creating a child of the + // throwaway root is what the pid-named helper used to give for free. + let root = temp_dir("narrow"); + let dir = &root.path().join("atgc"); + create_private(dir).expect("create it"); + let mode = || std::fs::metadata(dir).unwrap().permissions().mode() & 0o777; let set = - |m: u32| std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(m)).unwrap(); + |m: u32| std::fs::set_permissions(dir, std::fs::Permissions::from_mode(m)).unwrap(); - assert!(matches!(narrow_if_wide(&dir), Narrowed::Nothing)); + assert!(matches!(narrow_if_wide(dir), Narrowed::Nothing)); set(0o775); assert!( - matches!(narrow_if_wide(&dir), Narrowed::From(0o775)), + matches!(narrow_if_wide(dir), Narrowed::From(0o775)), "a group-writable directory was not narrowed" ); assert_eq!(mode(), 0o700); @@ -663,16 +675,16 @@ mod tests { // the machine, and 0640 is the group half of the same thing. for wide in [0o701, 0o750, 0o770] { set(wide); - assert!(matches!(narrow_if_wide(&dir), Narrowed::From(m) if m == wide)); + assert!(matches!(narrow_if_wide(dir), Narrowed::From(m) if m == wide)); assert_eq!(mode(), 0o700, "{wide:04o} was not narrowed"); } set(0o500); - assert!(matches!(narrow_if_wide(&dir), Narrowed::Nothing)); + assert!(matches!(narrow_if_wide(dir), Narrowed::Nothing)); assert_eq!(mode(), 0o500, "a narrower mode was widened"); + // Back to writable, or the directory cannot be cleaned up. set(0o700); - std::fs::remove_dir_all(&dir).ok(); } /// `write_atomic` exists because `sessions.json` used to be created at @@ -694,8 +706,8 @@ mod tests { fn write_atomic_lands_the_requested_mode_new_or_replacing() { use std::os::unix::fs::PermissionsExt; - let dir = std::env::temp_dir().join(format!("atgc-write-atomic-{}", std::process::id())); - std::fs::create_dir_all(&dir).unwrap(); + let dir = tempfile::tempdir().unwrap(); + let dir = dir.path(); let path = dir.join("state.json"); write_atomic(&path, b"{\"a\":1}", 0o600).expect("first write"); @@ -717,12 +729,10 @@ mod tests { // Only the destination remains — no leftover temp file from either // write. - let entries: Vec<_> = std::fs::read_dir(&dir) + let entries: Vec<_> = std::fs::read_dir(dir) .unwrap() .map(|e| e.unwrap().file_name()) .collect(); assert_eq!(entries, vec![std::ffi::OsString::from("state.json")]); - - std::fs::remove_dir_all(&dir).ok(); } } diff --git a/src/config/lock.rs b/src/config/lock.rs index e240429..49d34c4 100644 --- a/src/config/lock.rs +++ b/src/config/lock.rs @@ -401,10 +401,20 @@ fn ms_since(start: Instant) -> u64 { mod tests { use super::*; - fn temp_dir(label: &str) -> PathBuf { - let dir = std::env::temp_dir().join(format!("atgc-lock-{label}-{}", std::process::id())); - std::fs::create_dir_all(&dir).unwrap(); - dir + /// A throwaway directory, named for the test so a leftover from a killed + /// run says which one made it. + fn temp_dir(label: &str) -> tempfile::TempDir { + tempfile::Builder::new() + .prefix(&format!("atgc-lock-{label}-")) + // 0o700, because these stand in for ~/.config/atgc, which + // production creates owner-only. `tempfile` defaults a + // directory to 0o777 & ~umask, which would quietly make + // every mode assertion below weaker than the real thing. + .permissions( + ::from_mode(0o700), + ) + .tempdir() + .unwrap() } /// The deadlock this exists to avoid, driven directly. @@ -498,7 +508,8 @@ mod tests { /// careless caller would hit against itself. #[test] fn a_second_holder_is_excluded_until_the_first_lets_go() { - let path = temp_dir("exclusion").join(".lock"); + let dir = temp_dir("exclusion"); + let path = dir.path().join(".lock"); let first = open_at(&path).expect("open the lock file"); first.try_lock().expect("take it"); @@ -512,8 +523,6 @@ mod tests { first.unlock().unwrap(); second.try_lock().expect("the lock was not released"); second.unlock().unwrap(); - - std::fs::remove_dir_all(path.parent().unwrap()).ok(); } /// Dropping the guard has to release the lock, or the first command to @@ -522,7 +531,8 @@ mod tests { /// the release being automatic is the part callers rely on. #[test] fn dropping_the_guard_releases_the_lock() { - let path = temp_dir("release").join(".lock"); + let dir = temp_dir("release"); + let path = dir.path().join(".lock"); let file = open_at(&path).expect("open the lock file"); file.try_lock().expect("take it"); @@ -589,8 +599,8 @@ mod tests { const EACH: usize = 25; let dir = temp_dir("lost-update"); - let lock = dir.join(".lock"); - let counter = dir.join("counter"); + let lock = dir.path().join(".lock"); + let counter = dir.path().join("counter"); std::fs::write(&counter, "0").unwrap(); std::thread::scope(|scope| { diff --git a/src/logging/file.rs b/src/logging/file.rs index f8ce639..4db972c 100644 --- a/src/logging/file.rs +++ b/src/logging/file.rs @@ -672,9 +672,8 @@ mod tests { #[test] fn rotation_keeps_one_generation_and_leaves_a_small_log_alone() { - let dir = std::env::temp_dir().join(format!("atgc-rotate-{}", Invocation::new().id)); - std::fs::create_dir_all(&dir).unwrap(); - let path = dir.join("oauth.jsonl"); + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("oauth.jsonl"); std::fs::write(&path, b"first\n").unwrap(); assert_eq!(rotate_if_large(&path, 1024), None, "small log left alone"); @@ -706,7 +705,6 @@ mod tests { // A log that does not exist yet is not an error. std::fs::remove_file(path.with_extension("jsonl.1")).unwrap(); assert_eq!(rotate_if_large(&path, 1), None); - std::fs::remove_dir_all(&dir).ok(); } /// A log inherited at a wide mode is narrowed, and one at a narrow mode @@ -721,8 +719,8 @@ mod tests { #[test] fn a_log_inherited_world_readable_is_narrowed_on_open() { use std::os::unix::fs::PermissionsExt; - let dir = std::env::temp_dir().join(format!("atgc-mode-{}", Invocation::new().id)); - std::fs::create_dir_all(&dir).unwrap(); + let dir = tempfile::tempdir().unwrap(); + let dir = dir.path(); let mode_of = |p: &std::path::Path| std::fs::metadata(p).unwrap().permissions().mode() & 0o7777; @@ -758,7 +756,7 @@ mod tests { assert!(open_sink_at(&unopenable).is_none()); assert_eq!(mode_of(&unopenable), 0o444); - std::fs::remove_dir_all(&dir).ok(); + std::fs::remove_dir_all(dir).ok(); } /// The envelope itself has to leave room. A test-only event of entirely diff --git a/src/logging/oauth.rs b/src/logging/oauth.rs index d207538..0a6c963 100644 --- a/src/logging/oauth.rs +++ b/src/logging/oauth.rs @@ -1703,13 +1703,8 @@ mod tests { const WRITERS: usize = 16; const PER_WRITER: usize = 200; - let dir = std::env::temp_dir().join(format!( - "atgc-oauthlog-{}-{}", - std::process::id(), - crate::logging::file::invocation().id - )); - std::fs::create_dir_all(&dir).unwrap(); - let path = LOG.path_in(&dir); + let dir = tempfile::tempdir().unwrap(); + let path = LOG.path_in(dir.path()); std::thread::scope(|scope| { for writer in 0..WRITERS { @@ -1753,16 +1748,21 @@ mod tests { serde_json::from_str::(line) .unwrap_or_else(|e| panic!("line {n} did not parse ({e}): {line}")); } - std::fs::remove_dir_all(&dir).ok(); } /// A throwaway store directory, named for the test so two cannot collide. - fn store_dir(label: &str) -> std::path::PathBuf { - let dir = - std::env::temp_dir().join(format!("atgc-authstore-{label}-{}", std::process::id())); - let _ = std::fs::remove_dir_all(&dir); - std::fs::create_dir_all(&dir).unwrap(); - dir + fn store_dir(label: &str) -> tempfile::TempDir { + tempfile::Builder::new() + .prefix(&format!("atgc-authstore-{label}-")) + // 0o700, because these stand in for ~/.config/atgc, which + // production creates owner-only. `tempfile` defaults a + // directory to 0o777 & ~umask, which would quietly make + // every mode assertion below weaker than the real thing. + .permissions( + ::from_mode(0o700), + ) + .tempdir() + .unwrap() } use crate::testutil::oauth_session as a_session; @@ -1806,7 +1806,7 @@ mod tests { #[tokio::test] async fn an_unchanged_upsert_does_not_rewrite_the_store() { let dir = store_dir("unchanged-upsert"); - let path = dir.join("sessions.json"); + let path = dir.path().join("sessions.json"); let store = LoggedAuthStore(crate::clients::atproto::oauth::store::SessionStore::new( path.clone(), )); diff --git a/src/testutil.rs b/src/testutil.rs index 6211cba..d8ca010 100644 --- a/src/testutil.rs +++ b/src/testutil.rs @@ -9,9 +9,10 @@ use std::path::{Path, PathBuf}; use std::process::Command; -use std::sync::atomic::{AtomicU32, Ordering}; use std::sync::{Mutex, MutexGuard}; +use tempfile::TempDir; + /// The working directory is process-wide and cargo runs tests on threads that /// share it, so anything that moves the process has to take this first. /// [`TempRepo`] is the only thing in the tree that does. @@ -27,7 +28,7 @@ fn cwd_lock() -> MutexGuard<'static, ()> { /// through it writes outside `path`, so a test that sets git config can only /// reach this repo's `.git/config` and never the user's. pub struct TempRepo { - path: PathBuf, + dir: TempDir, previous: PathBuf, _guard: MutexGuard<'static, ()>, } @@ -37,50 +38,53 @@ impl TempRepo { let guard = cwd_lock(); let previous = std::env::current_dir().expect("a working directory"); - // Unique per process and per call, so a stale directory left by a - // killed run cannot collide with a live one. - static COUNTER: AtomicU32 = AtomicU32::new(0); - let n = COUNTER.fetch_add(1, Ordering::Relaxed); - let path = - std::env::temp_dir().join(format!("atgc-test-{label}-{}-{n}", std::process::id())); - let _ = std::fs::remove_dir_all(&path); - std::fs::create_dir_all(&path).expect("temp dir"); + // `tempfile` picks the name, so two repos cannot collide however the + // suite is run — the old scheme was unique per pid and per call, + // which assumed one process at a time. + let dir = tempfile::Builder::new() + .prefix(&format!("atgc-test-{label}-")) + .tempdir() + .expect("temp dir"); // -b main so the branch does not depend on whatever // init.defaultBranch the machine running the tests has set. - run_git(&path, &["init", "-q", "-b", "main"]); - std::env::set_current_dir(&path).expect("enter the temp repo"); + run_git(dir.path(), &["init", "-q", "-b", "main"]); + std::env::set_current_dir(dir.path()).expect("enter the temp repo"); TempRepo { - path, + dir, previous, _guard: guard, } } + fn path(&self) -> &Path { + self.dir.path() + } + pub fn commit(&self, name: &str, body: &str, subject: &str) { - std::fs::write(self.path.join(name), body).expect("write a file"); - run_git(&self.path, &["add", name]); - run_git(&self.path, &["commit", "-q", "-m", subject]); + std::fs::write(self.path().join(name), body).expect("write a file"); + run_git(self.path(), &["add", name]); + run_git(self.path(), &["commit", "-q", "-m", subject]); } pub fn git(&self, args: &[&str]) -> String { - run_git(&self.path, args) + run_git(self.path(), args) } /// This repo's own config file, to prove a write landed here and not in /// the user's `~/.gitconfig`. pub fn local_config(&self) -> String { - std::fs::read_to_string(self.path.join(".git/config")).expect("a local config") + std::fs::read_to_string(self.path().join(".git/config")).expect("a local config") } } impl Drop for TempRepo { fn drop(&mut self) { - // Leave the directory before deleting it, and restore whatever the - // harness was in — a later test may be relative to it. + // Leave the directory before it is deleted, and restore whatever the + // harness was in — a later test may be relative to it. `dir` drops + // straight after this and takes the tree with it. let _ = std::env::set_current_dir(&self.previous); - let _ = std::fs::remove_dir_all(&self.path); } } @@ -98,17 +102,22 @@ impl Drop for TempRepo { /// passed in explicitly. `ssh-keygen` is a hard requirement of the suite, the /// way `git` already is — a machine that can push to Tangled has it. pub struct TempKeys { - dir: PathBuf, + dir: TempDir, } impl TempKeys { pub fn new(label: &str) -> Self { - static COUNTER: AtomicU32 = AtomicU32::new(0); - let n = COUNTER.fetch_add(1, Ordering::Relaxed); - let dir = - std::env::temp_dir().join(format!("atgc-test-keys-{label}-{}-{n}", std::process::id())); - let _ = std::fs::remove_dir_all(&dir); - std::fs::create_dir_all(&dir).expect("temp dir"); + let dir = tempfile::Builder::new() + .prefix(&format!("atgc-test-keys-{label}-")) + // 0o700, because these stand in for ~/.config/atgc, which + // production creates owner-only. `tempfile` defaults a + // directory to 0o777 & ~umask, which would quietly make + // every mode assertion below weaker than the real thing. + .permissions( + ::from_mode(0o700), + ) + .tempdir() + .expect("temp dir"); let keys = TempKeys { dir }; // -N "" is an empty passphrase and -q keeps the randomart off the @@ -130,15 +139,15 @@ impl TempKeys { /// The directory holding the pair, which stands in for `~/.ssh`. pub fn dir(&self) -> &Path { - &self.dir + self.dir.path() } pub fn private(&self) -> PathBuf { - self.dir.join("id_ed25519") + self.dir.path().join("id_ed25519") } pub fn public(&self) -> PathBuf { - self.dir.join("id_ed25519.pub") + self.dir.path().join("id_ed25519.pub") } /// The `.pub` file's contents: `ssh-ed25519 atgc-test-