Wednesday, July 14, 2010

Unit testing, how do I unit test a Singleton?

Well, as with our WebService client sample, this question is a misnomer. More often than not we are interested in unit testing consumers of a Singleton than unit testing a Singleton's functional set - and depending on implementation, this may not be as straightforward as we think.

Consider

// a fairly typical Data Access Layer implementation (DAL). have actually
// encountered this on-site [shudders].
public static class DatabaseConnectionFactory
{
    public static IDbConnection GetDatabaseConnection ()
    {
        // 1. get connection string from config
        string connectionString = null;

        // 2. create connection
        SqlConnection connection = null;
        connection = new SqlConnection (connectionString);
        connection.Open ();

        // 3. some other custom stuff
        // 4. return connection
        return connection;
    }

    public static IDbCommand GetDatabaseCommand (
        string commandString,
        IDbConnection connection)
    {
        // 1. set custom stuff like transaction and timeout
        // 2. return command
        return new SqlCommand (commandString, connection);
    }
}

// embedded DAL logic in business tier - anyone else vomit in their 
// mouth just a little? - however what is especially offensive is the
// direct calls to data tier via static class DatabaseConnectionFactory
public class AppointmentBusinessObject
{
    public const string CommandString_LoadById_OneParameter = 
@"SELECT * 
FROM Appointments 
WHERE AppointmentId = {0}";

    public void LoadById (long appointmentId)
    {
        // 1. create Sql command string,
        string commandString = 
            string.Format (
            CommandString_LoadById_OneParameter,
            appointmentId);

        try
        {
            // 2. get connection,
            IDbConnection connection = 
                DatabaseConnectionFactory.
                GetDatabaseConnection ();

            // 3. get command,
            IDbCommand command = 
                DatabaseConnectionFactory.
                GetDatabaseCommand (commandString, connection);

            // 4. execute command
            Reader reader = command.ExecuteReader ();

            // 5. read contents
        }
        finally
        {
            // 6. close connection,
            if (connection != null) 
            {
                connection.Close ();
            }
        }

    }

}

Now, there are many reasons why this is a poor design, not the least of which is that we are locked into a single connection and a single implementation. We are forced to duplicate this source if any of these parameters need to change - and believe me this has happened!

If this were not reason enough to contemplate a complete revision, our ability to unit test is also impaired. Specifically, we cannot unit test or verify LoadById without hitting a datastore! In fact, we cannot unit test any class or method that invokes LoadById without hitting a datastore.

So what can we do?

Well the first thing to do is identify the actual problem - and here it appears to be a tight coupling between our business and data tiers. If we were to abstract DatabaseConnectionFactory then the result would be a loosely-coupled and more flexible system. To this end, I would suggest introducing an interface (that defines DatabaseConnectionFactory's existing members) and consuming this wherever possible.

For example,

// a simple data store interface, exposes full functional set
// of existing DatabaseConnectionFactory
public interface IDatabaseConnectionFactory
{
    IDbConnection GetDatabaseConnection ();
    IDbCommand GetDatabaseCommand (
        string commandString, 
        IDbConnection connection);
}

// we want this to be as low-impact as possible, so new
// interface defines members that already exist. sadly, 
// static classes cannot implement interfaces directly, 
// and so DatabaseConnectionFactory must be made an instance 
// class. while this requires a little deft maneuvering, 
// this may be accomplished with *zero* impact to existing 
// consumers! yay!
//
// 1. remove static key word from class declaration, this makes
// class instance-based
public class DatabaseConnectionFactory : IDbConnectionFactory
{

    // 2. remove static key word from members, this simply
    // makes existing method implementations instance-based
    public IDbConnection GetDatabaseConnection () { ... }
    public IDbCommand GetDatabaseCommand (
        string commandString,
        IDbConnection connection) { ... }

    // 3. define *new* static members that delegate to
    // instance-based members, this ensures that existing 
    // consumers do not break
    public static IDbConnection GetDatabaseConnection ()
    {
        // instantiate new factory every call for clarity,
        // potential optimization in declaring a static
        // lazy-loaded instance member, and delegating
        // to that instead
        DatabaseConnectionFactory factory = 
            new DatabaseConnectionFactory ();
        return factory.GetDatabaseConnection ();
    }
    public static IDbCommand GetDatabaseCommand (
        string commandString,
        IDbConnection connection) 
    {
        DatabaseConnectionFactory factory = 
            new DatabaseConnectionFactory ();
        return factory.GetDatabaseCommand (commandString, connection);
    }
}

// embedded DAL logic in business tier - vomitting just a little less -
// less offensive now we reference implementation-independent datastore
// definition
public class AppointmentBusinessObject
{
    // still tightly coupled to Sql-compliant datastore
    // but manageable.
    public const string CommandString_LoadById_OneParameter = 
@"SELECT * 
FROM Appointments 
WHERE AppointmentId = {0}";

    // 1. declare new factory member. this is our *dependency*
    // it *must* be fulfilled for class to operate successfully
    private readonly IDbConnectionFactory _factory = null;

    // 2. expose new parameter constructor, permitting consumers
    // of *this* class to specify an appropriate datastore
    public AppointmentBusinessObject (IDbConnectionFactory factory)
    {
        _factory = factory;
    }

    // 3. expose new parameterless constructor, this preserves
    // existing consumers who may not be "up to speed" regarding
    // this new-fangled connection specification. we also preserve
    // previous operating expectations by defaulting to ... 
    // "default" connection factory, so at worst, we deliver
    // *EXACTLY* same behaviour as before
    public AppointmentBusinessObject ()
        : this (new DatabaseConnectionFactory ())
    {
    }

    // 4. consume!
    public void LoadById (long appointmentId)
    {
        // ...
        try
        {
            IDbConnection connection = 
                _factory.GetDatabaseConnection ();
            IDbCommand command = 
                _factory.GetDatabaseCommand (commandString, connection);
            // ...
        }
        finally
        {
            // ...
        }
    }

}

So, where is the payoff exactly? Well, for one if we now wish to change datastore implementation (say to a MySql, or Oracle, or some other Sql-compliant datastore) we implement a new class and may swap between the two when desired. We also gain the ability to load objects from two or more datastores at the same time!

As a pleasant side-effect, we are also able to unit test LoadById directly, and any consumers that permit datastore specification.

// test business logic without hitting datastore! yay!
[TestMethod]
public void Test_LoadById ()
{
    IDbConnectionFactory mockFactory = null;
    // instantiate mock with expectations
    AppointmentBusinessObject appointment = 
        new AppointmentBusinessObject (mockFactory);
    appointment.LoadById (1024);
    // verify results
}