Isolate environment state changes into Host. (#1296)

Moves the state changes for setting and removing environment variables
into the context's host as opposed to calling `std::env::*` directly
from anywhere else.

Introduced FakeHost to prevent environemnt state changes leaking
between unit tests and cause random test failures.

src/env/environment_syncer.rs

@@ -97,17 +97,17 @@ impl EnvironmentSyncer {
     }
 
     #[cfg(test)]
-    pub fn clear_env_vars(&mut self) {
+    pub fn clear_env_vars(&mut self, ctx: &mut Context) {
-        for (key, _value) in std::env::vars() {
+        for (key, _value) in ctx.with_host(|host| host.vars()) {
             if key != "path" && key != "PATH" {
-                std::env::remove_var(key);
+                ctx.with_host(|host| host.env_rm(std::ffi::OsString::from(key)));
             }
         }
     }
 
     #[cfg(test)]
-    pub fn clear_path_var(&mut self) {
+    pub fn clear_path_var(&mut self, ctx: &mut Context) {
-        std::env::remove_var("PATH")
+        ctx.with_host(|host| host.env_rm(std::ffi::OsString::from("PATH")));
     }
 }
 
@@ -120,13 +120,15 @@ mod tests {
     use nu_errors::ShellError;
     use nu_test_support::fs::Stub::FileWithContent;
     use nu_test_support::playground::Playground;
+    use parking_lot::Mutex;
     use std::path::PathBuf;
+    use std::sync::Arc;
 
-    //#[test]
+    #[test]
-    #[allow(unused)]
     fn syncs_env_if_new_env_entry_in_session_is_not_in_configuration_file() -> Result<(), ShellError>
     {
         let mut ctx = Context::basic()?;
+        ctx.host = Arc::new(Mutex::new(Box::new(crate::env::host::FakeHost::new())));
 
         let expected = vec![
             (
@@ -155,14 +157,16 @@ mod tests {
             // Here, the environment variables from the current session
             // are cleared since we will load and set them from the
             // configuration file (if any)
-            actual.clear_env_vars();
+            actual.clear_env_vars(&mut ctx);
 
             // We explicitly simulate and add the USER variable to the current
             // session's environment variables with the value "NUNO".
-            std::env::set_var(
+            ctx.with_host(|test_host| {
-                std::ffi::OsString::from("USER"),
+                test_host.env_set(
-                std::ffi::OsString::from("NUNO"),
+                    std::ffi::OsString::from("USER"),
-            );
+                    std::ffi::OsString::from("NUNO"),
+                )
+            });
 
             // Nu loads the environment variables from the configuration file (if any)
             actual.load_environment();
@@ -219,10 +223,10 @@ mod tests {
         Ok(())
     }
 
-    //#[test]
+    #[test]
-    #[allow(unused)]
     fn nu_envs_have_higher_priority_and_does_not_get_overwritten() -> Result<(), ShellError> {
         let mut ctx = Context::basic()?;
+        ctx.host = Arc::new(Mutex::new(Box::new(crate::env::host::FakeHost::new())));
 
         let expected = vec![(
             "SHELL".to_string(),
@@ -245,12 +249,14 @@ mod tests {
             let mut actual = EnvironmentSyncer::new();
             actual.set_config(Box::new(fake_config));
 
-            actual.clear_env_vars();
+            actual.clear_env_vars(&mut ctx);
 
-            std::env::set_var(
+            ctx.with_host(|test_host| {
-                std::ffi::OsString::from("SHELL"),
+                test_host.env_set(
-                std::ffi::OsString::from("/usr/bin/sh"),
+                    std::ffi::OsString::from("SHELL"),
-            );
+                    std::ffi::OsString::from("/usr/bin/sh"),
+                )
+            });
 
             actual.load_environment();
             actual.sync_env_vars(&mut ctx);
@@ -286,11 +292,11 @@ mod tests {
         Ok(())
     }
 
-    //#[test]
+    #[test]
-    #[allow(unused)]
     fn syncs_path_if_new_path_entry_in_session_is_not_in_configuration_file(
     ) -> Result<(), ShellError> {
         let mut ctx = Context::basic()?;
+        ctx.host = Arc::new(Mutex::new(Box::new(crate::env::host::FakeHost::new())));
 
         let expected = std::env::join_paths(vec![
             PathBuf::from("/path/to/be/added"),
@@ -319,15 +325,17 @@ mod tests {
             // Here, the environment variables from the current session
             // are cleared since we will load and set them from the
             // configuration file (if any)
-            actual.clear_path_var();
+            actual.clear_path_var(&mut ctx);
 
             // We explicitly simulate and add the PATH variable to the current
             // session with the path "/path/to/be/added".
-            std::env::set_var(
+            ctx.with_host(|test_host| {
-                std::ffi::OsString::from("PATH"),
+                test_host.env_set(
-                std::env::join_paths(vec![PathBuf::from("/path/to/be/added")])
+                    std::ffi::OsString::from("PATH"),
-                    .expect("Couldn't join paths."),
+                    std::env::join_paths(vec![PathBuf::from("/path/to/be/added")])
-            );
+                        .expect("Couldn't join paths."),
+                )
+            });
 
             // Nu loads the path variables from the configuration file (if any)
             actual.load_environment();

src/env/host.rs

@@ -1,4 +1,6 @@
 use crate::prelude::*;
+#[cfg(test)]
+use indexmap::IndexMap;
 use nu_errors::ShellError;
 use std::ffi::OsString;
 use std::fmt::Debug;
@@ -13,7 +15,7 @@ pub trait Host: Debug + Send {
     fn stdout(&mut self, out: &str);
     fn stderr(&mut self, out: &str);
 
-    fn vars(&mut self) -> std::env::Vars;
+    fn vars(&mut self) -> Vec<(String, String)>;
     fn env_get(&mut self, key: OsString) -> Option<OsString>;
     fn env_set(&mut self, k: OsString, v: OsString);
     fn env_rm(&mut self, k: OsString);
@@ -38,7 +40,7 @@ impl Host for Box<dyn Host> {
         (**self).stderr(out)
     }
 
-    fn vars(&mut self) -> std::env::Vars {
+    fn vars(&mut self) -> Vec<(String, String)> {
         (**self).vars()
     }
 
@@ -93,8 +95,8 @@ impl Host for BasicHost {
         }
     }
 
-    fn vars(&mut self) -> std::env::Vars {
+    fn vars(&mut self) -> Vec<(String, String)> {
-        std::env::vars()
+        std::env::vars().collect::<Vec<_>>()
     }
 
     fn env_get(&mut self, key: OsString) -> Option<OsString> {
@@ -122,6 +124,82 @@ impl Host for BasicHost {
     }
 }
 
+#[cfg(test)]
+#[derive(Debug)]
+pub struct FakeHost {
+    line_written: String,
+    env_vars: IndexMap<String, String>,
+}
+
+#[cfg(test)]
+impl FakeHost {
+    pub fn new() -> FakeHost {
+        FakeHost {
+            line_written: String::from(""),
+            env_vars: IndexMap::default(),
+        }
+    }
+}
+
+#[cfg(test)]
+impl Host for FakeHost {
+    fn out_terminal(&self) -> Option<Box<term::StdoutTerminal>> {
+        None
+    }
+
+    fn err_terminal(&self) -> Option<Box<term::StderrTerminal>> {
+        None
+    }
+
+    fn stdout(&mut self, out: &str) {
+        self.line_written = out.to_string();
+    }
+
+    fn stderr(&mut self, out: &str) {
+        self.line_written = out.to_string();
+    }
+
+    fn vars(&mut self) -> Vec<(String, String)> {
+        self.env_vars
+            .iter()
+            .map(|(k, v)| (k.clone(), v.clone()))
+            .collect::<Vec<_>>()
+    }
+
+    fn env_get(&mut self, key: OsString) -> Option<OsString> {
+        let key = key.into_string().expect("Couldn't convert to string.");
+
+        match self.env_vars.get(&key) {
+            Some(env) => Some(OsString::from(env)),
+            None => None,
+        }
+    }
+
+    fn env_set(&mut self, key: OsString, value: OsString) {
+        self.env_vars.insert(
+            key.into_string().expect("Couldn't convert to string."),
+            value.into_string().expect("Couldn't convert to string."),
+        );
+    }
+
+    fn env_rm(&mut self, key: OsString) {
+        self.env_vars
+            .shift_remove(&key.into_string().expect("Couldn't convert to string."));
+    }
+
+    fn out_termcolor(&self) -> termcolor::StandardStream {
+        termcolor::StandardStream::stdout(termcolor::ColorChoice::Auto)
+    }
+
+    fn err_termcolor(&self) -> termcolor::StandardStream {
+        termcolor::StandardStream::stderr(termcolor::ColorChoice::Auto)
+    }
+
+    fn width(&self) -> usize {
+        1
+    }
+}
+
 pub(crate) fn handle_unexpected<T>(
     host: &mut dyn Host,
     func: impl FnOnce(&mut dyn Host) -> Result<T, ShellError>,