Using Should().Throw() with Func<T> fails after upgrading to version 5.5.0

[jeremyw5]

Description

After upgrading FluentAssertions from version 5.4.2 to 5.5.0, usage of Should().Throw with async method does not yield expected behavior. The following code example functions as expected with version 5.4.2.

Complete minimal example reproducing the issue

public class FluentAssertionsExceptionTests
{
    [Fact]
    public void Test1()
    {
        Func<Task<int>> func1 = () => TestAsync(false);
        func1.Should().NotThrow<Exception>();

        // Does not assert thrown exception
        Func<Task<int>> func2 = () => TestAsync(true);
        func2.Should().Throw<Exception>();
    }

    private async Task<int> TestAsync(bool throwException)
    {
        if (throwException)
        {
            throw new Exception("Error");
        }

        return await Task.FromResult(123);
    }
}

Expected behavior:

The assertion for the thrown exception passes.

Actual behavior:

The assertion for the thrown exception does not pass. The assertion does not detect the thrown exception.

Xunit.Sdk.XunitException
  HResult=0x80131500
  Message=Expected System.Exception, but no exception was thrown.
  Source=FluentAssertions
  StackTrace:
   at FluentAssertions.Execution.XUnit2TestFramework.Throw(String message)
   at FluentAssertions.Execution.AssertionScope.FailWith(Func`1 failReasonFunc)
   at FluentAssertions.Specialized.FunctionAssertions`1.Throw[TException](Exception exception, String because, Object[] becauseArgs)
   at FluentAssertions.Specialized.FunctionAssertions`1.Throw[TException](String because, Object[] becauseArgs)

Versions

Visual Studio 2017, .NET Core 2.1, XUnit 2.4.1, FluentAssertions 5.5.0

Additional Information

Removal of the 'TResult' argument from the Func<Task> yields expected behavior and the assertion functions as expected with version 5.5.0.

Func<Task> func3 = () => TestAsync(true);
func3.Should().Throw<Exception>();

[dennisdoomen]

Looks like #951 broke the overload resolution. That's unfortunate.

cc @krajek @jnyrup

[krajek]

I will take a look in the evening.

[dennisdoomen]

The problem is that the new overload that takes a Func<T> will always overrule the Func<Task<T>>.

[krajek]

I haven't looked in the code yet, but what would be your opinion on the idea to change the name of Should for Func<T> to something like ShouldSynchronousFunction?

[dennisdoomen]

No, it's the Should overloads that are causing the problem.

[krajek]

Let me clarify.

I understand the fact that it is the Should overload that causes the problem. If it will turn out that we cannot avoid the problem we would need to do one of two. Either remove Should for Func<T> altogether or change the name of the extension method.

Assuming that my reasoning is sound, my question is whether, as a last resort, you would accept using other extension method name than Should. (BTW, the question is also relevant for #963)

[dennisdoomen]

Maybe we can keep the Func<T> overload and internally see if it's a Task and wait for it.

[jnyrup]

It seems doable with the following new overload of Should()

public static AsyncFunctionAssertions Should<T>(this Func<Task<T>> action)
{
    return new AsyncFunctionAssertions(action, extractor);
}

[jnyrup]

@jeremyw5 Thanks again for the detailed issue description.
This is now fixed and will be a part of 5.5.1