29

Let's say I start developing a role game with characters that attack other characters and that kind of stuff.

Applying TDD, I make some test cases to test the logic inside Character.receiveAttack(Int) method. Something like this:

@Test
fun healthIsReducedWhenCharacterIsAttacked() {
    val c = Character(100) //arg is the health
    c.receiveAttack(50) //arg is the suffered attack damage
    assertThat(c.health, is(50));
}

Say I have 10 methods testing receiveAttack method. Now, I add a method Character.attack(Character) (that calls receiveAttack method), and after some TDD cycles testing it, I make a decision: Character.receiveAttack(Int) should be private.

What happens with previous 10 test-cases? Should I delete them? Should I keep method public (I don't think so)?

This question is not about how to test private methods but how to deal with them after a re-design when applying TDD

7
  • 2
    Possible duplicate of Testing private methods as protected
    – gnat
    Commented Oct 3, 2017 at 8:52
  • 10
    If it's private you don't test it, it's that easy. Remove and do the refactor dance
    – kayess
    Commented Oct 3, 2017 at 8:54
  • 6
    I'm probably going against the grain here. But, I generally avoid private methods at all costs. I prefer more tests than less tests. I know what people are thinking "What, so you never have any kind of functionality you don't want to expose to the consumer?". Yes, I have plenty I don't want to expose. Instead, when I have a private method I instead refactor it into it's own class and use said class from the original class. The new class can be marked as internal or your language's equivalent to still prevent it being exposed. In fact Kevin Cline's answer is this kind of approach.
    – user9993
    Commented Oct 3, 2017 at 10:44
  • 3
    @user9993 you seem to have it backwards. If it's important for you to have more tests, the only way to ensure that you have not missed anything important is to run coverage analysis. And for coverage tools it doesn't matter at all if the method is private or public or whatever else. Hoping that making stuff public will somehow compensate for the lack of coverage analysis gives a false sense of security I'm afraid
    – gnat
    Commented Oct 3, 2017 at 13:36
  • 2
    @gnat But I never said anything about "not having coverage"? My comment about "I prefer more tests than less tests" should have made that obvious. Not sure what you are getting at exactly, of course I also test the code I've extracted. That's the whole point.
    – user9993
    Commented Oct 3, 2017 at 14:12

3 Answers 3

52

In TDD, the tests serve as executable documentation of your design. Your design changed, so obviously, your documentation must, too!

Note that, in TDD, the only way in which the attack method could have appeared, is as the result of making a failing test pass. Which means, attack is being tested by some other test. Which means that indirectly receiveAttack is covered by attack's tests. Ideally, any change to receiveAttack should break at least one of attack's tests.

And if it doesn't, then there is functionality in receiveAttack that is no longer needed and should no longer exist!

So, since receiveAttack is already tested through attack, it doesn't matter whether or not you keep your tests. If your testing framework makes it easy to test private methods, and if you decide to test private methods, then you can keep them. But you can also delete them without losing test coverage and confidence.

12
  • 14
    This is a good answer, save for "If your testing framework makes it easy to test private methods, and if you decide to test private methods, then you can keep them." Private methods are implementation details and should never, ever be directly tested.
    – David Arno
    Commented Oct 3, 2017 at 9:55
  • 20
    @DavidArno: I disagree, by that reasoning the internals of a module should never be tested. However, the internals of a module can be of great complexity and therefore having unit-tests for each individual internal functionality can be valuable. Unit-tests are used to check the invariants of a piece of functionality, if a private method has invariants (pre-conditions/post-conditions) then a unit-test can be valuable. Commented Oct 3, 2017 at 12:16
  • 8
    "by that reasoning the internals of a module should never be tested". Those internals should never be directly tested. All tests should only test public APIs. If an internal element is unreachable via a public API, then delete it as it does nothing.
    – David Arno
    Commented Oct 3, 2017 at 12:18
  • 28
    @DavidArno By that logic, if you're building an executable (rather than a library) then you should have no unit tests at all. - "Function calls are not part of the public API! Only command line arguments are! If an internal function of your program is unreachable via a command line argument, then delete it as it does nothing." -- While private functions aren't part of the public API of the class, they're part of the internal API of the class. And while you don't necessarily need to test the internal API of a class, you can, using the same logic for testing the internal API of an executable.
    – R.M.
    Commented Oct 3, 2017 at 12:37
  • 7
    @R.M., if I were to create an executable in a non-modular fashion, then I would be forced into choosing between brittle tests of the internals, or only using integration tests using the executable and runtime I/O. Therefore by my actual logic, rather than your strawman version, I would create it in a modular fashion (eg via a set of libraries). The public APIs of those modules can then be tested in a non-brittle fashion.
    – David Arno
    Commented Oct 3, 2017 at 12:56
23

If the method is complex enough to need testing, it should be public in some class. So you refactor from:

public class X {
  private int complexity(...) {
    ...
  }
  public void somethingElse() {
    int c = complexity(...);
  }
}

to:

public class Complexity {
  public int calculate(...) {
    ...
  }
}

public class X {
  private Complexity complexity;
  public X(Complexity complexity) { // dependency injection happiness
    this.complexity = complexity;
  }

  public void something() {
    int c = complexity.calculate(...);
  }
}

Move the current test for X.complexity to ComplexityTest. Then text X.something by mocking Complexity.

In my experience, refactoring toward smaller classes and shorter methods pays huge benefits. They are easier to understand, easier to test, and end up being reused more than one might expect.

6
  • Your answer explains much more clearly the idea I was trying to explain in my comment on OP's question. Nice answer.
    – user9993
    Commented Oct 3, 2017 at 10:48
  • 3
    Thanks for your answer. Actually, receiveAttack method is quite simple (this.health = this.health - attackDamage). Perhaps extract it to another class is an overengineered solution, for this moment.
    – Héctor
    Commented Oct 3, 2017 at 11:03
  • 1
    This is definitely way overkill for the OP - he wants to drive to the store, not fly to the moon - but a good solution in the general case.
    – user22815
    Commented Oct 3, 2017 at 15:36
  • If the function is that simple maybe it's overengineering that it even is defined as a function in the first place.
    – David K
    Commented Oct 4, 2017 at 0:05
  • 1
    it may be overkill today but in 6 month's time when there are a ton of changes to this code the benefits will be clear. And in any decent IDE these days surely extraction of some code to a separate class should be a couple of keystrokes at most hardly an over-engineered solution considering that in the runtime binary it will all boil down to the same anyway. Commented Oct 4, 2017 at 13:42
6

Say I have 10 methods testing receiveAttack method. Now, I add a method Character.attack(Character) (that calls receiveAttack method), and after some TDD cycles testing it, I make a decision: Character.receiveAttack(Int) should be private.

Something to keep in mind here is that the decision you are making is to remove a method from the API. The courtesies of backwards compatibility would suggest

  1. If you don't need to remove it, then leave it in the API
  2. If you don't need to remove it yet, then mark it as deprecated and if possible document when the end of life will happen
  3. If you do need to remove it, then you have a major version change

The tests get removed / or replaced when your API no longer supports the method. At that point, the private method is an implementation detail that you should be able to refactor away.

At that point, you are back in the standard question of whether your test suite should be directly accessing implementations, rather interacting purely through the public API. A private method is something that we should be able to replace without the test suite getting in the way. So I would expect the tests couple to it to go away -- either getting retired, or moving with the implementation to a separately testable component.

1
  • 3
    Deprecation is not always a concern. From the question: "Let's say I start developing..." if the software is not released yet, deprecation is a non-issue. Furthermore: "a role game" implies this is not a reusable library, but binary software aimed at end-users. While some end-user software does have a public API (e.g. MS Office), most does not. Even the software that does have a public API only has a portion of it exposed for plugins, scripting (e.g. games with LUA extension), or other functions. Still, it is worth bringing up the idea for the general case that the OP describes.
    – user22815
    Commented Oct 3, 2017 at 15:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.