Tuesday, June 25, 2013

Static constructors and sealed classes - how to make your code untestable

In almost all programming languages there is a possibility to put some application logic in static constructor. However, it's usually up to the platform to decide when to execute the code we put there - usually it happens when we the environment runs a piece of code that refers that class.

Recently I was reading through the Windows Azure Training Kit (written by Microsoft) and this is a piece of code from the kit:

public class GuestBookDataSource{
...
static GuestBookDataSource()
{
storageAccount = CloudStorageAccount
    .Parse(CloudConfigurationManager.GetSetting("DataConnectionString"));
CloudTableClient cloudTableClient = storageAccount.CreateCloudTableClient();
CloudTable table = cloudTableClient.GetTableReference("GuestBookEntry");
table.CreateIfNotExists();
}
...
}
This code will open or create a cloud SQL table before accessing anything around it. As the code runs only once, it's seems like an efficient way to do the initialization of the environment.

There are multiple issues with this approach

- We don't know when the code is actually running. Depending where we refer GuestBookDataSource for the first time, the code would execute at a different stage in our application. As it will actually connect to another machine to open the table, it will suffer from some kind of delay. We don't know when though. Predictability is really important, so a simple Init() would be easier to understand and debug.

- A constructor -or in this case referencing to a class- really should not throw random network or file exceptions. This is not something we expect when we call var gds = new GuestBookDataSource(); This line should always execute instantly without any delay and without throwing exceptions.

- It's completely untestable. The implementation is tightly bound to CloudStorageAccount and we cannot mock it or inject a dummy implementation. In a unit test, it will call the actual cloud implementation it no matter what. Moreover, we cannot reset its state as a static constructor will be called exactly once in a process. This means that depending the order we run our unit tests, our tests may or may not fail.

So what would be a better solution?

Unfortunately the CloudStorageAccount is not implementing the interface, so we would need to wrap it in our custom interfaces. Something like this would work:

public interface IStorageAccount
{
public void Init(string connectionString);
public ICloudTable GetTable(string tableName);
}

public interface ICloudTable
{
public TableResult Execute(TableOperation operation,
    TableRequestOptions requestOptions = null,
    OperationContext operationContext = null);

// Add other methods you actually need.
}

public class StorageAccount : IStorageAccount
{
CloudStorageAccount storageAccount;

public void Init(string connectionString)
{
    storageAccount = CloudStorageAccount
        .Parse(CloudConfigurationManager.GetSetting(connectionString));
}

public ICloudTable GetTable(string tableName)
{
    CloudTableClient cloudTableClient = storageAccount.CreateCloudTableClient();
    CloudTable table = cloudTableClient.GetTableReference(tableName);
    table.CreateIfNotExists();
    return new Table(table);
}
}

public class Table : ICloudTable
{
private CloudTable table;
public Table(CloudTable table)
{
    this.table = table;
}
public TableResult Execute(TableOperation operation,
    TableRequestOptions requestOptions = null,
    OperationContext operationContext = null)
{
    return this.table.Execute(operation, requestOptions, operationContext);
}
}

public class GuestBookDataSource_Testable
{
private static IStorageAccount storageAccount;
private static ICloudTable table;

// Probably called with some kind of dependency injection framework.
public GuestBookDataSource_Testable(IStorageAccount storageAccount)
{
    if (GuestBookDataSource_Testable.storageAccount == null)
    {
 GuestBookDataSource_Testable.storageAccount = storageAccount;
    }
}

public void Init()
{
    if (GuestBookDataSource_Testable.table == null)
    {
 storageAccount.Init("DataConnectionString");
 GuestBookDataSource_Testable.table = storageAccount.GetTable("GuestBookEntry");
    }
}

public void UnInit()
{
    GuestBookDataSource_Testable.storageAccount = null;
    GuestBookDataSource_Testable.table = null;
}
}

But wait, this code is much longer!?

Yes, unfortunately it is. The main reason is that Microsoft decided not to program against interfaces and make all Cloud* classes sealed. It means that we cannot inherit from them to "mock" them nor can we replace them with their interfaces. The code above is long, because we had to create wrapper interfaces and implementations to fix what Microsoft did not want to. As a result, the above implementation is testable as we can pass in any dummy implementation for the constructor of GuestBookDataSource_Testable.

So if Microsoft doesn't do it, why should I?

There are pros and cons of course. The code gets longer which statistically means more bugs. On the other hand, you can unit test your code to make sure any changes in the code are not ruining other parts of the project. However, keep in mind that there are great pieces of software out there without any kind of automated testing. Unit testing is not required when you are writing code - it will help you to maintain a generally higher quality of code but in some cases there might be other, much more important factors. One perfect example is timing: if you don't ship the code by the end of next week your competition would crush you and you are out of business. Well, do whatever you need to ship, even if it means you don't unit test everything. Although, even in this case you really should not connect to another machine in a static constructor.

No comments:

Post a Comment