Reqnroll
Call for action: review/try new dependency plugin design #472
Replies: 5 comments 8 replies
-
|
Tagging users who had related issues recently: @304NotModified @olaviedoc @justhoma @13dante04 @padnevici @vinnyrose @StefH @sanjeev-chevireddy @mbhoek @ajeckmans @mcraa @rban-newday @e317855 @Antwane @aoletubo @AhmedElbaik @RicardoLans |
Beta Was this translation helpful? Give feedback.
-
|
Cool stuff! So why do we need an attribute and inheritance? That 's a bit strange IMO. I think that's feedback topic 1 Im also not sure if we should support multiple DependencyConfigurations (with ordering etc). In the past I needed a different servicecollection for another (specflow) feature. It that maybe the idea? I assume that the abbreviation MSDI is temporary. I never heard of that abbreviation (: Im on mobile so havent checked all code. Do we really need a regex? (Inside MsdiDependencyConfiguration)
Is one of the reasons, the lack of (unit) tests for the plugins? It would be great if the new plugin is easy to unit test to prevent bugs. |
Beta Was this translation helpful? Give feedback.
-
|
Thx for the valuable feedback!
Good point. I did not think about that too much, but I think my decision was because "just having a class somewhere that implements an interface" feels a bit too implicit. But I'm open to get rid of the attribute. Would be glad to hear others options on this as well.
I think my original intention has been changed, so this can be removed. I was thinking on situations when you have configurations in multiple referred binding assemblies and you want to choose from that. But finally it searches in the test assembly only anyway, so this risk is not valid anymore. Obviously this might be related to your first comment. Because if we want to support multiple, we need metadata for the selection so that would imply that we need the attribute.
This is tough, because this library does not have a proper name. I was not coming up with MSDI (if you google for
😎 The regex is used only to give better suggestions in the error message if possible. So basically the error wrapper tries to guess the problematic interface name from the exception that comes from the DI. But this is just a convenience, if it could not find anything it uses a generic placeholder. (The exception from the DI is always added as an inner exception of course.)
That is definitely part of it, although the reason for designing the new model was not that, but that the usage, the functionality and the inner structure of them were quite different and not easily extensible. (The concept of expecting the user to attribute methods and guessing the proper signature (which might even change with versions) for it was a very bad idea (of me)... Mea culpa. 😁)
Definitely. Once it gets into the codebase we need to cover this with unit tests. Basically this samples in the repo serve only as a basic "smoke test", so we would probably include them to the |
Beta Was this translation helpful? Give feedback.
-
Same name, different namespace sounds good. Also the plugins will be in different nuget packages? So no clash if even in the same namespace. (Not saying that's a good idea)
If there are unit tests for these cases we could maybe fix it without regexes. Regexes give sometimes some security headaches and avoiding is sometimes better.
Yes me too :) |
Beta Was this translation helpful? Give feedback.
-
|
I have tested your prototype with in our codebase using MSDI. I can confirm that the singleton services are now disposed of properly when the test run ends (issue #465). FeedbackStep contextSince the Would it be possible to add the Old code[AfterStep]
public async Task AfterStep
(
ScenarioContext scenarioContext
)
{
var scenarioStepContext = scenarioContext.StepContext;
Debug.WriteLine($"After step: {scenarioStepContext.StepInfo.Text}");
}New code[AfterStep]
public async Task AfterStep
(
ReqnrollServiceHandle<ScenarioContext> scenarioContextHandle
)
{
var scenarioStepContext = scenarioContextHandle.Instance.StepContext;
Debug.WriteLine($"After step: {scenarioStepContext.StepInfo.Text}");
}
|
Beta Was this translation helpful? Give feedback.
-
|
I've just a quick remark / feedback. I did some quick prototyping and testing with your project, but could not get it the The public class Calculator(ILogger<Calculator> _logger, ICalculatorConfiguration _calculatorConfiguration) : ICalculatorAlso it seems that the ambient Reqnroll context accessor is not completely accessible in every stage of the test. But probably I missed some knowledge on this part. |
Beta Was this translation helpful? Give feedback.
-
|
Thx. The logger stuff is still to be done, indeed.
Could you please give an example of what you want by that? |
Beta Was this translation helpful? Give feedback.
-
|
My initial thoughts (a more in-depth review is deserved and pending): Dependency RegistrationRequiring an attribute is probably better than requiring inheritance, because it's directly descriptive of the intent. You can't "accidentally" pick up an unintended behaviour by inheriting from a class or implementing an interface. If we're looking for an attribute, inheritance or implementation of an actual interface should probably not be necessary; if we're invoking by reflection we are effectively doing so by convention and therefore can free users from the need to include the interface (or indeed for us to define it). However, this leads me to the thought that perhaps it's due time we created a proper "startup" routine for Reqnroll. Reqnroll StartupIn the same way that a console app has an entry point in Using just BoDi: Or with MSDI It's a system that's invoked by convention - just having the type with the name and the signatures is sufficient. The advantages of this are longer-term than simply DI, giving space to register all kinds of potential customisations. It opens the possibility of making customisations without the need for creating full plugins. Any time ordering matters, we now have a trivial place to register a sequence/chain of components that makes order an explicit part of registration. Perhaps most promising of all, this class could be invoked by tooling or other systems to gain access to a more complete model of the user's intentions. Want to supply configuration from environment variables? Our generators and IDE integration can invoke the same configuration method to build their own internal models. Microsoft Dependency Injection LimitationsSince there's a "special" syntax required for the registration of objects, we should consider creating new extension methods to encapsulate this help extend our terminology to those using it: It's an opportunity to add documentation in a place most users will encounter it (intellisense) to help explain what each registration method does and what scopes exist and helps "smooth over" the pain-points. |
Beta Was this translation helpful? Give feedback.
-
Agree with this. I'm in favor of an attribute and an optional inheritance/interface implementation |
Beta Was this translation helpful? Give feedback.
-
I really like this syntax :) But in the MSDI is by convention Add... So for MSDI services.AddTestRunScoped<MyService>();
services.AddFeatureScoped<MyService>();
services.AddWorkerScoped<MyService>();Not sure what is the convention with AutoFac. But I guess it not required to be same. |
Beta Was this translation helpful? Give feedback.
-
|
I really like your suggestion, it gives much more control and flexibility (and it's a pattern proven to work by ASP.NET etc.). |
Beta Was this translation helpful? Give feedback.
-
|
One thing though; we will probably run it to problems with the new suggested extension methods ( |
Beta Was this translation helpful? Give feedback.
-
|
I don't believe it's reasonable to have a set of dependency registrations produce a compatible suite of services between two different service contexts when one of those contexts has to work alongside the Reqnroll infrastructure and its specific lifecycle. If your services can run without a dependency on the Reqnroll library, they probably shouldn't be included in the Reqnroll service container and isolated to their own service container. It still breaks a lot of expectations to do this: there are no nested scopes with Microsoft's DI implementation and we created a scope for every test run. If your service registers a singleton, you'd get one for the whole test run. |
Beta Was this translation helpful? Give feedback.
-
We have quite many issues with our DI plugins for various reasons, and the different plugins solve the same problems differently that makes the maintenance harder.
With this motivation, I worked on a prototype to see the new concept.
I would like to ask you to review the concept (see details below) or even more, try it out in your own project! Any feedback is appreciated!
If the feedback is positive, the new design model will replace the existing plugins, but we will somehow make the existing plugins also available for a while to make the transition easier.
This is a long post, but I encourage you to scan it through to get a first impression and understand the concept. I also have a section at the end with the most important topics where I would like to get feedback.
Prototype structure
Support/SetupTestDependencies.cs).ToReqnrollfolder - these are the codes that would eventually be part of the Reqnroll codebase. This is what you need to copy to your own project if you want to test the new design.ToReqnroll/Runtime) are stored only in theReqnrollPlugins.DependencyInjectionproject and linked to the other projects to avoid duplication.Sample test
Calculator.feature,CalculatorStepDefinitions.cs), a test that uses differently scoped dependencies (StepDefinitionsWithDependencies.cs, Scenario A, B, D, E inFeature1.featureandFeature2.feature) and a test that uses Reqnroll infrastructure (StepDefinitionsWithReqnrollDependencies.cs, Scenario C, F inFeature1.featureandFeature2.feature).ICalculatorandCalculator) that has a dependency toICalculatorConfigurationwhich have a test-only implementation:TestCalculatorConfiguration. The idea is that the calculator instance should be created per scenario (scenario scope), but theTestCalculatorConfigurationshould be singleton (test-run scope).Usage model in the new design
The dependency configuration model has been somewhat unified. For each DI framework, you have to define a class (
SetupTestDependenciesin the sample) and mark it with the[DependencyConfiguration]attribute. You need to use a base type, depending on the DI framework and override methods. E.g. useAutofacDependencyConfigurationfor Autofac.This class contains all dependency configuration details.
Optionally you can pass an "options" object (e.g.
AutofacDependencyConfigurationOptionsfor Autofac) where some framework specific options can be set. The only interesting is theAutoRegisterBindingTypes(default: true) that controls whether all "binding" class should be automatically registered (in some of the old plugins this had to be done manually).Usage model for Autofac
Configuration base class:
AutofacDependencyConfigurationFor Autofac, the main method you need to override is the
SetupServicesmethod, where you can configure all dependencies (it receives aContainerBuilderinstance). There are different scope helper methods (ScenarioScope,FeatureScope, etc.), to define the lifetime of the particular objects.The usage is just straightforward, no special consideration needed.
There is an alternative way of configuring the scopes: you can optionally override the
SetupScenarioScope,SetupFeatureScope, etc. methods, to provide registrations right to those scopes. (The end result is the same.):Usage model for MSDI
Configuration base class:
MsdiDependencyConfigurationThe Microsoft's DI framework was originally designed to serve ASP.NET MVC apps and therefore limited in a few ways. There are two key limitations that impacts our usage:
At the end the prototype shows a model where using test-run and scenario dependencies (the two most commonly used) is quite straightforward, but for the feature and the worker scopes you need special usage and also special usage needed for some Reqnroll infrastructure.
Just like with Autofac, you need to start by overriding is the
SetupServicesmethod, where you can configure all dependencies (it receives aServiceCollectioninstance). For the two common scope, the usage is simple: useAddSingletonfor test-run andAddScopedfor scenario dependencies:For the others, you need to use wrapper classes both for registration and for usage. For example to use something in feature lifetime, you need to register it in two steps:
If you want to use this dependency, you need to depend on
FeatureScopeService<IFeatureDependency>:When accessing Reqnroll dependencies, some, like
IReqnrollOutputHelperworks just fine. However, for some other you need to change the usage. For example, you cannot directly depend onScenarioContext(because MSDI would dispose it), but you have to depend onIScenarioContextinstead. (Similarly withFeatureContext/IFeatureContext,TestThreadContext/ITestThreadContext). This is a limitation that we could later eliminate.Using the interface instead of the class is enough for most of the cases, but if not there is always a backdoor: you can depend on
ReqnrollServiceHandle<ScenarioContext>instead. This is a wrapper that protects the ScenarioContext from being disposed by MSDI. The usage is like:scenarioContextHandle.Instance.ScenarioInfo.Title.Usage model for BoDi
Configuration base class:
ReqnrollDiDependencyConfigurationThe dependency configuration with BoDi is also pretty straightforward. Here you can override any of the following methods:
SetupScenarioScope,SetupTestRunScope,SetupFeatureScope,SetupWorkerScopeand configure your dependencies as needed. The calculator sample config would look like this:The usage is just normal, no tricks. Obviously the same for Reqnroll dependencies.
Implementation details
I have added quite some comments to the classes in the
ToReqnrollfolder, so I do not want to go through that here, just a few notes:ITestObjectResolverinterface (like the old plugins did), but this does not have to be done for each DI framework, but the implementation scans through the test project for a[DependencyConfiguration]attribute and uses that class. If there are multiple classes marked, there is an optionalPrioritysetting that is used to pick one.[DependencyConfiguration]attribute must implement theIDependencyConfigurationinterface (the base classes of the known DI frameworks do that). This is an extremely simple interface, it has basically only a singleTryResolvemethod (and one for error messages). This might reduce the overhead of implementing a new configuration class for a new DI framework. You do not even need to make a Reqnroll plugin for that.ICalculatorregistration!)DependencyConfigurationOptionsclass that is the base class of all options class.[DependencyConfiguration]attribute it falls back to the default behavior. We can later improve this in a way that if the plugin is used but the[DependencyConfiguration]attribute is not, we throw an error with some details on how to start.Feedback questions, limitations
I tried to make the prototype "feature complete", although the code might need a few smaller adaption once it will be merged. You can give feedback about anything, but I am primarily interested to get feedback from the user perspective.
Some detailed feedback topics:
InitializeXXXScopemethods (e.g.InitializeScenarioScope). These methods are called after the particular scope has been built (so you cannot config anymore), but before they get used. They could be used to initialize some dependencies in the scopes (e.g. initialize a cache in feature scope). Does that make sense? Do you have any good use-case for this?DependencyConfigurationorSetupServices)Microsoft.Extensions.DependencyInjection. (What should we use instead in the class names?)DependencyConfiguration(orDependencyConfigurationBase) in each plugins (in different namespaces of course)?One limitation is that the logging support from @StefH is not yet integrated to the prototype. (Related issue: #484)
Beta Was this translation helpful? Give feedback.
All reactions