Thursday, February 14, 2013

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!

11 comments:

  1. NHibernate is open source and there is a "Pull Request" button on GitHub. You are welcome.

    Yuo can cover all the source code with "unit"-test but they will all be useless.

    ReplyDelete
    Replies
    1. I'm not saying that there should be 100% unit test coverage everytime. And first of all, I think it makes no sense to write unit tests retrospectively just for the sake of having any unit tests. I'm just pointing out that the majority of integration/regression/smoke/whatever-you-call-it tests do not verify any outcome. I think that makes them very weak.

      Delete
  2. Try to write unit tests for this issue (https://nhibernate.jira.com/browse/NH-3395)

    var query = from o in db.Orders
    select new ExpandedWrapper>
    {
    ExpandedElement = o,
    ProjectedProperty0 = o.OrderLines,
    Description = "OrderLine",
    ReferenceDescription = "OrderLine"
    };

    var result = query.ToList();
    Assert.That(result.Count, Is.EqualTo(830));
    Assert.That(NHibernateUtil.IsInitialized(result[0].ExpandedElement.OrderLines), Is.True);

    ReplyDelete
    Replies
    1. Well, there are some assertions, this is not the kind of test I'm complaining about.

      Delete
  3. I can certainly agree that there are problematic issues, e.g poorly named tests and other problems.

    However, one factor regarding the number of ignored tests is that the NHibernate test suite is regularly exercised on 7 different RDMBS engines/APIs. There are a bunch of tests that are ignored for a specific RDBMS due to lack of features in that DB (or lack of interest from its users to improve NHibernate's support for it).

    As for unit versus integration testing, I would say that the story is mixed. More (pure) unit tests would probably add value, but on the other hand it wouldn't obviate the need for integration testing. After all, it's the end-to-end functionality that the user (e.g. application developer) actually cares about.

    ReplyDelete
    Replies
    1. Agreed. But are those 300 tests that do not verify any outcome really integration tests? Are they proving anything about the correctness of query results etc.?

      Delete
    2. I suspect that many of those were added in respose to some exception, so they do verify that the exception doesn't occur. Not to say they couldn't be improved of course.

      Delete
  4. Did you notice that both MasterDetailTest and FooBarTest is located in a folder named "Legacy"? Seems someone have long since agreed with you on their quality. :)

    Look for e.g. the Linq tests, for a generally better structure.

    ReplyDelete
  5. Just for the record in C# you can catch an exception without creating a variable

    try
    {
    ...
    }
    catch(Exception)
    {
    ...
    }

    You don't even need to supress the compiler warning because the compiler warning is correct. The code should have been written in a different way.

    ReplyDelete
    Replies
    1. It was exactly what was on my mind here: "Not to mention that there are better ways to cope with compiler warnings" :)

      Delete
    2. May as well get rid of the (Exception) altogether if you're not using it ..

      try { .. } catch { .. }

      Delete