Backend Story

You Might Be Writing Your Test Wrong

Ugurcan Lacin -

Writing test cases might seem like an extra work for some developers. However, it is a crucial part of the development process. It helps you to ensure that your code works as expected and you don't break existing functionality when you make changes. In other words, it is a way to ensure that your task is done properly.

Developers often skip writing tests or write incomplete tests that miss edge cases. Sometimes, tests fail to verify the intended functionality. While this may sound strange, I will demonstrate common test writing mistakes and their solutions.

The following test case appears correct at first glance. Even static code analysis tools would not complain about it, because it achieves expected code coverage. However, closer inspection reveals several issues.

  @Test
void createShouldSucceed() {
   Customer customer = Mockito.mock(Customer.class);
   Mockito.when(customerService.getCustomer(Mockito.any())).thenReturn(customer);
   
   Customer merchantAccount =
       Customer.builder()
           .customerId(customerId)
           .externalId("abc")
           .countryCodes(List.of("SE"))
           .build();
   Mockito.when(customerService.getCustomerById(Mockito.anyString())).thenReturn(merchantAccount);

   ShippingProviderDto responseBody = Mockito.mock(ShippingProviderDto.class);

   ShippingProviderPayload payload = Mockito.mock(ShippingProviderPayload.class);
   Mockito.when(orderCreateMapper.to(Mockito.any(OrderCreatePayload.class)))
       .thenReturn(payload);
   Mockito.when(shippingProviderClient.createShippingOrder(Mockito.any()))
       .thenReturn(SuccessResponse.ok(responseBody, null));
   Mockito.when(shippingOrderResponseMapper.to(Mockito.any(ShippingProviderDto.class)))
       .thenReturn(OrderDto.builder().build());

   OrderCreatePayload orderCreatePayload = Mockito.mock(OrderCreatePayload.class);
   Mockito.when(orderCreatePayload.getCustomerId()).thenReturn(customerId);

   OrderDto result = shippingOrderService.create(orderCreatePayload);
   
   Mockito.verify(shippingProviderClient, Mockito.times(1)).createShippingOrder(Mockito.any());
   Mockito.verify(orderCreateMapper, Mockito.times(1)).to(Mockito.any());
   Mockito.verify(shippingOrderResponseMapper, Mockito.times(1)).to(Mockito.any());
   Assertions.assertNotNull(result);
   assertEquals(1, result.getOrderItems().size());
   assertTrue(result.getOrderItems().containsAll(List.of(1, 2, 3)))
}

At first glance, we can make a few assumptions. The test case is testing method of shippingOrderService.create(). So, you can assume that this is an e-commerce application test. Testing if shipping the order is successfully created.

However, there are some issues with the test case. Let's go through them one by one.

1. Test case naming

  @Test
void createShouldSucceed() {

The name createShouldSucceed fails to communicate the test's purpose and expected behavior. A good test name describes:

  • The scenario being tested
  • The expected outcome
  • The conditions under which the test runs

Here are better alternatives that follow common naming patterns:

given_valid_customer_details_when_creating_shipping_order_then_order_items_should_be_created()

should_create_order_items_when_customer_and_shipping_details_are_valid()

create_withValidCustomerAndShippingDetails_returnsOrderWithItems()

createsShippingOrderWithItemsForValidCustomer()

validCustomerShippingOrder_createsOrderWithExpectedItems()

Each of these names clearly communicates:

  • What functionality is being tested (shipping order creation)
  • The test conditions (valid customer and shipping details)
  • The expected outcome (order items are created)

2. Mocking object

Following code is calling method of customerService.getCustomer(). It is not clear that if the argument is an id or something else. Even though it is not related to purpose of the test, naming could be improved. If the argument is an id, then any() would be acceptable here. It is because any() is a method that accepts any argument, which means it is open to overuse. It is better to use it when you know that the argument is dynamic, like an id, or you don't care about the argument because you are testing something else.

In the following code, Mockito.when() seems okay if the purpose of the test is focusing on shipping order creation as I mentioned earlier. However, the return object is mocked. It is not clear what the return object is supposed to contain. There is no value is set to the return object. This makes it harder to understand what is expected.

    Customer customer = Mockito.mock(Customer.class);
   Mockito.when(customerService.getCustomer(Mockito.any())).thenReturn(customer);

3. Mocking mapper

The following code is mocking the mapper. Some might argue that it is okay to mock the mapper since the test is focusing on the service layer responsibility. However, as this mapper being used in the service layer, it is better to let the mapper being tested without mock. Mappers are simple mapping objects. They do not have any business logic. They are just mapping the input to the output, so it is better to test them without mock.

As a rule of thumb, you should mock the external dependencies(API), not the internal dependencies(mapper, service, etc.).

Mockito.when(orderCreateMapper.to(Mockito.any(OrderCreatePayload.class)))
       .thenReturn(payload);

4. Mocking response body

The following code mocks external API response. We understand this from the naming of the class shippingProviderClient, so mocking it is okay. However, the response body is mocked. This also makes it not clear. We do not know what is expected from the response body.

ShippingProviderDto responseBody = Mockito.mock(ShippingProviderDto.class);
Mockito.when(shippingProviderClient.createShippingOrder(Mockito.any()))
       .thenReturn(SuccessResponse.ok(responseBody, null));

5. Mocking payload

The following code is mocking the payload, and sending it to the service layer. Basically, this test method mocks the input and output objects at the end of the day. At this point, it becomes obvious that the test is not testing the actual business scenario at all. Changing internal logic of the service layer would not affect the test result.

OrderCreatePayload orderCreatePayload = Mockito.mock(OrderCreatePayload.class);
Mockito.when(orderCreatePayload.getCustomerId()).thenReturn(customerId);

OrderDto result = shippingOrderService.create(orderCreatePayload);

6. Verifying method calls

The following code is verifying the method calls. At the same time, there are mocks for these methods. So, if the method is executed up until this point, we already know that these methods are called. As a result, we don't actually verify anything here.

Mockito.verify(shippingProviderClient, Mockito.times(1)).createShippingOrder(Mockito.any());
Mockito.verify(orderCreateMapper, Mockito.times(1)).to(Mockito.any());
Mockito.verify(shippingOrderResponseMapper, Mockito.times(1)).to(Mockito.any());

7. Assertions

The following code is asserting the result of the method.

First assertion is not verifying any result, just that the result is not null. Second assertion would fail if the result is null anyway, so there is no additional value here.

Second assertion is actually verifying the expectedsize of the result, so it is okay.

Third assertion is verifying the result contains all the expected items, so it is okay too.

Assertions.assertNotNull(result);
assertEquals(1, result.getOrderItems().size());
assertTrue(result.getOrderItems().containsAll(List.of(1, 2, 3)))

We could improve the readability of the code by using assertThat instead of assertEquals and assertTrue.

    assertThat(result.getOrderItems())
        .hasSize(3)
        .containsExactlyInAnyOrderElementsOf(expectedOrderItems);

Final version of the test case:

@Test
void should_create_order_items_when_customer_and_shipping_details_are_valid() {
    // Given
    String customerId = "customer123";
    List<Integer> expectedOrderItems = List.of(1, 2, 3);
    
    // Setup customer
    Customer customer = Customer.builder()
        .customerId(customerId)
        .externalId("customer-ext-123")
        .countryCodes(List.of("SE"))
        .build();
    Mockito.when(customerService.getCustomerById(customerId)).thenReturn(customer);
    
    // Setup shipping order request
    OrderCreatePayload orderCreatePayload = OrderCreatePayload.builder()
        .customerId(customerId)
        .orderItems(expectedOrderItems)
        .build();
        
    // Setup shipping provider response
    ShippingProviderDto shippingProviderResponse = ShippingProviderDto.builder()
        .orderId("order123")
        .items(expectedOrderItems)
        .status("CREATED")
        .build();
    
    // Mock external service call
    Mockito.when(shippingProviderClient.createShippingOrder(Mockito.any()))
        .thenReturn(SuccessResponse.ok(shippingProviderResponse, null));
    
    // When
    OrderDto result = shippingOrderService.create(orderCreatePayload);
    
    // Then
    assertThat(result.getOrderItems())
        .hasSize(3)
        .containsExactlyInAnyOrderElementsOf(expectedOrderItems);
}

The test now focuses on the actual business scenario: creating a shipping order with items for a valid customer. It uses real objects where appropriate and only mocks the external service call (shippingProviderClient) which is a reasonable boundary for unit testing. Mocks can be easy escape for developers when they write tests. However, it is important to keep mocks for external dependencies like API calls.

The inital test case was basically mocking both input and output objects, so the test would never fail. The test was only making sure that the Java code was executed when it was called, which is not the concern at all.

In code reviews, it is common to spend more time on actualy implementation rather than how test cases are written. However, it is important to ensure that the test cases are also written correctly. I have seen many flaws in test cases over the years, which I also made mistakes myself. In this article, I tried to explain how to write and evaluate test cases.