Verify throws TargetInvocationException instead of MockException when SetupSequence includes returning a Task.FromException and the mocked method is not called enough times.

[Cufeadir]

I am using

• Moq v4.12.0
• xunit v2.4.1
• .NET Core SDK v2.2.401
• Visual Studio 16.2.1

I am mocking an interface with an asynchronous method on it. The code I am testing calls that interface multiple times and handles the case where the task from the mocked method fails with a custom exception. When mocking the interface, I use SetupSequence to prepare a sequence of different return values from the mocked method. As part of my assertions, I call Verify to check that the mocked method was called the correct number of times.

If my code under test is bug free, there is no problem. However, when a bug causes it to call the mocked method too few times, the Verify throws a TargetInvocationException instead of the expected MockException. As a result, my test does not report the correct cause of the failure. This only happens when at least one of the returned results in the SetupSequence includes a Task.FromException.

Here is a reproduction of the issue:

using System;
using System.Reflection;
using System.Threading.Tasks;
using Moq;
using Xunit;

namespace MoqIssue
{
  // The interface being mocked must has an asynchronous method
  public interface IFoo
  {
    Task DoAsync();
  }

  public class CustomException : Exception { }

  public class UnitTest1
  {
    // The code under test calls the mocked method
    // and handles the custom exception.
    async Task TestAsync(IFoo foo, int times)
    {
      times--; // Intentional bug to make the test fail
      for (int i = 0; i < times; i++)
      {
        try
        {
          await foo.DoAsync();
        }
        catch (CustomException)
        {
          // This bit doesn't matter.
        }
      }
    }

    [Fact]
    public async Task UnexpectedCase()
    {
      // Setup a sequence that returns at least one Task.FromException
      // The Task.FromException result needs to be one of the calls made
      // by the code under test. But the exact order or number of
      // Task.FromException results does not matter.
      var mockFoo = new Mock<IFoo>();
      mockFoo.SetupSequence(foo => foo.DoAsync())
        .Returns(Task.CompletedTask)
        .Returns(Task.FromException(new CustomException()))
        .Returns(Task.CompletedTask)
        .Returns(Task.CompletedTask);

      // Do a test that calls the mocked method multiple times.
      // But it needs to call it fewer times than expected.
      await TestAsync(mockFoo.Object, 4);

      // If uncommented, these checks pass as expected
      //mockFoo.Verify(foo => foo.DoAsync());
      //mockFoo.Verify(foo => foo.DoAsync(), Times.AtLeast(1));
      //mockFoo.Verify(foo => foo.DoAsync(), Times.AtLeast(2));
      //mockFoo.Verify(foo => foo.DoAsync(), Times.AtLeast(3));

      try
      {
        // This check throws a TargetInvocationException instead of
        // the expected MockException.
        mockFoo.Verify(foo => foo.DoAsync(), Times.AtLeast(4));
        // The same problem exists when using Times.Exactly(4)
      }
      catch (TargetInvocationException ex)
      {
        // This catch block is reached when it shouldn't be.
        // The InnerException is an AggregateException that
        // aggregates a CustomException, so the cause of the test failure
        // of not calling DoAsync enough times is not communicated.
        Assert.True(false, $"Verify failed with the wrong error: {ex}");
      }
    }

    [Fact]
    public async Task ExpectedCase()
    {
      // This is like the unexpected case,
      // except that all the calls return a successful task.
      var mockFoo = new Mock<IFoo>();
      mockFoo.SetupSequence(foo => foo.DoAsync())
        .Returns(Task.CompletedTask)
        .Returns(Task.CompletedTask)
        .Returns(Task.CompletedTask)
        .Returns(Task.CompletedTask);

      await TestAsync(mockFoo.Object, 4);

      try
      {
        // This check throws the expected MockException.
        // Expected invocation on the mock at least 4 times,
        // but was 3 times: foo => foo.DoAsync()
        mockFoo.Verify(foo => foo.DoAsync(), Times.AtLeast(4));
      }
      catch (TargetInvocationException ex)
      {
        // This catch block is correctly never reached.
        Assert.True(false, $"Verify failed with the wrong error: {ex}");
      }
    }
  }
}

[stakx]

@Cufeadir, thank you for the detailed description & repro.

This is indeed a bug, more specifically in these lines of code:

https://github.com/moq/moq4/blob/d8050e4cc9df4d074c68aa091f01bc20c0849211/src/Moq/Unwrap.cs#L26-L30

Moq is looking at each recorded invocation's return value to see whether the invocation returned a mocked object (it does this because verification is a recursive operation). The above lines assume that a completed task succeeded. However, a completed task can also have faulted.

If the above lines are changed to e.g.:

 if (isCompleted)
 {
+    try
+    {
         var innerObj = objType.GetProperty("Result").GetValue(obj, null);
         return Unwrap.ResultIfCompletedTask(innerObj);
+    }
+    catch
+    {
+        // Task may have faulted, so there's no result value to unwrap.
+    } 
 }

then the problem goes away. (There may be nicer / more correct bug fixes, that was just a quick experiment.)

Would you like to submit a PR, or would you prefer to let me deal with it?

[Cufeadir]

@stakx,

Thank you for looking into this so quickly and finding the root cause! I'm happy to let you deal with the fix. I don't have experience with Moq's internals, so I'd be a poor judge of the other impacts of the change or if there is a cleaner fix.