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

FsCheck.Xunit doesn't support C# async tests #167

Closed
Porges opened this Issue Oct 8, 2015 · 24 comments

Comments

Projects
None yet
4 participants
@Porges

Porges commented Oct 8, 2015

With a test like:

[Property]
public async Task TestWhatever(int i)
{
    Assert.True(await DoSomething(i));
}

The Xunit runner fails with:

 System.Exception:
 No instances of class FsCheck.Testable+Testable`1[a] for type System.Threading.Tasks.Task
@kurtschelfthout

This comment has been minimized.

Show comment
Hide comment
@kurtschelfthout

kurtschelfthout Oct 9, 2015

Member

This could be tricky, as we need to pass have some static data (in particular, the overridden arbitrary instances) around. Currently this is done using a thread static, because xunit2 by default runs different tests in parallel. But if we have async tests together with that we'd need to pass them accross async boundaries too.

Perhaps something like http://stackoverflow.com/questions/6896652/threadstatic-for-tpl-task helps.

Member

kurtschelfthout commented Oct 9, 2015

This could be tricky, as we need to pass have some static data (in particular, the overridden arbitrary instances) around. Currently this is done using a thread static, because xunit2 by default runs different tests in parallel. But if we have async tests together with that we'd need to pass them accross async boundaries too.

Perhaps something like http://stackoverflow.com/questions/6896652/threadstatic-for-tpl-task helps.

@Porges

This comment has been minimized.

Show comment
Hide comment
@Porges

Porges Oct 9, 2015

Hmm, so a custom synchronization context might be required. I'll try to investigate this weekend and see if there's a sane solution...

Porges commented Oct 9, 2015

Hmm, so a custom synchronization context might be required. I'll try to investigate this weekend and see if there's a sane solution...

@ploeh

This comment has been minimized.

Show comment
Hide comment
@ploeh

ploeh Oct 9, 2015

Member

@kurtschelfthout Would it be possible to move FsCheck in a direction where reliance on thread-static data isn't required?

Member

ploeh commented Oct 9, 2015

@kurtschelfthout Would it be possible to move FsCheck in a direction where reliance on thread-static data isn't required?

@kurtschelfthout

This comment has been minimized.

Show comment
Hide comment
@kurtschelfthout

kurtschelfthout Oct 9, 2015

Member

@ploeh Yes, but not without giving up a few features, in particular the ability to override Arbitrary instances and configure them via e.g. PropertyAttribute.

For vanilla FsCheck this is not a huge deal I think - though you lose the ability to e.g. override the string generator and still use the reflective generator to generate a large record type that contains strings, using your custom generator. But that could be solved by allowing you to pass in those overrides to the reflective generator. (Not sure I am making sense to you here)

The bigger change is that essentially the arguments to methods with a PropertyAttribuite could only come from the immutable default Arbitrary instances, and you couldn't customize them. So, essentially you'd have to use Prop.ForAll much more frequently.

This can work - it's what Erlang quickcheck does - but obviously a breaking change.

Member

kurtschelfthout commented Oct 9, 2015

@ploeh Yes, but not without giving up a few features, in particular the ability to override Arbitrary instances and configure them via e.g. PropertyAttribute.

For vanilla FsCheck this is not a huge deal I think - though you lose the ability to e.g. override the string generator and still use the reflective generator to generate a large record type that contains strings, using your custom generator. But that could be solved by allowing you to pass in those overrides to the reflective generator. (Not sure I am making sense to you here)

The bigger change is that essentially the arguments to methods with a PropertyAttribuite could only come from the immutable default Arbitrary instances, and you couldn't customize them. So, essentially you'd have to use Prop.ForAll much more frequently.

This can work - it's what Erlang quickcheck does - but obviously a breaking change.

@ploeh

This comment has been minimized.

Show comment
Hide comment
@ploeh

ploeh Oct 15, 2015

Member

I'm not sure I understand this... If we consider the current implementation of the PropertyAttribute, there's no thread-static state involved in that library. It collects Arbitraries (Type list) and binds those immutable values to a Config value, and then calls into FsCheck proper...

Member

ploeh commented Oct 15, 2015

I'm not sure I understand this... If we consider the current implementation of the PropertyAttribute, there's no thread-static state involved in that library. It collects Arbitraries (Type list) and binds those immutable values to a Config value, and then calls into FsCheck proper...

@kurtschelfthout

This comment has been minimized.

Show comment
Hide comment
@kurtschelfthout

kurtschelfthout Oct 15, 2015

Member

Here is what gets set by Config: https://github.com/fscheck/FsCheck/blob/master/src/FsCheck/Arbitrary.fs#L131

It is likely possible to not rely on side-effects to register Arb instances and pass the them all the way into the Gen type...

Member

kurtschelfthout commented Oct 15, 2015

Here is what gets set by Config: https://github.com/fscheck/FsCheck/blob/master/src/FsCheck/Arbitrary.fs#L131

It is likely possible to not rely on side-effects to register Arb instances and pass the them all the way into the Gen type...

@Porges

This comment has been minimized.

Show comment
Hide comment
@Porges

Porges Nov 6, 2015

I had a go at this, the changes needed weren't very big. It works on the small examples I have added and all the current tests still pass.

Porges commented Nov 6, 2015

I had a go at this, the changes needed weren't very big. It works on the small examples I have added and all the current tests still pass.

@kurtschelfthout

This comment has been minimized.

Show comment
Hide comment
@kurtschelfthout

kurtschelfthout Nov 6, 2015

Member

CallContext I did not know about, kind of interesting, but I think the right way to fix is to use AsyncLocal, but that would require us to switch to .NET 4.6.

Even righter would be if I could figure out a way to pass the registered instances around, but don't think that's possible without actually changing the API (aka FsCheck 3).

So it seems like AsyncLocal is our best bete short term. I wonder how people would feel about requiring .NET 4.6. Perhaps we could multi-target (i.e. if you want to write async tests, use .NET 4.6)

Member

kurtschelfthout commented Nov 6, 2015

CallContext I did not know about, kind of interesting, but I think the right way to fix is to use AsyncLocal, but that would require us to switch to .NET 4.6.

Even righter would be if I could figure out a way to pass the registered instances around, but don't think that's possible without actually changing the API (aka FsCheck 3).

So it seems like AsyncLocal is our best bete short term. I wonder how people would feel about requiring .NET 4.6. Perhaps we could multi-target (i.e. if you want to write async tests, use .NET 4.6)

@ploeh

This comment has been minimized.

Show comment
Hide comment
@ploeh

ploeh Nov 6, 2015

Member

if you want to write async tests, use .NET 4.6

I never really understood the desire to write async tests... What's the motivation for this?

Member

ploeh commented Nov 6, 2015

if you want to write async tests, use .NET 4.6

I never really understood the desire to write async tests... What's the motivation for this?

@kurtschelfthout

This comment has been minimized.

Show comment
Hide comment
@kurtschelfthout

kurtschelfthout Nov 6, 2015

Member

@ploeh The desire is not so much for async tests, but easily being able to test async code in the SUT afaik. How would you test async code?

Member

kurtschelfthout commented Nov 6, 2015

@ploeh The desire is not so much for async tests, but easily being able to test async code in the SUT afaik. How would you test async code?

@ploeh

This comment has been minimized.

Show comment
Hide comment
@ploeh

ploeh Nov 6, 2015

Member

How would you test async code?

In F#: Async.RunSynchronously

In C#: .Result or .Wait() for non-generic Task.

Member

ploeh commented Nov 6, 2015

How would you test async code?

In F#: Async.RunSynchronously

In C#: .Result or .Wait() for non-generic Task.

@kurtschelfthout

This comment has been minimized.

Show comment
Hide comment
@kurtschelfthout

kurtschelfthout Nov 6, 2015

Member

In C# that most likely deadlocks (at least in e.g. mstest), because when the async Task finishes it tries to get back on the main thread, but that is waiting for the async one to finish...

Member

kurtschelfthout commented Nov 6, 2015

In C# that most likely deadlocks (at least in e.g. mstest), because when the async Task finishes it tries to get back on the main thread, but that is waiting for the async one to finish...

@ploeh

This comment has been minimized.

Show comment
Hide comment
@ploeh

ploeh Nov 6, 2015

Member

I don't know what's going to happen in MsTest, but I've never had a problem with this approach in xUnit.net.

To be clear, I never define my test functions or methods as async; they always return unit/void (or Property in the case of FsCheck).

Member

ploeh commented Nov 6, 2015

I don't know what's going to happen in MsTest, but I've never had a problem with this approach in xUnit.net.

To be clear, I never define my test functions or methods as async; they always return unit/void (or Property in the case of FsCheck).

@kurtschelfthout

This comment has been minimized.

Show comment
Hide comment
@kurtschelfthout

kurtschelfthout Nov 6, 2015

Member

It depends in what sort of context you're running these things. If you're running them in a single-threaded context, they will deadlock. xUnit.Net, at least 2.0 onwards runs test in parallel by default so this sort of thing will work.

Point is, it's already complicated enough without your test framework having to add a bunch of ifs and buts of its own, so I'd like this to work in all cases, rather than some, or at least make it blatantly obvious when it's going to work and when not.

Member

kurtschelfthout commented Nov 6, 2015

It depends in what sort of context you're running these things. If you're running them in a single-threaded context, they will deadlock. xUnit.Net, at least 2.0 onwards runs test in parallel by default so this sort of thing will work.

Point is, it's already complicated enough without your test framework having to add a bunch of ifs and buts of its own, so I'd like this to work in all cases, rather than some, or at least make it blatantly obvious when it's going to work and when not.

@ploeh

This comment has been minimized.

Show comment
Hide comment
@ploeh

ploeh Nov 6, 2015

Member

This is probably derailing the discussion a bit, but I don't understand this. Isn't the point of async to avoid blocking threads?

If I have a Task<int> called i, and I call i.Result, why would a test deadlock?

Granted, calling Result blocks the calling thread, but lots of things do that. From the caller's perspective, isn't this similar to invoking Thread.Sleep?

Member

ploeh commented Nov 6, 2015

This is probably derailing the discussion a bit, but I don't understand this. Isn't the point of async to avoid blocking threads?

If I have a Task<int> called i, and I call i.Result, why would a test deadlock?

Granted, calling Result blocks the calling thread, but lots of things do that. From the caller's perspective, isn't this similar to invoking Thread.Sleep?

@kurtschelfthout

This comment has been minimized.

Show comment
Hide comment
@kurtschelfthout

kurtschelfthout Nov 6, 2015

Member

Result blocks the calling thread, and in a single-threaded context (e.g. when you have a message pump, like in a UI), when the async call returns, it will try to get the result back on the calling thread. But that thread is blocked on Result until the async call returns, which can't return because it can't get its result on the calling thread because it's blocked on Result, etc

Member

kurtschelfthout commented Nov 6, 2015

Result blocks the calling thread, and in a single-threaded context (e.g. when you have a message pump, like in a UI), when the async call returns, it will try to get the result back on the calling thread. But that thread is blocked on Result until the async call returns, which can't return because it can't get its result on the calling thread because it's blocked on Result, etc

@ploeh

This comment has been minimized.

Show comment
Hide comment
@ploeh

ploeh Nov 7, 2015

Member

This does makes some sense to me in the context of integration testing, but I don't understand how this can ever apply to unit tests. Perhaps it's just me being dense... 😳

Member

ploeh commented Nov 7, 2015

This does makes some sense to me in the context of integration testing, but I don't understand how this can ever apply to unit tests. Perhaps it's just me being dense... 😳

@Porges

This comment has been minimized.

Show comment
Hide comment
@Porges

Porges Nov 7, 2015

Yes, it fits integration testing, but in addition, if your classes under
test are very async it doesn't make sense to write your tests in a
synchronous manner.

There's also no reason that FsCheck shouldn't work with async tests ☺

Porges commented Nov 7, 2015

Yes, it fits integration testing, but in addition, if your classes under
test are very async it doesn't make sense to write your tests in a
synchronous manner.

There's also no reason that FsCheck shouldn't work with async tests ☺

@kurtschelfthout

This comment has been minimized.

Show comment
Hide comment
@kurtschelfthout

kurtschelfthout Nov 8, 2015

Member

My opinion on all that is that any test is better than no test, and so there's not reason to make testing harder than it has to be.

Not a big fan of "this is not a real unit test", it reeks of no true Scotsman, and waving a dictionary around never helped anyone (outside the occasional game of scrabble). shrug ;)

Member

kurtschelfthout commented Nov 8, 2015

My opinion on all that is that any test is better than no test, and so there's not reason to make testing harder than it has to be.

Not a big fan of "this is not a real unit test", it reeks of no true Scotsman, and waving a dictionary around never helped anyone (outside the occasional game of scrabble). shrug ;)

@ploeh

This comment has been minimized.

Show comment
Hide comment
@ploeh

ploeh Nov 8, 2015

Member

It was never my intention to turn this into a discussion about what a 'real' unit test is; I genuinely didn't understand what the point was - perhaps because I sometimes take too narrow a view on things.

I can't say that I fully understand, but I think I understand when it would be useful to support async tests.

Thank you for making me a little less stupid 😄

Member

ploeh commented Nov 8, 2015

It was never my intention to turn this into a discussion about what a 'real' unit test is; I genuinely didn't understand what the point was - perhaps because I sometimes take too narrow a view on things.

I can't say that I fully understand, but I think I understand when it would be useful to support async tests.

Thank you for making me a little less stupid 😄

@Porges

This comment has been minimized.

Show comment
Hide comment
@Porges

Porges Nov 8, 2015

Another reason is that Xunit supports Task-returning methods for [Fact] or [Theory] tests, and it's unfortunate to not have it for [Property] tests as well 😃

Porges commented Nov 8, 2015

Another reason is that Xunit supports Task-returning methods for [Fact] or [Theory] tests, and it's unfortunate to not have it for [Property] tests as well 😃

@bradphelan

This comment has been minimized.

Show comment
Hide comment
@bradphelan

bradphelan Sep 23, 2016

Any progress on this. Just tried the below as an experiment

[Property()]
public async Task IntersectingLineWithPlaneShouldReturnIntersectionPoint(Vector3 position, Vector3 forward, Vector3 up)
{

    await Task.Delay(TimeSpan.FromMilliseconds(10));
}

and got

FsCheck.Xunit.PropertyFailedException

Falsifiable, after 1 test (0 shrinks) (StdGen (1120699755,296208179)):
Original:
(<0.5, 0.333333333333333, 0.25>, <0.4, -0.454545454545455, 0.166666666666667>,
<-0.666666666666667, 0.666666666666667, 0.25>)

Exception doesn't have a stacktrace

System.Exception
No instances of class FsCheck.Testable+Testable`1[a] for type System.Threading.Tasks.Task
   at FsCheck.TypeClass.GetInstance@156-1.Invoke(String message) in C:\Users\Kurt\Projects\FsCheck\FsCheck\src\FsCheck\TypeClass.fs:line 156
   at FsCheck.TypeClass.GetInstance@145.Invoke(Type instance) in C:\Users\Kurt\Projects\FsCheck\FsCheck\src\FsCheck\TypeClass.fs:line 156
   at FsCheck.Common.f@1[a,b](IDictionary`2 memo, FSharpFunc`2 f, a n, Unit _arg1) in C:\Users\Kurt\Projects\FsCheck\FsCheck\src\FsCheck\Common.fs:line 25
   at FsCheck.Common.memoizeWith[a,b](IDictionary`2 memo, FSharpFunc`2 f, a n) in C:\Users\Kurt\Projects\FsCheck\FsCheck\src\FsCheck\Common.fs:line 20
   at FsCheck.Testable.evaluate[a,b](FSharpFunc`2 body, a a) in C:\Users\Kurt\Projects\FsCheck\FsCheck\src\FsCheck\Testable.fs:line 161

Any progress on this?

bradphelan commented Sep 23, 2016

Any progress on this. Just tried the below as an experiment

[Property()]
public async Task IntersectingLineWithPlaneShouldReturnIntersectionPoint(Vector3 position, Vector3 forward, Vector3 up)
{

    await Task.Delay(TimeSpan.FromMilliseconds(10));
}

and got

FsCheck.Xunit.PropertyFailedException

Falsifiable, after 1 test (0 shrinks) (StdGen (1120699755,296208179)):
Original:
(<0.5, 0.333333333333333, 0.25>, <0.4, -0.454545454545455, 0.166666666666667>,
<-0.666666666666667, 0.666666666666667, 0.25>)

Exception doesn't have a stacktrace

System.Exception
No instances of class FsCheck.Testable+Testable`1[a] for type System.Threading.Tasks.Task
   at FsCheck.TypeClass.GetInstance@156-1.Invoke(String message) in C:\Users\Kurt\Projects\FsCheck\FsCheck\src\FsCheck\TypeClass.fs:line 156
   at FsCheck.TypeClass.GetInstance@145.Invoke(Type instance) in C:\Users\Kurt\Projects\FsCheck\FsCheck\src\FsCheck\TypeClass.fs:line 156
   at FsCheck.Common.f@1[a,b](IDictionary`2 memo, FSharpFunc`2 f, a n, Unit _arg1) in C:\Users\Kurt\Projects\FsCheck\FsCheck\src\FsCheck\Common.fs:line 25
   at FsCheck.Common.memoizeWith[a,b](IDictionary`2 memo, FSharpFunc`2 f, a n) in C:\Users\Kurt\Projects\FsCheck\FsCheck\src\FsCheck\Common.fs:line 20
   at FsCheck.Testable.evaluate[a,b](FSharpFunc`2 body, a a) in C:\Users\Kurt\Projects\FsCheck\FsCheck\src\FsCheck\Testable.fs:line 161

Any progress on this?

@kurtschelfthout

This comment has been minimized.

Show comment
Hide comment
@kurtschelfthout

kurtschelfthout Sep 23, 2016

Member

No, probably not happening until 3.0.

Member

kurtschelfthout commented Sep 23, 2016

No, probably not happening until 3.0.

@kurtschelfthout kurtschelfthout added the v3 label Feb 6, 2017

@kurtschelfthout

This comment has been minimized.

Show comment
Hide comment
@kurtschelfthout

kurtschelfthout Sep 26, 2017

Member

This is now in pre-release in NuGet in 3.0-alpha1: https://www.nuget.org/packages/FsCheck/3.0.0-alpha1

Please try it out and open new issues in case of problems.

Member

kurtschelfthout commented Sep 26, 2017

This is now in pre-release in NuGet in 3.0-alpha1: https://www.nuget.org/packages/FsCheck/3.0.0-alpha1

Please try it out and open new issues in case of problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment