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 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?
- 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
- 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.
- 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.
Thanks for the great post. I learned the same lesson from “Working With Legacy Code” — each test, no matter how ugly, gives you another opportunity to improve the code. Without tests you’re stuck with the bad code, but once you start adding them then the code has started on the path to cleanliness :).
> “…I shouldnâ€™t let the imperfection of testing stop me from testing at all”
Your colleague has good advice :). Voltaire is quoted ad nauseum as writing something like “The perfect is the enemy of the good”, and I have to constantly remind myself of this while coding.