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

Allow custom service providers like autofac #1233

Merged
merged 15 commits into from
Oct 12, 2023

Conversation

inf9144
Copy link
Contributor

@inf9144 inf9144 commented Oct 10, 2023

Pull request description

ASP.NET Core / Blazor allows for custom containers via IServiceProviderFactory.
This pull request adds this ability to the TestServiceProvider of bUnit.

Two times requested, I also need this and you told that you would like to have that as PR ;-)
See:
#268
#213

PR meta checklist

  • Pull request is targeted at main branch for code
    or targeted at stable branch for documentation that is live on bunit.dev.
  • Pull request is linked to all related issues, if any.
  • I have read the CONTRIBUTING.md document.

Code PR specific checklist

  • My code follows the code style of this project and AspNetCore coding guidelines.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • I have updated the appropriate sub section in the CHANGELOG.md.
  • I have added, updated or removed tests to according to my changes.
    • All tests passed.

@inf9144
Copy link
Contributor Author

inf9144 commented Oct 10, 2023

@inf9144 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@dotnet-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@dotnet-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@dotnet-policy-service agree company="Microsoft"

Contributor License Agreement

@dotnet-policy-service agree

Revert code changes that were done by automatic clean up.
@egil
Copy link
Member

egil commented Oct 10, 2023

Hi @inf9144. interesting, thanks.

Can you explain why the fallback service provider feature isn't good enough for what you need?

@inf9144
Copy link
Contributor Author

inf9144 commented Oct 10, 2023

Hi @egil,
I have a large but very modular container building process in my software (big information system). I can easily reuse it if i can replace the container. I cant use the fallback feature because things in my container can depend on the blazor eco system (like an NavigationManager or ...) and vice versa (if there is a injection using one of my types in some Blazor component). And i want not only do unit testing with bUnit but also test for integration. I would need to copy/paste - replicate all those dependencies and would not test the "real thing" anymore but some synthetics.
The implemented way is the way Microsoft also supports this in their own DI, see:
https://source.dot.net/#q=UseServiceProviderFactory
They have this feature to support other DI frameworks like autofac (which i am using, because it supports hierarchies, factories and much more).
By accepting this PR, bUnit would behave symmetrical to what Blazor does. And this is one of the goals of this project or? Testing code build up on Blazor ;-)

//EDIT: Dont know if it get's clear enough: Fallback is a oneway street which doesnt work if you have dependencies in both containers that rely on something that is in the other container ;-)

//EDIT2: Copy the registrations is also not possible because autofac has a bigger feature set which can't be projected onto a simple registration in an MS DI container.

CHANGELOG.md Outdated Show resolved Hide resolved
@egil
Copy link
Member

egil commented Oct 10, 2023

Hi @egil, I have a large but very modular container building process in my software (big information system). I can easily reuse it if i can replace the container. I cant use the fallback feature because things in my container can depend on the blazor eco system (like an NavigationManager or ...) and vice versa (if there is a injection using one of my types in some Blazor component).

Thanks for making the use case clear. I am cautious about adding new public API surfaces to bUnit since I have to support it now and in the future. This is an excellent point though. The fallback provider does not have access to the primary provider. This feature would, to some extend, make the fallback service provider feature obsolete though. That is OK though.

And i want not only do unit testing with bUnit but also test for integration. I would need to copy/paste - replicate all those dependencies and would not test the "real thing" anymore but some synthetics. The implemented way is the way Microsoft also supports this in their own DI, see: https://source.dot.net/#q=UseServiceProviderFactory They have this feature to support other DI frameworks like autofac (which i am using, because it supports hierarchies, factories and much more). By accepting this PR, bUnit would behave symmetrical to what Blazor does. And this is one of the goals of this project or? Testing code build up on Blazor ;-)

It is definitely one of the goals!

//EDIT: Dont know if it get's clear enough: Fallback is a oneway street which doesnt work if you have dependencies in both containers that rely on something that is in the other container ;-)

//EDIT2: Copy the registrations is also not possible because autofac has a bigger feature set which can't be projected onto a simple registration in an MS DI container.

Ill take a look at the code as soon as possible get back to you. @linkdotnet, please also add your two cents.

@egil egil requested review from linkdotnet and egil October 10, 2023 18:35
Co-authored-by: Egil Hansen <[email protected]>
@egil
Copy link
Member

egil commented Oct 10, 2023

This change does require an update to the docs. A new section on https://bunit.dev/docs/providing-input/inject-services-into-components.html should do it. Do you feel up to contributing to that too @inf9144?

It would ideally be a description of the usage of the new methods and an example of their usage.

It is important to point out that bUnit also uses the provider internally, so any replacement providers must behave according to the IServiceProvider contract.

@linkdotnet
Copy link
Collaborator

The feature looks solid in itself and I do understand the reasoning. My only concern is the API itself, which becomes more convoluted. We introduce more and more ways to use DI - where this is nice, my look wanders to the future where we might go ahead and move that responsibility solely to the user, and bUnit just offers anchor points to let its service be registered in said container.

Adding generic notnull constraint to UseServiceProviderFactory
InitializeProvider public => private
@inf9144
Copy link
Contributor Author

inf9144 commented Oct 11, 2023

... and move that responsibility solely to the user, and bUnit just offers anchor points to let its service be registered in said container.

That sounds very reasonable, but would require a rework of TestContextBase/TestServiceProvider, which I can't do at the moment because I can't estimate all the required code changes and their impact on the public API.
If you ever decide to do this, I would be happy to offer my help.

@inf9144
Copy link
Contributor Author

inf9144 commented Oct 11, 2023

This change does require an update to the docs. A new section on https://bunit.dev/docs/providing-input/inject-services-into-components.html should do it. Do you feel up to contributing to that too @inf9144?

It would ideally be a description of the usage of the new methods and an example of their usage.

It is important to point out that bUnit also uses the provider internally, so any replacement providers must behave according to the IServiceProvider contract.

Commited. I tried to follow the style of your paragraph about the fallback mechanism :-)

@egil
Copy link
Member

egil commented Oct 11, 2023

... and move that responsibility solely to the user, and bUnit just offers anchor points to let its service be registered in said container.

That sounds very reasonable, but would require a rework of TestContextBase/TestServiceProvider, which I can't do at the moment because I can't estimate all the required code changes and their impact on the public API. If you ever decide to do this, I would be happy to offer my help.

Thanks, appreciate that. We may look at this as an option in v2. It would be a breaking change for v1, so its not on the table right now.

@linkdotnet
Copy link
Collaborator

Sorry, bringing up the topic about changes of the DI container was not meant in your direction to implement something, more like a consideration. We onboard new API's that might be dropped in the future.
Overall it might be fine in this case, as the enhancements sits on top of the current setup rather than be a crucial and central part.

Copy link
Member

@egil egil left a comment

Choose a reason for hiding this comment

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

Overall very good job. You pretty much nailed the code style.

Here are some suggested changes to the code which I think makes it a little easier to read.

src/bunit.core/TestServiceProvider.cs Outdated Show resolved Hide resolved
src/bunit.core/TestServiceProvider.cs Outdated Show resolved Hide resolved
src/bunit.core/TestServiceProvider.cs Outdated Show resolved Hide resolved
src/bunit.core/TestServiceProvider.cs Outdated Show resolved Hide resolved
tests/bunit.core.tests/TestServiceProviderTest.cs Outdated Show resolved Hide resolved
tests/bunit.core.tests/TestServiceProviderTest.cs Outdated Show resolved Hide resolved
@egil
Copy link
Member

egil commented Oct 11, 2023

After RC.2 has been released, a few of our tests stopped working. Have pushed a fix to main that you need to rebase into your branch though.

@inf9144
Copy link
Contributor Author

inf9144 commented Oct 12, 2023

I applied your recommendations, updated the branch from bUnit/main, documentation has now an section with Autofac sample and the tests (TestServiceProviderTest) are green ;-)

@inf9144 inf9144 requested a review from egil October 12, 2023 08:03
Comment on lines +116 to +118
#if !NETSTANDARD2_1
[MemberNotNull(nameof(serviceProvider))]
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Note to self: copy and make https://source.dot.net/#System.Private.CoreLib/src/libraries/System.Private.CoreLib/src/System/Diagnostics/CodeAnalysis/NullableAttributes.cs internal and scoped to NETSTANDARD2_1 to bUnit so we can avoid having #if !NETSTANDARD2_1 in the code. That way we can drop the serviceProvider >>!<< .GetService(serviceType) below, even in netstandard2_1.

Copy link
Member

@egil egil left a comment

Choose a reason for hiding this comment

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

I think we are there, more or less. Any things you want to add @linkdotnet ?

@linkdotnet
Copy link
Collaborator

linkdotnet commented Oct 12, 2023

Hmm the Ubuntu runner is running again for hours. I am not sure what is going on or causing this, but we didn’t see that behavior on main.

Would like to run it again a few times or even locally to understand that we didn’t introduce something here.

edit: I saw multiple builds were stuck.

@linkdotnet
Copy link
Collaborator

linkdotnet commented Oct 12, 2023

It is stuck again.

edit: there is also a stuck build in another dependabot PR run.

So it seems we might have that issue on main. I suspect our recent DisposeAsync change.

that said the pr is fine for me and can be merged.

@egil
Copy link
Member

egil commented Oct 12, 2023

I don't see how these changes should cause that. There have been some weird behaviors of the runners before.

@linkdotnet
Copy link
Collaborator

I don't see how these changes should cause that. There have been some weird behaviors of the runners before.

It is. Let’s take that discussion offline as it isn’t caused by the PR.

@egil
Copy link
Member

egil commented Oct 12, 2023

ok, ill merge this in and we can take a look at the disposeasync stuff tomorrow.

@inf9144 thanks for the effort on this!

@egil egil merged commit 3a18e8c into bUnit-dev:main Oct 12, 2023
1 of 4 checks passed
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.

3 participants