Merge pull request #2412 from rbtcollins/unit-tests

More unit-testification

Author: rbtcollins

Cargo.toml

@@ -107,7 +107,6 @@ members = ["download"]
 [lib]
 name = "rustup"
 path = "src/lib.rs"
-test = false # no unit tests
 
 [profile.release]
 lto = true

src/cli/self_update/windows.rs

@@ -134,7 +134,11 @@ pub fn wait_for_parent() -> Result<()> {
 
 pub fn do_add_to_path(methods: &[PathUpdateMethod]) -> Result<()> {
     assert!(methods.len() == 1 && methods[0] == PathUpdateMethod::Windows);
+    let new_path = _with_path_cargo_home_bin(_add_to_path)?;
+    _apply_new_path(new_path)
+}
 
+fn _apply_new_path(new_path: Option<String>) -> Result<()> {
     use std::ptr;
     use winapi::shared::minwindef::*;
     use winapi::um::winuser::{
@@ -143,39 +147,29 @@ pub fn do_add_to_path(methods: &[PathUpdateMethod]) -> Result<()> {
     use winreg::enums::{RegType, HKEY_CURRENT_USER, KEY_READ, KEY_WRITE};
     use winreg::{RegKey, RegValue};
 
-    let old_path = if let Some(s) = get_windows_path_var()? {
-        s
-    } else {
-        // Non-unicode path
-        return Ok(());
+    let new_path = match new_path {
+        Some(new_path) => new_path,
+        None => return Ok(()), // No need to set the path
     };
 
-    let mut new_path = utils::cargo_home()?
-        .join("bin")
-        .to_string_lossy()
-        .into_owned();
-    if old_path.contains(&new_path) {
-        return Ok(());
-    }
-
-    if !old_path.is_empty() {
-        new_path.push_str(";");
-        new_path.push_str(&old_path);
-    }
-
     let root = RegKey::predef(HKEY_CURRENT_USER);
     let environment = root
         .open_subkey_with_flags("Environment", KEY_READ | KEY_WRITE)
         .chain_err(|| ErrorKind::PermissionDenied)?;
 
-    let reg_value = RegValue {
-        bytes: utils::string_to_winreg_bytes(&new_path),
-        vtype: RegType::REG_EXPAND_SZ,
-    };
-
-    environment
-        .set_raw_value("PATH", &reg_value)
-        .chain_err(|| ErrorKind::PermissionDenied)?;
+    if new_path.is_empty() {
+        environment
+            .delete_value("PATH")
+            .chain_err(|| ErrorKind::PermissionDenied)?;
+    } else {
+        let reg_value = RegValue {
+            bytes: utils::string_to_winreg_bytes(&new_path),
+            vtype: RegType::REG_EXPAND_SZ,
+        };
+        environment
+            .set_raw_value("PATH", &reg_value)
+            .chain_err(|| ErrorKind::PermissionDenied)?;
+    }
 
     // Tell other processes to update their environment
     unsafe {
@@ -222,8 +216,23 @@ fn get_windows_path_var() -> Result<Option<String>> {
     }
 }
 
+// Returns None if the existing old_path does not need changing, otherwise
+// prepends the path_str to old_path, handling empty old_path appropriately.
+fn _add_to_path(old_path: &str, path_str: String) -> Option<String> {
+    if old_path.is_empty() {
+        Some(path_str)
+    } else if old_path.contains(&path_str) {
+        None
+    } else {
+        let mut new_path = path_str.clone();
+        new_path.push_str(";");
+        new_path.push_str(&old_path);
+        Some(new_path)
+    }
+}
+
 // Returns None if the existing old_path does not need changing
-fn _remove_from_path(old_path: &str, path_str: &str) -> Option<String> {
+fn _remove_from_path(old_path: &str, path_str: String) -> Option<String> {
     let idx = old_path.find(&path_str)?;
     // If there's a trailing semicolon (likely, since we probably added one
     // during install), include that in the substring to remove. We don't search
@@ -244,71 +253,22 @@ fn _remove_from_path(old_path: &str, path_str: &str) -> Option<String> {
     Some(new_path)
 }
 
-fn _path_without_cargo_home_bin() -> Result<Option<String>> {
-    let old_path = if let Some(s) = get_windows_path_var()? {
-        s
-    } else {
-        // Non-unicode path
-        return Ok(None);
-    };
-
+fn _with_path_cargo_home_bin<F>(f: F) -> Result<Option<String>>
+where
+    F: FnOnce(&str, String) -> Option<String>,
+{
+    let windows_path = get_windows_path_var()?;
     let path_str = utils::cargo_home()?
         .join("bin")
         .to_string_lossy()
         .into_owned();
-
-    Ok(_remove_from_path(&old_path, &path_str))
+    Ok(windows_path.and_then(|old_path| f(&old_path, path_str)))
 }
 
 pub fn do_remove_from_path(methods: &[PathUpdateMethod]) -> Result<()> {
     assert!(methods.len() == 1 && methods[0] == PathUpdateMethod::Windows);
-
-    use std::ptr;
-    use winapi::shared::minwindef::*;
-    use winapi::um::winuser::{
-        SendMessageTimeoutA, HWND_BROADCAST, SMTO_ABORTIFHUNG, WM_SETTINGCHANGE,
-    };
-    use winreg::enums::{RegType, HKEY_CURRENT_USER, KEY_READ, KEY_WRITE};
-    use winreg::{RegKey, RegValue};
-
-    let new_path = match _path_without_cargo_home_bin()? {
-        Some(new_path) => new_path,
-        None => return Ok(()), // No need to set the path
-    };
-
-    let root = RegKey::predef(HKEY_CURRENT_USER);
-    let environment = root
-        .open_subkey_with_flags("Environment", KEY_READ | KEY_WRITE)
-        .chain_err(|| ErrorKind::PermissionDenied)?;
-
-    if new_path.is_empty() {
-        environment
-            .delete_value("PATH")
-            .chain_err(|| ErrorKind::PermissionDenied)?;
-    } else {
-        let reg_value = RegValue {
-            bytes: utils::string_to_winreg_bytes(&new_path),
-            vtype: RegType::REG_EXPAND_SZ,
-        };
-        environment
-            .set_raw_value("PATH", &reg_value)
-            .chain_err(|| ErrorKind::PermissionDenied)?;
-    }
-
-    // Tell other processes to update their environment
-    unsafe {
-        SendMessageTimeoutA(
-            HWND_BROADCAST,
-            WM_SETTINGCHANGE,
-            0 as WPARAM,
-            "Environment\0".as_ptr() as LPARAM,
-            SMTO_ABORTIFHUNG,
-            5000,
-            ptr::null_mut(),
-        );
-    }
-
-    Ok(())
+    let new_path = _with_path_cargo_home_bin(_remove_from_path)?;
+    _apply_new_path(new_path)
 }
 
 pub fn run_update(setup_path: &Path) -> Result<utils::ExitCode> {
@@ -503,9 +463,26 @@ mod tests {
     }
 
     #[test]
-    fn windows_uninstall_doesnt_mess_with_a_non_unicode_path() {
+    fn windows_install_does_not_add_path_twice() {
+        assert_eq!(
+            None,
+            super::_add_to_path(
+                r"c:\users\example\.cargo\bin;foo",
+                r"c:\users\example\.cargo\bin".into()
+            )
+        );
+    }
+
+    #[test]
+    fn windows_doesnt_mess_with_a_non_unicode_path() {
         // This writes an error, so we want a sink for it.
-        let tp = Box::new(currentprocess::TestProcess::default());
+        let tp = Box::new(currentprocess::TestProcess {
+            vars: [("HOME".to_string(), "/unused".to_string())]
+                .iter()
+                .cloned()
+                .collect(),
+            ..Default::default()
+        });
         with_registry_edits(&|| {
             currentprocess::with(tp.clone(), || {
                 let root = RegKey::predef(HKEY_CURRENT_USER);
@@ -522,7 +499,10 @@ mod tests {
                 };
                 environment.set_raw_value("PATH", &reg_value).unwrap();
                 // Ok(None) signals no change to the PATH setting layer
-                assert_eq!(None, super::_path_without_cargo_home_bin().unwrap());
+                fn panic(_: &str, _: String) -> Option<String> {
+                    panic!("called");
+                }
+                assert_eq!(None, super::_with_path_cargo_home_bin(panic).unwrap());
             })
         });
         assert_eq!(
@@ -532,13 +512,38 @@ mod tests {
         );
     }
 
+    #[test]
+    fn windows_path_regkey_type() {
+        // per issue #261, setting PATH should use REG_EXPAND_SZ.
+        let tp = Box::new(currentprocess::TestProcess::default());
+        with_registry_edits(&|| {
+            currentprocess::with(tp.clone(), || {
+                let root = RegKey::predef(HKEY_CURRENT_USER);
+                let environment = root
+                    .open_subkey_with_flags("Environment", KEY_READ | KEY_WRITE)
+                    .unwrap();
+                environment.delete_value("PATH").unwrap();
+
+                assert_eq!((), super::_apply_new_path(Some("foo".into())).unwrap());
+
+                let root = RegKey::predef(HKEY_CURRENT_USER);
+                let environment = root
+                    .open_subkey_with_flags("Environment", KEY_READ | KEY_WRITE)
+                    .unwrap();
+                let path = environment.get_raw_value("PATH").unwrap();
+                assert_eq!(path.vtype, RegType::REG_EXPAND_SZ);
+                assert_eq!(utils::string_to_winreg_bytes("foo"), &path.bytes[..]);
+            })
+        });
+    }
+
     #[test]
     fn windows_uninstall_removes_semicolon_from_path_prefix() {
         assert_eq!(
             "foo",
             super::_remove_from_path(
                 r"c:\users\example\.cargo\bin;foo",
-                r"c:\users\example\.cargo\bin"
+                r"c:\users\example\.cargo\bin".into()
             )
             .unwrap()
         )
@@ -550,7 +555,7 @@ mod tests {
             "foo",
             super::_remove_from_path(
                 r"foo;c:\users\example\.cargo\bin",
-                r"c:\users\example\.cargo\bin"
+                r"c:\users\example\.cargo\bin".into()
             )
             .unwrap()
         )

src/currentprocess.rs

@@ -191,11 +191,11 @@ impl ProcessSource for OSProcess {
 
 #[derive(Clone, Debug, Default)]
 pub struct TestProcess {
-    cwd: PathBuf,
-    args: Vec<String>,
-    vars: HashMap<String, String>,
-    id: u64,
-    stdin: TestStdinInner,
+    pub cwd: PathBuf,
+    pub args: Vec<String>,
+    pub vars: HashMap<String, String>,
+    pub id: u64,
+    pub stdin: TestStdinInner,
     pub stdout: TestWriterInner,
     pub stderr: TestWriterInner,
 }

tests/cli-self-upd.rs

@@ -506,18 +506,6 @@ fn install_adds_path() {
     });
 }
 
-#[test]
-#[cfg(windows)]
-fn install_does_not_add_path_twice() {
-    setup(&|config| {
-        expect_ok(config, &["rustup-init", "-y"]);
-        expect_ok(config, &["rustup-init", "-y"]);
-
-        let path = config.cargodir.join("bin").to_string_lossy().to_string();
-        assert_eq!(get_path().unwrap().matches(&path).count(), 1);
-    });
-}
-
 #[test]
 #[cfg(windows)]
 fn uninstall_removes_path() {
@@ -1027,34 +1015,6 @@ fn rustup_init_works_with_weird_names() {
     });
 }
 
-// # 261
-#[test]
-#[cfg(windows)]
-fn doesnt_write_wrong_path_type_to_reg() {
-    use winreg::enums::{RegType, HKEY_CURRENT_USER, KEY_READ, KEY_WRITE};
-    use winreg::RegKey;
-
-    setup(&|config| {
-        expect_ok(config, &["rustup-init", "-y"]);
-
-        let root = RegKey::predef(HKEY_CURRENT_USER);
-        let environment = root
-            .open_subkey_with_flags("Environment", KEY_READ | KEY_WRITE)
-            .unwrap();
-        let path = environment.get_raw_value("PATH").unwrap();
-        assert!(path.vtype == RegType::REG_EXPAND_SZ);
-
-        expect_ok(config, &["rustup", "self", "uninstall", "-y"]);
-
-        let root = RegKey::predef(HKEY_CURRENT_USER);
-        let environment = root
-            .open_subkey_with_flags("Environment", KEY_READ | KEY_WRITE)
-            .unwrap();
-        let path = environment.get_raw_value("PATH").unwrap();
-        assert!(path.vtype == RegType::REG_EXPAND_SZ);
-    });
-}
-
 // HKCU\Environment\PATH may not exist during install, and it may need to be
 // deleted during uninstall if we remove the last path from it
 #[test]
@@ -1091,41 +1051,6 @@ fn windows_handle_empty_path_registry_key() {
     });
 }
 
-#[test]
-#[cfg(windows)]
-fn install_doesnt_mess_with_a_non_unicode_path() {
-    use winreg::enums::{RegType, HKEY_CURRENT_USER, KEY_READ, KEY_WRITE};
-    use winreg::{RegKey, RegValue};
-
-    setup(&|config| {
-        let root = RegKey::predef(HKEY_CURRENT_USER);
-        let environment = root
-            .open_subkey_with_flags("Environment", KEY_READ | KEY_WRITE)
-            .unwrap();
-
-        let reg_value = RegValue {
-            bytes: vec![
-                0x00, 0xD8, // leading surrogate
-                0x01, 0x01, // bogus trailing surrogate
-                0x00, 0x00,
-            ], // null
-            vtype: RegType::REG_EXPAND_SZ,
-        };
-        environment.set_raw_value("PATH", &reg_value).unwrap();
-
-        expect_stderr_ok(config, &["rustup-init", "-y"],
-                         "the registry key HKEY_CURRENT_USER\\Environment\\PATH does not contain valid Unicode. \
-                          Not modifying the PATH variable");
-
-        let root = RegKey::predef(HKEY_CURRENT_USER);
-        let environment = root
-            .open_subkey_with_flags("Environment", KEY_READ | KEY_WRITE)
-            .unwrap();
-        let path = environment.get_raw_value("PATH").unwrap();
-        assert_eq!(path.bytes, reg_value.bytes);
-    });
-}
-
 #[test]
 #[ignore] // untestable
 fn install_but_rustup_is_installed() {}