Posts Tagged code
Testing function vs testing implementation
Often I have got complaints from developers that I work with that their unit tests are prone to breakages, or they don’t like writing unit tests because their code changes frequently, which causes them to change their tests as well. Its just extra overhead at that point, and starts being a chore. Atleast thats what their claim is. Now of course, I don’t agree with this at all. Not. One. Bit.
You see, when I hear this, its always tells me that there is something wrong with the way tests are written. A unit test that requires changes every time someone changes the code implies that there is a extremely strong coupling between how the code is written to how its tested. Some useful indicators of such a thing could be having a getter methods or properties which are visible only for tests, but not to external code. Or Tests which check if a loop happened 6 times or a mock was called 17 times. Sure, these assert that the function is working as intended, but say you optimize and reduce the recursion or method calls, then you need to go and update your expectations.
Of course, some of this is unavoidable when you are working with classes that have mocks injected into them. But in such a case, unless it is plain delegation, there must be some logic that must be happening. That should be the target of your tests, not the mock delegations. Usually, when I work with mocks, I have a few tests to make sure the right methods are getting called, and only if there is logic, I test it further. Otherwise, 1 or 2 tests and then I go and test the implementation of the mocked class to make sure it works under all conditions.
So lets consider a run of the mill binary search method that would be tested with mocks (A little bit contrived, but bear with me on this) :
public int binarySearch(List<Integer> items, int itemToFind, int low, int high) {
// Do the needful, in a recursive fashion
}
// A Brittle test
public void testUsingMocks() {
List<Integer> list = mockery.mock(List.class);
mockery.checking(new Expectations() {{
oneOf(list).size(); will(returnValue(3));
oneOf(list).get(1); will(returnValue(6));
}});
assertEquals(1, binarySearch(list, 6, 0, 2));
}
Now, while a bit contrived, this is a familiar sight when mocks are used to test. Or it might happen that to check the correctness of the algorithm, the indices at which the split happens is stored in a list, and verified in the test. These are the kind of whitebox tests that make unit tests brittle. And the more of them there are, the harder it is to maintain or refactor code. Rather than testing it with for some use cases and boundary conditions, this is testing whether the algorithm itself is correct. Useful for some particular cases, but normally not required unless you are developing algorithm.
I would argue that its rare to write these kinds of tests if you write your tests before you write the methods. With a TDD, you just write your expectations, what you expect to give the method and what you expect out. You then write your code to get it to pass, and you might use internal variables or logic which the test really doesn’t care about. These tests are durable and hold up to refactorings, and even give you a nice safety net. There are times when these end up becoming integration tests rather than unit tests, but I still believe that they deliver more bang for the buck.
Of course, when you start testing edge cases, you do end up getting mostly a code dependent white box test, and those still are fine since they are supposed to be edge cases, which shouldn’t change that often. Though the fact that there are conditionals usually signifies that there is a polymorphic object hiding in there. But thats a blog post for another day.
Your code smells!!!
I was planning to talk about Dependency Injection or the difference between Mocks, Stubs and Fakes, but I think I would prefer to get this one out first. For those of you who haven’t heard the term before, a code smell is something that indicates there is something wrong with the source code, be they design problems or signs that the code needs refactoring. So in this post, I would like to mention a few common code smells, their identifying patterns and how to fix them. So without any further delay, lets start :
- Too many comments
The Problem : Lets start with the easiest to identify code smell. Comments are good, if they are describing a class or a method. But when you start having comments in your code explaining what a particular block of code does, you know you are in trouble.
The Fix : If there is a block of code which does some complicated stuff, and you feel you need to comment it to make it easily understood, pull it out into a well named method, and let the method name describe what it does.
. - Long Class / Method
The Problem : These two code smells actually are similar. If you have a method which goes beyond 10 – 15 lines, then you have a long method. While there is nothing technically wrong with long methods, its not as easy to comprehend as a nice small method, and can make maintenance a pain in the rear. Long classes also are similar, having too many things that its trying to do. If you generally try describing a class and have to use ands and ors, then you have a long class.
The Fix : Pull out parts of the long method into smaller well named methods. The advantage is two fold. One, your method is much more readable now. And two, you can now test individual methods you have pulled out, making testing a much easier task than one giant method. Same with classes, break it up into multiple smaller easily testable classes.
. - Primitive Obsession
The Problem : How many times have you had to write a class which takes in, say a phone number ? And how many times have you passed in a String or a long to the class which asks for the phone number ? If you raised your hand, then congrats, you have a code smell known as primitive obsession. This happens when instead of creating an object, you pass around primitives, and write functions to operate and convert that primitive from one form to another. So you might have to create utility classes which give you a phone number with brackets from a string, and so on and so forth.
The Fix : Just give the poor thing a class. If you operate on phone numbers, create a PhoneNumber class which has methods to operate on the number. Makes it easier for anyone using the class as well, and of course, its testable
.
. - Feature Envy / Inappropriate Intimacy
The Problem : Feature Envy is when a method on a class is more interested in other classes than the class to which it belongs. The reason for this could be as simple as the method being in the wrong class to something more non trivial. Inappropriate intimacy is when two classes are so tightly coupled that one depends on the other to work a particular way for it to work.
The Fix : For feature envy, just move the method to where it belongs. If it does work solely on some other class, then maybe it belongs on that class instead of where it is currently. For inappropriate intimacy, you need to figure out if the problem is something simple or something more complicated. It might be that the interfaces weren’t selected appropriately, or you might need to introduce a layer to keep the coupling loose, or even refactor the code to make sure it does not depend on another tightly coupled class.
. - Lazy class / Dead code / Duplicate code / Speculative Generality
The Problem : All of the above are usually simple code smells which indicate you have code which you don’t need. Lazy class is when a class doesn’t do enough to justify its existence. Dead code is obvious, code which is not used or dead. Duplicate code, duh, Its duplicated. Speculative generality is the most interesting of the lot. Its when you write some code for something which you don’t need yet, but may need at some point in the future.
The Fix : For the first three code smells, the fix is trivial. Delete it. Dont even think about it, just blindly delete it. Duplicate code is a pain to maintain as well, pull it out into a method and then delete the duplicates. Speculative generality is one thing people don’t realize they are doing or feel they need to do it now since otherwise, it might become difficult to do in the future. The interesting thing is that the feature they added speculatively is rarely ever used. Its an additional overhead to maintain and test for something you never use. Don’t do it. If you can implement it now, you can implement it when you need it. Just delete that darn code.
There are a lot more code smells than I could list out here, but these are a few of the most common ones you should keep a lookout for. Google search Code smells if you want to learn more about these insidious creations . Next time, before I start dependency injection, I think I will rant about things which make code untestable. So in a sense, Testability code smells.
Recent putbacks