I have a class with a public method. It has other methods that 'assist' with the purpose of the public method. The public method must be tested. However, I also want to unit test the private methods.

It would be impossible for my unit tests to call private methods and test them in isolation, so they must be public.

The problem here is that I'm left with code that, I feel, has been written with a lack of respect for OOP convention, purely for the sake of being able to unit test it.

Am I right in amending the access of methods purely for 'testability'? Or should I be rethinking and refactoring the structure as a whole to separate out the functionality of the private methods (despite the fact they don't do a lot at all and may, at times, resemble small helper methods)?

marked as duplicate by gnat, Doc Brown, Mike Nakis, GlenH7, Eric King Mar 2 '15 at 15:31

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

  • 6
    "Never touch your privates" applies. All private methods should be covered through your public methods. Those that don't are redundant :) – Lieven Keersmaekers Mar 2 '15 at 11:27
  • Check out the 'Internal' modifier and the 'InternalsVisibleTo' attribute. It can allow you to expose functions and fields for testing only, while leaving them private when being consumed outside your library. – KChaloux Mar 2 '15 at 13:26
  • 2
    @KChaloux that may be a practical way to achieve what the OP has asked for, but all the answers (so far) say that it would still be the wrong thing to do. Either these methods should be on some class's public interface or they should not be directly tested. – itsbruce Mar 2 '15 at 13:46
  • Does C# have similar facilities to package private (the 'default' / no modifier) protection that Java has? It is not uncommon for Java programmers to have unit tested internal methods be at this so that a unit test in the same package can access them, but they are not part of the public api nor accessible from outside of the package. – user40980 Mar 2 '15 at 15:03

Yes it is very bad practice - you're letting your tools make design decisions for you.

I think the main problem here is that you're trying to treat each individual method as a unit. This is generally the cause of all unit test woes. Except for some cases where your method is very complex and requires a lot of testing, you should be treating your classes as units. Martin Fowler even treats related classes as a unit too (sometimes).

As a result, you should be instantiating a class, then calling method(s) on its public interface as it would be used. This gives you your examples for your documentation, and ensures it works as the whole thing is intended. You should be trying here to test the private methods by calling the public ones - and if a private method is never called, then why do you still have it in code?

  • 3
    To add to gbjbaanb's answer, I have found that when you find your self wanting to test private methods, your class is trying to do too much. Thinking about the generalized concepts your code is trying to accomplish and break your code out into one class per concept. Think of validation, storage, caching, user input, networking as all separate concepts that require separate classes/interfaces. – mortalapeman Mar 2 '15 at 16:24
  • even small classes need private methods. The problem is really to do with testing via the public interface, rather than testing every method in isolation. – gbjbaanb Mar 2 '15 at 16:26
  • I agree with you gbjbaanb, testing public methods should exercise the private methods as well. But when your private methods are too complex or testing them via the public interface requires multiple method calls, you probably need to make your API more granular. – mortalapeman Mar 2 '15 at 16:31

If you have methods 'assisting', chances are your single, big method is actually doing too much. Splitting into smaller methods and then moving these methods into separate classes with public interfaces keeps the class with the big method responsible for one thing and one thing only (see Single Responsibility Principle). This move into separte classes automatically makes for a testable structure as your methods must be made public.

An example

Take a BankAccount-class that does budget calculations by looking at a list of Expenditures it contains. An Expenditure has a category (sports, food, car,...), an amount and a date. You might have a method on BankAccount called "CalculateExpenditureFor" taking a begin and enddate and a category. That method would filter the Expenditures to only have Expenditures that match the date and category and then proceed to sum up the amounts. Take note of the amount of 'ands' I need to express that requirement! This is clear indication that your method does too much.

This method will actually do two things: filter the expenditures and sum up the amounts. Now imagine your BankAccount contains an ExpenditureLedger-class, which contains the Expenditures (a 'ledger' is a book or file containing a list of financial transactions, hence the name). You could then have a FilterExpenditures-method on the ExpenditureLedger which takes care of the filtering on date and category. The resulting list of Expenditures can then be used by the BankAccount to tally up the amounts.

In this scenario, you have a public method on the ExpenditureLedger that can be tested (you feed the ledger Expenditures and check the result). You can also check the BankAccount-method by feeding it a (preferably mocked) ExpenditureLedger so you can test the code that sums up the amounts returned by the ExpenditureLedger.

You should not have to change your unit tests if you have only changed internal implementation details and not the API. The fact that your old tests still work after a change is the proof that your changes have not broken things. You can look at the commit log, see that absolutely nothing in the test code has changed and feel reassured (as long as your tests were good in the first place).

Now, you are proposing a design which forces changes to your tests when you alter implementation. You destroy that confidence. There will always be a risk that changes have been made to the genuine public API tests in the course of testing implementation changes.

Either your object hierarchy is wrong and those methods should be public on some other object, or they are implementation details to your object and you are wasting time testing them (and risking making your whole design more fragile). Test the contract.

An important question is "Why do you want to test them?" If these methods are in the right place and not relevant to external code then you need to stop being a control freak, shake off your OCD and trust the system. If not testing them really is a problem, your current object hierarchy (or your API design) is wrong.

Making the methods public - yes, that is bad practice. Making them internal - that depends.

Instead of making all methods to be tested public, and instead of redesigning your classes completely, sometimes the most pragmatic solution is to make the methods in stake "internal" and use the "InternalsVisibleTo" attribute to allow your unit tests access them. That may not lead to the ideal design in the first place, but sometimes you just have to get things done, and testing "internal" code is often better than having no tests at all or introducing accidentally new bugs by refactoring those private methods out to separate classes without having any unit tests written before.

See also How to define implementation details?

Not the answer you're looking for? Browse other questions tagged or ask your own question.