Result::Err is not interpreted as a failure

[tarka]

In Rust 2018 test can return a Result type, with Result::Err being interpreted as a failure. However with parameterised tests returned errors are treated as passing. While this can be fixed by specifying an expected value, it is unintuitive.

Example:

fn should_equal_4(num: i32) -> Result<(), ()> {
    if num == 4 {
        Ok(())
    } else {
        Err(())
    }
}

#[cfg(test)]
mod tests {
    use super::*;
    use test_case::test_case;

    #[test_case(4; "Passing")]
    #[test_case(5; "Failing")]
    fn with_params(num: i32) -> Result<(), ()> {
        should_equal_4(num)?;
        Ok(())
    }

    #[test]
    fn is_4() -> Result<(), ()> {
        should_equal_4(4)?;
        Ok(())
    }

    #[test]
    fn is_5() -> Result<(), ()> {
        should_equal_4(5)?;
        Ok(())
    }
}

Expected: Both tests::with_params::failing() and is_5() fail.

Actual: Only is_5() fails.

[luke-biel]

Hmm, we definitely need to look into that. For now you can work around it with #[test_case(5 => matches Ok(_); "Failing")].

Most of tests that I wrote for such scenarios included explicit unwrap without returning Result from test function, but being compatible with rust test is probably way to go.

[archoversight]

I just ran into this on accident, I wasn't expecting test_case to not function like the Rust standard #[test] in this situation.

[sminez]

This really should be tracked as a Bug not an Enhancement: moving from #[test] -> #[test_case(...)] shouldn't break the existing test.

This has lead to a large number of tests passing when they should have failed for us. We're now looking for an alternative to test-case because with this issue being open for this length of time, with no progress we just don't trust using this crate any more.

[luke-biel]

Yeah, you have a point that it should be a bug. Unfortunately I don't know when I'll have time to implement a fix. Maintaining this lib almost alone with work and studies in parallel ain't easiest thing to do & I don't see many people lining up trying to work on open issues.

[frondeus]

I recommend forking the repository. It is the essence of open source, after all.
You don't have to worry that the issue has been open for so long by forking the repository.
Of course, it requires an effort to patch the bug.

And while we made this library open-source and available for everyone, we (usually) are still driven by what we need in our personal and professional projects.

From my experience, I didn't fix this bug simply because it had no severity.
I rarely replaced #[test] with #[test_case], and when I did, I used unwrap() instead of the Result.

So while we value the input from other users, we have a limited amount of time for OSS projects.

That's why I didn't make any promises about this fix. If someone else thinks this is a critical issue, I'm more than happy to guide how to fix it and review and merge the PR.

[sminez]

I appreciate the responses on this. And yes, I do understand the nature of OSS and that everyone's time is limited for these sorts of things 🙂 Also I'd like to apologise for my original comment being as confrontational as it is: hitting this bug added to an already stressful day and I shouldn't have taken that out here.

As some additional context for why I'm finding this behaviour surprising: my main issue with this particular bug is that test-case is changing the semantics of returning results from tests as documented in the Rust book. This is not made clear in the README anywhere as far as I can tell, when an API change like that really should be called out quite clearly (please do correct me if I'm wrong on that!).

Given that all we want/need is parameterised tests, I've done as you suggest and written a minimal crate that just handles that side of things.

[tarka]

One short-term option might be to cause #[test_case] compilation to abort with an error if a return-type exists, which will at least remove the undetected failure problem. The following change to test-case seems to do what we need:

diff --git a/src/lib.rs b/src/lib.rs
index 8f38b09..2b30a87 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -231,7 +231,7 @@ extern crate proc_macro;
 
 use proc_macro::TokenStream;
 
-use syn::{parse_macro_input, ItemFn};
+use syn::{parse_macro_input, ItemFn, ReturnType};
 
 use proc_macro2::TokenStream as TokenStream2;
 use quote::quote;
@@ -310,6 +310,18 @@ pub fn test_case(args: TokenStream, input: TokenStream) -> TokenStream {
     let test_case = parse_macro_input!(args as TestCase);
     let mut item = parse_macro_input!(input as ItemFn);
 
+    match item.sig.output {
+        ReturnType::Type(_, ret) => {
+            return syn::Error::new(
+                ret.span(),
+                format!("Test function {} has a return-type. This is currently unsupported.", item.sig.ident),
+            )
+                .to_compile_error()
+                .into();
+        },
+        _ => {}
+    }
+
     let mut test_cases = vec![test_case];
     let mut attrs_to_remove = vec![];
     for (idx, attr) in item.attrs.iter().enumerate() {

This causes this error for a #[test_case] attribute on a function with any return type:

error: Test function equals_5 has a return-type. This is currently unsupported.
 --> src/lib.rs:8:36
  |
8 |     fn equals_5(a: i32, b: i32) -> Result<(), String> {
  |                                    ^^^^^^

error: could not compile `test-case-test` due to previous error

Thoughts on this? If this approach seems OK I could put together a PR.

[frondeus]

That unfortunately would be a major breaking change.
In test case an user can write a test_case like:

#[test_case(1, 1 => 2)]
fn adding(a: usize, b: usize) -> usize {
    a + b
}

As you can see the return type is supported in that case and it is asserted with assert_eq!(func_result, 2).

However, I think we could do such a detection with extra condition:
"If return-type is present AND the => syntax is not used, then return the error".

[frondeus]

Because it is a bug, we would have to patch it in the 1.x branch.
I think the best place to check the return-type would be https://github.com/frondeus/test-case/blob/1.0.x-support/src/test_case.rs#L85-L95

[tarka]

Thanks @frondeus. Here's an alternate version targeting the 1.0.x-support branch below. I'm not sure about the expected types test, input welcome.

diff --git a/src/lib.rs b/src/lib.rs
index 26b5210..48e3857 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -217,7 +217,7 @@ extern crate proc_macro;
 
 use proc_macro::TokenStream;
 
-use syn::{parse_macro_input, ItemFn};
+use syn::{parse_macro_input, ItemFn, ReturnType};
 
 use quote::quote;
 use syn::parse_quote;
@@ -305,6 +305,10 @@ pub fn test_case(args: TokenStream, input: TokenStream) -> TokenStream {
                     .into()
                 }
             };
+            if let Err(err) = return_test(&test_case, &item) {
+                return err;
+            }
+
             test_cases.push(test_case);
             attrs_to_remove.push(idx);
         }
@@ -317,6 +321,21 @@ pub fn test_case(args: TokenStream, input: TokenStream) -> TokenStream {
     render_test_cases(&test_cases, item)
 }
 
+fn return_test(test_case: &TestCase, item: &ItemFn) -> Result<(), TokenStream> {
+    let fn_ret = &item.sig.output;
+    match fn_ret {
+        ReturnType::Type(_, ret_type) if  !test_case.expects_return() =>  {
+            Err(syn::Error::new(
+                ret_type.span(),
+                format!("Test function {} has a return-type but no exected clause in the test-case. This is currently unsupported.", item.sig.ident),
+            )
+            .to_compile_error()
+            .into())
+        },
+        _ => Ok(())
+    }
+}
+
 fn render_test_cases(test_cases: &[TestCase], item: ItemFn) -> TokenStream {
     let mut rendered_test_cases = vec![];
 
diff --git a/src/test_case.rs b/src/test_case.rs
index 5a39f58..3c6968e 100644
--- a/src/test_case.rs
+++ b/src/test_case.rs
@@ -121,6 +121,16 @@ impl TestCase {
         crate::utils::escape_test_name(case_desc)
     }
 
+    pub fn expects_return(&self) -> bool {
+        match self.expected {
+            Some(Expected::Pat(_)) => true,
+            Some(Expected::Panic(_)) => false,
+            Some(Expected::Ignored(_)) => false,
+            Some(Expected::Expr(_)) => true,
+            None => false,
+        }
+    }
+
     pub fn render(&self, mut item: ItemFn) -> TokenStream2 {
         let item_name = item.sig.ident.clone();
         let arg_values = self.args.iter();

Now given these test cases:

#[cfg(test)]
mod tests {
    use test_case::test_case;

    #[test_case(2, 2; "Wrong")]
    #[test_case(2, 3; "Right")]
    fn return_no_expect(a: i32, b: i32) -> Result<i32, String> {
        let result = a + b;
        return if result == 5 {
            Ok(result)
        } else {
            Err("Not equal 5".to_owned())
        }
    }

    #[test_case(2, 2 => Err("Not equal 5".to_owned()); "Wrong")]
    #[test_case(2, 3 => Ok(5); "Right")]
    fn return_with_expect(a: i32, b: i32) -> Result<i32, String> {
        let result = a + b;
        return if result == 5 {
            Ok(result)
        } else {
            Err("Not equal 5".to_owned())
        }
    }
}

You get this:

error: Test function return_no_expect has a return-type but no exected clause in the test-case. This is currently unsupported.
 --> src/lib.rs:8:44
  |
8 |     fn return_no_expect(a: i32, b: i32) -> Result<i32, String> {
  |                                            ^^^^^^

error: could not compile `test-case-test` due to previous error

[frondeus]

That looks like an awesome fix. Thank you for your contribution :)!
Could you create a PR?

[tarka]

No worries, I'll put it together of the next few days.

[tarka]

@frondeus There doesn't actually appear to be a v1 support branch to target; the 1.0.x-support branch is behind the head of the v.1.2.1 tag. Where should I target the PR?

[luke-biel]

1.0.x support is old branch with some incompatible changes. @frondeus we probably should create similar branch for 1.2.1 from 1.2.1 tag since master is already at 2.0.0-rc.

I guess we need to come up with some proper branching strat.

[frondeus]

I will prepare proper target branch today

[luke-biel]

Ok, had some free time, I created branch 1.2.x-support. You can target it. I'll take care of porting the change to master when I'll return to working on it.

I see now that it'd be better if I worked on 2.0 release on some different branch than master. Well, will remember to do so in the future.

[frondeus]

As @bspradling noticed this code:

#[test_case("ISBN:1839485743"; "isbn10")]
#[test_case("ISBN:1839485743123"; "isbn13")]
#[tokio::test]
async fn test_bibliography_key_serde(expected: &str) -> Result<(), Box<dyn Error>> {
    let key: BibliographyKey = serde_json::from_str(expected)?;
    let actual = serde_json::to_string(&key)?;
    assert_eq!(expected, actual);
    Ok(())
}

cannot be simply fixed by adding => Ok(()).

Because Err in this case is Box<dyn Error> which does not implement PartialEq.

---

In that case the author doesn't want to check the error message, he just want to make sure the function does not return any errors at all.

But Rust doesn't know that.

The only solution I see for now is to do something like this:

#[test_case("ISBN:1839485743"; "isbn10")]
#[test_case("ISBN:1839485743123"; "isbn13")]
#[tokio::test]
async fn test_bibliography_key_serde(expected: &str)  {
    async fn inner(expected: &str) -> Result<(), Box<dyn Error>> {
        let key: BibliographyKey = serde_json::from_str(expected)?;
        let actual = serde_json::to_string(&key)?;
        assert_eq!(expected, actual);
        Ok(())
    }
    inner(expected).await.unwrap();
}

But it requires a lot of boilerplate and it's just... ugly.

Therefore I think this case proves we should increase the priority of this issue. It can be really inconvenient and the workaround is not easy as I wish it would be.

---

Also keep in mind that I believe matching the Err matching could be possible but the Rust does not support it either.
I mean you cannot do this:

#[test]
#[should_panic(expected = "123")]
fn my_test() -> Result<(), Box<dyn Error>> { ... }

but perhaps test_case could handle it by simply... unwrapping the result and using should_panic

[frondeus]

About the last paragraph:

I would imagine something like:

#[test_case(1, 2 => panics "fail")]
fn test_me(a: usize, b: usize) -> Result<usize, Box<dyn Error>> {
   ....
}

To work just fine.

[frondeus]

Extra: #86 (comment)