-
Notifications
You must be signed in to change notification settings - Fork 155
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for async methods returning Task<T> or Task #179
Conversation
We do this by adding a Testable instance for Task<T> and Task, and also making the thread-local storage of typeclass instances use CallContext instead. Support for Task<T> and Task is only enabled for non-PCL builds as PCL doesn't have the CallContext class.
Thanks for looking at this. I had not heard of CallContext before, but am actually pretty skeptical it is the right way to address this issue (given it seems to be introduced in the context of remoting in .NET 1.1!). For example, if you use Task.Run in a test does it work? |
FWIW, many years ago I had to look through the IL code of ASP.NET to see how it dealt with It's far from pretty, but seemed to work. Although not what I would consider good design, I got the impression that it was well-tested in that framework. IIRC, it only copied certain well-know data around, such as the current principal and the current culture. Custom data wasn't copied around, which is why, in ASP.NET, one should always use HttpContext to store Ambient Contexts, instead of Thread Local Storage or It wouldn't surprise me if the TPL has similar low-level code that ensures that the |
Yes, good point, In ASP.NET likely works, but outside it? Good point about F# async...I guess the same is true for The more I think about this, the more thorny it gets, to the point where I'm starting to believe the only way to really make this work is to pass the type -> Arbitrary map around.... |
As of .NET 4.5, CallContext is flowed across all tasks. FsCheck requires .NET 4.5 at a minimum for the non-PCL library, and this functionality is disabled for PCL libraries.. We could certainly use AsyncLocal if on 4.6 (ASP.NET MVC does this, see e.g. https://github.com/aspnet/Mvc/blob/91aeec95e99c006c99f675e2ba887d4ed731532a/src/Microsoft.AspNet.Mvc.Core/Infrastructure/ActionBindingContextAccessor.cs). I think F# Async is an orthogonal issue - this only enables tests to be run on |
Interesting about the CallContext, do you have link explaining that?
|
This is a good link explaining CallContext: http://blog.stephencleary.com/2013/04/implicit-async-context-asynclocal.html. At work we rely on this heavily, both in ASP.NET and outside it. |
@frankshearar Thanks, I'll take a look at that. Sorry for dragging this along, I want to make sure what I get into here before merging, but haven't had sufficient time yet to investigate. |
First of all, thanks for taking the time to send this PR and my apologies for the extremely long lead time on this. I've recently given this PR some thought, and have decided not to accept it, for the following reasons. As more or less discussed in #198 I would like to reduce the scope of core FsCheck on the test runner front (and also on the assertions front). I think there are other projects that do a better job at this (e.g. xunit, NUnit, Unquote,...) and so would like FsCheck to be available as a library (rather than a framework) that you can use with any of these things. I feel like this PR on the other hand is an extension of FsCheck as a "test runner" and so is somewhat against this philosophy. That said, however I do see that many test runner frameworks expose some way to run async tests, and given FsCheck necessarily sits in between the test runner and the system under test, I see the usefulness of this PR. Here though I expect things to be more stable and understandable if we get rid of the need to pass the context altogether, which is planned for FsCheck 3. With that, I'm waiting until that refactoring is complete and then will re-evaluate - it may change the cost/benefit ratio in favor of this feature. |
This should fix #167 but I haven't had a chance to test it on anything
very async-heavy.
We do this by adding a Testable instance for Task and Task,
and also making the thread-local storage of typeclass instances
use CallContext instead.
Support for Task and Task is only enabled for non-PCL builds
as PCL doesn't have the CallContext class.