ReturnsAsync and ThrowsAsync with delay parameter starts timer at setup

[snrnats]

ReturnsAsync and ThrowsAsync with delay parameter start timer at setup, not at mock method invocation.

The reason is that Task.Delay is called inside DelayedResult/DelayedException but it needs to be deferred until mock method is invoked.

public class Issue593
{
	[Fact]
	public void ReturnsAsync_ThrowsAsync_start_delay_timer_at_mock_invocation()
	{
		var mock = new Mock<IFoo>();
		mock.Setup(a => a.Boo()).ReturnsAsync(true, TimeSpan.FromMilliseconds(100));
		mock.Setup(a => a.Foo()).ThrowsAsync(new ArgumentException(), TimeSpan.FromMilliseconds(100));

		//Wait for the delay greater then specified for Boo setup
		Task.Delay(200).GetAwaiter().GetResult();

		Assert.False(mock.Object.Boo().IsCompleted);
		Assert.False(mock.Object.Foo().IsCompleted);
	}

	public interface IFoo
	{
		Task<bool> Foo();
		Task<bool> Boo();
	}
}

[stakx]

Thanks for reporting! 👍 Looks like a definite bug to me. (Or can you imagine any scenario where this could be the intended behavior?)

Since you're already working on ReturnsExtensions for #592, would you be willing this fix this problem, too?

One thing about your test, I find that even if I remove the Task.Delay(200)... line of code, the test would still fail. I'm not quite sure why that is: Are the timers that inaccurate, or is the whole async machinery adding that many milliseconds of execution overhead, or is the ratio 200 ms : 100 ms too small?

(And one more thing out of personal curiosity: I'm often seeing people using .GetAwaiter().GetResult(). I've been repeatedly wondering, wouldn't it be more idiomatic to use await Task.Delay(200) (in this particular case) and make the test method async? xUnit shouldn't have any trouble with that. Is there a specific advantage of blocking / synchronous execution over an asynchronous await in this case?)

[informatorius]

As you want to wait in that line before the Asserts the simplest line would be Task.Delay(200).Wait()
await Task.Delay would return and internally create another continuation task appended after the line.

[snrnats]

@stakx I will fix it too as it is related to the same code.

The unit test depends on execution time. Seems like there are some implementation details of Task that doesn't allow to verify that bug equally on different machines. These 100 and 200 ms are magic numbers that work for me. I tried to choose them big enough so that delayed tasks are run for sure asynchronous. Do we have time limit for unit test?

I agree to use await as xUnit supports it.

[stakx]

@snrnats: There are a few unit tests dealing with e. g. thread safety that work with fixed time spans, so your test wouldn't be the first. It's not an ideal way of testing things but often it's the only way. 100 ms seems to be a good compromise to me between keeping the test quick and keeping it "accurate". I'd say 1 sec would be the upper limit, but it's good to stay away from it if possible.

Thanks for looking into this.

@informatorius: Thanks for confirming that .GetAwaiter().GetResult() and .Wait() do pretty much the same thing. (Full async is probably better in this case as it potentially frees a test runner thread to execute other tests while the timer's running.)