Using 'should throw' or 'should throwWithMessage' never succeeds unless function returns unit

[abelbraaksma]

Description

This is related to #214, but not the same.

The following test always fails:

[<Fact>]
let ``Test raising exception`` () =
    fun () -> raise (ArgumentException "help")
    |> should (throwWithMessage "help") typeof<ArgumentException>

Repro steps

See above.

Expected behavior

Test should succeed.

Actual behavior

Test fails with the following message:

Message: 
   FsUnit.Xunit+MatchException : Exception of type 'FsUnit.Xunit+MatchException' was thrown.
   Expected: System.ArgumentException "help"
   Actual:   <fun:Test raising exception@127>

Known workarounds

The following fixes the test, but this is not obvious. I should note that in the FsUnit help pages, all examples include ignore, but this isn't obvious and it took me a while to figure out what was wrong with my test (after our discussion in #214.

[<Fact>]
let ``Test raising exception`` () =
    fun () -> raise (ArgumentException "help") |> ignore
    |> should (throwWithMessage "help") typeof<ArgumentException>

Related information

There may be a reason why ignore is required in such tests. But if there's some way that this isn't required, I think that would be awesome: mainly because it isn't trivial to figure this out once it hits you.

PS: great work on fixing and maintaining this lib! It's generally a breeze to use!

[CaptnCodr]

Thank you @abelbraaksma for pointing out. 🙂

@sergey-tihon How do we proceed with this issue? Maybe taking an additional note as comment at line 92-93 in the docs?

(fun () -> failwith "BOOM!" |> ignore) |> should throw typeof<System.Exception> // |> ignore is important here.
(fun () -> failwith "BOOM!" |> ignore) |> should (throwWithMessage "BOOM!") typeof<System.Exception> // |> ignore is important here aswell.

Any other suggestions or smth else?

[abelbraaksma]

Isn't it possible to just catch & verify the exception? The return type shouldn't be a problem, right? It would alleviate the subtle requirement of using ignore and as such behave more like Assert.Throws (or whatever the xUnit method is).

[sergey-tihon]

We have 2 options

1. Support functions that return obj and merge #220
2. Fail in runtime and ask to refactor testFunc to return unit

I have no idea why unit -> obj was not supported before, it was way before me 🧐

[CaptnCodr]

Hello @abelbraaksma, a fix (#220) has been released in v5.0.5 and is already published on Nuget. 🙂

ignore is not necessary anymore. 😉

[abelbraaksma]

Oh wow! @CaptnCodr that’s awesome! I’ll try it out.

[abelbraaksma]

@CaptnCodr, are you sure it is part of 5.0.5? I just double-checked that I have that version, but I get the following:

    FsUnit.Xunit+MatchException : Exception of type 'FsUnit.Xunit+MatchException' was thrown.
Expected: System.Text.Json.JsonException "Expected a string, but given a Json Token Type of Number for 'amount'."
Actual:   <fun:Pipe #1 input at line 29@29>

The function looked like this, which is then called from the tests:

let shouldThrowWith msg data =
    fun () -> data |> deserialize<Money>
    |> should (throwWithMessage msg) typeof<JsonException>

It works with ignore.

Did I hit a fringe case or is it possible that the fix didn't make it to 5.0.5?

[CaptnCodr]

are you sure it is part of 5.0.5?

Yes, definitely! I downloaded the nuget package and decompiled the library and the change is in version 5.0.5.

May you try to capsulate the actual part with paranthesis? Like this:

let shouldThrowWith msg data =
    (fun () -> data |> deserialize<Money>)
    |> should (throwWithMessage msg) typeof<JsonException>

or did you try that already?

[abelbraaksma]

I'll try to make a minimal repro, I'll get back to this.

[CaptnCodr]

Hey @abelbraaksma, is here any progress? Let me know when I can help you.

[abelbraaksma]

@CaptnCodr, thanks for pinging me. Sorry, I kinda forgot about this but now, cleaning up my messages, I'm back ;).

Using your latest version, this is a simple example of a function that does throw, but does not successfully pass (adopted from a similar function in my TaskSeq library, where I just hit this issue again recently):

// minimal repro:

[<Fact>]
let ``Null source is invalid`` () =
    let test1 () = Seq.empty |> Seq.append null
    test1 |> should throw typeof<ArgumentNullException>

This gives the follow (wrong) error:

Message: 
  FsUnit.Xunit+MatchException : Exception of type 'FsUnit.Xunit+MatchException' was thrown.
  Expected: System.ArgumentNullException
  Actual:   <fun:test1@28T>

Changing this by adding ignore as before, the test succeeds:

[<Fact>]
let ``Null source is invalid`` () =
    let test1 () = Seq.empty |> Seq.append null |> ignore
    test1 |> should throw typeof<ArgumentNullException>

What I currently use as a workaround is just a simple helper:

let inline assertThrows ty (f: unit -> 'U) = f >> ignore |> should throw ty
let inline assertNullArg f = assertThrows typeof<ArgumentNullException> f

[CaptnCodr]

Hey @abelbraaksma, thanks for the little repro, I'll look at it this week. 🙂

[CaptnCodr]

Hey @abelbraaksma, ignore is definately needed here, the signature needs to be (unit -> unit). This is the intended way. So, I would not implement something for your concern here. Thank you.

[abelbraaksma]

Well, I appreciate it, assuming this isn’t as simple to fix as I thought. Though you did write a few messages before:

ignore is not necessary anymore.

And I have seen some functions not needing it. @CaptnCodr do you know in what cases or why it is still needed sometimes?

[CaptnCodr]

Hey @abelbraaksma, when you throw an exception, you don't need ignore anymore as in your opening post:

(fun() -> raise(ApplicationException "BOOM!"))
|> should (throwWithMessage "BOOM!") typeof<ApplicationException>

Also on functions that return obj or a generic type 'a. That is valid for throwWithMessage.

I tested it with throw without ignore, that test failed for xUnit. I'll put it on my list (https://github.com/fsprojects/FsUnit/issues/225).

[abelbraaksma]

you don't need ignore anymore as in your opening post:

exactly, but…

Also on functions that return obj or a generic type 'a. That is valid for throwWithMessage.

this didn’t work

tested it with throw without ignore, that test failed for xUnit. I'll put it on my list (https://github.com/fsprojects/FsUnit/issues/225).

Awesome, tx!