refactor(upgrade): add unit tests for lsp upgrade check (#21244)

1 files changed, 310 insertions, 105 deletions

diff --git a/cli/tools/upgrade.rs b/cli/tools/upgrade.rs
index dccde44e2..cd886fbd7 100644
--- a/cli/tools/upgrade.rs
+++ b/cli/tools/upgrade.rs
@@ -12,11 +12,10 @@ use crate::util::progress_bar::ProgressBarStyle;
 use crate::util::time;
 use crate::version;
 
+use async_trait::async_trait;
 use deno_core::anyhow::bail;
 use deno_core::anyhow::Context;
 use deno_core::error::AnyError;
-use deno_core::futures::future::BoxFuture;
-use deno_core::futures::FutureExt;
 use deno_core::unsync::spawn;
 use deno_semver::Version;
 use once_cell::sync::Lazy;
@@ -43,9 +42,7 @@ const UPGRADE_CHECK_FETCH_DELAY: Duration = Duration::from_millis(500);
 
 /// Environment necessary for doing the update checker.
 /// An alternate trait implementation can be provided for testing purposes.
-trait UpdateCheckerEnvironment: Clone + Send + Sync {
-  fn latest_version(&self) -> BoxFuture<'static, Result<String, AnyError>>;
-  fn current_version(&self) -> Cow<str>;
+trait UpdateCheckerEnvironment: Clone {
   fn read_check_file(&self) -> String;
   fn write_check_file(&self, text: &str);
   fn current_time(&self) -> chrono::DateTime<chrono::Utc>;
@@ -53,15 +50,13 @@ trait UpdateCheckerEnvironment: Clone + Send + Sync {
 
 #[derive(Clone)]
 struct RealUpdateCheckerEnvironment {
-  http_client: Arc<HttpClient>,
   cache_file_path: PathBuf,
   current_time: chrono::DateTime<chrono::Utc>,
 }
 
 impl RealUpdateCheckerEnvironment {
-  pub fn new(http_client: Arc<HttpClient>, cache_file_path: PathBuf) -> Self {
+  pub fn new(cache_file_path: PathBuf) -> Self {
     Self {
-      http_client,
       cache_file_path,
       // cache the current time
       current_time: time::utc_now(),
@@ -70,22 +65,6 @@ impl RealUpdateCheckerEnvironment {
 }
 
 impl UpdateCheckerEnvironment for RealUpdateCheckerEnvironment {
-  fn latest_version(&self) -> BoxFuture<'static, Result<String, AnyError>> {
-    let http_client = self.http_client.clone();
-    async move {
-      if version::is_canary() {
-        get_latest_canary_version(&http_client, false).await
-      } else {
-        get_latest_release_version(&http_client, false).await
-      }
-    }
-    .boxed()
-  }
-
-  fn current_version(&self) -> Cow<str> {
-    Cow::Borrowed(version::release_version_or_canary_commit_hash())
-  }
-
   fn read_check_file(&self) -> String {
     std::fs::read_to_string(&self.cache_file_path).unwrap_or_default()
   }
@@ -99,15 +78,82 @@ impl UpdateCheckerEnvironment for RealUpdateCheckerEnvironment {
   }
 }
 
-struct UpdateChecker<TEnvironment: UpdateCheckerEnvironment> {
+#[derive(Debug, Copy, Clone)]
+enum UpgradeCheckKind {
+  Execution,
+  Lsp,
+}
+
+#[async_trait(?Send)]
+trait VersionProvider: Clone {
+  fn is_canary(&self) -> bool;
+  async fn latest_version(&self) -> Result<String, AnyError>;
+  fn current_version(&self) -> Cow<str>;
+
+  fn release_kind(&self) -> UpgradeReleaseKind {
+    if self.is_canary() {
+      UpgradeReleaseKind::Canary
+    } else {
+      UpgradeReleaseKind::Stable
+    }
+  }
+}
+
+#[derive(Clone)]
+struct RealVersionProvider {
+  http_client: Arc<HttpClient>,
+  check_kind: UpgradeCheckKind,
+}
+
+impl RealVersionProvider {
+  pub fn new(
+    http_client: Arc<HttpClient>,
+    check_kind: UpgradeCheckKind,
+  ) -> Self {
+    Self {
+      http_client,
+      check_kind,
+    }
+  }
+}
+
+#[async_trait(?Send)]
+impl VersionProvider for RealVersionProvider {
+  fn is_canary(&self) -> bool {
+    version::is_canary()
+  }
+
+  async fn latest_version(&self) -> Result<String, AnyError> {
+    get_latest_version(&self.http_client, self.release_kind(), self.check_kind)
+      .await
+  }
+
+  fn current_version(&self) -> Cow<str> {
+    Cow::Borrowed(version::release_version_or_canary_commit_hash())
+  }
+}
+
+struct UpdateChecker<
+  TEnvironment: UpdateCheckerEnvironment,
+  TVersionProvider: VersionProvider,
+> {
   env: TEnvironment,
+  version_provider: TVersionProvider,
   maybe_file: Option<CheckVersionFile>,
 }
 
-impl<TEnvironment: UpdateCheckerEnvironment> UpdateChecker<TEnvironment> {
-  pub fn new(env: TEnvironment) -> Self {
+impl<
+    TEnvironment: UpdateCheckerEnvironment,
+    TVersionProvider: VersionProvider,
+  > UpdateChecker<TEnvironment, TVersionProvider>
+{
+  pub fn new(env: TEnvironment, version_provider: TVersionProvider) -> Self {
     let maybe_file = CheckVersionFile::parse(env.read_check_file());
-    Self { env, maybe_file }
+    Self {
+      env,
+      version_provider,
+      maybe_file,
+    }
   }
 
   pub fn should_check_for_new_version(&self) -> bool {
@@ -131,14 +177,15 @@ impl<TEnvironment: UpdateCheckerEnvironment> UpdateChecker<TEnvironment> {
     // - We already check for a new version today
     // - The user have probably upgraded today
     // So we should not prompt and wait for tomorrow for the latest version to be updated again
-    if file.current_version != self.env.current_version() {
+    let current_version = self.version_provider.current_version();
+    if file.current_version != current_version {
       return None;
     }
-    if file.latest_version == self.env.current_version() {
+    if file.latest_version == current_version {
       return None;
     }
 
-    if let Ok(current) = Version::parse_standard(&self.env.current_version()) {
+    if let Ok(current) = Version::parse_standard(&current_version) {
       if let Ok(latest) = Version::parse_standard(&file.latest_version) {
         if current >= latest {
           return None;
@@ -201,18 +248,21 @@ pub fn check_for_upgrades(
     return;
   }
 
-  let env = RealUpdateCheckerEnvironment::new(http_client, cache_file_path);
-  let update_checker = UpdateChecker::new(env);
+  let env = RealUpdateCheckerEnvironment::new(cache_file_path);
+  let version_provider =
+    RealVersionProvider::new(http_client, UpgradeCheckKind::Execution);
+  let update_checker = UpdateChecker::new(env, version_provider);
 
   if update_checker.should_check_for_new_version() {
     let env = update_checker.env.clone();
+    let version_provider = update_checker.version_provider.clone();
     // do this asynchronously on a separate task
     spawn(async move {
       // Sleep for a small amount of time to not unnecessarily impact startup
       // time.
       tokio::time::sleep(UPGRADE_CHECK_FETCH_DELAY).await;
 
-      fetch_and_store_latest_version(&env).await;
+      fetch_and_store_latest_version(&env, &version_provider).await;
     });
   }
 
@@ -246,39 +296,60 @@ pub fn check_for_upgrades(
   }
 }
 
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct LspVersionUpgradeInfo {
+  pub latest_version: String,
+  pub is_canary: bool,
+}
+
 pub async fn check_for_upgrades_for_lsp(
   http_client: Arc<HttpClient>,
-) -> Result<Option<(String, bool)>, AnyError> {
+) -> Result<Option<LspVersionUpgradeInfo>, AnyError> {
   if !upgrade_check_enabled() {
     return Ok(None);
   }
-  let is_canary = version::is_canary();
-  let latest_version;
-  let mut is_upgrade;
-  if is_canary {
-    latest_version = get_latest_canary_version(&http_client, true).await?;
-    is_upgrade = latest_version != version::GIT_COMMIT_HASH;
+
+  let version_provider =
+    RealVersionProvider::new(http_client, UpgradeCheckKind::Lsp);
+  check_for_upgrades_for_lsp_with_provider(&version_provider).await
+}
+
+async fn check_for_upgrades_for_lsp_with_provider(
+  version_provider: &impl VersionProvider,
+) -> Result<Option<LspVersionUpgradeInfo>, AnyError> {
+  let latest_version = version_provider.latest_version().await?;
+  let current_version = version_provider.current_version();
+  if current_version == latest_version {
+    Ok(None) // nothing to upgrade
+  } else if version_provider.is_canary() {
+    Ok(Some(LspVersionUpgradeInfo {
+      latest_version,
+      is_canary: true,
+    }))
   } else {
-    latest_version = get_latest_release_version(&http_client, true).await?;
-    is_upgrade = true;
-    if let Ok(current) = Version::parse_standard(version::deno()) {
+    if let Ok(current) = Version::parse_standard(&current_version) {
       if let Ok(latest) = Version::parse_standard(&latest_version) {
         if current >= latest {
-          is_upgrade = false;
+          return Ok(None); // nothing to upgrade
         }
       }
     }
-  };
-  Ok(is_upgrade.then_some((latest_version, is_canary)))
+    Ok(Some(LspVersionUpgradeInfo {
+      latest_version,
+      is_canary: false,
+    }))
+  }
 }
 
 async fn fetch_and_store_latest_version<
   TEnvironment: UpdateCheckerEnvironment,
+  TVersionProvider: VersionProvider,
 >(
   env: &TEnvironment,
+  version_provider: &TVersionProvider,
 ) {
   // Fetch latest version or commit hash from server.
-  let latest_version = match env.latest_version().await {
+  let latest_version = match version_provider.latest_version().await {
     Ok(latest_version) => latest_version,
     Err(_) => return,
   };
@@ -290,7 +361,7 @@ async fn fetch_and_store_latest_version<
         .current_time()
         .sub(chrono::Duration::hours(UPGRADE_CHECK_INTERVAL + 1)),
       last_checked: env.current_time(),
-      current_version: env.current_version().to_string(),
+      current_version: version_provider.current_version().to_string(),
       latest_version,
     }
     .serialize(),
@@ -365,14 +436,18 @@ pub async fn upgrade(
       passed_version
     }
     None => {
-      let latest_version = if upgrade_flags.canary {
+      let release_kind = if upgrade_flags.canary {
         log::info!("Looking up latest canary version");
-        get_latest_canary_version(client, false).await?
+        UpgradeReleaseKind::Canary
       } else {
         log::info!("Looking up latest version");
-        get_latest_release_version(client, false).await?
+        UpgradeReleaseKind::Stable
       };
 
+      let latest_version =
+        get_latest_version(client, release_kind, UpgradeCheckKind::Execution)
+          .await?;
+
       let current_is_most_recent = if upgrade_flags.canary {
         let latest_hash = &latest_version;
         crate::version::GIT_COMMIT_HASH == latest_hash
@@ -479,30 +554,46 @@ pub async fn upgrade(
   Ok(())
 }
 
-async fn get_latest_release_version(
+#[derive(Debug, Clone, Copy)]
+enum UpgradeReleaseKind {
+  Stable,
+  Canary,
+}
+
+async fn get_latest_version(
   client: &HttpClient,
-  for_lsp: bool,
+  release_kind: UpgradeReleaseKind,
+  check_kind: UpgradeCheckKind,
 ) -> Result<String, AnyError> {
-  let mut url = "https://dl.deno.land/release-latest.txt".to_string();
-  if for_lsp {
-    url.push_str("?lsp");
-  }
+  let url = get_url(release_kind, check_kind);
   let text = client.download_text(url).await?;
-  let version = text.trim().to_string();
-  Ok(version.replace('v', ""))
+  Ok(normalize_version_from_server(release_kind, &text))
 }
 
-async fn get_latest_canary_version(
-  client: &HttpClient,
-  for_lsp: bool,
-) -> Result<String, AnyError> {
-  let mut url = "https://dl.deno.land/canary-latest.txt".to_string();
-  if for_lsp {
-    url.push_str("?lsp");
+fn normalize_version_from_server(
+  release_kind: UpgradeReleaseKind,
+  text: &str,
+) -> String {
+  let text = text.trim();
+  match release_kind {
+    UpgradeReleaseKind::Stable => text.trim_start_matches('v').to_string(),
+    UpgradeReleaseKind::Canary => text.to_string(),
   }
-  let text = client.download_text(url).await?;
-  let version = text.trim().to_string();
-  Ok(version)
+}
+
+fn get_url(
+  release_kind: UpgradeReleaseKind,
+  check_kind: UpgradeCheckKind,
+) -> String {
+  let file_name = match release_kind {
+    UpgradeReleaseKind::Stable => "release-latest.txt",
+    UpgradeReleaseKind::Canary => "canary-latest.txt",
+  };
+  let query_param = match check_kind {
+    UpgradeCheckKind::Execution => "",
+    UpgradeCheckKind::Lsp => "?lsp",
+  };
+  format!("https://dl.deno.land/{}{}", file_name, query_param)
 }
 
 async fn download_package(
@@ -676,9 +767,8 @@ impl CheckVersionFile {
 
 #[cfg(test)]
 mod test {
-  use std::sync::Arc;
-
-  use deno_core::parking_lot::Mutex;
+  use std::cell::RefCell;
+  use std::rc::Rc;
 
   use super::*;
 
@@ -733,10 +823,11 @@ mod test {
 
   #[derive(Clone)]
   struct TestUpdateCheckerEnvironment {
-    file_text: Arc<Mutex<String>>,
-    current_version: Arc<Mutex<String>>,
-    latest_version: Arc<Mutex<Result<String, String>>>,
-    time: Arc<Mutex<chrono::DateTime<chrono::Utc>>>,
+    file_text: Rc<RefCell<String>>,
+    is_canary: Rc<RefCell<bool>>,
+    current_version: Rc<RefCell<String>>,
+    latest_version: Rc<RefCell<Result<String, String>>>,
+    time: Rc<RefCell<chrono::DateTime<chrono::Utc>>>,
   }
 
   impl TestUpdateCheckerEnvironment {
@@ -744,53 +835,61 @@ mod test {
       Self {
         file_text: Default::default(),
         current_version: Default::default(),
-        latest_version: Arc::new(Mutex::new(Ok("".to_string()))),
-        time: Arc::new(Mutex::new(crate::util::time::utc_now())),
+        is_canary: Default::default(),
+        latest_version: Rc::new(RefCell::new(Ok("".to_string()))),
+        time: Rc::new(RefCell::new(crate::util::time::utc_now())),
       }
     }
 
     pub fn add_hours(&self, hours: i64) {
-      let mut time = self.time.lock();
+      let mut time = self.time.borrow_mut();
       *time = time
         .checked_add_signed(chrono::Duration::hours(hours))
         .unwrap();
     }
 
     pub fn set_file_text(&self, text: &str) {
-      *self.file_text.lock() = text.to_string();
+      *self.file_text.borrow_mut() = text.to_string();
     }
 
     pub fn set_current_version(&self, version: &str) {
-      *self.current_version.lock() = version.to_string();
+      *self.current_version.borrow_mut() = version.to_string();
     }
 
     pub fn set_latest_version(&self, version: &str) {
-      *self.latest_version.lock() = Ok(version.to_string());
+      *self.latest_version.borrow_mut() = Ok(version.to_string());
     }
 
     pub fn set_latest_version_err(&self, err: &str) {
-      *self.latest_version.lock() = Err(err.to_string());
+      *self.latest_version.borrow_mut() = Err(err.to_string());
+    }
+
+    pub fn set_is_canary(&self, is_canary: bool) {
+      *self.is_canary.borrow_mut() = is_canary;
     }
   }
 
-  impl UpdateCheckerEnvironment for TestUpdateCheckerEnvironment {
-    fn latest_version(&self) -> BoxFuture<'static, Result<String, AnyError>> {
-      let env = self.clone();
-      async move {
-        match env.latest_version.lock().clone() {
-          Ok(result) => Ok(result),
-          Err(err) => bail!("{}", err),
-        }
+  #[async_trait(?Send)]
+  impl VersionProvider for TestUpdateCheckerEnvironment {
+    fn is_canary(&self) -> bool {
+      *self.is_canary.borrow()
+    }
+
+    async fn latest_version(&self) -> Result<String, AnyError> {
+      match self.latest_version.borrow().clone() {
+        Ok(result) => Ok(result),
+        Err(err) => bail!("{}", err),
       }
-      .boxed()
     }
 
     fn current_version(&self) -> Cow<str> {
-      Cow::Owned(self.current_version.lock().clone())
+      Cow::Owned(self.current_version.borrow().clone())
     }
+  }
 
+  impl UpdateCheckerEnvironment for TestUpdateCheckerEnvironment {
     fn read_check_file(&self) -> String {
-      self.file_text.lock().clone()
+      self.file_text.borrow().clone()
     }
 
     fn write_check_file(&self, text: &str) {
@@ -798,7 +897,7 @@ mod test {
     }
 
     fn current_time(&self) -> chrono::DateTime<chrono::Utc> {
-      *self.time.lock()
+      *self.time.borrow()
     }
   }
 
@@ -807,17 +906,17 @@ mod test {
     let env = TestUpdateCheckerEnvironment::new();
     env.set_current_version("1.0.0");
     env.set_latest_version("1.1.0");
-    let checker = UpdateChecker::new(env.clone());
+    let checker = UpdateChecker::new(env.clone(), env.clone());
 
     // no version, so we should check, but not prompt
     assert!(checker.should_check_for_new_version());
     assert_eq!(checker.should_prompt(), None);
 
     // store the latest version
-    fetch_and_store_latest_version(&env).await;
+    fetch_and_store_latest_version(&env, &env).await;
 
     // reload
-    let checker = UpdateChecker::new(env.clone());
+    let checker = UpdateChecker::new(env.clone(), env.clone());
 
     // should not check for latest version because we just did
     assert!(!checker.should_check_for_new_version());
@@ -835,16 +934,16 @@ mod test {
     assert!(checker.should_check_for_new_version());
     assert_eq!(checker.should_prompt(), Some("1.1.0".to_string()));
 
-    fetch_and_store_latest_version(&env).await;
+    fetch_and_store_latest_version(&env, &env).await;
 
     // reload and store that we prompted
-    let checker = UpdateChecker::new(env.clone());
+    let checker = UpdateChecker::new(env.clone(), env.clone());
     assert!(!checker.should_check_for_new_version());
     assert_eq!(checker.should_prompt(), Some("1.2.0".to_string()));
     checker.store_prompted();
 
     // reload and it should now say not to prompt
-    let checker = UpdateChecker::new(env.clone());
+    let checker = UpdateChecker::new(env.clone(), env.clone());
     assert!(!checker.should_check_for_new_version());
     assert_eq!(checker.should_prompt(), None);
 
@@ -864,7 +963,7 @@ mod test {
     env.set_latest_version("1.3.0");
 
     // this will silently fail
-    fetch_and_store_latest_version(&env).await;
+    fetch_and_store_latest_version(&env, &env).await;
     assert!(checker.should_check_for_new_version());
     assert_eq!(checker.should_prompt(), None);
   }
@@ -884,7 +983,7 @@ mod test {
     env.write_check_file(&file_content);
     env.set_current_version("1.27.0");
     env.set_latest_version("1.26.2");
-    let checker = UpdateChecker::new(env);
+    let checker = UpdateChecker::new(env.clone(), env);
 
     // since currently running version is newer than latest available (eg. CDN
     // propagation might be delated) we should not prompt
@@ -906,7 +1005,113 @@ mod test {
     env.write_check_file(&file_content);
     // simulate an upgrade done to a canary version
     env.set_current_version("61fbfabe440f1cfffa7b8d17426ffdece4d430d0");
-    let checker = UpdateChecker::new(env);
+    let checker = UpdateChecker::new(env.clone(), env);
     assert_eq!(checker.should_prompt(), None);
   }
+
+  #[test]
+  fn test_get_url() {
+    assert_eq!(
+      get_url(UpgradeReleaseKind::Canary, UpgradeCheckKind::Execution),
+      "https://dl.deno.land/canary-latest.txt"
+    );
+    assert_eq!(
+      get_url(UpgradeReleaseKind::Canary, UpgradeCheckKind::Lsp),
+      "https://dl.deno.land/canary-latest.txt?lsp"
+    );
+    assert_eq!(
+      get_url(UpgradeReleaseKind::Stable, UpgradeCheckKind::Execution),
+      "https://dl.deno.land/release-latest.txt"
+    );
+    assert_eq!(
+      get_url(UpgradeReleaseKind::Stable, UpgradeCheckKind::Lsp),
+      "https://dl.deno.land/release-latest.txt?lsp"
+    );
+  }
+
+  #[test]
+  fn test_normalize_version_server() {
+    // should strip v for stable
+    assert_eq!(
+      normalize_version_from_server(UpgradeReleaseKind::Stable, "v1.0.0"),
+      "1.0.0"
+    );
+    // should not replace v after start
+    assert_eq!(
+      normalize_version_from_server(
+        UpgradeReleaseKind::Stable,
+        "  v1.0.0-test-v\n\n  "
+      ),
+      "1.0.0-test-v"
+    );
+    // should not strip v for canary
+    assert_eq!(
+      normalize_version_from_server(
+        UpgradeReleaseKind::Canary,
+        "  v1452345asdf   \n\n   "
+      ),
+      "v1452345asdf"
+    );
+  }
+
+  #[tokio::test]
+  async fn test_upgrades_lsp() {
+    let env = TestUpdateCheckerEnvironment::new();
+    env.set_current_version("1.0.0");
+    env.set_latest_version("2.0.0");
+
+    // greater
+    {
+      let maybe_info = check_for_upgrades_for_lsp_with_provider(&env)
+        .await
+        .unwrap();
+      assert_eq!(
+        maybe_info,
+        Some(LspVersionUpgradeInfo {
+          latest_version: "2.0.0".to_string(),
+          is_canary: false,
+        })
+      );
+    }
+    // equal
+    {
+      env.set_latest_version("1.0.0");
+      let maybe_info = check_for_upgrades_for_lsp_with_provider(&env)
+        .await
+        .unwrap();
+      assert_eq!(maybe_info, None);
+    }
+    // less
+    {
+      env.set_latest_version("0.9.0");
+      let maybe_info = check_for_upgrades_for_lsp_with_provider(&env)
+        .await
+        .unwrap();
+      assert_eq!(maybe_info, None);
+    }
+    // canary equal
+    {
+      env.set_current_version("123");
+      env.set_latest_version("123");
+      env.set_is_canary(true);
+      let maybe_info = check_for_upgrades_for_lsp_with_provider(&env)
+        .await
+        .unwrap();
+      assert_eq!(maybe_info, None);
+    }
+    // canary different
+    {
+      env.set_latest_version("1234");
+      let maybe_info = check_for_upgrades_for_lsp_with_provider(&env)
+        .await
+        .unwrap();
+      assert_eq!(
+        maybe_info,
+        Some(LspVersionUpgradeInfo {
+          latest_version: "1234".to_string(),
+          is_canary: true,
+        })
+      );
+    }
+  }
 }