Practical Testing: 6 - Tests refactored

Previously on Practical Testing: The last entry ended with us having two tests, both of which were in need to a good refactoring. The second test had uncovered an unexpected bug… This time around we’ll refactor the tests, fix the bug and finally write the test for the tick count wrap bug…

Our new test breaks because the CCallbackTimer is failing to keep the timers in the correct order in its queue. We have a test that adds the following timers; 1(100), 2(200), 3(150) and 4(150) to the queue. All timers are added at the same instant (ah, the wonders of controlling time…). The timer queue should end up looking like this: 1, 3, 4, 2. But instead it actually seems to look like this: 1, 2, 3, 4. The bug is in this piece of code; and it’s fairly obvious what’s wrong:

   Node *pNode= m_pendingList.Head();
  
   while (pNode && pNode->m_millisecondTimeout < pNewNode->m_millisecondTimeout && m_pendingList.Next(pNode))
   {
      pNode = m_pendingList.Next(pNode);
   }
  
   if (pNode)
   {
      m_pendingList.InsertAfter(pNode, pNewNode);         
   }
   else
   {
      m_pendingList.PushNode(pNewNode);
   }

To make sure we know what’s going on we’re going to change the test ever so slightly before we fix the problem. We’ll add two more timers to the queue so that our timers are: 1(200), 2(100), 3(300), 4(150), 5(150) and 6(160). The queue should then look like this; 2, 1, 4, 5, 6, 3, 1 but it wont… We’ll also add a separate sequence of two timers to the end of the test: 1(100) and 2(200). These new numbers exercise the edge cases a little better than the original test. We run the test and it still fails, which is good.

So, the problem is that we scan along a sorted list of nodes looking for one that has a timeout value larger than ours. When we find it we currently insert ourselves after the node that has the bigger timeout rather than before; doh!

Fixed code is something like:

   Node *pPrev = 0;
  
   Node *pNode = m_pendingList.Head();
  
   while (pNode && pNode->m_millisecondTimeout < pNewNode->m_millisecondTimeout)
   {
      pPrev = pNode;
  
      pNode = m_pendingList.Next(pNode);
   }
  
   if (pPrev)
   {
      m_pendingList.InsertAfter(pPrev, pNewNode);         
   }
   else
   {
      m_pendingList.PushNode(pNewNode);
   }

And the test passes. Time to refactor, not the code, not yet, the tests.

Our tests both require us to wire up a CMockTickCountProvider to a CCallbackTimer and then hook up a helper object to deal with exceptions, and then call a bunch of functions to get everything set up correctly; screw that. With the pieces we’re currently using for the test we can compose a new object; CTestCallbackTimer that does all of this set up for us. Likewise, having to wire our Callback to our Handle each time is a pain, and doubles the lines of code required; a new object can be built from the parts we have and results in a single object that does both jobs; just for testing. Both these new objects can live in the mock library as they’re used for testing objects in the main library.

Every time we set a timer we have the same 4 lines of code; we can move these to a static function in the test, or, better yet, move them to a helper function in the new CTestCallbackTimer object. It’s the same with setting the tick count, hoist the duplicate code into a function on the new object.

Testing for timer expiry or non-expiry is a two part process that we repeat a lot; move it into the new CLoggingCallbackTimerHandle object and everything becomes a little neater.

Our original test now becomes this:

void CCallbackTimerTest::TestTimer()
{
   const _tstring functionName = _T("CCallbackTimerTest::TestTimer");
  
   Output(functionName + _T(" - start"));
  
   CTestCallbackTimer timer(1000, s_delay);
  
   CLoggingCallbackTimerHandle handle;
  
   timer.SetTimerAndWait(handle, 100, 1);
  
   // Prove that time is standing still
   THROW_ON_FAILURE(functionName, false == handle.WaitForTimer(0));
  
   handle.CheckResult(_T("|"));
  
   timer.SetTickCountAndWait(1100, false);
  
   THROW_ON_FAILURE(functionName, true == handle.WaitForTimer(s_delay));
  
   handle.CheckResult(_T("|OnTimer: 1|"));
  
   Output(functionName + _T(" - stop"));
}

and the other tests shrink in a similar fashion.

Whilst I was making this change I noticed another potential problem; the code that sets timers eventually manipulates the timer queue; it uses a critical section to protect this data structure from concurrent access. Unfortunately the queue is also accessed from the timer management thread; and this access isn’t protected. I can understand how this came to be, in the one project where we use this class we set and reset timers from multiple threads as socket operations complete. The need to lock these threads out was obvious, the need to lock out the timer management thread was less so… At this point I could, some would say, should, write a test to prove that the code is broken; however, the code is so obviously broken here and structuring a test to reliably produce a failure due to this issue and then reliably prove that the problem is fixed is such a complex undertaking that I tend to be pragmatic in situations like this. Fix the code and move on.

Now we can add some more tests. A test that we can cancel timers and, at last, our test for GetTickCount() wrap. As expected, our test for tick count wrap fails. We’ll look at the changes required to make it pass in the next posting. Code is here. Same rules as before.