Photo by Alain Pham on Unsplash
During my time working on various open source projects as well as my regular job, I’ve been seeing more and more unit test setups that rely on methods that create the SUT (Subject Under Test) pre-configured for a specific set of unit test methods.
I believe this is an anti-pattern.
It encourages lazy copy and paste unit test development, makes refactoring harder and most likely will result in more code being written than the SUT setup method was written in order to avoid.
Using shared SUT creation can impact the clarity and maintainability of the unit tests as time goes on, and often obfuscates what test setup objects/data really need to be done in order to a get a unit test to pass.
The Problem with Shared SUT Setup
Let’s take a look at unit test “helper” method that creates a SUT.
private static ItineraryController GetItineraryControllerWithDetailsQuery(string userType, int organizationId)
{
var mediator = new Mock<IMediator>();
mediator.Setup(x => x.SendAsync(It.IsAny<ItineraryDetailQuery>()))
.ReturnsAsync(new ItineraryDetailsModel
{
Id = 1,
Name = "Itinerary",
EventId = 1,
EventName = "Event Name",
OrganizationId = 1,
Date = new DateTime(2016, 07, 01)
});
var sut = new ItineraryController(mediator.Object, MockSuccessValidation().Object);
sut.SetClaims(new List<Claim>
{
new Claim(UserType, userType),
new Claim(Organization, organizationId.ToString())
});
return sut;
}
This method is used to create the SUT (ItineraryController) for unit tests that need specific data on the ItineraryDetailsModel and a list of Claims created. The method also calls out to MockSuccessValidation().Object, which delegates the creation of a mock of IItineraryEditModelValidator to the MockSuccessValidation method:
private static Mock<IItineraryEditModelValidator> MockSuccessValidation()
{
var mockValidator = new Mock<IItineraryEditModelValidator>();
mockValidator.Setup(mock => mock.Validate(It.IsAny<ItineraryEditModel>(),
It.IsAny<EventSummaryModel>())).Returns(new List<KeyValuePair<string, string>>())
.Verifiable();
return mockValidator;
}
So far, so good. My guess is that someone, along the way, saw these two blocks of code that were the same for more than one unit test method, and thought encapsulating this setup in a some methods would be a better approach.
The SUT setup method, GetItineraryControllerWithDetailsQuery, is used across three test methods:
[Fact]
public async Task DetailsReturnsHttpUnauthorizedResultWhenUserIsNotOrgAdmin()
{
var sut = GetItineraryControllerWithDetailsQuery(UserType.OrgAdmin.ToString(),
It.IsAny<int>());
Assert.IsType<UnauthorizedResult>(await sut.Details(It.IsAny<int>()));
}
[Fact]
public async Task DetailsReturnsCorrectViewAndViewModelWhenEventIsNotNullAndUserIsOrgAdmin()
{
var sut = GetItineraryControllerWithDetailsQuery(UserType.SiteAdmin.ToString(), 0);
var result = await sut.Details(It.IsAny<int>()) as ViewResult;
Assert.Equal(result.ViewName, "Details");
var resultViewModel = result.ViewData.Model;
Assert.IsType<ItineraryDetailsModel>(resultViewModel);
}
[Fact]
public async Task DetailsReturnsCorrectViewModelWhenEventIsNotNullAndUserIsSiteAdmin()
{
var sut = GetItineraryControllerWithDetailsQuery(UserType.SiteAdmin.ToString(), 0);
Assert.IsType<ViewResult>(await sut.Details (It.IsAny<int>()));
}
At first glance, it looks like this method has really cleaned up the unit tests to make them more readable and has encapsulated the SUT setup. All three test methods are testing the .Details method on the SUT:
[HttpGet]
[Route("Admin/Itinerary/Details/{id}")]
public async Task<IActionResult> Details(int id)
{
var itinerary = await _mediator.SendAsync(new ItineraryDetailQuery
{
ItineraryId = id
});
if (itinerary == null)
{
return NotFound();
}
if (!User.IsOrganizationAdmin(itinerary.OrganizationId))
{
return Unauthorized();
}
return View("Details", itinerary);
}
Taking a look at what is being supplied to the ItineraryController’s constructor from the GetItineraryControllerWithDetailsQuery method:
var sut = new ItineraryController(mediator.Object, MockSuccessValidation().Object);
then taking a look at the IntineraryController’s constructor:
private readonly IMediator _mediator;
private readonly IItineraryEditModelValidator _itineraryValidator;
public ItineraryController(IMediator mediator, IItineraryEditModelValidator itineraryValidator)
{
_mediator = mediator;
_itineraryValidator = itineraryValidator;
}
and finally circling back to the code in the Details() action method, there are unnecessary mock and data setups being done across all unit tests.
Decomposing the SUT Setup
The first thing I see is an unnecessary creation of a mock of IItineraryEditModelValidator:
var sut = new ItineraryController(mediator.Object, MockSuccessValidation().Object);
It’s unnecessary because none of the dependency’s contract is being invoked by the Details action method.
At first glance, this might not seem like a “big deal”. But as more and more unit tests are written, and more and more mocks are created per unit test method as the project grows, the amount of time and memory wasted on creating these mock objects will get greater and greater, slowly down build times.
Diving even deeper, we can see that there is other unit test setup being done that is un-needed for the three test methods to test what they say they’re testing.
Example 1
The first unit test, “DetailsReturnsHttpUnauthorizedResultWhenUserIsNotOrgAdmin” uses the GetItineraryControllerWithDetailsQuery method, which sets Claims on the SUT it’s creating:
var sut = new ItineraryController(mediator.Object, MockSuccessValidation().Object);
sut.SetClaims(new List<Claim>
{
new Claim(UserType, userType),
new Claim(Organization, organizationId.ToString())
});
Stepping back, what is DetailsReturnsHttpUnauthorizedResultWhenUserIsNotOrgAdmin testing? That the Details action method returns an UnauthorizedResult when the user is not an org admin.
Yet this unit test is setting up its SUT with an Oganization claim. Why?
Don’t populate the claim at all and your test is finished. Here is the test setup divorced from the GetItineraryControllerWithDetailsQuery method:
[Fact]
public async Task DetailsReturnsHttpUnauthorizedResultWhenUserIsNotOrgAdmin()
{
var mediator = new Mock<IMediator>();
mediator.Setup(x => x.SendAsync(It.IsAny<ItineraryDetailQuery>()))
.ReturnsAsync(new ItineraryDetailsModel());
var sut = new ItineraryController(mediator.Object, null);
sut.SetClaims(new List<Claim> { new Claim(AllReady.Security.ClaimTypes.UserType,
UserType.BasicUser.ToString()) });
Assert.IsType<UnauthorizedResult>(await sut.Details(It.IsAny<int>()));
}
Some of you might be saying “well, that’s more code that was there before”. Although there is more code, look at what the code is doing.
I’m doing the minimal amount of test/data setup in order for the unit test to pass.
For the code above, it means returning an ItineraryDetailsModel that has no properties set (b/c I don’t need any set for the unit test) and only passing in the claims I need (I don’t need an Organization claim for this unit test to pass).
But what we’ve really gained here is an optimized setup based on the minimal amount of work that needs to be done in order to get the unit test written, and what comes out of this extra effort is clarity and maintainability for the project.
As a developer, I’m not making assumptions that all the properties need to be populated on ItineraryDetailsModel in order for this test to pass anymore which is an unintended side-effect of shared unit test setup.
Example 2
The mediator mock is being setup to return an ItineraryDetailsModel with many of its properties populated:
mediator.Setup(x => x.SendAsync(It.IsAny<ItineraryDetailQuery>())).ReturnsAsync(
new ItineraryDetailsModel
{
Id = 1,
Name = "Itinerary",
EventId = 1,
EventName = "Event Name",
OrganizationId = 1,
Date = new DateTime(2016, 07, 01)
});
yet when looking at the sut’s action method being tested, we only ever use one of the properties (OrganizationId) on ItineraryDetailsModel for making an execution path decision:
if (!User.IsOrganizationAdmin(itinerary.OrganizationId))
{
return Unauthorized();
}
so why are the other properties being populated at all? Not only that, but none of the existing three unit tests actually assert/test on any of those properties coming back from the action method (which they shouldn’t… those unit tests belong with the tests that test the ItineraryDetailQueryHandler):
[Fact]
public async Task DetailsReturnsHttpUnauthorizedResultWhenUserIsNotOrgAdmin()
{
…
Assert.IsType<UnauthorizedResult>(await sut.Details(It.IsAny<int>()));
}
[Fact]
public async Task DetailsReturnsCorrectViewAndViewModelWhenEventIsNotNullAndUserIsOrgAdmin()
{
…
Assert.Equal(result.ViewName, "Details");
var resultViewModel = result.ViewData.Model;
Assert.IsType<ItineraryDetailsModel>(resultViewModel);
}
[Fact]
public async Task DetailsReturnsCorrectViewModelWhenEventIsNotNullAndUserIsSiteAdmin()
{
…
Assert.IsType<ViewResult>(await sut.Details(It.IsAny<int>()));
}
many of you might be thinking, “what’s the big deal”?
The big deal is when the properties on ItineraryDetailsModel that are not used for these unit tests are updated/changed/deleted. Now you have to go though your unit test code and fix a bunch of compiler errors on properties of objects you set that you’re not even using for the given unit tests.
The other “big deal” is a developer is going to assume these properties are being set on ItineraryDetailsModel for a reason. But because the setup is shared, the developer is left guessing which of the many possible unit tests using this setup method are relying on property value they need in order for the test to pass. This type of setup leaves other developers in the dark as to whether or not they should use the sut setup, or write their own. And you can bet that no one is going to want to touch this code, which makes handling change via refactoring a very painful experience.
Again, do the minimum it takes for the test to pass.
Let’s fix it:
private static ItineraryController GetItineraryControllerWithDetailsQuery(string userType, int organizationId)
{
var mediator = new Mock<IMediator>();
mediator.Setup(x => x.SendAsync(It.IsAny<ItineraryDetailQuery>()))
.ReturnsAsync(new ItineraryDetailsModel { OrganizationId = 1});
…
}
All three unit tests still run successfully, and I’m only assigning the OrganiztaionId property on ItineraryDetailsModel .
Not All Reuse Is Bad
Instead of wrapping up the creation of the SUT in a method, break out any mocks or test data you need to setup into methods for reuse across unit tests. That way, your code can call the setup you need in the test method, but you’re still controlling the instantiation of the SUT. By taking control of the creation of the SUT, you’re explicit about its setup per test method.
For instance, if you need the same claims across multiple unit tests, then break out the creation of those claims to a shared method, but explicitly assign the claims to the sut in your unit test. Here’s a method for returning user type and organization claims:
public List<Claims> GetUserTypeAndOrganizationClaims(UserType userType, int organizationId)
{
return new List<Claim>
{
new Claim(AllReady.Security.ClaimTypes.UserType, userType),
new Claim(AllReady.Security.ClaimTypes.Organization, organizationId.ToString())
};
}
Then call the method from the unit test to populate the SUT’s Claims with these particular set of claims.
var sut = new ItineraryController(…);
sut.SetClaims(GetUserTypeAndOrganizationClaims());
Now any unit test that might happen to need these two claims created can use this method to create the claims, and then assign them to the SUT.
In Closing
There was a lot of code included in this blog post that I had to walk you through in order for you to gain understanding of why SUT setup methods are not a friend to your unit test code.
Call me picky, call me crazy, but optimizing unit test setup and assertion to use the minimal amount of setup/data/property population as possible is worth taking the extra minute or two in order to accomplish before submitting your next PR.
That being said, SUT creation in unit tests is not 100% bad. I will be examining using the Builder Pattern in an upcoming post to show you how you can accomplish shared SUT creation using a fluent style builder interface.