[Discussion] Enable *Context.Current in multi thread execution

[SabotageAndi]

This enables Scenario/Feature/ScenarioStep- Context.Current in multithreaded execution.

Currently it has the limitation, that it only works when you use it in the test assembly (the one with the features).
The reason is, that the TestRunnerManager is registered with test assembly as key in the testRunnerManagerRegistry. This is, because Assembly.GetCallingAssembly() in TestRunnerManager.GetTestRunner(...) is called in FeatureSetup of the Code-Behind-Class in the feature file.

*Context.Current is now implemented, that it tries to get the testRunner also with Assembly.GetCallingAssembly().

The only idea I have currently to solve this is, to look at the CallStack (new StackTrace().GetFrames) and search the test assembly in it.
Does somebody have a better idea? @samholder @darrencauthon @dirkrombauts @gasparnagy

[SabotageAndi]

@gasparnagy
Why ist the testRunnerManagerRegistry actually needed?
It would be needed if a testrunner loads multiple specflow test dlls in one run, or?
Does this some testrunner?

[gasparnagy]

@SabotageAndi yes. R# loads multiple test assemblies to the same appdomain for example.

In general: I don't think any of the two proposed solution would be good. The assembly-based solution is not good in parallel execution as there are multiple test runners bound to the same assembly.
The call stack option would not work, because from the stack trace you don't see the thread... also ScenarioContext.Current might get called from also from a new arbitrary thread of the test...

I see two real options:

1. we really don't solve this, but for example ask the users to inherit from the "Steps" base class that solves the "complexity" of injection and provides the current SC as a property.
2. we provide a functionality that generalizes [ThreadStatic] and/or SynhronizationContext in a way that we can figure out what is the best way of accessing the right current context depending on the situation. But we should find a solution that also works with async, where there are multiple physical threads playing together to implement a logical test thread.

[SabotageAndi]

Ah, I see now. And I wondered why this was so easy. It was a trap ;-)
Now I am wondering, if we could solve this problem at all.

Option 2 does not look easy, specially if we want also support async.

[gasparnagy]

(inspired by @darrencauthon :-)

[darrencauthon]

I was listening to a podcast yesterday with @kytrinyx as the guest. Can't remember the name of the podcast... Anyway she said something funny that I had never heard, something to the effect of: OSS isn't free as in beer, it's free as in puppy. Because when someone adds these features, they're also asking you to maintain them forever... and those things can grow. 😄

I think that analogy fits the async features added to SpecFlow. Which is fine, and good in the long-term... it's just a case where the ability to go async is a fundamental change that is growing.

@SabotageAndi I can't give you a list of technical reasons why I don't think this is going to work like @gasparnagy (I wish I was smarter 👅)... it's like, I don't know how the stove works, but I know not to touch it when it's hot. And that's what this feature does: It's touching a hot stove. You're mixing in the "static" world of C# in our new async world. You can't mix state with static. You might be able to find some hack around it, but the both sides naturally oppose each other.

I think @gasparnagy 's suggestion of a base class is the only way to get this to work... but even then, it's not static anymore and people are going to adjust.

Crazy as this sounds, I'm imagining marking ScenarioContext.Current as deprecated, to-be-removed in SpecFlow 3.0.

[SabotageAndi]

@darrencauthon you are absolute right. This is a really hot stove and as said, I now think that we are not able to implement this.
I have no clear opinion about the base class, but I like the idea to deprecate the *Context.Currents!

[samholder]

+1 for deprecation. We never use them anyway 👅

[gasparnagy]

BTW: it looks like i have seen a future at some point, just have forgotten about it (does this make sense?). Anyway the "Steps" base class already has a ScenarioContext property... https://github.com/techtalk/SpecFlow/blob/master/Runtime/Steps.cs#L27

So you can taste it

[darrencauthon]

Oh I can taste it!

Given that, perhaps we could go the route of deprecating ScenarioContext.Current? It'd have to be far off in the future, on the next major release -- because this will certainly be a breaking fix. But inheriting from a new base class and replacing ".Current" with nothing seems like a manageable change.

Or... here's another thought. Maybe we could keep ScenarioContext.Current, and just let those that use it, use it?

Look, I've been using dependency injection for many years, I test my apps, all that stuff. I understand why singletons can be very bad, and wiring up ScenarioContext.Current means your entire test suite is not safe to go async. But is that a problem for those that plan to run their tests synchronously? (which, BTW, I bet is the vast majority of SpecFlow users?)

I am a little concerned that the async changes may start to bleed through the framework, far beyond their internal implementation.

[jmezach]

As a SpecFlow user I thought I'd pitch in on this issue. I would definitely be in favor of deprecating (and eventually removing) the *Context.Current properties and here's why:

When I first started out using SpecFlow on a project we were using those properties all over the place. It wasn't until I attended the excellent SpecFlow training by @gasparnagy when I found out that there was built-in dependency injection and I saw some clever ways of using it. Since then I've been using that exclusively, instead of the contexts and it just leads to better maintainable test code.

So if you're still deciding, here's a +1 for deprecating it all together and allow SpecFlow users to just write better test code.

[SabotageAndi]

Closing this PR, because this will not be happening.
*Context.Current will get obsolete after merging of #779 & #679

[Andras-Csanyi]

Hi All,

Would it be possible to update the documentation accordingly? I spent a few hours recently to investigate non-deterministic behavior of my code. Fortunately, I met a few defects caused by mixing sync-async code.

[SabotageAndi]

@SayusiAndo Did you had a look at the Parallel execution page in the documentation? http://specflow.org/documentation/parallel-execution/

[Andras-Csanyi]

I'm sorry, it's my bad. I did not read the page further than constructor injection.