8

If I have a complex unit tested function:

def do_everything():
    # turn twizzles
    # push buttons
    # move mountain

And I re-factor it into some smaller units:

def do_everything():
    turn_twizzles()
    push_buttons()
    move_mountain()

def turn_twizzles():
    # turn twizzles

def push_buttons():
    # push buttons

def move_mountain():
    # move mountain

Am I wasting my time writing extra unit tests for those smaller units?

Adam Terrey
  • 191

4 Answers4

15

I assume you already have unit tests covering the behavior of do_everything()? If you have broken turn_twizzles() etc. out as private methods, then you have not changed any external behavior so you don't need to change any tests.

If however turn_twizzles() is made public, then you have introduced a new functionality (as observed from outside the class) so it would be valuable to test this.

JacquesB
  • 59,334
  • 2
    Not sure why this answer has been downvoted. It is short, to the point and 100% spot-on in its advice. – David Arno Nov 17 '17 at 17:12
  • This is a very clear answer, thank you. I think the issue (credit to @Samuel for mentioning it fist) is the issue of weather or not the public interface has changed. – Adam Terrey Nov 17 '17 at 22:44
  • Downvote because this is not complete. Even if the new functions were not part of a public API, writing tests can be very helpful. Say a test for do_everything() fails: where is the error? If you have tests for all three sub-functions it will be way easier to find. That's a benefit of writing the tests and it should be mentioned here. – marstato Nov 18 '17 at 13:25
  • 1
    @marstato: It is generally considered a bad idea to unittest private methods, since it couples the tests to implementation details. – JacquesB Nov 18 '17 at 15:14
  • @JaxquesB That's not generally a bad thing. You could say that anything more precise than a usual integration test is tied to implementation detail. But you write those tests anyways because they help you track bugs, not because they help you prove correct functionality. The access modifier is always subjective. A private keyword in a programming language is just one of many ways of restricting access to a piece of code. – marstato Nov 19 '17 at 01:41
  • @marstato: How is access modifiers subjective? If a method is private it cannot be accessed externally and is therefore an implementation detail by definition. – JacquesB Nov 19 '17 at 16:49
  • @JacquesB it always depends on context. E.g. I could say that all code in a program that is not part of a user interface is implementation detail (e.g. in an MVC app, everything not a controller is implementation detail). Then, by your logic, I'd only need to write integration tests. Those tests will prove whether the software fulfills all requirements but won't be any help in finding and fixing bugs. Thus, we write code for smaller pieces of code to test in isolation. That also applies to those sub functions. – marstato Nov 19 '17 at 17:06
  • @marstato: In the context of unit tests, "implementation details" means details which is not part of the public contract of the unit which you are testing. If a class is so big and convoluted that testing against the public interface is not sufficient to help you find bugs, then the class is almost certainly doing to much and should be refactored. Apply a bit of "separation of concerns" and "single responsibility principle". (To be fair, the method names in the example does indicate this might be the case!) – JacquesB Nov 19 '17 at 17:20
  • @JacquesB i agree that the sub methods are dependencies of do_everything and a test should never be concerned about the correctness of the subjects dependencies. But whether the dependencies need tests themselves is another story. I disagree with If a class is so big and convoluted that testing against the public interface is not sufficient to help you find bugs. More often than not a very small public interface is desireable (e.g. when writing a library) but at the same time, it makes sense to split the task up and both keep all code for the task close together. – marstato Nov 19 '17 at 17:34
12

It depends. To be more precise, it depends on

  • the complexity of the original function (if it was very complex, testing each parts on its own will pay off)
  • the complexity of the smaller functions (if they are complex parts on its own, testing them individually will lead to more fine-grained tests and more precise root-cause detection in case of a defect)
  • the existing unit tests (if they already produce sufficient coverage for all those "parts", then it is probably less worth to write individual tests)
  • if you want to keep those smaller functions "implementation details" of the original functions, or not (for the former, writing unit tests for the smaller functions would become counter-productive for this goal).

Especially when those "smaller functions" are not that trivial as in your example, but will have a more or less complex list of input parameters, it can become really hard to produce enough unit tests for your original function to guarantee the smaller functions are tested with all "interesting" input combinations. That would be a clear sign to write specific unit tests for the smaller functions as well.

So, there is no clear "yes" or "no" to this question, it is a trade-off you have to decide per case .

Doc Brown
  • 206,877
6

If turn_twizzles, push_buttons, and move_mountain are public and called by other code, then I think it's important to refactor your tests to test these functions individually.

Unfortunately after your refactor you have a problem: to unit test do_everything you need to be able to mock turn_twizzles, push_buttons, and move_mountain. Writing tests for do_everything without mocking the dependencies will be an integration test--not necessarily a bad thing depending on your test plan, but there won't be much benefit because you're already testing the three smaller functions individually. This may be the right time for you to redesign this component and collaborate with other objects to do all the work of do_everything.

If turn_twizzles, push_buttons, and move_mountain are not called externally, they should be marked private, and I wouldn't recommend testing them separately from do_everything. This is because from an outside-looking-in perspective, do_everything would be the smallest unit (because the others are inaccessible). Also see this answer about breaking up methods using private methods.

Samuel
  • 9,187
  • 4
    I have downvoted this as, "to unit test do_everything you need to be able to mock turn_twizzles, push_buttons, and move_mountain..." depends on a nonsense definition of "unit test". – David Arno Nov 17 '17 at 17:14
  • 1
    @David to be fair, Samuel does acknowledge that in the answer. Sort of. – GnP Nov 17 '17 at 22:34
4

No. The extra unit tests are more precise. If move_mountain fails, then a single test would fail that says very specifically what went wrong.

That precision cuts down on debugging time, which is valuable. Also, since the test is more focused, it should be faster to run than testing the same functionality via the full function, providing quicker feedback, which is valuable.

Telastyn
  • 109,398