Tag Archives: UnitTest

Your unit tests are #rubbish #coding

This post might contain the answers to writing better unit tests… or it might not…

Having searched for examples, trying and hoping to find someone doing something vaguely similar to me, all I find are what I would call “bad examples”.

I’ve had and seen more arguments about unit tests than any other piece of code. There are always two sides to anything, but it does appear more like one person’s unit test is another’s nightmare. Over time I’ve improved the structure of my unit tests and learned what people are willing to accept for a unit test in a code review.

Here’s a bad test:

[Test]
public void TestCalculateOffsetFromUtcNow1()
{
  const int hours = 1;
  const int minutes = 2;
  const int seconds = 3;
  var now = new DateTimeOffset(2017, 9, 12, 22, 20, 0, TimeSpan.Zero);
  var instance = new ClassToTest(() => now);
  var result = instance.CalculateOffsetFromUtcNow(new TimeSpan(hours, minutes, seconds));
  var expected = now.UtcTicks
    + (TimeSpan.TicksPerHour * hours)
    + (TimeSpan.TicksPerMinute * minutes)
    + (TimeSpan.TicksPerSecond * seconds);
  Assert.That(result.UtcTicks, Is.EqualTo(expected));
}

Here’s a slightly better test:

[Test]
public void TestCalculateOffsetFromUtcNow2()
{
  // Arrange.

  const int hours = 1;
  const int minutes = 2;
  const int seconds = 3;
  var now = new DateTimeOffset(2017, 9, 12, 22, 20, 0, TimeSpan.Zero);
  var instance = new ClassToTest(() => now);

  // Act.

  var result = instance.CalculateOffsetFromUtcNow(new TimeSpan(hours, minutes, seconds));

  // Assert.

  var expected = now.UtcTicks
    + (TimeSpan.TicksPerHour * hours)
    + (TimeSpan.TicksPerMinute * minutes)
    + (TimeSpan.TicksPerSecond * seconds);
  Assert.That(result.UtcTicks, Is.EqualTo(expected));
}

Indeed, the internet examples I’ve seen often go no further than simple AAA comments, but we can do better still…

[Test]
[Description(@"
  GIVEN I have an instance of ClassToTest
  WHEN I call CalculateOffsetFromUtcNow with a TimeSpan
  THEN it returns the correct DateTimeOffset.
")]
public void TestCalculateOffsetFromUtcNow3()
{
  // GIVEN I have an instance of ClassToTest

  const int hours = 1;
  const int minutes = 2;
  const int seconds = 3;
  var now = new DateTimeOffset(2017, 9, 12, 22, 20, 0, TimeSpan.Zero);
  var instance = new ClassToTest(() => now);

  // WHEN I call CalculateOffsetFromUtcNow with a TimeSpan

  var result = instance.CalculateOffsetFromUtcNow(new TimeSpan(hours, minutes, seconds));

  // THEN it returns the correct DateTimeOffset.

  var expected = now.UtcTicks
    + (TimeSpan.TicksPerHour * hours)
    + (TimeSpan.TicksPerMinute * minutes)
    + (TimeSpan.TicksPerSecond * seconds);
  Assert.That(result.UtcTicks, Is.EqualTo(expected));
}

So, with a DescriptionAttribute and some simple commenting in the Gherkin style, we have made leaps and bounds in the maintenance quality of the test, without actually changing the code. Once you have comments to read, the name of the method is lost and becomes irrelevant; I find it only serves to find a failing test.

A common trap with TestCaseData is to want to use complex types, so you resort to TestCaseSourceAttribute, like so:

[Test]
[Description(@"
  GIVEN I have an instance of ClassToTest
  WHEN I call CalculateOffsetFromUtcNow with a TimeSpan
  THEN it returns the correct DateTimeOffset.
")]
[TestCaseSource(nameof(GetTestCases))]
public void TestCalculateOffsetFromUtcNow4(DateTimeOffset now, TimeSpan timeSpan, DateTimeOffset expected)
{
  // GIVEN I have an instance of ClassToTest
  var instance = new ClassToTest(() => now);

  // WHEN I call CalculateOffsetFromUtcNow with a TimeSpan
  var result = instance.CalculateOffsetFromUtcNow(timeSpan);

  // THEN it returns the correct DateTimeOffset.
  Assert.That(result, Is.EqualTo(expected), $"Expected a result of {expected}");
}

private static IEnumerable<TestCaseData> GetTestCases()
{
  // ( now , timeSpan , expected )
  yield return new TestCaseData( new DateTimeOffset(2017, 9, 12, 22, 20, 0, TimeSpan.Zero) , TimeSpan.FromSeconds(0) , new DateTimeOffset(2017, 9, 12, 22, 20, 0, TimeSpan.Zero) );
  yield return new TestCaseData( new DateTimeOffset(2017, 9, 12, 22, 20, 0, TimeSpan.Zero) , new TimeSpan(1, 2, 3)   , new DateTimeOffset(2017, 9, 12, 23, 22, 3, TimeSpan.Zero) );
  yield return new TestCaseData( new DateTimeOffset(2017, 9, 12, 23, 20, 0, TimeSpan.Zero) , new TimeSpan(1, 2, 3)   , new DateTimeOffset(2017, 9, 13, 0, 22, 3, TimeSpan.Zero)  );
}

Indeed, we now test for more values, our test code itself is reduced, but the values have moved into some other method that gets very messy as the number of parameters grows. I’ve spent many hours in the past going through TestCaseData constructor parameter lists and I can say that I don’t care much for it. Even just earlier today I banging was my head against the wall at some huge TestCaseSource methods, they are painful to read, painful to maintain and probably more burden than benefit.

Therefore, don’t move away from¬†TestCaseAttribute in the first place, work around it, like so:

[Test]
[Description(@"
  GIVEN I have an instance of ClassToTest
  WHEN I call CalculateOffsetFromUtcNow with a TimeSpan
  THEN it returns the correct DateTimeOffset.
")]
//       ( now                         , timeSpan   , expected                    )]
[TestCase( "2017-09-12T23:20:00+00:00" , "00:00:00" , "2017-09-12T23:20:00+00:00" )]
[TestCase( "2017-09-12T22:20:00+00:00" , "01:02:03" , "2017-09-12T23:22:03+00:00" )]
[TestCase( "2017-09-12T23:20:00+00:00" , "01:02:03" , "2017-09-13T00:22:03+00:00" )]
public void TestCalculateOffsetFromUtcNow5(string now, string timeSpan, string expected)
{
  // GIVEN I have an instance of ClassToTest
  var instance = new ClassToTest(() => DateTimeOffset.Parse(now));

  // WHEN I call CalculateOffsetFromUtcNow with a TimeSpan
  var result = instance.CalculateOffsetFromUtcNow(TimeSpan.Parse(timeSpan));

  // THEN it returns the correct DateTimeOffset.
  Assert.That(result, Is.EqualTo(DateTimeOffset.Parse(expected)), $"Expected a result of {expected}");
}

Top tip #1: Spend the time to line up your values, it’s only a few spaces and makes it a lot more readable. Also repeat the parameter names and line those up, it helps a lot in larger tables of test data.
Top tip #2: I often make good use of enum parameters, they are easy to combine, you can drive logic from them that is pretty readable too.

My version of unit test good practice:

  1. Unit test code must fulfill the same quality criteria as the code it is testing.
  2. Comment the test method at the top with the intention, the requirements being tested, etc. Gherkin is a very helpful syntax for this.
  3. Don’t put Act Arrange Asset comments without anything else, it is just plain lazy and virtually useless. You can split up the test method comment and replicate where they apply to. Future you or a colleague will be glad you did.
  4. Avoid generated test case inputs (e.g. TestCaseSource), they make the tests less readable, though there are occasionally very good reasons for using them, just do so wisely.

My rules that aren’t universally accepted:

  1. Code is code, no snake_casing methods, for example if PascalCase is the language standard, then do that.
  2. The name of a test method does NOT matter, yes, I said that. I once renamed all my test methods with vague names, showed them to a colleague, they read the comment at the top and didn’t care what the name was. Though I do like my test methods to begin with the word Test, it’s usually because it is something like TestAbcDoesSomething and it stands out from helper methods within the test class.
  3. Don’t create a test class for each test you write, group them by the class that is under test.

My observations:

  1. Writing good unit tests is harder than writing good code.
  2. Unit testing when prototyping will slow you up.
  3. Unit testing improves the quality of my code. I believe I write good code, so I often question why I am writing a test… then I find an issue and I’m glad I did.
  4. The biggest enemy of a unit test is a refactor, minimizing copy and paste code in your tests will help reduce the burden. Also don’t fear deleting a test, just ensure those requirements are covered by new or other existing tests.
  5. A “good” unit test is something you are proud of and also something someone is willing to accept in a code review – fortunately, the two normally go hand in hand.
  6. SpecFlow should not be used for unit tests, this may be a whole post in itself, so I won’t dwell on it.
  7. A unit test will take on average twice or three times the amount of time the actual code took to write.

To see the examples all in once place: https://github.com/piddle/Piddle.NUnitExamples

I hope someone finds this post useful, please comment if you wish.

Advertisements

Detouring a constructor with Microsoft Fakes #dev #coding #tdd

I wanted to ensure every SmtpClient instance that was created by a library was also disposed of, to do this I first needed to Shim SmtpClient using Microsoft Fakes:

  1. Add Fakes Assembly for System.
  2. Edit Fakes/System.fakes
    Add

    <ShimGeneration>
      <Add TypeName="SmtpClient"/>
    </ShimGeneration>
  3. Save the file and close
  4. Rebuild, open the System.4.0.0.0.Fakes reference and under System.Net.Mail.Fakes you should see ShimSmtpClient.

My idea was to count up every time the constructor was called and to count down every time one was disposed, to do this I created a counter:

int instancesNotDispose = 0;

Incremented it in the Constructor Shim, that is wrap the constructor and do some more things rather than completely detouring the constructor:

ShimSmtpClient.Constructor =
    x =>
    {
        // Execute outside of Shims otherwise you'll be in StackOverflowException territory
        ShimsContext.ExecuteWithoutShims(
        () =>
        {
            // Use reflection to get the default constructor and invoke it, the magic here is the instance is stashed into x!
            var constructor = typeof(SmtpClient).GetConstructor(new Type[0]);
            constructor.Invoke(x, new Object[0]);
        });

        instancesNotDispose++;
    };

Then decremented it in the Dispose Shim:

ShimSmtpClient.AllInstances.Dispose =
    x =>
    {
        // Execute outside of Shims otherwise you'll be in StackOverflowException territory
        ShimsContext.ExecuteWithoutShims(x.Dispose);

        instancesNotDispose--;
    };

Then I can call my library a bunch of times to do whatever it does and sends some e-mails, then I can assert they have all be disposed:

Assert.AreEqual<int>(0, instancesNotDispose, String.Format("Doh! {0} were not disposed", instancesNotDispose));

Assuming there aren’t other SmtpClient constructors that do not call the default constructor then this code should work fine, if there are other constructors called that do not call the default constructor then this Assert will also help find those. There are other ways, for example, stashing each instance in a List and ensuring each is in both the Constructor List and the Dispose List, but this counting approach was enough for my needs for now.

Unit Test Projects and NUnit compatibility

I often need to write Unit Tests and have them accessible from NUnit as well as the Visual Studio Test View (etc), so I thought I’d write myself a walkthrough of the common things I have to do.

Firstly, whichever language I’m using, Visual Studio is normally configured to create the wrong one, so let’s say I’ve got my C# hat on and I create a test project:

Darn it, it’s created a VB.NET one, so I delete it and search for the setting or way I create a C# one. This can be located under Tools -> Options -> Test Tools -> Test Project, then choose your preference from the “Default test project language” dropdown, like so:

You will also notice you can choose what (if any) template test classes are created for you. 99% of the time I use Unit Test and that is it.
So you go back in and create the project again and it’s created a C# one, it automatically adds a reference to the Visual Studio Test classes, great:

Add a reference to NUnit, so you have:

Now let’s look at the Unit Test class, I’ve stripped it down to the bare bones:

using System;
using System.Text;
using System.Collections.Generic;
using System.Linq;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace TestProject1
{
    [TestClass]
    public class UnitTest1
    {
        [TestMethod]
        public void TestMethod1()
        {
        }
    }
}

Since we don’t want the Visual Studio and the NUnit classes to conflict my approach is to take out the Visual Studio using statement like so:

using System;
using System.Text;
using System.Collections.Generic;
using System.Linq;

namespace TestProject1
{
    [Microsoft.VisualStudio.TestTools.UnitTesting.TestClass]
    public class UnitTest1
    {
        [Microsoft.VisualStudio.TestTools.UnitTesting.TestMethod]
        public void TestMethod1()
        {
        }
    }
}

Now this will still work as before, but we can add NUnit in now too, like so:

using System;
using System.Text;
using System.Collections.Generic;
using System.Linq;
using NUnit.Framework;

namespace TestProject1
{
    [Microsoft.VisualStudio.TestTools.UnitTesting.TestClass]
    [TestFixture]
    public class UnitTest1
    {
        [Microsoft.VisualStudio.TestTools.UnitTesting.TestMethod]
        [Test]
        public void TestMethod1()
        {
        }
    }
}

And proof of the pudding, Visual Studio:

NUnit:

This also reminds me, while I’m on the subject, to be able to open a Visual Studio project from within NUnit there is a setting you need to change, go to Tools -> Settings -> IDE Support -> Visual Studio and then tick the “Enable Visual Studio Support” checkbox, like so: