How can I refactor and unit test complex legacy Java EE5 EJB methods?

Question

My colleagues and I are currently introducing unit tests to our legacy Java EE5 codebase. We use mostly JUnit and Mockito. In the process of writing tests, we have noticed that several methods in our EJBs were hard to test because they did a lot of things at once.

I'm fairly new to the whole testing business, and so I'm looking for insight in how to better structure the code or the tests. My goal is to write good tests without a headache.

This is an example of one of our methods and its logical steps in a service that manages a message queue:

• consumeMessages

  • acknowledgePreviouslyDownloadedMessages

  • getNewUnreadMessages

  • addExtraMessages (depending on somewhat complex conditions)

  • markMessagesAsDownloaded

  • serializeMessageObjects

The top-level method is currently exposed in the interface, while all sub-methods are private. As far as I understand it, it would be bad practice to just start testing private methods, as only the public interface should matter.

My first reaction was to just make all the sub-methods public and test them in isolation, then in the top-level method just make sure that it calls the sub-methods. But then a colleague mentioned that it might not be a good idea to expose all those low-level methods at the same level as the other one, as it might cause confusion and other developers might start using when they should be using the top-level one. I can't fault his argument.

So here I am.

How do you reconcile exposing easily testable low-level methods versus avoiding to clutter the interfaces? In our case, the EJB interfaces.

I've read in other unit test questions that one should use dependency injection or follow the single responsibility principle, but I'm having trouble applying it in practice. Would anyone have pointers on how to apply that kind of pattern to the example method above?

Would you recommend other general OO patterns or Java EE patterns?

Answer 1

At first glance, I would say that we probably need to introduce a new class, which would 1) expose public methods that can be unit tested but 2) not be exposed in the public interface of your API.

As an example, let's imagine that you are designing an API for a car. To implement the API, you will need an engine (with complex behavior). You want to fully test your engine, but you don't want to expose details to the clients of the car API (all I know about my car is how to push the start button and how to switch the radio channel).

In that case, what I would do is something like that:

public class Engine {
  public void doActionOnEngine() {}
  public void doOtherActionOnEngine() {}
}


public class Car {
  private Engine engine;

  // the setter is used for dependency injection
  public void setEngine(Engine engine) {
    this.engine = engine;
  }

  // notice that there is no getter for engine

  public void doActionOnCar() {
    engine.doActionOnEngine();
  }

  public void doOtherActionOnCar() {
    engine.doActionOnEngine();
    engine.doOtherActionOnEngine(),
  }
}

For the people using the Car API, there is no way to access the engine directly, so there is no risk to do harm. On the other hand, it is possible to fully unit test the engine.

Answer 2

Dependency Injection (DI) and Single Responsibility Principle (SRP) are highly related.

SRP is basicly stating that each class should only do one thing and delegate all other matters to separate classes. For instance, your serializeMessageObjects method should be extracted into its own class -- let's call it MessageObjectSerializer.

DI means injecting (passing) the MessageObjectSerializer object as an argument to your MessageQueue object -- either in the constructor or in the call to the consumeMessages method. You can use DI frameworks to do this for, but I recommend to do it manually, to get the concept.

Now, if you create an interface for the MessageObjectSerializer, you can pass that to the MessageQueue, and then you get the full value of the pattern, as you can create mocks/stubs for easy testing. Suddenly, consumeMessages doesn't have to pay attention to how serializeMessageObjects behaves.

Below, I have tried to illustrate the pattern. Note, that when you want to test consumeMessages, you don't have to use the the MessageObjectSerializer object. You can make a mock or stub, that does exactly what you want it to do, and pass it instead of the concrete class. This really makes testing so much easier. Please, forgive syntax errors. I did not have access to Visual Studio, so it is written in a text editor.

// THE MAIN CLASS
public class MyMessageQueue() 
{

    IMessageObjectSerializer _serializer; 



    //Constructor that takes the gets the serialization logic injected
    public MyMessageQueue(IMessageObjectSerializer serializer)
    {
        _serializer = serializer;

        //Also a lot of other injection 
    }


    //Your main method. Now it calls an external object to serialize
    public void consumeMessages()
    {
        //Do all the other stuff

        _serializer.serializeMessageObjects()
    }
}

//THE SERIALIZER CLASS
Public class MessageObjectSerializer : IMessageObjectSerializer 
{
    public List<MessageObject> serializeMessageObjects()
    {
        //DO THE SERILIZATION LOGIC HERE 
    }
}


//THE INTERFACE FOR THE SERIALIZER 
Public interface MessageObjectSerializer 
{
    List<MessageObject> serializeMessageObjects(); 
}

EDIT: Sorry, my example is in C#. I hope you can use it anyway :-)

Answer 3

Well, as you have noticed, it's very hard to unit test a concrete, high-level program. You have also identified the two most common issues:

Usually the program is configured to use specific resources, such as a specific file, IP address, hostname etc. To counter this, you need to refactor the program to use dependency injection. This is usually done by adding parameters to the constructor that replace the ahrdcoded values.

It's also very hard to test large classes and methods. This is usually due to the combinatorical explosion in the number of tests required to test a complex piece of logic. To counter this, you will usually refactor first to get lots more (but shorter) methods, then trying to make the code more generic and testable by extracting several classes from your original class that each have a single entry method (public) and several utility methods (private). This is essentially the single responsibility principle.

Now you can start working your way "up" by testing the new classes. This will be a lot easier, as the combinatoricals are much easier to handle at this point.

At some point along the way you will probably find that you can simplify your code greatly by using these design patterns: Command, Composite, Adaptor, Factory, Builder and Facade. These are the most common patterns that cut down on clutter.

Some parts of the old program will probably be largely untestable, either because they are just too crufty, or because it's not worth the trouble. Here you can settle for a simple test that just checks that the output from known input has not changed. Essentially a regression test.