From 8ea79b0fe815d1800b3a2ceaf46fe186c29791ec Mon Sep 17 00:00:00 2001 From: Markus Pettersson Date: Mon, 16 Dec 2024 17:35:41 +0100 Subject: [PATCH 1/4] Split up `Service` error variants --- test/test-rpc/src/lib.rs | 8 +++- test/test-runner/src/app.rs | 2 +- test/test-runner/src/sys.rs | 91 ++++++++++++++++++++----------------- 3 files changed, 57 insertions(+), 44 deletions(-) diff --git a/test/test-rpc/src/lib.rs b/test/test-rpc/src/lib.rs index 7c10f0df5317..a23eb8426645 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 6c6ed0b369be..82f332b81c83 100644 --- a/test/test-runner/src/app.rs +++ b/test/test-runner/src/app.rs @@ -9,7 +9,7 @@ pub async fn version() -> Result { .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 e1060124d7f4..56edcc75d321 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::(&[]) - .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) -> 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) -> 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) -> 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 { #[cfg(target_os = "macos")] pub async fn set_daemon_environment(env: HashMap) -> 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) -> 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) -> 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, test_rp #[cfg(target_os = "macos")] pub async fn get_daemon_environment() -> Result, 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::(parsed_plist) }) @@ -675,14 +682,14 @@ pub async fn get_daemon_environment() -> Result, 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); From 4bfed4546cb572d5d7735e4627f661bcea723b27 Mon Sep 17 00:00:00 2001 From: Markus Pettersson Date: Tue, 17 Dec 2024 07:56:19 +0100 Subject: [PATCH 2/4] Add more debug logging to test_upgrade_app --- test/test-manager/src/tests/install.rs | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/test/test-manager/src/tests/install.rs b/test/test-manager/src/tests/install.rs index ee4062ae8981..9283e4e21180 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 { From b86f4f664ebb67718f30fe90009c1f4246a9ebe1 Mon Sep 17 00:00:00 2001 From: Markus Pettersson Date: Wed, 18 Dec 2024 16:55:38 +0100 Subject: [PATCH 3/4] Swap `if + bail!` for `ensure!` --- test/test-manager/src/tests/install.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/test-manager/src/tests/install.rs b/test/test-manager/src/tests/install.rs index 9283e4e21180..936f07d65d79 100644 --- a/test/test-manager/src/tests/install.rs +++ b/test/test-manager/src/tests/install.rs @@ -102,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?; From 5f0cca3041157a2c845e8e6dcb454e3dd2b24ed5 Mon Sep 17 00:00:00 2001 From: Markus Pettersson Date: Tue, 7 Jan 2025 10:20:36 +0100 Subject: [PATCH 4/4] Use absolute path to refer to `mullvad` binary in `test_upgrade_app` --- test/test-runner/src/app.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/test/test-runner/src/app.rs b/test/test-runner/src/app.rs index 82f332b81c83..009638c31515 100644 --- a/test/test-runner/src/app.rs +++ b/test/test-runner/src/app.rs @@ -5,7 +5,14 @@ use test_rpc::{AppTrace, Error}; /// Get the installed app version string pub async fn version() -> Result { - 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