Refactoring Code for Testability: An Example

Working on a legacy project those last weeks gave me plenty of material to write about tests, Mockito and PowerMock. Last week, I wrote about abusing PowerMock. However, this doesn’t mean that you should never use PowerMock; only that if its usage is commonplace, it’s a code smell. In this article, I’d like to show an example how one can refactor legacy code to a more testable design with the temporary help of PowerMock.

Let’s check how we can do that using the following code as an example:

public class CustomersReader {

    public JSONObject read() throws IOException {
        String url = Configuration.getCustomersUrl();
        CloseableHttpClient client = HttpClients.createDefault();
        HttpGet get = new HttpGet(url);
        try (CloseableHttpResponse response = client.execute(get)) {
            HttpEntity entity = response.getEntity();
            String result = EntityUtils.toString(entity);
            return new JSONObject(result);
        }
    }
}

Note that the Configuration class is outside our reach, in a third-party library. Also, for brevity’s sake, I cared only about the happy path; real-world code would probably be much more complex with failure handling.

Obviously, this code reads an HTTP URL from this configuration, browse the URL and return its output wrapped into a JSONObject. The problem with that it’s that it’s pretty hard to test, so we’d better refactor it to a more testable design. However, refactoring is a huge risk, so we have to first create tests to ensure non-regression. Worse, unit tests do not help in this case, as refactoring will change classes and break existing tests.

Before anything, we need tests to verify the existing behavior – whatever we can hack together, even if they don’t adhere to good practives. Two alternatives are possible:

• Fakes: set up an HTTP server to answer the HTTP client and a database/file for the configuration class to read (depending on the exact implementation)
• Mocks: Create mocks and stub their behavior as usual

Though PowerMock is dangerous, it’s less fragile and easy to set up than Fakes. So let’s start with PowerMock but only as a temporary measure. The goal is to refine both design and tests in parallel to that at the end, PowerMock will be removed. This test is a good start:

@RunWith(PowerMockRunner.class)
public class CustomersReaderTest {

    @Mock private CloseableHttpClient client;
    @Mock private CloseableHttpResponse response;
    @Mock private HttpEntity entity;

    private CustomersReader customersReader;

    @Before
    public void setUp() {
        customersReader = new CustomersReader();
    }

    @Test
    @PrepareForTest({Configuration.class, HttpClients.class})
    public void should_return_json() throws IOException {
        mockStatic(Configuration.class, HttpClients.class);
        when(Configuration.getCustomersUrl()).thenReturn("crap://test");
        when(HttpClients.createDefault()).thenReturn(client);
        when(client.execute(any(HttpUriRequest.class))).thenReturn(response);
        when(response.getEntity()).thenReturn(entity);
        InputStream stream = new ByteArrayInputStream("{ \"hello\" : \"world\" }".getBytes());
        when(entity.getContent()).thenReturn(stream);
        JSONObject json = customersReader.read();
        assertThat(json.has("hello"));
        assertThat(json.get("hello")).isEqualTo("world");
    }
}

At this point, the test harness is in place and the design can change bit by bit (to ensure non-regression).

The first problem is calling Configuration.getCustomersUrl(). Let’s introduce a service ConfigurationService class as a simple broker between the CustomersReader class and the Configuration class.

public class ConfigurationService {
    public String getCustomersUrl() {
        return Configuration.getCustomersUrl();
    }
}

Now, let’s inject this service into our main class:

public class CustomersReader {

    private final ConfigurationService configurationService;

    public CustomersReader(ConfigurationService configurationService) {
        this.configurationService = configurationService;
    }

    public JSONObject read() throws IOException {
        String url = configurationService.getCustomersUrl();
        // Rest of code unchanged
    }
}

Finally, let’s change the test accordingly:

@RunWith(PowerMockRunner.class)
public class CustomersReaderTest {

    @Mock private ConfigurationService configurationService;
    @Mock private CloseableHttpClient client;
    @Mock private CloseableHttpResponse response;
    @Mock private HttpEntity entity;

    private CustomersReader customersReader;

    @Before
    public void setUp() {
        customersReader = new CustomersReader(configurationService);
    }

    @Test
    @PrepareForTest(HttpClients.class)
    public void should_return_json() throws IOException {
        when(configurationService.getCustomersUrl()).thenReturn("crap://test");
        // Rest of code unchanged
    }
}

The next step is to cut the dependency to the static method call to HttpClients.createDefault(). In order to do that, let’s delegate this call to another class and inject the instance into ours.

public class CustomersReader {

    private final ConfigurationService configurationService;
    private final CloseableHttpClient client;

    public CustomersReader(ConfigurationService configurationService, CloseableHttpClient client) {
        this.configurationService = configurationService;
        this.client = client;
    }

    public JSONObject read() throws IOException {
        String url = configurationService.getCustomersUrl();
        HttpGet get = new HttpGet(url);
        try (CloseableHttpResponse response = client.execute(get)) {
            HttpEntity entity = response.getEntity();
            String result = EntityUtils.toString(entity);
            return new JSONObject(result);
        }
    }
}

The final step is to remove PowerMock altogether. Easy as pie:

@RunWith(MockitoJUnitRunner.class)
public class CustomersReaderTest {

    @Mock private ConfigurationService configurationService;
    @Mock private CloseableHttpClient client;
    @Mock private CloseableHttpResponse response;
    @Mock private HttpEntity entity;

    private CustomersReader customersReader;

    @Before
    public void setUp() {
        customersReader = new CustomersReader(configurationService, client);
    }

    @Test
    public void should_return_json() throws IOException {
        when(configurationService.getCustomersUrl()).thenReturn("crap://test");
        when(client.execute(any(HttpUriRequest.class))).thenReturn(response);
        when(response.getEntity()).thenReturn(entity);
        InputStream stream = new ByteArrayInputStream("{ \"hello\" : \"world\" }".getBytes());
        when(entity.getContent()).thenReturn(stream);
        JSONObject json = customersReader.read();
        assertThat(json.has("hello"));
        assertThat(json.get("hello")).isEqualTo("world");
    }
}

No trace of PowerMock whatsoever, neither in mocking static methods nor in the runner. We achieved a 100% testing-friendly design, according to our initial goal. Of course, this is a very simple example, real-life code is much more intricate. However, by changing code little bit by little bit with the help of PowerMock, it’s possible to achieve a clean design in the end.

The complete source code for this article is available on Github.