Jakub is a Java EE developer since 2005 and occasionally a project manager, working currently with Iterate AS. He's highly interested in developer productivity (and tools like Maven and AOP/AspectJ), web frameworks, Java portals, testing and performance and works a lot with IBM technologies. A native to Czech Republic, he lives now in Oslo, Norway. Jakub is a DZone MVB and is not an employee of DZone and has posted 155 posts at DZone. You can read more from them at their website. View Full User Profile

Help, My Code Isn't Testable! Do I Need to Fix the Design?

09.16.2012
| 3615 views |
  • submit to reddit

Our code is often untestable because there is no easy way to “sense1” the results in a good way and because the code depends on external data/functionality without making it possible to replace or modify these during a test (it’s missing a seam2, i.e. a place where the behavior of the code can be changed without modifying the code itself). In such cases the best thing to do is to fix the design to make the code testable instead of trying to write a brittle and slow integration test. Let’s see an example of such code and how to fix it.

Example Spaghetti Design

The following code is a REST-like service that fetches a list of files from the Amazon’s Simple Storage Service (S3) and displays them as a list of links to the contents of the files:

public class S3FilesResource {

    AmazonS3Client amazonS3Client;

    // ...

    @Path("/files")
    public String listS3Files() {
        StringBuilder html = new StringBuilder("<html><body>");
        List<S3ObjectSummary> files = this.amazonS3Client.listObjects("myBucket").getObjectSummaries();
        for (S3ObjectSummary file : files) {
            String filePath = file.getKey();
            if (!filePath.endsWith("/")) { // exclude directories
                html.append("<a href='/content?fileName=").append(filePath).append("'>").append(filePath)
                    .append("</a><br>");
            }
        }
        return html.append("</body></html>").toString();
    }

    @Path("/content")
    public String getContent(@QueryParam("fileName") String fileName) {
        throw new UnsupportedOperationException("Not implemented yet");
    }

}

Why is the code difficult to test?

  1. There is no seam that would enable us to bypass the external dependency on S3 and thus we cannot influence what data is passed to the method and cannot test it easily with different values. Moreover we depend on network connection and correct state in the S3 service to be able to run the code.
  2. It’s difficult to sense the result of the method because it mixes the data with their presentation. It would be much easier to have direct access to the data to verify that directories are excluded and that the expected file names are displayed. Moreover the core logic is much less likely to change than the HTML presentation but changing the presentation will break our tests even though the logic won’t change.

What can we do to improve it?

We first test the code as-is to be sure that our refactoring doesn’t break anything (the test will be brittle and ugly but it is just temporary), refactor it to break the external dependency and split the data and presentation, and finally re-write the tests.

We start by writing a simple test:

public class S3FilesResourceTest {

    @Test
    public void listFilesButNotDirectoriesAsHtml() throws Exception {
        S3FilesResource resource = new S3FilesResource(/* pass AWS credentials ... */);
        String html = resource.listS3Files();
        assertThat(html)
            .contains("<a href='/content?fileName=/dir/file1.txt'>/dir/file1.txt</a>")
            .contains("<a href='/content?fileName=/dir/another.txt'>/dir/another.txt</a>")
            .doesNotContain("/dir/</a>"); // directories should be excluded
        assertThat(html.split(quote("</a>"))).hasSize(2 + 1); // two links only
    }

}

Refactoring the Design

This is the refactored design, where I have decoupled the code from S3 by introducing a Facade/Adapter and split the data processing and rendering:

public interface S3Facade {
    List<S3File> listObjects(String bucketName);
}
public class S3FacadeImpl implements S3Facade {

    AmazonS3Client amazonS3Client;

    @Override
    public List<S3File> listObjects(String bucketName) {
        List<S3File> result = new ArrayList<S3File>();
        List<S3ObjectSummary> files = this.amazonS3Client.listObjects(bucketName).getObjectSummaries();
        for (S3ObjectSummary file : files) {
            result.add(new S3File(file.getKey(), file.getKey())); // later we can use st. else for the display name
        }
        return result;
    }

}

 

public class S3File {
    public final String displayName;
    public final String path;

    public S3File(String displayName, String path) {
        this.displayName = displayName;
        this.path = path;
    }
}

 

public class S3FilesResource {

    S3Facade amazonS3Client = new S3FacadeImpl();

    // ...

    @Path("/files")
    public String listS3Files() {
        StringBuilder html = new StringBuilder("<html><body>");
        List<S3File> files = fetchS3Files();
        for (S3File file : files) {
            html.append("<a href='/content?fileName=").append(file.path).append("'>").append(file.displayName)
                    .append("</a><br>");
        }
        return html.append("</body></html>").toString();
    }

    List<S3File> fetchS3Files() {
        List<S3File> files = this.amazonS3Client.listObjects("myBucket");
        List<S3File> result = new ArrayList<S3File>(files.size());
        for (S3File file : files) {
            if (!file.path.endsWith("/")) {
                result.add(file);
            }
        }
        return result;
    }

    @Path("/content")
    public String getContent(@QueryParam("fileName") String fileName) {
        throw new UnsupportedOperationException("Not implemented yet");
    }

}

In practice I’d consider using the built-in conversion capabilities of Jersey (with a custom MessageBodyWriter for HTML) and returning List<S3File> from listS3Files.

This is what the test looks like now:

public class S3FilesResourceTest {

    private static class FakeS3Facade implements S3Facade {
        List<S3File> fileList;

        public List<S3File> listObjects(String bucketName) {
            return fileList;
        }
    }

    private S3FilesResource resource;
    private FakeS3Facade fakeS3;

    @Before
    public void setUp() throws Exception {
        fakeS3 = new FakeS3Facade();
        resource = new S3FilesResource();
        resource.amazonS3Client = fakeS3;
    }

    @Test
    public void excludeDirectories() throws Exception {
        S3File s3File = new S3File("file", "/file.xx");
        fakeS3.fileList = asList(new S3File("dir", "/my/dir/"), s3File);
        assertThat(resource.fetchS3Files())
            .hasSize(1)
            .contains(s3File);
    }

    /** Simplest possible test of listS3Files */
    @Test
    public void renderToHtml() throws Exception {
        fakeS3.fileList = asList(new S3File("file", "/file.xx"));
        assertThat(resource.listS3Files())
            .contains("/file.xx");
    }
}

Next I’d implement an integration test for the REST service but still using the FakeS3Facade to verify that the service works and is reachable at the expected URL and that the link to a file content works as well. I would also write an integration test for the real S3 client (through S3FilesResource but without running it on a server) that would be executed only on-demand to verify that our S3 credentials are correct and that we can reach S3. (I don’t want to execute it regularly as depending on an external service is slow and brittle.)

Disclaimer: The service above isn’t a good example of proper REST usage and I have taken couple of shortucts that do not represent good code for the sake of brevity.

—-

1) Introduced by Michael Feathers in Working Effectively with Legacy Code, page 20-21
2) ibid, page 31

 

 

 

Published at DZone with permission of Jakub Holý, author and DZone MVB. (source)

(Note: Opinions expressed in this article and its replies are the opinions of their respective authors and not those of DZone, Inc.)