Writing Testable Code
TypeScript and JavaScript Edition

This one is my favourite. Mostly because I've never seen it articulated and written down so clearly before.

It is also very powerful as it influences code structure in fundamental way and paves the road for the rest of testable design.

The essence of the rule is to not have any logic in constructors besides setting instance variables. It follows that all your objects have to be constructed (and only constructed) at the start of your application. This phase could be understood as configuration part. Once everything is constructed and wired, execution part begins and business gets done. At this time only value (pure) objects can be created.

Applying this rule to my React projects lead to unorthodox architectures which some people found off-putting because of unfamiliar looks. The upside of it is that all of the components can be pure, easily storybookable and testable.

Following is an example of how it might look.

// ...
const map = new GoogolMap();
const MapView = mapView(map);
// ...
const App = app(storage, MapView, ...);

ReactDOM.render(<App />, document.getElementById(... ));

function mapView(map: MapServiceProvider) {
  return function MapView({longitude, latitude}) {
    const [mapUrl, setMapUrl] = useState(null);

    useEffect(async () => {
      setMapUrl(await map.getImageUrl(longitude, latitude));
    }, [longitude, latitude]);

    return <div>{mapUrl ? <img src={mapUrl}/> : SOME_DEFAULT_MAP_IMAGE}</div>;
  }
}

function app(storage, MapView) {
  return function App() {
    // ...
    const coordinates = storage.getCoordinates();

    return (
      <div>
	// ...
	<MapView {...coordinates} />
	// ...
      </div>
    );
  };
}

Global state is sneaky. It hides under mutable variables, static methods, singleton objects, and any dependency that is not passed as an argument. Global state is infectious. Single instance of it in a small module will corrupt all of your application.

Singleton pattern in OOP is well spread practice, so how come it's bad? Answer is that, unless global, it is not.

Let's take a look at small example of sneaky global state.

const cache = {};

function set(key, value) {
  return cache[key] = value;
}

function get(key) {
  return cache[key];
}

module.exports = {
  set,
  get,
};

const fetch = require('node-fetch');
const cache = require('./cache');

const BASE_URL = 'http://localhost:8080';

async function fetchPosts(since) {
  const timestamp = since.getTime();
  const KEY = 'posts_since_' + timestamp;
  let posts = cache.get(KEY);
  if (posts == null) {
    const response = await fetch(BASE_URL + '/posts?since=' + timestamp);
    if (response.status === 200) {
      posts = await response.json();
      cache.set(KEY, posts);
    } else {
      return {error: 'Cannot fetch posts'};
    }
  }
  return {result: posts};
}

module.exports = {
  fetchPosts,
};

const test = require('tape');
const proxyquire = require('proxyquire');

test('fetchPosts()', async (assert) => {
  const expectedPosts = [1, 2, 3];
  const expectedError = 'Cannot fetch posts';
  let fetchPostsModule = proxyquire('./fetch-posts', {
    'node-fetch': (url) =>
      Promise.resolve({status: 200, json: () => Promise.resolve(expectedPosts)})
  });

  assert.deepEqual(
    await fetchPostsModule.fetchPosts(new Date()),
    {result: expectedPosts},
    'successfully fetches posts'
  );

  fetchPostsModule = proxyquire('./fetch-posts', {
    'node-fetch': (url) => Promise.resolve({status: 500})
  });

  assert.deepEqual(
    await fetchPostsModule.fetchPosts(new Date()),
    {error: expectedError},
    'returns empty array and error on failure'
  );

  assert.end();
});

fetchPosts()

  ✔ successfully fetches posts
  ✔ returns empty array and error on failure


total:     2
passing:   2
duration:  25ms

Great success?

Not really. Even though tests have passed, we had to use proxyquire⁴ to tap into module system and override dependencies. This a neat trick, but ultimately it's a sign of global state issue. Modules are singletons, they're run only once. Even though it's pretty common to see such code in the wild, it's not testable or REPL friendly.

There is another catch – our tests are only accidentally successful. If they would run fast enough to produce the same timestamp, we would get data served from the cache, which is a singleton object for the whole application.

What does testable code look like?

First, Cache module exposes a function to create one.

function Cache() {
  const store = {};

  function set(key, value) {
    return store[key] = value;
  }

  function get(key) {
    return store[key];
  }

  return {
    set,
    get,
  };
}

module.exports = Cache;

The same goes for utility to fetch posts. It's a repository object now and can be configured with all of dependencies.

function PostsRepo(BASE_URL, fetch, cache) {
  async function query(since) {
    const timestamp = since.getTime();
    const KEY = 'posts_since_' + timestamp;
    let posts = cache.get(KEY);
    if (posts == null) {
      const response = await fetch(BASE_URL + '/posts?since=' + timestamp);
      if (response.status === 200) {
	posts = await response.json();
	cache.set(KEY, posts);
      } else {
	return {error: 'Cannot fetch posts'};
      }
    }
    return {result: posts};
  }

  return {
    query,
  }
}

module.exports = PostsRepo;

As a result, we no longer need proxyquire. It's also painfully clear, that we have a cache, and we should be careful with it.

const test = require('tape');

const Cache = require('./cache');
const PostsRepo = require('./posts-repo');

test('PostsRepo', (assert) => {
  function newPostsRepo(fetch) {
    return PostsRepo('/', fetch, Cache());
  }

  assert.test('fetch()', async () => {
    const expectedPosts = [1, 2, 3];
    const okFetch = () => Promise.resolve({
      status: 200,
      json: () => expectedPosts
    });
    assert.deepEqual(
      await newPostsRepo(okFetch).query(new Date()),
      {result: expectedPosts},
      'successfully fetches posts'
    );

    const failFetch = () => Promise.resolve({status: 500});
    assert.deepEqual(
      await newPostsRepo(failFetch).query(new Date()),
      {error: 'Cannot fetch posts'},
      'returns empty array and error on failure'
    );

    assert.end();
  });
});

Nice consequence of ditching proxyquire is that our tests run faster.

PostsRepo


fetch()

  ✔ successfully fetches posts
  ✔ returns empty array and error on failure


total:     2
passing:   2
duration:  11ms

This one is easy to spot. For example, if we see a second level access of property in your code like this

this.session.getAccount().getNickname()

we have a problem. It will become clear when writing a test for it. We will have to either mock or construct Account in addition to Session in all places where Session is a dependency. Renaming a method of Account will impact places where direct dependency is Session, not Account. While most IDEs have no problem with renaming, your code repository history will become noisy.

Let's take a look at the following example.

function App() {
  const session = useSession();
  return (
    <div>
      <div>Hello, {session?.account?.name ?? 'Anonymous'}!</div>
      // ...
    </div>
  );
}

Instead of chains of method calls we have nested property access exhibiting the same issue. To avoid digging into session, we might introduce a selector getAccountName or pass only necessary data to child component.

Here's how second solution might look.

function App() {
  const session = useSession();
  return (
    <div>
      <Greet account={session.account} />
      // ...
    </div>
  );
}

Both of the solutions are related to separating concerns. To choose correct one, you need to answer a question of what exactly does component need to do its job.

Also known as a failure to maintain single responsibility principle. Usual rule of thumb is to look for names containing "and". But I find this problematic as some people are good (or bad) at naming. It is possible to name PersistentCache as CacheAndSave, but it does not mean that former is good and latter is bad. It's hard to come up with example that is not ridiculous, but here we go.

function UserRepo(store, mailer) {
  return {
    list() {
      return store.fetch('user:*');
    },
    fetch(id) {
      return store.fetch('user:' + id);
    },
    async update(id, data) {
      const old = await this.fetch(id);
      const result = await store.save('user:' + data.id, data);
      if (old.email != data.email) {
	await mailer.confirmEmail(id, data.email);
      }
      return result;
    },
    async save(data) {
      const id = await store.nextId();
      const result = await store.save('user:' + id, data);
      await mailer.confirmEmail(id, data.email);
      return result;
    }
  };
}

Besides the funky key value store, we have repository initiating email confirmation. On its own, this only feels wrong, but might be completely benign. Why does it feel wrong? First, mailer is used only in two of four methods. Maybe it would make sense to split UserRepo into reader and writer parts? Queries and commands? Invocation of confirmation flow also raises some questions. Since we are passing user ID, is mailer supposed to understand it's meaning? Will we have to encode confirmation link inside mailer? Does it mean that mailer will have to understand routing? Oh my… It seems that mailer is not supposed to be here, and its interface has to be different.

Where should we put it? Let's help ourselves by imagining that we also have account registration form, and profile view. Now, mailer makes sense in a registration form, but profile screen does not need it. So we are going to move mailer to registration handler (a.k.a. controller). It makes sense that request handler knows how to construct links, thus we can build email message with confirmation URL there. We'll pass it to mailer via generic interface, for example mailer.send(recepients, subject, body).

To solve this puzzle of responsibilities, we asked some questions and gave ourselves hypothetical answers. In real world, question will have real answers, which will lead to other solutions.