Repository navigation
Fixed UnobservedTaskException after using ThrowsAsync. Fixed ReturnsAsync and ThrowsAsync with delay - #595
Conversation
|
@stakx I have have a concern about changed behavior that we have discussed. We chose to keep the behavior that |
|
@snrnats: Thanks for this new PR. Thank you also for raising above question.
Yes, after thinking about this a little longer, I believe this would be the better solution; for two reasons:
Task<int> IFoo.GetSomeNumberAsync() // returns a new Task each time
{
return Task.FromResult(42);
}
Task<int> IFoo.GetSomeNumberAsync() // returns a new Task each time
{
var tcs = new TaskCompletionSource<int>();
tcs.SetException(new Exception("..."));
return tcs.Task;
}
async Task<int> IFoo.GetSomeNumberAsync() // returns a new Task each time
{
await Task.Delay(0);
return 42;
}
async Task<int> IFoo.GetSomeNumberAsync() // returns a new Task each time
{
await Task.Delay(0);
throw new Exception("...");
}To return the same private Lazy<Task<int>> cachedTask = new Lazy<Task<int>>(() =>
{
return Task.FromResult(42);
});
Task<int> IFoo.GetSomeNumberAsync() => cachedTask.Value;And I dare say this isn't exactly what most people will do in practice, so why should Moq? And a third reason specific to So I think I'd be OK with a breaking change here, as long as this is explicitly marked in the changelog entry, e.g.: So you'd have to find out exactly which methods are affected, and mention them all in the changelog. |
There was a problem hiding this comment.
Looks good! I'd like to request only two minor changes (line spacing, and simplifying the changelog entry).
Please allow me one comment, IMHO it would've been better to create two separate PRs, one for each issue. But there's no need now to re-do all the work you've done already; I'm fine with having one single PR that closes both issues.
Thanks for your great work so far! 👍
| } | ||
|
|
||
| #endregion | ||
| #region 593 |
There was a problem hiding this comment.
Please insert a blank line above this one (i.e. between #endregion and #region 593).
| { | ||
| var tcs = new TaskCompletionSource<TResult>(); | ||
| Task.Delay(delay).ContinueWith(task => tcs.SetException(exception)); | ||
| return tcs.Task; |
There was a problem hiding this comment.
I was almost going to suggest that you rewrite (but please don't!) the above 3 lines to the following:
return Task.Delay(delay).ContinueWith<TResult>(task => throw exception);to make it more similar to the other implementations; but then I realised that this would result in a needlessly more complex exception stack trace. So better leave it as it is. :-)
|
|
||
| #### Changed | ||
|
|
||
| * **Breaking change:** `sequenceSetup.ReturnsAsync(...)`, `sequenceSetup.ThrowsAsync(...)`, `setup.ReturnsAsync(..., TimeSpan)`, `setup.ReturnsAsync(..., TimeSpan, TimeSpan)`, `setup.ReturnsAsync(..., TimeSpan, TimeSpan, Random)` and all overloads of `setup.ThrowsAsync(...)` now return a different `Task` on each invocation, which wasn't previously the case (@snrnats, #595) |
There was a problem hiding this comment.
It appears I was under the false impression that those methods that always returned the same Task were in the minority. But this long list suggests that the opposite is true: reused Task results currently appear to be the norm.
In order to make the changelog entry easier to read, may I suggest that we rephrase this changelog entry to something simpler, for example:
- Breaking change: All
ReturnsAsyncandThrowsAsyncsetup methods now consistently return a newTaskon each invocation (...)
|
I wanted to make two separate PRs. But I wasn't comfortable changing the code when I know that it still has issues:smile: |
There was a problem hiding this comment.
Looks good! 👍 (Method names in the changelog are missing code formatting (the backticks), but that can be added any time before the next release is published.)
Thanks for the great work!
|
@snrnats - I just pushed Moq 4.8.3 to NuGet, which includes your changes. Sorry it's taken so long. |
This closes #592, closes #593