Unit testing retrieval methods - redundant?

Question

I have the following method in my service layer

public ModuleResponse GetModules(ModuleRequest request)
{
    var response = new ModuleResponse(request.RequestId);
    try
    {
        response.Modules = Mapper.ToDataTransferObjects(ModuleDao.GetModules());
        return response;
    }
    catch (Exception ex)
    {
        Log.Error(ex);
        response.Acknowledge = AcknowledgeType.Failure;
        response.Message = "An error occured.";
        return response;
    }
}

And I have a unit test written in xUnit like this:

[Fact]
public void GetModulesTest()
{
    //Arrange            
    var mockModuleDao = Mock.Create<IModuleDao>();
    var mockLog = Mock.Create<ILog>();
    var mockAuditDao = Mock.Create<IAuditDao>();

    var moduleList = new List<ModuleItem>
    {
        new ModuleItem {Id = 100, Category = "User Accounts", Feature = "Users"},
        new ModuleItem {Id = 101, Category = "User Accounts", Feature = "Roles Permissions"}
    };

    mockModuleDao.Arrange(dao => dao.GetModules()).Returns(moduleList);

    IUserManagementService userService = new UserManagementService(mockModuleDao, mockLog, mockAuditDao);

    var request = new ModuleRequest().Prepare();

    //Act
    var actualResponse = userService.GetModules(request);

    //Assert
    Assert.Equal(AcknowledgeType.Success, actualResponse.Acknowledge);
    Assert.Equal(2, actualResponse.Modules.Count);
}

Now I have a whole other bunch of retrieval methods in my code similar to the one above.

Are testing such methods redundant? I mean, they are almost a sure pass test, unless I mess up the logic of my Mapping or something.

Also, when testing retrieval methods, what is it that I should be testing for? In my scenario above, I have 2 assert statements, 1 to check if the response is a success, and the 2nd is to check the count of the list.

Is this sufficient? or how can this be further improved to enhance the value of such a unit test?

Answer 1

As always, whether or not a test like that is valuable depends on your motivation for testing.

• Is this piece of code mission-critical?
• What is the cost if that code fails?
• How easily can you address errors, should they occur?

The higher the cost of failure, the more important it is to test a piece of code.

The GetModules method does at least four things:

• It returns the modules from the DAO.
• It maps the modules from the DAO into the desired return types.
• It returns an error message if something goes wrong.
• It logs any errors that may occur.

The GetModulesTest tests a single of these four responsibilities, which means that three other tests are still required to fully cover the GetModules method.

Writing small-grained unit tests are valuable, because it enables you do decompose a complex piece of production code into a set of simple, easy-to-understand unit tests. Sometimes, these unit tests become almost inanely simple, to the point where you'll begin to doubt the value of it, but the value isn't in a single unit test - it's in the accumulation of simple tests, which, together, specify how the entire system ought to work.

Answer 2

Now I have a whole other bunch of retrieval methods in my code similar to the one above.

Really? Don't they feel a little... repetitive?

I think Lilshieste made a very appropriate point, that one intrinsic value of unit tests is that they highlight maintainability issues like this. You might say they make code smells more pungent.

Mark Seemann identified four individual responsibilities for this one method you showed us. The Single Responsibility Principle would dictate that you should only have one.

You could conceivably turn this method (and all its kin) into something more like this:

public ModuleResponse GetModules(ModuleRequest request)
{
    return _responder.CreateMappedDtoResponse(
        request,
        ModuleDao.GetModules,
        modules => new ModuleResponse {Modules = modules}));
}

Now, at this point, I think you could make a decent argument against unit-testing this method. You'd pretty much be testing the implementation of this method, rather than its behavior. Your unit test would be testing that you call a given method with given arguments, and that's it!

But even if you decided to be a purist and unit test this, there's really only one unit test that you could conceivably write, as opposed to the four that you would have needed to fully cover this method before. Then you write the appropriate unit tests for the CreateMappedDtoResponse methods (and whatever methods it may delegate parts of its work to), and you've got a DRY, well-tested system with a fraction of the number of tests. And if you change a common responsibility like your exception-logging strategy, you can change it in one place, change one unit test, and be done.

So even if your unit tests never catch a bug for you, being a purist helped you to avoid a maintainability issue that would have forced you to write just as much extra code in the first place, and be likely to re-write just as much code later on. Of course, this only happens if you know to listen to your unit tests and change your design accordingly.