When retrofitting tests, should my test cases be designed around code behaviour (ie: what the code is actually doing) or user cases (ie: as if i’d written the code TDD to begin with)?

I’ve been investigating a bug in one of our products which has inherited code written over 10 years ago. The application is written in Delphi, and the one thing i’ve more recently learnt (and realised i’ve taken for granted) are development tools and paradigms we’re used to in .NET land, but scarcely exist in Delphi mainly as a combination of the differences between the languages/environments and the depleting community. Still, the fundamental programming challenges we face are similar, if not the same.

In this instance, I identified the bug to be incorrect logic in an IF statement, nested deep within a highly coupled, highly dependent, impossibly tricky to test procedure.

function TDatabaseModel.GetActionsForLastSessionByHardware(const AUserUID: string; AHardwareID: string): TDBObjectList;
  User: TUser;
  MostRecentSession: TSession;
  Action: TAction;
  ActionItems: TDBObjectList;
  I: Integer;
  Result := nil;

  User := DBAccess.GetUserAndSessionsFromDB(AUserUID) as TUser;

  if Assigned(User) then
    if User.Sessions.Count > 0 then
      MostRecentSession := User.Sessions.Items[0] as TSession;
      for I := 0 to MostRecentSession.Actions.Count - 1 do
        Action := MostRecentSession.Actions.Items[I] as TAction;

        if Action.T_HardwareID = AHardwareStringID then
          ActionItems := DBAccess.GetActionItemsAndAssociatedObjects(Action.UIDString);
          Result := ActionItems;

In a nutshell, this code is dependent on a data access object to query a SQL datastore and get the results as heavy business-objects (not light-weight containers). Once it finds the particular object it needs, it performs an IF comparison on one of the properties to check that particular values are equal. This condition is the one with the bug. Rather than check for equality on these values, there is some logic behind whether the two values are compatible, so straight equality is insufficient.

Having identified the bug, my objective is to expose the bug from a unit test and then provide a fix. The question was – what’s the *right* way to test this?

  1. I can mock out the data access classes and manually inject my mock in testing. In .NET this is insanely easy with Rhino Mocks (for instance). With Delphi, it’s quite painful because there being no runtime environment, means there is no automatic proxy generation. So i’d have to extract the interface by hand (no ReSharper, remember?), statically define my mock object and hard-code any expectations. Oh and there are some idiosyncrasies when working with interfaces in Delphi, because they’re not strictly first-class language citizens. There are means by which the pain could be reduced, but it’s a lot of work
  2. Write an end-to-end test which sets up test data, sets up a valid query and then gets real data from the database. I then need to write several assertions that under different combinations of data, the IF statement reacts appropriately. This is possibly the least likely option given that it is highly dependent on database querying and if getting a Mock to work is tough, then this is like banging my head against a brick wall. Pass.
  3. Create a protected method, refactor the IF logic into the protected, and expose it through a subclass created for testing purposes. This contains minimal impact and will allow me to directly test that the logic used to determine compatibility between the values. It doesn’t test that the original method actually CALLS my logic, but the fix is implied because the test still exposes the problem (and makes the code more expressive)

Having unconvincingly decided in my mind to go with option 3, my code was looking like this, and I was still quite uncomfortable about it because the test is now brittle and not refactor friendly.

function TDatabaseModel.HardwareIsCompatible(AFirstHW, ASecondHW: String): Boolean;
  Converter: TConverter;
  Converter := TConverter.Create;
  Result := Converter.CanConvertToBetweenTypes(AFirstHW, ASecondHW);

//...and in the original method:

if HardwareIsCompatible(Action.T_HardwareID, AHardwareStringID) then
    ActionItems := DBAccess.GetActionItemsAndAssociatedObjects(Action.UIDString);
    Result := ActionItems;

I talked to one of my colleagues about my situation and he had some good advice – that I shouldn’t let the imperfection of testing stop me from testing at all. The basic premise behind the very excellent book Working With Legacy Code by Michael Feathers is that when you have legacy codebase with no tests, the first thing is to do whatever you can to get tests in. Once the tests are there, you can refactor to make the code better and more testable, but without something there to begin with, you’re only making it worse.

So that’s what I did. I put the test in, fixed the bug and life goes on. Moral of the story – When retrofitting tests do whatever is necessary as a first measure. However, i’m interested now to improve the mock generation features in Delphi….Will have to look into this at some point.

I’ve long been noticing the following errors come up at the end of our NUnit testrun, but never had a chance to look into it in detail until now…They’ve never resulted in a broken build, and they do clearly look like something happening outside the scope of our unit test (even if it was caused by our test)

Unhandled exceptions:
1) : System.InvalidOperationException: This action is invalid when the mock object is in verified state.
at Rhino.Mocks.Impl.VerifiedMockState.MethodCall(IInvocation invocation, MethodInfo method, Object[] args)
at Rhino.Mocks.MockRepository.MethodCall(IInvocation invocation, Object proxy, MethodInfo method, Object[] args)
at Rhino.Mocks.Impl.RhinoInterceptor.Intercept(IInvocation invocation, Object[] args)
at ProxyInterfaceSystemSystemObject_System_DataIDataReader_Rhino_Mocks_InterfacesIMockedObject_System_Runtime_SerializationISerializable.Close()
at ExternalDataComponent.Data.DataRecordEnumerator.Dispose()
at ExternalDataComponent.Data.DataRecordEnumerator.Finalize()

Any obvious names have been masked to protect the innocent/stupid.

The error message quite is quite obvious in what the problem is, and leaves little doubt as to where it’s originating from….But the difficulty in this problem is finding out what causes the exception in the first place.

After attaching the VS debugger to NUnit, and turning on “Break On Exception” for the System.InvalidOperationException exception, All i would get is a debugger breaking into a location which was not relevant to any executing code, and the entire call stack consisted of non-managed code.

After a lot of thought, and with a little luck, i realised that what’s happening is that some of our tests are creating a stub object and passing it into the ExternalDataComponent during testing. The ExternalDataComponent is assuming that it is safe to call dispose on the object and thus doing so. The Rhino.Mocks framework then throws an exception when it intercepts the Dispose() method of the mocked interface, because the mock object “is in [the] verified state”.

My GoogleFu is in good form.

Unfortunately, we aren’t able to make changes to the ExternalDataComponent i’ve so eloquently disguised, as it’s a dependency library we’re using, so the “solution” is to reset all mocks to the original Record state before calling dispose (which additionally the tests weren’t doing).

All i can start with at this stage is wow…..

I went through by feeds this morning and innocently picked up an article introducing mocking using Moq. I’ve read a little about it, followed the fact that it’s gathered quite a following, and figured it best I give it a spin to see for myself.

And then I followed the links in that article….
And then I read the links in that article…..
And i was shocked to learn about the bad blood between the very two opposing camps, which i’d never realised….

Incidentally, I started off trying to learn more about Moq, and now i’m reading up about TypeMock Isloator…all in a day’s work i suppose 😉