Refactor test_macro error handling without panics

3 files changed, 107 insertions, 88 deletions

diff --git a/test/Cargo.lock b/test/Cargo.lock
index b974ef9217..60ab48547b 100644
--- a/test/Cargo.lock
+++ b/test/Cargo.lock
@@ -3229,6 +3229,7 @@ dependencies = [
  "proc-macro2",
  "quote",
  "syn 1.0.109",
+ "test-rpc",
 ]
 
 [[package]]
diff --git a/test/test-manager/test_macro/Cargo.toml b/test/test-manager/test_macro/Cargo.toml
index a064b6d200..19a405d08f 100644
--- a/test/test-manager/test_macro/Cargo.toml
+++ b/test/test-manager/test_macro/Cargo.toml
@@ -14,3 +14,4 @@ proc-macro = true
 syn = "1.0"
 quote = "1.0"
 proc-macro2 = "1.0"
+test-rpc = { path = "../../test-rpc" }
diff --git a/test/test-manager/test_macro/src/lib.rs b/test/test-manager/test_macro/src/lib.rs
index 387676d80a..fdf7e5539c 100644
--- a/test/test-manager/test_macro/src/lib.rs
+++ b/test/test-manager/test_macro/src/lib.rs
@@ -1,6 +1,7 @@
 use proc_macro::TokenStream;
 use quote::{quote, ToTokens};
-use syn::{AttributeArgs, Lit, Meta, NestedMeta};
+use syn::{AttributeArgs, Lit, Meta, NestedMeta, Result};
+use test_rpc::meta::Os;
 
 /// Register an `async` function to be run by `test-manager`.
 ///
@@ -76,7 +77,10 @@ pub fn test_function(attributes: TokenStream, code: TokenStream) -> TokenStream
     let function: syn::ItemFn = syn::parse(code).unwrap();
     let attributes = syn::parse_macro_input!(attributes as AttributeArgs);
 
-    let test_function = parse_marked_test_function(&attributes, &function);
+    let test_function = match parse_marked_test_function(&attributes, &function) {
+        Ok(tf) => tf,
+        Err(e) => return e.into_compile_error().into(),
+    };
 
     let register_test = create_test(test_function);
 
@@ -88,19 +92,31 @@ pub fn test_function(attributes: TokenStream, code: TokenStream) -> TokenStream
     .into()
 }
 
-fn parse_marked_test_function(attributes: &AttributeArgs, function: &syn::ItemFn) -> TestFunction {
-    let macro_parameters = get_test_macro_parameters(attributes);
+/// Shorthand for `return syn::Error::new(...)`.
+macro_rules! bail {
+    ($span:expr, $($tt:tt)*) => {{
+        return ::core::result::Result::Err(::syn::Error::new(
+            ::syn::spanned::Spanned::span(&$span),
+            ::core::format_args!($($tt)*),
+        ))
+    }};
+}
 
-    let function_parameters = get_test_function_parameters(&function.sig.inputs);
+fn parse_marked_test_function(
+    attributes: &AttributeArgs,
+    function: &syn::ItemFn,
+) -> Result<TestFunction> {
+    let macro_parameters = get_test_macro_parameters(attributes)?;
+    let function_parameters = get_test_function_parameters(&function.sig.inputs)?;
 
-    TestFunction {
+    Ok(TestFunction {
         name: function.sig.ident.clone(),
         function_parameters,
         macro_parameters,
-    }
+    })
 }
 
-fn get_test_macro_parameters(attributes: &syn::AttributeArgs) -> MacroParameters {
+fn get_test_macro_parameters(attributes: &syn::AttributeArgs) -> Result<MacroParameters> {
     let mut priority = None;
     let mut cleanup = true;
     let mut always_run = false;
@@ -108,53 +124,57 @@ fn get_test_macro_parameters(attributes: &syn::AttributeArgs) -> MacroParameters
     let mut target_os = None;
 
     for attribute in attributes {
-        if let NestedMeta::Meta(Meta::NameValue(nv)) = attribute {
-            if nv.path.is_ident("priority") {
-                match &nv.lit {
-                    Lit::Int(lit_int) => {
-                        priority = Some(lit_int.base10_parse().unwrap());
-                    }
-                    _ => panic!("'priority' should have an integer value"),
-                }
-            } else if nv.path.is_ident("always_run") {
-                match &nv.lit {
-                    Lit::Bool(lit_bool) => {
-                        always_run = lit_bool.value();
-                    }
-                    _ => panic!("'always_run' should have a bool value"),
-                }
-            } else if nv.path.is_ident("must_succeed") {
-                match &nv.lit {
-                    Lit::Bool(lit_bool) => {
-                        must_succeed = lit_bool.value();
-                    }
-                    _ => panic!("'must_succeed' should have a bool value"),
-                }
-            } else if nv.path.is_ident("cleanup") {
-                match &nv.lit {
-                    Lit::Bool(lit_bool) => {
-                        cleanup = lit_bool.value();
-                    }
-                    _ => panic!("'cleanup' should have a bool value"),
-                }
-            } else if nv.path.is_ident("target_os") {
-                match &nv.lit {
-                    Lit::Str(lit_str) => {
-                        target_os = Some(lit_str.value());
-                    }
-                    _ => panic!("'target_os' should have a string value"),
-                }
+        // we only use name-value attributes
+        let NestedMeta::Meta(Meta::NameValue(nv)) = attribute else {
+            bail!(attribute, "unknown attribute");
+        };
+        let lit = &nv.lit;
+
+        if nv.path.is_ident("priority") {
+            match lit {
+                Lit::Int(lit_int) => priority = Some(lit_int.base10_parse().unwrap()),
+                _ => bail!(nv, "'priority' should have an integer value"),
             }
+        } else if nv.path.is_ident("always_run") {
+            match lit {
+                Lit::Bool(lit_bool) => always_run = lit_bool.value(),
+                _ => bail!(nv, "'always_run' should have a bool value"),
+            }
+        } else if nv.path.is_ident("must_succeed") {
+            match lit {
+                Lit::Bool(lit_bool) => must_succeed = lit_bool.value(),
+                _ => bail!(nv, "'must_succeed' should have a bool value"),
+            }
+        } else if nv.path.is_ident("cleanup") {
+            match lit {
+                Lit::Bool(lit_bool) => cleanup = lit_bool.value(),
+                _ => bail!(nv, "'cleanup' should have a bool value"),
+            }
+        } else if nv.path.is_ident("target_os") {
+            let Lit::Str(lit_str) = lit else {
+                bail!(nv, "'target_os' should have a string value");
+            };
+
+            if target_os.is_some() {
+                bail!(nv, "can't specify multiple targets");
+            }
+
+            target_os = match lit_str.value().parse() {
+                Ok(os) => Some(os),
+                Err(e) => bail!(lit_str, "{e}"),
+            }
+        } else {
+            bail!(nv, "unknown attribute");
         }
     }
 
-    MacroParameters {
+    Ok(MacroParameters {
         priority,
         cleanup,
         always_run,
         must_succeed,
         target_os,
-    }
+    })
 }
 
 fn create_test(test_function: TestFunction) -> proc_macro2::TokenStream {
@@ -162,15 +182,10 @@ fn create_test(test_function: TestFunction) -> proc_macro2::TokenStream {
         Some(priority) => quote! { Some(#priority) },
         None => quote! { None },
     };
-    let target_os = match test_function.macro_parameters.target_os.as_deref() {
-        Some("linux") => quote! { Some(::test_rpc::meta::Os::Linux) },
-        Some("macos") => quote! { Some(::test_rpc::meta::Os::Macos) },
-        Some("windows") => quote! { Some(::test_rpc::meta::Os::Windows) },
-        Some(target_os) => {
-            return quote! {
-                compile_error!("invalid target_os: {:?}", #target_os);
-            };
-        }
+    let target_os = match test_function.macro_parameters.target_os {
+        Some(Os::Linux) => quote! { Some(::test_rpc::meta::Os::Linux) },
+        Some(Os::Macos) => quote! { Some(::test_rpc::meta::Os::Macos) },
+        Some(Os::Windows) => quote! { Some(::test_rpc::meta::Os::Windows) },
         None => quote! { None },
     };
     let should_cleanup = test_function.macro_parameters.cleanup;
@@ -193,7 +208,7 @@ fn create_test(test_function: TestFunction) -> proc_macro2::TokenStream {
                     use std::any::Any;
                     let mullvad_client = mullvad_client.downcast::<#mullvad_client_type>().expect("invalid mullvad client");
                     Box::pin(async move {
-                        Ok(#func_name(test_context, rpc, *mullvad_client).await?)
+                        #func_name(test_context, rpc, *mullvad_client).await.map_err(Into::into)
                     })
                 }
             }
@@ -202,9 +217,9 @@ fn create_test(test_function: TestFunction) -> proc_macro2::TokenStream {
             quote! {
                 |test_context: crate::tests::TestContext,
                 rpc: test_rpc::ServiceClient,
-                mullvad_client: Box<dyn std::any::Any + Send>| {
+                _mullvad_client: Box<dyn std::any::Any + Send>| {
                     Box::pin(async move {
-                        Ok(#func_name(test_context, rpc).await?)
+                        #func_name(test_context, rpc).await.map_err(Into::into)
                     })
                 }
             }
@@ -237,7 +252,7 @@ struct MacroParameters {
     cleanup: bool,
     always_run: bool,
     must_succeed: bool,
-    target_os: Option<String>,
+    target_os: Option<Os>,
 }
 
 enum MullvadClient {
@@ -269,36 +284,38 @@ struct FunctionParameters {
 }
 
 fn get_test_function_parameters(
-    inputs: &syn::punctuated::Punctuated<syn::FnArg, syn::Token![,]>,
-) -> FunctionParameters {
-    if inputs.len() > 2 {
-        match inputs[2].clone() {
-            syn::FnArg::Typed(pat_type) => {
-                let mullvad_client = match &*pat_type.ty {
-                    syn::Type::Path(syn::TypePath { path, .. }) => {
-                        match path.segments[0].ident.to_string().as_str() {
-                            "mullvad_management_interface" | "MullvadProxyClient" => {
-                                let mullvad_client_version =
-                                    quote! { test_rpc::mullvad_daemon::MullvadClientVersion::New };
-                                MullvadClient::New {
-                                    mullvad_client_type: pat_type.ty,
-                                    mullvad_client_version,
-                                }
-                            }
-                            _ => panic!("cannot infer mullvad client type"),
-                        }
-                    }
-                    _ => panic!("unexpected 'mullvad_client' type"),
-                };
-                FunctionParameters { mullvad_client }
-            }
-            syn::FnArg::Receiver(_) => panic!("unexpected 'mullvad_client' arg"),
-        }
-    } else {
-        FunctionParameters {
+    args: &syn::punctuated::Punctuated<syn::FnArg, syn::Token![,]>,
+) -> Result<FunctionParameters> {
+    if args.len() <= 2 {
+        return Ok(FunctionParameters {
             mullvad_client: MullvadClient::None {
-                mullvad_client_version: quote! { test_rpc::mullvad_daemon::MullvadClientVersion::None },
+                mullvad_client_version: quote! {
+                    test_rpc::mullvad_daemon::MullvadClientVersion::None
+                },
             },
-        }
+        });
     }
+
+    let arg = args[2].clone();
+    let syn::FnArg::Typed(pat_type) = arg else {
+        bail!(arg, "unexpected 'mullvad_client' arg");
+    };
+
+    let syn::Type::Path(syn::TypePath { path, .. }) = &*pat_type.ty else {
+        bail!(pat_type, "unexpected 'mullvad_client' type");
+    };
+
+    let mullvad_client = match path.segments[0].ident.to_string().as_str() {
+        "mullvad_management_interface" | "MullvadProxyClient" => {
+            let mullvad_client_version =
+                quote! { test_rpc::mullvad_daemon::MullvadClientVersion::New };
+            MullvadClient::New {
+                mullvad_client_type: pat_type.ty,
+                mullvad_client_version,
+            }
+        }
+        _ => bail!(pat_type, "cannot infer mullvad client type"),
+    };
+
+    Ok(FunctionParameters { mullvad_client })
 }