How can you unit test private methods? If you google this question, you find several different suggestions: test them indirectly, extract them into their own class and make them public there, or use reflection to test them. All these solutions have flaws. My preference is to simply remove the private modifier and make the method package private. Why is this a good idea? I will get to that after I discus the problems with the other methods.
First of all, not all methods need to be unit tested. Some methods are so simple that there is no point in unit-testing them. For example:
private void startLowBalanceTimer(int timeout) {
if (lowBalanceTimer != null) {
lowBalanceTimer.start(timeout);
}
}
The use of the lowBalanceTimer, including its timeout, needs to be tested, but this is better done in functional testing. It will quickly become apparent if the method above doesn’t work as expected. The methods that need to be unit tested are the ones that contain algorithmic logic of some kind. The article Selective Unit Testing – Costs and Benefits makes a distinction between algorithms and coordinators that I think is very useful. The best use of unit testing is for algorithms, but not for coordinators. Furthermore, these two types of functionality should not be mixed in the same method.
Indirect Testing?
Proponents of indirect testing of private methods argue that you should only test the public methods of a class. If the public methods make use of private methods, then these private methods automatically get tested when the observable results of the public methods are checked. But this logic is flawed. We can use the same logic to argue that no unit testing whatsoever is needed. We just test the complete program. All methods will be indirectly tested when we check the results of the complete program. Clearly this is a bad idea. Either the private method does something interesting, and then it should be unit tested in its own right, or it doesn’t do anything interesting, and then it doesn’t need to be unit tested at all.
Furthermore, this doesn’t work well if you practice Test Driven Development (TDD). If you take a bottom up approach, and develop and test the building blocks before putting them together, you often don’t have the public method ready when you are developing the helper methods. Thus you don’t get the benefits of testing while developing.
A Separate Class?
Another argument goes like this: if you have private methods that do something interesting, then it is a sign that that functionality should be extracted out into its own class. In the new class, the methods become public methods (since the main functionality of the new class is to do what the private methods did).
Sometimes this is a valid argument, especially if the extracted functionality will be useful in more than one case. However, quite often this is not the case. The functionality in the private methods is often very specific supporting functionality of the original class, and is not useful in other cases. While it certainly could be extracted, it often makes more sense to keep it close to the functionality in the original class.
If it later turns out that the same functionality is needed somewhere else, then it is a good time to extract the functionality into its own class, but not before that.
Reflection?
By using reflection, you can access the private methods of a class while still keeping them private. In my mind, this is the worst of the suggested ways. Since the method names are treated as strings, it doesn’t handle refactorings well. It is also too complicated, especially given how easily the methods can be tested if the private modifier is dropped.
Solution: Package Private
By removing the private modifier, the method is visible in the package of the class, and nowhere else. This means that the method can now be unit tested (provided that the same package is used in the test class). But aren’t we exposing too much of the internals of the class?
The answer is no. The public interface of the class is still only the public methods in the class. Unless the calling class is in the same package, it can’t use the package private methods. Furthermore, you most likely have direct control over all classes in the same package. By direct control I mean that you can check out the code, make modifications to it, and check it back in again. So it is pointless to hide methods by making them private. Whoever feels the need to call one of the private methods from another class in the same package could just change the access modifier from private to package private or public and be done.
The idea of the original class is that its interface is the public methods. The package private methods are only helper methods, and not meant to be used from outside the class. For classes in the same package, we already rely on them to use it correctly (it can’t be enforced anyway). For classes outside the package, either by another team, or from external use, the only access is via the public methods, so the encapsulation is not broken where it matters.
Example
At work, we use a traffic generation tool. One of its features is that you can generate test traffic with a given rate and duration, for example 200 SMS per second for 1 hour. Both the rate and duration are parameters that can be set in the traffic generation script. However, it is often interesting to have some variation in the load. For example, it can be good to test how the system handles a load that starts at 200 SMS per second, then decreases to 10 SMS per second, and then increases again.
At our most recent hackathon at work, I added the capability to add “scaling values” to a script to achieve this. The scaling values are a number of integer values between 0 and 100 that indicate what percentage of the nominal rate should be used. So if the values [100, 80, 20, 100] are given, then the rate will vary linearly between 100% and 80% for the first third of the duration. The rate then varies linearly between 80% and 20% for the middle third, and between 20% and 100% for the last third.
There are three advantages with this format. First, it is not necessary to give absolute values for the rate. If the rate is 200 SMS per second, 50% of that rate is 100 SMS per seconds, but if the rate is 300 SMS per second, the same scaling value of 50 would instead give 150 SMS per second. Second, the scaling values are relative to the duration of the script. The same scaling values can be used both for a 10 minute script and a 1 hour script. Third, it is possible to get a more detailed shape of the rate by increasing the number of scaling values. For example, the values [100, 90, 70, 40, 20, 10, 0, 0, 10, 20, 40, 70, 90, 100] give a more detailed shape of the rate than the previous example did.
To find the scaled rate at any given time in the script, the class ScalingValue provides the following public method:
public double getCurrentRate(long now, double maxRate)
Given the time, the nominal rate, and the scaling values (provided when ScalingValue was instantiated), the current rate is returned. Internally, there are several helper methods. One internal method, getNextIndex finds which two scaling values the current time stamp falls between. Another method, getScalingFactor, takes two scaling values and the time stamp and calculates the scaling factor at that point. For example, if the scaling value at time 1000 is 60, and the scaling value at time 2000 is 80, then the scaling value at time 1500 is 70, and at time 1750 it is 75, and so on.
Clearly there is quite a bit of logic needed to get this to work. In cases like this, I prefer to divide the tasks up into small methods, and unit test each method individually. It helps a lot to have well-tested parts before they are combined to create the complete solution.
In this case, all the internal methods are package private, so they can be unit tested easily. Should they be tested indirectly, by only testing the public method getCurrentRate? It is possible, but that is making it unnecessarily difficult. The helper methods are much easier to both write and test as separate parts.
Should there be a separate class that provides getNextIndex and getScalingFactor as its own public methods? Again, no. They are the guts of the ScalingValue implementation, and are not useful to any other class. But aren’t we unnecessarily exposing the internals of ScalingValue? Once again, no. The class that uses ScalingValue is in the same package as ScalingValue. So nothing is gained by making methods in ScalingValue private. If a developer wants to use methods other than getCurrentRate (and all helper methods were private), there is nothing that stops him/her from changing ScalingValue to make the helper methods not be private anymore. You have to trust the other developer’s judgment.
Conclusion
Unit testing methods that contain algorithmic code is important. This can be achieved with a minimum of problems by just dropping the private access modifier. There are no downsides to doing this, and you get well-factored, coherent and tested code. Great, isn’t it?
EDIT: Discussing different coding techniques without referring to code can often lead you wrong. So I have added ScalingValue.java and TestScalingValue.java to make the discussion more concrete (I should have done this from the beginning, my mistake).
EDIT 2: I have been thinking about this case a lot lately, and I have to admit that I find the arguments below by eecolor and Arho Huttunen quite persuasive. When I apply their logic to my example class ScalingValue, I can make getScalingFactor and getNextIndex private, and still cover them well in the tests. So I will follow their advice in the future (keeping the methods private, and only test through the public interface). I will however leave this blog post as it is, since the discussion of it in the comments made me change my mind. Thanks eecolor and Arho for contributing!
Henrik Warne's blog 
TDD doesn’t advocate that you take a bottom up approach, and I would argue that it’s more of a top down approach. I never really start with private methods in a class I always mock or stub out the public methods I need then extract and re factor as they get filled in. That way only testing the public methods of a class works well as you will be testing the extracted private methods as well. Testing every private method is overkill to me even if you leave out the trivial ones.
Hi Lee,
Thanks for reading. I think TDD works well for both top down and bottom up design, but when you do bottom up design, like in the example I gave, I prefer to test the methods as I develop them.
I also think that the only criteria for whether you test a method should be if it needs to be tested of not (i.e. if there is enough algorithmic logic in it to warrant testing). If a method is private that becomes another factor, because then you can’t test it directly, which is why I prefer package private.
The class only has the responsibility for fulfilling its obligations with regards to the public interface, since that’s your contract. A thorough testing of the public interface should thoroughly test all the private methods, otherwise you have unreachable code. So only test the public methods and use a code coverage analysis tool to ensure there’s no dead code lying around in the non-public methods. If you took a top-down approach to development and started with the implementation of public methods first then you shouldn’t have any dead code in the non-public methods when you’re done.
Hi Don,
Thanks for your comment! I find it easier to test the methods in isolation, then putting them together. Like I wrote in the post, why doesn’t your argument extend to the whole program as well? Why test the public methods of a class? They will get tested when you run the program as a whole. My answer to that question is that it is too difficult. We make software out of parts and it is easier to test the individual parts and then the whole, than to only test the whole.
The reason I wouldn’t apply my argument to the entire program is I look at the class as the ‘atomic building block’ of the program, it’s the lowest level of abstraction and my testing strategy is to test down to the lowest level of abstraction. The class’ public interface comprises the entirety of the class’ abstraction and therefore I don’t see the need to test further.
To take the discussion further we need to delve into OO design. My design strategy is to make the public interface invariant, as these are breaking changes, i.e. the public interface is the interface contract. I use unit tests to ensure the contract has not been broken. After testing the class I know any object interacting with that class will continue to do so successfully. Your mileage may vary, I’ve been using this practice for 20 years now and can attest to its efficacy.
You should be testing behavior of individual components and testing their interface to the rest of the program, that makes sense. I believe that TDD should help you verify and maintain the design of the program as much as the behavior. You do in fact test the whole software by testing the behavior. The design should reflect that. TDD isn’t there just to verify that each method returns the right values it much more than that. How do you know you will need X, Y and Z private methods before you build the public interface, you are just guessing.
Lee,
I completely agree that TDD isn’t only, or even mostly, about checking the results. The main benefit is getting a clean, separated structure (but the checking part is good too ;-)).
As for top down versus bottom up: it is never really an either/or case. In the example I gave, I identified the need for finding the correct scaling value by taking the linear value between the to enclosing percentage values (as determined but the time stamp). This was top down designing. Then I switched to bottom up, designing the two helper methods that I needed. One method to find which are the two enclosing values, and the other to calculate the linear value between them. So I wouldn’t call it guessing. It is more switching back and forth between a high level view and a details view.
@taylodl Well, I guess I see building blocks inside a class as well (in the form of methods).
But the key criteria is that it works in practice (and not only in theory), which obviously is the case for you. My way works well for me 🙂
Pingback: The Baeldung Weekly Review 6
While your approach does provided immediate testing for methods you write, changing the visibility of a method in order to facilitate testing seems like bad design to me and your package private proposition seems like a hack. The downside is also that when your tests become the sole users of a piece of code, it becomes mighty hard to remove dead production code since you cannot rely on code coverage anymore.
Your case is exactly why BDD in this sense is a better approach than TDD: with BDD you only test the visible behavior of your application. When all visible behavior (public and in some case package private) is tested and any private or package private methods aren’t covered then than means it’s dead code and should therefor be removed. You’ve taken TDD a bit too far and because of your approach you don’t really have dead code in your system since it’s always tested. But that doesn’t mean it’s not dead seen from a production point of view.
Thanks for your comment. I think you misunderstood what I meant. It is not dead code. Both getNextIndex and getScalingFactor are called by getCurrentRate. It would be pointless to have methods that are only called from tests.
I am all for removing dead code, but I don’t use test coverage to decide what is dead code (it doesn’t work, since it only shows code that has tests). Instead, I check manually (or with the help of the IDE) if the code is used or not.
I might make assumptions that are totally incorrect. Let’s say you have a public method that calls a (package) private one. You will probably have tests on both public and private method. Let’s say that the public method simply calls the private method. Wouldn’t the tests be exactly the same?
If you are testing if your public method calls your private method I think you are making a mistake as you are testing implementation rather than behavior. This makes your software rigid and causes refactoring to be become more tedious. I can not see a way of testing behavior where you do not have duplication of tests if you are also testing private methods.
Yes, if all the public method did was to call the (package) private method, the tests would be the same. But that is not the case. The public method performs some logic, and in the process it calls the other two methods in order to get some sub-tasks done.
I might have misunderstood, but wouldn’t the test of the public method cover the behaviour of the two sub-tasks?
Yes, you are right, it is possible to only test the methods performing the sub-tasks indirectly. My point is that I find it easier to test them directly, rather than indirectly. When the sub-tasks are tested, it is easier to get the complete method to work correctly.
The same argument applies to the whole program. You can test the whole program by only using it directly. All the components will be tested indirectly. But it is easier to test the components separately first, then put them together as the complete program. Then the bugs you find will often mostly be related to how the components interact with each other, rather bugs purely in one component.
Congratulations on a well written article, I agree with pretty much all of it, bar the main point you’re trying to make. :o)
While you consider reflection the worst approach I consider it the best approach by far. It’s the only approach that doesn’t require you to alter your code. If you decided that a certain piece of code belonged in a private method on a certain object you probably had good reasons for this and altering that just for the sake of making it testable is a big NO for me, even if the change is as minor as dropping the private keyword. Although that’s clearly still much better then unnecessarily extracting the code into another object.
Your main gripe with using reflection seems to be that it’s using Strings to represent method or variable names. In general I share your dislike of this approach as doing that indeed breaks many of the benefits offered by IDEs. In this case however I consider that to be of minor concern. Your method is private, so apart from your unit test no one will be making any calls to it. This means all your references are grouped in a single class and can be changed by a simple search and replace. As for detecting the error, I’m fairly certain you wouldn’t refactor a class without running its unit tests so even though you’re right about there not being a compile error, the error will still be detected shortly after.
Your second argument seems to be that it is hard using reflection. Using a library makes it dead easy and almost as readable as calling a method the normal way (Example using Jmockit, but there are several frameworks offering this functionality)
invoke(objectUnderTest, “methodName”, param1, param2);
Thanks Stijn, good input! At least we agree that the methods need to be tested, even though we disagree on the best way to do it.
Good post. I really like this approach myself. No, it’s not pure or ideal, but it’s practical. At the end of the day, loosening the visibility to package level is very unlikely to be the cause of a bug, and if it’s a method that the developer feels would be benefited with unit test, the developer can create a unit test. Seems like a net win to me.
All the people who say you shouldn’t unit test private methods are thinking a little too black and white for me. Sometimes you have a private method, and you think it would be better to have a unit test for it than not have one. I should say though that this practice should be the exception rather than the rule. *Most* private methods should not need to be unit tested directly.
If you use Google’s Guava library, they even provide a @VisibleForTesting annotation. It doesn’t do anything, but it’s a very handy way of documenting that the visibility of a method has been loosened for the purposes of testing (and it’s cleaner than free-form comments on everything).
Changing the visibility of a method purely so you can test it IS a bad reason to do so. And the fact that you’re stating that an annotation suddenly makes it a good practice, I’m wondering why there isn’t a @HasBugs annotation yet.
There are so many code smells on testing private methods. It is for example you’re not working interface-driven and therefor have tight coupling in your code.
Thanks Kevin! I agree, most private methods don’t need to be tested. And sometimes it is better to break out the functionality in its own class. But sometimes (like in my example), testing them is a good idea. Better to be pragmatic than too dogmatic.
@lievendoclo The interface to ScalingValue is getCurrentRate. That doesn’t change because the helper-methods are tested separately.
I can see how testing only public methods might sound like a bad idea when using a code-first approach. However, this is not the case when doing proper test-first development.
The problem with your approach is that the minute you switched to coding private helper methods first you reverted back to code-first approach with all the issues with it. You needed to be certain that your implementation works so you thought you need to be able to test private methods.
If you had continued with the test-first approach, you would have implemented a naive solution, added more tests, refined the implementation and finally refactored your code. The refactoring part is where you get your private helper methods. It is the stage where your tests provide a safety net to create those helper methods via refactoring. Whenever you are not refactoring anymore, you should alter your tests first.
Testing individual methods leads to tight coupling of tests and implementation. This is something to be avoided as it leads to unmaintainable tests. This is why you always should test behavior and not methods.
Hi Arho,
Thanks for a well-argued comment! I agree that this is often a very good approach. However, in this case I think the benefits of my way outweigh the disadvantages.
In this case, when you have to take your implementation beyond the naïve (incomplete) first solution, how do you continue? My approach was to find which two scaling values you fall between, and then take the linearly scaled value between them. This naturally lead to two helper-methods, which I implemented. How would it work in your case? How would you develop the solution? Feel free to link to some code and tests, since having concrete examples is much better than a general discussions.
Well, basically my first implementation would have just one public method called getCurrentRate(). I would still find the values you fall between and take the linearly scaled value but it would all be inline code. There would be tests to verify the current rate. If I would leave it like this I’d be happy with my tests, right?
However, the code would be ugly and I would refactor by extracting some private methods. But since I already covered the same code by testing the public method, there’s no need to separately test these private methods. Absolutely nothing changed in the implementation except some refactoring was done.
If you take a look at testGetScalingFactor method it’s basically repeating the logic from getCurrentRate. There should be no logic in the tests, especially not tied to the implementation.
The problem with this approach is that when you develop the solution, and the tests don’t pass, it’s hard to know where the problem is. Is the result wrong because you got the wrong indices, or is it wrong because the calculation of the linearly scaled value is wrong? Developing the parts in insolation, then putting them together makes developing the code more straightforward.
Not really. You wouldn’t implement the method in one go. You would start by a really simple test case and create a very naive implementation for that, e.g. only test with as little data as possible. Then you would gradually add tests cases and refine your implementation running the existing tests in every step. Whenever you introduce an error the existing tests will catch that. Finally you would have all the test cases covering different scenarios and the complete implementation. The whole point is to advance in very small increments repeatedly running the tests as we go.
Actually, in your first implementation you could start by hard coding the previous and next index and the calculation of scaled value. Once you know that works you can replace that with the logic that fetches the indices etc. You’ll instantly know when you make a mistake if you work with very small increments.
Arho, I am not convinced that’s the better way in this case, but I’m glad you took the time to comment.
If you are still not convinced I would take a look at the testGetScalingFactor method. Since you have duplicated the logic from getCurrrentRate into testGetScalingFactor, how do you know the problem is in the calculation and not in your test code?
Suggested reading:
http://www.objectmentor.com/resources/articles/xpepisode.htm
http://practicalunittesting.com/btgt.php
See EDIT 2 above – I have changed my mind.
I see you have added the code. I first want to say that I think that you handle critique very well, compliments for that.
There are from my perspective several flaws in your code. There are a few selected cases where I can understand that it’s more practical to write a test for a private method (legacy code in combination with time constraints). And even in most of those cases I would advise to refactor the code. In most other cases I would advise against writing tests for private methods.
Making private methods available for test should give you a signal that something is wrong. In most cases your function does too much. Sometimes this is solved by creating private methods, yet having a public method that calls many private methods in itself is a smell. If you inspect the methods of your class you can see that there are many subjects covered. The Single Responsibility Principle states that “that every class should have a single responsibility”, this is an article you might find interesting: http://eyalgo.com/2014/02/01/the-single-responsibility-principle/
I also noticed that you are testing the contents of the `valueList`. What happens if a colleague want’s to replace it with another type of object that is more efficient? He would need to adjust a lot of test cases while he is only interested in providing a faster implementation with the same behavior.
Another problem is the `setTimeAnchors`. It’s mandatory to call this method before you can call the `getCurrentRate`. That makes this class hard to use from an end user perspective. There is a name for this anti-pattern but I can’t remember it. On top of that, it has a different visibility.
About visibility, it’s unclear what methods are used in other classes within the same package from viewing this file. The constructor seems visible only to the current package while the `getCurrentRate` method is public. This makes the class more complicated than it needs to be.
The way it is now makes me think that there must be some special class somewhere in this package that is allowed to create an instance. That class will perform some magic around the instantiation of this class (it must be, otherwise the constructor would be public, right?). I can not change any of the method names without searching the whole package for usage, this class must be very important within this package.
As you can see, the visibility (or lack of private) makes this class more complex in the mind of the reader of your code.
Some other flaws:
– Duplication of tests. You are testing the same thing over and over again.
– It does not give the user of your class an indication what’s wrong when an empty input list is provided and the user calls `setTimeAnchors`. He will just receive “devision by zero”.
– Providing a different value for ‘now’ in `setTimeAnchors` and `getCurrentRate` can be problematic. Your class is not helping the user of the class in this case.
– A lot of your code is untested. An example is the return of the maxRate in if statements. The end user might make a typeo and you are not letting him know that he is providing an out of range value. If it is intended behavior it should in the test and in the comment as well.
– I would expect a test that would check the whole algorithm. From start to end with steps that are as small as is needed by the precision requirements. A test library like QuickCheck might help you with that: https://bitbucket.org/blob79/quickcheck
– The code is quite hard to read, I expected private methods like “isValidStartTime(now)”, “findFirstAnchorAfter(now)”, “findLastAnchorBefore(now)”, “getScaleFactorBetween(firstAnchor, secondAnchor, time)”
– The concept you are modeling in your code is not new. A quick google on “linear method with anchor points” led me to “Piecewise linear function”. In some cases you find that it can help you finding a solution to your problem either in the form of a library or a mathematical notation.
– I would expect such a program to be a bit easier to use. Essentially I could pass in a number between 0 and 1 and it would given be back the rate. The concept of time seems not needed.
I am not trying to bash on your code, but trying to convey things I have learned. I think you could really benefit from practicing with strict Test or Behavior Driven Development. I really recommend you watch the first few episodes of Clean Coders. Episode 6 goes into the details of strict Test Driven Development. Uncle Bob is a bit wacky, but he is saying really interesting stuff: http://cleancoders.com/category/fundamentals#videos
Anyway, have fun on your programmer journey!
Hi eecolor,
Thanks for your long comment. You are making a lot of good points, and I appreciate that. I am the first to admit that ScalingValue is not perfect (whatever that means for a program).
First off, the reason the time values are set at the beginning is because where the script is parsed, you know the duration of the script, but from where getCurrentRate is called, you are not aware of the duration, only the current time. This you can not tell just by looking at the ScalingValue.
I make a distinction between an API-class, and a regular class. An API-class is meant to be used by others (other teams, external users), and a regular class is only used internally. ScalingValue is only used internally, from one place in the code. Therefore, it is not big deal to change how it is used (in other words, there are no “breaking changes”). If something is changed in the ScalingValue interface, all you have to do is make the corresponding change from where it is called, an you are done. It is trivial to find all the usages of its methods and fields (just “find-usage in the IDE).
That is obviously not the case for an API-class. There you have to be much more careful. Most of the code I write is not API-code, and so it doesn’t require as much polish.
This doesn’t mean that “anything goes”, but you get to a point where there are diminishing returns from improving ScalingValue. It does what it needs to do, it works, but it is not perfect. So the trade-off is to decide if it is good enough and move on, or to keep improving it.
It is possible (but not very likely) that ScalingValue will turn out to be needed in more than one place. If that happens, that’s the time to polish it up a bit more, and make it more of an API-class.
As for the SRP, it depends on what granularity you use. The single responsibility of ScalingValue is to give you the scaled value given the time. You can split it up in several parts, but I not sure that’s an improvement.
Since you have already invested a lot of time in this, it would be really nice to see what your code would look like. Just put something up and link to it. Thanks again for your thoughtful comment.
See EDIT 2 above – I have changed my mind.
Thanks for this amazing information