Merge branch 'test_upgrade_app-is-missing-from-the-test-result-matrix-des-1468'

6 files changed, 52 insertions, 23 deletions

diff --git a/test/test-manager/src/main.rs b/test/test-manager/src/main.rs
index 8628502eff..a56f1f2ea8 100644
--- a/test/test-manager/src/main.rs
+++ b/test/test-manager/src/main.rs
@@ -229,7 +229,7 @@ async fn main() -> Result<()> {
         }
         Commands::ListTests => {
             println!("priority\tname");
-            for test in tests::get_tests() {
+            for test in tests::get_test_descriptions() {
                 println!(
                     "{priority:8}\t{name}",
                     name = test.name,
diff --git a/test/test-manager/src/run_tests.rs b/test/test-manager/src/run_tests.rs
index dd07ef5f3a..88577fb3b3 100644
--- a/test/test-manager/src/run_tests.rs
+++ b/test/test-manager/src/run_tests.rs
@@ -102,7 +102,7 @@ impl TestHandler<'_> {
 
 pub async fn run(
     instance: &dyn vm::VmInstance,
-    tests: Vec<&TestMetadata>,
+    tests: Vec<TestMetadata>,
     skip_wait: bool,
     print_failed_tests_only: bool,
     summary_logger: Option<SummaryLogger>,
@@ -139,6 +139,11 @@ pub async fn run(
         logger: Logger::get_or_init(),
     };
 
+    // We need to handle the upgrade test separately since it expects the daemon to *not* be
+    // installed, which is done by `tests::prepare_daemon`, and only runs with
+    // `app_package_to_upgrade_from_filename` set.
+    // TODO: Extend `TestMetadata` and the `test_function` macro to specify what daemon state is
+    // expected, and to allow for skipping tests on arbitrary conditions.
     if TEST_CONFIG.app_package_to_upgrade_from_filename.is_some() {
         test_handler
             .run_test(
diff --git a/test/test-manager/src/summary.rs b/test/test-manager/src/summary.rs
index 1beaef91b9..f0c0723c48 100644
--- a/test/test-manager/src/summary.rs
+++ b/test/test-manager/src/summary.rs
@@ -5,6 +5,8 @@ use tokio::{
     io::{AsyncBufReadExt, AsyncWriteExt},
 };
 
+use crate::tests::should_run_on_os;
+
 #[derive(thiserror::Error, Debug)]
 pub enum Error {
     #[error("Failed to open log file {1:?}")]
@@ -118,7 +120,7 @@ pub struct Summary {
 impl Summary {
     /// Read test summary from `path`.
     pub async fn parse_log<P: AsRef<Path>>(
-        all_tests: &[&crate::tests::TestMetadata],
+        all_tests: &[crate::tests::TestDescription],
         path: P,
     ) -> Result<Summary, Error> {
         let file = fs::OpenOptions::new()
@@ -155,7 +157,7 @@ impl Summary {
         for test in all_tests {
             // Add missing test results
             let entry = results.entry(test.name.to_owned());
-            if test.should_run_on_os(os) {
+            if should_run_on_os(test.targets, os) {
                 entry.or_insert(TestResult::Unknown);
             } else {
                 entry.or_insert(TestResult::Skip);
@@ -184,12 +186,12 @@ impl Summary {
 /// be parsed, we should not abort the entire summarization.
 pub async fn print_summary_table<P: AsRef<Path>>(summary_files: &[P]) {
     // Collect test details
-    let tests = crate::tests::get_tests();
+    let tests = crate::tests::get_test_descriptions();
 
     let mut summaries = vec![];
     let mut failed_to_parse = vec![];
     for sumfile in summary_files {
-        match Summary::parse_log(&tests, sumfile).await {
+        match Summary::parse_log(&tests[..], sumfile).await {
             Ok(summary) => summaries.push(summary),
             Err(_) => failed_to_parse.push(sumfile),
         }
diff --git a/test/test-manager/src/tests/mod.rs b/test/test-manager/src/tests/mod.rs
index a7b3778907..9eb23fe13e 100644
--- a/test/test-manager/src/tests/mod.rs
+++ b/test/test-manager/src/tests/mod.rs
@@ -15,6 +15,7 @@ mod tunnel;
 mod tunnel_state;
 mod ui;
 
+use itertools::Itertools;
 pub use test_metadata::TestMetadata;
 
 use anyhow::Context;
@@ -29,7 +30,7 @@ use config::TEST_CONFIG;
 use helpers::{get_app_env, install_app};
 pub use install::test_upgrade_app;
 use mullvad_management_interface::MullvadProxyClient;
-use test_rpc::ServiceClient;
+use test_rpc::{meta::Os, ServiceClient};
 
 const WAIT_FOR_TUNNEL_STATE_TIMEOUT: Duration = Duration::from_secs(40);
 
@@ -75,15 +76,43 @@ pub enum Error {
     Other(String),
 }
 
+#[derive(Clone)]
+/// An abbreviated version of [`TestMetadata`]
+pub struct TestDescription {
+    pub name: &'static str,
+    pub targets: &'static [Os],
+    pub priority: Option<i32>,
+}
+
+pub fn should_run_on_os(targets: &[Os], os: Os) -> bool {
+    targets.is_empty() || targets.contains(&os)
+}
+
 /// Get a list of all tests, sorted by priority.
-pub fn get_tests() -> Vec<&'static TestMetadata> {
-    let mut tests: Vec<_> = inventory::iter::<TestMetadata>().collect();
-    tests.sort_by_key(|test| test.priority.unwrap_or(0));
-    tests
+pub fn get_test_descriptions() -> Vec<TestDescription> {
+    let tests: Vec<_> = inventory::iter::<TestMetadata>()
+        .map(|test| TestDescription {
+            priority: test.priority,
+            name: test.name,
+            targets: test.targets,
+        })
+        .sorted_by_key(|test| test.priority)
+        .collect_vec();
+
+    // Since `test_upgrade_app` is not registered with inventory, we need to add it manually
+    let test_upgrade_app = TestDescription {
+        priority: None,
+        name: "test_upgrade_app",
+        targets: &[],
+    };
+    [vec![test_upgrade_app], tests].concat()
 }
 
-pub fn get_filtered_tests(specified_tests: &[String]) -> Result<Vec<&TestMetadata>, anyhow::Error> {
-    let tests = get_tests();
+/// Return all tests with names matching the input argument. Filters out tests that are skipped for
+/// the target platform and `test_upgrade_app`, which is run separately.
+pub fn get_filtered_tests(specified_tests: &[String]) -> Result<Vec<TestMetadata>, anyhow::Error> {
+    let mut tests: Vec<_> = inventory::iter::<TestMetadata>().cloned().collect();
+    tests.sort_by_key(|test| test.priority.unwrap_or(0));
 
     let mut tests = if specified_tests.is_empty() {
         // Keep all tests
@@ -94,13 +123,13 @@ pub fn get_filtered_tests(specified_tests: &[String]) -> Result<Vec<&TestMetadat
             .map(|f| {
                 tests
                     .iter()
-                    .find(|t| t.command.eq_ignore_ascii_case(f))
+                    .find(|t| t.name.eq_ignore_ascii_case(f))
                     .cloned()
                     .ok_or(anyhow::anyhow!("Test '{f}' not found"))
             })
             .collect::<Result<_, anyhow::Error>>()?
     };
-    tests.retain(|test| test.should_run_on_os(TEST_CONFIG.os));
+    tests.retain(|test| should_run_on_os(test.targets, TEST_CONFIG.os));
     Ok(tests)
 }
 
diff --git a/test/test-manager/src/tests/test_metadata.rs b/test/test-manager/src/tests/test_metadata.rs
index 70476ac199..79c7f74def 100644
--- a/test/test-manager/src/tests/test_metadata.rs
+++ b/test/test-manager/src/tests/test_metadata.rs
@@ -1,20 +1,14 @@
 use super::TestWrapperFunction;
 use test_rpc::{meta::Os, mullvad_daemon::MullvadClientVersion};
 
+#[derive(Clone)]
 pub struct TestMetadata {
     pub name: &'static str,
-    pub command: &'static str,
     pub targets: &'static [Os],
     pub mullvad_client_version: MullvadClientVersion,
     pub func: TestWrapperFunction,
     pub priority: Option<i32>,
 }
 
-impl TestMetadata {
-    pub fn should_run_on_os(&self, os: Os) -> bool {
-        self.targets.is_empty() || self.targets.contains(&os)
-    }
-}
-
 // Register our test metadata struct with inventory to allow submitting tests of this type.
 inventory::collect!(TestMetadata);
diff --git a/test/test-manager/test_macro/src/lib.rs b/test/test-manager/test_macro/src/lib.rs
index 1b80776f29..cddb6c5a2f 100644
--- a/test/test-manager/test_macro/src/lib.rs
+++ b/test/test-manager/test_macro/src/lib.rs
@@ -187,7 +187,6 @@ fn create_test(test_function: TestFunction) -> proc_macro2::TokenStream {
     quote! {
         inventory::submit!(crate::tests::test_metadata::TestMetadata {
             name: stringify!(#func_name),
-            command: stringify!(#func_name),
             targets: &[#targets],
             mullvad_client_version: #function_mullvad_version,
             func: #wrapper_closure,