Fix flaky editor tests by extracting env reads from resolution logic

Amolith created

Change summary

src/editor.rs | 122 +++++++++++++++++++++++++++-------------------------
1 file changed, 63 insertions(+), 59 deletions(-)

Detailed changes

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<Vec<String>> {
-    // 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<String>,
+    visual: Option<String>,
+    editor: Option<String>,
+) -> Result<Vec<String>> {
+    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<Vec<String>> {
     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<Vec<String>> {
+    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");
     }