TDD and Unit Testing for new code, but existing patterns

Question

How should I go about using TDD and writing tests in this scenario?

Suppose we have request handler classes. In short, a handler applies a "request" (e.g. Update details) to an entity then saves the result to the repo.

However, the existing handlers follow this sort of implementation pattern (simplified):

// You can safely assume that tests for the repository and mappers exist
public class FooHandler
{
    IRepository<Foo> _repository;
    IMapper _mapper;
    
    public FooHandler(IRepository<Foo> repository, IMapper mapper)
    {
        _repository = repository;
        _mapper = mapper;
    }

    public void Handle(FooRequest request)
    {
        var fooItem = _repository.Get(request.Id);
        _mapper.Map(fooItem, request);
        _repository.Save(fooItem);
    }
}

Now my task is to implement a new handler, e.g. BarHandler, following the lines of FooHandler.

I understand what the intended behaviour should be so the implementation shouldn't matter i.e. instead of using a mapper I can manually "apply" the request, but here we have an existing implementation pattern, so what should I actually be testing here? What would be a good starting point for a test if I'm using TDD? Should I write a basic test for the behaviour then refactor toward the existing implementation? What about the mapper? Should I mock the behaviour, or use the real object in the Test Fixture setup?

I think I need to add more detail to this question. Assuming I were to use TDD, would my process look something like this:

// First failing test
[Test]
public void Handle_Should_Update_Repository_Item()
{
    var request = new FooRequest(id: 1, newName: "newName");
    
    // private field constructed in Fixture setup
    _mockRepo.Setup(m => m.Get(1)).Returns(new Foo(id: 1, name: "existingName"))

    // private field constructed in Fixture setup
    _handler.Handle(request);

    // If I'm only testing the intended behaviour, I shouldn't need to check that
    // that mapper was called?
    _mockRepo.Verify(m => m.Save(It.Is<Foo>(f => f.Name == "newName")), Times.Once);
}

My uncertainties/questions are:

1. I have to change the test after the refactor to ensure that the mapper is called. This appears to go against TDD (don't modify tests during refactor)
2. I can avoid this by modifying the Fixture setup, but does this also imply modifying the Test?
3. If I inject a mapper in the Fixture setup, should I use a mock that emulates the expected behaviour so that my test still passes, or should I inject the real object?

All of this assumes that my proposed initial test within the context of TDD is correct. If it's not correct, what should I be doing? Should I only be testing that the expected methods on the interface mocks are called so that it falls in line with the pre-existing implementation pattern, as opposed to testing the behaviour?

Answer 1

Kent Beck made an important observation in 2008

I get paid for code that works, not for tests, so my philosophy is to test as little as possible to reach a given level of confidence

For code that is "so simple that there are obviously no deficiencies", a reasonable "testing" strategy is to review the code (preferably with a partner), then sign off on it, and move on to something important.

This is especially true when you are looking at code that is unlikely to change in the future.

---

With this code, there are a small number of refactorings we might consider. For instance, it wouldn't be unreasonable to extract the protocol into its own function...

public class FooHandler
{
    IRepository<FooRepository> _repository;
    IMapper _mapper;
    
    public FooHandler(IRepository<FooRepository> repository, IMapper mapper)
    {
        _repository = repository;
        _mapper = mapper;
    }

    public void Handle(FooRequest request)
    {
        DoTheRealWork(
            _repository,
            _mapper,
            request
        )
    }

    static void DoTheRealWork(
            IRepository<FooRepository> repository, 
            IMapper mapper, 
            FooRequest request
    ) {
        var fooItem = repository.Get(request.Id);
        mapper.Map(fooItem, request);
        repository.Save(fooItem);
    }
}

Riddle: how confident are you about refactoring the code like this without introducing a subtle error that the compiler doesn't catch for you?

Similarly: if these three lines of code are appearing often (because this is your teams "framework" for managing change, you are very close to being able to lift this code into a common class (with a generic repository and a generic request. Again, is having a test here going to catch errors that you make refactoring the code from one shape to another?

---

If you did need to test drive this code (because it is more complicated, or because regulations, or whatever), the tool you will likely be leaning on is Endo-testing, aka Mocks.

Framing: what you have here is a very simple protocol: objects sending messages to other objects. The testing goal is to ensure that the correct messages are sent.

In this example, Repository::Save is the important message (that's the one that produces observable side effects), and you'd presumably want to check a few things (a) that the fooItem passed to Save is the same item that was returned by Repository::Get and (b) that the fooItem is "dirty", having been modified by Mapper::Map.

So your test of FooHandler would wire up an instance to substitute implementations of Repository and Mapper whose behaviors are (a) easy to configure and (b) easy to measure, and you would assert that the protocol is implemented correctly.

The tests to ensure that the Repository and Mapper do the right thing would live "somewhere else".

The definitive reference for this testing approach is Growing Object Oriented Software by Pryce and Freeman. Magic Tricks of Testing by Sandi Metz is a good introduction to these ideas.

Answer 2

Now that you updated your question, it seems to be about how write an incremental test following TDD guidelines if you first want to ignore the mapper-related part of the logic, but add it later.

TDD guidelines are just that - guidelines. If you're confident about the design and about how things would work, you can choose to break them. Yes, sometimes this will come back to bite you, but so what?

So one option is to just write the test for the whole logic at the start - in this particular case, it's fairly simple. You're just testing that the appropriate calls are made. Inject a mock mapper (that just returns the passed object without doing anything, and records the call, and does nothing else), make the call in your code-under-test, then proceed to code around it. (You mentioned in your question that the mapper and the repository are tested elsewhere - good, those tests don't belong here.)

Or you could add only the repository assert, write code for that first, then change the test later to involve the mapper as well. (Sometimes, this can be done by adding separate test cases, but in this particular scenario, these calls made on their own without other context don't make much sense.)

I have to change the test after the refactor to ensure that the mapper is called. This appears to go against TDD (don't modify tests during refactor)

Well, you're not doing refactoring here. You're not in the refactor step of the TDD cycle. Refactoring is changing internal structure without affecting behavior and without changing the outside-facing API. Here, you're restructuring/redesigning the outside-facing API.

This means that you're in the red step - you are adding a new test. (Modifying an existing test is effectively the same as throwing away the old test and writing a new one.) So, it's fine. Add the assertion, run the test to make sure it fails, then go into the code to make it pass. So, change the test first, then the code! Once the test is green, then you can proceed to the refactor step - to do cleanup of what you've written, make the code more readable, etc.

---

Original answer, for reference:

When writing tests, you have to consider how the class will actually be used, in the sense of how the code that uses it (client code) will actually look like when written. Treat the test as a small, contrived example of such a client. Note that the same class can be used in different ways, and keep in mind that different users might have different expectations and may rely on different things. For example, the code that creates and passes the dependencies is more coupled to FooHandler than the code that calls the handle method (although they can be the same).

To help you understand what how you'd go about writing a test for BarHandler, let me first discuss FooHandler.

So, from the way it's written, its design communicates that you'd use this class by supplying the two dependencies, and then calling Handle at some point, possibly in a different part of the system.

Now here's the kicker - if you look closely, the only externally observable behavior of this class is that a call to Handle results in (1) a call to _repository.Get with the externally supplied request id, then (2) a call to _mapper.Map with the object returned by the repository, and (3) a call to _repository.Save on the same object (by some definition of "same" - same instance, or same value, same id, ...). It's externally observable in the sense that these dependencies are externally supplied and at the boundary of the object, where it interfaces with the external world.

Now, you might say, but those are all implementation details - except they are not (but see the caveat below). That is the core peace of business logic this class is responsible for, it's just that this particular implementation doesn't do much else. Encapsulation doesn't mean "create an inscrutable black box that does dark magic". These dependencies are part of the API of this class. You'd definitely write about what gets called on them in the documentation of FooHandler. It's part of its specification, it's fundamentally related to what FooHandler is for, to the service it provides.

The code that ultimately wants to make use of the class would have to create these dependencies, and pass them along - that code relies on these specific behaviors, on the promise that these calls will be made. If you don't consider these calls to be the behavior on interest, then all you're left with is the Handle method, which then basically just eats some input, making the FooHandler class an overly abstract black box that is hard to reason about - because then it has no externally visible behavior that client code can be written against.

---

Anything beyond that, anything else that the class might do around and in between these calls would be internal details that both clients and the tests should not know about, as well as any internal state that the class would use to facilitate this additional logic.

---

The code that actually calls the Handle method may treat it as a black box and can be written to have no knowledge of the implementation, but the overall correctness of the system depends on this class exhibiting these behaviors on the supplied dependencies.

So, for this particular class, you'd write something like this (think of this as a sort of annotated test-pseudocode):

    // ====== Test setup (a.k.a. "arrange", a.k.a. "given") ======
    
    FooItem fooItem = new FooItem(id: 123)
    
    // This just returns the fake foo item you supplied, 
    // records if the calls to Get and Save were made, and **does nothing else**
    MockFooRepository fooRepo = new MockFooRepository(() => fooItem);
    
    // The mock mapper: records if the call to Map was made, does nothing else
    MockMapper mapper = new MockMapper();
    
    FooHandler fooHandler = new FooHandler(fooRepo, mapper);
    
    // ====== The actual test (a.k.a. "act", a.k.a. "when") ======
    
    Request dummyRequest = new Request(...);
    fooHandler.Handle(dummyRequest);
    
    // ====== Checks (a.k.a. "assert", a.k.a. "then") ======
    
    Assert.IsTrue(fooRepo.GetWasCalledWith(dummyRequest.Id);
    Assert.IsTrue(mapper.MapWasCalledWith(fooItem, dummyRequest));
    Assert.IsTrue(fooRepo.SaveWasCalledWith(fooItem));


    // Note: these mocks are simple and can be hand-written, 
    //       or you can use a mocking library

It's actually a fairly short test - here's the same thing without all the annotations (more readable):

    FooItem fooItem = new FooItem(id: 123)
    MockFooRepository fooRepo = new MockFooRepository(() => fooItem);
    MockMapper mapper = new MockMapper();
    
    FooHandler fooHandler = new FooHandler(fooRepo, mapper);
    
    Request dummyRequest = new Request(...);
    fooHandler.Handle(dummyRequest);
    
    Assert.IsTrue(fooRepo.GetWasCalledWith(dummyRequest.Id));
    Assert.IsTrue(mapper.MapWasCalledWith(fooItem, dummyRequest));
    Assert.IsTrue(fooRepo.SaveWasCalledWith(fooItem));

Caveat: Note that, if the core responsibility of FooHandler is not to orchestrate all of these calls (its specification doesn't make the promise that the supplied dependencies will be called), then some of these asserts (most likely fooRepo, and maybe mapper asserts) would not be necessary, and doing them would be an overspecification - which is bad for refactoring. Even though nothing about the code is different. So you have to think about what the class is actually for, and which side effects the calling code relies on. In other words, think about what would you write in the documentation for this class to tell someone who has never seen it before how to use it.

Now, you said

I understand what the intended behaviour should be so the implementation shouldn't matter i.e. instead of using a mapper I can manually "apply" the request

Not quite, there's a subtlety there. When it comes to FooHandler, assuming that the contract of this class is such that it promises the orchestration of these calls, then other classes that use it rely on the fact that they can pass their own mapper so that they can have control over the mapping process. In other words, removing or ignoring the mapper potentially breaks other code. Unless you explicitly state in the documentation of the class that the mapper may or may not be used depending on some hidden logic internal to FooHandler - but that doesn't seem very useful in general. What's more useful is if there's another class that explicitly doesn't use a mapper (maybe BarHandler), that clients can pick to be used when and where they need it. So a test for BarHandler might look slightly different.

However, note that the code/component that actually calls the Handle method doesn't itself have to rely on this particular behavior, or know about any of these dependencies. You can create the dependencies in one place, create the FooHandler instance, and pass it fully formed to some other piece of code. That code doesn't care if you do the mapping manually. It might not even care what the handler does - it's only job might be to invoke the Handle method at the appropriate time. Fire and forget.

So that code can take an IHandler, and you can give it a FooHandler, or a BarHandler that uses no mapper, or a handler wrapped in another handler, etc., without breaking anything. In this particular case, the IHandler interface just has the Handle method (and can potentially be a lambda instead of an interface, or you can have a helper that creates an IHandler from a lambda).

There would probably be a test somewhere that would check if this client code calls IHandler.Handle, but that's not a test of FooHandler or BarHandler - it belongs in a different set of tests for a different component (the client).

Answer 3

In this case the intended behaviour is probably a repository change on some request given some mapper.

Usually I use mocks, but for the IMapper you probably might write a simple class in the test, and a repository probably has to hold things too, do real work. This of course is the overhead. Sometimes it is easier to use a embedded database, when there are many data. Real database extracts need anonymisation.

Now for a derived BarHandler it might be that some quite specific FooHandler behaviour is required/assumed. This could result in adding some tests to FooHandler. Actually specifying some requirements more concretely.

And then you are developing the BarHandler. Even if the API does not change, the calls may use some caching, speed improvement expected. This you may test. It is the perfect place for a micro-benchmark (but short one, or later-on ignored test).

You should not copy all tests of FooHandler that would evidently still hold. But some might be significant. Different behaviour requires new tests; maybe you can adapt some tests from FooHandler.

I am not a fan of complete test coverage. But the above is workable, fruitful, immediately bug saving and instigates confidence in the code quality.