Code reusability/inheritance introduces pointless testing

Question

Say I have a bunch of classes that imitate cars: SportsCar, Truck, and SUV. All of these classes share some public methods like start() and stop() which they inherit from an abstract class Car. While the classes have their differences, imagine they all share some form of private functionality. They all might use the same private method openTrunk() or pumpFuelToEngine(). I don't want to have to write a openTrunk() method in each class, so I put them in Car as protected/package-private method instead for code reusability.

According to what I have been taught so far unit tests should test the public / protected / package-private interface of a class. These previously trivial and private methods are now reachable for any (test-)class inside of the package, which means they are subject to testing. It seems to me these tests become useless, or even harmful, as they test implementation and not behaviour.

• Do I skip testing these types of methods, or is there some design solution or unit testing principle I am overlooking?
• Perhaps the common functionality of these methods imply they should belong to another class?

• Filip Milovanović: Declare those internals as separate classes. You can then either inherit them or compose them into your object. Then write tests for those classes once (if you're using inheritance and the base class is not itself instantiable, derive a dummy subclass for the purposes of the test), and write separate tests for the classes that use them internally, without testing those internals again. In other words, in this other set of tests, only test the abstract/high-level behavior that you want to remain unchanged if you took out those internals and replaced them with some other implementation.
• Filip Milovanović: "I have changed their visibility, therefore making them subject to testing, when that was not previously necessary" - while it probably isn't a bad idea to write a couple of tests, if the behavior is trivial or well-understood and hard to get wrong, and the risks are low, you could skip writing those tests. There's no testing police out there that will come and get you (though you might have to argue for it if you have to deal with particularly pedantic code reviews).
• freakish: The real problem is that you use inheritance. Why? This literally begs for composition. And solves your problem as a side effect: openTrunk() should be a method on trunk, not car. Car should be a collection of components, that's all.
• Rob: If you are going to use inheritance in your implementation, why wouldn't you also use inheritance in your tests? The CarTest base class tests the default implementations. TruckTest extends CarTest and may or may not override the test for openTrunk(), as appropriate. In any case, TruckTest will run all of the test methods (including those inherited from CarTest).
• JonasH: I do not think cars is a good example of inheritance used well. Even your own trivial example will not work for electric cars without fuel pumps. In most cases you should use composition over inheritance to reuse code. And if you need examples of inheritance it might be better to use real world examples.

Answer 1

Do i skip testing these types of methods ... ?

If you can give me coverage I'm fine with that.

Unit tests always go against an API. Not everything has to be considered part of that API. How you remove stuff from being considered part of that API is up to you. Insisting it's always every non-private method is just hiding behind a brain dead rule. One that predates coverage tools.

There should be some deterministic lump of code we call a unit under that API that might have many methods and objects hiding inside it. You give me tests that cover it well and I'm not going to insist that every accessible method be called directly. Just show me a convincing coverage report.

Now that said, I'm also not opposed to developing a private method with unit tests. The thing is, keep those tests to yourself. Don't commit them. Because while they may help you write this method they'll lock down an implementation choice later that we don't want to be slaves too. We want to test behavior as loosely as we can. That enables refactoring. It's not fun having to rewrite tests every time you change code.

But understand, I don't feel that way because the method is private. I feel that way because I know that method is not part of the API that we care about testing. I don't really care about why it's not part of the API. Private, public, whatever, if it's not needed by the abstraction that we need to work then get this noise away from me.

Why do I have to be this paranoid? Because people are cowards when it comes to deleting old useless tests. They'd rather drag them around. That makes me conservative with what I allow. Document when a test should die well and maybe I'll relax a little. Maybe.

_{I’ve suffered in many shops that clung to that brain dead rule. I’m glad to see it eroding. When you let it rule you it changes the way you code. Many times we could have simplified a problem by adding a new object but wouldn’t because of the extra testing it would cause. We resorted to procedural programming just because we were forced to blindly adhere to that rule. I’d love to see accessibility decisions less coupled to testing decisions. Just give me coverage.}

Answer 2

Focus on defect localization as a testing strategy; you want unit tests that pinpoint failure, rather that test for every possible scenario in every concrete class.

---

As an aside, I'm going to interpret "[concrete classes] all might use the same private method openTrunk() or pumpFuelToEngine()" to mean "protected methods" — if they were truly private then child classes couldn't call them.

---

I would opt for a two-pronged approach:

• Create child classes of Car just for testing those protected methods so your test code has a public entry point into the system under test. Try for 100% code coverage.
• Limit tests for the real concrete classes that cover deviations from the expected behavior of the parent class.

As an example, let's pretend for a moment that PickupTruck should throw an exception when opening the trunk because it has a tailgate instead (and we pretend we aren't violating the Liskov Substitution Principle or we are and don't care because reasons). Write a test to cover that exceptional condition for PickupTruck and then lean on the test coverage established in the previous bullet point for the other sub types of Car.

This strikes a balance between code coverage and "unit test all the things ever because someone said 'best practice'" while still affording you detect localization. The unit tests in bullet point one should spot defects with openTrunk while the tests in bullet point 2 spot defects in how child classes differ at a macro level for methods that ultimately call up to Car.openTrunk. It's a way to divide and conquer test coverage without needless permutations of duplicate tests.

• Ruben Rundström: Your answer was a good read! But in refrence to your aside: What i mean is each class has their own identical trivial private method openTrunk. In accordance with DRY, i move them to the superclass for code reusability. But in doing so i change their visibility and expose them to testing, which was not previously neccesary.
• Greg Burghardt: @RubenRundström - I didn't understand that these were duplicate private methods consolidated into a protected method on the base class. That process of refactoring would change my approach as opposed to having written the protected method to begin with. Now I'm solidly in the same camp as candied_orange and Basilevs. Pick one place to cram in all necessary test cases to get coverage. The trick is to remember that one place and change the tests if they no longer provide enough coverage.

Answer 3

You touch at a sore point with inheritance which is more fundamental that just the issue of testing: Whether protected members should be considered implementation details or part of the exposed interface of the class.

Both viewpoints can be valid depending on context. At the one extreme, imagine the Car class is published as an object library and many independent code bases inherit Car and rely on protected members like openTrunk(). If you make changes you want to be sure that openTrunk() does not change behavior in any way since this might break inheritors. (It is not enough to test concrete implementations like SportsCar because openTrunk() might have changed behavior and SportsCar have adapted to the new behavior.)

But if you have a closed set of inheritors and everything is in the same code base, then it is reasonable to consider protected members implementation details and only test the public surface of the concrete classes.

Answer 4

New interface methods were already covered by tests of previously public interface. As they are covered, no additional tests are needed.

Answer 5

If you are going to use inheritance in your implementation, why wouldn't you also use inheritance in your tests?

The CarTest base class tests the default implementations. TruckTest extends CarTest and may or may not override the test for openTrunk(), as appropriate. In any case, TruckTest will run all of the test methods (including those inherited from CarTest).

And you only end up writing new tests when you write new implementation code.