Aug 4, 2008

Should I Unit Test Private Methods?

I was talking with a team last week about unit testing and TDD and the question came up as to whether private methods should be tested.  I immediately said "no way" without any hesitation and was really surprised at the reaction from some of the team.  We had a very interesting chat about it, and I thought I'd put down my thinking here as well to see what you think.

Firstly I see unit testing as black box testing.  We should test our classes through their public and internal methods - i.e. from the viewpoint of a consumer of of the class; without any understanding of how that functionality may be implemented internally.

Next, the only reason for a class to have private methods is to support the public and internal methods of that class.  If there are private methods that aren't reached through public or internal methods then you have dead code and it should be removed.  Similarly, if you can't hit all of the code in your private methods through the public/internal methods then you have dead code in your method and it, likewise, needs to be removed.

Now, in order to test private methods your code needs to have some way of exposing those private methods to the test classes, meaning they can't actually be private methods in all situations.  They need to be private for release builds, but accessible for test builds.  You can try using compiler directives or some other mechanism to control accessibility however this is error prone and it's easy to accidentally release a version of an assembly with the private methods exposed as internal or public methods.  More often though you'll simply see people writing exposing methods that really should be private just to make it easier to test.  Not a design choice.

Further, tying tests to private methods makes your tests brittle.  If start to refactor a class I should be free to make changes to the private methods without impacting the externally exposed functionality of the class.  For example, if I rename an internal or public method I would expect tests to fail, but renaming a private method should have no impact on the tests and I would expect that they should continue to function correctly.

For me these reasons are reason enough.

 

So why would you want to test a private method?  The argument is something like this "If my public method does a lot of work - e.,g. setting up connections to databases and so forth, and my private method is just doing a small piece of logic then I want to test that the logic is correct without needing to go through all the other work."  It sounds like a reasonable argument but it's an argument that's only required when poor design decisions have been made.

A well designed class will do one thing and one thing only (the single concern principle) and will call out to other classes for functionality such as connecting to databases or file systems, etc.  By using dependency injection and programming to interfaces you can make testing much simpler by passing mock objects to the class under test without actually needing to do the work of connecting to a database for instance.  Not only does this help with the separation of concerns principle, but it also means my unit tests will actually be unit tests and not integration tests.  And by using mock objects they'll also be a lot faster to run.

 

So, what do you think? Leave a comment if you agree or disagree.  I'd like to hear your thoughts on the subject.

7 comments:

  1. The whole "You should be using interfaces everywhere and then unit testing with mock objects" idea comes with a lot of development overheads.
    And I still don't see how supplying a mock object auto generated from a database table is better than using a Build database that is created with the same data during each build.
    Does it give you more confidence in your tests? Definitely not. Is it more maintainable? Probably not. Does it make your unit tests run 10 seconds quicker? Yes, well woopdyfuckingdoo.

    ReplyDelete
  2. Hi Richard,

    Quis custodiet ipsos custodes?

    Any tests for private methods which are covered for the public methods is similar to guarding the guards and no one should write more code than they have to.

    Using some form of code coverage analysis would determine if the public tests were executing all the private code.

    Gary

    ReplyDelete
  3. Richard - I completely agree. Unit testing the public interface provides full code coverage - the remainder is simply cruft.

    Anon - Does the use of a 'build' database really consitute focused and isolated unit tests? Probably not. Does it easily facilitate the creation of non 'happy day' tests? Definitely not.

    ReplyDelete
  4. I'm with you, unit tests are there to test the public facing methods on your class. Ideally they should be testing some sort of functionality that the class is providing.

    Going into the internals of a class and testing those is a bad bad idea as far as I'm concerned. You then can't refactor the class without breaking your tests and as you say it's a symptom of a poor and probably coupled design.

    ReplyDelete
  5. I love this old argument, but I still can't get my head around having to change/compromise my perfectly legitimate and secure code for the sake of test tools and test methodologies that aren't up to the challenge.

    Do you REALLY think if, say, a cruiseship manufacturer was told by a test crew "it's too hard to test the engines so we need to change the design of the ship to place them somewhere easier to get to - i know, OUTSIDE the hull" that they wouldn't be laughed out of town?

    It's equally daft to be told "we'll test all the components powered by the engines once they're operational" - so don't give me the spiele that public methods should test their privates (oo-er!) and code-coverage will prove it. Coz there goes your integrated, real-time unit tests until the public methods are available.
    Madness & Dodgy++

    Bottom line: software is what it is and the tools need to support it accordingly.
    Our software is working, our geeks are productive; stop messing around and gimme my test utils that support our processes and stop making things so darn hard.

    ReplyDelete
  6. Well, well, well. "A well designed class will do one thing and one thing only (the single concern principle) and will call out to other classes for functionality such as connecting to databases or file systems, etc....."
    With design like this I smell patterms, and with huge use of patterns I smell brainless zombie developers. Nothing personal, but you are wrong :)

    From time to time I develop controls for my customers (other developers). So I really use a lot of private/protected methods inside control. And I provide public methods only for control functionality. No need to know how control is drawn (some hypothetical control), how it operates on data internally. It is a black box. It provides (simply said) buttons to push, and boxes to show processed data.

    Do you really want to see all the classes and functions related to single button control when you use the button?

    Remember basic principles of OOP. Only after that think about the patterns. Not vice versa. Private methods were created for a reason. They were successfully used for years. Now we have patterns fashion, and it seems like everyone forgot to use brains.

    ReplyDelete
  7. @Green What's wrong with using 'internal' if you're building an API and not wanting to expose it's *internal* workings. Private means private to the class, not private to the assembly.

    Of course, I'm assuming that your control impolementation over multiple classes and not all dumped into a single class.

    ReplyDelete