Unit test helper methods?

Question

I have classes which previously had massive methods so I subdivided the work of this method into 'helper' methods.

These helper methods are declared private to enforce encapsulation - however I want to unit test the big public methods. Is it good to unit test the helper methods too as if one of them fail the public method that calls it will also fail, and this way we can identify why it failed?

Also in order to test these using a mock object I would need to change their visibility from private to protected, is this desirable?

Ulane

Answer 1

One way is to omit private and put the tests in the same package. Then the tests can call the internal methods but no one else (= outside the package) can.

Also, failing internal methods should produce error messages which make it easy to fix the issue. When you put the code into production, you'll see less than the tests and you'll be under a lot of pressure to fix the issues fast. So one minute spent here will save you one hour later with your boss sitting in your neck.

Afterpiece

Instead of makeing them package private, I prefer to make helper methods protected, and test them from an anonymous subclass in the JUnit test. – Lyell

@rsp: That's a fantastic solution that I've never thought of before. It should be its own answer so I can upvote it! :) – Voyles

Answer 2

This smells like you have the wrong problem. What you've described is akin to creating a sub-"unit test", which leads me to believe that your unit tests are really testing a unit after all.

Which is not a criticism of what you are trying to do: going from "where we are today" to "somewhere else that's measurably better" is a winning move. However, it is a suggestion that you step back a bit to evaluate where you are - understanding how your current situation differs from some Platonic ideal could help to show new possibilities.

There are plenty of suggestions here about scoping your helper methods. Another possibility would be to review the implementation to determine if there are helper classes lurking in your current implementation. Creating a new class and a suite of tests to exercise it is always acceptable.

Note that this approach insulates you from the refactoring: you can change the implementation without changing your test suite (because the unit tests for the helper object continue to pass even when the helper object is no longer part of your production implementation), and you get a clean packaging of the implementation and the tests for it (usecase: you decide that bozo-sort is the wrong implementation, and should no longer be used. If the bozo-sort implementation is isolated, then you simply remove it and its tests. But when the tests of the bozo-sort implementation are tangled with all of the other tests, there's more thinking involved).

It may also help to review why you have unit tests for your code. If one of the reasons is "to make refactoring safe", then you don't want to be writing tests that lock you into an implementation.

Northeastwards

Answer 3

If your class really is that big, then it sounds like you should be breaking out helper objects, not just helper methods (although extracting methods is often a step along the way). Once you've done that, your old class becomes simpler and easier to test (perhaps with mocks, perhaps not), and you can test the methods on the new supporting classes directly.

My preference is to test through the public API of an object. If that's too hard, then it's a hint that the object should be broken up.

Addington

"If that's too hard, then it's a hint that the object should be broken up." Take your upvote. – Quesenberry

Answer 4

I'm pretty shocked at some of the answers here.

In essence some people are saying "Don't test the private code, because that violates the TDD paradigm"

Test the damn code. Do whatever you need to in order to make sure that it works exactly as it should.

Personally, I would make the methods protected or default, write the tests, run the tests, and upon success, revert back to private. At this point, I would comment out the relevant tests, and leave an instruction block above them:

/** Sorry about this, but I inherited a mess... * if you need to test these methods, expose them in source and un-comment the following * lines */

But absolutely never let rigid adherence to a development methodology get in the way of improving code.

Elide

Your suggestion is basically just as dogmatic - you MUST test every method individually, because otherwise it violates the TDD paradigm. Nonsense. A helper method can just be a common piece of code - and your code coverage tools can verify that the unit tests for the public methods covered all of it, if need be. – Newsmonger

Amen @M1EK, I just had to post an answer as nobody else had even mentioned code coverage! – Urinate

Original Poster expressed a desire to test those methods individually. "...test the helper methods too as if one of them fail the public method that calls it will also fail - but this way we can identify why it failed." – Elide

I think we're talking about two different things here. In my world, such tests run automatically all the time. There's no way I could live with altering code based on instructions. Your world is obviously different. – Addington

Answer 5

If you want to test Helper methods you can change them from private but you might consider this.

You should not unit test private details of your implementation mainly because it might change due to refactoring and "break" your test.

Equisetum

Answer 6

This is one of those cases where I would say go ahead and break the rules.

If you were designing the class from scratch, you definitely don't want helper methods to be unit tested on their own, but... since you are refactoring an existing class, it's acceptable to bend the rules in order to make sure you don't break anything.

Making them protected will let you test the helpers themselves, to make sure they still have the behavior you are expecting as you pull the logic out of the big hairball method, as well as allow you to stub them out and return fixed responses so that you can make sure the big method you are refactoring behaves as expected for certain results of the helper methods.

But you're not done yet at that point. Splitting the method up isn't really getting to the root of your problem. Now that you have the method broken up and a set of (slightly unorthodox) tests that show you exactly what all of the logic does, you are in a good position to re-examine the whole class and try to figure out why the method was so big in the first place. Most likely your whole class also needs to be broken up into smaller units with discrete responsibilities that will then be easier to test without bending any rules.

Mccown

Answer 7

You basically have 2 options:

1. Increase the scope of the helper methods from private to default. You can then test these methods (assuming the test classes are in the same package as the test subject). This improves the testability of the class but you sacrifice some encapsulation

2. Leave everything the way it is. This will prevent you from writing very fine-grained tests, but doesn't require you to sacrifice any encapsulation.

Personally, I would choose (2), because you shouldn't really need to test private methods. The class should be tested through it's public interface (which in turn will call the private methods. Testing private methods can result in brittle tests, i.e. tests that fail when only the internal behavior of a class changes.

There is a third option (which I'm reluctant to mention): use reflection (or some other voodoo) to invoke private methods within your test class. This has the disadvantages of (1) and also the disadvantages intrinsic to reflective code (e.g. bypasses type checking and is hard to read)

Peterkin

could also use a subclass defined in the test case which calls the protected method, so test classes dont need to be in the same pacakge as implementation – Ulane

Answer 8

As Don and Dror say, making the methods public so you can create unit tests for them breaks encapsulation. You then tie yourself to a particular implementation. By making them public, you declare to the world that these methods are part of the published interface and therefore their specifications are locked.

Personally, I'd go for a more pragmatic solution: Keep them private and don't write unit tests. If you get to a case where a public method fails and you can't figure out why but you think it might be a problem in one of your private methods, then temporarily make them public, write the unit test, debug, and when you're done, make them private again and comment out the unit test.

Tamboura

Answer 9

You unit test those helper methods by writing unit tests that exercise those portions of your code.

You defined a public API in this class for a reason, right? Test that. If it works, the class works.

Use code coverage tools to help you know whether a sufficient portion of that class is being tested, and if it's not, write more unit tests to exercise the public API and hit those corners that aren't being covered.

Urinate