Support async steps

[adamralph]

To allow async step definition, e.g.

"Given foo"
    f.(async () => await Foo());

[adamralph]

depends on #51

[adamralph]

Initial tests seem to indicate this may already 'just work'...

UPDATE

It doesn't - https://twitter.com/dschenkelman/status/415957643354574848

[adamralph]

Taken by @dschenkelman

[dschenkelman]

I have an initial version working at https://github.com/dschenkelman/xbehave.net/tree/issues/25.

This is still missing support for Given, Then, When, etc. methods, but I thought I ought to share the approach ASAP to get feedback.

I've been going over the XUnit code and the approach they are using for commands related to Task returning methods is implemented by ReflectionMethodInfo::Invoke(object testClass, params object[] parameters) (code here, line 138).

Most of their additional work is related to rethrowing the correct exception, which is something we eventually will want to achieve, but might not be necessary to get done for the initial merge since having async/await support is an improvement in an of itself.

Thoughts?

[JakeGinnivan]

FYI here is my pull request which adds async support to BDDfy. TestStack/TestStack.BDDfy#32

Feel free to ping me with any questions you guys have while implementing this

[adamralph]

Thanks @dschenkelman. Did you actually push your changes? I see your issues/25 branch sitting on latest master.

[adamralph]

Latest dev, that is.

[dschenkelman]

It's in now. Forgot to git stash pop.

On Mon, Jan 6, 2014 at 3:23 AM, Adam Ralph [email protected] wrote:

Latest dev, that is.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/25#issuecomment-31641994
.

[adamralph]

Great, I'll take a look

[adamralph]

@dschenkelman I've taken a look at your issues/25 branch it looks great.

I agree that the exception stuff can come later.

One observation (and I realise it's just a work in progress) is that instead of base(name, () => { }, stepType) in AsyncStep I wonder if we should change Step to being abstract Step<TDelegate> and then have SyncStep : Step<Action> and AsyncStep : Step<Func<Task>>?

Also, I really like the way you've implemented the timeout in AsyncStep. I think we could just reuse this in the sync implementation by wrapping body() in a task. If we took the abstract base approach then the only thing that the inheritors would need to provide is a way to get a Task from each of their respective body fields. I think that could be very elegant indeed. Mind you if we did this, we'd need to build in the exception stuff straight way, otherwise we'd start getting AggregateException from the sync implementation.

[dschenkelman]

@adamralph I like the idea of having Step<T>. I did not like the () => {} creation but left it there for the time being, so your feedback is really appreciated.

Regarding the timeout matter, I'll give. The only case where I'm not sure if it will work is for async void methods. I still need to figure out how the SynchronizationContext works (I only know the theory), but calling Task.Run to wrap body() might not work well with that together.

If we discard async void methods the idea would definitely work, but we might need to change it once we want to add support for them.

[adamralph]

How would async void methods fit into the syntax?

[JakeGinnivan]

The way to support async void is:

1. Check if the method you are executing has the AsyncStateMachineAttribute attribute (means that method is async/void or async/Task)
2. If it does, and the method does not return a task (is async void) then instead of running the function directly, you should use code like this - https://github.com/TestStack/TestStack.BDDfy/pull/32/files#diff-e81ea45899904ca160fd6b8970d7b793R43
   • The code above sets a custom sync context which gets notified whenever an async void method starts executing, then gets notified when it completes. You can cound the start/finish pairs, when it gets to 0 again, the async void method is done.
   • The code uses a sync handle to block until the async void method is done.

Few things, if you want to rethrow the inner exceptions from the AggregateException you can use https://github.com/TestStack/TestStack.BDDfy/pull/32/files#diff-a8d5eed69362f140514897b312913231R78 to preserve the stacktrace before you rethrow.

https://github.com/TestStack/TestStack.BDDfy/pull/32/files#diff-a8102fea6ecd77a3994fa2e6dcf9f2d2R1 is the synchronisation context I took from xUnit 2.0 for use in BBDfy

Hope that helps :)

[adamralph]

@JakeGinnivan that's awesome info, thank you!.

@dschenkelman I'm still struggling to see where we need support for async void in xBehave. With your WIP extensions we now support

"When bazzing"._(async () => await Bar.Baz());

is there something else we need to support which I'm missing?

[JakeGinnivan]

"When bazzed"._(Bar.OnBaz()); //async void OnBaz(object sender, EventArgs e)

?

[JakeGinnivan]

@adamralph how do you get syntax highlighing? :P

[adamralph]

Aha! Thank you sir!

Syntax highlighting:

my code here

[dschenkelman]

@JakeGinnivan thanks for explaining! you did a much better job than I would have :)

@adamralph as Jake said, I think we should support:

"When bazzed"._(Bar.OnBaz); //async void OnBaz()

[adamralph]

Most of the work is now done but https://github.com/xbehave/xbehave.net/wiki/Object-disposal does not work currently for async steps. Object disposal uses a thread static collection to collect the disposable objects which are registered within a step body with the Using() extension method. Since async steps run on their own thread, after the step has completed there's no way for the AsyncStep thread to get a handle on those objects.

This is a tough one. If we cannot use the thread to correlate the objects with a scenario then the only way I can think of is to have overloads of the step extension methods and Using() which accept a context object and deprecate the parameterless Using(), e.g.

"Given a DB connection"
    ._(async c => connection = await GetConnectionAsync("foo").Using(c));

"And another DB connection"
    ._(c => connection2 = GetConnection("foo").Using(c));

Although the context object isn't needed in the above sync step, it would be confusing and ineffective to attempt to dictate the use of the parameterless Using() in a sync step but Using(context) in an async step.

Also, if scenario authors are registering disposables object in other threads within a sync step, those registrations are also being lost. If the parameterless Using() is deprecated and later removed then they will be forced to use the context for correlation and this problem will be avoided.

It's also worth noting that this problem exists right now in 1.0 for steps which have timeouts attached to them. In these cases, we also invoke the step in a separate thread so objects registered with Using() are not recognised. I've raised #135 to track this.

[adamralph]

@dschenkelman I've started putting together a solution for the object disposal in https://github.com/adamralph/xbehave.net/tree/25 which introduces the step overloads with the context object which I mentioned above. At the moment I can't think of a better solution. Unless anyone can come up with a better idea, it shouldn't take me long to finish up that work and then we can push out the async feature in 1.1.

[adamralph]

The .net 4.5 assemblies still need to be added to the NuGet package.

[adamralph]

https://www.nuget.org/packages/Xbehave/1.1.0
https://github.com/xbehave/xbehave.net/releases/tag/1.1.0

Thanks very much for the PRs @dschenkelman and thanks @JakeGinnivan for the valuable input. Look out for your names in the release notes 🏆.

[dschenkelman]

Thanks Adam, no problem. A pleasure to help.

[JakeGinnivan]

Ditto, glad I could help out