Once you have been on a team long enough, your bugs eventually catch up to you. As an intern, I wrote a new microservice that needed to integrate with an internal API to look up information.

Now, after I returned as a full-time engineer, a mistake I made as an intern has shown up. Not only did I fix the bug, but I learned something and became a better developer in the process! 

Some well-tested code

Here’s a simplified example of what my code did.

** 
* Method that calls API to get ApiData for a particular input 
* 
* @param input Small input string. 
* @return ApiData, POJO representing data from API 
* @throws ApiException if might be retryable, including parsing errors, 503. Any cases that are not non-retryable. 
* @throws ApiNonRetryableException If unlikely to succeed on retry, such as no data found, 404, 400, or input is null/empty. 
*/ 
public ApiData getApiDataForInput(String input) throws ApiException, ApiNonRetryableException { 
  try { 
    if (isNullOrEmpty(input)) { 
      throw new ApiNonRetryableException(INPUT_NULL_EMPTY_MESSAGE); 
    }   

    String url = ApiConfiguration.getApiDataFromInputUrl() + input; 
    RestTemplate apiRestTemplate = getRestTemplate();  

    ResponseEntity response = 
        apiRestTemplate.exchange(url, HttpMethod.GET, new HttpEntity<>(), String.class); 
    List dataList = HttpStatus.OK.equals(response.getStatusCode()) 
      ? parseResponse(response.getBody()) 
      : null; 
    // Non Retryable if list empty, 404 or 400. Unlikely these will ever work. 
    if (isNonRetryableError(response, dataList)) { 
      throw new ApiNonRetryableException(NON_RETRYABLE_MESSAGE + input); 
    } 
    // Retryable if had trouble parsing response (list null), or something like a 503. 
    if (isRetryableError(response, dataList)){ 
      throw new ApiException(RETRYABLE_MESSAGE + input); 
    } 
    return dataList.get(0); 
  } catch (Exception e) { 
    // General case is retryable 
    throw new ApiException(e.getMessage()); 
  } 
} 

I wrote unit tests such as the following to ensure that my error handling worked as expected.

@Test 
public void getApiDataFromInputNotFound() throws Exception{ 
  try { 
    // Mock Rest Template to return 404 when I tell it to make the call to the API 
    doReturn(createApiResponse(NOT_FOUND, TEST_DATA)) 
      .when(mockRestTemplate) 
      .exchange(anyString(),eq(HttpMethod.GET),any(HttpEntity.class), eq(String.class)); 

    apiClient.getApiDataForInput(TEST_INPUT); 
    fail(EXPECTED_EXCEPTION); 
  } catch (ApiNonRetryableException e) { 
    // Ensure that we made the call 
    verify(mockRestTemplate, times(1)).exchange(anyString(),eq(HttpMethod.GET),any(HttpEntity.class), eq(String.class)); 
    assertTrue(e.getMessage().contains(NON_RETRYABLE_MESSAGE + TEST_INPUT)); 
  } 
} 

I used Mockito to make a mock of the Spring Rest Template we used. This allows me to define the behavior when a certain method is called by the code I am unit testing.

In this case, I told it to return a 404 when exchange() was called. I then verified that the proper exception was thrown by getApiDataForInput(), and its message was the expected message for a 404. 

The bug that slipped through 

I made similar unit tests for the other cases, retryable and non-retryable. This looked well tested, and passed code review, as it appeared to be robust code with good test coverage.

I wrote an integration test with a simulator simulating the happy path from the API to ensure that the entire system worked, but did not write integration tests for the failure cases, as I had already made unit tests for those.

Also, our regression tests checked that the expected end behavior was given for a particular response from the API, but did not test if it was retried or not under the various outcomes. All of these small changes let one big oversight slip past: 

RestTemplate throws an exception for most non 2xx responses. 

That means that all of my code checking for response statuses for retryable/non-retryable was never run.

Everything that was not 2xx threw an exception at exchange() and went straight to the catch-all that threw a retryable exception from whatever exception was caught.

My metrics never got incremented if there were issues. There were no helpful log messages. Bad requests with no hope of being fixed would be retried. All because I made the assumption that RestTemplate would provide a response with the error http status rather than throw an exception.

Because I was mocking RestTemplate, my tests relied on my assumption of how it behaved, rather than the actual behavior of RestTemplate. 

The implicit assumption 

Lisa, an experienced senior dev on our team decided to also integrate with this API. She examined my integration and quickly noticed this issue, having been caught by it before herself. Lisa wrote unit tests to demonstrate the problem and created a story for me to fix the issue. 

All I needed to change was my code to handle errors by catching the exceptions from RestTemplate rather than branching on the response.

I also modified unit tests so I was correctly mocking the exceptions thrown by RestTemplate. This was an easy fix for a stupid bug. But stupid bugs are the types of things we normally write tests for. Why were my tests inadequate? 

The big issue was my assumptions about the behavior of components I mocked in my unit tests. I was new to this library, being an intern with no previous Spring experience. I made the assumption that made sense to me. I checked off those cases as being tested by my unit test, so I never bothered to set up integration tests with a simulated endpoint for them. I never checked to see if the behavior I mocked was accurate. 

The lessons I learned 

How could have this been avoided? The most obvious answer would be to write integration tests for all of the failure cases. This would have caught the errors and would have been very thorough in testing the external behavior of the code.

But not all behavior can be shown externally, and integration tests are cumbersome to write, due to the need to set up an external endpoint for every test, and the need to actually start up the app to run them.

Unit tests are also nice because it is trivial to debug your code as the test runs, without the need to attach the debugger to your JVM instance running the application.

While there are proponents of doing most testing end-to-end, I am of the opinion that unit tests are much more manageable, easier to write, and do a good job in most cases. Additionally, QA teams do a lot of end-to-end testing and should catch most bugs that are externally apparent. Unit testing exposes bugs that may be hard to reproduce or observe externally. 

I think the main issue is that I was making an assumption about something I was new to. I think a good rule of thumb would be to pay special attention to assumptions you make when unit testing, and to double-check those assumptions if you are not sure.

This is especially true when creating mocks, and double-especially true if you are using something for the first time. I was mocking RestTemplate, which I was new to. I should have looked up the documentation to ensure that I understood all the possible outcomes of the call to exchange().

This also rings true for mocking database access, or anything else that hides the true behavior of external dependencies. Relying on yourself to notice assumptions is a little risky. It can be hard to tell what is an assumption, but it is something you can develop with experience and discipline, and it can be an effective tool in ensuring good unit tests.

Also, if you are doing a code review, maybe take a closer look at unit tests and see if there are some assumptions you think are incorrect.