The "untestable" System
I once maintained a software system that had no automated tests. It also had no testers, test plan, or any kind of scripted tests of any sort.
When a new team (which I was part of) took over maintaining the system we quickly recognized the need for some amount of automated tests. So we decided to unit test the core functionality. After which we could then gradually refactor parts of the system.
I will write about the refactoring another time, but let me just say: it did not go over well.
We immediately ran into problems:
- The code was tightly coupled
- There was little to no abstraction, no inversion of control, or any kind of dependency handling
- The methods and variables were so poorly named, that it wa not only hard to figure out what the code was doing, the code was often doing something different (or opposite!) of what those names implied
- Methods were often hundreds of lines long
- What little documentation existed was outdated and no longer described the functionality of the system accurately
Let me give you one example of what we would run into. The following pseudocode illustrates how data access was handled:
UserHasPermission(User user, bool transferPerXml) {
string sqlQuery = "SELECT * FROM USERS u INNER JOIN USERPERMISSIONS up ON u.ID = up.USER_ID "
string whereClause = " WHERE TRUE "
bool canPrintReport = false
bool canEdit = false
if (user.Department == "Sales") {
whereClause += " AND u.DepartmentId = '101' "
canPrintReport = user.IsAdmin || user.CostCenter != 4020
}
if (user.Department == "Processing") {
whereClause += " AND u.DepartmentId = '104' "
}
if (user.UserId == 2813 || user.UserId == 2822) {
canEdit = LookUpEditPermission(sqlQuery, true)
}
/* a few hundred lines more of this */
whereClause = whereClause.Replace(" AND CostCenter = 2150 ", " AND (CostCenter = 3000 OR u.ShoeSize > 12) ")
/* a couple more replace statements for edge cases */
sqlQuery += whereClause
Permission permissions = Database.ExecuteSql(sqlQuery)
if (transferPerXml && permission.CanTransferXml) {
FileTransferService.TransferXmlFile(user, FileTransferService.CreateXmlFile(GetPathForUser(user), sqlQuery)
}
return CorrectPermissions(permissions, user, canPrintReport, canEdit)
}
I know what you are thinking: "This is bonkers! No way anybody would really write code like this!"
Well, I hate to disappoint your faith in your fellow engineers.
What we should have done
This was one of the most challenging pieces of code I have ever worked on. Even with years more of experience, I don't know if I would be able to tame it today. But here are some ideas what we could have tried:
- Get all code without external dependencies (database, API, file system, etc.) under test first
- Wrap all the sql string manipulation into classes and pass an instance of the class between methods
- Get rid of static references to database, file system, etc. and instead pass an object into every class and method that called those. The object could then have been replaced with a test double, allowing us to unit test the methods.
- Split up long methods as best as possible. This is something I tried to do at the time, but without tests in place I ended up introducing bugs and side effects. I still think it was the right idea and only failed due to sloppy execution and flawed refactoring tools.
Slowly chipping away at the parts that caused the most pain would have been the proper way to go about it.
What we did instead
After failing to introduce a data access layer and causing major outages, we faced too much backlash from management and customers. The decision was reached to abandon refactoring efforts and instead try to stop the bleeding and then replace the old system altogether.
This ended up being a very costly decision with no happy ending.
"Untestable"
Over time, I have heard the expression "This system is untestable" here and there from engineers I worked with.
Really, I think it is mostly a lack of experience and know-how in how to get the code under control.
I believe firmly that automated tests can be written for any software system. And it takes know-how, skill, experience, and time to do so.