Support for async steps

[michalkovy]

This change adds support for steps that return:

• Task
• Task<Unit>
• Task<SomeReturnValue>
• Async<Unit>
• Async<SomeReturnValue>

the implementation is currently simple, it wraps it into Async.RunSynchronously. We could later support async all way into test frameworks but that is out of scope of this change.

[bartelink]

🤔 does this have any purpose if it's not being surfaced all the way up to the xunit async/task ? I'd tend to do a spike making the main runner API async all the way to the top and seeing how messy that gets before introducing the complexity of sync over async. (my thought is tempered by it not touching many things, having examples and being small, I must admit)

[michalkovy]

@bartelink The purpose of this is to allow testing asynchronous code without need make steps synchronous. Basically the author of BDD can write asynchronous call all way up in his library without Task.Wait or Async.RunSynchronously which makes the code nice and good for reusing e.g. in F# scripts (and without deadlocks).
It is very similar to the reason why test framework support async methods that return Tasks. It isn't e.g. to get performance, the reason is to allow to write unit tests which uses async methods easily (and so avoid test projects to contain things like Task.Wait)

[bartelink]

OK, I accept the purpose - but does it have any point ;)
A lot of the point of supporting Asyncness in xUnit is actually the efficient parallelism ti enables for stuff that's async vs blocking a thread per running test at all times.
An equivalent example is that FsCheck.xUnit does not support Async properties and won't until it's a first class impl - you're forced to put in the Async.RunSnchronously to acknowledge that this is a place where you're giving up the asynchrony.
While I would not 100% against merging this, I feel doing the full job (and considering how much upheaval/complexity that would introduce) should be considered there; if it can, it actually is a very valuable thing to be able to do (🤔 does SpecFlow do this yet?).

[michalkovy]

@bartelink The main point for nearly all unit testing frameworks to introduce async test is really just to support testing using async APIs. See the introductory posts for individual framework:

• xUnit: http://bradwilson.typepad.com/blog/2012/01/xunit19.html
• NUnit: http://simoneb.github.io/blog/2013/01/19/async-support-in-nunit/
• MSTest: https://blogs.msdn.microsoft.com/devops/2012/03/13/whats-new-for-mstest-unit-tests-in-visual-studio-11-beta/

You can see that none of the blogs write that they introduced it to improve efficient parallelism. The NUnit blog even writes that they did basically the same what I did:
"Introducing this support in NUnit doesn’t necessarily mean that NUnit has now become asynchronous or that it allows to run tests in parallel. This is something which is being worked on for the next 3rd major release of NUnit though.
What we did in NUnit 2 was to allow users to write async tests without worrying about tests completing before their assertions were even evaluated, in addition to supporting thorough use of asynchronous methods in some specific framework use cases. In other words, when invoking asynchronous test methods NUnit will “sit and wait” on your behalf, until the test method, along with all the asynchronous operations it invokes, has finally completed."

FsCheck.xUnit contains async support in version 3. SpecFlow supports async steps, too.

I would like to write full async support but it really doesn't seem to me trivial today. This change fulfills the value that I am looking for.

[bartelink]

OK, thanks for taking the time to lay that out.

I'm not entirely against against merging but the points you make for me all have counterpoints:

• FsCheck 3 remains pre-release, but it does seem they are shooting for proper parallelism
• xUnit 1.9 was an MVP in 2012
• xUnit 2.x has proper deep parallelism support based on my reading of the release notes?
• if I had to work in the NUnit codebase, I'd definitely be looking to cut corners too ;)
• I assume the async steps in SpecFlow lean on sync over async ?

So, my one and only point remains this - putting support in this is not for free if nobody then completes the deal and adds proper parallelism; it adds complexity to the codebase without really unblocking anything (i.e. people can already do their own wrapping). I'm not making this point with the goal of stopping you, just to raise that there are two ways of looking at how to address this :

1. let people remove some Async.RunSynchronouslys from their code without changing semantics, but hide lots of things like propagation of cancellation and/or potential deadlocks
2. take a deeper look at the problem and see if a fully Async impl can be achieved. If it can then one'd be in a position to say "if you call ExecuteAsync, inner steps can be Async and cancellation and waits are non blocking and deadlock free; if you call ExcuteNotAsync, then inner steps may not be Async [they can of course do their own .Wait() and run risks of deadlocks/wierd semantics as desired]

If this is unblocking people, an MVP stance makes sense. If ultimately the intention is to do it all, I'm just saying "Hm, maybe it makes sense to do the hard bit first".

[mchaloupka]

I somehow agree with both of you. Have you made a research what would be needed for the final correct solution?

What it would take to implement the async properly? Probably the code should generate some IAsyncStateMachine class for every scenario. This class will contain the context (like instance provider and information about next method to call, potentially exception handling). See this:
https://weblogs.asp.net/dixin/understanding-c-sharp-async-await-1-compilation

It seems to me, that maybe the emit was taken too far. Especially for the asynchronous scenario it would make perfect sense to limit the emit code just for the usable purpose - it can generate just a small wrapper for steps, so it will be possible to debug the tests.

My question is, how hard it is to implement it properly. If it is hard can we at least update this implementation in a way that it will be somehow helpful to the final one?

[bartelink]

Re the updating FSharp.Core - is there any way to avoid that ? In general it's kinder to dual target and shim for F# pre 4.5 (or (I'm not paying complete attention) does the readme already document a min version strategy - there's definitely a case to be made for not supporting old stuff where that helps with implementation simplicity and/ot testing)

[michalkovy]

I have done some research and the proper implementation seams achievable but I don't think I will find time to make it fully any time soon. If you have time to do it within next weeks than we can skip this pull request.

However, I think @bartelink opened good question regarding how the final implementation should look like and whether this is step forward to it.

The one thing is if the code should have both asynchronous and synchronous implementation. The home page states "we want to keep TickSpec powerful, but minimal". I think if we had both variants in code properly then it would increase amount of code by large margin without adding a big value. So, my preference is to have asynchronous variant only in the future to keep TickSpec easier to maintain. What do you think about that?

If you agreed with my view then I think the steps from user perspective will look like in this pull request. I could also make the method scenario.Action.Invoke() asynchronous to make even the wiring look like in future but that would be kind of fake. Or I could even do the non-ILed version final.

The other perspective regarding proper implementation is if we want use Tasks or Async. I think we should allow both on steps (like I did in the pull request) but the method scenario.Action.Invoke() should be at the end at minimum Task based so that it can be used from C# easily, so scenario.Action.InvokeAsync(), but we could have both variants. Within the TickSpec implementation I would use what is simpler and easier to maintain. It looks to me today that Async will be simpler to generate/maintain in the genIL code even though I could make even the task based probably relatively condensed when using Task.ContinueWith. So, regardless I still feel the change in the pull request is step forward the right direction.

Regarding FSharp.Core version, I today actually don't see a reason why not to take the newest FSharp.Core which is now 4.5.2. I though that the reason we used 3.1.2.5 was to ensure that somebody can still develop TickSpec in Visual Studio 2013 so that we don't introduce F# 4 features into TickSpec code. However, the changes for .NET Core made a requirement that you need to have now a recent version of Visual Studio 2017 so you basically have even F# 4.5 in place. So, my opinion is to let's use the oportunity and when writing new code let's base on F# 4.5 - probably we will stick on that version then for some time. Btw. I was missing non-generic AwaitTask in 3.1.2.5 but I can't put work-around if necessary.

[bartelink]

Here's an AwaitTask

In general I'd favor sticking with Async for simplicity of maintenance, even if some Task tricks can wring out perf from time to time. In general, as Gherkin based tests tend to be on the acceptance level of the test triangle, I'd submit that it's pretty clear that one can afford to go Async in the engine.

Where there's a will and a need, there's normally a way to support Async and Task side by side via various forms of overloading (either by doing methods with overloading, or DU args on a function).

For me, doing multitargeting as in CallPolly seems pretty doable; There are plenty codebases out there that have tests targeting xUnit 1.9 etc, and unless it's really painful, keeping that backcompat is pretty important (and IME doing these types of things 'the right way' in a library can be a valuable exercise in terms of how to manage these same transitions at app level).

The powerful but minimal maxim is a random thought I typed out, but in general the idea of that should be that the code should remain roughly in line with how Phil implemented it - compact, not a monolith, but also not tonnes of files - this leaves it in the best place for people to grab it and do things with it; ultimately if someone can use it to learn F# techniques, fork it do something only tangentially related to running under a .NET test runner, or port to a more basic platform, that's also valuable.

[mchaloupka]

I today actually don't see a reason why not to take the newest FSharp.Core which is now 4.5.2

Well, it may not make a reason for new implementation but some implementations may use older version. It should be up to them. We should require the minimum version we really need.

So, my preference is to have asynchronous variant only in the future to keep TickSpec easier to maintain. What do you think about that?

That would be probably the biggest breaking change introduced. At the same moment, it will be somehow weird to use it from a testing framework that does not support async. At the same moment, I would be probably fine with having the synchronous method implemented using the active waiting on the asynchronous one.

the method scenario.Action.Invoke() should be at the end at minimum Task based so that it can be used from C# easily

Agree.

If you are able to prepare an implementation without the emited code. What if we emit just a dynamic method that will (as a parameter) get a method to call and its parameters? If we reduce the emit code just to that, it will be simplified while keeping the same functionality from user perspective. Integrating such an emit to the implementation should be simple. I will try to find some time to play with the emits to see whether I will be able to reduce it just to such wrappers.

[michalkovy]

Ok, I am fine waiting for somebody implementing full async support, hopefully it won't take long time