7 Common Mistakes Programmers Make When Writing Unit Tests

Writing good unit tests is not easy. Especially the first tests can be a bit difficult for you, so to help you I wanted to present you 7 in my opinion the most common mistakes beginners make when writing unit tests, which you should avoid.

1) Bad naming of test methods

public class LeapYear
{
    public bool IsLeapYear(int year)
    {
        if (year % 4 != 0)
            return false;

        if (year % 100 == 0 && year % 400 != 0)
            return false;

        return true;
    }
}

I also wrote a sample unit test for this method.

public class LeapYearTests
{
    [Test]
    public void IsLeapYear()
    {
        var leapYear = new LeapYear();

        var result = leapYear.IsLeapYear(2020);

        result.Should().BeTrue();
    }
}

The name of the method in the test class is the same as in the tested class. So, this name is not the worst yet. Because I have come across worse names, such as Test1, etc. But despite everything, this name is still not good, because it tells us nothing about the test. We can only guess that it tests the IsLeapYear method. But if we want to know what exactly it tests, we have to delve into the entire test. The test is not complicated either, in this case we will quickly find out what is being tested, but what about more complicated tests?

Let's see what happens if there is an error in our tested method.

public bool IsLeapYear(int year)
{
    if (year % 41 != 0)//intentional error
        return false;

    if (year % 100 == 0 && year % 400 != 0)
        return false;

    return true;
}

After running the test, we received the following message: "IsLeapYear - Expected results to be true, but found False."

You have to admit that the message is very general and doesn't tell us much.

So what should this method be called? Of course, there are different conventions for naming tests, it's important to establish one with your team and use it throughout the project. The worst thing that can happen is if each developer uses a different convention. I usually use a naming convention where I first give the name of the method, then describe the scenario being tested, and the expected result. So in our case, the name of the test method might look like this:

public class LeapYearTests
{
    [Test]
    public void IsLeapYear_WhenLeapYear_ShouldReturnTrue()
    {
        var leapYear = new LeapYear();

        var result = leapYear.IsLeapYear(2020);

        result.Should().BeTrue();
    }
}

With such a method name, we no longer have to look into the body of this method, because we know exactly what it is responsible for. If we now have an error, we can immediately see in test explore what and in which method went wrong. This time, the message is: " IsLeapYear_WhenLeapYear_ShouldReturnTrue - Expected results to be true, but found False."

The error message itself does not change, but thanks to the name of the test - the appropriate name of the test, we immediately know what, how and where went wrong.

2) No assertions in unit tests

public class EvenOrOdd
{
    public string GetEvenOrOddMsg(int number)
    {
        if (number % 2 == 0)
            return "Even";

        return "Odd";
    }
}

We also already have a unit test prepared for the method (which is not good).

public class EvenOrOddTests
{
    [Test]
    public void GetEvenOrOddMsg_WhenNumberIsEven_ShouldReturnEven()
    {
        var evenOrOdd = new EvenOrOdd();

        var even = evenOrOdd.GetEvenOrOddMsg(2);
    }
}

As you can see in this test, first we have the initialization of the object, i.e. arrange. Then we have act, i.e. calling the tested method. However, at the end we lack assertion, i.e. verification whether the result returned in act will be correct. If I run this test now, it will be green, even though we did not write the assertion. Such a lack of assertion is a very common practice.

It most often results from 3 things. First, the programmer could have forgotten about this assertion, although I admit that it is unlikely, but someone could have distracted him from work, and later he forgot that he had an infinite test. The worst thing is that tests without assertion are green tests, i.e. they pass as correct. Secondly, it often happens that the programmer does not really want to write tests, or simply does not know how, and because the business requires it, then such tests are created that do not really test anything, but increase code coverage. Because if you look at the % of code coverage by tests, then of course such a test increases this indicator. Thirdly, and I think this is the most common reason for such assertions. If a programmer sees that a test written by another programmer does not pass, then instead of delving into the details, they simply comment or remove the assertion and then magically the tests are green.

3) Using loops or conditional statements in tests

[Test]
public void GetEvenOrOddMsg_WhenCalled_ShouldReturnCorrectMsg()
{
    var evenOrOdd = new EvenOrOdd();

    for (int i = 0; i  < 10; i++)
    {
        var msg = evenOrOdd.GetEvenOrOddMsg(i);

        if (i % 2 == 0)
            Assert.That(msg, Is.EqualTo("Even"));
        else
            Assert.That(msg, Is.EqualTo("Odd"));
    }
}

Here we have a loop that will execute 10 times and depending on whether the number should be even or odd, an appropriate message is returned. Apart from the fact that this is a repetition of the logic from the production code, it is unreadable and useless, in the event that this test fails, we will get a message that probably will not help us at all.

Maybe I will first add an intentional error to this code, then we will run the test and see what the result is.

[Test]
public void GetEvenOrOddMsg_WhenCalled_ShouldReturnCorrectMsg()
{
    var evenOrOdd = new EvenOrOdd();

    for (int i = 0; i  < 10; i++)
    {
        var msg = evenOrOdd.GetEvenOrOddMsg(i);

        if (i % 2 != 0)//intentional error
            Assert.That(msg, Is.EqualTo("Even"));
        else
            Assert.That(msg, Is.EqualTo("Odd"));
    }
}

After running this test, of course the result is red. We got the following message: "GetEvenOrOddMsg_WhenCalled_ShouldReturnCorrectMsg - Expected string length 3 but was 4. Strings differ at index 0. Expected: Odd But was: Even", which doesn't really say anything about it.

Of course, this example is very bad, but if we want to use loops in our tests, we can probably write the same thing according to good practices using parameterized tests. In NUnit, the TestCase attribute is used for this. Such a test might look like this:

[TestCase(2, "Even")]
[TestCase(4, "Even")]
[TestCase(2, "Odd")]
[TestCase(3, "Odd")]//celowy błąd
public void GetEvenOrOddMsg_WhenCalled_ShouldReturnCorrectMsg(int number, string message)
{
    var evenOrOdd = new EvenOrOdd();

    var result = evenOrOdd.GetEvenOrOddMsg(number);

    result.Should().Be(message);
}

We have similar code, i.e. one that tests several cases, but it is much more readable. We do not have repeated logic in the test and if the test fails, we know exactly which case returned an error. All these tests are treated as separate tests. In the test explorer, 4 different errors will appear and if an error occurs, it will know exactly which case it concerns. Example error message: "GetEvenOrOddMsg_WhenCalled_ShouldReturnCorrectMsg(2, Odd - Expected result to because Odd with a length of 3, but Even has length of 4, differs near Eve (index 0)."

4) Assertions with logic

public class Calculator
{
    public int Add(int number1, int number2)
    {
        return number1 + number2;
    }
}

We also have a unit test for this method (which is bad):

public class CalculatorTests
{
    [Test]
    public void Add_WhenCalled_ShouldReturnSum()
    {
        var calculator = new Calculator();

        var result = calculator.Add(1, 2);

        result.Should().Be(1 + 2);
    }
}

In the assertion we used calculations. Additionally, the same ones as in the tested method. Which makes this test useless. Specific results should be passed to the assertion, not calculated only in the assertion. So in our example, there should be a hardcoded 3 and then the assertion is correct. So the test should look like this:

public class CalculatorTests
{
    [Test]
    public void Add_WhenCalled_ShouldReturnSum()
    {
        var calculator = new Calculator();

        var result = calculator.Add(1, 2);

        result.Should().Be(3);
    }
}

5) Testing only the optimistic path

public class Calculator
{
    public int Divide(int dividend, int divisor)
    {
        if (divisor == 0)
            throw new DivideByZeroException();

        return dividend / divisor;
    }
}

Inside the method we also have a divisor check. If the divisor is 0, an appropriate exception will be thrown. Finally, calculations are performed.

This is what an example test might look like:

public class CalculatorTests
{
    public void Divide_WhenCalled_ShouldReturnDivision()
    {
        var calculator = new Calculator();

        var result = calculator.Divide(4, 2);

        result.Should().Be(2);
    }
}

This method is well written, but we have 1 bug here. Only the optimistic path is tested, but it is not enough. We should also test different boundary conditions. In particular, we should be interested in the condition when the divisor is 0. In this case, a DivideByZeroException exception should be thrown.

public class CalculatorTests
{
    public void Divide_WhenCalled_ShouldReturnDivision()
    {
        var calculator = new Calculator();

        var result = calculator.Divide(4, 2);

        result.Should().Be(2);
    }

    public void Divide_WhenDivisorIsZero_ShouldThrowDivideByZeroException()
    {
        var calculator = new Calculator();

        Action action = () => calculator.Divide(4, 0);

        action.Should().ThrowExactly <DivideByZeroException>();
    }
}

So, you have to remember not only to test optimistic paths, but also all boundary conditions, only then can you say that the method is properly tested. Additionally, you have to remember that sometimes we test not only what is supposed to happen, but also what is not supposed to happen. That is, if you are testing a method that takes orders only from a specific user, you should test whether it does not also return orders from other users.

6) Trying to test everything

7) Overusing mocks

public class OrderService
{
    public decimal CalculateDiscount(decimal orderAmount, Customer customer)
    {
        if (customer == null)
            throw new ArgumentNullException(nameof(customer));

        if (customer.IsNewCustomer)
            return 0;

        if (orderAmount > 100)
            return 20;

        return 0;
    }
}

public class Customer
{
    public int Id { get; set; }
    public bool IsNewCustomer { get; set; }
}

I have prepared the OrderService class. Here we have one public method CalculateDiscount, which based on the passed customer and the order amount, returns the discount value. A well-written Unit Test for such a method might look like this:

public class OrderServiceTests
{
    [Test]
    public void CalculateDiscount_WhenCustomerIsNew_ShouldReturn0()
    {
        var orderService = new OrderService();

        var discount = orderService.CalculateDiscount(1m, new Customer { IsNewCustomer = true });

        discount.Should().Be(0);
    }
}

Of course, this is just a test for one case that we want to test. We will not analyze all cases. So we will only look at this one case, that is, if the customer is a new customer, the discount will be zero. This test is very good. After running the test, the result is green, which means the test passes correctly.

Now I will show you how you should not write tests for such a case. I often come across tests where mocks are substituted almost everywhere. See how such tests look then.

First, instead of the Customer class, a new ICustomer interface is created.

public interface ICustomer
{
    int Id { get; set; }
    bool IsNewCustomer { get; set; }
}

The Customer class implements this interface:

public class Customer : ICustomer
{
    public int Id { get; set; }
    public bool IsNewCustomer { get; set; }
}


public class Customer : ICustomer
{
    public int Id { get; set; }
    public bool IsNewCustomer { get; set; }
}

The interface we just created is passed as a parameter to the CalculateDiscount method:

public class OrderService
{
    public decimal CalculateDiscount(decimal orderAmount, ICustomer customer)
    {
        if (customer == null)
            throw new ArgumentNullException(nameof(customer));

        if (customer.IsNewCustomer)
            return 0;

        if (orderAmount > 100)
            return 20;

        return 0;
    }
}

Thanks to which this property can be mocked. And the test in this case can look like this:

public class OrderServiceTests
{
    [Test]
    public void CalculateDiscount_WhenCustomerIsNew_ShouldReturn0()
    {
        var orderService = new OrderService();

        var discount = orderService.CalculateDiscount(1m, new Customer { IsNewCustomer = true });

        discount.Should().Be(0);
    }

    [Test]
    public void CalculateDiscount_WhenCustomerIsNew_ShouldReturn0_Mock()//bad example
    {
        var customer = new Mock <ICustomer>();
        customer.Setup(x => x.IsNewCustomer).Returns(true);
        var orderService = new OrderService();

        var discount = orderService.CalculateDiscount(1m, customer.Object);

        discount.Should().Be(0);
    }
}

True, after running both tests pass. Both tests seem to be fine. However, compare these 2 tests with each other, firstly the test using mock is much less readable. If the example were even more complicated, then initializing the mock would reduce the readability of this test even more. Secondly, we had to create a new interface for the Customer class, so that it would be possible to mock it, and in this situation it is completely unnecessary.

So, to sum up, it is best to mock only external dependencies, i.e. contact with the database, files, web services, etc. If we mock too many, then firstly our test classes become more extensive, more complicated, and they should be short, readable and simple. If we want to mock too much, then we have to create many interfaces. Each such class must have its own interface, often just to write tests for it. If we pass a lot of such dependencies, usually through constructors, then these constructors have a lot of parameters, and this is also not good. You have to remember that every mock introduces distortion into the tests, and it is best to have as few distortions in the tests as possible. So, most often, you should mock only external dependencies, there are of course exceptions to this rule. For example, if we have 2 classes, one of them uses the other and because of this we have a lot of new test cases, then sometimes it is worth using a mock for such cases. In our case, however, in the example above, mocks are unnecessary.

Unit Testing School

If you would like to learn how to write unit tests in C#/.NET applications, consider joining the Unit Testing School - [here].

SUMMARY

• Bad naming of test methods.
• Lack of assertions.
• Using loops or conditionals in tests.
• Assertions with logic.
• Testing only the optimistic path.
• Trying to test everything.
• Overusing mocks.

Please try to avoid at least these 7 basic mistakes. This will make your unit tests good and bring a lot of value to your projects.

If you liked this article, be sure to join my community. Sign up for the free newsletter, where every week I share valuable materials, especially regarding C# and the .NET platform (free subscription - newsletter).