Dependency Injection support #2
Conversation
|
Hey, thanks for this; Sorry I won't have time to do more than a scan until later in the week. One quick thing, also to assist other reviewers, |
There was a problem hiding this comment.
In general, this looks good and is along the lines of what I had in mind when I requested similar on codeplex back in the day.
Overall there's nothing specifically scaring me, but I'm wondering how this works overall
- in the large in a real project - have you applied it to an interesting suite yourself ?
- with xUnit's fixture support and parallelism (see https://xunit.github.io/docs/shared-context.html) (it doesn't look like it won't fit, but I'm thinking that I'd like to be able to implement IInstanceProvider in 5 lines with Autofac to get it's [configurable] registration/discovery semantics and then have xUnit wire up the sharing/disposal and manage the concurrency of different shared fixtures)
Until I get some time to think about these things, and review the other changes after the reformatting, I'd prefer not to move toward merging as yet.
For now, obviously having this branch already gets us to a point where people can take it for a spin and/or poke holes in it.
Would be great if @ptrelford has bandwidth to assess and comment....
|
|
||
| Scenario: Store and retrieve the value using the second implementation | ||
| When I store "test" using the second implementation | ||
| Then I retrieve "Second: test" using the second implementation |
There was a problem hiding this comment.
Hm, this could get messy in a real scenario; do you envisage having some shared state and then having the step dig stuff "out of the context" ?
There was a problem hiding this comment.
Ah, ok, getting it now looking at the impl
Perhaps having a Given we are using the second impl and have the steps neutral ?
Either that or try to work toward a scenario that (no matter how whimsical) is not abstract and technical
| type public StepsWithSecondImplementation(dependency: SecondDependencyImplementation) = | ||
| [<When>] | ||
| member this.``I store "(.*)" using the second implementation`` (text:string) = | ||
| (dependency :> IDependency).Value <- text |
There was a problem hiding this comment.
do these downcasts as a let at the top of the type
TickSpec/ScenarioRun.fs
Outdated
| afterStepEvents |> invokeEvents | ||
| ) | ||
| finally | ||
| module internal TickSpec.ScenarioRun |
There was a problem hiding this comment.
need to be able to see what happens here as covered in mainline. @ptrelford review when that's tidied would be awesome if you can magic up some time...
TickSpec/ScenarioRun.fs
Outdated
| let generate events parsers (scenario,lines) = | ||
| fun () -> | ||
| /// Type instance provider | ||
| use provider = new ServiceProvider() |
There was a problem hiding this comment.
if you made this an optional arg and/or overload it in a different way, perhaps people could feed in a service provider which is instantiated in terms of a DI container (i.e. allowing you to do a 10 line impl which delegates to Autofac. While this should not be the main path and I'm not entirely averse to having a batteries-included experience, we don't want to build a monster either. Having such an extensibility point may allow you to push the InstanceProvider out into the test project rather than relying on it being a part of TickSpec itself.).
I need to think about this aspect a bit more, but my gut sense is that it should be possible to separate out
- the the DI impl side from TickSpec (i.e. default to the boring ServiceProvider unless a specific one is provided and let that be the way toy feed one in
- the DI wiring from the step impls in the Scenario / Steps impl (have not played with it and your example is too small for me to get the full sense of it)
|
Regarding providing your own DI container - at which point do you expect to inject the container? Do you expect it in the:
I guess that there should be an interface as simple as possible to implement. Even the IInstanceProvider is too complex if you will use different container. Maybe it can be just an interface inheriting from IDispose and having Resolve method. Or we can stick to the IServiceProvider interface and checking actively whether it is IDisposable and if so then call the Dispose() method. What do you prefer? Done:
TBD:
|
|
Hi @mchaloupka thanks for the quick followup; sorry I'm a bit snowed atm and won't be able to match your pace for the next month ;) |
|
Thanks for cleaning the line endings; your core implementation-incision looks neat now I can see it ;) Re when to provide In terms of the API, yes, pretty much exactly as you suggest makes sense:-
This core change makes a clean initial PR that's easy to agree/merge. Then, at the point where we have actual usage extracted from a real test suite (or ideally 3) and have an example of applying the scheme using xUnit in an appropriate fashion, one might include a batteries-included (This also matches a realistic window within which I might more easily extract time to work out an xUnit binding!) |
.gitignore
Outdated
| @@ -237,6 +237,7 @@ ModelManifest.xml | |||
|
|
|||
| # Paket dependency manager | |||
| .paket/paket.exe | |||
| .paket/Paket.Restore.targets | |||
|
|
|||
There was a problem hiding this comment.
can you gitignore and add nuget nunit via paket (perhaps in a group build to make sure it's not transitive in the Nuget) rather than including the .dll in the repo please ? I'd really like to avoid including binaries and/or otherwise making things heavy if possibel.
There was a problem hiding this comment.
I can see that @michalkovy opened a pull request with that. If you accept it, then I will merge it here and add it even for the newly added projects.
There was a problem hiding this comment.
@mchaloupka there are 3x PRs. You are correct that the Paket and NUnit stuff in that overlaps and covers this issue, but his stuff is reliant on your core work so I'm keen for us to work to get this trimmed down to a minimal impl which meets the goal and allows him to build his stuff from your base. @michalkovy it does seem to make sense to carve off your NUnit / Paket bits right away and then get this ready to merge after that ?
There was a problem hiding this comment.
Seems Paket.Restore.targets should actually be added, not .gitignored, see https://fsprojects.github.io/Paket/faq.html#What-files-should-I-commit
There was a problem hiding this comment.
Probably that information isn't up to date, see this documentation issue in Paket
There was a problem hiding this comment.
I am not sure that I understand the question, but I will try to reply. I fully reused what Milos did, I just missed one method in the container. Milos has there RegisterInstanceAs<'TInterface>method which calls instances.Add(typeof<'TInterface>, instance). That wasn't good enough for me as I need to be able to replace instance in container, too. Hence I added method RegisterOrReplaceInstanceAsType which calls instances.[t] <- instance. However, when I think about more my implementation actually breaks the concept that you would replace DI container how Milos wanted, we will think it through.
I have forward integrated the Functional DI branch. Half of the code is Milos's DI branch.
There was a problem hiding this comment.
I am not sure that I understand the question, but I will try to reply. I fully reused what Milos did, I just missed one method in the container. Milos has there RegisterInstanceAs<'TInterface>method which calls instances.Add(typeof<'TInterface>, instance). That wasn't good enough for me as I need to be able to replace instance in container, too. Hence I added method RegisterOrReplaceInstanceAsType which calls instances.[t] <- instance. However, when I think about more my implementation actually breaks the concept that you would replace DI container how Milos wanted, we will think it through.
I have forward integrated the Functional DI branch. Half of the code is Milos's DI branch.
There was a problem hiding this comment.
@michalkovy Thanks for the response. I did assume you were building on Milos' work.
Regarding merging this, my main concern for any impl is that TickSpec's core should touch IServiceProvider and IDisposable only.
Then the specific DI container (perhaps a batteries included basic impl can live in core to serve the samples but in principle I should be able to wire in a random version of e.g. Autofac if I want) can should be fed in via the top level of the API.
If we get something together that fits that description, then you guys can both build examples of your approaches which would work against an unmodified TickSpec NuGet.
My overall idea/plan is to get either this PR or a new one to do basic plumbing that's able to cater to both needs and then review the approaches on top of that via separate PRs.
(Please let me know if I should try to explain a different way)
There was a problem hiding this comment.
I think Milos achieved that but unfortunately I need for the functional DI to work the TickSpec core to be able to change instances in container by the TickSpec core. Or, what I could do is to have a custom container in TickSpec core that I know within the container - but I don't feel that it would be better for custom DI framework. So, one option would be to not use IServiceProvider but have a slightly bigger interface that would also contain the method to change an instance in the container. What do you think?
There was a problem hiding this comment.
I have not looked at your code but its definitely best to keep the interface from the core to the container minimal. IServiceProvider and IDisposable have clear known semantics that we dont need to write an article about. So I'd suggest adding a function or single method interface to model the exact semantic of how stuff is emitted would be best (or could you model it as a post step Event that something external can Subscribe to ?). Growing a big state ball with lots of proprietary methods is the thing I'm seeking to avoid - if we can do it as discrete extension points, there's far better chance we end up providing something useful in the medium term and/or for as yet unforeseen needs.
@mchaloupka what do you think - is there a point in getting minimal IDisposable support and extending that or do you see the value being something broader and more complete like Michal's overall approach ?
|
So, there was quite a huge discussion. I will try to address all mentioned parts. When looking at the pull request made by @michalkovy, I really liked the way as it allows a functional way of writing tests. It enables easier way how to share a state/context/whatever without having mutable variables. On the other hand, I completely agree with @bartelink that it should be possible to provide own way of providing some standard DI container. But I do not see a reason why to keep the old implementation as the new one behaves exactly the same for the previously supported scenarios. It just supports more. So, the new implementation should be the default. However, in the samples (and in our real test cases) we actually do not need the type registration. So, the default implementation can be simplified to have just two methods: "Resolve" and "RegisterInstance". These two methods will be latter used also in the @michalkovy approach. If you will want anything more, then you can use some more advanced DI container. So, what I suggest is that I will do the next:
@bartelink Do you agree? |
|
I fine with the approach that @mchaloupka proposed |
|
@mchaloupka Yes, that sounds great.
Some people claim I'm a common factor in those ;P
I assume by this you mean that the TickSpec engine will a) use a supplied IServiceProvider Nothing is popping into my head about where to fall wrt to: Do you have ideas or are you intending to experiment and report back? |
There was a problem hiding this comment.
Apologies for having lots of comments. I don't need them all to be addressed, but I'm keen for us to make sure we cover as many scenarios as possible without paining ourselves into a corner.
I'm thinking if you can provide the hooks (allow me to feed in a pair of resolve and a register function), I'll probably supply a sample which has a single file called AutofacIntegration.fs and the example using this, then that is a very small impl and we have documented semantics and the maintenance and doc of that feature is outsourced
One thing that might be useful/possible too is to use xUnit's shared fixtures to manage some global state too.
I'm going to cherrypick the reformatting aspect onto master now; will ping when you can merge/rebase
Ideally we can get this clean and the yank in @ptrelford for a final blessing when we are done.
| @@ -0,0 +1,13 @@ | |||
| Feature: Feeding dog | |||
There was a problem hiding this comment.
Am liking this scenario, it strikes a good balance between insane-artificial and showing some of the power.
| | "large" -> instanceProvider.RegisterInstance typeof<Dog> (Dog(Large)) | ||
| | "medium" -> instanceProvider.RegisterInstance typeof<Dog> (Dog(Medium)) | ||
| | "small" -> instanceProvider.RegisterInstance typeof<Dog> (Dog(Small)) | ||
| | _ -> Assert.Fail("Unsupported dog size") |
There was a problem hiding this comment.
use invalidArg or an assert that includes the value
| type DogSteps(bowl: DogBowl, dog: Dog) = | ||
| [<When>] | ||
| member this.``The dog eats the food from bowl`` () = | ||
| bowl.FoodAmount <- bowl.FoodAmount - dog.AmountToEat |
There was a problem hiding this comment.
model the domain - bowl.Eat ;) (and guard invariants!) (not really)
| open NUnit.Framework | ||
| open TickSpec | ||
|
|
||
| let assembly = Assembly.GetExecutingAssembly() |
There was a problem hiding this comment.
consider separating this out like in xUnit2 with a TickScenarios.fs (that a NuGet package could emit - ideally with a type that you don't have to derive from and can just use as a one line) and a Features.fs which in one line lets you expose a set of scenarios to the runner. (Only if you have appetite, this is unimportant and I will prob do it as a backport as part of the work of porting your example to xUnit -- but it is nice to have a new user not have a mangle of tech code with no comments and a base class they need to understand)
TickSpec/ServiceProvider.fs
Outdated
|
|
||
| /// Provides an instance provider for tests | ||
| type IInstanceProvider = | ||
| inherit IDisposable |
There was a problem hiding this comment.
in general people don't do this (and/or it causes a mess when they do) - IDisposable is an orthogonal interface and we should not force people to implement it
TickSpec/ServiceProvider.fs
Outdated
| abstract member Resolve : Type -> obj | ||
|
|
||
| /// Registers an instance for a type (if there is already a registered instance, it will be replaced) | ||
| abstract member RegisterInstance : Type -> obj -> unit |
There was a problem hiding this comment.
is it a problem for this to be generic (is it much more messy in the codegen) ? Or is it a specific choice from an interface design perspective?
There was a problem hiding this comment.
I wanted to make the interface in the exact way how it is used by TickSpec. TickSpec will not use the generic variant. So, I tried to make the interface as small as possible. I can easily add generic variants into it.
There was a problem hiding this comment.
You are correct to model the extensibility point directly as TickSpec wants it and have wrapping elsewhere. (i.e. there's a lot to be said for the IServiceProvider method definition)
My concern is that if its also supposed to be user facing, then there should be a clean idiomatic F# route that allows you to work with type inference in the main
TickSpec/ServiceProvider.fs
Outdated
| | None -> () | ||
|
|
||
| match t with | ||
| | t when t = typeof<IInstanceProvider> -> self :> obj |
There was a problem hiding this comment.
in line with the ISP it would be good to be able to indicate one only consumes and does not provide by asking for IServiceProvider ?
Having said that, this will probably complicate the impl a bit here
There was a problem hiding this comment.
supporting fun unit -> 't and fun 't -> unit (maybe that gets messy) would allow one to bind to an F# type:-
type MyFeature(resolve, register) =
let x : DogBowl = resolve()
let doStuff () =
register <| SmallDog()
that might get us out of the interface game?
Or e.g. delegate factories can allow us to pass args to ctors.
Why am I mentioning this?
I'm concerned that having a very specific interface and semantics in this container will make it much harder to ever support other semantics. Thinking....
There was a problem hiding this comment.
So, you prefer to have two separate interfaces for registration and resolving and you would not be able to ask for the complete container, just for one of the two interfaces? I can do that, but I do not understand the reason. Regarding ISP, both methods are for container manipulation, storing and retrieving so from this principle it seems ok to me... Again, I have no problem doing it, although I am not 100% sure, that it is worth it.
Regarding the two methods - they look nice and I can imagine, that we may want to add it as an addition. However, it does not look to be right in terms of DI, because what type you use when you call register in that case? SmallDog, Dog or IDog? I think that it is good to be explicit in this case.
There was a problem hiding this comment.
Sorry, I misformatted the example and omitted the type definition.
The container should be substitutable and is not the interesting bit. For the bulk of what TickSpec wants, IServiceProvider is sufficient.
If we build a way for return values to be registered, then we need a way for that to work.
But in general an author of a Feature type should be able to ask for either
- 80% of the time (ideally via ctor args), values like normal DI, by supplying params as opposed to explicit calls
- 5 % of the time registering stuff to share with other steps
- 15% of the time interacting in fancy ways - people in that space will probably be best off using Autofac or their favorite container.
Baking an interface which does the register and the resolve doesn't really help anyone in the above categories as I see it. Does this make any sense ?
Re whether the signatures are generic or have an explcit type arg, for resolve that gets very ugly very fast. Also if a container ha preregistered stuff, you dont want to do explicit calls
type MyFeature(kennel : Kennel, resolve, register) =
let x : DogBowl = resolve()
let doStuff () =
register <| SmallDog()
let letTheDogsOut() =
kennel.letThemOut ()
If we do implicit register of the return value, then you're in the land of having to make sure the types line up anyway.
None of this precludes also having a way to supply a Type too, but in general, tpying typeof<X> is not something I'd want in normal DI code, so why would I want to be doing that in step defintiions ?
There was a problem hiding this comment.
The other thing is regarding splitting registering and resolving, many containers (such as Autofac) make a very deliberate contrast between registering and resolving from a CQS perspective (I dont even know how you do runtime registers in Autofac as in general, it bakes a graph, but assume there is a way)
From an ISP perspective I'd argue that you're only very rarely doing both registering and resolving in any given context
TickSpec/ServiceProvider.fs
Outdated
| interface IDisposable with | ||
| member this.Dispose() = | ||
| instances.Values | ||
| |> Seq.choose (fun x -> |
There was a problem hiding this comment.
I would merge the iter and the choose
Or
for i in instances.Values do
match i with
| :? Disposable as disposable -> disposable.Dispose()
| _ -> ()
There was a problem hiding this comment.
Also function could be use instead of fun + match
TickSpec/ServiceProvider.fs
Outdated
| t.GetConstructors() | ||
| |> List.ofArray | ||
|
|
||
| let (_, widestConstructors) = |
There was a problem hiding this comment.
am thinking, oh noes, we is writin' another DI container (though I agree that some form of batteries being included is important (how does this implf fare under .NET Core, did we say it was OK ?))
There was a problem hiding this comment.
If we move under .NET core then I would replace it by the one used in .NET Core. However, if you look on the implementation now, it is not DI, it is just the previous provider with recursion (so a type may need a different type) and with the ability to prepare an instance from outside. It seems to me ok, to use this approach., to have such simple container here and provide the ability to replace it by something real.
There was a problem hiding this comment.
Do you mean the one in ASP.NET Core ? Is that in a separable package ?
There was a problem hiding this comment.
Cool - that might make a pretty decent LCD container to target given the degree to which one should ideally be employing containers in an acceptance testing scenario.
TickSpec/ScenarioGen.fs
Outdated
| // Dispose the ServiceProvider | ||
| gen.Emit(OpCodes.Ldarg_0) | ||
| gen.Emit(OpCodes.Ldfld, providerField) | ||
| gen.Emit(OpCodes.Callvirt, typeof<IDisposable>.GetMethod("Dispose")) |
There was a problem hiding this comment.
can this be made conditional please ? i.e. Dispose it iff it impls IDisposable
|
@mchaloupka seeing whitespace diffs above (its saying 1000+ lines changed); can you have a peep and let me know when it's good to review? |
|
@bartelink Argh, I do not understand when it happens. Anyway, feel free to review the other parts. I guess that it currently happened only in the single file. I will fix it tomorrow. The change in the generation is just the usage of lambda func to create the provider and then the conditional Dispose call (I took it from your branch). |
|
👍 Will have a look later with a local diff tool |
| @@ -0,0 +1,13 @@ | |||
| Feature: Feeding dog | |||
|
|
|||
| Scenario: Feeding dog with enough food | |||
There was a problem hiding this comment.
this should be Scenario Outline instead of Scenario
|
@mchaloupka I have realized in the dog feeding example that it may be nice to register DogSize instead of Dog and so show a little bit more how DI works and can help. See my changeset michalkovy@e7596f7 in FI variant. What do you think? |
|
V nice @michalkovy this is all starting to take shape - need to rename the PersonSteps at some point ;) |
|
So, the whitespaces are fixed. Have I addressed all the comments? Or is there anything else you want me to do before merging? |
|
Thanks Milos - unfortunately I'm majorly snowed atm and thus have not gotten around to looking yet :( will do as soon as I can |
|
Hey @mchaloupka have not forgotten this, but insanely busy this week so still have not made time to look, sorry :( |
|
Looking at this now... First step is rebasing on master, which is slightly messy due to a combo of lots of commits here and me having added a commit regarding IDisposable. I'm reluctant to just shave that off though... Setting up a gitter room https://gitter.im/fsprojects/TickSpec |
|
Yes, the history is somehow messy but the diff seems to be ok. Moreover, I expect squash merge so it does not matter. |
|
Closing the pull request as it was merged as part of other pr. |
I want to resolve #1.
I have been thinking about using some preexisting DI container but I have not found any suitable one. If the project will be migrated to .NET Core, then it would make sense to use the build-in one, but for now it seems to me better to have a very simple one as part of the project. So, I have added the ability to resolve more complicated types to the ServiceProvider. It is possible to construct a class which depends on another classes (using constructor injection). It is also possible to do simple type or instance registrations, even as part of other steps. The usage is added as an example.
The ScenarioRun and ScenarioGen were modified, but most of the changes are whitespaces (end of line, they were probably inconsistent).