Merge branch 'test_upgrade_app-fails-on-windows-des-1565'

4 files changed, 77 insertions, 58 deletions

diff --git a/test/test-manager/src/tests/install.rs b/test/test-manager/src/tests/install.rs
index ee4062ae89..936f07d65d 100644
--- a/test/test-manager/src/tests/install.rs
+++ b/test/test-manager/src/tests/install.rs
@@ -28,16 +28,14 @@ pub async fn test_upgrade_app(
     _mullvad_client: MullvadClientArgument,
 ) -> anyhow::Result<()> {
     // Install the older version of the app and verify that it is running.
-    install_app(
-        &rpc,
-        TEST_CONFIG
-            .app_package_to_upgrade_from_filename
-            .as_ref()
-            .unwrap(),
-        &ctx.rpc_provider,
-    )
-    .await
-    .context("Failed to install previous app version")?;
+    let old_version = TEST_CONFIG
+        .app_package_to_upgrade_from_filename
+        .as_ref()
+        .context("Could not find previous app version")?;
+    log::debug!("Installing app version {old_version}");
+    install_app(&rpc, old_version, &ctx.rpc_provider)
+        .await
+        .context("Failed to install previous app version")?;
 
     // Verify that daemon is running
     if rpc.mullvad_daemon_get_status().await? != ServiceStatus::Running {
@@ -104,9 +102,10 @@ pub async fn test_upgrade_app(
     tokio::time::sleep(Duration::from_secs(3)).await;
 
     // verify that daemon is running
-    if rpc.mullvad_daemon_get_status().await? != ServiceStatus::Running {
-        bail!(Error::DaemonNotRunning);
-    }
+    ensure!(
+        rpc.mullvad_daemon_get_status().await? == ServiceStatus::Running,
+        Error::DaemonNotRunning
+    );
 
     // Verify that the correct version was installed
     let running_daemon_version = rpc.mullvad_daemon_version().await?;
diff --git a/test/test-rpc/src/lib.rs b/test/test-rpc/src/lib.rs
index 7c10f0df53..a23eb84266 100644
--- a/test/test-rpc/src/lib.rs
+++ b/test/test-rpc/src/lib.rs
@@ -47,8 +47,14 @@ pub enum Error {
     Ping(String),
     #[error("Failed to get or set registry value: {0}")]
     Registry(String),
+    #[error("Failed to start the service: {0}")]
+    ServiceStart(String),
+    #[error("Failed to stop the service: {0}")]
+    ServiceStop(String),
     #[error("Failed to change the service: {0}")]
-    Service(String),
+    ServiceChange(String),
+    #[error("Failed to find the service: {0}")]
+    ServiceNotFound(String),
     #[error("Could not read from or write to the file system: {0}")]
     FileSystem(String),
     #[error("Could not serialize or deserialize file: {0}")]
diff --git a/test/test-runner/src/app.rs b/test/test-runner/src/app.rs
index 6c6ed0b369..009638c315 100644
--- a/test/test-runner/src/app.rs
+++ b/test/test-runner/src/app.rs
@@ -5,11 +5,18 @@ use test_rpc::{AppTrace, Error};
 
 /// Get the installed app version string
 pub async fn version() -> Result<String, Error> {
-    let version = tokio::process::Command::new("mullvad")
+    // The `mullvad` binary is seemingly not in PATH on Windows after upgrading the app..
+    // So, as a workaround we use the absolute path instead.
+    const MULLVAD_CLI_BIN: &str = if cfg!(target_os = "windows") {
+        r"C:\Program Files\Mullvad VPN\resources\mullvad.exe"
+    } else {
+        "mullvad"
+    };
+    let version = tokio::process::Command::new(MULLVAD_CLI_BIN)
         .arg("--version")
         .output()
         .await
-        .map_err(|e| Error::Service(e.to_string()))?;
+        .map_err(|e| Error::ServiceNotFound(e.to_string()))?;
     let version = String::from_utf8(version.stdout).map_err(|err| Error::Other(err.to_string()))?;
     // HACK: The output from `mullvad --version` includes the `mullvad-cli` binary name followed by
     // the version string. Simply remove the leading noise and get at the version string.
diff --git a/test/test-runner/src/sys.rs b/test/test-runner/src/sys.rs
index e1060124d7..56edcc75d3 100644
--- a/test/test-runner/src/sys.rs
+++ b/test/test-runner/src/sys.rs
@@ -180,7 +180,7 @@ ExecStart=/usr/bin/mullvad-daemon --disable-stdout-timestamps {verbosity}"#
     if let Some(parent) = override_path.parent() {
         tokio::fs::create_dir_all(parent)
             .await
-            .map_err(|e| test_rpc::Error::Service(e.to_string()))?;
+            .map_err(|e| test_rpc::Error::ServiceChange(e.to_string()))?;
     }
 
     let mut file = tokio::fs::OpenOptions::new()
@@ -189,17 +189,17 @@ ExecStart=/usr/bin/mullvad-daemon --disable-stdout-timestamps {verbosity}"#
         .write(true)
         .open(override_path)
         .await
-        .map_err(|e| test_rpc::Error::Service(e.to_string()))?;
+        .map_err(|e| test_rpc::Error::ServiceChange(e.to_string()))?;
 
     file.write_all(systemd_service_file_content.as_bytes())
         .await
-        .map_err(|e| test_rpc::Error::Service(e.to_string()))?;
+        .map_err(|e| test_rpc::Error::ServiceChange(e.to_string()))?;
 
     tokio::process::Command::new("systemctl")
         .args(["daemon-reload"])
         .status()
         .await
-        .map_err(|e| test_rpc::Error::Service(e.to_string()))?;
+        .map_err(|e| test_rpc::Error::ServiceStart(e.to_string()))?;
 
     restart_app().await?;
     Ok(())
@@ -214,7 +214,7 @@ pub async fn restart_app() -> Result<(), test_rpc::Error> {
         .args(["restart", "mullvad-daemon"])
         .status()
         .await
-        .map_err(|e| test_rpc::Error::Service(e.to_string()))?;
+        .map_err(|e| test_rpc::Error::ServiceStart(e.to_string()))?;
     wait_for_service_state(ServiceState::Running).await?;
     Ok(())
 }
@@ -228,7 +228,7 @@ pub async fn stop_app() -> Result<(), test_rpc::Error> {
         .args(["stop", "mullvad-daemon"])
         .status()
         .await
-        .map_err(|e| test_rpc::Error::Service(e.to_string()))?;
+        .map_err(|e| test_rpc::Error::ServiceStop(e.to_string()))?;
     wait_for_service_state(ServiceState::Inactive).await?;
 
     Ok(())
@@ -243,7 +243,7 @@ pub async fn start_app() -> Result<(), test_rpc::Error> {
         .args(["start", "mullvad-daemon"])
         .status()
         .await
-        .map_err(|e| test_rpc::Error::Service(e.to_string()))?;
+        .map_err(|e| test_rpc::Error::ServiceStart(e.to_string()))?;
     wait_for_service_state(ServiceState::Running).await?;
     Ok(())
 }
@@ -267,7 +267,7 @@ pub async fn stop_app() -> Result<(), test_rpc::Error> {
         .args(["stop", "mullvadvpn"])
         .status()
         .await
-        .map_err(|e| test_rpc::Error::Service(e.to_string()))?;
+        .map_err(|e| test_rpc::Error::ServiceStop(e.to_string()))?;
     Ok(())
 }
 
@@ -280,7 +280,7 @@ pub async fn start_app() -> Result<(), test_rpc::Error> {
         .args(["start", "mullvadvpn"])
         .status()
         .await
-        .map_err(|e| test_rpc::Error::Service(e.to_string()))?;
+        .map_err(|e| test_rpc::Error::ServiceStart(e.to_string()))?;
     Ok(())
 }
 
@@ -325,7 +325,7 @@ pub async fn set_daemon_log_level(verbosity_level: Verbosity) -> Result<(), test
     };
 
     let manager = ServiceManager::local_computer(None::<&str>, ServiceManagerAccess::CONNECT)
-        .map_err(|e| test_rpc::Error::Service(e.to_string()))?;
+        .map_err(|e| test_rpc::Error::ServiceNotFound(e.to_string()))?;
     let service = manager
         .open_service(
             "mullvadvpn",
@@ -334,23 +334,23 @@ pub async fn set_daemon_log_level(verbosity_level: Verbosity) -> Result<(), test
                 | ServiceAccess::START
                 | ServiceAccess::STOP,
         )
-        .map_err(|e| test_rpc::Error::Service(e.to_string()))?;
+        .map_err(|e| test_rpc::Error::ServiceNotFound(e.to_string()))?;
 
     // Stop the service
     // TODO: Extract to separate function.
     service
         .stop()
-        .map_err(|e| test_rpc::Error::Service(e.to_string()))?;
+        .map_err(|e| test_rpc::Error::ServiceStop(e.to_string()))?;
     tokio::process::Command::new("net")
         .args(["stop", "mullvadvpn"])
         .status()
         .await
-        .map_err(|e| test_rpc::Error::Service(e.to_string()))?;
+        .map_err(|e| test_rpc::Error::ServiceStop(e.to_string()))?;
 
     // Get the current service configuration
     let config = service
         .query_config()
-        .map_err(|e| test_rpc::Error::Service(e.to_string()))?;
+        .map_err(|e| test_rpc::Error::ServiceNotFound(e.to_string()))?;
 
     let executable_path = "C:\\Program Files\\Mullvad VPN\\resources\\mullvad-daemon.exe";
     let launch_arguments = vec![
@@ -375,13 +375,13 @@ pub async fn set_daemon_log_level(verbosity_level: Verbosity) -> Result<(), test
     // Apply the updated configuration
     service
         .change_config(&updated_config)
-        .map_err(|e| test_rpc::Error::Service(e.to_string()))?;
+        .map_err(|e| test_rpc::Error::ServiceChange(e.to_string()))?;
 
     // Start the service
     // TODO: Extract to separate function.
     service
         .start::<String>(&[])
-        .map_err(|e| test_rpc::Error::Service(e.to_string()))?;
+        .map_err(|e| test_rpc::Error::ServiceNotFound(e.to_string()))?;
 
     Ok(())
 }
@@ -445,19 +445,19 @@ pub async fn set_daemon_environment(env: HashMap<String, String>) -> Result<(),
         .map(|env_var| env_var.to_systemd_string())
     {
         writeln!(&mut override_content, "{env_var}")
-            .map_err(|err| test_rpc::Error::Service(err.to_string()))?;
+            .map_err(|err| test_rpc::Error::ServiceChange(err.to_string()))?;
     }
 
     let override_path = std::path::Path::new(SYSTEMD_OVERRIDE_FILE);
     if let Some(parent) = override_path.parent() {
         tokio::fs::create_dir_all(parent)
             .await
-            .map_err(|e| test_rpc::Error::Service(e.to_string()))?;
+            .map_err(|e| test_rpc::Error::ServiceChange(e.to_string()))?;
     }
 
     tokio::fs::write(override_path, override_content)
         .await
-        .map_err(|e| test_rpc::Error::Service(e.to_string()))?;
+        .map_err(|e| test_rpc::Error::ServiceChange(e.to_string()))?;
 
     if tokio::process::Command::new("systemctl")
         .args(["daemon-reload"])
@@ -467,7 +467,7 @@ pub async fn set_daemon_environment(env: HashMap<String, String>) -> Result<(),
         .success()
         .not()
     {
-        return Err(test_rpc::Error::Service(
+        return Err(test_rpc::Error::ServiceChange(
             "Daemon service could not be reloaded".to_owned(),
         ));
     };
@@ -480,7 +480,7 @@ pub async fn set_daemon_environment(env: HashMap<String, String>) -> Result<(),
         .success()
         .not()
     {
-        return Err(test_rpc::Error::Service(
+        return Err(test_rpc::Error::ServiceStart(
             "Daemon service could not be restarted".to_owned(),
         ));
     };
@@ -541,8 +541,9 @@ pub fn get_system_path_var() -> Result<String, test_rpc::Error> {
 #[cfg(target_os = "macos")]
 pub async fn set_daemon_environment(env: HashMap<String, String>) -> Result<(), test_rpc::Error> {
     tokio::task::spawn_blocking(|| {
-        let mut parsed_plist = plist::Value::from_file(PLIST_OVERRIDE_FILE)
-            .map_err(|error| test_rpc::Error::Service(format!("failed to parse plist: {error}")))?;
+        let mut parsed_plist = plist::Value::from_file(PLIST_OVERRIDE_FILE).map_err(|error| {
+            test_rpc::Error::ServiceNotFound(format!("failed to parse plist: {error}"))
+        })?;
 
         let mut vars = plist::Dictionary::new();
         for (k, v) in env {
@@ -551,25 +552,25 @@ pub async fn set_daemon_environment(env: HashMap<String, String>) -> Result<(),
                 .arg("setenv")
                 .args([&k, &v])
                 .status()
-                .map_err(|e| test_rpc::Error::Service(e.to_string()))?;
+                .map_err(|e| test_rpc::Error::ServiceChange(e.to_string()))?;
             vars.insert(k, plist::Value::String(v));
         }
 
         // Add permanent env var
         parsed_plist
             .as_dictionary_mut()
-            .ok_or_else(|| test_rpc::Error::Service("plist missing dict".to_owned()))?
+            .ok_or_else(|| test_rpc::Error::ServiceChange("plist missing dict".to_owned()))?
             .insert(
                 "EnvironmentVariables".to_owned(),
                 plist::Value::Dictionary(vars),
             );
 
         let daemon_plist = std::fs::File::create(PLIST_OVERRIDE_FILE)
-            .map_err(|e| test_rpc::Error::Service(format!("failed to open plist: {e}")))?;
+            .map_err(|e| test_rpc::Error::ServiceChange(format!("failed to open plist: {e}")))?;
 
         parsed_plist
             .to_writer_xml(daemon_plist)
-            .map_err(|e| test_rpc::Error::Service(format!("failed to replace plist: {e}")))?;
+            .map_err(|e| test_rpc::Error::ServiceChange(format!("failed to replace plist: {e}")))?;
 
         Ok::<(), test_rpc::Error>(())
     })
@@ -586,15 +587,20 @@ pub async fn set_daemon_environment(env: HashMap<String, String>) -> Result<(),
 
 #[cfg(target_os = "macos")]
 async fn set_launch_daemon_state(on: bool) -> Result<(), test_rpc::Error> {
-    tokio::process::Command::new("launchctl")
-        .args([
-            if on { "load" } else { "unload" },
-            "-w",
-            PLIST_OVERRIDE_FILE,
-        ])
-        .status()
-        .await
-        .map_err(|e| test_rpc::Error::Service(e.to_string()))?;
+    let mut launchctl = tokio::process::Command::new("launchctl");
+    if on {
+        launchctl
+            .args(["load", "-w", PLIST_OVERRIDE_FILE])
+            .status()
+            .await
+            .map_err(|e| test_rpc::Error::ServiceStart(e.to_string()))?;
+    } else {
+        launchctl
+            .args(["unload", "-w", PLIST_OVERRIDE_FILE])
+            .status()
+            .await
+            .map_err(|e| test_rpc::Error::ServiceStop(e.to_string()))?;
+    }
     Ok(())
 }
 
@@ -665,8 +671,9 @@ pub async fn get_daemon_environment() -> Result<HashMap<String, String>, test_rp
 #[cfg(target_os = "macos")]
 pub async fn get_daemon_environment() -> Result<HashMap<String, String>, test_rpc::Error> {
     let plist = tokio::task::spawn_blocking(|| {
-        let parsed_plist = plist::Value::from_file(PLIST_OVERRIDE_FILE)
-            .map_err(|error| test_rpc::Error::Service(format!("failed to parse plist: {error}")))?;
+        let parsed_plist = plist::Value::from_file(PLIST_OVERRIDE_FILE).map_err(|error| {
+            test_rpc::Error::ServiceNotFound(format!("failed to parse plist: {error}"))
+        })?;
 
         Ok::<plist::Value, test_rpc::Error>(parsed_plist)
     })
@@ -675,14 +682,14 @@ pub async fn get_daemon_environment() -> Result<HashMap<String, String>, test_rp
 
     let plist_tree = plist
         .as_dictionary()
-        .ok_or_else(|| test_rpc::Error::Service("plist missing dict".to_owned()))?;
+        .ok_or_else(|| test_rpc::Error::ServiceNotFound("plist missing dict".to_owned()))?;
     let Some(env_vars) = plist_tree.get("EnvironmentVariables") else {
         // `EnvironmentVariables` does not exist in plist file, so there are no env variables to
         // parse.
         return Ok(HashMap::new());
     };
     let env_vars = env_vars.as_dictionary().ok_or_else(|| {
-        test_rpc::Error::Service("`EnvironmentVariables` is not a dict".to_owned())
+        test_rpc::Error::ServiceNotFound("`EnvironmentVariables` is not a dict".to_owned())
     })?;
 
     let env = env_vars
@@ -707,7 +714,7 @@ async fn wait_for_service_state(awaited_state: ServiceState) -> Result<(), test_
     loop {
         attempt += 1;
         if attempt > RETRY_ATTEMPTS {
-            return Err(test_rpc::Error::Service(String::from(
+            return Err(test_rpc::Error::ServiceStart(String::from(
                 "Awaiting new service state timed out",
             )));
         }
@@ -716,7 +723,7 @@ async fn wait_for_service_state(awaited_state: ServiceState) -> Result<(), test_
             .args(["status", "mullvad-daemon"])
             .output()
             .await
-            .map_err(|e| test_rpc::Error::Service(e.to_string()))?
+            .map_err(|e| test_rpc::Error::ServiceNotFound(e.to_string()))?
             .stdout;
         let output = String::from_utf8_lossy(&output);