NHibernate's test suite is a mess

I'm not doing it on purpose, I haven't converted into fighting NHibernate's foe, but it looks it will be another rather not encouraging post about NHibernate. But when I saw it's so called unit tests suite, I couldn't resist on sharing my thoughts.

NHibernate is using CodeBetter's TeamCity. We can see more than 200 tests ignored there. This doesn't bode well itself.

I've looked at NHibernate 3.2 source code I already had on the disk, but I've also compared it a bit with the current GitHub version and the situation haven't changed drastically since then.

Looking at the scope of most of the tests, they are more integration than unit tests - they run a complete top-down calls. This means that it requires a deep knowledge of the system to tell what went wrong when test fails and virtually no chance for anyone not being deep inside the codebase to know how to fix it.

It's nothing wrong with integration testing, of course, but not if the recurring pattern is that they are just a smoke tests - verifying only if there was an exception thrown or not. I've run the tool to find tests without assertions (that I'm going to publish some day) - and it turned out that there are 328 assertionless tests! More than 7% of the whole suite doesn't actually verify any outcome. And it is for sure possible to remove a whole lot of the code, even theoretically test-covered code, and cause no test to fail!

But way more interesting are the tests that do not increase the code coverage even incidentally! I like this one:

[TestFixture]
public class Fixture : BugTestCase
{
    [Test]
    public void Bug()
    {
        // Do nothing, we only want to check the configuration
    }
}

Note also the incredibly pure naming schema. There are 36 tests named Bug and 35 named just Test in the codebase and at least 80 test classes named Fixture. There is some kind of convention for namespaces, though, but with a lot of exceptions.

What else can we see in the NHibernate's "unit" test suite? Well, everything! We have a real file system access, real database access, time measurement, dependency on driver configuration etc. There is a lot of code which should be unit-tested itself.

Those tests, no matter if we name it unit-, integration- or whatever-tests, are poorly factored and contain multiple asserts. Look at the MasterDetail test here - 180 lines of code and 40 asserts in single test! The longest class test I've encountered was FooBarTest - it has 5729 lines (and has pretty nice name, hasn't it?).

And for the end, this is the line I liked the most:

catch(Exception e)
{
    Assert.IsNotNull(e); //getting ride of 'e' is never used compile warning
}

Wow, we have an assertion! But, as the comment suggests, it was added only to shut the compiler up, not to verify anything valuable. Not to mention that there are better ways to cope with compiler warnings...

Anyway, if you'd ever be looking for a good unit test suite to learn something from, at least you know where definitively not to look!

Top 5 Pragmatic TDD mistakes

Today, on Telerik's blog, Bradley Braithwaite published his post "Top 5 TDD Mistakes". I like to comment on "top N mistakes"-kind of posts, so read the Bradley's post first and let me add my three cents about this one, too.

I'm pretty satisfied to read someone who is passionate and pragmatic about Test-Driven Development in the same time. Let's just quote two important sentences from Bradley's article I agree with totally:

1. It's better to have no unit tests than to have unit tests done badly.
2. Unit Tests make me more productive and my code more reliable.

Let me comment on each of the mistake mentioned in the article.

Not using a Mocking Framework vs. Too Much Test Setup

One may say that these two are a bit contradictory as using a mocking framework may lead to too much test setup. This is of course a matter of good balance we need to find and maintain.

If we're testing a code that has some dependencies and not using mocking framework, we're not doing unit testing in its pure form, for sure. But let's take my favourite example of testing the code that talks with the database - is there really anyting better than testing against the real database, run in-memory? Call it integration test, half-arsed test or whatever, that's the only reliable method of testing the queries itself.

But in most cases, sure, let's use stubs, but avoid mocks - the difference is very important. Mocks as a behaviour testing tool are often overused to check if the implementation is exactly as defined in the test, written by the same person in the same time - it's like checking if the design we've just intentionally created was really intentional. Let's focus on verifying the actual outcome of the method (in which stubs may help a lot) instead of coupling to the implementation details.

And, agreed, there's nothing worse to maintain than a test suite with those spaghetti-flavoured mock setups.

Asserting Too Many Elements

Agreed. There's a good rule of having single logical assert per test, even if it physically means several Assert calls. And for me having any asserts at all is a rule of equal importance.

Writing Tests Retrospectively

Agreed again, the most of TDD's gain is from having to think about the implementation before it comes to existence. It can make developer think not only about happy paths, but also about negative cases and boundary values. It also strongly supports KISS and YAGNI rules which are vital for long-living codebases.

Also, I like using TDD to cope with reported bugs. Writing failing unit (or - again - probably more integration-like) test by recreating the failure's conditions makes analysis easier, helps isolating the root cause of the failure and is often much easier than reproducing the bug in real-life scenario. Retrospectively written test for a bug is only for regression testing.

Testing Too Much Code

Not an universal truth, but I feel quite the same. I see a lot of value in unit-testing my workhorse parts of code, I do create it more-less according to the TDD principles (especially having no production code without failing test).

But I don't think having 100% code coverage as an ultimate goal makes any sense. I think there's always quite a lot of code that is just not suitable for valuable unit testing, i.e. coordinating/organizing kind of code (let's call it composition nodes as a reference to composition root), that just takes some dependencies, calls several methods and pass things from here to there, not adding any logic and not really interfering with the data. Tests written for that kind of code are often much more complicated than the code itself, due to heavy mocks and stubs utilization. Bradley's rule of thumb is what works for me, too: write a separate test for every IF, And, Or, Case, For and While condition within a method. I'd consider the code to be fully covered when just all that branch/conditional statements are covered.

Does it make sense to AssertWasNotCalled?

In the recent post about the importance of assertions in the unit tests I've mentioned that we should not test our code to prove what it's not doing and focus on testing what it is actually doing. Regarding that point of view I was asked whether I find it useful to use AssertWasNotCalled or similiar assertion methods provided by the mocking frameworks, as generally they are to ensure no work was done and an empty method is passing that kind of tests. This is an interesting question, let's analyze it.

I don't blindly believe in TDD and all its rules in every scenario, but let's stick to that context. As an example, let's write and test a Work method which is taking a boolean parameter to decide whether the actual work should be done.

[Test]
public void Should_do_no_work_when_false_given()
{
    // arrange
    var sut = new Service(workerFake);

    // act
    sut.Work(false);

    // assert
    workerFake.AssertWasNotCalled(x => x.DoWork());
}

I've discouraged testing what the code is not doing in the context of the third test-driven development rule, which says "you may not write more production code than is sufficient to pass the current failing test". According to that rule, for the test above, we may only end up with an empty method as an implementation - it will be just enough for the test to pass.

But earlier, there is the first rule of TDD - "You may not write production code until you have written a failing unit test". We've never saw the test failing.

Let's than approach the problem differently - starting from the "positive" path, where we can actually verify any outcome, instead of verifying the lack of it.

[Test]
public void Should_do_the_work_when_true_given()
{
    // arrange
    var sut = new Service(workerFake);

    // act
    sut.Work(true);

    // assert
    workerFake.AssertWasCalled(x => x.DoWork());
}

We can easily see this test failing for the empty Work method. Let's then implement it according to TDD rule number three (no single line of code not enforced by the tests).

public void Work(bool really)
{
     worker.DoWork();
}

The test is now passing, rules are kept. We may now add back our "negative" test case - verifying no work is done when false is passed. This time the test will fail, rule number one is satisfied. We may now legally (according to the TDD) add the condition and finish our pretty sophisticated implementation:

public void Work(bool really)
{
     if (really)
         worker.DoWork();
}

The conclusion from this oversimplified example is that the AssertWasNotCalled test alone probably makes a little sense. But when considered as a part of the larger test suite for the method, and when implemented in correct order (outcomes verified first, lack of outcome last), it may even be the only TDD-compliant way of testing the intended behaviour.

The same rule applies to the validation method example I've mentioned in the previous post. Even if I'm against the unit tests that just expect no exception being thrown, if throwing an exception when needed is the method's single responsibility, there's no other way to test the "positive" (not throwing) behaviour in the unit tests other than verifying no exception was thrown.

As a side note - AssertWasNotCalled assertion method can actually be rewritten (in Rhino Mocks syntax) into:

workerFake.Stub(x => x.DoWork()).Throws(new Exception());

And when the production code actually calls the method it should not, the exception is thrown and the test fails. So the distinction between the test I consider assertionless and the valid one can be a bit blurry. But my personal opinion I'll stick with is to always have the assertions as explicit as possible, even if this is just a syntactic sugar or no-op like AssertDoesNotThrow. This allows us to express the intent of the test clearly, making it readable and maintainable. Smoke testing approach - "if it throws, it will fail anyway" doesn't.

Image borrowed from http://www.yesodweb.com.

Assertion is the key of unit testing

We all probably know about the importance of good unit test suite for our projects and about the Arrange-Act-Assert pattern we should be using in our tests. I think I'm lot more pragmatist than purist and I don't have anything against the tests not following the AAA form as long as I can read, understand and maintain them easily. But there's one thing I do consider a must in each and every unit test - an assertion.

Assertion not only decides whether the test passes or fails, it also makes the purpose of the test clear and ensures it is verifiable. Test without assertion is nothing more than a random snippet of code.

What I consider to be the assertion?

• A direct call to a known asserting method, provided most often by the testing frameworks, assert or mocking libraries, etc., like the classic Assert.Equal(actual, expected) or Shouldy's extension method like actual.ShouldBe(expected). The naming convention of these constructs always makes it clear that they actually verify the outcome - "assert that...", "verify that...", "something should be...".
• An indirect call to a method described above. When verification of the outcome is not just a comparison of a single object (or the comparison we need is not included in our favourite testing framework), we often create our own helper methods that play a role of a single logical assertion, despite multiple physical ones, i.e.

  private void ShouldBeAValidExternalUri(this string value)
  {
      value.StartsWith("http://our.domain.com/").ShouldBeFalse();
      Uri.IsWellFormedUriString(value, UriKind.Absolute).ShouldBeTrue();
  }

  That's still a great assertion, call to ShouldBeAValidExternalUri will eventually trigger a call to the known asserting method provided by the framework. It's nice if we maintain the naming schema that indicates we're going to verify something in our helper method.
• Throwing an exception within the test or within the helper asserting method. It's technically the equivalent of calling Assert.Fail. It also provides a good way of describing what went wrong using the exception message as it will probably end up shown in the test runner.

What I do not consider to be the assertion?

Throwing an exception in the SUT itself! It is quite an often thing to see in some projects - tests that call a method on the tested object with an assumption that the tested method will throw if something goes wrong.

public class Sut
{
    public static void DoTheWork(int howManyTimes)
    {
        if (howManyTimes <= 0)
            throw new ArgumentOutOfRangeException("howManyTimes");

        for (var i = 0; i < howManyTimes; ++i)
            DoTheRealWork();
    }

    // ...
}

public class Test
{
    [Test]
    [ExpectedException(typeof(ArgumentOutOfRangeException))]
    public void SutThrowsWhenNegativeCounterPassedIn()
    {
        Sut.DoTheWork(-1);
    }

    [Test]
    public void SutDoesntThrowWhenPositiveCounterPassedIn()
    {
        Sut.DoTheWork(1);
    }
}

The Sut class is a perfectly valid piece of defensive code. And we have two tests, one checking if the exception is thrown when negative number is passed, and the second...? What is the second testing? Technically it works - if an exception is thrown in the tested code, the test would fail. If not - it would pass, proving that no exception was thrown.

But what is the value given by this test? It is not verifying whether the work was actually done. What more, according to the TDD rule that we should have only the minimal amount of code making the test pass, the correct implementation of the DoTheWork method required by SutDoesntThrowWhenPositiveCounterPassedIn test is a no-op! Both tests will still pass if we remove the whole loop. So was there anything really tested?

A good test should always make it clear and explicit what is tested and what is the expected outcome. "Expect no exception" approach doesn't follow that rules. "No exception" is not a good outcome if an empty method is enough for it to pass. And proving that your code doesn't do something is much more difficult than proving what it does.

Of course, as always, there are some cases when it's maybe acceptable to have tests without explicit assertion. Tests for validation methods that exist only to throw when certain condition is not met may be a good example. Another example is smoke testing, as a part of integration test suite. When we have the functionality well-covered with lower-level unit test suite and we just want to ensure that the code doesn't blow up when run together and verifying the outcome at this level is hard enough, we may go with "expect no exception" test.

But even in those cases I personally would use some kind of explicit construct to show the intent of the test. For example, NUnit offers assertion like this:

Assert.DoesNoThrow(() => Validator.Validate("valid string"));

It's not perfect, but at least more explicit than the sole SUT method call.