2

This question already has an answer here:

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

A diagram showing function 1 calling sub 1 and 2, while function 2 calling sub 2 and 3

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.

marked as duplicate by Doc Brown, gnat, Scant Roger Nov 19 '15 at 6:12

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

  • 1
  • "in both cases, each of the internally called functions has their own unit tests". Why? Avoid testing internal functions as they are implementation details. Test just the public APIs and your problem goes away. – David Arno Nov 18 '15 at 10:34
  • 2
    @DavidArno Good point. A) Because sometimes there quite a few permutations for each sub-function but these are independent; so it's easier to test each separately. B) Sometimes the sub-functions are reusable (called by more than one function) and there's always the chance such will be a future case (although the solution for this will be to make the sub-functions internal - closure or scope - to the calling function). – Izhaki Nov 18 '15 at 10:49
  • 1
    Honestly, where is the question in your question? Seems you have already understood your situation very well, now stop writing useless unit tests. – Doc Brown Nov 18 '15 at 11:55
  • @DocBrown That's good to know. I've added 'The question' section, also showing a graphical expression of the problem. – Izhaki Nov 18 '15 at 12:26
2

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().

  • 1
    IMHO, if this is the correct answer to your question depends on if you are a "true TDD believer", or if you believe that software development is more efficient when you do not apply 100% TDD. – Doc Brown Nov 18 '15 at 15:13
  • @DocBrown Not sure what 100% TDD means, but I definitely don't suggest that 100% test coverage is a viable aim; this has nothing to do with test coverage. The most important point is that people stop looking at tests as defence mechanism only - for this you don't need TDD, you can write tests as an afterthought. – Izhaki Nov 18 '15 at 15:17
  • 2
    @Izhaki: Aren't you now testing an implementation detail instead of what should happen? A better test would be setting up a list of requests, calling the method, and checking that the proper items were deselected, and the proper requests were made (possibly mocking the actual making of the requests/actual deselecting). You do not want to test that a function knows who to delegate to, but that it gets the job done. – Sjoerd Job Postmus Nov 18 '15 at 17:28
  • @SjoerdJobPostmus. One way to look at this is that you do end up testing an implementation detail for the sake of ensuring proper documentation and removing what would otherwise be unnecessary test redundancy (remember that in this case the whole equals its parts). But another way is a simple logical argument where Given X and Y, T will hold. Whether X is delegated (making it an implementation detail) or not is not important - what's important is that T holds. – Izhaki Nov 18 '15 at 17:54
  • 2
    @Izhaki: yes. But the T that must hold is of the form "items are no longer selected and requests have been made", not "I have asked somebody to deselect them and I have asked somebody else to do requests". Given the test you described, I still have to look up the separate tests to see what actually happens. For a great talk which is somewhat relevant, watch vimeo.com/68730418 ("467 tests, 0 failures, 0 confidence" by Katrina Owen). – Sjoerd Job Postmus Nov 18 '15 at 18:27

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