DEV Community: Pete Hodgson

Testing redux reducers - Story Tests

Pete Hodgson — Fri, 16 Aug 2019 15:54:29 +0000

This post is part of a series, covering techniques for testing redux reducers.

In the first post of this series we discovered that it's better to test reducers and actions together, as an integrated, cohesive unit - the duck. In the next part of the series we pulled selectors into the scope of that integrated unit. We also looked at how we can simulate different scenarios in a selector test by using a reducer to get our redux state into the appropriate shape. However, the way we performed that set up was a little clunky.

In this post we'll explore cleaner, more expressive technique for creating this valuable type of reducer unit test - ones which validate the high-level behavior of a reducer as it responds to a series of actions, by moving a store's through a corresponding sequence of state transitions.)

Testing stateful behavior

We'll return to the same todo list reducer that we've been working with throughout this series. We’re going to verify that this reducer supports adding duplicate todo items with identical text. Here’s a first pass at testing this behavior:

import {getTodoCounts} from './selectors';
import reducer, {addTodo} from './duck';

describe('todos duck', () => {
  it('supports multiple identical items', () => {
    let state = undefined;

    state = reducer(
      state,
      addTodo('identical todo')
    );

    expect(getTodoCounts(state).total).toEqual(1);

    state = reducer(
      state,
      addTodo('identical todo')
    );

    expect(getTodoCounts(state).total).toEqual(2);
  });
});

We start with an initial state (conventionally represented in redux as undefined). We then use the reducer to apply an addTodo action to that initial state, generating a new, second state containing one item. We confirm that this second state looks correct (leveraging selectors, as discussed previously). We then we take that second state and use the reducer again to apply a second addTodo action. This generates a third state which should contain two identical items, which we then verify.

I call these types of stateful unit tests Story Tests. Each test tells the story of a series of actions occurring over time, describing how those actions change the store's state as they are processed by a reducer. I've found Story Tests to be a really concise, expressive way to validate the core behavior of a reducer via its public API ¹.

You may have noticed that our test above doesn't follow the Arrange/Act/Assert structure that's often recommended for unit tests. Instead of a test invoking a single action and then asserting what happened, a Story Test invokes a series of actions, with assertions interspersed between these invocations. This violates a unit testing rule of thumb which some might consider sacrosanct - each unit test should only assert one thing. However, I have found that this is a case where it's OK to violate that guidance. When used appropriately, this style of test provides a very nice additional tool in my redux testing toolbelt, particularly handy when validating stateful behavior - as we often find ourselves wanting to do when testing reducers.

Streamlining our Stories

While I like this testing approach, I'm not happy with the manual juggling of states that we're currently having to do in that example test. That single state variable which we're repeatedly mutating through the course of the test feels a little clunky. Let's see if we can do better.

If we take a step back and squint a little, what we’re really doing in that test is re-implementing a redux store, repeatedly hand-cranking that state variable through our reducer as we use it to apply our sequence of actions. What would it look like if we embraced this similarity and replaced our hand-cranked impression of a store with an actual redux store?

import {createStore} from 'redux';

import {getTodoCounts} from './selectors';
import reducer, {addTodo} from './duck';

describe('todos duck', () => {
  it('supports multiple identical items', () => {
    const store = createStore(reducer);


    store.dispatch(addTodo('identical todo'));

    expect(getTodoCounts(store.getState()).total).toEqual(1);


    store.dispatch(addTodo('identical todo'));

    expect(getTodoCounts(store.getState()).total).toEqual(2);
  });
});

That's much better! We've removed a bunch of noise, boiling our test down to just showing the sequence of actions and the corresponding state expectations.

Note that this is different for alternative testing approaches using something like redux-mock-store. We’re using a standard redux store - not a test double of some kind - but it's acting as a sort of test harness - an isolated environment which we can use to validate the unit that we’re testing in a fairly integrated way, while still keeping our tests deterministic and fast.

This is my preferred approach to writing Story Tests for reducers. Each test sets a reducer up within a fresh redux store, then dispatches a series of actions into the store, using getState to verify the state transitions the reducer is making along the way.

Telling a longer Story

As I mentioned above, I've found Story Tests to be an expressive way to describe the state transitions that a series of actions will move our store through. However our Story Test example so far is still pretty anemic, involving just a couple of actions. Here's a sketched-out example which gives an idea of what a longer Story Test for our todo reducer might look like:

import {createStore} from 'redux';

import {getTodoCounts,isTodoCompleted} from './selectors';
import reducer, {addTodo,completeTodo,deleteCompletedTodos} from './duck';

describe('todos duck', () => {
  it('marks a todo as completed, then deletes the completed todo', () => {
    const store = createStore(reducer);


    store.dispatch(addTodo({text:'todo a', id:'todo-a'}));
    store.dispatch(addTodo({text:'todo b', id:'todo-b'}));

    expect(getTodoCounts(store.getState())).toEqual({
      total: 2,
      complete: 0,
      incomplete: 2
    });


    store.dispatch(completeTodo({id: 'todo-a'}));

    expect(getTodoCounts(store.getState())).toEqual({
      total: 2,
      complete: 1,
      incomplete: 1
    });
    expect(isTodoCompleted(store.getState(),'todo-a')).toBe(true);


    store.dispatch(deleteCompletedTodos());

    expect(getTodoCounts(store.getState())).toEqual({
      total: 1,
      complete: 0,
      incomplete: 1
    });
  });
});

Hopefully this gives a taste of the type of expressive reducer tests you can write with the Story Test approach. These are fast, decoupled unit tests which allow you to validate how our actions and reducers work together as an integrated unit, running inside a redux store just as they would in real application code.

I encourage you to give Story Tests a try yourself, and let me know how they work out!

Previously in this series I showed that action creators and selectors make up the public API for a "superduck" - a module encapsulating a reducer that manages a bucket of state, the actions that modify that state, and the selectors which query that state. ↩

Testing redux reducers - leveraging selectors

Pete Hodgson — Fri, 09 Aug 2019 17:38:54 +0000

This post is part of a series, covering techniques for testing redux reducers. In the first installment of the series We saw that it's better to test reducers and actions together as an integrated, cohesive unit. In this post we'll take this idea a little further and see what happens when we pull selectors into our reducer testing strategy. Specifically, we're going to see whether we can make our reducer unit tests even more expressive, by validating our expectations of redux state using selectors. We'll also look briefly at the inverse - using reducers to make our selector unit tests more expressive.

Completing some todos

Let's start by looking at an example reducer test to see whether we can improve it. As with the other posts in this series, we'll be testing a reducer that manages a list of todos. Specifically, in this example we're testing how we track which todos are complete:

import reducer, {addTodo,completeTodo} from './todos';

describe('todos duck', () => {
  describe('completing todos', () => {
    it('initializes new todos as not completed', () => {
      const todoId = 'todo-a';

      const initialState = undefined;
      const nextState = reducer(
        initialState,
        addTodo({id:todoId,text:'blah'})
      );

      expect(nextState).toEqual([
        {
          id: todoId,
          text: 'blah',
          completed: false
        }
      ]);
    });

    it('allows us to mark an existing todo as completed', () => {
      const todoId = 'todo-a';

      let state = undefined;
      state = reducer(
        state,
        addTodo({id:todoId,text:'blah'})
      );
      state = reducer(
        state,
        completeTodo({id:todoId})
      );

      expect(state).toEqual([
        {
          id: todoId,
          text: 'blah',
          completed: true
        }
      ]);
    });
  });
});

These tests do the job and are pretty readable, although they're a little verbose.

A reader looking at the expectations at the end of these tests might need a moment to understand which part of the expectation is relevant to these specific tests. Is the value of text important here? The answer is no. In the context of this test we don't care at all about that field. However, since we're using toEqual to verify our expectations we have to specify the value of every field, even though we only actually care about the completed field. Put another way, our tests are over-specified.

Over-specified tests are fragile tests

Not only are over-specified tests distracting, but they are also more fragile. Our expectations are explicitly checking every aspect of our state, which means that if we make any changes to that state shape we'll have to touch a bunch of tests that aren't relevant to the change. Let's say we decided to track creation time for each todo by adding a createdAt timestamp. This new field would break the over-specified expectations in these tests, and we'd have to come back and update each one.

There are a few options for resolving this. Instead of checking the entirety of the state, we could instead write multiple expectations which check the individual fields we care about - but that can get unwieldy pretty quickly. We could also look at using some more advanced features of Jest's matchers:

import reducer, {addTodo,completeTodo} from './todos';

describe('todos duck', () => {
  describe('completing todos', () => {
    it('initializes new todos as not completed', () => {
      const todoId = 'todo-a';

      const initialState = undefined;
      const nextState = reducer(
        initialState,
        addTodo({id:todoId,text:'blah'})
      );

      expect(nextState).toEqual([
        expect.objectContaining({     // 👈 fancier expectatations
          id: todoId,
          completed: false
        })
      ]);
    });

    it('allows us to mark an existing todo as completed', () => {
      const todoId = 'todo-a';

      let state = undefined;
      state = reducer(
        state,
        addTodo({id:todoId,text:'blah'})
      );
      state = reducer(
        state,
        completeTodo({id:todoId})
      );

      expect(state).toEqual([
        expect.objectContaining({     // 👈 fancier expectatations
          id: todoId,
          completed: true
        })
      ]);
    });
  });
});

This is perhaps a little nicer. We are no longer distracted by an expectation verifying the value of the text field when it's not relevant to the test at hand. More importantly, these tests are more resilient to unrelated changes - we wouldn't need to touch these tests if we added that createdAt timestamp to our state, for example.

However, we're also making our expectation statements more complex, and pulling in features that someone reading the code may well not be familiar with.

Moving to selectors

Let's try taking a different tack. Conceptually, what we're trying to do in these tests is verify the completed-ness of a todo after certain operations. If we were writing production code that wanted to look at the completed-ness of a todo, we wouldn't go spelunking around in our redux state directly - we'd use a selector to extract that information. For example, here's an isTodoCompleted selector:

export function isTodoCompleted(state,todoId){
  const targetTodo = state.find( (todo) => todo.id === todoId );
  return targetTodo.completed;
};

What would it look like if we started using selectors like this in our test code, just like we would in our production code?

import reducer, {addTodo,completeTodo} from './todos';
import {isTodoCompleted} from './selectors';

describe('todos duck', () => {
  describe('completing todos', () => {
    it('initializes new todos as not completed', () => {
      const todoId = 'todo-a';

      const initialState = undefined;
      const nextState = reducer(
        initialState,
        addTodo({id:todoId,text:'blah'})
      );

      // 👇 using a selector to query our state
      expect(isTodoCompleted(nextState,todoId)).toBe(false);
    });

    it('allows us to mark an existing todo as completed', () => {
      const todoId = 'todo-a';

      let state = undefined;
      state = reducer(
        state,
        addTodo({id:todoId,text:'blah'})
      );
      state = reducer(
        state,
        completeTodo({id:todoId})
      );

      // 👇 using a selector to query our state
      expect(isTodoCompleted(state,todoId)).toBe(true);
    });
  });
});

Now we're getting somewhere! These expectations are way easier to read, and more importantly they're a lot more resilient to changes in our production code. If we change the shape of our state in production code we'd necessarily have to update any associated selectors, which means that tests which are using those selectors would get the update "for free". No need to touch a single line of test code unless there's a material change in the external behavior of our redux code. This applies even if we were to make dramatic changes to the internal implementation of behavior that's under test. For example, we could move where we store the completed-ness of a todo to a totally different area of our redux state without needing to touch a single test. This is a big win, and in my opinion the biggest motivator for moving to a more integrated approach for testing redux reducers.

Testing selectors

We've just been looking at using selectors to verify the state produced by a reducer, but we can also do the inverse - use a reducer to set up state which we then use to verify a selector's behavior.

Here we have a new selector, getTodoCounts, which tells us how many todos we have that are complete vs incomplete:

export function getTodoCounts(state){
  const completedTodos = state.filter( (todo) => todo.completed );

  const total = state.length;
  const complete = completedTodos.length;
  const incomplete = total - complete;

  return {
    total,
    complete,
    incomplete
  };
}

To test this selector we want to get our redux state into a shape that simulates having a few todos in our list, one of which is completed. We could create that simulated state by hand in our test, but this would approach would have many of the same drawback of verifying state by hand in our reducer tests - it would be verbose, hard to read, and would make the test implementation pretty fragile - changes to the shape of our redux state would require changes to these tests. So, instead of hand-crafting this state, let's use our reducer instead:

import reducer, {addTodo,completeTodo} from './todos';
import {getTodoCounts} from './selectors';

describe('todos selectors', () => {
  it('correctly counts todos in various states', () => {
    let state = undefined;
    state = reducer( state, addTodo({id:'todo-a',text:'blah'}) );
    state = reducer( state, addTodo({id:'todo-b',text:'blah'}) );
    state = reducer( state, addTodo({id:'todo-c',text:'blah'}) );
    state = reducer( state, completeTodo({id:'todo-b'}) );

    expect(getTodoCounts(state)).toEqual({
      total: 3,
      complete: 1,
      incomplete: 2
    });
  });
});

This should be fairly self-explanatory. Starting from an empty initial state, we add three todos, complete one, and then verify that the counts that come out of our getTodoCounts match what just happened. This style of selector test has the same benefits that we saw with our reducer tests - the tests are more expressive and way less coupled to the internals of how we model the todo list state.

Couldn't we use snapshot testing?

Some folks might be wondering about Jest's snapshot testing features as another way to approach verifying the state that a reducer operation creates. Here's an example of how we could re-write the reducer tests we were looking at previously to use snapshots:

import reducer, {addTodo,completeTodo} from './todos';

describe('todos duck', () => {
  describe('completing todos', () => {
    it('initializes new todos as not completed', () => {
      const todoId = 'todo-a';

      const initialState = undefined;
      const nextState = reducer(
        initialState,
        addTodo({id:todoId,text:'blah'})
      );

      expect(nextState).toMatchSnapshot();
    });

    it('allows us to mark an existing todo as completed', () => {
      const todoId = 'todo-a';

      let state = undefined;
      state = reducer(
        state,
        addTodo({id:todoId,text:'blah'})
      );
      state = reducer(
        state,
        completeTodo({id:todoId})
      );

      expect(state).toMatchSnapshot();
    });
  });
});

I'm not a fan of this approach for two main reasons: these tests no longer communicate expected behavior, and they are extremely brittle. While these tests are certainly succinct, I would not characterize them as expressive. Looking at this test, can we tell what the these addTodo and completeTodo actions are supposed to do to our redux state? We can guess based on the descriptions of the tests and the names of the actions themselves, but the test itself doesn't communicate anything other than "it should work the same way it did last time". And the flip side of this issue is that if anything at all changes in the shape of the state, these tests will start to fail. The tests are fragile in the same way that our original tests using toEqual were. And while it's easy to re-snapshot the tests with jest --updateSnapshot, this is really just deodorant over the underlying problem - an over-specified test.

Drawbacks to using selectors

We've seen that when we use selectors in our reducer tests we get tests which are more expressive, and less brittle. But what are some of the drawbacks? One concern is that as our tests pull in more production code, they become less focused. If we get a failure from a reducer tests which is using a selector, we don't initially know whether the issue is with the reducer we're testing, or the selectors that we're using to help us test. We'd have to do some digging to figure that out.

This is an example of the more general set of tradeoffs you'd see along the spectrum between focused, isolated tests and broader, integrated tests:

more isolated	more integrated
focused: reason test failure is easier to locate	broader: failing test could have several causes
artificial: tend to rely on test doubles, chance that these don't reflect reality	realistic: real dependencies are used, tests are closer to production scenarios
brittle: changes in a dependency's code are more likely to require test changes	supple: changes to dependencies are less likely to require changes to test code

The best way to work within these tradeoffs is to create a blended test portfolio, where various styles of testing are compensating for the drawbacks of other styles.

Moving beyond ducks

At the start of this article I mentioned another post, which argued for including redux actions in our reducer tests, and showed how that revealed the shape of a larger organizational unit in our redux code - the duck. I pointed out how action creators form a sort of public interface for the reducer, and that we should be testing the duck as an integrated unit.

I would now take that argument a step further. From our exploration of testing strategies, it is clear to me that selectors for the missing second half of a duck's public interface.

This structure is quite reminiscent of a CQRS (Command Query Responsibility Segregation) architecture. Selectors are Queries, used to inspect the current state of our application, while Actions are Commands, used to effect changes to that state¹.

While the original proposal for the duck module doesn't mention selectors, I believe it makes sense to expand the scope of the duck module to also include selectors. I'm not the first to point this out.

Summarizing our options

In this article we've used four different approaches to validating expecations of redux state.

We used Jest's toEqual matcher to do exact equality matching:

expect(state).toEqual([
  {
    id: todoId,
    text: 'blah',
    completed: true
  }
]);

We used Jest's objectContaining matcher for more flexible, partial matching:

expect(state).toEqual([
  expect.objectContaining({
    id: todoId,
    completed: true
  })
]);

We used selectors:

expect(isTodoCompleted(state,todoId)).toBe(true);

And finally we tried out Jest's snapshot testing facility:

expect(state).toMatchSnapshot();

Looking at these options side-by-side, it's even clearer that selectors are the most expressive option. We've also seen that selectors keeping our tests more flexible, and resilient to production code refactoring.

Next time you're writing a reducer test, I hope you've consider using selectors to validate redux state. And let me know how it works out! And follow me here or on twitter to get notified when I publish the 3rd post in this series, focused on more expressive ways to write higher-level, stateful unit tests for reducers.

I find it quite interesting that redux, which hews closely to a functional programming style, ends up modeling Command/Query Seperation, which comes very much from an Object-Oriented perspective of software. My friend and former colleague Andrew Kiellor pointed this out to me. ↩

Testing redux reducers - embrace action creators

Pete Hodgson — Thu, 18 Jul 2019 14:37:53 +0000

One of the benefits of using redux is the ease of testing. Keeping state management separate from the rest of our application makes it easier to test in isolation.

That said, the mechanics of testing the various moving parts in a redux app - stores, reducers, actions, action creators, selectors - is not entirely obvious. One question that I see crop up frequently is how granular our tests should be. Should we test each of these moving parts in isolation, or in more integrated groupings?

Isolation by default

We'll explore some different options by testing a simple redux reducer. The reducer in question is responsible for managing a list of todos. Here’s the implementation we have so far, which only supports a single action that adds a todo to the list.

// actions.js
import * as types from './types';

export function addTodo(text){
  return {
    type: types.ADD_TODO,
    text
  };
}

// reducer.js
import * as types from './types';

const initialState = [];

export default function reducer(state=initialState,action){
  switch(action.type){
    case types.ADD_TODO:
      const newTodo = {
        text: action.text,
        completed: false
      };
      return [
        ...state,
        newTodo
      ];
  }
  return state;
};

We’ll start from the position that our tests should be as focused as possible, exercising small units of code in isolation. Here's a test like that for our reducer:

import * as types from './types';
import reducer from './reducer';

describe('todos reducer', () => {
  describe('adding todo items', () => {
    it('adds an item', () => {
      const action = {
        type: types.ADD_TODO,
        text: 'write tests'
      };
      const initialState = undefined;
      const nextState = reducer(initialState,action);

      expect(nextState).toEqual([
        {
          text: 'write tests',
          completed: false
        }
      ]);
    });
  });
});

and here's an isolated test for our addTodo action creator:

import * as types from './types';
import * as actions from './actions';

describe('todos actions', () => {
  test('addTodo', () => {
    const action = actions.addTodo('my new todo');

    expect(action).toEqual({
      type: types.ADD_TODO,
      text: 'my new todo'
    });
  });
});

These tests seem reasonable - in fact, they are extremely similar to the testing examples you’d find in the redux docs. They’re certainly focused on a small isolated chunk of code, and it’s pretty easy to understand what they’re doing.

However, this type of extremely fine-grained tests are not without their downsides. These fine-grained tests are quite verbose, and not as expressive as I’d like. The intention of the reducer test is a little lost amongst the boilerplate of creating an action of the right shape and then verifying the details of the state that the reducer produces when processing that action. Additionally, as we build out our application logic we’ll find ourselves having to write a lot of these type of tests. Despite being a strong proponent of testing (and having spent over a decade practicing TDD), I find myself quietly sighing as I copy-pasta boilerplate test code in order to build out new behavior.

Brittle tests

What’s more troubling is how brittle these tests are. Let’s consider what would happen if we decided to refactor the internal structure of the addTodo action to conform to the Flux Standard Action format. We’d have to change our action creator implementation, of course, along with the corresponding test:

import * as types from './types';

export function addTodo(text){
  return {
    type: types.ADD_TODO,
    payload: {            // 👈 add payload wrapper
      text
    }
  };
}

import * as types from './types';
import * as actions from './actions';

describe('todos actions', () => {
  test('addTodo', () => {
    const action = actions.addTodo('my new todo');

    expect(action).toEqual({
      type: types.ADD_TODO,
      payload: {
        text: 'my new todo'
      }
    });
  });
});

We’d also need to make a small change to our reducer implementation, so that it can still pluck information from the action correctly:

import * as types from './types';

const initialState = [];

export default function reducer(state=initialState,action){
  switch(action.type){
    case types.ADD_TODO:
      const newTodo = {
        text: action.payload.text, // 👈 one-line change
        completed: false
      };
      return [
        ...state,
        newTodo
      ];
  }
  return state;
};

Unfortunately, this one-line reducer change also means that we have to change every related test for the reducer:

import * as types from './types';
import reducer from './reducer';

describe('todos reducer', () => {
  describe('adding todo items', () => {
    it('adds an item', () => {
      const action = {
        type: types.ADD_TODO,
        // 👇 we also have change how we're building our action here in this test
        payload: {
          text: 'write tests'
        }
      };
      const initialState = undefined;
      const nextState = reducer(initialState,action);

      expect(nextState).toEqual([
        {
          text: 'write tests',
          completed: false
        }
      ]);
    });
  });
});

This is a shame, because this reducer test shouldn’t really care about an implementation detail like the action’s internal structure - the focus of the test is the behavior of the reducer, not the shape of the action. We're seeing a classic characteristic of brittle tests here - a refactoring change has forced us to update tests which aren’t directly associated with the change. Now, with our current test suite this isn’t a big deal - we only have one test to update - but in a real-world test suite these types of brittle tests become a serious liability. They add significant additional overhead to refactoring work, which in turn discourages engineers from making the types of small, opportunistic “keep the campground clean” improvements which help keep a codebase ever-green.

false-negative tests

There’s an even scarier issue with our current testing strategy, one that I had sort of glossed over. When we changed our action’s internal structure just now, our reducer was broken until we made a corresponding update to how the reducer was working with that action. However, our reducer tests would not have detected this and would have continued to pass, despite the fact that the reducer wouldn’t work when integrated into our real codebase. This is because our reducer tests aren't using the action creator that our regular production code uses.

This type of false-negative test result is a common challenge with isolated tests - the very fact that a unit is isolated from its dependencies during test means that the test won’t detect breaking changes in those dependencies.

Getting less isolated

Let's resolve these issues by adjusting our testing strategy. The problem with our initial approach is that our reducer tests are isolating our reducer from the action creator, causing false-negatives. What's more, this isolation was achieved by re-implementing our action creator logic within the tests, causing brittle tests.

Both these issues go away if we change our reducer tests to use the real action creator implementation, making the tests a little less isolated:

import * as actions from './actions';
import reducer from './reducer';

describe('todos reducer', () => {
  describe('adding todo items', () => {
    it('adds an item', () => {
      const initialState = undefined;
      const nextState = reducer(
        initialState,
        actions.addTodo('write tests') // 👈 use action creator
      );

      expect(nextState).toEqual([
        {
          text: 'write tests',
          completed: false
        }
      ]);
    });
  });
});

Reducer tests using this approach no longer need to be changed when the internal shape of the action changes. In addition, such tests will fail appropriately when a change is made in the action creator implementation without a corresponding change in the reducer implementation. Win!

The fact that we are no longer testing our reducer in isolation might raise concerns - we’re using an action creator which isn’t part of the unit under test. Can we even call these unit tests any more?

Such concerns might be valid if we considered the reducer in isolation as the unit that we’re testing. However, I’d argue that it doesn’t really make sense to consider the reducer as a cohesive unit on its own. A reducer is only ever used in concert with actions - actions that should always be created by action creators. It doesn’t make sense to consider these things in isolation, and so it doesn’t make sense to try and test them in isolation. Therefore, these broader tests which consider both action creators and reducer as within their scope are still unit tests. We’re still testing a unit; it’s just that the scope of the "unit" under test has widened a little to include both the reducer and the actions which that reducer is built to process.

Action creator tests are unnecessary

Now that our tests are covering the implementation of both the reducer and the action creator, the isolated action creator tests which we initially wrote have become redundant. We are already verifying the implementation of the action creator via our new, broader reducer tests - those tests would give us good feedback if we were to break the output of the action creator somehow. We no longer need to write or maintain action creator tests, reducing our test burden and making our test suite more supple. With those tests gone, we can now refactor the internal structure of an action without having to touch any tests, while still retaining the safety net of having tests fail if we change the implementation in the action creator without the correct corresponding changes in the reducer. A win-win.

Enter the duck

Let’s step back, take a look at how we've changed our testing strategy, and see what that might tell us about the system that we're testing.

We started with fine-grained, isolated tests which exercised our action creator and our reducer independently.

What we’ve moved to is a broader test which covers both action creator and reducer together, as a unit.

Our tests have led us to a realization that we shouldn’t consider our reducer and the actions that it processes as independent parts - they are a cohesive unit. In fact, we can consider the action creators as the public API for a reducer - they’re the interface that we interact with when we want to make use of a reducer - we never interact with a reducer directly.

It's always exciting when our tests lead us to a deeper understanding of the design of our code!

We’re not the first to walk this path. Erik Rasmussen came before us, coining the concept of a duck: a formalized modular grouping of reducer + actions.

When we start thinking of our reducers and actions being implemented as a single module (a duck), we see why directly testing the output of the action creators wasn’t a sound approach. The shape of the actions that the action creators create is an internal implementation detail of that duck. We shouldn’t test these internals directly, just as we wouldn’t directly test the private methods or internal state of a class in an Object-Oriented system. The fact that our isolated reducer tests were rolling their own action data structures is another example of the same violation of our module's private internals.

Ok, so now that we're sold on the concept of the duck, let's refactor our existing implementation to make it into a duck module:

//duck.js
const ADD_TODO = 'todos/ADD_TODO';

export function addTodo(text){
  return {
    type: ADD_TODO,
    payload: {
      text
    }
  };
}

const initialState = [];

export default function reducer(state=initialState,action){
  switch(action.type){
    case ADD_TODO:
      const newTodo = {
        text: action.payload.text,
        completed: false
      };
      return [
        ...state,
        newTodo
      ];
  }
  return state;
};

//duck.test.js
import reducer, {addTodo} from './duck';

describe('todos duck', () => {
  it('adds an item to an empty list', () => {
    const initialState = undefined;
    const nextState = reducer(
      initialState,
      addTodo('my initial todo')
    );

    expect(nextState).toEqual([
      {
        text: 'my initial todo',
        completed: false
      }
    ]);
  });
});

There's nothing exciting going on here, to be honest. We're just merging the various components of the duck together into a single file.

the duck is the right unit to test

We started this article in search of a good strategy for testing reducers and the actions they process, and I'd say we've found one. By treating these as a cohesive unit, we can create unit tests which are shorter, more expressive, and more resilient to refactoring and code cleanup, without losing any tests coverage. Along the way, our testing has led us to a realization of a nice way to modularize our redux code.

But why stop here? We can make our tests even better! I'll be publishing a follow-up post soon, in which we'll explore testing techniques for the more complex, stateful behavior in our reducers. We'll also see how we can make our reducer tests even more expressive and resilient by expanding the scope of our unit just a little more, pulling another part of redux in.

Follow me on twitter if you'd like to know when I publish that followup post.

6 Practices for Effective Pull Requests

Pete Hodgson — Tue, 28 May 2019 16:30:58 +0000

This article was originally posted on my blog

I once thought that code review was all about catching bugs, but I was wrong.

In this article I'll discuss the additional value that teams get from code review beyond defect detection, and how Pull Requests (PRs) can get in the way of maximizing that value. Understanding the drawbacks of PR-based workflows, we'll then discover a few simple practices which prevent pull requests from dragging down a team's efficiency.

Almost every team I've worked with in the last few years has been using some variant of Github Flow - a code development branching workflow where developers do their work on relatively short-lived feature branches, and use a pull request to request a code review of the changes on that branch from other members of their team, before the changes are merged into a shared integration branch (usually master, sometimes develop).

This methodology works pretty well for many teams - it's a straight-forward branching strategy which provides frequent integration into a shared branch, with some control and rigor added via the pre-merge code review. However, Github Flow is not without its drawbacks, particularly when followed dogmatically.

Practices for better PR-based code review

We'll dig deeper into some of the benefits and drawbacks of Github Flow - specifically around pull request-based code review - later in this article, with the goal of identifying practices that allow us to work most effectively within this model. Here's a sneak peek at some of those ideas:

Don't couple dev tasks 1:1 with pull requests. It's OK to split development of a feature across multiple pull requests. It's also OK to create pull requests that aren't associated with a specific task.

Use a naming convention to explicitly categorize your code review feedback. The PR submitter should know which code review comments are calling out must-fix issues, and which are simply stylistic feedback, without ambiguity.

Use tooling and conventions to reduce tedious arguments. Back-and-forth discussion over whether there should be a space before or after a brace is an extremely low-value activity. Pick a convention and enforce it with tooling that resolves these stylistic issues before changes ever get to code review.

Don't require code review. Shock! Horror! Perhaps a two-line copy change or a small update to some simple, well-understood logic can be reviewed after code has been merged?

Discuss the overall technical approach before coding, not after. No-one enjoys a code review where the conclusion is that the entire branch needs to be re-done because the approach was wrong.

Augment async code review with inline feedback. Practices like mob coding and pair programming provide an insanely tight feedback loop compared to pull requests - seconds vs. days.

With that preview aside, let's start by looking at why the Pull Request model has become so popular.

The Point in Pull Requests

To understand the motivation and benefits for these practices, let's first look in more detail at why pull requests are valuable in the first place.

Pull requests are used within Github Flow as the mechanism to orchestrate code review. But why do we need code review? What value does it provide?

Code review catches bugs

Additional brains looking at a set of code changes can spot errors or defects that the original author missed. At one time I considered this to be basically the point of code review - catching bugs - but over time I have come to realize that code review actually provides other benefits, which are as valuable as defect detection, if not more so.

Code review enforces norms

To illustrate these other benefits, let's take a look at some code review comments:

"Unnecessary whitespace"

"We should be using all caps for constant names"

"This feels like too much logic to be living in a controller class"

"I'm not sure we should be doing this work on the client"

The feedback in those comments varies widely in scope, from small coding style details - should a newline go before or after a method call - through to broad architectural decisions - should some logic live on the client or the server. However while the scope of these comments varies widely, all of them follow a common theme: calling out deviation from the team's agreed norms. These issues don't constitute bugs or defects per se, but they do represent important feedback that should be addressed. It turns out that keeping a codebase (and a team) aligned around coding and design conventions is a key benefit of code review.

Code review evolves norms

Here are some more code review examples, demonstrating another class of feedback:

"Oh, neat, this lookup table is much cleaner than the switch statement we had before."

"Hmm, I wasn't sure about doing the validation here, but now that I think about it this makes a lot more sense"

"We really need to decide whether we want constants like these to live in a central file or at the top of the file that references them"

These comments invite discussion. Rather than enforcing existing coding conventions, they are opening up the opportunity to evolve or adjust those conventions. This is the third benefit I see from code reviews - PR commentary can act as a sort of "town square", a shared space for a team to discuss new approaches and new norms.

Drawbacks of Pull Requests

We've seen that pull requests can act as a conduit to bring in the value of code review, but they also bring some drawbacks along for the ride.

Merge delay

A PR suffers from two types of delay. After a PR is created it can sit waiting for engineers to simply find time to start reviewing the code. Then once review starts there can be a period of back-and-forth as review comments are issued, changes made, and code re-reviewed. If a team is distributed across timezones, or has a particularly stringent review culture, then this back-and-forth can drag on for hours, days or even weeks!

This delay is extraordinarily expensive. Let's talk about why. Firstly, the changes in a PR grow stale as it waits for approval. The chance of expensive merge conflicts increases, as other changes continue to land on the integration branch in the meantime. More fundamentally, this code review delay represents a delay in completed work getting into production and actually delivering value to users. In the Lean way of thinking, this delay represents waste.

On a high-performing team this delay might add up to a few hours, but as I just mentioned I've seen teams where a PR can spend days or even weeks in review before it is finally accepted and merged. This is an extremely expensive overhead.

Focus on the wrong things

There's a strange phenomenon that can occur with the type of offline code review that PRs encourage. A reviewer will spend a lot of time on feedback for relatively minor issues - whitespace, naming conventions, and so on - while at the same time failing to provide any feedback at all on much more serious design or architectural problems in the PR.

Why is this? I suspect that the line-oriented commenting mechanism that pull requests provide is partly to blame, in that it pushes reviewers towards low-level, line-oriented feedback rather than higher-level design feedback which doesn't necessarily map to a specific set of lines in a PR's changeset.

Beyond the review feedback mechanics, there's also a lot of human psychology at play. A reviewer can be uncomfortable suggesting that the design approach that's been used is wrong and the whole PR should be reworked. It's easy to succumb to the Sunk Cost fallacy and feel reluctant to suggest "throwing away" the existing implementation, particularly if it's functional as is. It's also simply a tough thing to tell another human being that you work with that they got it wrong and need to go back to the drawing board. And so sometimes we punt, and don't make give the critical feedback that we should.

The asynchronous, textual nature of PR-based code review also makes it hard for a submitter to gauge the relative weight of each comment. Does a comment saying "naming seems confusing" mean that the naming must be improved before the PR can be merged, or that we should fix it in a future PR, or the next time we're in this area of the code, or just that the reviewer just commenting in passing that the naming didn't seem particularly clear?

The nature of PR-based code review also makes certain issues hard to spot during review. Creeping code duplication is a good example. When performing a code review primarily through the lens of what's changed, you might not spot that a new class is in fact 70% the same as four other classes that were copy-pasted from, or that a new unit test is extremely similar to some existing tests. This is because that existing code isn't readily visible when reviewing a diff that focuses on what's changed.

PRs are coupled to branches

With Github Flow we define the set of changes that we want to be code reviewed using branches. That's how the pull request mechanism works - you mark a branch as ready for review. There is always a direct correspondence between the set of changes on a branch and the changes that will be reviewed, as a unit. This coupling means that if a feature branch contains a large set of changes then all the changes must be tackled in one large code review. Splitting up the changes into several smaller reviews is possible, but only if the author is able to tease the changes apart into a sequence of independent feature branches, which can be integrated one by one into the shared branch. As it happens, there are quite a few other benefits to this more trunk-based approach, but it takes thought and care, and the tools and practices required to do this are not readily available to every development team.

This means that for many teams a complex feature means a large feature branch, which means a large code review changeset. Research indicates that code reviews of large changesets are less effective, but unfortunately the branch-oriented model that pull requests are based on makes it hard to divide the review of a large feature into smaller chunks.

Techniques for better PR-based code review

We've seen that code review serves a valuable purpose, and that Github Flow uses pull requests as the mechanism for code review. But we've also seen that there are drawbacks to PR-based code review. So we wonder how we can maximize the benefits of PRs while minimizing the downsides, and we're brought back to the practices for effective pull requests which I outlined at the start of this article.

Use fast-follow PRs

One drawback of PRs is the delay they introduce in getting changes merged. We can reduce that delay by not requiring every issue raised during code review to be addressed in the same PR. Instead, we can opt to fix just the critical, merge-blocking issues within the same PR, and fix other, non-critical issues in a fast-follow PR. This allows a PR submitter to merge their changes with less delay, while still addressing all issues raise in code review.

This approach requires a certain amount of discipline and trust within a team. PR submitters must have the discipline to deliver on their promise to address issues in a fast-follow PR, and reviewers must trust that they'll do so. However, I'd say that this is the sort of discipline and trust that's reasonable to expect from professional developers working in a team with a healthy dynamic. If engineers don't trust other engineers to act like adults then a team has more pressing issues to focus on than their pull request conventions.

Use a notation to categorize code review comments

We saw earlier that it can be hard to understand whether a code review comment is identifying a must-fix defect, or simply a deviation from agreed conventions. If we plan to only fix critical issues within the originating PR then we need a clear way to distinguish between critical and non-critical defects.

To cut through this ambiguity, I encourage teams to adopt a simple notation in their code review comments:

"BLOCKER: this API is not available on mobile browsers and will cause the phone to implode"

"FAST-FOLLOW: We really shouldn't have this business logic in a controller action - please pull it out into a domain class"

"NIT: I would probably have pulled this out into a little helper function"

The notation is hopefully fairly self-explanatory. A BLOCKER must be addressed before this PR can be merged. A FAST-FOLLOW must be addressed, but optionally via a follow-up PR. A NIT is feedback that the author can take or leave, or an opportunity to open up a discussion on team norms.

This notation doesn't need to be followed dogmatically. For example if a reviewer wants to leave a comment which simply compliments a design approach or implementation detail they should feel free to do so. Many teams that I've worked with would have benefited from a little more positive feedback in code review, along with the constructive criticism!

Use tools to enforce coding conventions

Engineers discussing the placement of punctuation within a code review is a very expensive way to enforce team norms. Tooling running within some sort of CI/CD infrastructure is a much more effective and consistent approach. Leverage static analysis tools such as linters, code formatters, etc. to automatically enforce team standards, and save the code review comments for meaningful discussion on more nuanced topics where machines can't (yet!) detect deviation from the team's agreed norms.

Break up dev tasks into multiple pull requests

Code review simply doesn't deliver as much value when review changesets are too large. Breaking a task up into multiple smaller PRs results in higher quality review feedback. Additionally, by breaking a development task up into multiple PRs we reduce the cost of delay, as the required changes can be reviewed and merged incrementally rather than having to wait for everything to be fully completed before anything can start being reviewed.

There are two ways we can break up our changes across multiple PRs. First, we can use trunk-based development techniques like feature-flagging, which allow us to incrementally deliver code changes to a shared integration branch before a full feature is complete.

Secondly, we can address non-critical issues that are raised in a code review using fast-follow PRs, rather than requiring all changes to be made against the original PR.

Encourage "campsite" PRs

The scouts have a rule about leaving the campsite cleaner than you found it. This "campsite rule", applied to software, is perhaps the single best trick to achieving an evergreen codebase; a codebase that does not degrade into a pile of unworkable spaghetti as it ages. Unfortunately when followed dogmatically, Github Flow tends to discourage the campsite rule, since developers are incentivized to keep the changes in their branch focused on the feature at hand, in order to reduce the size of the resulting code review, as well as to avoid the potential distraction of unrelated changes done as part of a "campsite cleanup". One compromise is to build a team culture of tackling this type of opportunistic refactoring in small, separate PRs - "campsite" PRs. This takes a little discipline, and a willingness to learn the necessary git-fu required to safely put your feature branch to one side and create a new branch when you come across a cleanup opportunity, but it's a team habit that will pay wonderful dividends over the long term life of your codebase.

Review the technical approach outside of PRs, early and often

We've seen that code review isn't a great mechanism for ensuring that a change is suitable from a broader perspective than the technical implementation. The format doesn't lend itself to this type of discussion, and fundamentally the feedback comes too late - after the implementation of those decisions are complete.

There are better forums for these broader technical discussions. Teams can institute "story kickoffs", where the developer of a feature huddles with the tech lead and a product person to discuss the details just before development begins.

In addition, setting up various opportunities for lightweight touchpoints as feature development gets underway allows the technical direction to be refined in the light of new information - most good plans benefit from some adjustment once we encounter the enemy. I personally like to establish "tech huddles"¹, a very brief tech-oriented chat straight after daily standup where team members can raise any interesting technical topics that have come up over the previous workday.

Even when a feature is complete and ready for review, teams shouldn't rely on pull requests as the only way of doing code review. Tapping someone on the shoulder (or firing up a quick screenshare) and doing an in-person review provides for a much higher-bandwidth discussion than a text box in a web browser. You can take this to the next level and perform "mob reviews", gathering all or part of the team in front of a monitor, projector, or screenshare to perform code review as a group. While this has value for critical changes that benefit from extra brains, it's also a good way to maximize opportunities for that third benefit of code review - evolving team norms. Getting the team together to talk through the pros and cons of a new approach can be much more productive - and much more inclusive - than discussing ideas via pull request comments.

Diminishing the importance of PRs

I started off this article discussing the benefits of pull requests. However, what we've seen is that PRs don't provide much intrinsic value at all - they mostly serve as a useful mechanism for code review. It's code review that provides the true value. Moreover, we've seen that while PRs are one way to get the value of code review there are other mechanisms too, which teams who follow Github Flow dogmatically could be missing out on.

Practices like mob coding, pair programming and good old-fashioned team discussion can provide a much higher-bandwidth alternative to pull request-based code review.

Additionally, while we use PRs as the organizing structure for code review, we don't need to force that structure to align 1:1 with our development tasks. We can opt to split development work across multiple PRs, reducing the expensive delay that asynchronous pre-merge code review can cause.

Thanks for reading. I hope you're excited to try out some of these ideas on your team soon!

I am a big fan of Tech Huddles, partly because I like to call them Tech Cuddles in a rushed voice and see how long it take for folks to catch on. A few teams have "embraced" this alternative nomenclature. ↩