Humblecoder

Caution, this blog may be ironically named

Don’t Be a Fool, Wrap Your Tool!

| Comments

As a hormone ravaged teenage, I squirmed uncomfortably as parents, teachers and community health practitioners imparted the words of wisdom “Don’t be a fool, wrap your tool”.  So it is fitting, I’m equally as squeamish when coming across the same advice as an adult.

What am I talking about?  Creating wrappers for anything at all on the boundaries of your code for the purpose of unit testing.  I’ve been struggling to think of a succinct way to explain this so I decided to go through a worked example.

Consider the following code:

   public class CommandReceiver
    {  
        public void WaitForMessage()
        {
            using (NamedPipeServerStream pipeServer = new NamedPipeServerStream("testpipe", PipeDirection.In))
            {  
                // Wait for a client to connect
                Trace.Write("Waiting for client connection...");
                pipeServer.WaitForConnection();  
                Trace.WriteLine("Client connected.");
                try
                {
                    // Read user input and send that to the client process.
                    using (StreamReader sr = new StreamReader(pipeServer))
                    {
                        String command = sr.ReadLine();
                        DispatchCommand(command);
                    }
                }
                catch (IOException e)
                {
                    Trace.WriteLine("ERROR: {0}", e.Message);
                }
            }
        }  
        private void DispatchCommand(String command)
        {
            //Knows how to deal with messages.
        }
   }

Looking at this code there is a number of problems but lets focus on the unit testing problems.  It is impossible to unit test this code because it creates a real pipe server and waits on a blocking call before continuing.  This means to test this code we need to create a pipe client, connect and all this would have to be threaded because of the blocking call.  Of course, this would make a good integration test because it tests that the pipe is connectable and receives a string message.

Before we set about making this code more unit test friendly, lets look at what we are trying to unit test.  We are trying to test that the WaitForMessage method can receive a string and pass it on.  For us to do this we need to abstract the pipe and stream out.  Also, while we are there lets remove the DispatchCommand method since it violates SRP and it would be more testable on its own.  So lets take a second stab at the code.

public interface INamedPipeServer : IDisposable
{
    void WaitForConnection();
}  
public class ManagedNamedPipeServer : INamedPipeServer
{
    private NamedPipeServerStream _pipeServer;  
    public ManagedNamedPipeServer(String name, PipeDirection pipeDir)
    {
        _pipeServer = new NamedPipeServerStream(name, pipeDir);
    }  
    public void WaitForConnection()
    {
        _pipeServer.WaitForConnection();
    }  
    public void Dispose()
    {
        _pipeServer.Dispose();
    }  
}  
public interface IStreamReader: IDisposable
{
    String ReadLine();
}  
public class ManagedStreamReader : StreamReader, IStreamReader
{
    public ManagedStreamReader(Stream stream) : base(stream)
    {}
}  
public class CommandReceiver : ICommandReceiver
{
    INamedPipeServer _NamedPipeServer;
    ICommandDispatcher _CommandDispatcher;  
    public CommandReceiver(INamedPipeServer pipeServer, ICommandDispatcher dispatch)
    {
        _NamedPipeServer = pipeServer;
        _CommandDispatcher = dispatch;
    }  
    protected virtual IStreamReader GetStreamReader()
    {
        //Code to create a stream reader from the pipe
    }  
    public void WaitForMessage()
    {
        // Wait for a client to connect
        Trace.Write("Waiting for client connection...");
        _NamedPipeServer.WaitForConnection();  
        Trace.WriteLine("Client connected.");
        try
        {
            // Read user input and send that to the client process.
            using (var sr = GetStreamReader())
            {
                String command = sr.ReadLine();
                _CommandDispatcher.DispatchCommand(command);
            }
        }
        catch (IOException e)
        {
            Trace.WriteLine("ERROR: {0}", e.Message);
        }
    }
}

This is where my uncomfortable squirm returns because my “keep-it-simple” sense is tingling.  I’ve just taken a fairly simple class that was around 30 lines and turned it into 75 lines of complicated OOP code.  I would hope that anyone reading this can follow it but in a real project this will probably be split over several files and the two styles of wrapping (because NamedPipeServerStream is sealed) can add significant cognitive burden to understanding what is going on.

What benefits does this bring?  It allows us to unit test that we read a string and pass it on to a dispatcher.  But I would go out on a limb and say that this is least likely of all the code in that class to go wrong.  The real problem area will be in connecting the pipe and reading from it.  We can make assumptions about failure conditions from the docs but as we all know docs != reality.

Is the abstraction here a benefit? In my opinion, not to the extreme level we have. The abstraction at ICommandReceiver will allow us to swap out how the application does IPC calls making it flexible in the future and, as I eluded to above, the unit test ‘coverageability’ is of lower value in this instance.

My point in all this rambling nonsense?  In an ideal world we would have both sets of tests and the unit tests would cover all the error conditions but in the real world we only have a finite and, usually, short amount of time with pressure from project managers and deadlines.  So we have to look at what will bring us the most value and focus on that.  I believe at application boundaries like this one we should focus our attention on writing integration tests because it will bring us more value in the long run.  I would not shun unit testing entirely and in this example the ICommandDisptacher and other supporting classes would have a full suite of unit tests.

As a side note, my final version of the code would be a mid way point between the two listings.

Comments