add new test and add fish script to add to path

Artemis Livingstone · Artemis Livingstone · commit 0c96a4590714 · 2022-12-30T01:44:46.000Z
diff --git a/.gitignore b/.gitignore
@@ -9,3 +9,4 @@
 /local-rustup
 /snapcraft
 flake.lock
+.vscode/
diff --git a/src/cli/self_update/env.fish b/src/cli/self_update/env.fish
@@ -0,0 +1 @@
+contains {cargo_bin} $fish_user_paths; or set -Ua fish_user_paths {cargo_bin}
diff --git a/src/cli/self_update/shell.rs b/src/cli/self_update/shell.rs
@@ -1,3 +1,7 @@
+#![warn(clippy::all)]
+#![warn(clippy::pedantic)]
+#![allow(clippy::doc_markdown)]
+
 //! Paths and Unix shells
 //!
 //! MacOS, Linux, FreeBSD, and many other OS model their design on Unix,
@@ -95,7 +99,7 @@ pub(crate) trait UnixShell {
     // Gives rcs that should be written to.
     fn update_rcs(&self) -> Vec<PathBuf>;
 
-    // By defualt follows POSIX and sources <cargo home>/env
+    // By default follows POSIX and sources <cargo home>/env
     fn add_to_path(&self) -> Result<(), anyhow::Error> {
         let source_cmd = self.source_string()?;
         let source_cmd_with_newline = format!("\n{}", &source_cmd);
@@ -203,7 +207,7 @@ impl Zsh {
             }
         } else {
             match std::process::Command::new("zsh")
-                .args(&["-c", "'echo $ZDOTDIR'"])
+                .args(["-c", "'echo $ZDOTDIR'"])
                 .output()
             {
                 Ok(io) if !io.stdout.is_empty() => Ok(PathBuf::from(OsStr::from_bytes(&io.stdout))),
@@ -249,13 +253,13 @@ impl Fish {
     #![allow(non_snake_case)]
     /// Gets fish vendor config location from `XDG_DATA_DIRS`
     /// Returns None if `XDG_DATA_DIRS` is not set or if there is no fis`fish/vendor_conf.d`.d directory found
-    pub fn get_XDG_DATA_DIRS() -> Option<PathBuf> {
-        #[cfg(test)]
-        return None;
+    pub fn get_vendor_config_from_XDG_DATA_DIRS() -> Option<PathBuf> {
+        // Skip the directory during testing as we don't want to write into the XDG_DATA_DIRS by accident
+        // TODO: Change the definition of XDG_DATA_DIRS in test so that doesn't happen
 
+        // TODO: Set up XDG DATA DIRS in a test to test the location being correct
         return process()
             .var("XDG_DATA_DIRS")
-            // If var is found
             .map(|var| {
                 var.split(':')
                     .map(PathBuf::from)
@@ -274,14 +278,15 @@ impl UnixShell for Fish {
 
     fn rcfiles(&self) -> Vec<PathBuf> {
         // As per https://fishshell.com/docs/current/language.html#configuration
-        // Vendor config files should be written to `/fish/vendor_config.d/`
+        // Vendor config files should be written to `/usr/share/fish/vendor_config.d/`
         // if that does not exist then it should be written to `/usr/share/fish/vendor_conf.d/`
         // otherwise it should be written to `$HOME/.config/fish/conf.d/ as per discussions in github issue #478
         [
-            #[cfg(test)] // Write to test location so we don't pollute during testing.
-            utils::home_dir().map(|home| home.join(".config/fish/conf.d/")),
-            Self::get_XDG_DATA_DIRS(),
+            // #[cfg(test)] // Write to test location so we don't pollute during testing.
+            // utils::home_dir().map(|home| home.join(".config/fish/conf.d/")),
+            Self::get_vendor_config_from_XDG_DATA_DIRS(),
             Some(PathBuf::from("/usr/share/fish/vendor_conf.d/")),
+            Some(PathBuf::from("/usr/local/share/fish/vendor_conf.d/")),
             utils::home_dir().map(|home| home.join(".config/fish/conf.d/")),
         ]
         .iter_mut()
@@ -291,17 +296,16 @@ impl UnixShell for Fish {
     }
 
     fn update_rcs(&self) -> Vec<PathBuf> {
+        // TODO: Change rcfiles to just read parent dirs
         self.rcfiles()
             .into_iter()
             .filter(|rc| {
-                rc.metadata() // Returns error if path doesnt exist so separate check is not needed
-                    .map_or(false, |metadata| {
-                        dbg!(rc);
-                        dbg!(metadata.permissions());
-                        dbg!(metadata.permissions().readonly());
-                        dbg!();
-                        !metadata.permissions().readonly()
-                    })
+                // We only want to check if the directory exists as in fish we create a new file for every applications env
+                rc.parent().map_or(false, |parent| {
+                    parent
+                        .metadata() // Returns error if path doesn't exist so separate check is not needed
+                        .map_or(false, |metadata| !metadata.permissions().readonly())
+                })
             })
             .collect()
     }
@@ -331,8 +335,8 @@ impl UnixShell for Fish {
 
     fn env_script(&self) -> ShellScript {
         ShellScript {
-            name: "env",
-            content: include_str!("env.sh"),
+            name: "env.fish",
+            content: include_str!("env.fish"),
         }
     }
 
@@ -346,13 +350,14 @@ impl UnixShell for Fish {
 }
 
 pub(crate) fn legacy_paths() -> impl Iterator<Item = PathBuf> {
-    let zprofiles = Zsh::zdotdir()
+    let z_profiles = Zsh::zdotdir()
         .into_iter()
         .chain(utils::home_dir())
         .map(|d| d.join(".zprofile"));
-    let profiles = [".bash_profile", ".profile"]
+
+    let bash_profiles = [".bash_profile", ".profile"]
         .iter()
         .filter_map(|rc| utils::home_dir().map(|d| d.join(rc)));
 
-    profiles.chain(zprofiles)
+    bash_profiles.chain(z_profiles)
 }
diff --git a/tests/cli-paths.rs b/tests/cli-paths.rs
@@ -59,18 +59,43 @@ export PATH="$HOME/apple/bin"
             assert!(cmd.output().unwrap().status.success());
             let mut rcs = files.iter();
             let env = rcs.next().unwrap();
-            let envfile = fs::read_to_string(&env).unwrap();
+            let envfile = fs::read_to_string(env).unwrap();
             let (_, envfile_export) = envfile.split_at(envfile.find("export PATH").unwrap_or(0));
             assert_eq!(&envfile_export[..DEFAULT_EXPORT.len()], DEFAULT_EXPORT);
 
             for rc in rcs {
                 let expected = source("$HOME/.cargo", POSIX_SH);
-                let new_profile = fs::read_to_string(&rc).unwrap();
+                let new_profile = fs::read_to_string(rc).unwrap();
                 assert_eq!(new_profile, expected);
             }
         });
     }
 
+    #[test]
+    fn install_creates_necessary_fish_scripts_new() {
+        clitools::setup(Scenario::Empty, &|config| {
+            let mut cmd = clitools::cmd(config, "rustup-init", &INIT_NONE[1..]);
+            let rcs: Vec<PathBuf> = [".cargo/env.fish", ".config/fish/config.fish"]
+                .iter()
+                .map(|file| config.homedir.join(file))
+                .collect();
+            for file in &rcs {
+                assert!(!file.exists());
+            }
+            cmd.env_remove("CARGO_HOME");
+            cmd.env("SHELL", "fish");
+            cmd.env("XDG_DATA_DIRS", ""); // Overwrite XDG as host shouldn't be affected by the test.
+            assert!(cmd.output().unwrap().status.success());
+            let expected = include_str!("../src/cli/self_update/env.fish")
+                .replace("{cargo_bin}", "$HOME/.cargo/bin");
+
+            assert!(rcs
+                .iter()
+                .filter_map(|rc| fs::read_to_string(rc).ok()) // Read contents of files that exist
+                .any(|rc_content| rc_content.contains(&expected)));
+        })
+    }
+
     #[test]
     fn install_updates_bash_rcs() {
         clitools::setup(Scenario::Empty, &|config| {
@@ -86,7 +111,7 @@ export PATH="$HOME/apple/bin"
 
             let expected = FAKE_RC.to_owned() + &source(config.cargodir.display(), POSIX_SH);
             for rc in &rcs {
-                let new_rc = fs::read_to_string(&rc).unwrap();
+                let new_rc = fs::read_to_string(rc).unwrap();
                 assert_eq!(new_rc, expected);
             }
         })
@@ -191,9 +216,9 @@ export PATH="$HOME/apple/bin"
 
                 expect_ok(config, &INIT_NONE);
 
-                let new1 = fs::read_to_string(&path1).unwrap();
+                let new1 = fs::read_to_string(path1).unwrap();
                 assert_eq!(new1, expected);
-                let new2 = fs::read_to_string(&path2).unwrap();
+                let new2 = fs::read_to_string(path2).unwrap();
                 assert_eq!(new2, expected);
             }
         });
@@ -221,7 +246,7 @@ export PATH="$HOME/apple/bin"
             expect_ok(config, &["rustup", "self", "uninstall", "-y"]);
 
             for rc in &rcs {
-                let new_rc = fs::read_to_string(&rc).unwrap();
+                let new_rc = fs::read_to_string(rc).unwrap();
                 assert_eq!(new_rc, FAKE_RC);
             }
         })
@@ -255,11 +280,11 @@ export PATH="$HOME/apple/bin"
             assert!(cmd.output().unwrap().status.success());
             let fixed_rc = FAKE_RC.to_owned() + &source("$HOME/.cargo", POSIX_SH);
             for rc in &rcs {
-                let new_rc = fs::read_to_string(&rc).unwrap();
+                let new_rc = fs::read_to_string(rc).unwrap();
                 assert_eq!(new_rc, fixed_rc);
             }
             for rc in &zprofiles {
-                let new_rc = fs::read_to_string(&rc).unwrap();
+                let new_rc = fs::read_to_string(rc).unwrap();
                 assert_eq!(new_rc, FAKE_RC);
             }
         })
@@ -291,14 +316,14 @@ export PATH="$HOME/apple/bin"
                 raw::write_file(rc, &old_rc).unwrap();
             }
 
-            let mut cmd = clitools::cmd(config, "rustup", &["self", "uninstall", "-y"]);
+            let mut cmd = clitools::cmd(config, "rustup", ["self", "uninstall", "-y"]);
             cmd.env("SHELL", "zsh");
             cmd.env("ZDOTDIR", zdotdir.path());
             cmd.env_remove("CARGO_HOME");
             assert!(cmd.output().unwrap().status.success());
 
             for rc in &rcs {
-                let new_rc = fs::read_to_string(&rc).unwrap();
+                let new_rc = fs::read_to_string(rc).unwrap();
                 // It's not ideal, but it's OK, if we leave whitespace.
                 assert_eq!(new_rc, FAKE_RC);
             }
@@ -324,7 +349,7 @@ export PATH="$HOME/apple/bin"
             let expected = format!("{}. \"$HOME/.cargo/env\"\n", FAKE_RC);
             assert_eq!(new_profile, expected);
 
-            let mut cmd = clitools::cmd(config, "rustup", &["self", "uninstall", "-y"]);
+            let mut cmd = clitools::cmd(config, "rustup", ["self", "uninstall", "-y"]);
             cmd.env_remove("CARGO_HOME");
             assert!(cmd.output().unwrap().status.success());
 
diff --git a/tests/mock/clitools.rs b/tests/mock/clitools.rs
@@ -99,6 +99,7 @@ pub fn setup(s: Scenario, f: &dyn Fn(&mut Config)) {
     env::remove_var("RUSTUP_TOOLCHAIN");
     env::remove_var("SHELL");
     env::remove_var("ZDOTDIR");
+    env::remove_var("XDG_DATA_DIRS");
     // clap does it's own terminal colour probing, and that isn't
     // trait-controllable, but it does honour the terminal. To avoid testing
     // claps code, lie about whatever terminal this process was started under.

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+contains {cargo_bin} $fish_user_paths; or set -Ua fish_user_paths {cargo_bin}`