SpecFlowOSS / SpecFlow

#1 .NET BDD Framework. SpecFlow automates your testing & works with your existing code. Find Bugs before they happen. Behavior Driven Development helps developers, testers, and business representatives to get a better understanding of their collaboration
https://www.specflow.org/
Other
2.24k stars 754 forks source link

Windsor DI plugin cannot inject ISpecFlowOutputHelper #2550

Open JoshKeegan opened 2 years ago

JoshKeegan commented 2 years ago

SpecFlow Version

3.9.40

Which test runner are you using?

xUnit

Test Runner Version Number

2.4.1

.NET Implementation

equal or greater .NET Framework 4.6.1

Project Format of the SpecFlow project

Sdk-style project format

.feature.cs files are generated using

SpecFlow.Tools.MsBuild.Generation NuGet package

Test Execution Method

Visual Studio Test Explorer

SpecFlow Section in app.config or content of specflow.json

No response

Issue Description

ISpecFlowOutputHelper cannot be accessed via DI, so you have to use ScenarioContext.Current.ScenarioContainer.Resolve<ISpecFlowOutputHelper>(), which is marked as obsolete. This is the same for other things that with BoDi you could inject, e.g. ScenarioInfo, and looking at the source code it also looks like it would be the case for the Autofac plugin too as there is no registration of these in either DI container plugin.

As a workaround, when wiring up your windsor container you can register this manually yourself like so:

container.Register(Component.For<ScenarioInfo>().Instance(ScenarioContext.Current.ScenarioInfo));

Which then allows for it to be accessed via DI throughout the rest of your tests and is what I am doing as a workaround, but it would be nice if these were registered with the container by default like they are with BoDi.

Steps to Reproduce

Link to Repro Project

No response

JoshKeegan commented 2 years ago

Update: I was looking at the windsor plugin code and saw this: https://github.com/SpecFlowOSS/SpecFlow/blob/edf21f7b2d48c3335d89b59b0f92d4dd2619956a/Plugins/SpecFlow.Windsor.SpecFlowPlugin/WindsorPlugin.cs#L61

So the ScenarioContext itself does get registered with windsor, just not all of the other things you might be able to inject with BoDi such as ISpecFlowOutputHelper that I was originally trying to use.

So I don't know if this is a bug any more or just a feature request, but it would still be good if there was parity around what is available for injection between each of the supported DI containers.

SabotageAndi commented 2 years ago

So when you are using another DI container than BoDi, not everything is using the another DI container. SpecFlow itself is still using BoDi internally. Only the binding classes are resolved via the other DI container. We would need to register everything from the SpecFlow infrastructure to the other DI container, that everything is working. Not sure if this makes more problems.

But as you have the ScenarioContext injected, you can get it via Windsor, and then call Resolve on that instance. So you don't need ScenarioContext.Current but get the correct ScenarioContext instance.

JoshKeegan commented 2 years ago

Yeah, I found out that I can inject ScenarioContext earlier and it still work so it's not really a big deal to me now but is still a nice to have.

I hadn't realised BoDi is still used by Specflow internally, but looking at the WindsorPlugin again I see that. Getting all of those dependencies available in Windsor could get a bit messy. I wonder whether it might actually be easier to write a decorator for the IWindsorContainer that could resolve from both the underlying BoDi and Windsor containers?

SabotageAndi commented 2 years ago

I am not sure how to make this part of SpecFlow better at all. Having instances of classes registered in multiple containers which probably have different behavior is giving me headaches alone when I think about it.

JoshKeegan commented 2 years ago

I had a go at a PoC for this here https://github.com/JoshKeegan/SpecFlow/commit/6ef763fb0dd84b1da70e9462c856951d8ca7a29f

My idea was to inject a custom implementation IWindsorContainer that resolves from both Windsor & BoDi so that we wouldn't need to re-register everything in BoDi with windsor too.

The general idea seems good and the existing tests pass, as well as a new one I added to resolve ISpecFlowOutputHelper since that's an example of something that currently gets missed. However, many of the .Resolve(...) methods available on IWindsorContainer have no equivelant in BoDi so I've had to make those not check BoDi at all which I don't like the sound of.

Having done that though, I noticed the WindsorTestObjectResolver and the ITestObjectResolver it implements. @SabotageAndi do all resolutions for bindings go via this? If so then a small change here could have it fall back to resolving via BoDi if it can't find the type in windsor.

JoshKeegan commented 2 years ago

I came back to this today and looked at ITestObjectResolver again. What I was thinking of when I wrote my previous comment won't work as even if we try and resolve a binding instance from both Windsor & BoDi, if that type isn't registered and it needed to create it from it's ctor it's possible for neither instance to know about all of the dependencies of that type.

So, I'm now in the same position as @SabotageAndi and I don't see any simple way to fix this. It could still be fixed by doing something like abstracting away the IoC container used within SpecFlow & providing an interface that all supported IoC containers could implement within their plugins, but that's not a simple task and for my problem it's not worth the time. I can just keep using the workaround.

Maybe it should just be documented as a limitation when using IoC plugins? It could always be revisited if there was a major rewrite of SpecFlow or there was more of a need for it, but I don't see it worthwhile at the moment.

SabotageAndi commented 2 years ago

Maybe it should just be documented as a limitation when using IoC plugins? It could always be revisited if there was a major rewrite of SpecFlow or there was more of a need for it, but I don't see it worthwhile at the moment.

I thought about this in the last few days. I am not even sure if it is a good idea, to make the complete DI part pluggable so that you can use any DI library. All these libraries behave slightly differently so no idea in which problems we would run with some strange combinations in the core of SpecFlow. Testing SpecFlow for various .NET implementations and unit test frameworks is already very cumbersome. This would add a new axis to it. This would explode the test run time.

And I have no desire to have a lot of DI library quirk hacks in SpecFlow.

So there has to be a very good reason to persuade me to this.