You are doing TDD wrong

TDD (Test Driven Development) is often described as a process that focuses on creating unit tests before writing the code to make those tests pass. As a software engineer it seems to make sense and be a sensible way of ensuring your code functions the way it should and continues to remain that way.

The process of TDD encourages us to use short iterative cycles where we write a test, make it fail, write some code to make it pass and then refactor the code (which now has a test case to ensure the refactor doesn’t introduce a regression), thereby improving the code.

Practicing TDD has become the badge of a mature, experienced and capable software engineering team.

So what’s wrong?

To be clear I’m an advocate of writing unit tests, in my opinion the test coverage of your code base should be 100%. In my experience however when teams talk about TDD they focus on the process rather than the outcome or the value.

Good code can be a difficult and subjective thing to quantify and for the sake of this discussion I want to qualify it as 3 things:

1. Robust — it does what it’s expected to do with no bugs
2. Easy to understand — anyone reading the code can easily understand it
3. Extensible — it is easy to change, extend and modify with minimum risk or negative impact on unrelated code

TDD as a process only really focuses on making your code robust, so being a practitioner of TDD does not mean your code is inherently better or good even. Its not uncommon for teams that practice TDD to have hard to understand code that is not resilient to requirement change.

There are a couple of things wrong with the TDD mindset.

Tests are the most important thing

By making us write tests first, and then the code to honour them, TDD tries to make tests a first class citizen by forcing us to think of them as the most important thing in the code base. The problem with this is that the test has become the contract to how our code should behave, there is a sense that the test is an immutable precious source of truth.

I prefer to make tests first class citizens by treating them as equal partners to the code that honours them. One way we can do this is by shifting the focus to our code and making it easier to test. I’ll outline an example pattern I like to use below but ultimately by making code more testable we will likely write better code as a result.

Iterating the wrong thing

The iterative process within TDD focuses on refactoring the code that fulfils the test. This means we are focused at a very micro level on the individual units of our code, ie the low level things we have unit tests for (typically functions and classes), rather than iterating on the higher level design of how these units fit together and depend on each other.

Use fundamental patterns and an iterative mindset to improve code design

Programming is a naturally iterative and creative process. It is unlikely that the first solution you write will be the best solution or design of your system. TDD tries to recognise this but falls short by locking you into a way of thinking that is too low level.

I think teams need to focus on what it means to write good code, and what the value of good code actually is.

Once the conversation is focused on code quality we can start to understand how we can better design our code. As I mentioned earlier, making code easier to test often results in better code. I’ll try and demonstrate that next.

Focusing on making code easier to test will improve its quality

The TDD thinking approach

Imagine we are creating a system that needs to connect to an SFTP server, download some files and then do some processing of them. Different engineers will look at this problem from different starting points, regardless, we know we need 3 things:

1. A way to connect to an SFTP server and get files
2. Something that needs to process those files
3. A way of orchestrating these 2 things (such as an AWS lambda function handler)

A common approach, and one well suited to TDD, might be to solve the difficult problems first — ie the logic to connect to and get files from the SFTP server.

When practicing TDD we might start off with some tests that look something like this:

// a function whose responsibility it is to get files from an SftpServer
// and return a collection of files
const getFilesFromSftpServer = (): File[] => {};

describe('Test getFilesFromSftpServer()', () => {
  it('should connect to the sftp server and get the files', () => {});
  it('should throw an error if the username is wrong', () => {});
  it('should throw an error if the password is wrong', () => {});
});

We would then write some code to fulfil these tests and iterate on other scenarios like incorrect host name, port, failure to copy, efficiency, parallel downloads, etc etc, improving and refactoring the code until we have a very robust and beautiful piece of code that is capable of getting the files we need from the SFTP server.

We then move on to the next step of processing the files:

// a function whose responsilbility is to process a collection of files
const processFiles = (files: File[]) => {};

describe('Test processFiles()', () => {
  it('should save the same number of files it is given', () => {});
  it('should throw an error if it fails to process a file', () => {});
});

Then we tie these 2 things together in some orchestration or handling:

const handler = () => {};

describe('Test handler()', () => {
  it('should call getFilesFromSftpServer() and pass the result to processFiles()', () => {});
});

With this approach we have seemingly robust code (it is well covered with unit tests) and the code in this simple example is easily understood, but how extensible is it?

If you take the actual amount of code it might take to fulfil what I’m trying to demonstrate here, and even extrapolate it out into a bigger system we’d end up with a non-trivial amount of code / time / effort. Its easy to see the sunk cost fallacy at play in situations like this, ie; we’ve invested so much time and energy into this design / solution that we are unwilling to redesign / rewrite / abandon it.

By sinking all our time and energy early on in solving the lowest level problems, we are committing early on to a design and approach that might cause us issues later on.

The design thinking approach

Rather than starting with the lowest level code I advocate taking a step back and looking at how each of the elements might depend on each other and fit together.

Using the 3 things we know we need (a way of getting files from the sftp server, a way of processing the files, and orchestration / a handler) we could start with an initial design like this:

Iteration 1 — first idea based on current assumptions

// define an interface for the function that gets files from our sftp server
// the list of files returned will be an imaginary File object
// at the moment we don't know if there are any inputs or parameters in 
// terms of how to get the files 
export interface GetFilesFromSftpServer {
  (): File[];
}

// define an interface for the function that does the processing
// at the moment we don't know what it might return so we leave it as void
export interface FileProcessor {
  (files: File[]): void;
}

// define a handler that takes these 2 interfaces as dependencies
export const handler = (
  getFiles: GetFilesFromSftpServer,
  processFiles: FileProcessor
) => {
  const files = getFiles();
  processFiles(files);
}

Although we have only written a small amount of code, it is still fully testable and represents a valid design.

As the GetFilesFromSftpServer and FileProcessor are just interfaces with no implementation we can write a test for our design and validate it:

describe('Test handler()', () => {
  it('should call processFiles() with the files returned from getFiles()', () => {
    
    const filesOnServer = [mock<File>(), mock<File>()];

    // as the GetFilesFromSftpServer and FileProcessor are interafces 
    // we can mock them
    const getFilesFromSftpServer = mock<GetFilesFromSftpServer>();
    getFilesFromSftpServer.mockReturnValue(filesOnServer);
    const processFiles = mock<ProcessFiles>();
    
    // execute the unit under test
    handler(getFilesFromSftpServer, processFiles);
    
    expect(processFiles).toHaveBeenCalledWith(filesOnServer);
  });
});

Although the tests are tightly coupled to the code design (they describe exactly what the code is doing through the mock definitions), they do not define our system contracts — the interfaces and the code does that.

We often see interfaces as something only required if you are likely to have multiple different implementations of that thing. Here however we are using them as a tool to allow us to validate our design decisions early on.

Iteration 2 — implement the SFTP server interaction

As we are happy with the initial high level design, let’s move on to some of the lower level implementation and write some code that will connect to an SFTP server and get the files.

We decide on a popular 3rd party package acme/sftp-client to handle the low-level connection, we just need to write some “wrapper” code:

import { SftpClient } from 'acme/sftp-client'; 

export const getFilesFromSftpServer: GetFilesFromSftpServer = (): File[] {
  const sftpClient = new SftpClient(
    process.env.SFTP_HOST, 
    process.env.SFTP_PORT, 
    process.env.SFTP_USER, 
    process.env.SFTP_PASSWORD
  );
  
  sftpClient.connect();
  const files = sftpClient.get();
  sftpClient.disconnect();

  return files;
}

Hang on, how are we going to test this? The main problem is we are are creating an instance of the SftpClient from within the thing we want to test so have no control over it’s behaviour or any way of observing or mocking it. We are also binding the configuration of the SftpClient to process.env.

We could handle this with things like global mocks or stubs but as we have the goal of making our code as easy to test as possible, let’s refactor it by slightly modifying our interface:

import { SftpClient } from 'acme/sftp-client'; 

// make the interface enforce the need of a getFiles function rather than the
// interface BE the function
export interface GetFilesFromSftpServer {
  getFiles(): File[];
}

// this change allows our implemntation to be a class with the SftpClient 
// injected as a dependency
export class AcmeSftpFileRepository implements GetFilesFromSftpServer {
  constructor(private readonly sftpClient: SftpClient) {}

  public getFiles(): File[] {  
    this.sftpClient.connect();
    const files = this.sftpClient.get();
    this.sftpClient.disconnect();
  
    return files;
  }
}

Testing this is now easy as we can inject a mock of the SftpClient:

import { SftpClient } from 'acme/sftp-client';

describe('test AcmeSftpFileRepository.getFiles()', () => {
  it('should call connect, get, disconnect and return the result of get', () => {
    const files = [mock<File>()];
    
    // we can observe the SftpClient and control its behaviour for our test
    const acmeClient = mock<SftpClient>();
    acmeClient.get.mockReturnValue(files);

    const acmeSftpClient = new AcmeSftpFileRepository(acmeClient);
    
    expect(acmeSftpClient.getFiles()).toStrictEqual(files);
    expect(acmeClient.connect).toHaveBeenCalledOnce();
    expect(acmeClient.get).toHaveBeenCalledOnce();
    expect(acmeClient.disconnect).toHaveBeenCalledOnce();
  });
});

As the tests are tightly coupled to our code, we also need to change the tests for our handler()function to account for the change to the interface:

describe('Test handler()', () => {
  it('should call processFiles() with the files returned from getFiles()', () => {
    
    ...

    // we've added the getFiles method to the interafce
    getFilesFromSftpServer.getFiles.mockReturnValue(filesOnServer);

    ...
  });
});

Iteration 3 — clarify the intent

Because interfaces are intentionally devoid of any implementation they are naturally decoupled. The name we have for GetFilesFromSftpServer implies a coupling with an SFTP server / implementation, but if we look at it’s contract the only requirement is that it should return a collection of Files.

It’s now only a semantic change to make this code really extensible and allow us to change the mechanism by which we get files in the future:

// rather than this name:
// export interface GetFilesFromSftpServer {
//   getFiles(): File[];
// }

// we can rename the interface like this to explicitly decouple it from SFTP
// and describe what it is:
export interface FileRepository {
  getFiles(): File[];
}

It’s now trivial for us to implement other ways of getting files if we wish:

class AwsS3FileRepository implements FileRepository {
  construct(private readonly s3Client: S3Client) {}
  public getFiles(): Files[] {}
}

The end design after all the iterations looks like this (omitting the FileRepository implementations and tests for clarity):

export interface FileRepository {
  getFiles(): File[];
}

export interface FileProcessor {
  (files: File[]): void;
}

export const handler = (
  fileRepository: FileRepository,
  processFiles: FileProcessor
) => {
  const files = fileRepository.getFiles();
  processFiles(files);
}

Hopefully you can see that the outcome we’ve arrived at is some high quality code according to our previous definition:

1. Robust — we’ve validated it does what we expect it to do, and it’s resilient to regressions as it has full unit test coverage.
2. Easy to understand — clear contracts define the interactions of each piece of code and the unit tests act as documentation in terms of how we expect the code to function.
3. Extensible — we’ve created clear abstractions and dependencies that are isolated from each other. We can extend functionality, for example by adding additional ways of getting file without modifying any handler / domain logic code.

We’re still heavily relying on a testing feedback cycle to iterate the code but the process is focused on designing an extensible, understandable system rather than achieving a fully tested system.

By focusing on making the code more testable we can use the unit testing process as a tool to aid in and improve our design.