Humblecoder

I’ve read many books and blogs that advocate only having one assert in a unit test and lots of people take that to mean literally assert statement.  I’ve always disagreed with taking it literally, I’ve always thought of it as one logical assert, as in you assert one concept at a time which could lead to multiple assert statements.

The main arguments against having more than one assert statement seems to be it’s not as readable and it’s sometimes difficult to understand what is failing because of it.  My normal response is to create my own asserts that accurately describe what the multiple asserts do and hide the real asserts in there.  For example:

   1: public void AssertIsValidClone(Customer oldCustomer, Customer actualCustomer)

   2: {

   3:     Assert.AreNotSame(oldCustomer, actualCustomer);

   4:     StringAssert.AreEqual(oldCustomer.Name, actualCustomer.Name);

   5:     StringAssert.AreEqual(oldCustomer.Address, actualCustomer.Address);

   6: }

   7:  

   8: [Test]

   9: public void Clone_ValidCustomer_ValuesAreTheSameReferenceIsDifferent()

  10: {

  11:     //Some setup

  12:  

  13:     var result = aCustomer.Clone();

  14:     

  15:     AssertIsValidClone(aCustomer, result);

  16:     

  17: }

Ok so this is a very contrived example but we can clearly see what the intent of the assert is rather than having several making it harder to understand.  But what this doesn’t do is address the second concern. IE anyone of those three asserts could fail, so we fix it then the next fails, etc.

Enter an nUnit plug in called OAPT, this allows you to have multi asserts that generate multi unit tests in the runner so you can see exactly what is failing.  I won’t warble on too much about the details because it’s all in the link But let’s just rewrite our unit test:

   1: [Test, ForEachAssert]

   2: public void Clone_ValidCustomer_CloneIsNewItemWithValidData()

   3: {

   4:     //some setup

   5:     

   6:     var newCustomer = originalCustomer.Clone();

   7:  

   8:     AssertOne.From(

   9:         () => Assert.AreNotSame(originalCustomer, newCustomer)

  10:         () => StringAssert.AreEqual(originalCustomer.Name, newCustomer.Name)

  11:         () => StringAssert.AreEqual(originalCustomer.Address, newCustomer.Address));

  12: }

Much more concise and it will run as three separate tests.  I still do have an issue with it though, each test uses the test name with an appended number. It would be nice if you could pass in some text for it to append.  But then again it is open source so maybe I could add that feature myself

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.

I’ve been spending a bit of time lately learning Python and Pythonista ways, although a lot of the stuff I’ve read is more about the how of dynamic languages not the why but that’s another discussion.  I find the best way to learn a new language is to pick a pet project and run with it.  As random as it may seem this fits in nicely with my new years resolution of losing weight.  So I started a Diet Tracker website using the Django framework.

In my continuing quest to use and learn more about Test Driven Development one of the first things I spent time diving into (after the basic syntax) was the built-in unit test framework in Django, the Python standard library has one too but the Django one is richer.  It paid off quite quickly.

One of the key parts of the site was the ability to track your changing weight.  The first version of the weight display page is sortable in ascending or descending order and can have a selectable number of entries.  Also, weights will be stored in the model as KG but displayed in stone and pounds, this will be made user configurable at a later date.  The exceedingly simple algorithm for this page is:

1. Accept a list of weights
2. Reverse the sort order if we need it to be newest first
3. Take X number of items
4. Create a new list of the display adapter class

I would write this in C# as something like:

public List<DisplayWeight> ConvertToDisplayWeightList(List<ModelWeight> weights, int numberForDisplay, bool newestFirst)
{
	if (newestFirst)
	{
		weights.Reverse();
	}

	List<DisplayWeight> returnList = new List<DisplayWeight>();
	weights.GetRange(0, numberForDisplay).ForEach( mw => returnList.Add(new DisplayWeight(mw)));

	return returnList;
}

This does look WTF-esq but it’s not that bad once you understand how Django’s ORM works.  The Python I wrote to do this was:

def get_display_weight_list_by_date(self, numberToLoad = 5, newest_first = True):
        currentWeightList = Weight.objects.all().order_by('date')

        if newest_first:
            currentWeightList.reverse()

        finalList =[]

        for w in self.currentWeightList[:numberToLoad]:
            finalList.append(DisplayWeight(w))

        return finalList

def testLoadWeights_5items_newest_first(self):
        "Checks we have 5 weights returned and the newest is first"
        loader = DisplayWeightLoader()
        loader.get_display_weight_list_by_date(numberToLoad=5, newest_first=True)

        self.assertEquals(len(loader.currentWeightList), 5)
        self.assertEquals(loader.currentWeightList[0].date, datetime.date(2010, 01, 19))

def testLoadWeights_10items_oldest_first(self):
        "Checks we have 10 weights returned and the last is first"
        loader = DisplayWeightLoader()
        loader.get_display_weight_list_by_date(numberToLoad=10, newest_first=False)

        self.assertEquals(len(loader.currentWeightList), 10)
        self.assertEquals(loader.currentWeightList[0].date, datetime.date(2010, 01, 9))

The function is not very pythonic and a bit WTFy, I’ve rewritten it since, but the second unit test failed.  Without going into where the test data is coming from or what is going on, the test is failing because the newest date is still first.  Why is it failing? Because I made an assumption.

The assumption?  That an instance method on an instance of a mutable object affects the state of that object.  I think it’s a fair assumption and it is the way that C# works.  But it turns out not to be the case in Python, the line at fault:

currentWeightList.reverse()

It should be

currentWeightList = currentWeightList.reverse()

There is a couple of very important lessons here:

• Never make assumptions: Yet more proof that assumptions make an ass out of you and me.
• Reading language and library documentation is extremely important:  I had skimmed over the tutorials and docs for the standard Python library and I thought I knew enough to get up and running.  I should really have gone back and read the documentation properly for the functions I’m using for the first time.

The most important lesson though:

Unit testing is not an advanced skill or topic, it is essential and part of the basics 

Unit testing is not part of the standard Python docs tutorial, it is at the end of the Django tutorial (to be fair it is mentioned briefly early on) and barely gets a mention on MSDN.  Implying that it’s not that important and it’s something you can leave to later.  I think that it is extremely important, whether you’re an experienced developer trying to get to grips with a new language or a beginner trying to get to grips with programming it is the single most important thing after learning the syntax.

At every level it helps you to think about what you’re actually trying to achieve, gives you instance feedback to your understanding of a feature, language or library and helps you to write small simple functions.