From 41955ef6e28d275dd17feaf5067f0bb8b753a039 Mon Sep 17 00:00:00 2001 From: Sebastian Holmin Date: Thu, 21 Nov 2024 13:12:46 +0100 Subject: [PATCH 1/4] Add `test_upgrade_app` to list tests --- test/test-manager/src/run_tests.rs | 2 +- test/test-manager/src/summary.rs | 4 +-- test/test-manager/src/tests/mod.rs | 27 +++++++++++++++----- test/test-manager/src/tests/test_metadata.rs | 1 + 4 files changed, 25 insertions(+), 9 deletions(-) diff --git a/test/test-manager/src/run_tests.rs b/test/test-manager/src/run_tests.rs index dd07ef5f3a08..b3b910f05dec 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, skip_wait: bool, print_failed_tests_only: bool, summary_logger: Option, diff --git a/test/test-manager/src/summary.rs b/test/test-manager/src/summary.rs index 1beaef91b957..55d290ec5a02 100644 --- a/test/test-manager/src/summary.rs +++ b/test/test-manager/src/summary.rs @@ -118,7 +118,7 @@ pub struct Summary { impl Summary { /// Read test summary from `path`. pub async fn parse_log>( - all_tests: &[&crate::tests::TestMetadata], + all_tests: &[crate::tests::TestMetadata], path: P, ) -> Result { let file = fs::OpenOptions::new() @@ -189,7 +189,7 @@ pub async fn print_summary_table>(summary_files: &[P]) { 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 a7b3778907bb..e974430e6d4e 100644 --- a/test/test-manager/src/tests/mod.rs +++ b/test/test-manager/src/tests/mod.rs @@ -29,7 +29,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::{mullvad_daemon::MullvadClientVersion, ServiceClient}; const WAIT_FOR_TUNNEL_STATE_TIMEOUT: Duration = Duration::from_secs(40); @@ -76,14 +76,29 @@ pub enum Error { } /// Get a list of all tests, sorted by priority. -pub fn get_tests() -> Vec<&'static TestMetadata> { - let mut tests: Vec<_> = inventory::iter::().collect(); +pub fn get_tests() -> Vec { + let mut tests: Vec<_> = inventory::iter::().cloned().collect(); tests.sort_by_key(|test| test.priority.unwrap_or(0)); - tests + let test_upgrade_app = TestMetadata { + priority: None, + name: "test_upgrade_app", + command: "test_upgrade_app", + targets: &[], + mullvad_client_version: MullvadClientVersion::None, + func: |_, _, _| { + Box::pin(async { + unreachable!("`test_upgrade_app` should not be executed from this function pointer") + }) + }, + }; + [vec![test_upgrade_app], tests].concat() } -pub fn get_filtered_tests(specified_tests: &[String]) -> Result, 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 handle separately. +pub fn get_filtered_tests(specified_tests: &[String]) -> Result, anyhow::Error> { + let mut tests: Vec<_> = inventory::iter::().cloned().collect(); + tests.sort_by_key(|test| test.priority.unwrap_or(0)); let mut tests = if specified_tests.is_empty() { // Keep all tests diff --git a/test/test-manager/src/tests/test_metadata.rs b/test/test-manager/src/tests/test_metadata.rs index 70476ac19959..66447f5ae745 100644 --- a/test/test-manager/src/tests/test_metadata.rs +++ b/test/test-manager/src/tests/test_metadata.rs @@ -1,6 +1,7 @@ use super::TestWrapperFunction; use test_rpc::{meta::Os, mullvad_daemon::MullvadClientVersion}; +#[derive(Clone)] pub struct TestMetadata { pub name: &'static str, pub command: &'static str, From 6e8d73cf873cec9af6a9a238dc643707a6f2517f Mon Sep 17 00:00:00 2001 From: Sebastian Holmin Date: Thu, 21 Nov 2024 14:25:52 +0100 Subject: [PATCH 2/4] Remove the `command` field of `TestMetadata` --- test/test-manager/src/tests/mod.rs | 3 +-- test/test-manager/src/tests/test_metadata.rs | 1 - test/test-manager/test_macro/src/lib.rs | 1 - 3 files changed, 1 insertion(+), 4 deletions(-) diff --git a/test/test-manager/src/tests/mod.rs b/test/test-manager/src/tests/mod.rs index e974430e6d4e..f50742c04237 100644 --- a/test/test-manager/src/tests/mod.rs +++ b/test/test-manager/src/tests/mod.rs @@ -82,7 +82,6 @@ pub fn get_tests() -> Vec { let test_upgrade_app = TestMetadata { priority: None, name: "test_upgrade_app", - command: "test_upgrade_app", targets: &[], mullvad_client_version: MullvadClientVersion::None, func: |_, _, _| { @@ -109,7 +108,7 @@ pub fn get_filtered_tests(specified_tests: &[String]) -> Result 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, From e5a9881991be7351df62a5231a6f6bdb18e753d8 Mon Sep 17 00:00:00 2001 From: Sebastian Holmin Date: Thu, 21 Nov 2024 15:01:48 +0100 Subject: [PATCH 3/4] Improve type safety --- test/test-manager/src/summary.rs | 6 ++-- test/test-manager/src/tests/mod.rs | 37 +++++++++++++------- test/test-manager/src/tests/test_metadata.rs | 6 ---- 3 files changed, 29 insertions(+), 20 deletions(-) diff --git a/test/test-manager/src/summary.rs b/test/test-manager/src/summary.rs index 55d290ec5a02..29a2f8d974dd 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>( - all_tests: &[crate::tests::TestMetadata], + all_tests: &[crate::tests::TestDesciption], path: P, ) -> Result { 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); diff --git a/test/test-manager/src/tests/mod.rs b/test/test-manager/src/tests/mod.rs index f50742c04237..e3b485972a91 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::{mullvad_daemon::MullvadClientVersion, ServiceClient}; +use test_rpc::{meta::Os, ServiceClient}; const WAIT_FOR_TUNNEL_STATE_TIMEOUT: Duration = Duration::from_secs(40); @@ -75,20 +76,32 @@ pub enum Error { Other(String), } +#[derive(Clone)] +/// An abbriviated version of [`TestMetadata`] +pub struct TestDesciption { + pub name: &'static str, + pub targets: &'static [Os], + pub priority: Option, +} + +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 { - let mut tests: Vec<_> = inventory::iter::().cloned().collect(); - tests.sort_by_key(|test| test.priority.unwrap_or(0)); - let test_upgrade_app = TestMetadata { +pub fn get_tests() -> Vec { + let tests: Vec<_> = inventory::iter::() + .map(|test| TestDesciption { + priority: test.priority, + name: test.name, + targets: test.targets, + }) + .sorted_by_key(|test| test.priority) + .collect_vec(); + let test_upgrade_app = TestDesciption { priority: None, name: "test_upgrade_app", targets: &[], - mullvad_client_version: MullvadClientVersion::None, - func: |_, _, _| { - Box::pin(async { - unreachable!("`test_upgrade_app` should not be executed from this function pointer") - }) - }, }; [vec![test_upgrade_app], tests].concat() } @@ -114,7 +127,7 @@ pub fn get_filtered_tests(specified_tests: &[String]) -> Result>()? }; - 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 341562acaedd..79c7f74def7e 100644 --- a/test/test-manager/src/tests/test_metadata.rs +++ b/test/test-manager/src/tests/test_metadata.rs @@ -10,11 +10,5 @@ pub struct TestMetadata { pub priority: Option, } -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); From 6afad8129c0ec848dd56c2d603fb291bcbce077b Mon Sep 17 00:00:00 2001 From: Sebastian Holmin Date: Thu, 21 Nov 2024 16:27:19 +0100 Subject: [PATCH 4/4] Improve documentation --- test/test-manager/src/main.rs | 2 +- test/test-manager/src/run_tests.rs | 5 +++++ test/test-manager/src/summary.rs | 4 ++-- test/test-manager/src/tests/mod.rs | 14 ++++++++------ 4 files changed, 16 insertions(+), 9 deletions(-) diff --git a/test/test-manager/src/main.rs b/test/test-manager/src/main.rs index 8628502eff10..a56f1f2ea877 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 b3b910f05dec..88577fb3b3cd 100644 --- a/test/test-manager/src/run_tests.rs +++ b/test/test-manager/src/run_tests.rs @@ -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 29a2f8d974dd..f0c0723c48de 100644 --- a/test/test-manager/src/summary.rs +++ b/test/test-manager/src/summary.rs @@ -120,7 +120,7 @@ pub struct Summary { impl Summary { /// Read test summary from `path`. pub async fn parse_log>( - all_tests: &[crate::tests::TestDesciption], + all_tests: &[crate::tests::TestDescription], path: P, ) -> Result { let file = fs::OpenOptions::new() @@ -186,7 +186,7 @@ impl Summary { /// be parsed, we should not abort the entire summarization. pub async fn print_summary_table>(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![]; diff --git a/test/test-manager/src/tests/mod.rs b/test/test-manager/src/tests/mod.rs index e3b485972a91..9eb23fe13e8d 100644 --- a/test/test-manager/src/tests/mod.rs +++ b/test/test-manager/src/tests/mod.rs @@ -77,8 +77,8 @@ pub enum Error { } #[derive(Clone)] -/// An abbriviated version of [`TestMetadata`] -pub struct TestDesciption { +/// An abbreviated version of [`TestMetadata`] +pub struct TestDescription { pub name: &'static str, pub targets: &'static [Os], pub priority: Option, @@ -89,16 +89,18 @@ pub fn should_run_on_os(targets: &[Os], os: Os) -> bool { } /// Get a list of all tests, sorted by priority. -pub fn get_tests() -> Vec { +pub fn get_test_descriptions() -> Vec { let tests: Vec<_> = inventory::iter::() - .map(|test| TestDesciption { + .map(|test| TestDescription { priority: test.priority, name: test.name, targets: test.targets, }) .sorted_by_key(|test| test.priority) .collect_vec(); - let test_upgrade_app = TestDesciption { + + // 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: &[], @@ -107,7 +109,7 @@ pub fn get_tests() -> Vec { } /// 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 handle separately. +/// the target platform and `test_upgrade_app`, which is run separately. pub fn get_filtered_tests(specified_tests: &[String]) -> Result, anyhow::Error> { let mut tests: Vec<_> = inventory::iter::().cloned().collect(); tests.sort_by_key(|test| test.priority.unwrap_or(0));