Friday, 29 June 2012

Why do I consider interfaces superior to classes for mocking?

There's a debate of using classes instead of interfaces for mocks. You get a class mock by taking a class that has at least some of its public methods virtual and make a subclass where you override those methods and inject your own behaviors. Proponents of this technique point out the following advantages over interface mocks:

  1. No interface bloat
  2. Easier navigation in IDEs (pointing to method usage and choosing an option like "go to definition" takes you to the method implementation rather than the interface signature)
  3. No need to maintain another set of method signatures in interfaces - less work when changing things.

Personally, I myself use class mocks very rarely. For the following reasons:

1. Trouble overriding the implementation.

When you intend to mock a class, you have to watch out how you implement it. There are parts of class implementation that cannot be overridden, and these parts, when done wrong, may affect your specs/Unit tests in an unexpected way. Let's examine the following, bad from mocking perspective, example of a class:

public class PersonDao
{
  public PersonDao()
  {
    if(!wrappedTable.Exists)
    {
      wrappedTable.Create();
    }
  }

  public virtual void Close()
  {
    connection.Close();
  }

  private Table wrappedTable 
   = new Table("Person", "CreatePersonTableDdl.sql");
  private Connection connection = Connection.Open();
}

This is an example of a simple DAO class.

Let's take a look at the constructor first - someone tried to be clever and wanted to make sure that whenever we create a data access object, the table wrapped by it exists. This is bad, because creating a mock of this class (which is almost always about creating a subclass at runtime - this is how most mocking libraries work) ends up invoking the class's constructor. The effect? Each time we make a new mock, an attempt will be made to access the database and there's nothing we can do about it (apart from setting up a database on every machine that runs your unit tests, but believe me, you don't wanna do it).

Now look at the bottom of this class definition - here we've got an inline initialization of two fields, one of which is initialized with an open connection, which also cannot be mocked - it will always be executed when creating instances of this class or any subclass (including a mock). The only way to deal with such static method invocation as seen here is to add some extra implementation to the Connection class that would allow us to somehow set a fake instance to be returned by this method. But let's imagine that this is actually a third party class and we have no access to the source code. What can we do? again - nothing.

There's only one way to deal effectively with issues described above - we have to change the design to allow dependency injection, plus, such design has to be maintained as the code evolves. My practical experience says it's unrealistic to expect every programmer that works with you to understand design guidelines that apply to mockable classes. Even if you train every developer in your team, sooner or later someone new will come and make changes against those guidelines. So, either turn into a policeman or give up.

2. Maintaining virtuality

In order to be able to mock a method inside a class, the method has to be virtual. It's best to keep all methods virtual in order to keep the mocks maintainable - when some of the methods are virtual and some of them not, it's hard to actually figure out what actually gets called in given circumstances and hard to read specifications that use such mocks - each time you read a spec, you have to go to the class definition to check whether the method in question will be mocked or not. Again, all of this requires everyone to follow strict class design guidelines, which is troublesome to enforce.

3. Constructor maintenance

Given such class:

public class MockedObject
{
  public MockedObject(int arg)
  {
  }
}

..Let's execute this code using a popular Moq mocking framework:

[Test]
public void ShouldAllowSubstitutingConcreteObjects_Moq()
{
  var x = new Mock<MockedObject>();
  var y = x.Object;
}

..or this one using NSubstitute framework:

[Test]
public void ShouldAllowSubstitutingConcreteObjects_NSubstitute()
{
  var x = Substitute.For<MockedObject>();
}

Both Moq and NSubstitute rely on Castle.Proxy library to create proxy objects (i.e. runtime objects subclassing the class). So no wonder both of these snippets give you the same result:

System.ArgumentException: Can not instantiate proxy of class: 
Playground.OutExample+MockedObject.
Could not find a parameterless constructor.
Parameter name: constructorArguments
  at Castle.DynamicProxy.ProxyGenerator.CreateClassProxyInstance...

Why is that? In both cases we create a new mock without supplying any parameters to the constructor, which takes one parameter - even when creating a subclass, we still have to go through superclass's constructor (because it's the only constructor available). Now let me show you a corrected version of both of these snippets:

[Test]
public void ShouldAllowSubstitutingConcreteObjects_Moq()
{
  var x = new Mock<MockedObject>(Any.Integer());
  var y = x.Object;
}

[Test]
public void ShouldAllowSubstitutingConcreteObjects_NSubstitute()
{
  var x = Substitute.For<MockedObject>(Any.Integer());
}

It means every time the constructor changes, we have to update the constructor's parameter list in each mock creation, which makes such mocks hard to maintain.

All of these points make me accept gladly the interface bloat and mock interfaces rather than classes. I tend to use class mocks in some special cases only (like specifying abstract classes by mocking abstract methods and specifying how non-abstract methods of the same object use them).

And that's it. Maybe next time I'll show you how to deal with some of the downfalls of class mocks mentioned here. In the meantime, take care and enjoy your weekend ;-)

Saturday, 23 June 2012

Mock the operating system: timers example

Hi, today, I'd like to mention the topic of mocking the operating system and getting rid of it (especially the non-deterministic parts) as we write our specifications AKA unit tests.

The use cases

In general, there are two cases where we interact with the operating system:

  1. We tell our code to use the operating system's resources (start a thread, write to a hard drive etc.)
  2. We tell the operating system to use our code (when a timer expires, when a thread execution finishes etc.)

Timers

The topic of today's post is a combination of both above cases. Timers work as a way to say "in X milliseconds (or any other time unit), invoke Y and in the meantime, let me do my job."

The scheduling phase is where our code uses the timer and the calling back phase is where the timer uses our code. But before I show you how to properly handle this kind of situation, let's examine how things can go wrong.

A Baaad Idea

Let's assume that our specified class invokes an action periodically, each time passing in the current time. Also, let's assume that we don't care about the current time value (e.g. whether the passed time is really current) - in the real world we would, but this is just a toy example, so please bear with it. The most widely seen (but wrong - read on) way of writing a spec of a class that executes some code periodically usually consists of the following steps

  1. Make the object using the timer start this timer with a callback interval of X (so each X units of time, let's say seconds, the timer will call back)
  2. Sleep for X*Y units of time (where Y is any positive integer) + some slight tolerance
  3. After the spec wakes up from the sleep, assert that callback was called Y times

Let's quickly examine how would an implementation of such reasoning look like:

[Test]
public void ShouldPingPeriodically()
{
  int actualCallbackCount = 0;
  int interval = 4000;
  int expectedCallbackCount = 3;
  int tolerance = 1000;

  var timeNotification = 
    new CurrentTimeNotification( _ => actualCallbackCount++ );

  timeNotification.Schedule(interval);
  Thread.Sleep(interval * expectedCallbackCount + tolerance);
  timeNotification.Halt();

  Assert.AreEqual(expectedCallbackCount, actualCallbackCount);
}

This approach has one disadvantage - the interval * expectedCallbackCount is not the total time that is spent in tested code - there's one more thing we have to take into account.

There are different ways a timer can work. Either it waits for one callback to finish until it fires new wait period (this is not the case with most today's library timers, but may be with some homegrown ones). In this case, the added duration is the duration of execution of the callback passed to CurrentTimeNotification constructor. The way most of today's timers work is to fire each callback in a thread taken from thread pool. Using a thread pool boosts performance, so it's not a big penalty, yet the penalty on retrieving the thread from the pool exists. Also, usually we don't invoke our callbacks directly from the timer, because callbacks executed by the timer often require a specific signature which our passed action does not provide (if you're curious about the details, read TimerCallback delegate documentation on MSDN), so probably there is an additional code that's executed by the timer and this code invokes the action that we pass in. Usually the 1-second tolerance is more than enough to cater for all of this (by the way, remember that in case the timer fires a callback multiple times, it's really 1 second for the sum of all the calls, so each call must take under (1/expectedCallbackCount) seconds), but what if we run unit tests on a developer machine that's performing another build at the same time and is short on resources? Can the execution of the additional code that we just mentioned take more than the assumed tolerance? Of course it can! So what do we do about it?

A Better solution

The time has come to draw a line between the behaviors of our code and the operating system. In a specification of a class using a timer, we want to describe three behaviors in particular:

  1. It should create the timer passing desired method as callback
  2. It should schedule the timer for desired number of time units
  3. It should perform a desired behavior when the callback is invoked

To achieve this goal, we have to get rid of the timer by creating a thinnest possible wrapping layer. This layer has to be super thin because it's the code we're NOT going to write specs against. So, here we go - a timer wrapper:

public interface IPeriodicExecution
{
  TimerCallback Callback { get; }
  void Schedule(int milliseconds);
  void Halt();
}

public class PeriodicExecution : IPeriodicExecution
{
  private Timer timer;

  public TimerCallback Callback
  {
    get; private set;
  }

  public PeriodicExecution(TimerCallback callback)
  {
    timer = new Timer(callback);
    Callback = callback;
  }

  public void Schedule(int milliseconds)
  {
    timer.Change(0, milliseconds);
  }

  public void Halt()
  {
    timer.Change(0, Timeout.Infinite);
  }
}

As you can see, the wrapper is really thin. The only unclear element is this Callback property. We'll reveal what it is for in a moment. In the meantime, note that the timer callback is passed in a constructor. We can't mock constructors, so from the list of the three behaviors outlined in the beginning of this section, we wouldn't be able to specify the first one. In order to be able to do that, we need to introduce another thin layer - a factory!

public interface IPeriodicExecutionFactory
{
  IPeriodicExecution Create(TimerCallback callback);
}

public class PeriodicExecutionFactory
{
   public 
   IPeriodicExecution Create(TimerCallback callback)
   {
     return new PeriodicExecution(callback);
   }
}

By the way, this is not the main topic of this post, but note that because the IPeriodicExecution interface has this callback property I mentioned, we can actually test-drive this factory object in the following way:

[Test]
public void ShouldCreatePeriodicExecutionWithSpecifiedCallback()
{
  var anyCallback = Substitute.For<TimerCallback>();
  var factory = new PeriodicExecutionFactory();
  var periodicExecution = factory.Create(anyCallback);

  Assert.AreSame(anyCallback, periodicExecution.Callback);
}

Ok, now we're ready to write the specs for the three behaviors from this section start. The first one: "Should schedule the timer for desired number of time units":

[Test]
public void 
ShouldSchedulePeriodicExecutionEverySpecifiedNumberOfMilliseconds()
{
  //GIVEN
  var anyAction = Substitute.For<Action<DateTime>>();
  var factory = Substitute.For<IPeriodicExecutionFactory>();
  var periodicExecution = Substitute.For<IPeriodicExecution>();
  var anyNumberOfMilliseconds = Any.PositiveInteger();
  var notification = new CurrentTimeNotification2(anyAction, factory);

  factory.Create(notification.NotifyAboutCurrentTime).Returns(periodicExecution);

  //WHEN
  notification.Schedule(anyNumberOfMilliseconds);

  //THEN
  periodicExecution.Received().Schedule(anyNumberOfMilliseconds);
}

And the second one: "Should perform a desired behavior when the callback is invoked":

[Test]
public void 
ShouldInvokeActionWithCurrentTimeWhenTimerExpires()
{
  //GIVEN
  bool gotCalled = false;
  var factory = Substitute.For<IPeriodicExecutionFactory>();
  var notification = 
    new CurrentTimeNotification2(_ => gotCalled = true, factory);

  //WHEN
  notification.NotifyAboutCurrentTime(Any.Object());

  //THEN
  Assert.IsTrue(gotCalled);
}

And that's it. Note that this set of two specifications better describe what the actual object does, not how it works when connected to the timer. This way, they're more accurate as specifications of the class than the one using Thread.Sleep().

Summary

In this post, I tried to show how to approach timer-based designs. The same approach can be used for mocking pretty much every non-deterministic or time-based or threaded element in our implementation.

Note that this is a toy example In the real life, I'd strongly consider not creating a new timer for each new request, but pass a IPeriodicExecution instance by constructor instead of factory and reschedule it every time the request is made. This way, I'd not loose track of the started timers as I do in this example.

And remember, try to keep your design separated from your implementation.

Wednesday, 13 June 2012

TDD Series: Test-driving attributes

This post is mostly C#-specific, although Java guys can make some use of the ideas presented here.

Simplest attributes

Sometimes, especially when dealing with third-party frameworks such as WCF, you scratch your head for how far can you really go with unit tests to specify that "method X should be a remote operation?". As we all know, firing up a WCF server just to make a call inside unit test is unacceptable. What do we have left?

Let's see, remote operations are marked with OperationContract attribute:

[ServiceContract(Namespace="X.Y.Z")]
public interface RemoteEntity
{
  [OperationContract]
  void DoSomething();
}

So, this is what really defines a remote operation: an attribute. So why not test-drive it? You may think this is insane, until you realize that attributes are just classes that are instantiated and configured at runtime. So, if you test-drive assigning values to C# properties, why not attributes?

Ok, I assume that you calmed down after the last paragraph and don't consider this idea crazy (because it's not). So, the question changes to "how do we specify that method X has attribute Y?". It appears that nUnit has the capability to do this, but it's quite limited.

I've decided to design my own helper, also as an exercise in coding. Using reflection, I came up with the following API for asserting the attribute:

[Test]
public void ShouldContainRemoteOperationForDoingSomething()
{
 Assert.IsTrue(
 Method.Of<RemoteEntity>(entity => entity.DoSomething())
   .HasAttribute<OperationContractAttribute>()
 );
}

And here's the implementation:

public class Method
{
  public static Method Of<T>(Expression<Action<T>> expression)
  {
    return new Method((expression.Body as MethodCallExpression).Method);
  }

  public bool HasAttribute<T>()
  {
    return Attribute.IsDefined(methodInfo, typeof(T));
  }

  private Method(MethodInfo method)
  {
    methodInfo = method;
  }

  private MethodInfo methodInfo;
}

Note that the method Of() takes a lambda expression consisting solely of a call to the method we want to check. Also note that this lambda expression is actually never called! We need it only to extract the metadata for the method that the expression is referring to. Also note that the expression must consist of a single call - otherwise we won't be able to cast it to MethodCallExpression.

Ok, so what if our DoSomething() method takes a parameter? We have to provide it, but the value doesn't really matter, since (remember!) the call is never made and method metadata is extracted from the expression by signature, not by call parameters. So, assuming DoSomething() takes an integer, we can write:

[Test]
public void ShouldContainRemoteOperationForDoingSomething()
{
 Assert.IsTrue(
  Method.Of<RemoteEntity>(entity => entity.DoSomething(Any.Integer()))
   .HasAttribute<OperationContractAttribute>()
 );
}

Easy, right? Ok, but what if we have an attribute that takes a constructor parameter and some properties?

Attributes with properties and constructor parameters

Consider this:

public class Book
{
  [StringLength(256, ErrorMessage = "Max 256 characters!")]
  public string GetDescription();
}

Here, we have to use more elaborate techniques, but we'll try to keep the syntax clean:

[Test]
public void ShouldContainLengthValidationFor256Characters()
{
  Assert.IsTrue(
    Method.Of<Book>(book => book.GetDescription())
      .HasAttribute(new StringLengthAttribute(256) 
        { ErrorMessage = "Max 256 characters!" })
  );
}

All we have to do is to create an overload for HasAttribute(), taking one parameter instead of none. Note that, compared to the previous examples, in this one, we add the actual instance of the attribute object we are comparing against. This leads to one complication - while the comparison of types was relatively easy to achieve through the Attribute.isDefined() method, comparing objects requires equality to be properly implemented for those objects.

For now, int the following implementation, we'll leave the equality issue for later and close it inside a method named AreEqual(), then I'll show you two ways to go forward with the equality check.

public bool HasAttribute<T>(T expectedAttribute) where T : class
{
  var attrs = Attribute.GetCustomAttributes(methodInfo, typeof(T));
  var any = attrs.Any(
    currentAttribute => AreEqual(expectedAttribute, currentAttribute)
  );
  return any;
}

Now for the two methods of filling in this AreEqual() gap. The first option is quite straightforward:

private static bool AreEqual<T_Attribute>
(
  T_Attribute attr1, Attribute attr2
) where T_Attribute : class
{
  return attr1.Equals(attr2);
}

The implementation above requires the attribute classes to have properly defined equality, which may turn out not to be true in some third party libraries you may be forced to use in your project.

Another option is to use Ploeh's Semantic Comparison library, which is a part of Autofixture. It contains a Likeness class that we can use to write the following:

private static bool AreEqual<T_Attribute>
(
  T_Attribute attr1, Attribute attr2
) where T_Attribute : class
{
  return attr2 is T_Attribute 
    && new Likeness<T_Attribute, T_Attribute>
       (attr2 as T_Attribute).Equals(attr1);
}

Likeness works by searching both objects and comparing their public fields and properties (By the way, it's interesting in that it can compare unrelated types, but that's beyond the scope of this post).

Why is it reasonable to compare two attributes by their "likeness"? It's because attributes are mostly used as containers for the information. So, any information you put into the attribute by setting its properties and passing parameters to a constructor, you sooner or later want to get back from it and that's usually achieved by properties.

What about class attributes?

So, what about classes and their attributes? Luckily, nUnit has a decent support for these - just look it up in the documentation! Also, if you dig deep enough into the mentioned topic on nunit discussion list, you'll reach a way to achieve the same end result as this article shows by using extension methods to plug into nUnit's objects used for fluent assertions. It's a good idea to use what I've shown you as a building block for that if you decide to perform this exercise.

Good night everyone and sweet dreams!

Monday, 11 June 2012

TDD Series: How to test-drive decorators - adding same logic to all wrapped methods

Hi, today we're gonna talk a little bit about test driving decorators. In particular I'm going to take on a case where we are wrapping all methods of decorated object with the same logic (a good example is synchronization or tracing).

Synchronized decorator - a simple example

The example I'm gonna give is very simplistic, however, there's no need to complicate it, since all more complex objects follow the same pattern.

Let's imagine that we're creating a stack. We support the following operations:

  1. put element on top of a stack ("push element")
  2. get last put element from stack without removing it
  3. get last put element from stack and remove it ("pop element")
  4. clear the stack
  5. copy all elements of the stack on top of another stack

Now, let's imagine we have already test-driven an implementation that follows this interface:

public interface Stack<T>
{
  void Push(T element);
  T GetTopElement();
  T PopElement();
  void Clear();
  void CopyElementsOnTopOf(Stack<T> anotherStack);
}

Everything's fine until a new project is started that handles multiple simultaneous requests made to the stack object. So, we need to add synchronization to the stack.

If you've read other posts on this blog, you know that we like cohesion and Single Responsibility Principle very much and tend not to break it. That's why baking in concurrency inside the stack class (which already has a responsibility of managing storage and access to the elements) is not an option. After considering all pros and cons, we decide to make a synchronized wrapper that would implement Stack interface and route each method call to the original stack with synchronization added. Also, to make things more efficient (by the way, it doesn't always make things more efficient, but let's assume that it does in our case), we decide to use reader-writer lock (i.e. when writer lock is obtained, no one else can use the object. When reader lock is obtained, no one can obtain the writer lock, but other readers can use the object simultaneously).

How do we do it?

First, classify the operations

Looking at the method names, it's pretty evident which are "write" operations and should be guarded by writer lock and which are "read" operations and should be guarded by reader lock.

The "write" operations are:

  1. put element on top of a stack ("push element")
  2. get last put element from stack and remove it ("pop element")
  3. clear the stack

And the "read" operations are:

  1. get last put element from stack without removing it
  2. copy all elements of the stack on top of another stack

Ok, done. What's the next step?

Remember - no redundancy.

What we'd like to be able to do is to write the result of this analysis in the form of executable specifications AKA unit tests. So, for example, such specification would say: "putting an element on top of a stack should perform the same operation of wrapped object, with write access acquired". The same would go for all other "write" operations. So, not to be redundant, we'd like to encapsulate the "write access acquired" into its own method (and maybe in its own object, but let's leave that alone for now).

Also, from what we know about synchronization, the workflow for performing operations in context of a lock is like this (example for writing):

  1. acquire write lock
  2. perform operation
  3. release write lock

What we'd like to do is to leave the middle step varying while encapsulating the first and last step (i.e. a context of the operation) into one method. There are at least three ways to do it in C# (pass the middle step as parameter to lambda expression or implement IDisposable and use using construct or use some funny hacking that will be a topic of a separate post ;-)), of which we'll choose the lambda solution (since it's applicable in other languages as well, so I expect to draw the most value for you - my readers - from it).

Applying the same reasoning to the "read" methods, we can come up with the following interface of the synchronization decorator:

public class SynchronizedStack<T> : Stack<T>
{
  public void Push(T element);
  public T GetTopElement();
  public T PopElement();
  public void Clear();
  public void CopyElementsOnTopOf(Stack<T> anotherStack);
  
  //New operations not in Stack<T> interface:
  public SynchronizedStack(Stack<T> stack);
  public virtual void InWriteContext(Action action);
  public virtual void InReadContext(Action action);

  //This is the read-write lock we were talking about.
  public readonly ReaderWriterLockSlim SyncObject 
    = new ReaderWriterLockSlim(LockRecursionPolicy.SupportsRecursion);

}

Now, for each method, we should write two specs. In the following example, please note how we're going to deal with verifying that a method was called in a lambda expression that's passed to another method (this is a situation where matchers built inside mock object frameworks won't suffice, at least not in C#). The example is for PopElement():

[Test]
public void ShouldPopElementInWriterContextIfWriterContextImplemented()
{
  // GIVEN
  var anyPoppedElement = Any.Integer();
  var wrappedStack = new Mock<Stack<int>>();
  var synchronizedStack 
    = new Mock<SynchronizedStack<int>>(wrappedStack.Object);
  wrappedStack.Setup(m => m.PopElement()).Returns(anyPoppedElement);

  //Any action passed to write context will just be executed
  synchronizedStack
    .Setup(m => m.InWriteContext(It.IsAny<Action>()))
    .Callback<Action>(a => a.Invoke());

  // WHEN
  var element = synchronizedStack.Object.PopElement();

  // THEN
  Assert.AreEqual(anyPoppedElement, element);
}

[Test]
public void 
ShouldNotPopElementInWriterContextIfWriterContextNotImplemented()
{
  // GIVEN
  var wrappedStack = new Mock<Stack<int>>();
  var synchronizedStack 
    = new Mock<SynchronizedStack<int>>(wrappedStack.Object);

  // Any action passed to write context will 
  // not be executed - empty implementation:
  synchronizedStack
    .Setup(m => m.InWriteContext(It.IsAny<Action>()))
    .Callback(() => {});

  // WHEN
  synchronizedStack.Object.PopElement();

  // THEN
  wrappedStack.Verify(m => m.PopElement(), Times.Never());
}

The whole trick is about providing a different implementation for InWriteContext() each time: in the first spec, the implementation is just a pass-through and invokes everything it gets. In the second spec, the implementation ignores anything it gets. So, if in first spec the PopElement() of wrapped object gets called and in the second one it doesn't, it must go through the InWriteContext(). Also note that in the first spec I don't use Verify() - that's ok since I've got the return value PopElement()'s result to assert against and check the call this way.

Ok, what more do we need? We have certainly omitted a spec for the SyncObject field (I decided to use a field instead of property, but you may choose otherwise), where it should be specified that it should be a non-null object and that it should support recursion (that's my design choice, yours may be different). Also, we need to somehow specify the InWriteContext() and InReadContext(). Each of them needs two specs:

  1. The proper lock should be acquired before the call and release it after the call finishes successfully
  2. If any exception is thrown by the action executed in context, the lock should be released anyway

Let's go with an example for InWriteContext()

[Test]
public void 
ShouldObtainWriterLockDuringTheCallAndReleaseItAfterTheCall()
{
  // GIVEN
  var wrappedStack = new Mock<Stack<int>>();
  var synchronizedStack 
    = new SynchronizedStack<int>(wrappedStack.Object);
  var gotCalled = false;

  // WHEN
  synchronizedStack.InWriteContext(() => 
  {
    gotCalled = true;
    Assert.IsTrue(synchronizedStack.SyncObject.IsWriteLockHeld);
    Assert.IsFalse(synchronizedStack.SyncObject.IsReadLockHeld);
  });

  // THEN
  Assert.IsTrue(gotCalled);
  Assert.IsFalse(synchronizedStack.SyncObject.IsWriteLockHeld);
  Assert.IsFalse(synchronizedStack.SyncObject.IsReadLockHeld);
}

[Test]
public void 
ShouldReleaseWriteLockWhenAWrappedCallExecutionEndsWithException()
{
  // GIVEN
  var wrappedStack = new Mock<Stack<int>>();
  var synchronizedStack 
    = new SynchronizedStack<int>(wrappedStack.Object);

  // WHEN
  Assert.Throws<Exception>( () =>
    synchronizedStack.InWriteContext(() => {throw new Exception();})
  );

  // THEN  
  Assert.IsFalse(synchronizedStack.SyncObject.IsWriteLockHeld);
  Assert.IsFalse(synchronizedStack.SyncObject.IsReadLockHeld);
}

Is it worth the hassle?

"Well", you might say, "it's two specs for each InXContext() plus two per each wrapped method. Isn't it too much for a wrapper that's implemented almost mechanically?". And you might be right. This is a case where I'd weigh pros against cons - you might decide not to test-drive this code, since the only value added by such tests is of the "living documentation" sort. I usually do it, but if you decide it's stupid, I won't blame you (much :-)). Another reason why I decided it's a good blog post is that it provides one way of specifying that something goes through lambda, which are a real horror for mock frameworks to match (you can't just say: match every method with argument being a lambda like THIS, at least not always). You could decide to go another way: take the specs we've written for InWriterContext() as a template, and use it for every method, just ignoring the existence of lambda and InXContext() methods and focus on the synchronization in each spec, but that would be potentially harder to maintain.

In one of my future posts, I'll show you how to achieve one spec per wrapped method instead of two by using some C# magic, but for now, that's it.

Good night, everyone!

Thursday, 7 June 2012

TDD series: Null object specifications - Booby Trap pattern

Hi, Today I'd like to write a little about specifying null objects and a simple Booby Trap pattern that can be used for this purpose.

A case for a null object?

First, I'll try to introduce the example we're going to work on, at the same time introducing the null object pattern briefly for those that don't know it (are there such programmers?)

Let's imagine that you're selling an email gateway that stands between your customer's network and the rest of the world. Its job is to do some kind of processing of incoming and outgoing email and then pass it forward in the processed form to the customer's network. In order to lower the traffic inside this network, the incoming email is compressed and to improve security, the outgoing mail is encrypted.

Now imagine that for various regulatory reasons, some customers need to encrypt the e-mail using one algorithm, while others use another algorithm. Also, there are customers that use more than one algorithm interchangeably depending on a day of month. So for different customers, you're maintaining separate encryption algorithms and allow swapping them at runtime. The conceptual class diagram may look like this:

One day, another customer appears and says "we don't want the encryption, since we already have a solution that handles this issue. All we want is the compression". You're then left with two choices: either to treat "no encryption" as a special case and put an if-else (i.e. a variation of behavior) into the processing logic, or follow the Gang of Four's principle of encapsulating what varies and put the variation into the factory, treating "no encryption" as a kind of encryption. The factory then would be able to create an encryption object that... does not encrypt at all. We chose the second option, of course. So let's quickly add it to our diagram:

This kind of object - an object that encapsulates "doing nothing" when filling a specified role - is called a null object.

Specifying null object - does that make any sense?

What's the point of specifying "nothing"? Isn't it a nonsense similar to specifying that the code will not accidentally format the hard drive?

I may be alone in this opinion, but I think that in this case it does make sense. The main difference is that usually in case of null objects, our "nothing" is pretty well defined.

Let's go back to our example and the null encryption object. What's its definition of nothing? It's "it should not encrypt the message". But can it, e.g. Write something into the system log? Sure it can! So the "nothing" is really related to the message only. Hence, in the most strict case, we want to specify that the message shouldn't be touched at all (the more loose case will be covered by a separate post somewhere in the future).

Booby traps - how to specify that something should not be touched?

Let's consider a null object that should REALLY do nothing with the message (a null object that triggers some behavior on passed object is more a complex topic - maybe I'll write a follow up to this post discussing this particular case). What can we do to specify this requirement?

We can use a testing pattern that I call a Booby Trap (although, as you'll see in a minute, the pattern can so be trivially implemented that one may argue whether it deserves its own name). In the real world, there are many kinds of booby traps - ranging from fun ones that make a noise when stepped on, to the military ones that may make a massive damage. What we'll use this time is an equivalent of the latter.

The easiest way to implement a full booby trap is to use a mocking framework that has a feature called Strict Mocks. Strict mock is a variant of mock object that can only receive calls that we mark as "expected". Any other call made on a strict mock blows things up with an exception and the test fails. So, the easiest way to implement a full booby trap is to just use a strict mock without ANY calls marked as "expected". Of course, as mock objects rely mostly on inheritance, you'd have to pass an interface mock or mark all methods as virtual in your mocked class, but that's a relatively small price to pay.

Having such a strict mocks, you can use it like this (example with Moq framework):

[Test]
public void ShouldNotPerformAnyKindOfOperationsOnMessage()
{
  //GIVEN
  var nullEncryption = new NullEncryption();
  var messageBoobyTrap = new Mock<Message>(MockBehavior.Strict);

  //WHEN
  nullEncryption.Perform(messageBoobyTrap.Object);

  //THEN
  messageBoobyTrap.VerifyAll();
}

There is, however, some bad news - the Strict Mock pattern is considered a relic of the past (the new and better way of mocking is to use so called Loose Mocks, that by default ignore unexpected calls instead of blowing things up) and many newer mocking frameworks don't implement them (e.g. NSubstitute or FakeItEasy do not). In some cases we can still get the same benefit when the framework allows us to access the information on how many calls were made on a mock. The following example uses NSubstitute to do just that:

[Test]
public void ShouldNotPerformAnyKindOfOperationsOnMessage()
{
  //GIVEN
  var nullEncryption = new NullEncryption();
  var messageBoobyTrap = Substitute.For<Message>();

  //WHEN
  nullEncryption.Perform(messageBoobyTrap);

  //THEN
  CollectionAssert.IsEmpty(messageBoobyTrap.ReceivedCalls());
}

It gets a little bit harder in maintenance with manual mocks, since your mock has to derive from the actual class and mechanically override each method as throwing exception - then you have to remember to add such override for each new method that is added later to the actual class. While this is relatively easy in case your mock implements an interface directly (because the compiler error will tell you that your mock does not implement added method and you can just go and add your implementation - you never forget), in case of mocks deriving from actual/abstract classes it can be a maintenance pain.

That's it for today, have a good day!

Saturday, 2 June 2012

How to test private methods?

(Thanks go to Oleksandr Kravchenko for inspiration.)

This question gets asked over and over again. Actually, this is the question I've been asking myself for quite a long time.

Until I realized it's the wrong question to ask. So let's rephrase the question a little, rewind and start over:

What to do when you feel you need to test private methods?

Now, that's better :-)

Basically there are few ways to tackle this issue, depending on circumstances. I'll try to point them out starting from simplest.

1. Don't do it.

As Liz Keogh pointed out (using BDD language, but it applies to TDD as well), unit tests in TDD or BDD are examples of how to use objects of the class and how is this class valuable to other classes. As such, unit test (or unit-level specifications if you will) should go through the public interface only (with an exception, that I'm going to introduce later).

Let's consider a code example:

public class DishWasher
{
  private DishWashing dishWashing;
  private int capacity;

  public DishWasher(int capacity, DishWashing dishWashing)
  {
    this.dishWashing = dishWashing;
    this.capacity = capacity;
  }
  
  public void Wash(Dishes dishes)
  {
    if(IsEnoughPlaceForAll(dishes)
    {
      dishWashing.PerformOn(dishes);
    }
  }

  private bool IsEnoughPlaceFor(Dishes dishes)
  {
    return dishes.CountIsLessThan(this.capacity);
  }
}

Here, the private method is used solely to enhance readability. There's no need to specify it separately. We can easily write two specifications:

  1. Given I have a new set of dishes And the count of dishes is less than washer capacity, When I try to wash the dishes in the washer, Then it should perform the washing process.
  2. Given I have a new set of dishes And the count of dishes is equal to or greater than washer capacity, When I try to wash the dishes in the washer, Then it should not start the washing process.

And that's all - no need to excercise the private mehod, because from the specification point of view, it does not exist - the "washer capacity" thing is abstract enough to leave it in the spec.

However, let's look at what I did with the actual washing - I made it a separate class and passed its instance to the washer. That's interesting - why isn't washing just a private method of the DishWasher class?

2. Move a responsibility by Introducing a collaborator (TDD) or moving method (Refactoring)

First, let's look at it from TDD (and Test-First) point of view.

Usually, when doing TDD, we always think "outside the object", because the "inside" is always a result of failing spec/test and comes AFTER the spec. So, following the TDD process, we'd have no code and start with a specification in mind (let's remind it here):

Given I have a new set of dishes And the count of dishes is less than washer capacity, When I try to wash the dishes in the washer, Then it should perform the washing process.

Looking at this spec, we quickly run into conclusion that the behavior description of the dish washer ends with starting the washing process. We didn't go any further than that. How can we use this information? We can notice that there are actually two responsibilities involved.

The first one is maintaining the right functionality of controlling the device (here, we're dealing with making sure the washing does not start when too many dishes are put inside, but it will probably include other scenarios soon, like "do not start washing when washing is in progress" or "Only start washing when the washer is turned on", or "Stop washing when washer is turned off" etc.)

The other responsibility is the washing process, which is not started at the same time the washer turned on, so it is something more separate and has a logic of its own (e.g. how many phases are there, when to apply the detergent, how long does it take, how's the temperature of the water going to change along the washing process etc.).

As we discover, we did't specify the second part at all, so looks like for us, the responsibility of the washer ends with starting the washing process. Following Robert C. Martin's Single Responsibility Principle, it's good to move this washing process into a separate class. So, In TDD, you don't think about privacy or not - you think about _the_spec_ and it often tells you when you need to introduce a new collaborator by stressing different responsibilities. Private methods are introduced during the refactoring phase when you need to make some things a little more readable or eliminate redundancy.

Ok, now let's look from the refactoring perspective.

Imagine that you're dealing with a legacy code. All you have is the DishWasher class with the dish washing process coded inside as a private method. What you surely need to do is make sure that washing works OK by writing some unit tests that no one wrote ever before. In this case, it's usually useful to ask yourself a question: "Why do I think that I need to test the private method?". The answer is usually something like "because it has some complex logic, but well defined logic of it's own and I need to make sure that it works". Let's try to rephrase what you were trying to say by this (by the way, notice that a lot of this post up to now is about understanding your own reasoning :-) ). You actually said that "the washing is a separate responsibility and I have to make sure that it's fulfilled correctly". Well, if it's a separate responsibility, why is it in the DishWasher class? Quickly move the method into a new class and make it public! There - it's ready for testing! Wasn't THAT easy? :-)

Now, these two patterns (not testing and moving responsibilities) cover about 90% of cases. There is, however, a third case when you really want to test/specify private methods. Let's take a look at it now.

3. Use inheritance

(First, a disclaimer - I find this case very rare and usualy never run into it, so it was hard for me to find a good example. Please don't shout "hey, I can solve it by delegation instead of inheritance" - just bear with me :-)).

I'll start with the code, not the spec, since it's easier to understant the problem this way.

Let's say that you've got two models of dish washers. Each of them has a different policy for starting washing - the first one washes all dishes at once, and the seond one (a bigger one) implements a swapping functionality - it let's you put in twice as many dishes, then splits them into two chunks and performs washing for the dirst chunk, then the second chunk. Other than that, the washing process is the same. Let's say that we decided to make subclasses for different washers, overriding the "start washing" functionality. So, our DishWasher is now abstract and its Wash() method is implemented like this:

  public void Wash(Dishes dishes)
  {
    if(IsEnoughPlaceForAll(dishes)
    {
      PerformWashing(dishes, dishWashing); 
    }
  }

  protected abstract void PerformWashing(dishes, dishWashing);

Now, the "small" dish washer implements the method the following way:

protected void PerformWashing(Dishes dishes, DishWashing dishWashing)
{
  dishWashing.PerformOn(dishes);
}

...and the "bigger", two-chunk washer does it like this:

protected void PerformWashing(Dishes dishes, DishWashing dishWashing)
{
  var chunks = dishes.DivideIntoChunks(2);
  dishWashing.PerformOn(chunks[0]);
  dishWashing.PerformOn(chunks[1]);
}

We've got two classes now, each of them inheriting the Wash() method from DishWasher class and implementing a different variant of starting the washing process. Do specify the whole Wash() method in both of these derived classes?

No. It's the responsibility of DishWasher and should be specified for it. But it's abstract. What do we do?

First of all, we're gonna use a partial mock inheriting from DishWasher to specify DishWasher's behavior. The only thing we want to specify/test is that the abstract PerformWashing() method actually gets called in certain circumstances, because this is where the DishWasher's responsibility ends. We can achieve this easily either with a mocking framework or with a simple manual mock:

public class DishWasherMock : DishWasher
{
  public bool callCount = 0;
  public DishWashing passedWashing = null;
  public Dishes passedDishes = null;
  
  protected void PerformWashing(Dishes dishes, DishWashing dishWashing)
  {  
    callCount++;
    passedWashing = dishWashing;
    passedDishes = dishes;
  }
} 

In the spec, we only care about whether this method was called once with correct arguments and that's it.

Now the more interesting part - what to do with the protected methods in the subclasses? We're gonna specify them as well! Let's see, who is allowed to access protected methods? Derived classes! Ha! Why not make our little test suite class derived from specific dish washer to allow it to access its protected members? Let's see how it works for the "small" dish washer:

public class SmallDishWasherSpecification : SmallDishWasher
{
  [Test]
  public void ShouldJustStartWashing()
  {
    var dishWashingMock = new DishWashingMock();
    var anyDishes = Any.InstanceOf<Dishes>();
    this.PerformWashing(anyDishes, dishWashingMock);
  
    Assert.AreEqual(1, dishWashingMock.callCount);
    Assert.AreEqual(anyDishes, dishWashingMock.passedDishes);
  }
}

Note that ShouldJustStartWashing() is an instance method, so it has access to all public and protected instance methods of SmallDishWasher class.

"Bigger" washer follows the same patterns, so I'll leave it to you as an excercise. By the way, this technique works quite fine with classes that have protected constructors as well (although I'd never make such a class, but I know some folks do, e.g. in singletons or abstract classes).

But what if something is private? Just make it protected - maybe shed a small tear, but in the long run, it's no big deal.

The last 1%

This covers 99% of cases where you "need to" test the private method. The last 1% are some corner cases that are hard for me to even imagine. You can then use some of the "last resort techniques", like friends (C++), InternalsVisibleTo (C#), instrumentation/profiling API, reflection etc. but I really advise you to strongly consider the three options mentioned above before going that far.

Ok, that's it. Have a good weekend!