Monday, January 28, 2013

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.

Wednesday, January 23, 2013

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.

Monday, January 21, 2013

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.

Tuesday, January 8, 2013

9th most common mistake - using "always" and "never"

The article I've found today in The Morning Brew is the great occasion to break the silence here on my blog. When I read an article that sounds like a definitive source of truth, I don't expect to read so subjective, single point of view arguments as I've found in Paweł Bejger's post "8 Most common mistakes C# developers make". I just couldn't resist to comment on it.

Paweł mixed some .NET Framework tips with performance advices and some architectural guidelines together. Let's briefly discuss his points.

1. String concatenation instead of StringBuilder

Advice here is to use StringBuilder as it doesn't allocate memory for each string part. But it adds its own overhead, it's often an overkill and actually may be a performance issue itself when concatenating few strings only. If StringBuilder was the only proper way to concatenate strings, the framework wouldn't expose such handy + operator for sure.

2. LINQ – ‘Where’ with ‘First’ instead of FirstOrDefault

Two different things were mentioned in this topic. The first one is that instead of chaining Where(x => sth) and First(), we should use First(x => sth) instead. Well, yes, but it's nothing more than one less method call. The number of elements actually fetched from the collection is equal to one in both cases, no performance difference. I would say this is up to personal preferences.

The second is that FirstOrDefault is better than First as it doesn't make an assumption that the collection is not empty. But I often do want to have that assumption. Otherwise, I need to be sure that my code handles null well. In many cases, when I'm sure that the collection is not empty, I just don't need that null handling code. And yes, First will throw for empty collections. I still find it better than some mighty NullReferenceException that may be thrown few lines later in case FirstOrDefault is used without proper attention.

3. Casting by means of ‘(T)’ instead of ‘as (T)’ when possibly not castable

Agreed. But again, not so big difference practically. NullReferenceException thrown unexpectedly later due to unproper handling of null returned by as operator is not a bit better than InvalidCastException thrown directly while casting. Anyway, the need for type casting itself can often indicate a code smell.

4. Not using mapping for rewriting properties

Paweł suggests here that we should use object-to-object mapping libraries like AutoMapper for rewriting properties from one object to another. Agreed, but only when we're doing A LOT of stuff like this, i.e. mapping between layers. When we just need to have several properties mapped, the cost of using the external library will never pay off. Moreover, in those cases it sounds like over-engineering and abuse of both KISS and YAGNI principles. And especially when some additional logic needs to be performed during the mapping, I would personally suggest being as explicit as possible, for readability and maintainability reasons.

5. Incorrect exceptions re-throwing

If we need to re-throw exceptions, we should be using throw alone to preserve the stack trace, agreed. Just a nitpick here - the example used in the article shows more harmful anti-pattern - catching general Exception itself. Wonder how we could ever handle OutOfMemoryException.

6. Not using ‘using’ for objects disposal

Well, can't disagree. Developers should know how to use the tools they were given.

7. Using ‘foreach’ instead of ‘for’ for anything else than collections

Don't disagree here, again, but I haven't really checked that, as for me this really sounds like a micro-optimization not needed in the vast majority of cases. And premature optimization, without known bottlenecks and metrics, even if not the root of all evil, is rather not a time well-spent.

8. Retrieving or saving data to DB in more than 1 call

Similiar to the one above, but on the different level. There's nothing wrong in reading data in two queries if our life becomes much easier then. Modern databases and ORMs offer features like multi-queries or batches (sending multiple commands to the database together to reduce round trips), so the optimization can be moved at infrastructural level, instead of unnecessarily complicating our query.

However, it's almost always wrong to have N+1 queries, but that's the separate story.

Conclusion

I do not negate that C# developers should know all the things mentioned in Paweł's article, but being aware of how things work, what are the pros and cons of each approach is something different than just blindly applying the rules like "don't do this, do that". I'm missing those pieces of advice in the original article.

There's no such thing as the single right answer for any question in our craft. There are always many options to consider and we can never be sure that something is always wrong or always right not knowing the whole context. Even classical design patterns proposed by the Gang of Four, always considered as the best practise we should not discussed with, were recently reasessed quite well by Ayende.

The answer to life, the universe and everything in programming is just always "it depends".