Brendan Enrick's Blog

Daily Software Development.


Overmocking

by Brendan Enrick Friday, August 5 2011 13:28

One of the most powerful tool available to developers testing a legacy code base is the ability to mock out classes that their code depends on. This is of great importance since our unit tests need to limit the scope, and we do this by trying to limit our dependencies. Through mocking we can exchange one dependency on the infrastructure of our application for an in-memory mock.

Sometimes mocking is overused, and I am not just talking about cases where every objected gets mocked to the point where we’re testing nothing. I am talking about a different issue in mocking. I am talking about where developers put on their mocking blinders when unit testing. It’s an easy thing to do, and I’ve done it plenty of times myself.

Setting Up Our Example

We will start of by defining our example. We will have a method to do some sort of calculation (an ideal and easy testing scenario). It will be a legacy code base, which means that it is untested code and probably hard to test. We’ll start off by making it a private static method and put in a nice dependency that one might try to mock to avoid.

private static decimal CalculateFooOnXyz(Xyz xyzItem, 
decimal calculationParameter1)
{
var numbersInCalculation = Repository.GetNumbers()
.Where(n => n.IsActive);

foreach (Number number in numbersInCalculation)
{
// Some code that executes for each one.
}
}

Now that we have this method, which is scary and really needs some testing we’ll take a look at the simple way of testing it; we will use parameter injection to pass in the dependency since we’re static we not easily able to do any other safe forms of dependency injection. When we do this, we end up with the following code.
 
private static decimal CalculateFooOnXyz(Xyz xyzItem, 
decimal calculationParameter1, IRepository repository)
{
var numbersInCalculation = repository.GetNumbers()
.Where(n => n.IsActive);

foreach (Number number in numbersInCalculation)
{
// Some code that executes for each one.
}
}

This is a pretty simple change. We now mock out the repository and pass in the mock in our test and we tell it instead to return the collection we specify in memory instead of requesting the data from the database.

Why This is Wrong

My first question is, what is the name of the method? CalculateFooOnXyz. Notice that I didn’t say, “GetDataFromTheDatabase”. That’s because we shouldn’t be doing that. It isn’t part of the calculation. The numbers returned from the database are required for calculating, so that means that we should have that object as our dependency instead.

How We Change It

So instead of making the repository our parameter, we should make the collection of numbers our parameter. This way we’re depending on the in-memory collection of CLR objects. This is much less of a dependency, and in is not one that needs to be mocked. By doing this alternative we’re better following the Single Responsibility and Dependency Inversion principles.

Our best code for testing will look like this.

private static decimal CalculateFooOnXyz(Xyz xyzItem, 
decimal calculationParameter1, List<CalcNumbers> numbersInCalculation)
{
foreach (Number number in numbersInCalculation)
{
// Some code that executes for each one.
}
}

Comments? Have a better way of doing it? Did I make a mistake?

Comments

8/9/2011 3:44:13 AM #

pingback

Pingback from blog.cwa.me.uk

The Morning Brew - Chris Alcock  » The Morning Brew #912

blog.cwa.me.uk

8/9/2011 7:27:19 PM #

Ferret

That's all very well in isolation, but let's assume CalculateFooOnXyz is called by several parts of the program. The trade-off here as I see it is redundant code whererever CalculateFooOnXyz is called, eg wherever :

CalculateFooOnXyz(myXyz,1)

now becomes:

CalculateFooOnXyz(myXyz,1, Repository.GetNumbers())

(keeping in mind it is the only thing which will be passed in except for in test scenarios)

Is that really an improvement?

Ferret Australia

8/10/2011 6:06:05 PM #

Peter Tran

Great advice!  Never thought of it this way.

Peter Tran United States

8/15/2011 10:30:52 AM #

Brendan Enrick

@Ferret Thanks for comment. You're correct. If that were all over your codebase, you would need to pass in the parameter in a lot of different locations. If, however, in all of these locations it's the exact same collection being passed in, I can make a method that does that call specifically for me. The code in the new method would look like this.

CalculateFooOnXyz(myXyz, 1, Repository.GetNumbers());

One additional advantage here is that I can now do this same calculation on different sets of data.

This isn't the best solution in all cases, but in many of them I think there is great advantage to be gained by depending on simpler objects.

Brendan Enrick United States

9/1/2011 2:17:42 AM #

Andrei Rinea

"Comments? Have a better way of doing it? Did I make a mistake?"

Excellent tip! Except making numbersInCalculation of type IEnumerable<CalcNumbers> instead of List<CalcNumbers> I can't see any further improvement possible.

Andrei Rinea Romania

Comments are closed