Unit testing a servlet in a meaningful way

Question

I have to write a unit test for the method processRequest in the servlet below and I'm wondering if:

1. It just shouldn't be done.

2. The class should be rewritten / refactored to allow easier unit testing. Suggestions as to how?

3. There is a meaningful way of getting the value of response.getWriter().print(result), which is all the method returns.

Method in question:

protected void processRequest(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {

    response.setContentType("text/plain;charset=UTF-8");
    session = request.getSession(false);
    User sessionUser = (User) session.getAttribute("user");
    int userId = sessionUser.getId();

    if (sessionUser != null) {
        switch (request.getParameter("function")) {
            // UPLOAD IMAGE FUNCTION
            case "uploadimg":
                // save image
                final String path = getServletContext().getRealPath("") + "/resources/img/profile/tmp/";
                String imagePath = imageService.uploadImage(request, path, userId);

                // return image path for use in frontend java script
                response.getWriter().print(getServletContext().getContextPath() + "/resources/img/profile/tmp/" + imagePath);
                break;

            // SAVE IMAGE FUNCTION
            case "save":
                // move image from temp folder to permanent storage
                final String pathOrigin = getServletContext().getRealPath("") + "/resources/img/profile/tmp/";
                final String pathDestination = getServletContext().getRealPath("") + "/resources/img/profile/";
                boolean result = imageService.moveImage(pathOrigin, pathDestination, userId);

                // Returns true whether or not the image was saved successfully
                response.getWriter().print(result);
                break;
        }

        response.getWriter().flush();
        response.getWriter().close();
    }
}



@Override
protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
    processRequest(request, response);
}

Answer 1

Bug: You're verifying if sessionUser is different from null, but you're doing sessionUser.getId() before you're sure if it's null or not. If it was null, doing getId would have thrown a NullPointerException

int userId = sessionUser.getId();

if (sessionUser != null) {

Answer 2

In processRequest, calling flush and close is unnecesary as you don't own the writers.

You can test the response text with a mock. For example, with Mockito:

ByteArrayOutputStream output = new ByteArrayOutputStream();
PrintWriter printWriter = new PrintWriter(new OutputStreamWriter(output));

HttpServletResponse response = mock(HttpServletResponse.class);
when(response.getWriter()).thenReturn(printWriter);

// Call method under test

printWriter.flush();
assertEquals("result", new String(output.toByteArray(), "UTF-8"));

The class is difficult to test because it has dependencies like HttpServletRequest and ServletContext. These interfaces have many methods, some of which return objects like HttpSession which also require mocking. If you look through each line in the method, most of them have some interaction with an external dependency.

It's possible to mock all of these, but look at a method like this instead:

public class UploadController {
    String rootPath;
    ImageService imageService;

    public String upload(String function, InputStream file, Map<Object, Object> session) {
        User sessionUser = (User) session.get("user");
        int userId = sessionUser.getId();

        if (sessionUser != null) {
            switch (function) {
                case "uploadimg":
                    String path = rootPath + "/resources/img/profile/tmp/";
                    String imagePath = imageService.uploadImage(path, file, userId);
                    return rootPath + "/resources/img/profile/tmp" + imagePath;
                case "save":
                    String pathOrigin = rootPath + "/resources/img/profile/tmp/";
                    String pathDestination = rootPath + "/resources/img/profile/";
                    return imageService.moveImage(pathOrigin, pathDestination, userId);
            }
        }

        return "";
    }
}

Here, the dependencies are more constrained and involve simpler and easier to mock types. Many frameworks allow writing controllers in a style similar to this, maybe with the help of a few annotations. It looks like you're writing a webservice so something like JAX-RS could help.

Even within a Servlet, it's possible to do some preprocessing in doGet/doPost and then call a method like this which would be covered by a test.

Answer 3

I have to write a unit test for the method processRequest in the servlet below

If you gotta do you gotta do. No, seriously, that's an excellent idea to unit test this.

It just shouldn't be done.

I think there's a thing that "just shouldn't be unit tested". On the other hand, there are things that are extremely difficult to unit test without heavy refactoring.

The class should be rewritten / refactored to allow easier unit testing. Suggestions as to how?

You can:

1. Use a mocking framework like Mockito. Pass mock HttpServletRequest and HttpServletResponse objects into the method. Make response.getWriter() return a mock as well, so that you can verify the result written with the getWriter().print call. This is possible with Mockito. A tricky part I see is the imageService field which is defined outside your processRequest method. But the second point can help with that.

2. Extract the switch statement to another method, and test that method instead. Pass to the method as parameters everything it needs to handle the request: the function parameter, the context, and the image service. Instead of writing the result with getWriter().print, make the method return the result instead, or null. Check the returned result in processRequest, only if not null then write it with getWriter().print.