Testing a function that only calls one or two other functions

Question

The short version

The code

As part of TDD, we often end up with functions that follow this pattern:

function onSendRequestForSelected() {
    this.deselectStepsWithRequest();
    this.sendRequestForSelected();
}

Each of the internally called functions has their own unit tests.

The tests

Our unit tests for this function end up pretty much ensuring the function body calls were made:

expect( iStep.deselectStepsWithRequest ).toHaveBeenCalled();
expect( iStep.sendRequestForSelected ).toHaveBeenCalled();

The problem

The problem is:

Our tests end up testing function calls rather than behaviour.
We're unsure what is exactly the purpose of these tests - they protect from nothing other than any future changes.
Nor do they serve as a design documentation.
They clearly test the how rather than what.

The long version

Starting point

We start with the test:

it( 'should send a request for each of the selected steps', ... )

And then implement:

function onSendRequestForSelected() { ... }

So far so good.

New requirement, refactoring

Then we realise we need to deselect steps that already have a request.

So we refactor:

it( 'should send a request for each of the selected steps', ... )

becomes the test for

function sendRequestForSelected() { ... }

and create

it( 'should deselect steps that already has a request', ... )

implemented by

function deselectStepsWithRequest() { ... }

The rubbish test

and then introduce:

function onSendRequestForSelected() {
    this.deselectStepsWithRequest();
    this.sendRequestForSelected();
}

but no idea how to test it.

it( 'should send a request for each of the selected steps', ... )

makes little sense as the test itself only ensure a function is called.

it( 'should call sendRequestForSelected()', ... )

seems like a rubbish test.

The question

So it seems you end up with two options, neither is ideal. Which one is better?

Given TDD we start with:

it( 'should deselect steps that already has a request', ... )
it( 'should send a request for each of the selected steps', ... )

function onSendRequestForSelected() {
    ...
}

Option 1

it( 'should deselect steps that already has a request', ... )
it( 'should send a request for each of the selected steps', ... )

function onSendRequestForSelected() {
    this.deselectStepsWithRequest();
    this.sendRequestForSelected();
}

Problems:

The actual sub-functions are not tests (if they would they would be redundant). Dangerous if they will be reused (consider them being needed by other functions).

Option 2

// No test

function onSendRequestForSelected() {
    // Test for each of the following
    this.deselectStepsWithRequest();
    this.sendRequestForSelected();
}

Problems:

This is not really TDD; this is not how we started.
No test to serve as a design documentation. We do not define the behaviour.

In graphical form

Where do you drop the tests?

Testing F1 and F2 only will yield redundant tests for S2.
Testing for S1, S2, and S3 will not test for the behaviour of F1 and F2.

score 3 · Answer 1 · edited Apr 12 '17 at 07:31

This is obviously a controversial topic.

Having consulted a multitude of opinions, the conclusion is that this is correct:

Given the method

function onSendRequestForSelected();

and its pre-implementation tests:

it( 'should deselect steps that already has a request', ... )
it( 'should send a request for each of the selected steps', ... )

the eventual implementation shall be:

function onSendRequestForSelected() {
    this.deselectStepsWithRequest();
    this.sendRequestForSelected();
}

and the eventual tests shall be:

it( 'should deselect steps that already has a request', function(){
    expect( iStep.deselectStepsWithRequest ).toHaveBeenCalled();
});

it( 'should send a request for each of the selected steps', function(){
    expect( iStep.sendRequestForSelected ).toHaveBeenCalled();
});

Reasoning

Much of the reasoning behind this is based on my argument why you should write a test for every method (well nearly), including setters and getters.

In short:

TDD is not only about defence, but also design and documentation.
For design and documentation purposes, tests shall be written before implementation.
- This is a fundamental principle of TDD so it may sound elementary.
- But the question needed to be asked is: "Can someone implement this part of the system based on the provided tests?".
- So if there is no test, there will be no implementation.
Test are subject to refactoring and extractions much as implementation code is.

Test extraction

So basically, the expect line here:

it( 'should deselect steps that already has a request', function(){
    expect( iStep.deselectStepsWithRequest ).toHaveBeenCalled();
});

is a way of saying:

The test condition, instead of being satisfied here will be satisfied by calling deselectStepsWithRequest()

(for which there is a separate test).

Is like code extraction

much in a way this.deselectStepsWithRequest() in here:

function onSendRequestForSelected() {
    this.deselectStepsWithRequest();
}

is a way of saying

Instead the execution code being here, it is in deselectStepsWithRequest().