Your unit tests are #rubbish #coding

Posted on September 20, 2017 | Leave a comment

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.

This entry was posted in Software development and tagged , , , , , . Bookmark the permalink.

Leave a comment