State and Interaction based testing using jMockIt

Question

I am new to jMockIt and so wanted to learn it. I thought of implementing a synonym servlet kata. The servlet takes a word as input and returns the synoyms of the words as a comma seperated string. The objective of the kata that I implemented was:

1. Learning jMockIt stubbing and mocking features.
2. Choosing between stubbing and mocking based on the situation.
3. Implementing readable, maintainable and trustworthy unit tests.

I request code reviewers to provide feedback in above mentioned areas. I am open to receive feedback about the servlet code as well. The code is below:

class SynonymServlet extends HttpServlet {
 public SynonymServlet(RecentCache cache, SynonymProvider provider) {
 this.cache = cache;
 this.provider = provider;
 }
 @Override
 protected void doGet(HttpServletRequest request, HttpServletResponse response)
 throws ServletException, IOException {
 //input processing
 String word = getInputWordFromURL(request.getRequestURI());
 //cache management
 List<String> synonyms = cache.get(word);
 if (synonyms == null) {
 synonyms = provider.getSynonyms(word);
 cache.put(word, synonyms);
 }
 //response creation
 String responseString = StringUtils.join(synonyms, ", ");
 response.getOutputStream().print(responseString);
 }
 private static String getInputWordFromURL(String URL) {
 String[] tokenizedURL = URL.split("/");
 String word = tokenizedURL[tokenizedURL.length - 1];
 return word;
 }
 private RecentCache cache;
 private SynonymProvider provider;
}
 interface RecentCache {
 List<String> get(String word);
 void put(String word, List<String> synonyms);
 }
 interface SynonymProvider {
 List<String> getSynonyms(String word);
 }
 public class SynonymServletTest {
 @Mocked 
 RecentCache cache;
 @Mocked
 SynonymProvider provider;
 @Test
 public void should_always_look_into_cache_first() throws ServletException, IOException {
 //Mock
 new Expectations() {
 {
 cache.get("reputation");
 times = 1;
 }
 };
 SynonymServlet servlet = new SynonymServlet(cache, provider);
 servlet.doGet(getRequest("/synonyms/reputation"), new MockHttpServletResponse());
 }
 @Test
 public void should_return_from_cache_when_found() throws Exception, IOException {
 //Stub
 new Expectations() {
 {
 cache.get("reputation");
 result = new ArrayList<String>() {{
 add("credit");
 add("reputation");
 }}; 
 }
 };
 MockHttpServletResponse response = new MockHttpServletResponse();
 SynonymServlet servlet = new SynonymServlet(cache, provider);
 servlet.doGet(getRequest("/synonyms/reputation"), response);
 assertThat(response.getContentAsString()).isEqualTo("credit, reputation");
 }
 @Test
 public void should_get_it_from_provider_when_not_found_in_cache() throws ServletException, IOException {
 //Stub
 new NonStrictExpectations() 
 {
 {
 cache.get("reputation");
 result = null;
 }
 };
 //Mock
 new Expectations() {
 {
 provider.getSynonyms("reputation");
 times = 1;
 }
 };
 SynonymServlet servlet = new SynonymServlet(cache, provider);
 servlet.doGet(getRequest("/synonyms/reputation"), new MockHttpServletResponse());
 }
 @Test
 public void should_return_the_same_as_provided_by_provider() throws ServletException, IOException {
 //Stub
 new NonStrictExpectations() {
 {
 cache.get("reputation");
 result = null;
 }
 };
 new Expectations() {
 {
 provider.getSynonyms("reputation");
 times = 1;
 result = new ArrayList<String>() {{
 add("credit");
 add("reputation");
 }}; 
 }
 };
 SynonymServlet servlet = new SynonymServlet(cache, provider);
 MockHttpServletResponse response = new MockHttpServletResponse();
 servlet.doGet(getRequest("/synonyms/reputation"), response);
 assertThat(response.getContentAsString()).isEqualTo("credit, reputation");
 }
 @Test
 public void should_update_cache_when_not_in_cache() throws ServletException, IOException {
 //Stub
 new Expectations() {
 {
 cache.get("reputation");
 result = null;
 }
 };
 //Stub
 new Expectations() {
 {
 provider.getSynonyms("reputation");
 result = new ArrayList<String>() {{
 add("credit");
 add("reputation");
 }}; 
 }
 };
 //Mock
 new Expectations() {
 {
 cache.put("reputation", (List<String>) any);
 times = 1;
 }
 };
 SynonymServlet servlet = new SynonymServlet(cache, provider);
 servlet.doGet(getRequest("/synonyms/reputation"), new MockHttpServletResponse());
 }
 private MockHttpServletRequest getRequest(String url) {
 return new MockHttpServletRequestBuilder()
 .withURL(url)
 .build();
 }
 }

• toto2: It's just a detail, but declaring the member variables at the end of the class is unusual and does not help for readability.

Answer 1

Your tests looks good although I don't have any experience with JMockit (just Mockito and EasyMock) so I might miss some JMockit-specific issues. Here is a few notes about the code:

1. Instead of

   String[] tokenizedURL = URL.split("/");
   String word = tokenizedURL[tokenizedURL.length - 1];
   

   You could use StringUtils.substringAfterLast. See also: Simplest way to get last word in a string

2. I would consider creating a new class for the cache management logic, so you could test it without any servlet logic. I mean something like this:

   public class CachingSynonymProvider implements SynonymProvider {
    private final SynonymProvider synonymProvider;
    private final RecentCache recentCache;
    public CachingSynonymProvider(final SynonymProvider synonymProvider, 
    final RecentCache recentCache) {
    this.synonymProvider = checkNotNull(synonymProvider, "synonymProdiver cannot be null");
    this.recentCache = checkNotNull(recentCache, "recentCache cannot be null");
    }
    @Override
    public List<String> getSynonyms(final String word) {
    ...
    }
   }
   

   It might make the RecentCache unnecessary or you just could refactor/move its content to the CachingSynonymProvider class.

3. In should_update_cache_when_not_in_cache() there is the following line:

   cache.put("reputation", (List<String>) any);
   

   I would be more strict here or verify it at the end of the test what's the value of the second parameter of put. If the code has a bug and always puts an empty list into the cache your second and subsequent get calls would give wrong results.

4. You could eliminate here a compiler warning:

   result = new ArrayList<String>() {{
    add("credit");
    add("reputation");
   }}; 
   

   and make it shorter with Guava's newArrayList:

   result = newArrayList("credit", "reputation");
   

5. I would consider extracting out common parts of unit tests to methods to remove duplication and express the purpose of the code. For example,

   //Stub
   new Expectations() {
    {
    cache.get("reputation");
    result = null;
    }
   };
   

   could be in a method called setEmptyCacheContentFor(String word) or something similar.

6. I usually declare my @Test methods with a generic throws Exception. JUnit will catch all of the exceptions and it makes life easier when a tested method gets a new checked exception: you don't have to modify the signature of all of your tests.

7. Fields (cache, provider) should be at the beginning of the class, then comes the constructor, then the other methods. (According to the Code Conventions for the Java Programming Language, 3.1.3 Class and Interface Declarations.)

8. Does it make sense to create a SynonymServlet with null RecentCache or SynonymProvider? Since it would throw an exception sooner or later I would check it and throw an IllegalArgumentException or NullPointerException from the constructor. Throwing an exception immediately helps debugging a lot since you get a stacktrace with the frames of the erroneous client, not just a NullPointerException later, maybe from another thread. (Effective Java, Second Edition, Item 38: Check parameters for validity; The Pragmatic Programmer: From Journeyman to Master by Andrew Hunt and David Thomas: Dead Programs Tell No Lies.)

9. The parameter name is uppercase here:

   private static String getInputWordFromURL(String URL) {
   

   while in the test is lowercase which is not consistent:

   private MockHttpServletRequest getRequest(String url) {
   

   I've found the lowercase one easier to read, I would use that.

   From Effective Java, 2nd edition, Item 56: Adhere to generally accepted naming conventions:

   While uppercase may be more common, a strong argument can made in favor of capitalizing only the first letter: even if multiple acronyms occur back-to-back, you can still tell where one word starts and the next word ends. Which class name would you rather see, HTTPURL or HttpUrl?

10. Instead of returning null from RecentCache.get I'd consider using Optional<List<String>>. It makes explicit that it could return absent/empty value, so it's harder to miss the null check. Java 8 introduced an Optional class but there is one in Guava too.

• Anand Patel: I am not able to understand the #2. What benefit we got by breaking the coupling between cache and servlet class? I am also not clear about the benefit of #10. Even if I use optional from Guava, the consumer of the cache still needs to make a check.
• palacsint: @AnandPatel: #2: Easier to understand/maintain and test in isolation (you need fewer mock objects), less complexity, higher abstraction level. #10: Yes, he or she still need to do a check but it's much harder to forget it. It makes explicit that get might return nothing (which is less error-prone). (Read the What's the point? part of the linked Guava documentation too.)