Dependency injection extensions and unit test to ensure correct DI registration of child dependencies

SimonGeering commented 4 years ago

WIP PR for issue #1

As it stands, there is currently an issue with the test erroring see below.
The PR could be submitted with the unit test set to Skip="WIP" so that you would get the benefit of the cleaner organization of the DI extension methods now, with work to resolve the test problem to follow subsequently.

Code Changes so far

[x] @benrick to smoke test that this still runs with a wired-up Twitch API account as I don't have one to hand.
[x] Extract Core DI into an extension method.
[x] Extract Twitch infrastructure DI into an extension method.
[x] Extract LiteDB infrastructure DI into an extension method.
[x] Extract Main App DI into an extension method.
[x] Add unit test to ensure correct DI registration of child dependencies by attempting to instantiate all container registered types.
[ ] Either set the above unit test to Skip or resolve exception from when attempting to resolve application statice resources for the DataTemplate "MenuItemTemplate".
[ ] Either resolve issues with MediatR in the above unit test or leave the code as is where it will exclude checking of MediatR types for the time being.

Changes to dependent packages:

Added FluentAssertions to ChatterBot.Test to improved readability of assertions in tests.
Added Xunit.StaFact to ChatterBot.Test to resolve an issue with the DI Unit Test:
- As it was attempting to validate DI registrations for the WPF windows it failed with an exception that they can only be loaded form STA type threads, the new WpfTheory attribute allows for this while not impacting other xunit tests in the same test assembly.
Upgraded xunit from V2.4.0 to V2.4.1 since this is the minimum version required by Xunit.StaFact.

Unit Test Errors - why this is a draft PR for now:

Currently having some problems when running the DI Container registration unit test. It is failing because it is unable to resolve the static resources DataTemplate - MenuItemTemplate which is coming from the Application Resources.
It may be possible to skip the forms in the unit test but given the way its setup with forms being injected into view models this may mean they also have to be skipped which defeats most of the point of the test in the first place.
An alternative resolution to this may be to switch from the current DI chain of injecting the forms into the view models and instead inject the ViewModel into the form ctor and set data context there. This would allow the forms and ViewModels to be separated into two DI extension methods and the forms could then be excluded from the dependency validation test. This would however also require MediatR messaging or similar to decouple the ViewModels and forms from having a direct dependency on each other. As it stands it is this strong coupling that is at the heart of the problem.

benrick commented 4 years ago

Seeing this, I'm really glad I wrapped up and merged in my code, since I knew I was adding a bunch of DI stuff that would impact how you would approach this. 👍

SimonGeering commented 4 years ago

Would be interested in your thoughts on this as I think it may center around if you want VM injection into Forms, or the other way around, or if you take a MediatR messaging approach and have something centrally such as the app class, act as the handler for management of forms.

Either way if you want to pull it down and see if you can make more sense of it than I can, happy to put it in with the test turned off for now though.

benrick commented 4 years ago

Yeah, so there are a few things here:

Regarding the MainWindow creation, it's only done once in the App, and isn't currently referenced elsewhere (as far as I remember), so if we skipped its registration-check, that's probably fine. If that's ever missed, the program just doesn't load at all.
Regarding the creation of sub windows (like the AccountWindow). Since it creates created when shown, it isn't even being injected into the MainWindow right now, but is instead just created in the MainViewModel using its constructor, so we could technically just remove it from the registration for now. In order to DI that (without changing how we do our DI), we'd need to inject a factory of some kind to create new ones when needed.
Regarding a MediatR-based approach, I'm generally in favor of one, since it will help us decouple a bit. Only added MediatR in the code yesterday.

benrick commented 4 years ago

Also, about a smoke test of this, I pulled it down and tried it. Things still appear to work in the application as expected. 👍 Still connects to Twitch and can send messages in response.

SimonGeering commented 4 years ago

So I have decided we should put this forward as a PR to get these changes in if you're happy with them @benrick . To work around the unit test issue for now I have commented out the UI extension.

benrick commented 4 years ago

Agreed. I think this is core enough of a change to get it merged in sooner rather than later.

DevChatter / ChatterBot