2

Assuming OldModule.oldFunc, if we want to move oldFunc to NewModule and, for backward compatibility, keep oldFunc there merely calling NewModule.newFunc by passing the exact same arguments and doing no tampering whatsoever -- do we need to still write unit tests for oldFunc? Or will having unit tests for newFunc suffice? 

# Are unit tests still needed for oldFunc?
OldModule.oldFunc(args)
{
  return NewModule.newFunc(args)
}
Adelin
  • 281
  • 1
  • 6
  • 1
    Possible duplicate of [Is the testing method for testing a function by testing a function which calls it still unit test?](https://softwareengineering.stackexchange.com/questions/343441/is-the-testing-method-for-testing-a-function-by-testing-a-function-which-calls-i) – Christophe Oct 10 '18 at 06:48
  • 1
    Possible duplicate of [Where is the line between unit testing application logic and distrusting language constructs?](https://softwareengineering.stackexchange.com/questions/322909/where-is-the-line-between-unit-testing-application-logic-and-distrusting-languag) – gnat Oct 10 '18 at 07:19
  • **Yes** you need tests for functions which call another functions – Fabio Oct 11 '18 at 02:25

3 Answers3

6

Luckily for you, since the function is not doing a lot, it would be easy to test it as well. A few seconds you'll spend adding a unit test is worth a potential loss of a few minutes if someone inadvertently modifies the function, or if the function breaks, affected by a change somewhere else.

How could such a simple function possibly break?

An example of a regression

Let's take an example of a dynamically-typed language. You're in the middle of a refactoring for the last two weeks; some calls were already migrated to newFunc, and there are only a very few which still rely on oldFunc. New code is well tested, but oldFunc has no tests at all.

Your colleague starts checking a bug report. It appears that it is related to a narrow piece of code in which, among others, newFunc is called. In order to fix the bug, your colleague decides to add an argument to newFunc: it's now newFunc(args, isSomething) and can be called by specifying either True or False for isSomething.

Your colleague runs unit tests, and, one by one, corrects them by adding the missing parameter to the callers. The test suite is now all green, so he pushes the changes to production.

A few minutes later, the phone starts ringing...

Since you forgot to test oldFunc, the change wasn't caught up by the test suite. Your colleague introduced a regression because the code was untested. By simply looking at the code, your colleague had no clue that he needed to update oldFunc and all the callers of oldFunc.

What to test?

As you may guess, there is no need to write all unit tests of newFunc for oldFunc: tests of oldFunc should test only what the actual function is doing, not what some other function which is called is performing.

This means that you should limit yourself to possibly one test.

Further changes

If the old function is kept only temporarily, you may—if the language has a support for it—decorate the function as obsolete to make the compiler produce a set of warnings for each location in the code which still uses the old function. The next step might be to replace the call to the new function by an exception, to catch a few locations where the function was called, without being caught by the compiler; the test will therefore be modified accordingly to check that the exception was thrown.

Arseni Mourzenko
  • 134,780
  • 31
  • 343
  • 513
  • The constraint of dynamically-typed languages in your answer makes it look like saying "in case of a strictly typed-language, an extra test is not necessary". Sure that is what you wanted to say? – Doc Brown Oct 10 '18 at 12:06
  • @DocBrown: if my example is converted to languages such as Java or C++, the problem of a missing argument will be obvious, since the compiler will catch it. This doesn't mean, however, that the test becomes unnecessary: the remaining part of my answer still applies to those languages. – Arseni Mourzenko Oct 10 '18 at 12:13
4

If you have a function, then you should have tests for it. This recommendation varies in strength according to the probability of defects in a function. For instance, auto-generated getters and setters with only one line are right at the bottom of the priority list, while business-critical, long, complicated functions need the hell tested out of them.

What does this mean for your legacy delegator? It's trivial and therefore unlikely to have defects; however, it does have a job to do, and you should test that it does this job. Depending on the nature of your test suite it might be easier to just adjust a copy the test of the test for the old function, or you might construct a mock and verify that oldFunc does, in fact, call newFunc with the same arguments.

But I would not omit testing just because the current incarnation of a point of functionality is trivial. That is an implementation detail which might easily change - after all, it did change drastically just now! And when it does change, it's unlikely that someone notices that the informal decision "we don't need to test this because it's trivial" will be remembered and revised. So it's better to write a simple test and keep your coverage.

Kilian Foth
  • 107,706
  • 45
  • 295
  • 310
  • There's no inherent value to testing this method. Underlying dependencies should be mocked anyway, so if `NewModule.newFunc(args)` is mocked, you have literally nothing worth testing anymore. And if you argue that you're really just testing how `NewModule.newFunc(args)` (unmocked) works, then you should be testing `NewModule.newFunc(args)` and not `OldModule.oldFunc(args)`. While I can agree that the rule (as it is currently stipulated) suggests that a test should be written, reality intervenes. There is (provably) no benefit gained from testing this method, and it should thus not be tested. – Flater Oct 10 '18 at 08:49
  • 1
    @Flater: the function is still doing *something*, i.e. performing a call and, moreover, passing arguments to it, so there are at least two regressions possible here. As I explained in my answer below, the function may later be changed to throw an exception, and this change would be reflected in the test as well. As soon as the function (1) can have regressions and (2) its logic may be altered, it can and probably should be tested. – Arseni Mourzenko Oct 10 '18 at 10:36
  • @ArseniMourzenko: You're applying to rule to the letter, but missing the point where there's no actual benefit to applying it. What would you test? The effect of calling `NewModule.newFunc`? That should be tested by testing `NewModule.newFunc` itself. Or are you testing that the `NewModule.newFunc` method is called? That is going to effectively lead to a test which mocks the entire method body - which is pointless. The `OldModule.oldFunc` method simply does not contain anything that is meaningfully testable. If it e.g. modified the args, that would be meaningully testable - but it doesn't. – Flater Oct 10 '18 at 10:48
  • 2
    @Flater: there is no actual benefit? What about... avoiding regressions? In order to avoid sterile arguments, I edited my answer below and added a very concrete example of a very concrete regression which could happen very easily (in a dynamically-typed language). In such case, spending a few seconds writing the test saves you from answering a phone call and explaining why the application crashed in production. IMO, the benefit is rather clear. – Arseni Mourzenko Oct 10 '18 at 11:15
3

Are unit tests needed for a function that only calls another function?

Yes.

Assuming OldModule.oldFunc, if we want to move oldFunc to NewModule and, for backward compatibility, keep oldFunc there merely calling NewModule.newFunc by passing the exact same arguments and doing no tampering whatsoever -- do we need to still write unit tests for oldFunc?

If oldFunc is still part of the supported public API, then there should be tests to ensure that its behavior is correct. The fact that it delegates all of its work is an implementation detail that is subject to refactoring.

The usual sequence for removing tests is:

  • Deprecate the old API, and schedule its removal
  • Wait until the grace period ends
  • Remove the tests
  • Remove the API

If the API hasn't been published, you have a lot more freedom in tearing things down.

VoiceOfUnreason
  • 32,131
  • 2
  • 42
  • 79