2

I'm new to JUnit/Mockito and to unit testing in general. I'm asking this question in order to get feedback and learn best practices/patterns/strategies.

I wrote a class but when came time to unit test I ran into issues because the class has a dependency on an external AWS DynamoDB. I re-wrote the class and implemented a sort of dependency injection and would like advice on my approach and also on the unit test.

This was was my original implementation:

     public InternalDataStorageClient() {
         tableName = System.getenv("TableName");
         DynamoDBMapperConfig mapperConfig = new DynamoDBMapperConfig.Builder().withTableNameOverride(DynamoDBMapperConfig.TableNameOverride.withTableNameReplacement(tableName))
                 .build();
         ddbMapper = new DynamoDBMapper(client, mapperConfig);
         itemBatch = new ArrayList<>();
     }

I used the above class in other parts of my code like this which feels nice and tidy:

public InternalPublisher() {
    internalDataStorageClient = new InternalDataStorageClient();
}

When came time to write the test I couldn't because creating an instance of InternalDataStorageClient would throw an exception since you can't create a DynamoDBMapper without a connection to AWS.

I rewrote the class as below. I feel that the class implementation is okay but it feels like a hack, especially feel that the static method createDynamoDBMapper() as a work-around to allow for unit testing.

My questions are:

  1. Is the use of the static method createDynamoDBMapper dependency injection?
  2. How can I improve my class implementation, what are some of the standard patterns/strategies I should be using?
  3. Is the problem my unit test and not the class implementation?
  4. In the Update below I write how this refactoring now breaks another unit test which I think is proof that there is an issue with my design
public class InternalDataStorageClient {
    private static final AmazonDynamoDB DYNAMO_DB = AmazonDynamoDBClientBuilder.standard().build();

    private final DynamoDBMapper ddbMapper;
    private List<DBItem> itemBatch;

    public InternalDataStorageClient(DynamoDBMapper mapper) {
        ddbMapper = mapper;
    }

    public static DynamoDBMapper createDynamoDBMapper(String tableName) {
        DynamoDBMapperConfig mapperConfig = new DynamoDBMapperConfig.Builder()
                .withTableNameOverride(DynamoDBMapperConfig.TableNameOverride.withTableNameReplacement(tableName))
                .build();
        return new DynamoDBMapper(DYNAMO_DB, mapperConfig);
    }

    public void storeResults(List<Event> successes, List<Event> failures) {
        itemBatch = new ArrayList<>();

        for (Event attempt : successes) {
            itemBatch.add(createDBItem(attempt, true));
        }
        for (Event attempt : failures) {
            itemBatch.add(createDBItem(attempt, false));
        }

        sendBatchRequestResult();
    }

    public List<DBItem> getItemBatch() {
        return itemBatch;
    }

    public static DBItem createDBItem(Event attempt, Boolean success){
        DBItem item = new DBItem();
        item.setEmployeeID(attempt.getId());
        item.setSucceeded(success);
        if (!success) {
            item.setExceptionReceived(attempt.getExceptionReceived());
        }
        return item;
    }

    private void sendBatchRequestResult() throws AmazonDynamoDBException {
        List<DynamoDBMapper.FailedBatch> failedBatches = ddbMapper.batchSave(itemBatch);
        if (!failedBatches.isEmpty()) {
            for (DynamoDBMapper.FailedBatch failedBatch : failedBatches) {
                log.error("Failed batch due to: ", failedBatch.getException());
            }
            throw new AmazonDynamoDBException("FAILED TO ADD ITEMS TO DB");
        }
    }
}

Now my other classes use the above class like this, which doesn't feel quite as clean as before:

public InternalPublisher() {
    String databaseTableName = System.getenv("TableName");
    DynamoDBMapper mapper = InternalDataStorageClient.createDynamoDBMapper(databaseTableName);
    internalDataStorageClient = new InternalDataStorageClient(mapper);
}

Here is the JUnit test:

public class InternalDataStorageClientTest {
    private InternalDataStorageClient internalDataStorageClient;
    private DynamoDBMapper dynamoDBMapperMock;

    @Before
    public void setup() {
        dynamoDBMapperMock = Mockito.mock(DynamoDBMapper.class);
        when(dynamoDBMapperMock.batchSave()).thenReturn(new ArrayList<>());
        internalDataStorageClient = new InternalDataStorageClient(dynamoDBMapperMock);
    }

    @Test
    public void WHEN_SingleSuccessRequestResult_THEN_CorrectItemBatch() throws Exception {
        Request correctSingleRequest = SQSTestEvents.createCorrectSingleTrainingRequest();
        internalDataStorageClient.storeResults(correctSingleRequest.getEventList(), Collections.emptyList());

        DBItem item = internalDataStorageClient.createDBItem(correctSingleRequest.getEventList().get(0), true);
        assertThat(internalDataStorageClient.getItemBatch(), containsInAnyOrder(item));

        verify(dynamoDBMapperMock, times(1)).batchSave(internalDataStorageClient.getItemBatch());
    }
}

Update:

Though the solution above of using a static method allowed me to write my unit test, it broke the unit tests for my InternalPublisher class! The InternalPublisher unit tests create a InternalPublisher object and the construction fails because it is trying to use the external DB dependency.

public class InternalPublisherClientTest {

    private InternalPublisher internalPublisher;
    private ExternalPublisherServiceClient externalPublisher;
    private InternalDataStorageClient internalDataStorageClient;

    @Before
    public void setup() {
        internalPublisher = new InternalPublisher();
        externalPublisher = Mockito.mock(ExternalPublisherServiceClient.class, RETURNS_DEEP_STUBS);
        internalDataStorageClient = Mockito.mock(InternalDataStorageClient.class, RETURNS_DEEP_STUBS);
    }

    //Test that we do the right thing on external failure
    @Test
    public void WHEN_InvalidEvent_THEN_Failed_List_With_ID() {
        Request request = createCorrectSingleTrainingRequest();
        List<Event> failures = new ArrayList<>(request.getEventList());

        //Make external dependency call fail
        when(externalPublisher.newPublishServiceNotificationCall()).thenThrow(new PublisherException());

        //Confirm our internal client has collected all the failures
        internalPublisher.publishToPTNS(Collections.singletonList(request), externalPublisher);
        assertThat(internalPublisher.getFailedRequests(), is(failures));
    }
}
  • 1
    @DocBrown thanks for the comment. I've re-written the question. – totsubo Jul 30 at 14:37
  • I would note that while probably necessary, changing something to test is somewhat paradoxical as you are not testing what is actually implemented but a changed version. Hopefully, someone (say working on a Microsoft language-compiler or something) will think of a syntax way out of this, I am not saying there is a practical alternative, but there you have it! – JosephDoggie Jul 30 at 20:31
1

Are there other way to solve the external dependency issue?

DynamoDB is tightly coupled to Amazon. Amazon built it. Unless you can identify a higher level of abstraction, just leave your class as it is. Identifying a higher level of abstraction involves analyzing all of the consumers of InternalDataStorageClient and possibly eliminating all references to DynamoDB classes from the public interface.

Otherwise it looks like your current implementation clearly allows for dependency injection. You've written some unit tests, and those tests cover the logic in the InternalDataStorageClient class. Nothing wrong here.

I especially feel that the static method createDynamoDBMapper() is probably not the best way to achieve dependency injection.

The createDynamoDBMapper() method is not dependency injection. It is a factory method that returns an abstract type. Dependency injection is achieved with the InternalDataStorageClient constructor that accepts a DynamoDBMapper object as a constructor argument, because DynamoDBMapper is an interface, not a concrete type. The true test for an abstraction is how much code you must change if the abstraction must evolve, or your data storage technology changes.

I wouldn't be worried about this class, or the unit test. Analyze the consumers of InternalDataStorageClient. That's where problems will arise if you switch data storage vendors, or if the Amazon DynamoDB API changes. I would work more on isolating consumers of your class from the underlying data storage technology. Your time is better spent there.

| improve this answer | |
  • Thanks for the comment. Unfortunately I found an issue with my implementation; it breaks other test cases which make me even more suspicious that there is an issue with my class design. I've updated my question to reflect this. – totsubo Jul 30 at 19:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.