Fix failing Unit Tests

[gep13]

Just ran the Unit Tests, and looks like there are 8 failing tests. We should probably get these passing :-)

[tarwn]

I have all the test failures fixed locally, but have two issues.

One: I'm unfamiliar with StructureMap (guess I should go back and read @chrissie1's posts) and the correction for several of the tests was to switch the constructors from using locally instantiated concrete objects (mostly Run and RunAsync) to use the provided interface parameters that would be supplied via IoC. So while the tests now pass, I'm less sure about how the code actually runs, as I don't see anything obvious that would tell it to correctly choose between the Sync and Async version of IRun objects.

Two: I'm tracking down 3 dialog boxes that appear while running the tests and have found another trail of concrete constructor calls. I can keep tracking these down, or I can try to rewire them to use the registry for StructureMap. My suspicion is that the dialogs are side effects from a concrete class constructor somewhere.

The project seems to be partially using IoC and partially bypassing it. Is there a preference? My options are that I can either continue to straighten it out in favor of StructureMap or in favor of the default constructors, but I don't want to get too much deeper into the tests without knowing which is the preference.

[gep13]

I would say that we need to be consistent. If we are using IoC elsewhere in the project, then we should be using it everywhere. This will make the code more maintainable in the long run. There is another open issue to replace StructureMap with TinyIoC, so perhaps this work should be done before any further investigation. Thoughts?

[chrissie1]

We definitly want to use IoC but structuremap can go away, I was thinking TinyIoc or Autofac. I think we need to educate @cemrich in the use of Ioc/DI and maybe add a irunsync and irunasync. I thought everything either ran sync or async but it doesn't seem to be true. Changing IoC container was issue number #11

[chrissie1]

Ripping out structuremap and replacing with another should be simple enough. Only one registry and one place where I use the container, in program.cs (which is how it should be).

[cemrich]

I am familiar with the concept of dependency injection from java development and I guess StructureMap is a way to achieve this in C#.
One class that doesn't make use of it yet is CachedPackagesService. This one is a bit tricky because it is able to cache package lists using any IPackagesService but it implements IPackagesService by itself. I'm not sure how to change the architecture in a way that:

1. the PackageManager can use a cached IPackagesService but does not have to
2. the CachedPackagesService can get its IPackagesService implementation via DI
   One easy way would be to let any implementation of IPackagesService handle the caching by itself but this could mean a lot of duplicated code.

Regarding the tests I think it's not wise to use DI for most of the tests. The test cases are mostly white box tests at the moment. They test for internal behavior like "does method xy call method z with these arguments". These tests are highly implementation dependent and should not use DI.
However there should be tests testing functionality that doesn't depend on the concrete implementation (like IfCurrentSourceIsNotemptyWhenInstantiated for testing ISourceService). These black box tests should run on all available implementations of the interface they test (which maybe could be achieved using DI).

I don't understand the sync and async thing yet. As far as I can see these classes are used to communicate with powershell. @chrissie1, can you please explain this a bit further?

[chrissie1]

To solve the problem with the cachedservice just create another interface it can be empty. It's a small hack but the world isn't perfect and I can live with it. The other way around is to do complicated IoC configuration but that just wastes time and can lead to much pain. I have found that you should not overcomplicate the IoC configuration. Better to do the simple thing and create an extra interface if you need a particular implentation of something. It also makes it clear to the reader of your code what it is you are doing when you use the interface. Lot's of configuration will lead to lots wondering why am I getting this implementation and not that one.

The DI is there mainly to make it easier to test and to mock out the dependencies to test the behaviour of the SUT not the dependencies. We don't want our unittests to have a need for chocolatey or even powershell. They should run in isolation without a need for their dependecies and test behaviour as you say.

We can and should have integration tests that go out to chocolatey but then we have to make sure that the buildserver can run them.

You can run the powershell commands in sync, wait for the result and then go on. Or run them async, don't wait for the result and go on. Either way should work. But using the Async way you have to be carefull with crossthreadexception in the view. The Async should be used as much as possible so that you don't block the viewthread.

So I would make an ICachedPackagesService that inherits from IPackagesService (if needed) and add that to the registry, make CachedPackagesService implement ICachedPackagesService and then use that.

And sorry if I insulted you, it was not my intention.

[tarwn]

I'm modifying things in favor of using the IoC container, it's nearly there and then I'll merge it in and let someone do the StructureMap to TinyIoC conversion. I've added IRunSync and IRunAsync, added a file system abstraction for the few places that access the filesystem to reduce their reliance on test files being placed just so (a couple of the failing tests were due to content changes in the sources file that don't have any effect on the actual logic but caused the test expectations to fail).

[cemrich]

The DI is there mainly to make it easier to test and to mock out the dependencies to test the behaviour of the SUT not the dependencies. We don't want our unittests to have a need for chocolatey or even powershell.

Ah, I didn't thought of that.

You can run the powershell commands in sync, wait for the result and then go on. Or run them async [...]

Okay, than it's just the powershell adapter. The name confused me because I've overlooked the namespace (I expected something like AsyncPowershelAdaper), that's my fault.

And sorry if I insulted you, it was not my intention.

It's okay - I just have to figure out which knowledge I'm able to transfer to C# an which not. I'm sorry if that messes up parts of the code. Of course you can always say so when things can be improved :)

I'll look after the CachedPackagesService as soon as @tarwn has finished.