Thursday, April 17, 2008 Posted by Marc Esher at 12:58 PM
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","com.argus.framework.retail.business.User")> <!--- 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.