Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Autofac global container #2579

Merged
merged 7 commits into from
Apr 11, 2022

Conversation

rossmasday
Copy link
Contributor

…tional Attribute and two new method signatures.

Types of changes

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).
  • Performance improvement
  • Refactoring (so no functional change)
  • Other (docs, build config, etc)

Checklist:

  • I've added tests for my code. (most of the time mandatory)
  • I have added an entry to the changelog. (mandatory)
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@SabotageAndi
Copy link
Contributor

@gasparnagy what do you say about this PR?

@gasparnagy
Copy link
Contributor

Will check it tomorrow

Copy link
Contributor

@gasparnagy gasparnagy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR, I think this is generally fine (see my two minor notes).

The only thing I was thinking of is how to make it less confusing for the users...

So far, they had to write a method that returned a ContainerBuilder and attribute it with [ScenarioDependencies].

Now if they choose the new model, they need to write a method that returns an IContainer ([GlobalDependencies]) and another void one that has a ContainerBuilder parameter.

Maybe later, we provide an option for the users to customize the test thread container and in that case, they probably need to make a third method also with a ContainerBuilder parameter.

My suggestion would be to promote the void method with ContainerBuilder parameter as a primary way:

  1. All methods they need to write ([GlobalDependencies], [ScenarioDependencies]) should be void methods with a ContainerBuilder parameter. The only exception is backwards compatibility: see below.
  2. For processing the global dependencies, we could just simply create a new ContainerBuilder and pass it to the method.
  3. For processing scenario dependencies without global dependencies, again we could just create a new ContainerBuilder and pass it to the method.
  4. Backwards compatibility: if there is no void method with [ScenarioDependencies], this means we should use the backwards compatibility mode: in this case we should not create the builder, but just call this legacy method.

The benefit of this approach would be that it is simpler -- we could even remove the "two solutions" from the documentation and only document the new one (maybe mentioning that the old still works).

As far as I see you have all the code for this, so this would only need a small change in the PR.

What do you think, @rossmasday?

Plugins/SpecFlow.Autofac.SpecFlowPlugin/AutofacPlugin.cs Outdated Show resolved Hide resolved
docs/Integrations/Autofac.md Outdated Show resolved Hide resolved
@rossmasday
Copy link
Contributor Author

I believe I have made the changes you require, I also did some refactoring and code improvements as well that should make it easier to extend for other scenarios and slightly easier to read, mainly that all finder methods are Func<ContainerBuilder, ContainerBuilder> this allows for a more fluent call signature and removes the issue where I had to return Object instead of void quirk of MethodInfo.Invoke(). It would have been cleaner to use Action<ContainerBuilder> instead but that obviously doesnt work with the legacy method and I did not want to have two FindMethods that were basically Identical.
As per your suggestion the two new methods have the same signature, the old one is still available,

I noticed because the container in the original method is not registered with the internal Specflow container, it does not appear to have it's dispose method called, so I registered it and resolved it so that process is now invoked. I hope this will not introduce any issues, I would be surprised if it did, but I can remove it too if you prefer.

I added a couple of helper extension methods as well, if you're not keen on them I am happy to remove them.

I have also tagged the child scopes in line with the contexts, so if someone wanted to, they could scope registrations at any level instead of needing a new a new attribute in the future, as long as they registered these in the Global configuration. Like so:
containerBuilder.RegisterType<MyType>().InstancePerMatchingLifetimeScope(nameof(FeatureContext))

Happy to discuss more if you need, thanks

Copy link
Contributor

@gasparnagy gasparnagy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it, thanks @rossmasday! I also liked the extension methods on the container builder.

I have two small comments for the documentation and I think we would also need an entry in the changelog that describes the change.

docs/Integrations/Autofac.md Outdated Show resolved Hide resolved
docs/Integrations/Autofac.md Outdated Show resolved Hide resolved
@rossmasday
Copy link
Contributor Author

Thanks for the review, I've updated the docs as requested. Wasn't sure about the version number for the change log so guessed what it would be

Copy link
Contributor

@gasparnagy gasparnagy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SabotageAndi SabotageAndi merged commit 05871ab into SpecFlowOSS:master Apr 11, 2022
@SabotageAndi
Copy link
Contributor

Thanks for your contribution to SpecFlow.
Please submit your contributions to our SpecFlow Community Heroes program at https://specflow.org/community/submit-a-contribution/

@SabotageAndi
Copy link
Contributor

Builds should be finished overnight and I will push new packages to NuGet.org tomorrow morning.

@rossmasday rossmasday deleted the AutofacGlobalContainer branch April 11, 2022 15:34
gasparnagy added a commit that referenced this pull request Apr 13, 2022
* origin/master:
  expand property aliases support (#1384) (#2581)
  Document how to run tests
  Autofac global container (#2579)
Code-Grump pushed a commit to Code-Grump/SpecFlow that referenced this pull request Mar 29, 2023
Co-authored-by: Ross Ackland <ross.ackland@newsignature.com>
Co-authored-by: Andreas Willich <SabotageAndi@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants