Merge branch 'test-manager-fixes'

Author: dlon

test/scripts/ssh-setup.sh

@@ -17,7 +17,7 @@ echo "Copying test-runner to $RUNNER_DIR"
 mkdir -p "$RUNNER_DIR"
 
 for file in test-runner connection-checker $APP_PACKAGE $PREVIOUS_APP $UI_RUNNER; do
-    echo "Moving $file to $RUNNER_DIR"
+    echo "Moving $SCRIPT_DIR/$file to $RUNNER_DIR"
     cp -f "$SCRIPT_DIR/$file" "$RUNNER_DIR"
 done
 
@@ -106,6 +106,7 @@ fi
 setup_systemd
 
 function install_packages_apt {
+    echo "Installing required apt packages"
     apt update
     apt install -yf xvfb wireguard-tools curl
     curl -fsSL https://get.docker.com | sh

test/scripts/test-utils.sh

@@ -50,7 +50,7 @@ function get_package_dir {
     local package_dir
     if [[ -n "${PACKAGE_DIR+x}" ]]; then
         # Resolve the package dir to an absolute path since cargo must be invoked from the test directory
-        package_dir=$(cd "$PACKAGE_DIR" > /dev/null && pwd)
+        package_dir=$(realpath "$PACKAGE_DIR")
     elif [[ ("$(uname -s)" == "Darwin") ]]; then
         package_dir="$HOME/Library/Caches/mullvad-test/packages"
     elif [[ ("$(uname -s)" == "Linux") ]]; then

test/test-manager/src/logging.rs

@@ -3,6 +3,8 @@ use colored::Colorize;
 use std::sync::{Arc, Mutex};
 use test_rpc::logging::{LogOutput, Output};
 
+use crate::summary;
+
 /// Logger that optionally supports logging records to a buffer
 #[derive(Clone)]
 pub struct Logger {
@@ -109,25 +111,94 @@ impl log::Log for Logger {
     fn flush(&self) {}
 }
 
+/// Encapsulate caught unwound panics, such that we can catch tests that panic and differentiate
+/// them from tests that just fail.
 #[derive(Debug, thiserror::Error)]
-#[error("Test panic: {0}")]
-pub struct PanicMessage(String);
+#[error("Test panic: {}", self.as_string())]
+pub struct Panic(Box<dyn std::any::Any + Send + 'static>);
+
+impl Panic {
+    /// Create a new [`Panic`] from a caught unwound panic.
+    pub fn new(result: Box<dyn std::any::Any + Send + 'static>) -> Self {
+        Self(result)
+    }
+
+    /// Convert this panic to a [`String`] representation.
+    pub fn as_string(&self) -> String {
+        if let Some(result) = self.0.downcast_ref::<String>() {
+            return result.clone();
+        }
+        match self.0.downcast_ref::<&str>() {
+            Some(s) => String::from(*s),
+            None => String::from("unknown message"),
+        }
+    }
+}
 
 pub struct TestOutput {
     pub error_messages: Vec<Output>,
     pub test_name: &'static str,
-    pub result: Result<Result<(), Error>, PanicMessage>,
+    pub result: TestResult,
     pub log_output: Option<LogOutput>,
 }
 
+// Convert this unwieldy return type to a workable `TestResult`.
+// What we are converting from is the acutal return type of the test execution.
+impl From<Result<Result<(), Error>, Panic>> for TestResult {
+    fn from(value: Result<Result<(), Error>, Panic>) -> Self {
+        match value {
+            Ok(Ok(())) => TestResult::Pass,
+            Ok(Err(e)) => TestResult::Fail(e),
+            Err(e) => TestResult::Panic(e),
+        }
+    }
+}
+
+/// Result from a test execution. This may carry information in case the test failed during
+/// execution.
+pub enum TestResult {
+    /// Test passed.
+    Pass,
+    /// Test failed during execution. Contains the source error which caused the test to fail.
+    Fail(Error),
+    /// Test panicked during execution. Contains the caught unwound panic.
+    Panic(Panic),
+}
+
+impl TestResult {
+    /// Returns `true` if test failed or panicked, i.e. when `TestResult` is `Fail` or `Panic`.
+    pub const fn failure(&self) -> bool {
+        matches!(self, TestResult::Fail(_) | TestResult::Panic(_))
+    }
+
+    /// Convert `self` to a [`summary::TestResult`], which is used for creating fancy exports of
+    /// the results for a test run.
+    pub const fn summary(&self) -> summary::TestResult {
+        match self {
+            TestResult::Pass => summary::TestResult::Pass,
+            TestResult::Fail(_) | TestResult::Panic(_) => summary::TestResult::Fail,
+        }
+    }
+
+    /// Consume `self` and convert into a [`Result`] where [`TestResult::Pass`] is mapped to [`Ok`]
+    /// while [`TestResult::Fail`] & [`TestResult::Panic`] is mapped to [`Err`].
+    pub fn anyhow(self) -> anyhow::Result<()> {
+        match self {
+            TestResult::Pass => Ok(()),
+            TestResult::Fail(error) => anyhow::bail!(error),
+            TestResult::Panic(error) => anyhow::bail!(error.to_string()),
+        }
+    }
+}
+
 impl TestOutput {
     pub fn print(&self) {
         match &self.result {
-            Ok(Ok(_)) => {
+            TestResult::Pass => {
                 println!("{}", format!("TEST {} SUCCEEDED!", self.test_name).green());
                 return;
             }
-            Ok(Err(e)) => {
+            TestResult::Fail(e) => {
                 println!(
                     "{}",
                     format!(
@@ -138,13 +209,13 @@ impl TestOutput {
                     .red()
                 );
             }
-            Err(panic_msg) => {
+            TestResult::Panic(panic_msg) => {
                 println!(
                     "{}",
                     format!(
                         "TEST {} PANICKED WITH MESSAGE: {}",
                         self.test_name,
-                        panic_msg.0.bold()
+                        panic_msg.as_string().bold()
                     )
                     .red()
                 );
@@ -191,13 +262,3 @@ impl TestOutput {
         println!("{}", format!("TEST {} END OF OUTPUT", self.test_name).red());
     }
 }
-
-pub fn panic_as_string(error: Box<dyn std::any::Any + Send + 'static>) -> PanicMessage {
-    if let Some(result) = error.downcast_ref::<String>() {
-        return PanicMessage(result.clone());
-    }
-    match error.downcast_ref::<&str>() {
-        Some(s) => PanicMessage(String::from(*s)),
-        None => PanicMessage(String::from("unknown message")),
-    }
-}

test/test-manager/src/main.rs

@@ -13,6 +13,7 @@ use std::{net::SocketAddr, path::PathBuf};
 
 use anyhow::{Context, Result};
 use clap::Parser;
+use tests::{config::TEST_CONFIG, get_filtered_tests};
 use vm::provision;
 
 use crate::tests::config::OpenVPNCertificate;
@@ -92,8 +93,9 @@ enum Commands {
         #[arg(long)]
         app_package: String,
 
-        /// App package to upgrade from when running `test_install_previous_app`, can be left empty
-        /// if this test is not ran. Parsed the same way as `--app-package`.
+        /// Given this argument, the `test_upgrade_app` test will run, which installs the previous
+        /// version then upgrades to the version specified in by `--app-package`. If left empty,
+        /// the test will be skipped. Parsed the same way as `--app-package`.
         ///
         /// # Note
         ///
@@ -117,7 +119,8 @@ enum Commands {
         #[arg(long)]
         openvpn_certificate: Option<PathBuf>,
 
-        /// Only run tests matching substrings
+        /// Names of tests to run. The order given will be respected. If not set, all tests will be
+        /// run.
         test_filters: Vec<String>,
 
         /// Print results live
@@ -293,6 +296,28 @@ async fn main() -> Result<()> {
                 .await
                 .context("Failed to run provisioning for VM")?;
 
+            TEST_CONFIG.init(tests::config::TestConfig::new(
+                account,
+                artifacts_dir,
+                manifest
+                    .app_package_path
+                    .file_name()
+                    .unwrap()
+                    .to_string_lossy()
+                    .into_owned(),
+                manifest
+                    .app_package_to_upgrade_from_path
+                    .map(|path| path.file_name().unwrap().to_string_lossy().into_owned()),
+                manifest
+                    .gui_package_path
+                    .map(|path| path.file_name().unwrap().to_string_lossy().into_owned()),
+                mullvad_host,
+                vm::network::bridge()?,
+                test_rpc::meta::Os::from(vm_config.os_type),
+                openvpn_certificate,
+            ));
+            let tests = get_filtered_tests(&test_filters)?;
+
             // For convenience, spawn a SOCKS5 server that is reachable for tests that need it
             let socks = socks_server::spawn(SocketAddr::new(
                 crate::vm::network::NON_TUN_GATEWAY.into(),
@@ -302,41 +327,16 @@ async fn main() -> Result<()> {
 
             let skip_wait = vm_config.provisioner != config::Provisioner::Noop;
 
-            let result = run_tests::run(
-                tests::config::TestConfig::new(
-                    account,
-                    artifacts_dir,
-                    manifest
-                        .app_package_path
-                        .file_name()
-                        .unwrap()
-                        .to_string_lossy()
-                        .into_owned(),
-                    manifest
-                        .app_package_to_upgrade_from_path
-                        .map(|path| path.file_name().unwrap().to_string_lossy().into_owned()),
-                    manifest
-                        .gui_package_path
-                        .map(|path| path.file_name().unwrap().to_string_lossy().into_owned()),
-                    mullvad_host,
-                    vm::network::bridge()?,
-                    test_rpc::meta::Os::from(vm_config.os_type),
-                    openvpn_certificate,
-                ),
-                &*instance,
-                &test_filters,
-                skip_wait,
-                !verbose,
-                summary_logger,
-            )
-            .await
-            .context("Tests failed");
+            let result = run_tests::run(&*instance, tests, skip_wait, !verbose, summary_logger)
+                .await
+                .context("Tests failed");
 
             if display {
                 instance.wait().await;
             }
             socks.close();
-            result
+            // Propagate any error from the test run if applicable
+            result?.anyhow()
         }
         Commands::FormatTestReports { reports } => {
             summary::print_summary_table(&reports).await;

test/test-manager/src/network_monitor.rs

@@ -98,7 +98,7 @@ impl Codec {
                 payload = seg.payload().to_vec();
             }
             IpHeaderProtocols::Icmp => {}
-            proto => log::debug!("ignoring v4 packet, transport/protocol type {proto}"),
+            proto => log::warn!("ignoring v4 packet, transport/protocol type {proto}"),
         }
 
         Some(ParsedPacket {
@@ -140,7 +140,7 @@ impl Codec {
                 payload = seg.payload().to_vec();
             }
             IpHeaderProtocols::Icmpv6 => {}
-            proto => log::debug!("ignoring v6 packet, transport/protocol type {proto}"),
+            proto => log::warn!("ignoring v6 packet, transport/protocol type {proto}"),
         }
 
         Some(ParsedPacket {

test/test-manager/src/package.rs

@@ -69,7 +69,7 @@ pub fn get_app_manifest(
     })
 }
 
-fn get_version_from_path(app_package_path: &Path) -> Result<String, anyhow::Error> {
+pub fn get_version_from_path(app_package_path: &Path) -> Result<String, anyhow::Error> {
     static VERSION_REGEX: Lazy<Regex> =
         Lazy::new(|| Regex::new(r"\d{4}\.\d+((-beta\d+)?(-dev)?-([0-9a-z])+)?").unwrap());