Great example of Hard-to-Test Code

Thursday, April 17, 2008

When we talk about designing for testability, it seems pretty abstract. We talk about "encapsulation", like everyone knows what it means. Here's an example of code i just stumbled into that demonstrates not being designed for testability and not being encapsulated Our guest of honor, the function: <cffunction name="getAllUserPlanDocs" hint="returns all plandocs user has permission to" returntype="query" output="false"> <cfset var plandocs=""> <cfset var safename=""> <cfquery name="planDocs" datasource="#dsn#"> select BunchOfStuff FROM planDocuments pd LEFT JOIN PlanDocGroups pdg on pdg.DocGroupID=pd.DocGroupID WHERE pd.PlanDocID IN (#session.user.getUserPlanDocList()#) ORDER BY pd.SortOrder,pd.PlanDocName </cfquery> <cfreturn planDocs> </cffunction> Now, bear in mind, this is probably 3 year old code. I haven't checked source control, but I very likely wrote this code. Who knows? Only source control, and I can't handle the truth right now. So, back then, we weren't doing any unit testing at all. In fact, this is the first time I've written a test for the component to which this function belongs, so I fired that up. <cfcomponent extends="some.parent.testcase"> <cffunction name="setUp"> <super.setUp()> </cffunction> <cffunction name="wizardPlanDocShouldNotBeVisibleUserPlanDoc"> <cfset q_pd= plan.getAllUserPlanDocs()> <cfset debug(q_pd)> </cffunction> </cfcomponent> (Note: I am not an advocate of using a generator to jam out stub tests for existing, legacy code. That's why this is the first test i'm writing for this particular component). All I'm doing here is writing a little test and dumping out the query because I want to see my data first. I just like that approach. I am fully expecting just to get a few-row cfquery dump. So, I go to run this function, and I get an error: "Incorrect syntax near )" WTF? So I open the file from the stack trace view in the plugin, and it takes me to this line in the query: WHERE pd.PlanDocID IN (#session.user.getUserPlanDocList()#) Son of a bitch. This, dear readers, is why people always say "don't use persistent scopes in your code". This violates encapsulation. Because now, the component under test needs to know about some other component in the session scope. Which, obviously, my simple little test doesn't have. So now, my simple little test has to grow, probably by 2x or more, because now i gotta get a session.user object set up and make sure it returns an appropriate list so that line of the query stops failing. But... I'm not going to get mad at myself for writing shitty code 3 years ago. It happens. Here's the lesson, which I'm writing to myself today to remind myself: When you design test-first, or design to make things easier to test, you'd probably not write code like this. Instead, you'd write the function such that it took the User's plandoc list, or maybe even took in a user object. This way, you inject the data you need into the function rather than have the function need to look outside the component for it (in this case, it's looking out into a session scope object). You might be thinking "but if you create the user object to inject in, then that's pretty much the same amount of work, isn't it?" Yes, yes it is. Right now, I'm not at liberty to change this code. Too many touchpoints and I'm not gonna go find them all. So I'm just going to have to make an uglier test case in order to test this functionalty. Coming to think of this, this is actually a good place where mocks are helpful. And I don't mean even using a mock framework or anything like that. I'm talking low-rent ghetto mocks you can use thanks to the glory of CF and mix-in methods. To wit: <cffunction name="wizardPlanDocShouldNotBeVisibleUserPlanDoc"> <!--- function under test uses user.getUserPlanDocList ---> <cfset session.user = createObject("component","")> <!--- I don't want to have to authenticate the user or go through all the rigamarole... so I'm mocking it just trump the getUserPlanDocList function with one I'm creating right inside this test case ---> <cfset session.user.getUserPlanDocList = getUserPlanDocList> <cfset q_pd = request.plan.getAllUserPlanDocs(71)> <cfset debug(q_pd)> </cffunction> <cffunction name="getUserPlanDocList" access="private" hint="I'll use this as a mix-in to mock the getUserPlanDocList function on the user object"> <cfreturn "1,2,3,4,5,6,7,8,9,10"> </cffunction> So here, I just created a new private function inside my testcase that returns a string. I then mix that function into the user object, thereby trumping the one that is in there for real. This way, I can create that static list I would've created anyway to test this code rather than worry about finding a user in the database whose set of permissions, etc will match the conditions I'm trying to recreate. Thank you, ColdFusion. Couldn't have done that in Java. (I know, I know... EZMock). So, my problem is largely solved. When I get the time, I'll get Mike Steele's CFEasyMock set up here at work and I'll swap out my little ghetto mock with a proper mock. Not because it'll change the behavior, but because the intentions will be clearer. It's much easier to udnerstand what I'm doing here if you see "user = CreateMock()......andReturn("1,2,3,4,5,6") " instead of this mix-in approach. There you have it. Testing unencapsulated legacy code, how to avoid such stuff, and how testing could help you avoid this kind of design problem in the first place. Enjoy.


Peter Bell said...

Great practical example of the whole "design for testability" idea. When I first skimmed the code (before you explained what was up) it took me a second to catch the session reference. I've also got code like this lying around, but if I'd started with tests I would have realized the problem upfront and would have come up with a better design.

Another good example is the old "getTimeToEndofDay()" method (or similar) that calls system.time (now() for the ColdFusion readers) to get the current time. Doesn't seem like much of a problem until you realize that yu're gonna have to stay up all night to test edge cases around midnight. Tests have to be repeatable, so you have to refactor to make the reference time a parameter of the method call.

Vive la TDD!

Marc Esher said...

You asked about the Koskela book, and he has a really good example of the Time problem you bring up! It'd seem a stupid thing to write an abstraction around Time, but he does a fine job of showing a) how to do it and b) why you'd want to do it, which is related directly to the problem you bring up.

Mike said...

Brother, I can't tell you how happy I am to not be looking at something called session.user.getUserPlanDocList().

Doesn't it suck when "git'er done" = "git'er wrong"? It sure is hard to rip those things out later, too.

HEY! I thought you were moving to subversion/tortoise. If you do that, you can use that handy "blame" feature to figure out who was responsible for which lines of code.

Marc Esher said...

Mike, do the letters F and U mean anything? just curious.

Now you see, Mike, I would never actively blame people in the manner you're describing. I like my team. I am not perfect, either. I probably wrote that code, but I'm not interested in blame at this point.

You, sir, are a jackass, even if you do buy me 20 dollar cohibas.

But still....

Marc Esher said...

oh, and by the way...

YOU wrote that code!

I just played my little Larry the cable guy "Git'r'done" key chain, in your honor.

orangepips said...

A great book about changing, and writing units test for, existing code is "Working Effectively with Legacy Code" by Michael C. Feathers. I think of particular interest for Coldfusion programmers are approaches for writing unit tests against code written in a procedural fashion.

Mike said...

lol, why do you think I mentioned it. I know I had a hand in there somewhere. I was all over that user object trying to get it out of some of those nested scopes at one point.

HA! Trying to pin that query on me? That REALLY doesn't look like my handiwork. More likely I was the first person to add it to source control.

I was even encouraging people to use the scribble pad as a poor-man's encapsulation test way back then.

Hmmm... let me count up. "F-U", "jackass", "disparaging key-chain played in my honor". I'm sensing a distinct lack of Cohiba respect ;). Plus, you know I'm not one to actually get things done.

Marc Esher said...

dude, you're killin' me!

i was just kidding. that ain't your code Son.

and by the way, i'm off of your ghetto cohibas and on to monte cristos.

i think i'm going to send you a little larry the cable guy keychain just so you can feel the love.