From a8e2fffad4d4a4613f71ff6d1a8230194981f9bf Mon Sep 17 00:00:00 2001 From: Amolith Date: Wed, 18 Mar 2026 22:14:15 -0600 Subject: [PATCH] Fix flaky editor tests by extracting env reads from resolution logic --- src/editor.rs | 122 ++++++++++++++++++++++++++------------------------ 1 file changed, 63 insertions(+), 59 deletions(-) diff --git a/src/editor.rs b/src/editor.rs index 366fff78d65d2243439b530b46df6c39145ac0e6..4d09bb7d3d7764223a8b5d0c343c511254d6a6e3 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -10,24 +10,26 @@ /// supply a fake one (e.g. a shell one-liner that writes known content). use anyhow::{anyhow, Result}; -/// Locate the editor binary (and any leading flags) from the environment. +/// Resolve the editor command from three optional sources. /// -/// Priority: `TD_FORCE_EDITOR` (tests) → `$VISUAL` → `$EDITOR` → -/// platform default. Returns the argv list ready to pass to -/// `std::process::Command`. -pub fn find_editor() -> Result> { - // TD_FORCE_EDITOR lets tests inject a fake editor without touching the - // real environment variables. - let raw = std::env::var("TD_FORCE_EDITOR") - .or_else(|_| std::env::var("VISUAL")) - .or_else(|_| std::env::var("EDITOR")) - .unwrap_or_else(|_| { - if cfg!(windows) { - "notepad".to_owned() - } else { - "nano".to_owned() - } - }); +/// Priority: `force` (`TD_FORCE_EDITOR`) → `visual` (`$VISUAL`) → +/// `editor` (`$EDITOR`) → platform default. Returns the argv list ready +/// to pass to `std::process::Command`. +/// +/// This is a pure function to allow deterministic testing without +/// touching process-wide env vars. +fn resolve_editor( + force: Option, + visual: Option, + editor: Option, +) -> Result> { + let raw = force.or(visual).or(editor).unwrap_or_else(|| { + if cfg!(windows) { + "notepad".to_owned() + } else { + "nano".to_owned() + } + }); let argv = shell_words::split(&raw) .map_err(|e| anyhow!("could not parse editor command {:?}: {}", raw, e))?; @@ -41,6 +43,19 @@ pub fn find_editor() -> Result> { Ok(argv) } +/// Locate the editor binary (and any leading flags) from the environment. +/// +/// Priority: `TD_FORCE_EDITOR` (tests) → `$VISUAL` → `$EDITOR` → +/// platform default. Returns the argv list ready to pass to +/// `std::process::Command`. +pub fn find_editor() -> Result> { + resolve_editor( + std::env::var("TD_FORCE_EDITOR").ok(), + std::env::var("VISUAL").ok(), + std::env::var("EDITOR").ok(), + ) +} + /// Strip lines beginning with `TD: `, then extract the first non-blank line /// as the title and everything that follows as the description. /// @@ -116,70 +131,60 @@ pub fn open(template: &str) -> Result<(String, String)> { mod tests { use super::*; - // ── find_editor ─────────────────────────────────────────────────────────── + // ── resolve_editor ──────────────────────────────────────────────────────── + // + // These tests call the pure `resolve_editor` function directly, avoiding + // process-wide env var mutations that caused flaky parallel test failures. #[test] - fn find_editor_prefers_td_force_editor() { - // TD_FORCE_EDITOR should take precedence over VISUAL and EDITOR. - std::env::remove_var("VISUAL"); - std::env::remove_var("EDITOR"); - std::env::set_var("TD_FORCE_EDITOR", "myfakeeditor"); - let argv = find_editor().unwrap(); + fn resolve_prefers_force_over_visual_and_editor() { + let argv = resolve_editor( + Some("myfakeeditor".into()), + Some("vis".into()), + Some("ed".into()), + ) + .unwrap(); assert_eq!(argv, vec!["myfakeeditor"]); - std::env::remove_var("TD_FORCE_EDITOR"); } #[test] - fn find_editor_prefers_visual_over_editor() { - std::env::remove_var("TD_FORCE_EDITOR"); - std::env::set_var("VISUAL", "vis"); - std::env::set_var("EDITOR", "ed"); - let argv = find_editor().unwrap(); + fn resolve_prefers_visual_over_editor() { + let argv = resolve_editor(None, Some("vis".into()), Some("ed".into())).unwrap(); assert_eq!(argv[0], "vis"); - std::env::remove_var("VISUAL"); - std::env::remove_var("EDITOR"); } #[test] - fn find_editor_falls_back_to_editor() { - std::env::remove_var("TD_FORCE_EDITOR"); - std::env::remove_var("VISUAL"); - std::env::set_var("EDITOR", "myeditor"); - let argv = find_editor().unwrap(); + fn resolve_falls_back_to_editor() { + let argv = resolve_editor(None, None, Some("myeditor".into())).unwrap(); assert_eq!(argv[0], "myeditor"); - std::env::remove_var("EDITOR"); } #[test] - fn find_editor_splits_args() { - // Values like "vim -u NONE" must be split into separate argv entries. - std::env::remove_var("TD_FORCE_EDITOR"); - std::env::remove_var("VISUAL"); - std::env::set_var("EDITOR", "vim -u NONE"); - let argv = find_editor().unwrap(); + fn resolve_falls_back_to_platform_default() { + let argv = resolve_editor(None, None, None).unwrap(); + if cfg!(windows) { + assert_eq!(argv, vec!["notepad"]); + } else { + assert_eq!(argv, vec!["nano"]); + } + } + + #[test] + fn resolve_splits_args() { + let argv = resolve_editor(None, None, Some("vim -u NONE".into())).unwrap(); assert_eq!(argv, vec!["vim", "-u", "NONE"]); - std::env::remove_var("EDITOR"); } #[test] - fn find_editor_splits_quoted_path() { - // Paths with spaces must survive shell-word quoting. - std::env::remove_var("TD_FORCE_EDITOR"); - std::env::set_var("VISUAL", "\"/usr/local/bin/my editor\""); - std::env::remove_var("EDITOR"); - let argv = find_editor().unwrap(); + fn resolve_splits_quoted_path() { + let argv = resolve_editor(None, Some("\"/usr/local/bin/my editor\"".into()), None).unwrap(); assert_eq!(argv, vec!["/usr/local/bin/my editor"]); - std::env::remove_var("VISUAL"); } #[test] - fn find_editor_errors_on_unmatched_quote() { - std::env::remove_var("TD_FORCE_EDITOR"); - std::env::remove_var("VISUAL"); - std::env::set_var("EDITOR", "vim 'unterminated"); - let result = find_editor(); + fn resolve_errors_on_unmatched_quote() { + let result = resolve_editor(None, None, Some("vim 'unterminated".into())); assert!(result.is_err()); - std::env::remove_var("EDITOR"); } // ── parse_result ────────────────────────────────────────────────────────── @@ -215,7 +220,6 @@ mod tests { let content = "Title\n\nParagraph one\n\nParagraph two\n"; let (title, desc) = parse_result(content).unwrap(); assert_eq!(title, "Title"); - // Leading/trailing blank lines stripped from description. assert_eq!(desc, "Paragraph one\n\nParagraph two"); }