Making a private method public to unit test it...good idea?

Note: This answer was originally posted for the question Is unit testing alone ever a good reason to expose private instance variables via getters? which was merged into this one, so it may be a tad specific to the usecase presented there.

As a general statement, I'm usually all for refactoring "production" code to make it easier to test. However, I don't think that would be a good call here. A good unit test (usually) shouldn't care about the class' implementation details, only about its visible behavior. Instead of exposing the internal stacks to the test, you could test that the class returns the pages in the order you expect it to after calling first() or last().

For example, consider this pseudo-code:

public class NavigationTest {
    private Navigation nav;

    @Before
    public void setUp() {
        // Set up nav so the order is page1->page2->page3 and
        // we've moved back to page2
        nav = ...;
    }

    @Test
    public void testFirst() {
        nav.first();

        assertEquals("page1", nav.getPage());

        nav.next();
        assertEquals("page2", nav.getPage());

        nav.next();
        assertEquals("page3", nav.getPage());
    }

    @Test
    public void testLast() {
        nav.last();

        assertEquals("page3", nav.getPage());

        nav.previous();
        assertEquals("page2", nav.getPage());

        nav.previous();
        assertEquals("page1", nav.getPage());
    }
}

Personally, I'd rather unit test using the public API and I'd certainly never make the private method public just to make it easy to test.

If you really want to test the private method in isolation, in Java you could use Easymock / Powermock to allow you to do this.

You have to be pragmatic about it and you should also be aware of the reasons why things are difficult to test.

'Listen to the tests' - if it's difficult to test, is that telling you something about your design? Could you refactor to where a test for this method would be trivial and easily covered by testing through the public api?

Here's what Michael Feathers has to say in 'Working Effectively With Legacy Code"

"Many people spend a lot of time trying ot figure out how to get around this problem ... 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 reponsibility; it should be on another class." [Working Effectively With Legacy Code (2005) by M. Feathers]

As others have said, it is somewhat suspect to be unit testing private methods at all; unit test the public interface, not the private implementation details.

That said, the technique I use when I want to unit test something that is private in C# is to downgrade the accessibility protection from private to internal, and then mark the unit testing assembly as a friend assembly using InternalsVisibleTo. The unit testing assembly will then be allowed to treat the internals as public, but you don't have to worry about accidentally adding to your public surface area.

Lots of answers suggest only testing the public interface, but IMHO this is unrealistic - if a method does something that takes 5 steps, you'll want to test those five steps separately, not all together. This requires testing all five methods, which (other than for testing) might otherwise be private.

The usual way of testing "private" methods is to give every class its own interface, and make the "private" methods public, but not include them in the interface. This way, they can still be tested, but they don't bloat the interface.

Yes, this will result in file- and class-bloat.

Yes, this does make the public and private specifiers redundant.

Yes, this is a pain in the ass.

This is, unfortunately, one of the many sacrifices we make to make code testable. Perhaps a future language (or a even a future version of C#/Java) will have features to make class- and module-testability more convenient; but in the meanwhile, we have to jump through these hoops.

There are some who would argue that each of those steps should be its own class, but I disagree - if they all share state, there is no reason to create five separate classes where five methods would do. Even worse, this results in file- and class-bloat. Plus, it infects the public API of your module - all those classes must necessarily be public if you want to test them from another module (either that, or include the test code in the same module, which means shipping the test code with your product).

A unit test should test the public contract, the only way how a class could be used in other parts of the code. A private method is implementation details, you should not test it; as far as public API works correctly, the implementation doesn't matter and could be changed without changes in test cases.

IMO, you should write your tests not making deep assumptions on how your class implemented inside. You probably want to refactor it later using another internal model but still making the same guarantees that previous implementation gives.

Keeping that in mind I suggest you to focus on testing that your contract is still holds no matter what internal implementation your class currently have. Property based testing of your public APIs.

How about making it package private? Then your test code can see it (and other classes in your package as well), but it is still hidden from your users.

But really, you should not be testing private methods. Those are implementation details, and not part of the contract. Everything they do should be covered by calling the public methods (if they have code in there that is not exercised by the public methods, then that should go). If the private code is too complex, the class is probably doing too many things and in want of refactoring.

Making a method public is big commitment. Once you do that, people will be able to use it, and you cannot just change them anymore.

Update: I have added a more expanded and more complete answer to this question in numerous other places. This is can be found on my blog.

If I ever need to make something public to test it, this usually hints that the system under test is not following the Single Reponsibility Principle. Hence there is a missing class that should be introduced. After extracting the code into a new class, make it public. Now you can test easily, and you are following SRP. Your other class simply has to invoke this new class via composition.

Making methods public/using langauge tricks such as marking code as visible to test assembilies should always be a last resort.

For example:

public class SystemUnderTest
{
   public void DoStuff()
   {
      // Blah
      // Call Validate()
   }

   private void Validate()
   {
      // Several lines of complex code...
   }
}

Refactor this by introducing a validator object.

public class SystemUnderTest
{
    public void DoStuff()
    {
       // Blah
       validator.Invoke(..)
    }
}

Now all we have to do is test that the validator is invoked correctly. The actual process of validation (the previously private logic) can be tested in pure isolation. There will be no need for complex test set up to ensure this validation passes.

Some great answers. One thing I didn't see mentioned is that with test-driven development (TDD), private methods are created during the refactoring phase (look at Extract Method for an example of a refactoring pattern), and should therefore already have the necessary test coverage. If done correctly (and of course, you're going to get a mixed bag of opinions when it comes to correctness), you shouldn't have to worry about having to make a private method public just so that you can test it.

If you are using C# you can make method internal. That way you don't pollute public API.

Then add attribute to dll

[assembly: InternalsVisibleTo("MyTestAssembly")]

Now all the methods are visible in your MyTestAssembly project. Maybe not perfect, but better then making private method public just to test it.

Why not split out the stack management algorithm into a utility class? The utility class can manage the stacks and provide public accessors. Its unit tests can be focussed on implementation detail. Deep tests for algorithmically tricky classes are very helpful in wrinkling out edge cases and ensuring coverage.

Then your current class can cleanly delegate to the utility class without exposing any implementation details. Its tests will relate to the pagination requirement as others have recommended.

In java, there's also the option of making it package private (ie leaving off the visibility modifier). If your unit tests are in the same package as the class being tested it should then be able to see these methods, and is a bit safer than making the method completely public.

Private methods are usually used as "helper" methods. Therefore they only return basic values and never operate on specific instances of objects.

You have a couple of options if you want to test them.

Use reflection

Give the methods package access

Alternatively you could create a new class with the helper method as a public method if it is a good enough candidate for a new class.

There is a very good article here on this.

Use reflection to access the private variables if you need to.

But really, you don't care about the internal state of the class, you just want to test that the public methods return what you expect in the situations you can anticipate.

In your update you say that it's good to just test using the public API. There is actually two schools here.

Black box testing The black box school says that the class should be considered as a black box that no one can see the implementation inside it. The only way to test this is through the public API -- just like the user's of the class will be using it. white box testing. The white box school thinks it naturally to use the knowledge about the implementation of the class and then test the class to know that it works as it should.

I really cannot take side in the discussion. I just thought it would be interesting to know that there are two distinct ways to test a class (or a library or whatever).

in terms of unit testing, you should definitely not add more methods; I believe you would better make a test case just about your first() method, which would be called before each test; then you can call multiple times the - next(), previous() and last() to see if the outcomes match your expectation. I guess if you don't add more methods to your class (just for testing purposes), you would stick to the "black box" principle of testing;

You should never ever ever let your tests dictate your code. I'm not speaking about TDD or other DDs I mean, exactly what your asking. Does your app need those methods to be public. If it does then test them. If it does not then then don't make them public just for testing. Same with variables and others. Let your application's needs dictate the code, and let your tests test that the need is met. (Again I don't mean testing first or not I mean changing a classes structure to meet a testing goal).

Instead you should "test higher". Test the method that calls the private method. But your tests should be testing your applications needs and not your "implementation decisions".

For example (bod pseudo code here);

   public int books(int a) {
     return add(a, 2);
   }
   private int add(int a, int b) {
     return a+b;
   } 

There is no reason to test "add" you can test "books" instead.

Never ever let your tests make code design decisions for you. Test that you get the expected result, not how you get that result.

I would say it is a bad idea for I am not sure whether you get any benefit and potentially problems down the line. If you are changing the contract of a calls, just to test a private method, you're not testing the class in how it would be used, but creating an artificial scenario which you never intended to happen.

Furthermore, by declaring the method as public, what's to say that in six months time (after forgetting that the only reason for making a method public is for testing) that you (or if you've handed the project over) somebody completely different won't use it, leading to potentially unintended consequences and/or a maintenance nightmare.

First see if the method ought to be extracted into another class and made public. If that's not the case, make it package protected and in Java annotate with @VisibleForTesting.

There are actually situations when you should do this (e.g. when you're implementing some complex algorithms). Just do it package-private and this will be enough. But in most cases probably you have too complex classes which requires factoring out logic into other classes.

Private methods that you want to test in isolation are an indication that there's another "concept" buried in your class. Extract that "concept" to its own class and test it as a separate "unit".

Take a look at this video for a really interesting take on the subject.

I tend to agree that the bonuses of having it unit tested outweigh the problems of increasing the visibility of some of the members. A slight improvement is to make it protected and virtual, then override it in a test class to expose it.

Alternatively, if it's functionality you want to test separately does it not suggest a missing object from your design? Maybe you could put it in a separate testable class...then your existing class just delegates to an instance of this new class.

I generally keep the test classes in the same project/assembly as the classes under test.
This way I only need internal visibility to make functions/classes testable.

This somewhat complicates your building process, which needs to filter out the test classes. I achieve this by naming all my test classes TestedClassTest and using a regex to filter those classes.

This of course only applies to the C# / .NET part of your question

I will often add a method called something like validate, verify, check, etc, to a class so that it can be called to test the internal state of an object.

Sometimes this method is wrapped in an ifdef block (I write mostly in C++) so that it isn't compiled for release. But it's often useful in release to provide validation methods that walk the program's object trees checking things.

Guava has a @VisibleForTesting annotation for marking methods that have enlarged scope (package or public) that they would otherwise. I use a @Private annotation for the same thing.

While the public API must be tested, sometimes it's convenient and sensible to get at stuff that wouldn't normally be public.

When:

a class is made significantly less readable, in toto, by breaking it up into multiple classes,

just to make it more testable,

and providing some test access to the innards would do the trick

it seems like religion is trumping engineering.

I usually leave those methods as protected and place the unit test within the same package (but in another project or source folder), where they can access all the protected methods because the class loader will place them within the same namespace.

No, because there are better ways of skinning that cat.

Some unit test harnesses rely on macros in the class definition which automagically expand to create hooks when built in test mode. Very C style, but it works.

An easier OO idiom is to make anything you want to test "protected" rather than "private". The test harness inherits from the class under test, and can then access all protected members.

Or you go for the "friend" option. Personally this is the feature of C++ I like least because it breaks the encapsulation rules, but it happens to be necessary for how C++ implements some features, so hey ho.

Anyway, if you're unit testing then you're just as likely to need to inject values into those members. White box texting is perfectly valid. And that really would break your encapsulation.

In .Net there is a special class called PrivateObject deigned specifically to allow you to access private methods of a class.

See more about it on the MSDN or here on Stack Overflow

(I am wondering that no one has mentioned it so far.)

There are situations though which this is not enough, in which case you will have to use reflection.

Still I would adhere to the general recommendation not to test private methods, however as usual there are always exceptions.

The point of the unit test is to confirm the workings of the public api for that unit. There should be no need to make a private method exposed only for testing, if so then your interface should be rethought. Private methods can be thought as 'helper' methods to the public interface and therefore are tested via the public interface as they would be calling into the private methods.

The only reason I can see that you have a 'need' to do this is that your class is not properly designed for testing in mind.

As is extensively noted by others' comments, unit tests should focus on the public API. However, pros/cons and justification aside, you can call private methods in a unit test by using reflection. You would of course need to make sure your JRE security allows it. Calling private methods is something that the Spring Framework employs with it's ReflectionUtils (see the makeAccessible(Method) method).

Here's a small example class with a private instance method.

public class A {
    private void doSomething() {
        System.out.println("Doing something private.");
    }
}

And an example class that executes the private instance method.

import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
public class B {
    public static final void main(final String[] args) {
        try {
            Method doSomething = A.class.getDeclaredMethod("doSomething");
            A o = new A();
            //o.doSomething(); // Compile-time error!
            doSomething.setAccessible(true); // If this is not done, you get an IllegalAccessException!
            doSomething.invoke(o);
        } catch (IllegalAccessException e) {
            e.printStackTrace();
        } catch (InvocationTargetException e) {
            e.printStackTrace();
        } catch (NoSuchMethodException e) {
            e.printStackTrace();
        } catch (SecurityException e) {
            e.printStackTrace();
        }
    }
}

Executing B, will print Doing something private. If you really need it, reflection could be used in unit tests to access private instance methods.