From 3cb234d9b3e8ed3d11462b00bb34ce9f9c2d0f8a Mon Sep 17 00:00:00 2001 From: Jake Massoth Date: Thu, 9 Jul 2026 16:24:28 +0200 Subject: [PATCH] test(e2e): unique tmux socket per test + RAII server disposal Each TestEnv now mints its own uniquely-named -L socket (PID + nanos + process-wide atomic counter) instead of a shared per-run OnceLock, and a Drop guard runs kill-server on that socket so the throwaway tmux server is torn down automatically on pass, fail, or panic. The socket is relocated under the env's temp dir via TMUX_TMPDIR so its socket file is cleaned up with the temp dir too -- nothing leaks into /tmp/tmux-*/. With fully independent per-test servers the suite is parallel-safe, so --test-threads=1 is dropped from docs, README, and the .#e2e flake app. The free helpers (tmux/capture/window_exists) become TestEnv methods on the per-env socket; test_socket() and the now-redundant WindowCleanup guard are removed. --- DEVELOPMENT.md | 31 +++--- README.md | 2 +- flake.nix | 4 +- src/backend/real.rs | 4 +- tests/e2e.rs | 233 ++++++++++++++++++++++++++------------------ 5 files changed, 161 insertions(+), 113 deletions(-) diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md index ad55927..77cc298 100644 --- a/DEVELOPMENT.md +++ b/DEVELOPMENT.md @@ -45,9 +45,9 @@ nix build .#check nix build .#test # or: cargo test --bin yeschef # E2E tests — require git + tmux on PATH (no containers/macOS). Drive real tmux -# sessions on a throwaway per-run `-L` socket (never the live `yeschef` server), -# so run single-threaded. -cargo test --test e2e -- --ignored --test-threads=1 +# sessions on a throwaway per-TEST `-L` socket (never the live `yeschef` +# server), so they run safely under cargo's default parallel execution. +cargo test --test e2e -- --ignored # or via the flake (PATH-checks git + tmux first): nix run .#e2e nix run .#e2e -- @@ -56,12 +56,15 @@ nix run .#e2e -- cargo test ``` -The e2e suite is light now (no image builds). It uses unique per-test project names and -runs on a **throwaway per-run tmux socket** — it picks a unique `-L` name (PID + nanos), -exports it as `YESCHEF_TMUX_SOCKET` so the spawned `yeschef` binary and the tests' own -`tmux` helpers drive that same private server, and never touches the operator's live -`yeschef` server. `--test-threads=1` avoids cross-test races, and each test kills its own -tmux session on drop. +The e2e suite is light now (no image builds). It uses unique per-test project names and a +**throwaway per-test tmux socket** — each `TestEnv` mints its own unique `-L` name +(PID + nanos + a process-wide atomic counter), exports it as `YESCHEF_TMUX_SOCKET` so the +spawned `yeschef` binary and the tests' own `tmux` helpers drive that one private server, +and never touches the operator's live `yeschef` server. Because every test has a fully +independent server, the suite runs safely under cargo's **default parallel execution** (no +`--test-threads=1`). Each `TestEnv`'s `Drop` runs `kill-server` on its socket to dispose of +the server automatically on pass, fail, or panic; the socket file lives under the test's +temp dir (via `TMUX_TMPDIR`) so it is cleaned up too — nothing leaks into `/tmp/tmux-*/`. ## CI — run `nix flake check` before you push @@ -76,7 +79,7 @@ nix run .#e2e # the e2e suite (run separately — see below) `checks` in `flake.nix` covers **fmt** (`cargo fmt --check`), **nixfmt** (nixfmt-rfc-style on `flake.nix`), **lint** (strict clippy), and **test** (unit tests). The **e2e** suite is deliberately *not* a flake check: it drives real -tmux sessions and real git worktrees (impure, though on a throwaway per-run `-L` +tmux sessions and real git worktrees (impure, though on a throwaway per-test `-L` socket rather than the live `yeschef` server), so it runs un-sandboxed via `nix run .#e2e`. Run both before pushing. @@ -94,8 +97,8 @@ actually exercise it: - Touching `store`/`names`/orchestration logic reachable from mocks → `cargo test --bin yeschef`. - Touching the real tmux/git backends or command wiring from `main.rs` → run the relevant - e2e test (`cargo test --test e2e -- --ignored --test-threads=1 `). The e2e tests - are the only thing that exercises real `tmux`/`git` behavior. + e2e test (`cargo test --test e2e -- --ignored `). The e2e tests are the only thing + that exercises real `tmux`/`git` behavior. - Touching a single e2e test → run that specific test, not the whole suite. ## Recording the terminal (demos / repros) @@ -160,7 +163,7 @@ derived name stays a clean tmux session id. the user's default tmux server or `~/.tmux.conf`. The socket name is resolved once per backend by `backend::real::resolve_tmux_socket` — `YESCHEF_TMUX_SOCKET` if set, else the `DEFAULT_TMUX_SOCKET` (`"yeschef"`). Production leaves it unset; the e2e tests point it at - a throwaway per-run socket so their `kill-session`/`kill-server` calls can never reach the + a throwaway per-test socket so their `kill-session`/`kill-server` calls can never reach the operator's live `yeschef` server. The config is baked into the binary (`include_str!("../tmux.conf")`) and rewritten to `/tmux.conf` on every `Config::load` (see `config::ensure_tmux_conf`). @@ -190,7 +193,7 @@ derived name stays a clean tmux session id. Defaults to `~/yeschef`; overridden with `YESCHEF_HOME` (used by e2e tests for isolation). A second env var, `YESCHEF_TMUX_SOCKET`, overrides the tmux `-L` socket name (default -`yeschef`) — the e2e tests set it to a throwaway per-run name so they never touch the +`yeschef`) — the e2e tests set it to a throwaway per-test name so they never touch the operator's live tmux server. Layout: ``` diff --git a/README.md b/README.md index cbcc77f..6011700 100644 --- a/README.md +++ b/README.md @@ -63,7 +63,7 @@ nix build .#clippy # clippy -D warnings -D clippy::pedantic nix build .#test # or: cargo test (unit tests, no external deps) # e2e (real git + tmux; no containers/macOS needed) -cargo test --test e2e -- --ignored --test-threads=1 +cargo test --test e2e -- --ignored nix run .#e2e # PATH-checks git + tmux first ``` diff --git a/flake.nix b/flake.nix index baec3e4..6823571 100644 --- a/flake.nix +++ b/flake.nix @@ -118,7 +118,7 @@ pkgs.git ]; text = '' - exec cargo test --test e2e -- --ignored --test-threads=1 "$@" + exec cargo test --test e2e -- --ignored "$@" ''; } }/bin/yeschef-e2e"; @@ -133,7 +133,7 @@ # `nix flake check` and run as a separate `nix run .#e2e` CI step. Two # reasons: # 1. e2e drives REAL tmux sessions and REAL git worktrees on a - # throwaway per-run `-L` socket (via `YESCHEF_TMUX_SOCKET`, never the + # throwaway per-test `-L` socket (via `YESCHEF_TMUX_SOCKET`, never the # live `yeschef` server) and spawns detached sessions. That is an # impure integration test, not a hermetic build. # 2. naersk's test mode can't cleanly run the `#[ignore]`d e2e tests: diff --git a/src/backend/real.rs b/src/backend/real.rs index fe0730e..73ade91 100644 --- a/src/backend/real.rs +++ b/src/backend/real.rs @@ -259,7 +259,7 @@ impl GitBackend for RealGitBackend { // // The socket name is configurable via `YESCHEF_TMUX_SOCKET` (default `yeschef`), // resolved once per backend in `new`. Production leaves it unset and runs on -// `yeschef`; the e2e tests point it at a throwaway per-run socket so they can +// `yeschef`; the e2e tests point it at a throwaway per-test socket so they can // create and kill sessions — even a whole `kill-server` — without ever touching // the operator's live `yeschef` server. @@ -274,7 +274,7 @@ pub const TMUX_SOCKET_ENV: &str = "YESCHEF_TMUX_SOCKET"; /// Resolve the tmux `-L` socket name from [`TMUX_SOCKET_ENV`], falling back to /// [`DEFAULT_TMUX_SOCKET`] when it is unset or empty. This is the single source /// of truth for which tmux server every yeschef invocation drives — making it -/// configurable is what lets the e2e suite run on a throwaway per-run socket +/// configurable is what lets the e2e suite run on a throwaway per-test socket /// instead of the operator's live `yeschef` server. pub fn resolve_tmux_socket() -> String { match std::env::var(TMUX_SOCKET_ENV) { diff --git a/tests/e2e.rs b/tests/e2e.rs index 42a7d88..ee3825e 100644 --- a/tests/e2e.rs +++ b/tests/e2e.rs @@ -1,26 +1,31 @@ //! End-to-end tests for yeschef. //! //! All tests are `#[ignore]` by default. Run with: -//! cargo test --test e2e -- --ignored --test-threads=1 +//! cargo test --test e2e -- --ignored //! //! Requirements: `git` and `tmux` in PATH. No containers, no Nix, no macOS //! requirement — the tests drive real git worktrees and real tmux sessions. //! //! ## Socket isolation //! -//! The suite NEVER touches the operator's live `yeschef` tmux server. It picks a -//! unique, throwaway `-L` socket for the run (see [`test_socket`]), exports it as -//! `YESCHEF_TMUX_SOCKET` so every spawned `yeschef` binary drives that same -//! private server, and points every direct `tmux` helper here at it too. So a -//! `kill-session` (or even a `kill-server`) in a test can only ever affect the -//! test's own server — running the full suite from inside a live yeschef session -//! leaves the operator's sessions untouched. `--test-threads=1` keeps the shared -//! per-run sessions sane across tests (each test also uses a unique project name, -//! so windows don't clash). +//! The suite NEVER touches the operator's live `yeschef` tmux server. Each +//! [`TestEnv`] mints its OWN unique, throwaway `-L` socket (see +//! [`unique_socket`]), exports it as `YESCHEF_TMUX_SOCKET` so every `yeschef` +//! binary that env spawns drives that same private server, and points every +//! direct `tmux` helper on the env at it too. So a `kill-session` (or the +//! whole-server `kill-server` teardown) can only ever affect that one env's +//! server — never another test's, and never the operator's live session (even +//! when the suite is run from inside one). When a `TestEnv` drops, its `Drop` +//! impl runs `kill-server` to dispose of the private server process on pass, +//! fail, or panic; the socket FILE itself lives under the env's temp dir (via +//! `TMUX_TMPDIR`), so it is removed with that dir too — nothing (server or +//! socket) leaks into the shared `/tmp/tmux-/` tree. Because every test has +//! a fully independent server, the suite is safe under cargo's default parallel +//! execution — no `--test-threads=1` needed. use std::path::Path; use std::process::Command; -use std::sync::OnceLock; +use std::sync::atomic::{AtomicU64, Ordering}; use predicates::prelude::*; use tempfile::TempDir; @@ -29,17 +34,35 @@ use tempfile::TempDir; // Helpers // --------------------------------------------------------------------------- -/// A sandboxed `YESCHEF_HOME` backed by a temp directory. +/// A sandboxed `YESCHEF_HOME` backed by a temp directory, wired to its OWN +/// private throwaway tmux server. The socket is unique per env (see +/// [`unique_socket`]), so two tests — even running in parallel — never share a +/// tmux server, and `Drop` tears the server down automatically. struct TestEnv { - _tmp: TempDir, + tmp: TempDir, home: std::path::PathBuf, + /// This env's private tmux `-L` socket name. Handed to every spawned + /// `yeschef` binary via `YESCHEF_TMUX_SOCKET` and to every direct `tmux` + /// helper on the env, and torn down in `Drop`. + socket: String, } impl TestEnv { fn new() -> Self { - let tmp = TempDir::new().expect("create temp dir"); + // Root the temp dir at `/tmp` rather than the platform default. We point + // `TMUX_TMPDIR` here (see `cmd`) so the tmux socket lives under it and is + // cleaned up on drop — but a unix socket path has a hard ~104-byte limit + // (`sun_path`), and macOS's default temp dir (`/var/folders/…`) is deep + // enough that the socket path overflows it ("File name too long"). `/tmp` + // is short, present on both macOS and Linux, and is where tmux puts its + // sockets by default anyway, so the relocated socket path stays valid. + let tmp = TempDir::new_in("/tmp").expect("create temp dir under /tmp"); let home = tmp.path().join("yeschef-home"); - TestEnv { _tmp: tmp, home } + TestEnv { + tmp, + home, + socket: unique_socket(), + } } fn home_path(&self) -> &Path { @@ -49,15 +72,67 @@ impl TestEnv { fn cmd(&self) -> assert_cmd::Command { let mut cmd = assert_cmd::Command::cargo_bin("yeschef").unwrap(); cmd.env("YESCHEF_HOME", &self.home); - // Pin the spawned binary to the run's throwaway tmux socket so it never - // drives the operator's live `yeschef` server (see `test_socket`). - cmd.env("YESCHEF_TMUX_SOCKET", test_socket()); + // Pin the spawned binary to THIS env's private tmux server: a unique + // `-L` socket name, relocated under this env's temp dir via + // `TMUX_TMPDIR`. The socket never drives the operator's live `yeschef` + // server or another test's server, and the relocation means its socket + // FILE is cleaned up with the temp dir on drop (see `Drop`) rather than + // littering the shared `/tmp/tmux-/` tree. + cmd.env("YESCHEF_TMUX_SOCKET", &self.socket); + cmd.env("TMUX_TMPDIR", self.tmp.path()); cmd } fn init(&self) { self.cmd().arg("init").assert().success(); } + + /// A `tmux` command wired to this env's private server (same `-L` socket + + /// `TMUX_TMPDIR` as [`TestEnv::cmd`], so they resolve to the same server). + /// Read helpers don't need `-f` — the server is already running with its + /// config from the binary's spawn. + fn tmux(&self, args: &[&str]) -> std::process::Output { + Command::new("tmux") + .env("TMUX_TMPDIR", self.tmp.path()) + .args(["-L", &self.socket]) + .args(args) + .output() + .expect("failed to run tmux") + } + + /// Capture a window's scrollback via this env's tmux server. + fn capture(&self, window: &str) -> String { + let out = self.tmux(&["capture-pane", "-p", "-S", "-", "-t", &sid(window)]); + String::from_utf8_lossy(&out.stdout).into_owned() + } + + fn window_exists(&self, window: &str) -> bool { + let id = sid(window); + let out = self.tmux(&["list-sessions", "-F", "#{session_name}"]); + out.status.success() + && String::from_utf8_lossy(&out.stdout) + .lines() + .any(|l| l.trim() == id) + } +} + +impl Drop for TestEnv { + fn drop(&mut self) { + // RAII teardown, two layers: + // 1. `kill-server` disposes this env's tmux server PROCESS (and every + // session it held) promptly — on pass, fail, or panic — so no live + // throwaway server ever leaks. `TMUX_TMPDIR` MUST match `cmd`/`tmux` + // so `-L` resolves to the same socket the server actually runs on. + // 2. The socket FILE (which tmux leaves behind after `kill-server`) + // lives under `self.tmp`, so it is removed with the temp dir when + // the `tmp` field drops immediately after this — keeping the shared + // `/tmp/tmux-/` tree clean with no socket-path math. + // Best-effort — the server may already be gone if none was started. + let _ = Command::new("tmux") + .env("TMUX_TMPDIR", self.tmp.path()) + .args(["-L", &self.socket, "kill-server"]) + .output(); + } } /// A temporary local git repository to clone from. @@ -110,34 +185,23 @@ fn unique_name() -> String { format!("t{pid:08x}{:04x}", nanos & 0xffff) } -/// A unique, throwaway tmux `-L` socket for THIS test-binary run. Computed once -/// (PID + startup nanos) and shared by every helper here and — via the -/// `YESCHEF_TMUX_SOCKET` env var set in [`TestEnv::cmd`] — by every `yeschef` -/// child process the tests spawn. This is the isolation guarantee: the suite -/// stands up and tears down its own private server and can never touch the -/// operator's live `yeschef` socket, even when run from inside a live session. -fn test_socket() -> &'static str { - static SOCKET: OnceLock = OnceLock::new(); - SOCKET.get_or_init(|| { - use std::time::{SystemTime, UNIX_EPOCH}; - let nanos = SystemTime::now() - .duration_since(UNIX_EPOCH) - .unwrap() - .subsec_nanos(); - let pid = std::process::id(); - format!("yeschef-test-{pid:08x}-{nanos:08x}") - }) -} - -/// A `tmux` command wired to the run's throwaway server ([`test_socket`]). -/// Read/kill helpers don't need `-f` (the server is already running with its -/// config from the binary's spawn), so only the socket is set. -fn tmux(args: &[&str]) -> std::process::Output { - Command::new("tmux") - .args(["-L", test_socket()]) - .args(args) - .output() - .expect("failed to run tmux") +/// A globally-unique tmux `-L` socket name for one [`TestEnv`]. Combines the +/// PID, a nanosecond timestamp, and a process-wide atomic counter so no two +/// envs ever collide: not two created back-to-back in the same thread (the +/// counter breaks any timestamp tie), not two separate test-binary runs +/// (distinct PIDs), and not a run beside a live yeschef instance (the +/// `yeschef-test-` prefix differs from the production `yeschef` socket). Each +/// env stands up and tears down the server on this socket by itself. +fn unique_socket() -> String { + use std::time::{SystemTime, UNIX_EPOCH}; + static COUNTER: AtomicU64 = AtomicU64::new(0); + let nanos = SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap() + .as_nanos(); + let pid = std::process::id(); + let seq = COUNTER.fetch_add(1, Ordering::Relaxed); + format!("yeschef-test-{pid:08x}-{nanos:x}-{seq:x}") } /// The flat tmux session id the backend uses for a ticket window: @@ -146,34 +210,6 @@ fn sid(window: &str) -> String { format!("yeschef-{window}") } -/// Kill a ticket's tmux session on drop (best-effort teardown). -struct WindowCleanup(Vec); - -impl Drop for WindowCleanup { - fn drop(&mut self) { - for window in &self.0 { - let _ = Command::new("tmux") - .args(["-L", test_socket(), "kill-session", "-t", &sid(window)]) - .output(); - } - } -} - -/// Capture a window's scrollback via the real tmux. -fn capture(window: &str) -> String { - let out = tmux(&["capture-pane", "-p", "-S", "-", "-t", &sid(window)]); - String::from_utf8_lossy(&out.stdout).into_owned() -} - -fn window_exists(window: &str) -> bool { - let id = sid(window); - let out = tmux(&["list-sessions", "-F", "#{session_name}"]); - out.status.success() - && String::from_utf8_lossy(&out.stdout) - .lines() - .any(|l| l.trim() == id) -} - // --------------------------------------------------------------------------- // init tests // --------------------------------------------------------------------------- @@ -423,7 +459,6 @@ fn spawn_creates_worktree_and_live_window() { .success(); let window = format!("{name}-demo"); - let _cleanup = WindowCleanup(vec![window.clone()]); // The prompt is no longer passed inline; spawn writes it to a file and // hands the agent a short "read this file" instruction (guards against the @@ -487,14 +522,14 @@ fn spawn_creates_worktree_and_live_window() { // tmux session is live. assert!( - window_exists(&window), + env.window_exists(&window), "tmux session for '{window}' should exist" ); // Give the shell a moment to echo, then peek should show the marker and the // file-indirection instruction the agent was launched with. std::thread::sleep(std::time::Duration::from_millis(800)); - let pane = capture(&window); + let pane = env.capture(&window); assert!( pane.contains("SPAWN_OK"), "pane should show the agent's output; got:\n{pane}" @@ -533,7 +568,6 @@ fn send_reaches_the_pane() { .success(); let window = format!("{name}-demo"); - let _cleanup = WindowCleanup(vec![window.clone()]); // `cat` reading stdin echoes whatever we send into the pane and stays // alive (tmux destroys a session whose process exits). Wrapping it in @@ -552,7 +586,7 @@ fn send_reaches_the_pane() { .success(); std::thread::sleep(std::time::Duration::from_millis(500)); - let pane = capture(&window); + let pane = env.capture(&window); assert!( pane.contains("HELLO_FROM_SEND"), "sent text should appear in the pane; got:\n{pane}" @@ -572,7 +606,6 @@ fn kill_removes_window_and_deregisters() { .success(); let window = format!("{name}-demo"); - let _cleanup = WindowCleanup(vec![window.clone()]); env.cmd() // A genuinely long-lived agent so the session stays up for the check; @@ -580,14 +613,20 @@ fn kill_removes_window_and_deregisters() { .args(["spawn", &name, "demo", "--agent", "sh -c 'sleep 30'"]) .assert() .success(); - assert!(window_exists(&window), "window should exist after spawn"); + assert!( + env.window_exists(&window), + "window should exist after spawn" + ); env.cmd() .args(["kill", &name, "demo", "--rm-worktree"]) .assert() .success(); - assert!(!window_exists(&window), "window should be gone after kill"); + assert!( + !env.window_exists(&window), + "window should be gone after kill" + ); let worktree = env .home_path() @@ -625,7 +664,6 @@ fn cleanup_dry_run_reports_without_removing() { .success(); let window = format!("{name}-demo"); - let _cleanup = WindowCleanup(vec![window.clone()]); // A branch spawned from `main` points at main's tip, so it is an ancestor // of origin/main — classified "merged". That alone is no longer enough to @@ -635,7 +673,10 @@ fn cleanup_dry_run_reports_without_removing() { .args(["spawn", &name, "demo", "--agent", "sh -c 'sleep 30'"]) .assert() .success(); - assert!(window_exists(&window), "window should exist after spawn"); + assert!( + env.window_exists(&window), + "window should exist after spawn" + ); env.cmd() .args(["ticket", &name, "demo", "status-set", "DONE"]) .assert() @@ -651,7 +692,10 @@ fn cleanup_dry_run_reports_without_removing() { .stdout(predicate::str::contains(format!("{name}/demo"))); // The window, worktree, and registry entry all survive the dry run. - assert!(window_exists(&window), "dry run must not kill the window"); + assert!( + env.window_exists(&window), + "dry run must not kill the window" + ); let worktree = env .home_path() .join("projects") @@ -679,13 +723,15 @@ fn cleanup_yes_reaps_merged_ticket() { .success(); let window = format!("{name}-demo"); - let _cleanup = WindowCleanup(vec![window.clone()]); env.cmd() .args(["spawn", &name, "demo", "--agent", "sh -c 'sleep 30'"]) .assert() .success(); - assert!(window_exists(&window), "window should exist after spawn"); + assert!( + env.window_exists(&window), + "window should exist after spawn" + ); // The branch is "merged" (points at main's tip), but reaping also requires // the cook to be inactive. Report DONE so both gates agree. @@ -703,7 +749,7 @@ fn cleanup_yes_reaps_merged_ticket() { // Session, worktree, and registry entry are all gone. assert!( - !window_exists(&window), + !env.window_exists(&window), "window should be killed by cleanup" ); let worktree = env @@ -736,13 +782,15 @@ fn cleanup_yes_keeps_active_ticket_even_when_merged() { .success(); let window = format!("{name}-demo"); - let _cleanup = WindowCleanup(vec![window.clone()]); env.cmd() .args(["spawn", &name, "demo", "--agent", "sh -c 'sleep 30'"]) .assert() .success(); - assert!(window_exists(&window), "window should exist after spawn"); + assert!( + env.window_exists(&window), + "window should exist after spawn" + ); // Even with --yes, the merged-but-active ticket is kept, and the report // says why. (The summary line always contains the word "reaped" — e.g. @@ -758,7 +806,7 @@ fn cleanup_yes_keeps_active_ticket_even_when_merged() { // Window, worktree, and registry entry all survive. assert!( - window_exists(&window), + env.window_exists(&window), "active ticket's window must survive" ); let worktree = env @@ -787,9 +835,6 @@ fn spawn_duplicate_window_rejected() { .assert() .success(); - let window = format!("{name}-demo"); - let _cleanup = WindowCleanup(vec![window]); - env.cmd() // A genuinely long-lived agent so the session stays up for the check; // the read-this-file instruction lands in `$0` and is ignored.