Agile Zone is brought to you in partnership with:

David Green is a developer and aspiring software craftsman. He has been programming for 20 years but only getting paid to do it for the last 10; in that time he has worked for a variety of companies from small start-ups to global enterprises. David co-founded the London Software Craftsmanship Community (http://www.londonswcraft.com/) - a group of professional programmers who meet regularly to exchange ideas and improve their craft. David is a DZone MVB and is not an employee of DZone and has posted 25 posts at DZone. You can read more from them at their website. View Full User Profile

Why shouldn't I test private methods?

03.07.2014
| 13581 views |
  • submit to reddit

Newcomers to TDD ask some interesting questions, here’s one I was asked recently: testing private methods is bad, but why?

How did we get here?

If you’re trying to test private methods, you’re doing something wrong. You can’t get to TDD nirvana from here, you’re gonna have to go back.

It all started with an innocuous little class with an innocuous little method. It did one little job, had a nice little unit test to verify it did its thing correctly. All was right with the world. Then, I had to add an extra little piece of logic. I wrote a test for it, changed the class until the test passed. Happy place. Then I started refactoring. I realised my little method, with its handful of test cases was getting quite complicated, so I used the extract method refactoring and boom! I have a private method.

While simple when I extracted it, another couple of corner cases and this private method evolves into quite a complicated piece of code – which now I’m testing one level removed: I’m testing the overall functionality of my outer method, which indirectly tests the behaviour of the private method. At some point I’m going to hit a corner case that’s quite awkward to test from the outside, it’d be so much easier if I could just test the private method directly.

What not to do

Don’t use a test framework that let’s you test private methods. Good God, no. For the love of all that is right with the world step away from the keyboard.

What to do instead

This is a prime example of your tests speaking to you. They’re basically shouting at you. But what are they saying?

Your design stinks!

If you need to test a private method – what you’re doing wrong is design. Almost certainly, what you’ve identified in your private method is a whole new responsibility. If you think about it carefully, it probably isn’t anything to do with what your original class is. Although your original class might need renaming to make that obvious. That’s ok, too. That’s incremental design at work.

An example would help about now

Say I started off with a Portfolio class – it has a bunch of Assets in it, each of which has a Value. So I can implement a Portfolio.GetValue() to tell me how much it’s all worth. But then I start dealing with weird corner cases like opening or closing prices. And what do I mean by value, what I could sell it for, right now? Or perhaps there’s some foreign currency conversion to do, or penalty clauses for early exit, how does all that get factored in?

Before too long, GetValue() has a fairly large piece of logic, which I extract into GetSpotSalePrice(Asset). This method is then hairy enough to need testing, so it’s pretty clear that my design stinks. The deafening din of my tests, finally persuades me to extract GetSpotSalePrice(Asset) into another class, but here’s the million dollar question: which?

What not to do – part 2

For the love of SOLID, don’t put it in a AssetSalePriceCalculator, or a SalePriceManager. This is the number one easy mistake to make – you can follow TDD and ignore decent OO design and still end up with a steaming turd pile of horribleness.

NounVerber class is always a design smell. Just stop doing it. Now. I mean it. I’m watching you. I will hunt you down and feed you to the ogre of AbstractSingletonProxyFactoryBean.

What should I do then?

The answer might seem obvious, but to too many people starting out doing TDD and half-decent design – it isn’t at all obvious. The method needs to move to a class where that responsibility makes sense. In our example, it’s crushingly obvious this is really a method on Asset – it even passes one in. If your method has one class parameter and uses a bunch of data from that class, you can bet your bottom dollar you’re suffering feature envy. Sort your life out, apply the method move refactoring. Go live happily ever after.

Summary

Why shouldn’t you test private methods? Because the fact you’re asking the question means the method shouldn’t be private – it’s a piece of independent behaviour that warrants testing. The hard choice, the design decision, is where you stick it.

Published at DZone with permission of David Green, author and DZone MVB. (source)

(Note: Opinions expressed in this article and its replies are the opinions of their respective authors and not those of DZone, Inc.)

Comments

Barry Smith replied on Fri, 2014/03/07 - 4:28am


Frankly, I have to disagree with this sort of TDD fundamentalism. We're software engineers, and the essence of engineering is being able to find a balance between competing pressures in our designs. We all know that software costs more to maintain than to develop and we need to consider the readability and maintainability of our code. That is rarely enhanced by replacing every ten line private method with a PrivateMethodInterface and PrivateMethodImplementaion framework. A test class is inherently tied to its subject and I don't really have any problem with this one exception of allowing intimate knowledge of the inner workings of another class. Basically, I consider a test class to be part of the target class code that just happens to be stripped out at runtime.

Oh, and a quick thought on the prejudice against noun-verber classes - the Spring Dependency Injection framework has around a thousand of them and I think most people would agree is one of the greatest pieces of Java software out there. Beware adhering too rigidly to your principles.

Peter Verhas replied on Tue, 2014/03/11 - 12:49pm

I agree with the statement to move those private methods to a separate class if their function makes more sense in a separate class. The fact that they are `private` and you still feel that some testing is in place is a smell. However never ever move a method to a separate class just because you want to test it and because you do not want to test private methods. That is the symptom, but the illness.

I also believe that in case there is no need to move the method to its own class, and you still feel need to test it, your feel is not correct. If the method truly belongs to the actual class and is private by its role and nature: why to test it on unit test level? Perhaps you want to introduce a new sub-unit test level? But then again: what is the unit? A class? A package? A method? Whenever I met a need to test private methods it turned out that it was a private by mistake, or we came to the conclusion that testing it separately does not add value to the test.

Finally, as for the Noun-Verber names my opinion would be totally irrelevant since I am not native English speaker. But I do read such comments with open ears and I am entertained.

Michael Geiser replied on Wed, 2014/03/12 - 7:15am


I think maybe in an effort to be clever, David is jumping the gun and over generalizing that if he were applying one of the basic tenants of our craft, reviews, he may have released a different product here...

In MOST cases, David is right, you might have a design issue; here is counterpoint:  I have a Interface defined for a use case that may have 1 to n implementations.  Because I believe in TDD and Design for Six Sigma, I have a private method in the interface for input validation.  The passed parameters are a few simple complex objects (like an Execution Context), but each implementation has some unique requirements.  If the input is bad, we should fail fast and throw specific and detailed exceptions.

This private validation method in each implementation class certainly should have test written against; to suggest otherwise is clearly wrong. 

There are other cases where David's point fails and while I appreciate the intended light hearted vein of the article, but your DoD for writing an article needs to not skip the review step.

Also, why is Thoughtworks allowed to advertise Mingle for free here?  I appreciate their contribution, but Atlassian is paying for the banner ad on this page; Thoughtworks' contribution does not earn them free advertising for their for sale products.  If I like their articles (oops..missed here too) I may hire them, but schilling of Mingle in this forum is unwelcome by me

 

Peter Verhas replied on Wed, 2014/03/12 - 9:09am in response to: Michael Geiser

If I neglected the fact that there are not private methods in an interface you would be right. Unfortunately I can not ignore facts.

One could argue that this is a shortage of Java. The fact is that this is not. Private methods have nothing to do in an interface. Private methods are private because outside world has nothing to do with them. That is exactly what the word "private" means. Full stop.

Unit test is outside world.

Interface is the contract the implementation follows when cooperating with outside world.

It is as simple as that.

If you test a private method you do not do unit testing. You are below the radar, under unit test level. Expect to hit some mountains.

Barry Smith replied on Wed, 2014/03/12 - 4:29pm in response to: Peter Verhas

Again, I feel unit tests fall in a slightly grey area. Personally, if I was designing something like the Java language now I'd be tempted to do so in such a way that the code for a class could be split across multiple files with the intention that the test code was actually part of the target class, but kept separate. I feel the debates about whether we should or shouldn't test private methods actually indicate a 'smell' at the language level. Java was designed before TDD was really mainstream and maybe these debates indicate there's really a need to provide language level support for it.

In terms of whether you should test private methods, I think having good, automated, testing for all your code is so much more important that maintaining some idea of fundamentalist code purity that it's a no-brainer to test private methods if getting good coverage without doing so is impractical.


Scott M Gardner replied on Fri, 2014/03/14 - 6:40pm in response to: Barry Smith

 Having principles gives a foundation for making good and consistent decisions. And basic architectural principles should almost never be violated.  They ARE fundamental, and should be followed with appropriate rigidity.  Most of the time I hear the argument that "this is a special case" or "we need balance" is that someone wants an excuse to do something wrong, or that there is an argument that is weak, and the only way it is even being considered is to create a false balance.

All other things being equal, readability is lost when you violate the standards. We have coding standards and architectural principles partially to support readability, because diverging from what is expected requires more cognitive effort to read.

Not every support class needs to be fully public nor does everyone require an interface to define the type. Often when I am refactoring and pulling functionality away from private methods, I create a package default visibility class to support the public classes in the package. This still can have unit tests in the package, but it does not require full visibility in the public interface of the package.  And enabling testability is a first class reason to factor the code this way. 

Finally, not everyone is enamored of Spring.  It has encouraged one of the greatest anti-practices in Java programing: Java beans getters and setters. They violate abstraction and information hiding, and leads to massively coupled code.

Barry Smith replied on Sat, 2014/03/15 - 5:37am in response to: Scott M Gardner

"All other things being equal, readability is lost when you violate the standards."

Sorry, I fail to see how taking a private method used solely by one class and moving it to a package-private class of its own enhances readability. It's more likely to confuse other programmers who expect it to be used across the package. I'd rather put any ugliness into test classes and keep it out of the production code.

As I say, I think the real problem is that test classes are a special case which were not considered when Java was designed and if it were being designed today there would probably be a mechanism to give them private access to their target class (well, they have it in setAccessible() anyway, but a more structural solution would be welcome).


Raging Infernoz replied on Sat, 2014/03/15 - 11:54am

There is an much easier way to solve this, just make the private method package private and put the test class in the same package; now how hard was that?

Anything calling library code should normally be calling a interface anyway, which hides support methods from a client, because most library code should not offer static methods, which can be part of the damned singleton anti-pattern.

Scott M Gardner replied on Sun, 2014/03/16 - 8:49pm

 One big reason to move private methods out of class into an external package class is to keep to the principle that a class does one thing. As the class becomes more rich and functional, it naturally evolves and adds capability.  That is the most obvious reason. For the rest of them look into some of the principles of object oriented design.

Barry Smith replied on Mon, 2014/03/17 - 4:38am

"There is an much easier way to solve this, just make the private method package private and put the test class in the same package; now how hard was that?"

It's not hard, just confusing. If a method is package-private then anyone reading through the code is going to assume it is intended for use by other classes in the package. If a method is intended just for private use in a class then it should be marked private; anything else is just going to cause confusion for others working on the code. At least if a test class accesses a private method by using setAccessible() then it's pretty clear what's going on.

Rick Janda replied on Thu, 2014/03/27 - 5:45am in response to: Barry Smith

 There is an easier solution to avoid this confusion. Just put a @TestOnly annotation at the package scope message if it is only package scope for testing purposes. You can even use static code analysis to check that this package scope method than is also really only used by the test and not by other production code classes.

The JetBrains annotations having this @TestOnly annotations already included and the IntelliJ inspections provide the according static code analysis, too.



Comment viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.