Lasse Koskela recently wrote Test Everything, But Not Private Methods on his blog. Thank you for writing this, Lasse. This is one of the most intelligent, albeit still wrong, posts I’ve read on the topic. Lasse specifically addresses one of my greatest concerns, which is simply that “if it can possibly break, it should be tested.”
It turns out that on the face, we are in agreement: you should not leave complicated code untested. Lasse’s answer (which is repeated by several folks, including Michael Feathers and Mike Bria) is simply to not write complicated private methods. If you have a complicated private method, you should make it public and test it. If it doesn’t belong on the public interface of the class, then move it to another class or create a new one where it can be public. As Michael Feathers puts it in his book Working With Legacy Code: “the real answer is that if you have the urge to test a private method, the method shouldn’t be private; if making the method public bothers you, chances are, it is because it is part of a separate responsibility: it should be on another class.”
I’m a huge fan of Michael Feathers, bordering on Fanboy lunacy, so I’m a bit hesitant to disagree with him. Also, to be fair, I agree with Lasse one hundred percent–about 90% of the time. However, Mike and Lasse present “extract method to class” as a panacea when it is in itself merely a tradeoff with serious consequences of its own.
Extracting a class dilutes and adds complexity to your namespace. Code should struggle very hard to earn a spot at this level. I spend a lot of mental energy on a project trying to understand the class hierarchy, so much so that it is worth my effort to spent time trying to keep it well-organized to assist this understanding. A method that was meant to be private and to serve a class at a specific point in time suddenly becomes a public object. Care must be taken to either make the new class generic enough and provably safe to be used by arbitrary clients, or to prevent other classes from accessing the class at all. If you find yourself trying to figure out how to control access to the class, chances are the code you are trying to extract really should be private after all.
If you are going to extract the private method to a class, considerable care should be taken that it does not, in fact, still fall under the responsibility of the class that contained it. If your new class ends up with an “-er” name, especially “-Manager” or “-Controller”, it’s probably just a Functor–a method call masquerading as an object. If the moved method has many side effects, especially ones that modify instance variables, the moved method will end up taking many arguments and returning many values that the original class must then use to modify itself. You may just end up passing in the original class to the method. Now what you’ve done is replaced the “this” keyword with a variable named “that”. This is a code smell; it’s called Feature Envy, and the solution is to move the method into the envied class. If the new class has the original class’s name in its own, you’ve all but conceded that the new class is useless in isolation from its progenitor; all you’ve really accomplished in this case is cluttering up the object namespace in order to achieve one outcome: making the private method testable. Lasse eschews inelegant workarounds like reflection nonsense or package hackery, but complicating the object map is just as inelegant if it achieves nothing more than making a private method public.
Another counterargument is–and though this is a situation that I wish did not actually exist, every developer has in fact faced it–extract class is not a free refactoring, and on large legacy projects it may be prohibitively expensive. Suddenly “change the design” becomes “they shouldn’t have designed it that way in the first place” and the only solution is to invent a time machine, go back in time, and not have done it wrong. This isn’t practical.
Sometimes you have a method that is, and should be, both private and complex enough to require testing. When that happens, you should test it.
I agree with Lasse and Mike about the problems inherent with testing private methods; I just don’t think their proposed solution is always workable.
This post is getting long, so I’m going to breaak it into to two separate entries. Next up: I want to present some cases when I think I should, even using TDD, design and write private methods that should be tested. I’m also going to address what I think is the primary concern of anti-private-testers, that testing private methods is testing implementation. (Teaser: I completely agree–but I hope to show you why, in some cases, that’s not a problem or at least an acceptable tradeoff.)
Why you shouldn’t test private methods? I have simple example for this. I have a public method in my class. It takes some arguments, then performs some action and returns result. Under the hood many private methods are called so the job can be done. This public method is tested with unit tests in a way that I supply the arguments and expect certain results. I don’t care what happens under the hood.
Now a change in requirements came in and some additional work needs to be done during this method call. This forces changes in some of the private methods (especially in their arguments). If I had had multiple tests for each of these private methods then introduction of this change would have forced me to change like 20 tests because otherwise they wouldn’t even compile. By testing only public methods I don’t need to alter any of my tests and they will still work fine. And if they still pass, I know the change didn’t break expected behavior. I just need to add some additional tests to check if added behavior works correct.
That’s huge difference in productivity in my opinion.
Thanks for this! So, this is going to sound weird, but while I haven’t changed my stance on testing private methods, I actually agree with you completely.
In the time since I wrote this, I’ve found two distinct types of testing behaviors. One is writing “guiding tests”, where you use small, TDD, micro-tests to inch forward carefully while writing complicated code. When I’m writing a tricky private method, I find guiding tests to be invaluable.
But the other type of test behavior is writing “regression tests” where you are documenting and asserting some externally visible behavior of the system. Nowadays when I write a private method it’s almost always to hide some volatile implementation that supports other public behavior. The fact that the code is volatile means that the code is subject to frequent change–and any tests still attached to it will also be affected by those changes. It absolutely incurs the maintenance cost you describe.
As a result of discovering this distinction, I let myself test private methods as much as I want when I’m trying to nail down new behavior. Once it’s nailed down, however, and I have a well-tested public interface around that private code, I commit the ultimate developer heresy: I delete my tests.
Doing this gives me the advantage of working with a safety net when writing new, complicated code, but without tying me down to the maintenance hassle of duplication by testing implementation-specific code.
I still check my private methods to see if they’re emitting design smells and need to be extracted and/or made public, and if so then I will keep the tests (because public behavior should be documented and asserted by a good test suite), but yeah, nowadays I use private to mean “subject to change” and that means “regression tests will have low value”–and that means I delete them. Crazy, right? 🙂
Maybe crazy but sounds reasonable to me ;). Worth trying. Thanks!
The debate about testing private methods has always seemed very strange to me. I think it’s a form of “do you walk to work or take your lunch?”.
I see no fundamental correlation between the need to test a method, and it’s visibility or scope. Why should they have ANYTHING to do with each other? Just because Java (for example) only has 4 scopes (public, protected, default/package, and private), doesn’t mean that this should somehow drive what we test and don’t test. It would be like deciding which car to drive based on what I’m wearing.
Now, *occasionally*, if I find I’m testing a private method, it *may* be a hint that perhaps I need to break it up somehow. Or make it public. Or something. But it’s just a hint, although admittedly one I should listen to.
Me, I test as much as possible. If the method really doesn’t belong in the public interface, I’ll typically make it (in Java) default/package scope, and put in a leading comment like /*TestScope*/ just to make it clear to other readers why I didn’t make it private. Yeah, it’s not perfect, but it’s as perfect as the language will let me.
(For a more whimsical take, see the Junit/Green Lantern Oath on my blog.)