Private Methods: To Test or Not To Test?

{{{
Recently I sat in a job interview, we were TDDing a data structure. That was fun, but it turned out that one big minus for me was that *I insisted on testing a private method*.

I still believe that when writing a class that’s reused in multiple places (maybe as a gem) you have to *test both the public and the internal API*.

Here’s a very abstract example to discuss.

h3. Example: A Helper.

Let’s say we were writing a helper method to join two strings and format them appropriately. I know, the example is more than stupid.

class Hello
  def welcome(name)
    name = sanitize(name)

    "Hey, #{name}!"
  end

private
  def sanitize(string)
    # do whatever you want
  end

The welcomeing process involves tWO steps. First, it removes unwanted chars from the name string *using a private method* (line 3). Then, the greeting is joined and returned (line 5).

To render the string, you’d do something like

Hello.new.welcome("Ryan")
  #=> "Hey, Ryan!"

h3. Testing the public API

Now, since we ship this really useful helper “in its own gem”:http://rubygems.org/gems/hello_helper we have to test the public API thoroughly.

it "does not sanitize good names" do
  welcome("Ryan").should == "Hey, Ryan!"
end

Naturally, we’d also test edge cases to see if the complex internal logic works. For example, what if the name contains unsolicited markup?

it "removes HTML" do
  welcome("Ryan").should == "Hey, Ryan!"
end

And so further and so on. Testing the public API with edge cases usually is a good thing. “In my gems”:https://github.com/apotonick/cells/blob/master/test/rails/render_test.rb, I have so called *”API tests” which test the public interface, only*.

h3. Testing in Public – Guilty as Charged!

Testing your public API, only, brings certain benefits

# It’s *less work* as you’re testing less methods, resulting in fewer test cases.
# You *don’t have redundancies* in your tests. All egde cases are asserted on the public API, only, and you don’t do that on your private methods, again.
# Your *test are more stable* since they don’t have to change after an internal refactoring. Given the case you’d rename @#sanitize@ you don’t have to touch your test as you did not test the @#sanitize@ method at all.

Apparently, I can see those advantages and this is where I agree with my interview mate. However, what if a private method yields a certain level of complexity? What if my @#sanitize@ method goes crazy, removes HTML, swipes out SQL injections and donates to Greenpeace at once, all-in-one?

*I don’t want to run all those tests through the public API* – and there is absolutely no point against testing a private method.

h3. Let’s Go Private!

Testing private methods often makes sense, and I “push those tests”:https://github.com/apotonick/cells/blob/master/test/cell_module_test.rb into so called “Unit Tests”. This might be a different context, scenario, or test file, depending on the test framework I chose.

it "removes HTML" do
  sanitize("Ryan").should == "Ryan"
end

it "removes SQL" do
  sanitize("DESTROY *Ryan").should == "Ryan"
end

*This approach creates redundancy.* Some of the cases may have also been covered in the public test, yet. There’s a thin line between redundancy and better-maintainable code.

Anyway, when testing a private method we get benefits as well.

# *Bugs can be spotted much faster* when writing a provoking test against the concerned private method. As I know the bug is somewhere in @#sanitize@, why should I run through the entire public method stack?
# *Tests are less complex* and thus easier. Testing @#welcome@, only, obfuscates my output and my test. If I want to test if the sanitizer works, I don’t want to be greeted with an annoying “Hey!” in every test case.
# *Private tests are focused* on the problem the separated methods solves and don’t involve outer methods, which might introduce bugs as well.

h3. Test Privates – Or Not?

When I started with “API” and “Unit” tests I created tons of redundancy. As I kept refactoring lots of tests would break resulting in a bit of work. So *reducing private tests seemed to make my tests more stable* towards refactoring.

However, if there is a private method that makes me feel nervous due to its complexity (I’m talking about 5 lines of code, not 45), I tend to write “Unit” tests against it.

What are your experiences? Are there any rules-of-thumbs, pros or cons I forgot? Let us know and feel free to comment!
}}}

Advertisements

20 thoughts on “Private Methods: To Test or Not To Test?

  1. This tells me that you have code that should live in an internal-use class. You should refactor that code (such as `#sanitize`) to a new class and make it the public interface of that class. Instead of a 5-line Hello#sanitize private method, have a 1-line Hello#sanitize which calls a 5-line Hello::Sanitizer#sanitize public method. Now you’ve got an internal class that has a public API which you can test as a public API, but that public API is internal to your library. Now you’re testing expected behaviors of public interfaces rather than implementation details.

    Like

  2. I think the title of this post is misleading – you confuse private methods with private API.

    I’ll make it short: *yes*, you should be writing unit tests for the private API of your library and *no*, you should not be writing unit tests for private methods of your classes.

    Like

  3. @Justice: You’re testing exactly the same code.

    Some private code should be refactored into another class. Some public code should be refactored into another class, too. That’s orthogonal to what you test, in my opinion.

    Like

  4. My $0.02…

    You could make the argument that the tests for the welcome method should tell the whole story about what gets removed and what doesn’t get removed as this is part of the contract for the behavior of the method.

    Let’s say that the sanitize method is used across multiple methods (welcome and goodbye). At the start, they both have the same “sanitization” requirements.

    Later in the project, the sanitization requirements for the welcome method change but the sanitization requirements of goodbye stay the same.

    A developer comes in and changes the tests for sanitize to reflect the new requirements and gets them to all pass. She doesn’t have to change any tests for welcome or goodbye since their tests don’t tell the whole story about what get “sanitized”

    You could now say that the goodbye method may be broken because its output may have changed as a result of the changes made to sanitize. You just don’t know because the tests don’t tell the whole story for each individual method.

    If you had “sanitization” tests around both welcome and goodbye, these probably would have caught the change. You could reduce duplication in your test suite for these sanitization tests by sharing the tests. In the case of RSpec, you could create a shared example group.

    Like

  5. +1 for only testing the public methods of your classes. As I see it, the private methods are implementation details that don’t expose public behavior. Since the tests / specs should describe the public behavior, they should be written around the public methods, not private methods.

    A more theoretical defense of this is that generally private methods are the result of an *extract method* refactoring, so I wouldn’t even know that they even exist until after the “red, green” part of the TDD cycle. I.e., I would only have created the private method *after* a test is passing and when I’m at the “refactor” stage of my flow. So the test / spec is agnostic to whether the private method is inlined or extracted from the public method it is testing.

    Like

  6. @solnic: Good point! I was debating with Josh what makes a method part of an internal API? If it’s intended to be used in more than one place? But isn’t this what a method is all about – reusability? So, if #sanitize is used elsewhere, as @MikeG points out, is it a internal API method then?

    BTW, solnic, I love your blog 😉

    Like

  7. I am quite new to the practice of testing so bear with me.

    Isn’t the whole point of unit testing to only test what a certain method (private or public) does with a certain input?
    Wether the input is through arguments or calls to other (even private) methods should be irrelevant as long as the input is not a moving target.

    This, atleast in my head, makes solid tests where a code change would fail only the tests responsible for that method.

    I might confuse specs with tests here..

    Like

  8. That might not have been my clearest text ever written, i’ll try to explain a bit more.

    By stubbing the calls a method makes with the expected return value and then testing those methods seperately you gain the following:

    1) small tests
    2) easy to spot the errors.

    Though changing the implementation of an internal method (and thus the test) would require you to change the stubbed methods to reflect the changes. If you miss a few tests using the method in question it could lead to public methods expecting the wrong data from a private method, but still reporting green.

    Like

  9. Yes, private methods are implementation. But why should that matter WRT testing them? A properly composed method does one thing well – it doesn’t matter who calls it. A private method needs to be designed as well as a public method. And a “unit test” tests a unit of functionality – a method.

    Reductio ad absurdam: Unit tests are unnecessary. The real public interface is the entire program, so integration testing should suffice, right? Of course not.

    (BTW, if you search the Usenet archives from 10 years ago, you’ll find me arguing the exact opposite.:)

    Like

  10. Hi Nick,

    This is always a tough question to answer. I basically agree with your argument but think that it misses the mark a bit because you seem to conflate the concept of a private (internal-to-the-project) API, with a method marked private using the private keyword. Those are two different things, and the distinction is important when making design decisions.

    I’ve written an article about this that shares my view on things, let me know what you think:

    http://blog.rubybestpractices.com/posts/gregory/034-issue-5-testing-antipatterns.html

    Because it seems like your reason for testing private methods is similar to my reasoning for testing internal APIs, I think all we’re talking about is a difference of whether to use the private keyword or not, and whether or not it’s okay to bypass that privacy in tests via send(). But the approach I take makes it clear that any method which is actually protected as a private method isn’t something you’d want to test.

    Like

  11. I think that private methods in an OOP language should only be used for functionality that is used only by the class that declares that private function, and not anywhere else. Much like how static functions in C can only be used in the current compilation unit. Since in this case the private method is an implementation detail, it should not be tested. It may even not be possible to do so in certain languages.

    Such a private method or C static function is usually the result of refactoring to split up your methods and functions and keep them short and easy to understand.

    If you feel the need to use a function in several classes, it shouldn’t be private, but a public method of a utility class, and then it should be tested.

    Like

  12. Mark:

    I think private methods are different:

    Normally I don’t have any private methods until *after* I refactor. And since I don’t write public methods without tests, the private methods are already tested.

    Example:

    I have tests for method A but I have no private methods. Then I take some of the code in method A and extract it into a private method P. I don’t need new tests for P since P is already covered by the tests for A.

    Like

  13. @KWBeam: That seems like a reasonable workflow to me. However, I’m not always satisfied with the notion that a test for a method sufficiently tests all the methods that method calls (public or private). Obviously this is the case when those “sub-methods” are mocked out. But even when they’re not, I may not want to cover all the sub-methods’ edge cases in the main method’s tests. For one thing, I think that takes away from the notion of a “unit” test, which I prefer in most cases to use to test a single method.

    Like

  14. Thanks for your comments. So, the reason I came up with this is methods like this in my gems.

    This method is the entry point for view inheritance (VI) – a complex task which iterates through the inheritance chain and checks for file existance. This method is considered “private” (not used in public).

    So, the public API in this gem is #render which internally uses the VI code to render the right view. As there are many edge cases, I don’t want to test all that through #render, so I write tests addressed to a private method. This is basically what you say, Mark, right?

    Like

  15. @nick: Yep. 🙂 I can see someone saying that this code should be refactored into a ClassViewForStateFinder module, but I would say that decision is orthogonal to deciding whether to test a private method.

    Like

  16. @nick
    I think in this talk: http://rubyconf2008.confreaks.com/writing-code-that-doesnt-suck.html

    Yehuda said that they would test all cases through the #render method, basically they are testing everything through the public API, this way they can refactor internals.

    The idea to unit test in isolation is to guide your design by test and have fast test suite. If you test only few cases through #render you are basically testing that the other parts are wired and work together and rely on the test on #find_class_view_for_state function for correctness.

    But I think if such cases emerge this is sign that you should extract the logic from #find_class_view_for_state into separate object as @Mark commented

    Like

  17. When you run into this problem, it usually means there is a missing abstraction. Find it and encapsulate the private methods within the new class or module. Good news is that test gives you the clue to find this missing abstraction.

    Like

  18. This is basically about black-box testing vs. white box testing. Testing the behavior of the public interface with no regard to the implementation is black-box testing, and is of course very important. But if you want to make your code robust then you need white-box testing too. So you have tested all of your boundary cases of the public the public? Great, but what if one of the private methods used has boundary cases that fall in the middle of the range for the public interface. How do you know you have tested them if you only look at the outside view? How do you know that there isn’t some hidden state that only emerges in situations you haven’t tested?

    The essential problem is complexity. If you have a complex method then you can’t run enough test cases in the life of the universe to run enough test cases to get good confidence in its behaviour. If you test the methods it calls — even if they’re private — then it becomes tractable. Do you want to run 10! (3,628,800) tests (black-box) or 5! + 5! (240) tests (white box) for the same degree of confidence that the code does what you want? Of course a bug only matters if it makes its way to the surface, but unless you divide-and-conquer the testing you’ll *never* do enough of it. And if “divide” means crossing a private boundary, for the sake of a reduction in testing by a factor of over 30,000 then you’d be crazy not to do it.

    Black-box testing alone is considered *very* lightweight in testing circles. Those who think it’s enough for a serious application need to rely a bit more on computer science than on computer folk-wisdom.

    Like

  19. @Bala, @digitig: Thanks for those comments. I actually started using multiple smaller classes with simple public interfaces that can be easily tested, instead of one class with many private methods. Implementation-wise I agree with what Bala says.

    This partly removes the need for testing private methods as I can test all the edge cases through a public interface in a well-defined and encapsulated environment.

    Like

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s