-
Notifications
You must be signed in to change notification settings - Fork 850
Add spec tests for external container implementations #629
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
Conversation
|
cc @Eilon for OSS approval. |
natemcmaster
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nit, otherwise LGTM>
...External.Tests/test/Microsoft.Extensions.DependencyInjection.ExternalContainers.Tests.csproj
Outdated
Show resolved
Hide resolved
|
FYI Autofac: @alexmg @tillig |
halter73
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. It's been a long time since we ran tests for external containers ourselves.
…sions.DependencyInjection.ExternalContainers.Tests.csproj Co-Authored-By: pakrym <pavel@krymets.com>
|
|
||
| namespace Microsoft.Extensions.DependencyInjection.Specification | ||
| { | ||
| public abstract class SkipableDependencyInjectionSpecificationTests: DependencyInjectionSpecificationTests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo - "skippable" has two p
|
How will container owners get notified if a new spec test doesn't pass? We run these for Autofac, too, and I'm excited to see the double-check here, but if something gets discovered I'd love to hear about it as it happens. Maybe some feed to subscribe to or something in the future? |
|
@tillig we, in general, won't add any new feature specification test that breaks existing implementations until their authors acknowledge and agree to support the feature. For spec tests that uncover bugs, we are going to merge and notify authors. I also was thinking about repeating cc'ing everyone in #629 (comment) for all spec changes that go in. |
...External.Tests/test/Microsoft.Extensions.DependencyInjection.ExternalContainers.Tests.csproj
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We specifically moved away from this model because it doesn’t scale very well. cc @Eilon
|
What exactly are you worried about? |
|
When I was looking at container feature support for my library, I had to write these kinds of tests myself. It's not fun, containers did not always update in a timely basis, and just trying to understand how to fix registration problems either put the onus on the container owners, or myself. I don't have a great alternative though - if I want to add a feature to the spec, like the constraints thing, how do I know what the bar is? If we have a list of containers that are to be targeted for support, like a blessed list of supported containers, then at least we can hone in on some consensus. At the very least an easy way for container authors to be able to verify against the spec would be very helpful. I don't thin that's easy today. |
|
I do not think feature support should be a problem. I would add new features and tests for them freely and publish resulting matrix per container. It could be a good starting point for developers to evaluate container implementations. Breaking existing code is bad but new stuff should be OK. Just because one of the containers does not implement feature it should not be omitted from library. |
…sions.DependencyInjection.ExternalContainers.Tests.csproj Co-Authored-By: pakrym <pavel@krymets.com>
Talked to David offline, we are going to try running these tests
|
Got OSS approval from @Eilon. |
So when there is a request to add a new feature to spec tests we can easily verify against external container implementations.
This list is a combination between README.md from this project and @jbogard's MediatR samples (https://github.com/jbogard/MediatR/tree/master/samples)
Some implementations do not pass all the tests so I've added a way to skip test methods per container. It's not ideal, but works.
I'll try to update versions every time new spec test is added but feel free to send PRs if you have a new release.