Adding Callback to a mock breaks async tests

[marcin-chwedczuk-meow]

I have the following csproj:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>netcoreapp2.1</TargetFramework>

    <IsPackable>false</IsPackable>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="15.8.0" />
    <PackageReference Include="Moq" Version="4.10.0" />
    <PackageReference Include="xunit" Version="2.3.1" />
    <PackageReference Include="xunit.runner.visualstudio" Version="2.3.1" />
  </ItemGroup>

</Project>

and the following test class:

    public class SomeComponentTest
    {
        private readonly SomeComponent _someComponent;
        private readonly Mock<ISomeDependency> _someDependency;

        public SomeComponentTest()
        {
            _someDependency = new Mock<ISomeDependency>();
            _someComponent = new SomeComponent(_someDependency.Object);
        }

        [Fact]
        public async Task Should_do_stuff()
        {
            _someDependency
                .Setup(x => x.DoMoreStuffAsync())
                // .Returns(Task.CompletedTask) // (line X)
                .Callback(() =>
                {
                    /*do nothing*/
                });

            await _someComponent.DoStuffAsync();
        }
    }

    public interface ISomeDependency
    {
        Task DoMoreStuffAsync();
    }

The test fails because DoMoreStuffAsync returns null which breaks async state machine generated by C# compiler. Uncommenting line that I called line X solves the problem.

In my opinion async methods should be treated specially by mock, and generally they should never return null (as this is certainly a mistake).

Expected behavior: When I define a callback on an async method it should by default return Task.CompletedTask to the caller so that it will not break async code.

[stakx]

Hi @marcin-chwedczuk-meow - what you are seeing here has nothing to do with .Callback at all. Rather, it is a consequence of setting up a method without explicitly defining a return value for that setup. As soon as you do a .Setup for a method, but omit a .Returns (or alternatively, .CallBase), Moq will simply return default for the method's return type. Note that the behavior is different when you don't .Setup the method at all; then Task will be handled specially (as you'd expect).

(See also #673 (comment) for a more detailed explanation of how Moq determines methods' return values.)

[marcin-chwedczuk-meow]

@stakx I understand your point, the rule for determining return values is simple and easy to follow. On the other hand if you are on .NET Core using async/await is a must (e.g. HttpClient provides only async API), and this Moq behavior is just uncomfortable when testing async method. I created this issue because I and my coworker wasted about 30 min of our time trying to figure out why our test is failing with seemingly unrelated exception. I believe that async methods are a very special case that needs a special handling...

[stakx]

@marcin-chwedczuk-meow - What exactly are you suggesting?

Expected behavior: When I define a callback on an async method it should by default return Task.CompletedTask to the caller so that it will not break async code.

If this is your full suggestion, please be aware that implementing this change has the potential of breaking other users' code. It would be far easier for you to simply add a .ReturnsAsync to your setup. Is that an option? (If not, could you briefly explain what keeps you from doing so?)

(Note, I do agree with you in principle. Moq's current way of determining a method's return value isn't exactly logical. I'd like to see this changed, but it's probably not worth the breaking change since there are easy workarounds available.)

[marcin-chwedczuk-meow]

Adding .ReturnsAsync is not a problem when you already know the solution. What I am afraid about is that juniors or less experienced programmers will use Callback as I used in my example and they will be baffled by a strange behavior of the test. They will get a NullReferenceException coming from async machinery. Maybe just extending documentation would be enough?

[stakx]

@marcin-chwedczuk-meow - I totally understand where you're coming from, and I agree there's a trap waiting here for anyone not overly familiar with Moq.

However, I don't buy the "juniors or less experienced programmers" argument. If a programmer does not know how to analyze and resolve a NullReferenceException (which is likely the most common exception), then they need to learn that skill. It shouldn't anyone too long to realize that they're trying to await a null reference. We simply cannot design Moq's API with (no insult intended—) totally inexperienced developers in mind, I think it's reasonable to expect a certain minimum skill level.

That being said, I am not sure how to best "fix" this problem. It'd be nice to simply make Moq's rules for determining return values less arcane, but that'd be a breaking change. Documenting the current rules would likely just make things more confusing, not less. So if we take the "better documentation" route, we should perhaps just state somewhere (possibly at the end of the Methods section in the Quickstart) that every setup for a non-void method should explicitly specify the return value via either of the .Returns or .CallBase verbs. That should be an easy enough guideline to follow.

Please feel free to improve the Quickstart page in the Wiki.

[stakx]

Possibly related: #327.

[stakx]

After looking at #794 I believe we could resolve this issue here by adding a .CallbackAsync method for setups on methods that return a [Value]Task:

mock.Setup(m => m.DoSomethingAsync()).CallbackAsync(async () => { ... });

(I know, lots of Async/async everywhere. #384 might help with that.)

Under the hood, it might be implemented like a simple .Returns.

[mnivet]

Another solution is to use Moq.SetupAsync which provide an extension method named SetupAsync on moq and will provide you all the you need to easily mock async methods respecting the state machine generated by dotnet

Once you add that lib to your project

<PackageReference Include="moq.SetupAsync" Version="1.0.0" />

you can use the SetupAsync method and simply write

someDependency.SetupAsync(x => x.DoMoreStuffAsync()); // this will return a competed task that do nothing to the statemachine

and get many more easy to use methods adapted to async scnearios