From 0e5c123cb7dbc25877d79fc620b7c7676c51df0c Mon Sep 17 00:00:00 2001
From: Sebastian Holmin <sebastian.holmin@mullvad.net>
Date: Thu, 21 Nov 2024 16:27:19 +0100
Subject: [PATCH] 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..5de756a63d14 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 if 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<P: AsRef<Path>>(
-        all_tests: &[crate::tests::TestDesciption],
+        all_tests: &[crate::tests::TestDescription],
         path: P,
     ) -> Result<Summary, Error> {
         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<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![];
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<i32>,
@@ -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<TestDesciption> {
+pub fn get_test_descriptions() -> Vec<TestDescription> {
     let tests: Vec<_> = inventory::iter::<TestMetadata>()
-        .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<TestDesciption> {
 }
 
 /// 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<Vec<TestMetadata>, anyhow::Error> {
     let mut tests: Vec<_> = inventory::iter::<TestMetadata>().cloned().collect();
     tests.sort_by_key(|test| test.priority.unwrap_or(0));