More PR feedback. Refactoring tests

cli/internal/packagemanager/berry.go

@@ -51,25 +51,6 @@ var nodejsBerry = PackageManager{
 		return true, nil
 	},
 
-	// Versions newer than 2.0 are berry, and before that we simply call them yarn.
-	Matches: func(manager string, version string) (bool, error) {
-		if manager != "yarn" {
-			return false, nil
-		}
-
-		v, err := semver.NewVersion(version)
-		if err != nil {
-			return false, fmt.Errorf("could not parse yarn version: %w", err)
-		}
-		// -0 allows pre-releases versions to be considered valid
-		c, err := semver.NewConstraint(">=2.0.0-0")
-		if err != nil {
-			return false, fmt.Errorf("could not create constraint: %w", err)
-		}
-
-		return c.Check(v), nil
-	},
-
 	// Detect for berry needs to identify which version of yarn is running on the system.
 	// Further, berry can be configured in an incompatible way, so we check for compatibility here as well.
 	detect: func(projectDirectory turbopath.AbsoluteSystemPath, packageManager *PackageManager) (bool, error) {

crates/turborepo-lib/src/package_manager/mod.rs

@@ -245,73 +245,71 @@ impl PackageManager {
 
 #[cfg(test)]
 mod tests {
-    use std::{env, fs::File, io::Write, os, path::Path};
+    use std::{fs::File, path::Path};
 
     use tempfile::tempdir;
 
     use super::*;
-    use crate::{get_version, Args};
+    use crate::{get_version, package_manager::yarn::YARN_RC, Args};
 
     #[test]
     fn test_read_package_manager() -> Result<()> {
+        let repo_root = tempdir()?;
         let mut package_json = PackageJson::default();
+        let repo_root_path = AbsoluteSystemPathBuf::new(repo_root.path())?;
+
+        // Set up .yarnrc.yml file
+        let yarn_rc_path = repo_root.path().join(YARN_RC);
+        fs::write(&yarn_rc_path, "nodeLinker: node-modules")?;
+
         package_json.package_manager = Some("npm@8.19.4".to_string());
-        let package_manager = PackageManager::read_package_manager(&package_json)?;
+        let package_manager = PackageManager::read_package_manager(&repo_root_path, &package_json)?;
         assert_eq!(package_manager, Some(PackageManager::Npm));
 
         package_json.package_manager = Some("yarn@2.0.0".to_string());
-        let package_manager = PackageManager::read_package_manager(&package_json)?;
+        let package_manager = PackageManager::read_package_manager(&repo_root_path, &package_json)?;
         assert_eq!(package_manager, Some(PackageManager::Berry));
 
         package_json.package_manager = Some("yarn@1.9.0".to_string());
-        let package_manager = PackageManager::read_package_manager(&package_json)?;
+        let package_manager = PackageManager::read_package_manager(&repo_root_path, &package_json)?;
         assert_eq!(package_manager, Some(PackageManager::Yarn));
 
         package_json.package_manager = Some("pnpm@6.0.0".to_string());
-        let package_manager = PackageManager::read_package_manager(&package_json)?;
+        let package_manager = PackageManager::read_package_manager(&repo_root_path, &package_json)?;
         assert_eq!(package_manager, Some(PackageManager::Pnpm6));
 
         package_json.package_manager = Some("pnpm@7.2.0".to_string());
-        let package_manager = PackageManager::read_package_manager(&package_json)?;
+        let package_manager = PackageManager::read_package_manager(&repo_root_path, &package_json)?;
         assert_eq!(package_manager, Some(PackageManager::Pnpm));
 
         Ok(())
     }
 
     #[test]
-    fn test_detect_package_manager() -> Result<()> {
+    fn test_detect_multiple_package_managers() -> Result<()> {
         let repo_root = tempdir()?;
-        let mut base = CommandBase::new(
+        let base = CommandBase::new(
             Args::default(),
             repo_root.path().to_path_buf(),
             get_version(),
         )?;
 
-        let lockfiles = [
-            (PackageManager::Npm, "package-lock.json"),
-            (PackageManager::Pnpm, "pnpm-lock.yaml"),
-        ];
-
-        for (expected_package_manager, lockfile) in lockfiles {
-            let lockfile_path = repo_root.path().join(lockfile);
-            File::create(&lockfile_path)?;
-            let package_manager = PackageManager::detect_package_manager(&mut base)?;
-            assert_eq!(package_manager, expected_package_manager);
-            fs::remove_file(lockfile_path)?;
-        }
-
-        let yarn_lock_path = repo_root.path().join("yarn.lock");
-        File::create(&yarn_lock_path)?;
+        let package_lock_json_path = repo_root.path().join(npm::LOCKFILE);
+        File::create(&package_lock_json_path)?;
+        let pnpm_lock_path = repo_root.path().join(pnpm::LOCKFILE);
+        File::create(&pnpm_lock_path)?;
 
-        env::set_var("TEST_YARN_VERSION", "1.9.0");
-        let package_manager = PackageManager::detect_package_manager(&mut base)?;
-        assert_eq!(package_manager, PackageManager::Yarn);
+        let error = PackageManager::detect_package_manager(&base).unwrap_err();
+        assert_eq!(
+            error.to_string(),
+            "We detected multiple package managers in your repository: pnpm, npm. Please remove \
+             one of them."
+        );
 
-        env::set_var("TEST_YARN_VERSION", "2.0.1");
-        let package_manager = PackageManager::detect_package_manager(&mut base)?;
-        assert_eq!(package_manager, PackageManager::Berry);
+        fs::remove_file(&package_lock_json_path)?;
 
-        fs::remove_file(yarn_lock_path)?;
+        let package_manager = PackageManager::detect_package_manager(&base)?;
+        assert_eq!(package_manager, PackageManager::Pnpm);
 
         Ok(())
     }

crates/turborepo-lib/src/package_manager/npm.rs

@@ -3,6 +3,8 @@ use turbopath::{AbsoluteSystemPathBuf, RelativeSystemPathBuf};
 
 use crate::package_manager::PackageManager;
 
+pub const LOCKFILE: &str = "package-lock.json";
+
 pub struct NpmDetector<'a> {
     repo_root: &'a AbsoluteSystemPathBuf,
     found: bool,
@@ -28,7 +30,7 @@ impl<'a> Iterator for NpmDetector<'a> {
         self.found = true;
         let package_json = self
             .repo_root
-            .join_relative(RelativeSystemPathBuf::new("package-lock.json").unwrap());
+            .join_relative(RelativeSystemPathBuf::new(LOCKFILE).unwrap());
 
         if package_json.exists() {
             Some(Ok(PackageManager::Npm))
@@ -37,3 +39,31 @@ impl<'a> Iterator for NpmDetector<'a> {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use std::fs::File;
+
+    use anyhow::Result;
+    use tempfile::tempdir;
+
+    use super::LOCKFILE;
+    use crate::{commands::CommandBase, get_version, package_manager::PackageManager, Args};
+
+    #[test]
+    fn test_detect_npm() -> Result<()> {
+        let repo_root = tempdir()?;
+        let mut base = CommandBase::new(
+            Args::default(),
+            repo_root.path().to_path_buf(),
+            get_version(),
+        )?;
+
+        let lockfile_path = repo_root.path().join(LOCKFILE);
+        File::create(&lockfile_path)?;
+        let package_manager = PackageManager::detect_package_manager(&mut base)?;
+        assert_eq!(package_manager, PackageManager::Npm);
+
+        Ok(())
+    }
+}

crates/turborepo-lib/src/package_manager/pnpm.rs

@@ -4,6 +4,8 @@ use turbopath::{AbsoluteSystemPathBuf, RelativeSystemPathBuf};
 
 use crate::package_manager::PackageManager;
 
+pub const LOCKFILE: &str = "pnpm-lock.yaml";
+
 pub struct PnpmDetector<'a> {
     found: bool,
     repo_root: &'a AbsoluteSystemPathBuf,
@@ -38,8 +40,36 @@ impl<'a> Iterator for PnpmDetector<'a> {
 
         let pnpm_lockfile = self
             .repo_root
-            .join_relative(RelativeSystemPathBuf::new("pnpm-lock.yaml").unwrap());
+            .join_relative(RelativeSystemPathBuf::new(LOCKFILE).unwrap());
 
         pnpm_lockfile.exists().then(|| Ok(PackageManager::Pnpm))
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use std::fs::File;
+
+    use anyhow::Result;
+    use tempfile::tempdir;
+
+    use super::LOCKFILE;
+    use crate::{commands::CommandBase, get_version, package_manager::PackageManager, Args};
+
+    #[test]
+    fn test_detect_pnpm() -> Result<()> {
+        let repo_root = tempdir()?;
+        let mut base = CommandBase::new(
+            Args::default(),
+            repo_root.path().to_path_buf(),
+            get_version(),
+        )?;
+
+        let lockfile_path = repo_root.path().join(LOCKFILE);
+        File::create(&lockfile_path)?;
+        let package_manager = PackageManager::detect_package_manager(&mut base)?;
+        assert_eq!(package_manager, PackageManager::Pnpm);
+
+        Ok(())
+    }
+}

crates/turborepo-lib/src/package_manager/yarn.rs

@@ -13,26 +13,35 @@ struct YarnRC {
     node_linker: Option<String>,
 }
 
+pub const LOCKFILE: &str = "yarn.lock";
+pub const YARN_RC: &str = ".yarnrc.yml";
+
 pub struct YarnDetector<'a> {
     repo_root: &'a AbsoluteSystemPathBuf,
+    // For testing purposes
+    version_override: Option<Version>,
     found: bool,
 }
 
 impl<'a> YarnDetector<'a> {
     pub fn new(repo_root: &'a AbsoluteSystemPathBuf) -> Self {
         Self {
             repo_root,
+            version_override: None,
             found: false,
         }
     }
 
     #[cfg(test)]
-    fn get_yarn_version(&self) -> Result<Version> {
-        Ok(std::env::var("TEST_YARN_VERSION")?.parse()?)
+    fn set_version_override(&mut self, version: Version) {
+        self.version_override = Some(version);
     }
 
-    #[cfg(not(test))]
     fn get_yarn_version(&self) -> Result<Version> {
+        if let Some(version) = &self.version_override {
+            return Ok(version.clone());
+        }
+
         let output = Command::new("yarn")
             .arg("--version")
             .current_dir(&self.repo_root)
@@ -42,7 +51,7 @@ impl<'a> YarnDetector<'a> {
     }
 
     fn is_nm_linker(repo_root: &AbsoluteSystemPathBuf) -> Result<bool> {
-        let yarnrc_path = repo_root.join_relative(RelativeSystemPathBuf::new("yarnrc.yml")?);
+        let yarnrc_path = repo_root.join_relative(RelativeSystemPathBuf::new(YARN_RC)?);
         let yarnrc = File::open(yarnrc_path)?;
         let yarnrc: YarnRC = serde_yaml::from_reader(&yarnrc)?;
         Ok(yarnrc.node_linker == Some("node-modules".to_string()))
@@ -81,7 +90,7 @@ impl<'a> Iterator for YarnDetector<'a> {
 
         let yarn_lockfile = self
             .repo_root
-            .join_relative(RelativeSystemPathBuf::new("yarn.lock").unwrap());
+            .join_relative(RelativeSystemPathBuf::new(LOCKFILE).unwrap());
 
         if yarn_lockfile.exists() {
             Some(
@@ -93,3 +102,52 @@ impl<'a> Iterator for YarnDetector<'a> {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use std::{fs, fs::File};
+
+    use anyhow::Result;
+    use tempfile::tempdir;
+    use turbopath::AbsoluteSystemPathBuf;
+
+    use super::LOCKFILE;
+    use crate::{
+        commands::CommandBase,
+        get_version,
+        package_manager::{
+            yarn::{YarnDetector, YARN_RC},
+            PackageManager,
+        },
+        Args,
+    };
+
+    #[test]
+    fn test_detect_yarn() -> Result<()> {
+        let repo_root = tempdir()?;
+        let base = CommandBase::new(
+            Args::default(),
+            repo_root.path().to_path_buf(),
+            get_version(),
+        )?;
+
+        let yarn_lock_path = repo_root.path().join(LOCKFILE);
+        File::create(&yarn_lock_path)?;
+
+        let yarn_rc_path = repo_root.path().join(YARN_RC);
+        fs::write(&yarn_rc_path, "nodeLinker: node-modules")?;
+
+        let absolute_repo_root = AbsoluteSystemPathBuf::new(base.repo_root)?;
+        let mut detector = YarnDetector::new(&absolute_repo_root);
+        detector.set_version_override("1.22.10".parse()?);
+        let package_manager = detector.next().unwrap()?;
+        assert_eq!(package_manager, PackageManager::Yarn);
+
+        let mut detector = YarnDetector::new(&absolute_repo_root);
+        detector.set_version_override("2.22.10".parse()?);
+        let package_manager = detector.next().unwrap()?;
+        assert_eq!(package_manager, PackageManager::Berry);
+
+        Ok(())
+    }
+}