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) {

• DiePartei: You are right. Bugs are absolutely ok though as fail is an acceptable condition of a unit test.
• Marc-Andre: @DiePartei Sure checking for exceptions are correct unit tests. The problem is your using sessionUser before checking if it's null or not. You can remove the if and make a test to assure the NullPointerException or move sessionUser.getId() after the if.

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.