Test cleanup of rustup-init functional tests

Run all rustup-init tests that can in parallel.

Use common helpers where possible.

Move some tests to more relevant modules.

Fixes test path leakages on Windows I think.

Author: rbtcollins

src/cli/self_update.rs

@@ -30,6 +30,20 @@
 //! Deleting the running binary during uninstall is tricky
 //! and racy on Windows.
 
+#[cfg(unix)]
+mod shell;
+pub mod test;
+#[cfg(unix)]
+mod unix;
+#[cfg(windows)]
+mod windows;
+mod os {
+    #[cfg(unix)]
+    pub use super::unix::*;
+    #[cfg(windows)]
+    pub use super::windows::*;
+}
+
 use std::borrow::Cow;
 use std::env;
 use std::env::consts::EXE_SUFFIX;
@@ -50,19 +64,6 @@ use crate::utils::utils;
 use crate::utils::Notification;
 use crate::{Cfg, UpdateStatus};
 use crate::{DUP_TOOLS, TOOLS};
-
-#[cfg(unix)]
-mod shell;
-#[cfg(unix)]
-mod unix;
-#[cfg(windows)]
-mod windows;
-mod os {
-    #[cfg(unix)]
-    pub use super::unix::*;
-    #[cfg(windows)]
-    pub use super::windows::*;
-}
 use os::*;
 pub use os::{complete_windows_uninstall, delete_rustup_and_cargo_home, run_update, self_replace};
 
@@ -1078,7 +1079,7 @@ pub fn cleanup_self_updater() -> Result<()> {
 }
 
 #[cfg(test)]
-mod test {
+mod tests {
     use std::collections::HashMap;
 
     use crate::cli::common;

src/cli/self_update/test.rs

@@ -0,0 +1,68 @@
+//! Support for functional tests.
+
+use std::sync::Mutex;
+
+use lazy_static::lazy_static;
+#[cfg(not(unix))]
+use winreg::{
+    enums::{RegType, HKEY_CURRENT_USER, KEY_READ, KEY_WRITE},
+    RegKey, RegValue,
+};
+
+#[cfg(not(unix))]
+use crate::utils::utils;
+
+#[cfg(not(unix))]
+pub fn get_path() -> Option<String> {
+    let root = RegKey::predef(HKEY_CURRENT_USER);
+    let environment = root
+        .open_subkey_with_flags("Environment", KEY_READ | KEY_WRITE)
+        .unwrap();
+    // XXX: copied from the mock support crate, but I am suspicous of this
+    // code: This uses ok to allow signalling None for 'delete', but this
+    // can fail e.g. with !(winerror::ERROR_BAD_FILE_TYPE) or other
+    // failures; which will lead to attempting to delete the users path
+    // rather than aborting the test suite.
+    environment.get_value("PATH").ok()
+}
+
+#[cfg(not(unix))]
+fn restore_path(p: Option<String>) {
+    let root = RegKey::predef(HKEY_CURRENT_USER);
+    let environment = root
+        .open_subkey_with_flags("Environment", KEY_READ | KEY_WRITE)
+        .unwrap();
+    if let Some(p) = p.as_ref() {
+        let reg_value = RegValue {
+            bytes: utils::string_to_winreg_bytes(&p),
+            vtype: RegType::REG_EXPAND_SZ,
+        };
+        environment.set_raw_value("PATH", &reg_value).unwrap();
+    } else {
+        let _ = environment.delete_value("PATH");
+    }
+}
+
+/// Support testing of code that mutates global path state
+pub fn with_saved_path(f: &dyn Fn()) {
+    // Lock protects concurrent mutation of registry
+    lazy_static! {
+        static ref LOCK: Mutex<()> = Mutex::new(());
+    }
+    let _g = LOCK.lock();
+
+    // On windows these tests mess with the user's PATH. Save
+    // and restore them here to keep from trashing things.
+    let saved_path = get_path();
+    let _g = scopeguard::guard(saved_path, restore_path);
+
+    f();
+}
+
+#[cfg(unix)]
+pub fn get_path() -> Option<String> {
+    None
+}
+
+#[cfg(unix)]
+fn restore_path(_: Option<String>) {}

src/cli/self_update/windows.rs

@@ -394,59 +394,13 @@ pub fn delete_rustup_and_cargo_home() -> Result<()> {
 
 #[cfg(test)]
 mod tests {
-    use std::sync::Mutex;
-
-    use lazy_static::lazy_static;
     use winreg::enums::{RegType, HKEY_CURRENT_USER, KEY_READ, KEY_WRITE};
     use winreg::{RegKey, RegValue};
 
     use crate::currentprocess;
+    use crate::test::with_saved_path;
     use crate::utils::utils;
 
-    pub fn get_path() -> Option<String> {
-        let root = RegKey::predef(HKEY_CURRENT_USER);
-        let environment = root
-            .open_subkey_with_flags("Environment", KEY_READ | KEY_WRITE)
-            .unwrap();
-        // XXX: copied from the mock support crate, but I am suspicous of this
-        // code: This uses ok to allow signalling None for 'delete', but this
-        // can fail e.g. with !(winerror::ERROR_BAD_FILE_TYPE) or other
-        // failures; which will lead to attempting to delete the users path
-        // rather than aborting the test suite.
-        environment.get_value("PATH").ok()
-    }
-
-    pub fn restore_path(p: Option<String>) {
-        let root = RegKey::predef(HKEY_CURRENT_USER);
-        let environment = root
-            .open_subkey_with_flags("Environment", KEY_READ | KEY_WRITE)
-            .unwrap();
-        if let Some(p) = p.as_ref() {
-            let reg_value = RegValue {
-                bytes: utils::string_to_winreg_bytes(&p),
-                vtype: RegType::REG_EXPAND_SZ,
-            };
-            environment.set_raw_value("PATH", &reg_value).unwrap();
-        } else {
-            let _ = environment.delete_value("PATH");
-        }
-    }
-
-    fn with_registry_edits(f: &dyn Fn()) {
-        // Lock protects concurrent mutation of registry
-        lazy_static! {
-            static ref LOCK: Mutex<()> = Mutex::new(());
-        }
-        let _g = LOCK.lock();
-
-        // On windows these tests mess with the user's PATH. Save
-        // and restore them here to keep from trashing things.
-        let saved_path = get_path();
-        let _g = scopeguard::guard(saved_path, restore_path);
-
-        f();
-    }
-
     #[test]
     fn windows_install_does_not_add_path_twice() {
         assert_eq!(
@@ -468,7 +422,7 @@ mod tests {
                 .collect(),
             ..Default::default()
         });
-        with_registry_edits(&|| {
+        with_saved_path(&|| {
             currentprocess::with(tp.clone(), || {
                 let root = RegKey::predef(HKEY_CURRENT_USER);
                 let environment = root
@@ -501,7 +455,7 @@ mod tests {
     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(&|| {
+        with_saved_path(&|| {
             currentprocess::with(tp.clone(), || {
                 let root = RegKey::predef(HKEY_CURRENT_USER);
                 let environment = root
@@ -528,7 +482,7 @@ mod tests {
         // during uninstall the PATH key may end up empty; if so we should
         // delete it.
         let tp = Box::new(currentprocess::TestProcess::default());
-        with_registry_edits(&|| {
+        with_saved_path(&|| {
             currentprocess::with(tp.clone(), || {
                 let root = RegKey::predef(HKEY_CURRENT_USER);
                 let environment = root
@@ -560,7 +514,7 @@ mod tests {
     fn windows_treat_missing_path_as_empty() {
         // during install the PATH key may be missing; treat it as empty
         let tp = Box::new(currentprocess::TestProcess::default());
-        with_registry_edits(&|| {
+        with_saved_path(&|| {
             currentprocess::with(tp.clone(), || {
                 let root = RegKey::predef(HKEY_CURRENT_USER);
                 let environment = root

src/test.rs

@@ -8,6 +8,7 @@ use std::io;
 use std::path::{Path, PathBuf};
 use std::process::Command;
 
+pub use crate::cli::self_update::test::{get_path, with_saved_path};
 use crate::currentprocess;
 use crate::dist::dist::TargetTriple;
 

tests/cli-exact.rs

@@ -15,7 +15,7 @@ fn setup(f: &dyn Fn(&mut Config)) {
 }
 
 #[test]
-fn update() {
+fn update_once() {
     setup(&|config| {
         expect_ok_ex(
             config,