Give ContentAssetHelper an interface to implement, so we can mock it

Vote:
 

Title says it all, doesn't it? Absolutely every publicly available service class that has further dependencies on other EPiServer service classes should have a simple interface so they are mockable for tests etc.

Adding it is done in a few minutes, and will save people countless hours in the long run.

#91243
Sep 30, 2014 18:58
Vote:
 

It's never few minutes :) You know how many EPiServer employees are needed to add and interface on top of concrete class? :)

#91250
Sep 30, 2014 21:21
Vote:
 

A problem with interfaces is that they do not support versioning (you can e.g not add an additonal method without causing a breaking change). A class is also an abstraction as long as it have virtual methods and the usage of the class is through injection (that is do not new it) and hence it should be possible to Mock.

However I can agree that we in many cases have classes that are both the abstraction and the implemenation. Some of these classes have further dependencies in their constructor which makes them sligthly harder to mock (you need to pass in correct numbers of arguments). This can be solved with an abstract baseclass (without dependencies) and then an actual implemetation (which might be internal), this is a pattern we try to use nowadays.

#91270
Oct 01, 2014 11:57
Vote:
 

The exact implementation needed to solve the problem isn't really important. The important part here is that I should be able to inject something into my own service classes that I can mock in some way to test things. My initial statement still stands, it will save people countless hours in just fiddling about with structuremap-setups for testing etc.

For simple mocking, a class must have a parameterless constructor. So if all constructors need a bunch of other classes, the problem is just halfway solved - the class will technically be mockable, but it's gonna be boring monkeywork to mock all its dependencies (which in turn might require the same setup, possibly specific for every test case).

So that could also be a potential fix: give the classes an empty constructor, and force Structuremap to use the right one when running properly (aka as a web application, not via Nunit or other test frameworks)

#91274
Oct 01, 2014 13:06
Vote:
 

I agree that the mocking should be as simple as possible.

We made a huge step in testability in CMS7, but absolutely there are more to do. For example have abstract classes or default constructors on classes. We have quite a huge code base so it is hard to change everything at once but we try to continuosly improve in testablitiy.

#91295
Oct 01, 2014 17:19
Vote:
 

I totally agree that that was a huge step forward from 6 to 7. Arve will Fakes or FakeItEasy or similar lib come handy in these cases? Not sure yet (haven't tested) but there are libs that mock entire class even it's not behind an interface..

#91305
Oct 02, 2014 7:15
Vote:
 

The example that triggered this (as far as I remember) was the ContentAssetHelper. And I think the UrlResolver is kindof quirky to manage too.

I'm using Moq for my mocking. There might be other frameworks that can do these things, but the "default requirements" for being mockable is either to be an interface or to have an empty constructor.

#91307
Oct 02, 2014 9:05
Vote:
 

Please add the classes that you find most annoying to Mock then we can prioritize them when we work further on with increasing testability. You mentioned ContentAssetHelper and UrlResolver. I guess TemplateResolver might be one also?

#91309
Oct 02, 2014 9:13
Vote:
 

I haven't tried to mock the TemplateResolver (yet?), but might be.

I can try and update this thread with the relevant classes as I find them. If I am to prioritize them:

  • 1. UrlResolver
  • 2. ContentAssetHelper
#91310
Edited, Oct 02, 2014 9:16
Vote:
 

I have now created a tech story that we should look over some of those classes. I guess we might take this story when we are approaching a 8 version since that makes it possible to do some minor breaking things (which might be needed to solve this in a way we think is appropriate, perhaps break the classes into an abstract class and a implementation class).

#91313
Oct 02, 2014 9:50
Vote:
 

Awesome stuff!

#91314
Oct 02, 2014 10:02
This thread is locked and should be used for reference only.
* You are NOT allowed to include any hyperlinks in the post because your account hasn't associated to your company. User profile should be updated.