How to implement Chain of Responsibility in a testable fashion?

Question

I'd like to implement a Chain of Responsibility pattern, taking care of the 'broken link' problem as follows:

 public abstract class Handler{

   private Handler m_successor;

   public void setSuccessor(Handler successor)
   {
     m_successor = successor;
   }

   protected abstract boolean handleRequestImpl(Request request);

   public final void handleRequest(Request request)
   {
     boolean handledByThisNode = this.handleRequestImpl(request);
     if (m_successor != null && !handledByThisNode)
     {
       m_successor.handleRequest(request);
     }
   }
 }

Seems like a common enough approach. But how is this testable with a protected abstract method? Ways to handle this seem to be:

1. Implement a test-only subclass of Handler that implements the abstract method. This seems bad for test maintenance.
2. Change the visibility of the abstract method to public, but I shouldn't need to change the SUT to accomodate the test.
3. Regard the abstract class as sufficiently simple to not require unit tests. Mmmm.
4. Implement unit tests for the handleRequest method on one or more concrete subclasses. But this doesn't seem like a sensible way to organize tests.
5. Is there someway to use a mock object? I've tried Mockito, but can't seem to get around the protected visibility.

I've read[1] that this sort of testing problem implies that the design is wrong, and suggest using composition rather than inheritence. I'm trying this now, but it seems strange that the recommended implementation of this pattern has this problem, yet I can't find any advice about unit testing it.

UPDATED: I've replaced the abstract class with dependency inversion as shown, and this is now easily easily testable using Mockito. It still looks like Chain of Responsibility... am I missing something?

// Implement a concrete class instead
public class ChainLink {

  // Successor as before, but with new class type
  private ChainLink m_successor;

  // New type, RequestHandler
  private RequestHandler m_handler;

  // Constructor, with RequestHandler injected
  public ChainLink(RequestHandler m_handler) {
    this.m_handler = m_handler;
  }

  // Setter as before, but with new class type
  public void setSuccessor(ChainLink successor) {
    m_successor = successor;
  }

  public final void handleRequest(Request request) {
    boolean handledByThisNode = m_handler.handleRequest(request);
    if (m_successor != null && !handledByThisNode) {
      m_successor.handleRequest(request);
    }
  }
}

Recce

Answer 1

If you use PowerMock + Mockito, you can write a test similar to this:

@RunWith(PowerMockRunner.class)
@PrepareForTest(Tests.class)
public class Tests {
    @Test
    public void testHandledByFirst() throws Exception {
        Request req = ...;
        Handler h1 = mock(Handler.class);
        Handler h2 = mock(Handler.class);

        when(h1, "setSuccessor", h2).thenCallRealMethod();
        when(h1, "handleRequestImpl", req).thenReturn(true);

        h1.setSuccessor(h2);
        h1.handleRequest(req);
        verify(h2, times(0)).handleRequest(req);
    }

    @Test
    public void testHandledBySecond() throws Exception {
        Request req = ...;
        Handler h1 = mock(Handler.class);
        Handler h2 = mock(Handler.class);

        when(h1, "setSuccessor", h2).thenCallRealMethod();
        when(h1, "handleRequestImpl", req).thenReturn(false);
        h1.setSuccessor(h2);

        h1.handleRequest(req);
        verify(h2, times(1)).handleRequest(req);
    }
}

Which will verify that your second handler's method is called when the first one returns false and that it is not called when it returns true.

Another option is to follow the well-known rule of "favor composition over inheritance" and change your class to something like this:

public interface Callable {
    public boolean call(Request request);
}

public class Handler {
    private Callable thisCallable;
    private Callable nextCallable;

    public Handler(Callable thisCallable, Callable nextCallable) {
        this.thisCallable = thisCallable;
        this.nextCallable = nextCallable;
    }

    public boolean handle(Request request) {
        return thisCallable.call(request) 
            || (nextCallable != null && nextCallable.call(request));
    }
}

Then you can mock it in this way (or use pretty much any mock framework since you don't have protected methods):

@RunWith(PowerMockRunner.class)
@PrepareForTest(Tests.class)
public class Tests {
    @Test
    public void testHandledByFirst() throws Exception {
        Request req = ...;
        Callable c1 = mock(Callable.class);
        Callable c2 = mock(Callable.class);
        Handler handler = new Handler(c1, c2);

        when(c1.call(req)).thenReturn(true);

        handler.handle(req);

        verify(c1, times(1)).call(req);
        verify(c2, times(0)).call(req);
    }

    @Test
    public void testHandledBySecond() throws Exception {
        Request req = ...;
        Callable c1 = mock(Callable.class);
        Callable c2 = mock(Callable.class);
        Handler handler = new Handler(c1, c2);

        when(c1.call(req)).thenReturn(false);

        handler.handle(req);

        verify(c1, times(1)).call(req);
        verify(c2, times(1)).call(req);
    }
}

In this solution, you can also make Handler inherit after Callable and then you are able to wrap it over any other callable which may have a successor and use the handler instead of the original callable; it is much more flexible.

Incunabulum

So to be clear PowerMock can stub abstract protected methods? – Recce

Yes. I have also edited my answer to show another way of doing it (by implementing your class slightly differently), where you avoid having to deal with protected methods altogether. – Incunabulum

Thanks - I was editing my question with something similar at the same time. This confirms my thinking, so I'll mark this as answered! Cheers! – Recce

Answer 2

I would go for option 1. That fake sub class is simple enough and you can place it in your tets class. There is nothing wrong with test-only sub classes.

Houdon

I did wonder if I was over complicating things. However, while the example given is simple I'm imagining that the abstract class could get more complicated over time. Its simple enough to implement a sub class now, but what if it evolves? – Recce

Well, if the abstract base class gets more complex over time, there could be something wrong with the design. Check to see whether your code and design complies with the SOLID principles. Big complicated base classes are often a sign of a violation of the SOLID principles and violating these principles often leads to unmaintainable software. When tests are hard to write, that too is often a sign of a violation of SOLID. – Houdon

Thanks - that was where my thinking was heading, and I have edited my question with an alternative approach. It still looks like the Chain of Responsibility but favors composition over inheritance. I wonder why Chain of Responsibility isn't described using this approach in the first place. – Recce