Automatic extraction of aggregate exceptions makes it harder for me to test

[chrischu]

I want to test a method that is synchronous but calls an asynchronous method and I want to ensure that I use AsyncMethod().WaitAndUnwrapExceptions() instead of AsyncMethod.Result, however this is currently not easily testable since FluentAssertions automatically unwraps the exception.

This is in my opinion a pretty bad idea as it might lead to a false feeling of security when in reality it might cover up a bug, because in reality no one unwraps the exception if I use .Result and then I have to catch AggregateException instead of the contained exceptions.

Therefore I think it would be best to completely remove the automatic exception unwrapping from assertions. However, if that is not desired for backwards compatibility I would at least like an option to disable the unwrapping in my solution (similiarly to how I can configure how ShouldBeEquivalentTo works).

[dennisdoomen]

You're specifically talking about the AggregateException that FA is trying to intercept, right? If so, what would the API look like?

[chrischu]

Something like AssertionOptions.UnwrapAggregateExceptionsAutomatically() (and the opposite).

[dennisdoomen]

That would be a global setting. Did you try Should().ThrowExact()?

[chrischu]

You probably mean ShouldThrowExactly<TException>()?

Unfortunately that also unwraps (it just calls the normal Throws under the hood as seen here: https://github.com/fluentassertions/fluentassertions/blob/release-5.0/Src/FluentAssertions/AssertionExtensions.Actions.cs#L36)

[davidomid]

@dennisdoomen I've taken a look at this and written some tests which confirm that current ShouldThrowExactly will work when an AggregateException is thrown. This is not the behaviour I would expect either, and would instead expect it to only work for the inner exception type.

Is this how it's meant to work or is this an issue? I'm happy to write a fix for this if needed.

[dennisdoomen]

The general guidelines are:

I think we should unwrap an AggregateException if it is FluentAssertions that causes it to be inserted. Only if invoking a Task directly would also raise an AggregateException, we should expose it. Throw() is special here. If you call Should().Throw() and the calling code raises this specific exception wrapped in an AggregateException, I believe it should succeed. This is the way it is currently implemented and documented. The same (should) apply to ThrowExactly, ThrowAfter, etc.

However, I'm in doubt about the ThrowExactly now. Currently, if the code being asserted throws an InvalidOperationException wrapped in an AggregateException, Should().ThrowExactly<InvalidOperationException>() will succeed. I now feel that that this is wrong as well. What do you think @jnyrup ?

[jnyrup]

From what I read here, one expected way to handling unwrapping of AggregateExceptions is that:

1. Fluent Assertions should not wrap exceptions: this is what Redundant AggregateException removal #1038 is about.

• Throw<TException>
• NotThrow<TException>
• ThrowAsync<TException>
• NotThrowAsync<TException>

Should recursively check if a thrown Exception is of type TException or if any inner exception of an AggregateException is.
The following (pseudo-)code might be clearer than my words.

bool Check<TException>(Exception ex):
  if ex is TException: 
    return true

  if ex is AggregateException ae:
    foreach(innerEx in ae.InnerExceptions)
      if Check<TException>(innerEx)
        return true;

  return false

On the other hand

• ThrowExactly<TException>
• ThrowExactlyAsync<TException>

should not check AggregateExceptions, but only the immediately thrown exception.

CheckExact<TException>(Exception ex):
  if ex.GetType() == typeof(TException): 
    return true

  return false

[dennisdoomen]

Exactly

[davidomid]

I’ll create an issue and a PR specifically for fixing ThrowExactly and ThrowExactlyAsync, if you guys don’t mind?

[dennisdoomen]

I’ll create an issue and a PR specifically for fixing ThrowExactly and ThrowExactlyAsync, if you guys don’t mind?

Don't forget to update the docs for that part as well. It should be clear how it is supposed to work.

[jnyrup]

@davidomid I've been working on something similar in #1041, but haven't finished it.
If you can use anything from it, feel free to copy.

[davidomid]

No worries, I'll update the documentation too.

@jnyrup Thanks very much, I'll check this out and try and make my changes consistent with yours.