Sunday 17 July 2011

Testing legacy: Hard-wired dependencies (part 1)

When pairing with some developers, I've noticed that one of the reasons they are not unit testing existing code is because, quite often, they don't know how to overcome certain problems. The most common one is related to hard-wired dependencies - Singletons and static calls.

Let's look at this piece of code:

public List<Trip> getTripsByUser(User user) throws UserNotLoggedInException {
    List<Trip> tripList = new ArrayList<Trip>();
    User loggedUser = UserSession.getInstance().getLoggedUser();
    boolean isFriend = false;
    if (loggedUser != null) {
        for (User friend : user.getFriends()) {
            if (friend.equals(loggedUser)) {
                isFriend = true;
                break;
            }
        }
        if (isFriend) {
            tripList = TripDAO.findTripsByUser(user);
        }
        return tripList;
    } else {
        throw new UserNotLoggedInException();
    }
}

Horrendous, isn't it? The code above has loads of problems, but before we change it, we need to have it covered by tests.

There are two challenges when unit testing the method above. They are:

User loggedUser = UserSession.getInstance().getLoggedUser(); // Line 3  
   
tripList = TripDAO.findTripsByUser(user);                    // Line 13

As we know, unit tests should test just one class and not its dependencies. That means that we need to find a way to mock the Singleton and the static call. In general we do that injecting the dependencies, but we have a rule, remember?

We can't change any existing code if not covered by tests. The only exception is if we need to change the code to add unit tests, but in this case, just automated refactorings (via IDE) are allowed.

Besides that, many of the mocking frameworks are not be able to mock static methods anyway, so injecting the TripDAO would not solve the problem.

Overcoming the hard-dependencies problem

NOTE: In real life I would be writing tests first and making the change just when I needed but in order to keep the post short and focused I will not go step by step here .

First of all, let's isolate the Singleton dependency on it's own method. Let's make it protected as well. But wait, this need to be done via automated "extract method" refactoring. Select just the following piece of code on TripService.java:

UserSession.getInstance().getLoggedUser()

Go to your IDE's refactoring menu, choose extract method and give it a name. After this step, the code will look like that:

public class TripService {

    public List<Trip> getTripsByUser(User user) throws UserNotLoggedInException {
        ...
        User loggedUser = loggedUser();
        ...
    }

    protected User loggedUser() {
        return UserSession.getInstance().getLoggedUser();
    }
}

Doing the same thing for TripDAO.findTripsByUser(user), we will have:

public List<Trip> getTripsByUser(User user) throws UserNotLoggedInException {
    ...
    User loggedUser = loggedUser();
    ...
        if (isFriend) {
            tripList = findTripsByUser(user);
        }
    ...
}  
 
protected List<Trip> findTripsByUser(User user) {
    return TripDAO.findTripsByUser(user);
} 
 
protected User loggedUser() {
    return UserSession.getInstance().getLoggedUser();
}

In our test class, we can now extend the TripService class and override the protected methods we created, making them return whatever we need for our unit tests:

private TripService createTripService() {
    return new TripService() {
        @Override protected User loggedUser() {
            return loggedUser;
        }
        @Override protected List<Trip> findTripsByUser(User user) {
            return user.trips();
        }
    };
}

And this is it. Our TripService is now testable.

First we write all the tests we need to make sure the class/method is fully tested and all code branches are exercised. I use Eclipse's eclEmma plugin for that and I strongly recommend it. If you are not using Java and/or Eclipse, try to use a code coverage tool specific to your language/IDE while writing tests for existing code. It helps a lot.

So here is the my final test class:

public class TripServiceTest {
        
    private static final User UNUSED_USER = null;
    private static final User NON_LOGGED_USER = null;
    private User loggedUser = new User();
    private User targetUser = new User();
    private TripService tripService;

    @Before
    public void initialise() {
        tripService  = createTripService();
    } 
        
    @Test(expected=UserNotLoggedInException.class) public void 
    shouldThrowExceptionWhenUserIsNotLoggedIn() throws Exception {
        loggedUser = NON_LOGGED_USER;
                 
        tripService.getTripsByUser(UNUSED_USER);
    }
        
    @Test public void 
    shouldNotReturnTripsWhenLoggedUserIsNotAFriend() throws Exception {             
        List<Trip> trips = tripService.getTripsByUser(targetUser);
                 
        assertThat(trips.size(), is(equalTo(0)));
    }
        
    @Test public void 
    shouldReturnTripsWhenLoggedUserIsAFriend() throws Exception {
        User john = anUser().friendsWith(loggedUser)
                            .withTrips(new Trip(), new Trip())
                            .build();
                 
        List<Trip> trips = tripService.getTripsByUser(john);
                 
        assertThat(trips, is(equalTo(john.trips())));
    }

    private TripService createTripService() {
        return new TripService() {
            @Override protected User loggedUser() {
                return loggedUser;
            }
            @Override protected List<Trip> findTripsByUser(User user) {
                return user.trips();
            }
        };
    }        
}

Are we done?

Of course not. We still need to refactor the TripService class. Check the part two of this post.

If you want to give it a go, here is the full code: https://github.com/sandromancuso/testing_legacy_code

4 comments:

Anonymous said...

To get rid of Singleton dependency for unit testing, you can make the singleton implement an interface. Then the class can get the interface as a parameter (dependency) in the constructor. In the production code we will give the singleton and in tests code we can give a mocked implementation to the interface. This way you can freely test without almost any change in production existing code.

Sandro Mancuso said...

Absolutely. That would be the end goal and a good approach to get rid of singletons. I try to write tests for existing code without making any manual intervention in the existing code, hence why I prefer the approach described here. However, once the tests are in place, I would start injecting/removing dependencies and as a result, remove all the protected methods that I had to create in order to test the class. I described some of it in "part two" of this post.

Sandro Mancuso said...

Thanks Dubai.

miguelbaldi said...

Just a tip about the "JUnit". When you use the notation "@ ​​Test (expected = UserNotLoggedInException.class)", you should expect that the test does not pass, unless the code throws an exception. But using the attribute "expected", throwing the exception or not, the test will pass. To achieve the desired behavior, you can use a "Rule". Thus the test will only pass if the exception is thrown. Here is a gist (git://gist.github.com/3098239.git), illustrating the solution. Thanks for the great article!

Post a Comment