XUnit Property attribute respects IAsyncLifetime

[arialdomartini]

Here's an attempt to fix #657.

This is my first change to FsCheck code, so I will surely need a good helping hand.

The change is few lines of code:

• When the testInstance is created (in PropertyAttribute), it checks whether the test class implements IAsyncLifetime.
• In that case, it invokes InitializeAsync()

    match testInstance with
    | :? IAsyncLifetime as asyncLifetime -> asyncLifetime.InitializeAsync()
    | _ -> Task.CompletedTask
    |> ignore

The PR includes a test, based on the sample code I proposed in #657 (comment)

Please, notice that DisposeAsync is not invoked. Apparently, neither xUnit invokes it. I am not sure if this is the correct behavior, though.

[kurtschelfthout]

Don't we need to wait for the Task to complete? I think this will just start the task and your test is really fast so works, but for longer tasks it may show a problem.

E.g. if you change the test below to include a await Task.Delay(500) and only then set executed, does it still work?

[arialdomartini]

Of course, yes we need to wait. And the test is green only because it's a bad test: it checks whether InitializeAsync is invoked, not if its Task is completed. My bad. I'm on it to fix it.
Thank you for catching the problem.

[arialdomartini]

I confirm your doubt: the following test is red

type Issue657() =

    let mutable executed = false;

    interface IAsyncLifetime with
        member _.InitializeAsync() =
            async {
                do! Async.Sleep 200
                executed <- true
                return ()
            } |> Async.StartAsTask :> Task

        member _.DisposeAsync() = Task.CompletedTask

    [<Property>]
    member this.``then InitializeAsync() is invoked``() =
        executed = true

[arialdomartini]

I got to the conclusion that InitializeAsync should not be invoked when target is created - either as a Some or a None - as I initially suggested, but it's probably a better fit for Runner.checkMethod.

I would put the invocation right before invoking the test method, at

FsCheck/src/FsCheck/Runner.fs

Line 590 in 4f3f865

Reflect.invokeMethod m target o

I thought that the following could work, but apparently I'm wrong:

let invokeAndThrowInner (m:MethodInfo) o = 
    try

        match target with
        | Some (:? IAsyncLifetime as asyncLifetime) -> asyncLifetime.InitializeAsync()
        | _ -> Task.CompletedTask
        |> Async.AwaitTask
        |> Async.StartAsTask
        |> ignore

        Reflect.invokeMethod m target o
    with :? TargetInvocationException as e ->
        System.Runtime.ExceptionServices.ExceptionDispatchInfo.Capture(e.InnerException).Throw()

[arialdomartini]

I manage to have the test passing.
The invocation is performed with:

match target with
    | Some (:? IAsyncLifetime as asyncLifetime) -> asyncLifetime.InitializeAsync()
    | _ -> Task.CompletedTask
    |> Async.AwaitTask
    |> Async.StartAsTask
    |> Task.WaitAll

I am not convinced of the last Task.WaitAll. Does not it mean that the whole test is invoked synchronously?

Before, I mentioned that the method could be invoked in Runner.checkMethod. In hindsight, I believe that would not be a good idea, because the project FsCheck would gain a dependency on xUnit, which does not seem correct.

[kurtschelfthout]

Look great, thanks!

Does not it mean that the whole test is invoked synchronously?

No - the test can still start tasks as normal. The WaitAll means we wait to invoke the test until the async initialization is performed, which is the expected behavior (I would think...otherwise why have an init?).

because the project FsCheck would gain a dependency on xUnit, which does not seem correct.

Right, definitely don't want that.