Best and Worst Practises in Unit Testing

Ok, so I had been in the process of writing a blog to summarize my opinions on TDD etc, and this article popped up on my feed reader – it covers a good deal of the things I had wanted to discuss, and is summarized in a very well constructed way. So rather repeating everything, I’ll focus on a couple the issues discussed that I feel are important.

The point I’d like to emphasize in this article, that truly strikes a chord with me is:

 a suite of bad unit tests is immensely painful: it doesn’t prove anything clearly, and can severely inhibit your ability to refactor or alter your code in any way

 

In fact, I can’t emphasize this point enough. A suite of bad tests inhibit change. Brittle unit tests, like brittle code, need to be refactored/replaced. I think the points made under the section “Name your unit tests clearly and consistently” I think are crucial here. Just today for instance, I came across the following tests methods in a class named DutyAssignmentChecks:

SeverityLevels()

ViolationChecks()

WarningChecks()

TranslatedMessage()

 

I was only looking at the test code so get a feel for the current behavior of the system; after all, tests should be executable specifications that define the behavior of our code, what better way to document the system! Looking at this “unit test” code however did not help one bit. Maintenance is hard when you don’t know what you’re maintaining. If I break these tests when modifying the original code base, what value do these tests provide? If I’m changing the expected behavior of the system, then breaking a test may be valid, but how am I to know this is the case, if I can’t deduce what the expected behavior under test is! The article suggests using  the subject, the scenario, and the result in the test name to clarify the behavior under test. This pattern of specifying is the basis of what has evolved into the Behavior Driven Development technique coined by Dan North.  

 

The style of text naming I prefer is as follows:

     using Skynet.Core

      

     public class when_initializing_core_module : concerns

    {

       SkynetCoreModule _core;

       

       public void context()

       {

         //we’ll stub it…you know…just in case

        var skynetController = stub<ISkynetMasterController>();

        _core = new SkynetCoreModule(skynetController);

        _core.Initialize();

       }

      

       [Specification]

       public void it_should_not_become_self_aware()

       {

       _core.should_not_have_received_the_call(x => x.InitializeAutonomousExecutionMode());

       }

      

       [Specification]

       public void it_should_default_to_human_friendly_mode()

       {

         _core.AssessHumans().should_equal(RelationshipTypes.Friendly);

       }

      

       // more specifications under this same context

       // …

     }

I think this kind of style of testing greatly improves the clarity and purpose of both the test, and the therefore subject under test.

The only point the article suggests that I’d take with a small pinch of salt is in the section “Unit testing is not about finding bugs”. I think the author slightly underplays the effectiveness of unit tests detecting regression bugs when you’re making any changes to the existing code. He does mention that unit testing can be effective in finding bugs when refactoring,  which I completely agree with – it’s this factor that makes refactoring confidently possible. I’d also argue however, that it helps find bugs introduced when a developer is extending the code base without following the Open/Closed Principal. In this scenario, the developer is modifying and existing code base to add a new feature or function. Although this scenario is undesirable (see the link to O/C P for details), and I would rather see the developers trained in good OO practice, in my experience I would say this scenario is extremely common in teams without much experience in good OO development. Under these conditions, unit tests are still quiet valuable in finding regression bugs.

 

What do you think?

Implementing a Customer Search Service – Part 2

Continuing on from my previous post, let’s start off by coding up the unit tests based on the behaviour scenarios we laid out. To ease my transition towards the BDD-like way of writing tests, I’m using xUnit along with a really handy base class and observation attribute developed by Fredrik Kalseth (check out his blog post here to see how this all works). As a refresher, the scenario I’ll be testing first is:

WHEN searching for customers

GIVEN customers exist matching the search criteria

THEN records matching the criteria will be retrieved from the persistence store AND customer summaries are returned AND the search is successful

And the coded test looks like this:

public class When_Searching_For_Customers 

              : Given_Customers_Exist_Matching_Criteria
{
    private ISearchResult<ICustomerSummary> _result;

    protected override void Observe()
    {
        ICustomerSearchService service = 

           new CustomerSearchService(_repository, _customerSummaryMapper);
        _result = service.Find(_criteria);
    }

    [Observation]
    public void Records_Matching_criteria_are_requested_from_repository()
    {
        Assert.Equal(1, _repositoryInvocationCount);
    }

    [Observation]
    public void Customer_Summaries_are_returned()
    {
        Assert.NotNull(_result.Hits);
    }

    [Observation]
    public void The_Search_is_successful()
    {
        Assert.True(_result.IsSuccessful);
    }
}

Pretty simple stuff. Following the Arrange, Act, Assert pattern for authoring tests, the “arranging” of the test takes place in the base class (which I’ll cover shortly), the “act” takes place by creating and invoking the service,  and finally there is then an assertion for each observation that we are hoping to prove.

The base class that manages the set-up of the test context looks like this:

public abstract class Given_Customers_Exist_Matching_Criteria 

                       : Specification
{
    protected IRepository<Customer> _repository;
    protected ICustomerSummaryMapper _customerSummaryMapper;
    protected ISearchCriteria<Customer> _criteria;
    protected int _repositoryInvocationCount;

    protected override void InitializeContext()
    {
        _repository = GetMockRepository();
       _customerSummaryMapper = GetMockCustomerMapper();
    }

    private static ICustomerSummaryMapper GetMockCustomerMapper()
    {
        var mock = new Mock<ICustomerSummaryMapper>();
        mock.Expect(map => map.MapFrom(It.IsAny<IList<Customer>>()))
            .Returns(new List<ICustomerSummary>());
        return mock.Object;
    }

    private IRepository<Customer> GetMockRepository()
    {
        var mockRepository = new Mock<IRepository<Customer>>();
        var mockCustomers = GetMockCustomers();
        mockRepository.Expect(rep => rep.Find(_criteria))
            .Returns(mockCustomers)
            .Callback(() => _repositoryInvocationCount++);
        return mockRepository.Object;
    }

    private static IList<Customer> GetMockCustomers()
    {
        var mockCustomers = new Mock<IList<Customer>>();
        mockCustomers.Expect(customers => customers.Count).Returns(10);
        return mockCustomers.Object;
    }
}

This class simply creates mocks for the dependencies of the SUT, such that we can test the unit in isolation. The only little extra here is that the mock repository behaves more like a test spy, since it allows us to monitor the number of times that the Find() method has been invoked.

 

Implementing the Service

Since I want to be completely flexible with my search criteria, I want the contract of my service to take in an abstraction of the Customer search criteria, rather than specifying a finite set of inputs. I think the following contract describes this quite well:

ISearchResult<ICustomerSummary> Find(ISearchCriteria<Customer> criteria);

 

I don’t want any of my data retrieval responsibilities leaking into the application layer. This work should be delegated repository. I therefore want my service to pass-through the search parameters into a repository implementation. Rather than making the repository aware of the presentation layer object ISearchCriteria, it is better to use a common, shared abstraction. A suitable abstraction to use here would be to use the specification pattern for the search criteria, and passing this abstraction to the repository.

As discussed briefly in my previous posts (and in detail here), I believe my repository should behave like a collection of entities (rather than serving up a query in the form IQueryable<T>). I also know my repository can only return a reference to entities defined as the root of an Aggregate.

Taking all this into account, I can specify my repository interface as:

public interface IRepository<TAggregateRoot>
    where TAggregateRoot : class, IAggregateRoot
{
    /// <summary>
    /// Find entities by specification.
    /// </summary>
    IList<TAggregateRoot> Find(ISpecificationExpression<TAggregateRoot> criteria);
}

NB: It’s likely that my Repository Interface will later include the ability to save new and updated entities, however as per YAGNI, I won’t include this for the moment.

 

Since my repository can only return a reference to an Aggregate Root, the Customer aggregate root will need to be mapped to the required CustomerSummary presentation object. A suitable interface for this mapper object could be:

ICustomerSummary MapFrom(Customer customer);
IEnumerable<ICustomerSummary> MapFrom(IList<Customer> customers);

All that remains then is to create an implementation of the service based on these interfaces. This is quite simply:

public ISearchResult<ICustomerSummary> Find(ISearchCriteria<Customer> criteria)
{
    var customers = _repository.Find(criteria);

    if (customers == null || customers.Count == 0)
    {
        return new CustomerSearchResult
        {
            IsSuccessful = false,
            FailureReasons = new[] { "No Customers found." }
        };
    }

    var summary = _mapper.MapFrom(customers);

    return new CustomerSearchResult { IsSuccessful = true, Hits = summary };
}

 

All that’s left is to create simple implementations for the remaining objects (CustomerSearchResult, CustomerSummary CustomerSearchCriteria) and we’re ready to rock. Now the logic of the Application Service is created and the tests pass, we can create a repository implementation making use of the Entity Framework sample’s EFPocoAdapter project. I’ll cover this in my next post.

Pluggable Dependencies demo part 1

I’ve been itching to build a “modern” application using an Agile approach with TDD since being inspired by Rob Conery’s MVC Storefront application, so  I figured the small example I discussed in my last post would be a good starting point for me to take my first baby steps.

To briefly re-cap and pad out the initial requirement, the idea is to build a small app where a person (and their address details) can be stored. There will be a minimum requirement that a person must have a full name (forename and surname) and an address for that person to be “published” to a list of people on the system. Draft person records can be created with incomplete details, and stored until such a time when they can be completed and published.

Please keep in mind as you read this that this will be this first time I’ve *attempted to* use TDD in anger, so please feel free to confront me about any questionable decisions I make, and I’ll try and make time to adjust the app/discuss the issues further!

Right, without further ado I’ll begin!

The first unit test

In attempting to follow the TDD mantra, the first step is to code out my first test. I figure I’d start with trying to return a list of people (or Persons) and since I know from my last post I want program against an abstraction, I know this list will be of type IPerson. Since I also know I wish to filter this list, I want my return type to be IQueryable. So this leaves me with:

[TestMethod]
public void Repository_ShouldReturn_Persons_AsQueryable()
{
    IDataContext rep = new InMemoryRepository();
    var query = from persons in rep.Repository<IPerson>()
                select persons;

    Assert.IsInstanceOfType(query, typeof(IQueryable<IPerson>));
    Assert.IsTrue(query.Count() > 0);
}

Of course since the none of these objects exist yet, the code will not build. With a little help from Resharper it’s easy to create some empty interfaces for IDataContext and IPerson and a basic implementation of InMemoryRepository to enable this code to build. These interfaces and classes now sit in the test project, within the InMemoryRepositoryTests code file and are marked as internal. At this point, this doesn’t matter. There’ll be a time to sort this out later in the process.

Now at this point, I run my first test, and of course, it fails – my repository method throws a NotImplementedException. At this point I really want to cheat a little since I already have an implementation I can use for this method that I’d like to use, but I know I would probably be burnt at the stake for such a blatant abuse of TDD, so I grit my teeth and continue. The TDD process tells me I should be adding the simplest implementation possible to make the test pass, so I write:

public IQueryable<T> Repository<T>()
{
    IList<T> items = new List<T>();

    return items.AsQueryable();
}

…and it fails again. This time with the message "Assert.IsTrue failed." – of course, there is no data being returned. Duh! This is easy to fix, I know I need to return a list of IQueryable<Person> so I add the following lines just above the return statement:

IPerson person = new Person();
items.Add((T)person);

This time, I run my test, and watch it pass!

Now I’m allowed to refactor. This is once again where Resharper comes in really handy; a few Alt+Enters and I’ve separated my objects and interfaces into separate files. I can then move IPerson and Person into a separate project called business objects (for want of a better name) and move IDataContext and InMemoryRepository into their own project (aptly named DataAccess). A few more tweaks are needed (like changing the protection level of the classes from internal to public, and altering namespaces) and I hit my next point requiring consideration – since I am making reference to the person object in the InMemoryRepository class, there is a one-way project dependency from my DataAccess project, to my BusinessObjects project and it appears that the dependency is avoidable. Since I want to avoid this dependency, I refactor the implementation to allow items to be inserted to an internally maintained list of objects:

private readonly List<object> _inMemoryDataStore = new List<object>();

public IQueryable<T> Repository<T>()
{
    var query = from objects in _inMemoryDataStore
                where typeof(T).IsAssignableFrom(objects.GetType())
                select objects; 

    return query.Select(o => (T)o).AsQueryable();
}

public void Insert<T>(T item)
{
    _inMemoryDataStore.Add(item);
}

I then add the Insert definition to my IDataContext so I can initialise the test with some data. Now my code builds again, I can now run my test again watch it pass!

Phew! That’s all I’m going to cover in this post. In later posts I will try to cover off returning Address details related to a person, implementing repositories that persist data to a database, implementing a “draft” repository, and switching between the pluggable dependencies.

Program against an abstraction, not an implementation

I was asked by another developer today how I would go about enabling an application to record “temporary” incomplete data for a particular object (lets say, a person) where usually that object would require particular attributes to contain values, and perhaps pass some other validation (lets say, the person must have address details). Now my first thought was that I’d require different objects for the temporary person record, and the final, published person record, and that each could have different levels of validation applied. At this point, my mind wandered to a blog post I’d read somewhere before…

In the end, I answered his question by pointing him to the blog that I had read. This blog discusses the Repository Pattern and goes on to propose a super-flexible repository interface that allows different implementations of the repository to be swapped out – the primary goal being the ability to run unit tests in isolation (without hitting the database). The blog also emphasises that the power of an abstraction is it’s transparent plugability; the ability to swap in a “draft repository” is now made possible and changes made in draft can be persisted to a different storage location than the live data.

It’s not a big leap from here to see how different implementations of an IPerson interface could have differing validation logic depending on the context (draft or published). By programming against the abstraction of the repository interface, and against abstractions of the returned types, we can (with minimal effort) swap out different implementations to provide the required functionality. I’d quite like to have a go at implementing this technique myself to see what I can come up with. Perhaps in future posts I’ll explore this technique further.