TDD - Refactoring into black-box?

Question

I have a nontrivial service object developed with TDD. It started with a simple task: For an object from queue, construct an attempt for asynchronous processing. So I wrote a test around my constructAttempt() method:

void constructAttempt() {...}

There are numerous possible scenarios that need to be taken into consideration, so I have a dozen tests for this method.

---

Then I implemented what I really needed it to do: Scan the whole queue and construct a batch of attempts. So the code looks more like:

public void go() {
    for (QueuedItem item : getQueuedItems()) {
        constructAttempt(item);
    }
}

So I added a new test or two for this go() method.

---

Finally I discovered I needed some preprocessing which sometimes may affect constructAttempt(). Now the code looks more like:

public void go() {
    preprocess();
    for (QueuedItem item : getQueuedItems()) {
        constructAttempt(item);
    }
}

---

I have a few doubts about what I should do now.

Shall I keep the code as is, with constructAttempt(), preprocess() and go() tested independently? Why yes/why not? I risk not covering side effects of preprocessing and break encapsulation.

Or shall I refactor my whole test suite to only call go() (which is the only public method)? Why yes/why not? This would make tests a little bit more obscure, but on the other hand it would take all possible interactions into consideration. It would in fact become a black-box test using only the public API, what may not be in line with TDD.

Answer 1

I say let your test suite only call go() since it is the only public API. Which means once you have covered all scenarios for go method (which would include preprocess and queue) then it no longer matters if you change internal implementation. Your class stays correct as far as public use is concerned.

I hope you are using dependency inversion for classes used for preprocessing/queue stuff- that way you can independently test preprocessing and then mock it in your go() test.

Answer 2

The go method is really just orchestrating several interactions, and isn't very interesting in its own right. If you write your tests against go instead of your subordinate methods, the tests are likely be hideously complicated because you'll have to account for the combinatorial explosion of interactions between preprocess and constructAttempt (and maybe even getQueuedItems, though that sounds relatively simple).

Instead, you should write tests for the subordinate methods - and the tests for constructAttempt need to account for all of preprocess' potential effects. If you can't simulate those side-effects (by manipulating the underlying queue or a test double) refactor your class until you can.

Answer 3

@Jeff has it about right. What your really have is two responsibilities occurring in this object. You may want to pull the queued items into their own class. Push the preprocess and constructAttempt into individual items. IMHO when you have a class that deals with individual items and a list of items you have a code smell. The responsibilities are the list container acts on the items.

public void go() {
    for (QueuedItem item : getQueuedItems()) {
        item.preprocess();
        item.constructAttempt();
    }
}

Note: This is similar to working with a command object pattern

[EDIT 1a] This makes it really easy to test with mocking. The go method only need to tests with a single queue item or no queue items. Also each item now can have their individuals tests separate from the combinatorial explosion of the go.

[EDIT 1b] You might even be able to fold the preprocess into the item:

public void go() {
    for (QueuedItem asyncCommunication: getQueuedItems()) {
        asyncCommunication.attempt();
    }
}

Now you have a true command pattern.