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;
var
  User: TUser;
  MostRecentSession: TSession;
  Action: TAction;
  ActionItems: TDBObjectList;
  I: Integer;
begin
  Result := nil;

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

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

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

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;
var
  Converter: TConverter;
begin
  Converter := TConverter.Create;
  Result := Converter.CanConvertToBetweenTypes(AFirstHW, ASecondHW);
  FreeAndNil(Converter);
end;

//...and in the original method:

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

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.