-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix unit tests on the 2.1 tree #688
Conversation
{ | ||
public async static Task SwitchToMainThreadAsync() | ||
{ | ||
if (!Guard.InUnitTestRunner()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure awaiting this will not do the job you want... because this is returning a Task when you await it it's going to try and continue on the capture context and not continue on the UI thread.
To prevent yourself form having to calling *.ConfiguerAwait(false)
you can do something like this gist for how i've solved this in the past
https://gist.github.com/tocsoft/afc9b5be41722f978ff0414d33afb1f1#file-theadinghelper-cs-L30
and
https://gist.github.com/tocsoft/afc9b5be41722f978ff0414d33afb1f1#file-theadinghelper-cs-L58
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that was just temporary code to get the CI to kick in 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😁.. only pointed it out 'cause I tried doing that exact thing the other week and got confused when my unit tests continued to fail. That was before I realised that, of course, it wouldn't work (when you get your head around how async/await works, in terms of threading).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's pretty insidious when the compiler starts piling on the syntactic sugar! It was good of you to point it out, you never know when it gets missed, this stuff is sneaky :P
Some services are not mocked by default, so getting the default service provider was returning a real instance, and setting a Returns on it does not make NSubstitute happy.
Might as well use the VisualStudio Threading library, since we're already bound to it because of the main thread switcher.
/// <returns></returns> | ||
public static IAwaitable SwitchToMainThreadAsync() | ||
{ | ||
return Guard.InUnitTestRunner() ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another way to implement this that wouldn't require the Awaitable plumbing would be:
public static async Task SwitchToMainThreadAsync()
{
if (!Guard.InUnitTestRunner())
{
await ThreadHelper.JoinableTaskFactory.SwitchToMainThreadAsync().ConfigureAwait(false);
}
}
I'm not sure about the perf considerations, but assuming that thread switching using async is relatively expensive, it might not have much of an impact?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That wouldn't work as when you call await SwitchToMainThreadAsync()
then the calling thread will be captured and continued on at the end of the await (unless you remember to call ConfigureAwait(false)
).
You don't want to have to remember to call ConfigureAwait(false)
every time its used. Its just one more thing to forget to do and cause yourself issues with calling stuff on the wrong thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think this is me misunderstanding ConfigureAwait
- I'd assumed that the ConfigureAwait(false)
state would be inherited in the Task that was returned, but yeah I see now that it'd affect the task that's being awaited rather than the task being returned. Wonder if there's a way to ConfigureAwait(false)
the resulting task?
{ | ||
return Guard.InUnitTestRunner() ? | ||
new AwaitableWrapper() : | ||
new AwaitableWrapper(scheduler); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly here could be something like:
public static async Task SwitchToPoolThreadAsync(TaskScheduler scheduler)
{
if (!Guard.InUnitTestRunner())
{
await Task.Factory.StartNew(
() => { },
new CancellationToken(false),
TaskCreationOptions.None,
scheduler).ConfigureAwait(false);
}
}
Yeah looks good to me - I tried to work out a more concise way to do the |
The threading helpers added here allow us to switch off threading for unit tests, and have one simple and common way of switching to the UI thread or to a background/scheduler thread without having to rely on VS APIs everywhere we want to do this.