Is it bad practice to make methods public solely for the sake of unit testing? [duplicate]

Question

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)?

Answer 1

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?

Answer 2

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.

Answer 3

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?

Answer 4

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.