In this post we'll cover Audacia's best practices for writing clear and robust unit tests. What does it mean for a unit test to add value? The value can be gained from a number of different aspects.
- Peace of mind: A passing unit test alone shouldn't be enough to give you confidence, for all you know the test is running
true.Should().Be(true); tests must also fail when they should in order to give you this reassurance. - Time saved testing: Some code can be very hard to test manually, especially when hard-to-reach edge cases are considered. Having unit tests that pass without you doing this heavy-lifting can save you an immeasurable amount of time over the course of a project lifecycle.
- Documentation: This will be covered in more detail below, but unit tests serve a great purpose in documenting the intended behaviour of your application, and can help other developers less familiar with certain areas of the application learn the nuances of it.
- Code quality: Test driven development (TDD) is proven to improve code readability and structure, as this article explains in more detail. These days, I much prefer writing code from scratch with failing tests already pointed at it. That may be because the act of turning the red crosses into green ticks during development is extremely satisfying, but it also allows me to write my code with confidence that I've got my safety net already prepared.
The examples used in the points below use the XUnit test framework. If this library is unfamiliar to you, NUnit / MSTest will certainly have equivalents in their API.
Without further ado, let's begin with our unit testing dos and don'ts.
DO arrange tests into 'Arrange' 'Act' 'Assert' (MUST)
Otherwise known as the 'AAA' pattern, this has become commonplace with unit test development and helps people less familiar with unit testing read and write tests. It involves splitting the test into three sections:
- Arrange: Set up your data and parameters to allow the code under test to execute for your specific scenario.
- Act: Execute your code under tests with the arranged parameters.
- Assert: Verify the expected outcome by checking the behaviour described in the test name.
Whether you explicitly separate out the test method into comments with //Arrange, //Act, //Assert is optional. In my opinion, if your test is succinct enough, multiple line breaks can also split this method up visually:
[Fact]
public async Task Order_status_is_updated_to_accepted()
{
var orderToAdd = new Order { Status = OrderStatus.New };
await _context.Orders.AddAsync(orderToAdd);
await _context.SaveChangesAsync();
var orderId = orderToAdd.Id;
var request = new AcceptOrderRequest(orderId);
await _acceptOrderService.ExecuteAsync(request);
var updatedOrder = await _context.Orders.FindAsync(orderId);
updatedOrder.Status.Should().Be(OrderStatus.Accepted);
}
For this example, we arranged our test data by setting up a test order, executed our service to accept the order (act), and asserted that the order status was updated as per the name of the method.
Microsoft are also advocates of this structure, as outlined in their unit testing documentation.
DO make tests atomic (MUST)
Put simply, tests should be able to run on their own, or in any order. When you begin to think about what life would be like if this weren't the case, and the work required to maintain a unit test library like that, you can imagine how challenging that would be. Unit tests that don't run atomically can be very brittle, and can unexpectedly fail at inopportune times.
Atomic unit tests are usually hoped for, but sometimes the way they are written can cause them not to be. Avoid sharing state between tests, for example something globally inserting data for all tests, as this is usually the main cause of tests not being atomic. In this section we'll cover some common pitfalls.
Required data not set up as part of the test
The below test uses a hardcoded id rather than setting up the data in the arrange part of the test:
[Fact]
public async Task Order_status_is_updated_to_accepted()
{
var orderId = 5;
var request = new AcceptOrderRequest(orderId);
await _acceptOrderService.ExecuteAsync(request);
var updatedOrder = await _context.Orders.FindAsync(orderId);
updatedOrder.Status.Should().Be(OrderStatus.Accepted);
}
This is an example of a unit test that isn't guaranteed to be able to pass in isolation, as we've assumed the existence of an order with id 5. If this test doesn't insert an order into the database itself (or as part of test initialisation), it will fail. Conversely, if another test removes an order from the database which may or may not have an id of 5, it will also fail.
Other tests change data you're relying on
For this test, the key difference is in the act where we query based on properties not specific to the entity set up in the arrange.
[Fact]
public async Task Order_status_is_updated_to_accepted()
{
var orderToAdd = new Order { Status = OrderStatus.New };
await _context.Orders.AddAsync(orderToAdd);
await _context.SaveChangesAsync();
var orderId = orderToAdd.Id;
var request = new AcceptOrderRequest(orderId);
await _acceptOrderService.ExecuteAsync(request);
var updatedOrder = await _context.Orders.SingleOrDefaultAsync(ord => ord.Status == OrderStatus.Accepted);
updatedOrder.Status.Should().NotBeNull();
}
This is an example of a unit test that isn't guaranteed to be able to pass in any order, as we've assumed there will be only one order in the database with a status of accepted. If you're using shared database contexts and another test were written which inserts an accepted order into the database, this test would pass or fail as a direct result of the order in which tests are run. To fix this, retrieve the updatedOrder from the database directly using the id of the orderToAdd, as with the first example.
Workaround
If adopting a code-base that cannot run tests atomically, your best bet is to manually specify the order in which they should run. Most unit testing frameworks offer support for specifying this (e.g XUnit's ITestCaseOrderer). Bear in mind that this should be a means to an end, and atomic unit tests without overriding the order should be the goal.
DO use method names to document your application logic (MUST)
A lot is said about the role that unit tests play in documenting the behaviour of the codebase. For example if AddUserServiceTests has a test called Returns_error_if_email_address_already_exists, I know without looking at the actual service that the intended behaviour of the system is to enforce uniqueness on a user's email address.
Naming things appropriately is one of the hardest things to do for even the most senior developers and a common way in which people fall into this trap is when you write parameterized tests. Assume we're testing the below:
public static class OrderExtensions
{
public static IsOpen(this Order order) => order.Status <= OrderStatus.Processing;
}
The below unit test's name isn't descriptive, and doesn't tell us anything about the domain logic:
[Theory]
[InlineData(OrderStatus.New)]
[InlineData(OrderStatus.Accepted)]
[InlineData(OrderStatus.Processing)]
public void Order_is_open_works_correctly(OrderStatus status)
{
var order = new Order { Status = status };
var actual = order.IsOpen();
actual.Should.Be(true);
}
The method name tells us no more than we would know if the test were passing or failing. Test method names should be descriptive, so the word correctly is meaningless in this context. The below method is exactly the same, but its name includes intended application behaviour:
[Theory]
[InlineData(OrderStatus.New)]
[InlineData(OrderStatus.Accepted)]
[InlineData(OrderStatus.Processing)]
public void Order_is_open_is_true_based_on_status(OrderStatus status)
{
var order = new Order { Status = status };
var actual = order.IsOpen();
actual.Should.Be(true);
}
This method name improves our understanding because it tells us that an order is treated as open based on its status. We're missing an accompanying test here Is_open_is_false_based_on_status for the converse, but hopefully this illustrates the point enough.
As a final point, if you think your method name for a parameterized unit test doesn't give enough clarity or context, consider exploding your tests into a test-per-status, i.e:
Is_open_is_true_if_status_is_newIs_open_is_true_if_status_is_acceptedIs_open_is_true_if_status_is_processing
DO include unit test runs in build validation pipelines (MUST)
Build validation is used to prove that the software can build successfully without the intention of deploying anywhere just yet. It commonly happens at the point a pull request is made, to ensure that the developer hasn't added anything that would prevent a release of the software.
We can take this a step further by running unit tests in the same pipeline, to ensure developers haven't unintentionally changed any desired application behaviour. Running them in this way also gives the added benefit that any brittle unit tests will soon make themselves known, as you'll be running them far more frequently than you previously were.
The drawback here would be the additional time it adds to your review process, especially if running your unit tests takes an especially long amount of time to run. Treat these un-performant tests as genuine performance issues, and fix them just like you would a performance bug. As a last resort, consider running them on a schedule to mitigate this bottle-neck.
DO write tests before you write code (SHOULD)
To use an example; studies have proven that reading through an exam paper helps your brain proactively break down the task ahead, and subconsciously start thinking about solutions to problems you may face.
In my experience, the same is true of Test Driven Development (TDD). Writing how I want my new feature to behave before I write the code makes me consider edge-cases sooner, and helps me structure the code in a clearer way.
DO start to fix bugs by writing a failing test (MUST)
It's not just new features you should write unit tests for. Legacy code is arguably more important to cover by unit tests, as the documentation exercise will undoubtedly help other developers understand the intended purpose of the code.
Start fixing your bug by asking yourself "Can I write a failing unit test for this?". If the bug violates intended application behaviour, you can be fairly sure that the reproduction of it is not covered by a unit test, and it may have even uncovered a previously unthought of edge-case.
Writing a test with the intention of it failing will seem strange at first, but it's not dissimilar to TDD, in that you're writing a test outlining what the code should do, before you make it do that thing.
Once your unit test is written, the act of fixing the bug simply becomes the act of making this test pass.
DO still manually test your code (MUST)
Having unit tests written for a feature is a great thing to have, but does not ensure with 100% certainty that it is working as intended. What good is a passing unit test if the application falls over for some unrelated reason? Some things to consider that aren't guaranteed by a unit test are things like Performance, Security, UX.
DO mock external dependencies (SHOULD)
As you continue to add unit tests to your project, you'll eventually come across testing something with a dependency. This could be something injected into the constructor that you'll have to set up in order to construct the code under test.
Dependencies can come in all shapes and sizes, and can be categorized in two ways: internal, and external. Internal dependencies are something you have control of (i.e something in the codebase you can alter), like a database context in the same codebase. Conversely external dependencies would be something like a third-party API. The default approach would be not to mock where possible, as this stack overflow blog post explains in more detail. However your tests can fail unexpectedly if the external dependency were to change or suddenly become unavailable. Therefore, it's best to mock these so that your tests aren't at the mercy of something out of your control.
DON'T test randomised information (MUST)
To help illustrate the point, here's a unit test which may pass or fail as a direct result of a randomly generated number assigned to the repairCost variable:
[Fact]
public async Task User_is_quoted_excess_if_repair_cost_exceeds_excess()
{
var job = new Job
{
Id = 5
Excess = 500
};
var repairCost = new Random().Next();
var request = new GetJobRepairQuoteRequest
{
JobId = job.Id,
Cost = repairCost
};
var result = await _service.Execute(request);
result.Cost.Should().Be(job.Excess);
}
In this example, we haven't guaranteed that that cost of the repair will be greater than the excess stored on the job. This means that new Random().Next() could feasibly return a number less than the excess and cause the test to fail.
This point doesn't prohibit you from using randomised information, so long as you're randomising information that has no effect on the outcome of the test. In fact, Audacia even have an open-source repo of extension methods to help generate randomised information.
DON'T use DateTime.Now (MUST)
Using the current system time (DateTime.Now or DateTimeOffset.Now), or anything relative to the system time (DateTime.Today), can cause unexpected outcomes for your tests. When you write your tests, you write them at a specific point in time, and can't guarantee that running your test at any time will always pass. This is causes a similar brittleness to the previously covered point about using randomised information in that each time you run the test, the value of certain variables will change. If these variables have a direct impact on the application behaviour, you'll find that they won't pass 100% of the time.
Below is a test using the current system date time.
[Fact]
public void Tomorrow_is_one_business_day_away()
{
var dateToTest = DateTime.Today;
var tomorrow = DateTime.Today.AddDays(1);
var result = dateToTest.BusinessDaysUntil(tomorrow);
result.Should().Be(1);
}
This might seem innocent enough at first glance, and you've probably assumed that we're testing an extension method called BusinessDaysUntil, which returns the number of business days between two dates (much like excel's NETWORKDAYS function). Luckily, this extension method takes a DateTime parameter which allows us to test it with any given dates, rather than using DateTime.Now inside the method itself.
The above test will fail over the weekend, even though nothing's actually wrong. Instead, consider using fixed dates, or in this example always ensuring the date you're testing is a weekday.
DON'T write tests for the sake of it (MUST)
Not all unit tests are valuable, and can even cause more harm than it's worth when maintaining a codebase. Each unit test should add value, and a lot of that value comes from spotting things before testers/end users do. Why then would you write a test covering something so trivial that it will always pass. To help decide whether a unit test is worth writing, consider the value that the unit test will add to your application, against the time it takes to write and subsequently run the tests.
This blog from google discusses code coverage and why 100% code coverage can sometime lead to a false sense of security.
DON'T test using domain-specific constants (SHOULD)
This recommendation comes from personal experience: I didn't always have this approach until a co-worker wrote a unit test in this way. My initial reaction was that we should be using these "magic numbers" in our unit tests, until it was explained to me why they wrote the test as they did, which I'll go into below.
Say we have a constants file dictating static domain-specific information for an entity called Item:
public static class ItemConstants
{
public const int MinValuablePrice = 1000;
}
Consider the following extension method:
public static bool IsValuable(this Item item)
{
return item.Value > ItemConstants.MinValuablePrice;
}
Concretely, an item is considered valuable if its price is over 1000. A developer may write a unit test using this MinValuablePrice constant as follows:
[Fact]
public void Item_is_valuable_if_price_is_over_min_valuable_price()
{
var itemPrice = ItemConstants.MinValuablePrice + 1;
var item = new Item { Price = itemPrice };
var result = item.IsValuable();
result.Should().BeTrue();
}
Consider the scenario where a developer changes our constant to zero. If unintentional and unspotted, this could release a devastating bug to production environments unless a unit test were to fail and prevent the code being merged. With the above test, a developer could make this change, and unit tests would still happily pass - which we don't want to happen.
Consider the same test with the below subtle tweak for the itemPrice variable:
[Fact]
public void Item_is_valuable_if_price_is_over_one_thousand()
{
var itemPrice = 1001;
var item = new Item { Price = itemPrice };
var result = item.IsValuable();
result.Should().BeTrue();
}
Having the test written in this way adds an extra layer of security to ensure no unintentional changes to application behaviour can be released, and the concept of an item being Valuable is further explained in our tests. Note that the method name also includes the value of the constant, as it can be considered as application behaviour and therefore when the value changes it's a lot more obvious why that test starts failing.
DO treat test code as you would production code (MUST)
Since unit test projects are rarely deployed anywhere, developers can lack the same care and attention when writing unit test code. All other things being equal, some may not care how the tests are written, so long as they add the value as described above. What they may be forgetting is the value in upholding similar coding standards as they would for deployed production code, in terms of making your unit tests more readable and self-documenting.
The reason I say 'similar' above is that though you may treat it as production code, there are some things in the nature of writing tests that may require different coding standard to be enforced. A good example of this is the RCS1046 analyzer, enforcing names of asynchronous methods to be suffixed with 'Async'; in this case it would be acceptable to override this rule (with an .editorconfig for example) so that you're method name continues to describe the behaviour of the application, and we don't have tests named like public async Task My_descriptive_method_name_async().
Though no end user is ever exposed to this code, your fellow developers certainly will be, and will only want to contribute if the codebase is readable and comprehensible. The more of var stuff = ... and public void Does_the_thing()... they see, the less likely they are to do so.
A consistent approach to testing
In this post we've covered best practices for writing and maintaining unit tests. All points made here are not hard-and-fast rules, and there will certainly be caveats for each. Having best practices defined for developers less familiar with unit testing enables a consistent approach to testing, as well as lowering the likelihood of unit tests becoming more of a hindrance later down the line.
At Audacia, we uphold a high level of unit testing, and constantly share new ways and techniques to help us do so.
Audacia is a software development company based in the UK, headquartered in Leeds. View more technical insights from our teams of consultants, business analysts, developers and testers on our technology insights blog.
Technology Insights
