Fix install environment bug

Author: Serock3

test/test-manager/src/tests/helpers.rs

@@ -86,7 +86,7 @@ pub async fn install_app(
     replace_openvpn_cert(rpc).await?;
 
     // Override env vars
-    ensure_daemon_environment(rpc).await?;
+    rpc.set_daemon_environment(get_app_env().await?).await?;
 
     // Wait for the relay list to be updated
     let mut mullvad_client = rpc_provider.new_client().await;
@@ -96,28 +96,6 @@ pub async fn install_app(
     Ok(mullvad_client)
 }
 
-/// Conditionally restart the running daemon
-///
-/// If the daemon was started with non-standard environment variables, subsequent tests may break
-/// due to assuming a default configuration. In that case, reset the environment variables and
-/// restart.
-pub async fn ensure_daemon_environment(rpc: &ServiceClient) -> Result<(), anyhow::Error> {
-    let current_env = rpc
-        .get_daemon_environment()
-        .await
-        .context("Failed to get daemon env variables")?;
-    let default_env = get_app_env()
-        .await
-        .context("Failed to get daemon default env variables")?;
-    if current_env != default_env {
-        log::debug!("Restarting daemon due changed environment variables. Values since last test {current_env:?}");
-        rpc.set_daemon_environment(default_env)
-            .await
-            .context("Failed to restart daemon")?;
-    };
-    Ok(())
-}
-
 pub async fn replace_openvpn_cert(rpc: &ServiceClient) -> Result<(), Error> {
     const SOURCE_CERT_FILENAME: &str = "openvpn.ca.crt";
     const DEST_CERT_FILENAME: &str = "ca.crt";

test/test-manager/src/tests/install.rs

@@ -6,6 +6,8 @@ use mullvad_types::{constraints::Constraint, relay_constraints};
 use test_macro::test_function;
 use test_rpc::{mullvad_daemon::ServiceStatus, ServiceClient};
 
+use crate::tests::helpers;
+
 use super::{
     config::TEST_CONFIG,
     helpers::{
@@ -86,10 +88,18 @@ pub async fn test_upgrade_app(
     let pinger = Pinger::start(rpc).await;
 
     // install new package
-    let mut mullvad_client = install_app(rpc, &TEST_CONFIG.app_package_filename, &ctx.rpc_provider)
-        .await
-        .context("Failed to install new app version")?;
 
+    log::debug!("Installing new app");
+    rpc.install_app(helpers::get_package_desc(&TEST_CONFIG.app_package_filename))
+        .await?;
+
+    // Give it some time to start
+    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);
+    }
     // Check if any traffic was observed
     //
     let guest_ip = pinger.guest_ip;
@@ -99,6 +109,10 @@ pub async fn test_upgrade_app(
         "observed unexpected packets from {guest_ip}"
     );
 
+    // NOTE: Need to create a new `mullvad_client` here after the restart
+    // otherwise we *probably* can't communicate with the daemon.
+    let mut mullvad_client = ctx.rpc_provider.new_client().await;
+
     // check if settings were (partially) preserved
     log::info!("Sanity checking settings");
 

test/test-manager/src/tests/mod.rs

@@ -20,7 +20,7 @@ use crate::{
 };
 use anyhow::Context;
 use config::TEST_CONFIG;
-use helpers::install_app;
+use helpers::{get_app_env, install_app};
 pub use install::test_upgrade_app;
 use mullvad_management_interface::MullvadProxyClient;
 pub use test_metadata::TestMetadata;
@@ -78,13 +78,13 @@ pub fn get_tests() -> Vec<&'static TestMetadata> {
     tests
 }
 
-/// Make sure the daemon is installed and logged in. and restore settings to the defaults.
+/// Make sure the daemon is installed and logged in and restore settings to the defaults.
 pub async fn prepare_daemon(
     rpc: &ServiceClient,
     rpc_provider: &RpcClientProvider,
 ) -> anyhow::Result<()> {
     // Check if daemon should be restarted
-    let mut mullvad_client = restart_daemon(rpc, rpc_provider)
+    let mut mullvad_client = ensure_daemon_version(rpc, rpc_provider)
         .await
         .context("Failed to restart daemon")?;
 
@@ -105,35 +105,69 @@ pub async fn prepare_daemon(
 /// Reset the daemons environment.
 ///
 /// Will and restart or reinstall it if necessary.
-async fn restart_daemon(
+async fn ensure_daemon_version(
     rpc: &ServiceClient,
     rpc_provider: &RpcClientProvider,
 ) -> anyhow::Result<MullvadProxyClient> {
     let mut mullvad_client = rpc_provider.new_client().await;
     let app_package_filename = &TEST_CONFIG.app_package_filename;
 
+    let mullvad_client = if correct_daemon_version_is_running(&mut mullvad_client).await {
+        ensure_daemon_environment(rpc)
+            .await
+            .context("Failed to reset daemon environment")?;
+        rpc_provider.new_client().await
+    } else {
+        // NOTE: Reinstalling the app resets the daemon environment
+        install_app(rpc, app_package_filename, rpc_provider)
+            .await
+            .with_context(|| format!("Failed to install app '{app_package_filename}'"))?
+    };
+    Ok(mullvad_client)
+}
+
+/// Conditionally restart the running daemon
+///
+/// If the daemon was started with non-standard environment variables, subsequent tests may break
+/// due to assuming a default configuration. In that case, reset the environment variables and
+/// restart.
+pub async fn ensure_daemon_environment(rpc: &ServiceClient) -> Result<(), anyhow::Error> {
+    let current_env = rpc
+        .get_daemon_environment()
+        .await
+        .context("Failed to get daemon env variables")?;
+    let default_env = get_app_env()
+        .await
+        .context("Failed to get daemon default env variables")?;
+    if current_env != default_env {
+        log::debug!("Restarting daemon due changed environment variables. Values since last test {current_env:?}");
+        rpc.set_daemon_environment(default_env)
+            .await
+            .context("Failed to restart daemon")?;
+    };
+    Ok(())
+}
+
+/// Reset the daemons environment.
+///
+/// Will and restart or reinstall it if necessary.
+async fn correct_daemon_version_is_running(mullvad_client: &mut MullvadProxyClient) -> bool {
+    let app_package_filename = &TEST_CONFIG.app_package_filename;
+    let expected_version = get_version_from_path(std::path::Path::new(app_package_filename))
+        .unwrap_or_else(|_| panic!("Invalid app version: {app_package_filename}"));
+
     use mullvad_management_interface::Error::*;
     match mullvad_client.get_current_version().await {
         // Failing to reach the daemon is a sign that it is not installed
         Err(Rpc(..)) => {
-            log::info!("Could not reach active daemon before test, (re)installing app");
-            // NOTE: Reinstalling the app resets the daemon environment
-            mullvad_client = install_app(rpc, app_package_filename, rpc_provider)
-                .await
-                .with_context(|| format!("Failed to install app '{app_package_filename}'"))?;
+            log::info!("Could not reach active daemon before test");
+            false
         }
-        Err(e) => return Err(anyhow::anyhow!(e).context("Failed to get app version")),
-        Ok(version) => {
-            if version != get_version_from_path(std::path::Path::new(app_package_filename))? {
-                log::info!("Daemon version mismatch, re-installing app");
-                mullvad_client = install_app(rpc, app_package_filename, rpc_provider)
-                    .await
-                    .context("Failed to install app")?;
-            }
-            helpers::ensure_daemon_environment(rpc)
-                .await
-                .context("Failed to reset daemon environment")?;
+        Err(e) => panic!("Failed to get app version: {e}"),
+        Ok(version) if version == expected_version => true,
+        _ => {
+            log::info!("Daemon version mismatch");
+            false
         }
     }
-    Ok(mullvad_client)
 }