Mistake #1: Excessive mocking

I've heard it said before that mocks are a code smell, but I disagree with that. Mocks are great, in moderation. It makes a lot of sense to mock out things like network calls or responses from a 3rd party library. Sometimes it also makes sense to mock your own modules in order to isolate the unit you're trying to test.

But when a particular test starts requiring you to mock out multiple other modules, or when the amount of code dedicated to mocking rivals or exceeds the amount of code actually dedicated to testing in that file, something has gone wrong. Those tests have now become much, much harder to maintain than they otherwise would be.

To fix this you either need to restructure your code to make it more testable, or write end-to-end tests to cover this module because it's not suitable for unit tests.

Mistake #2: Using enzyme

Enzyme is dead.

tldr; it was a bumpy road for them to create a React 16 enzyme adapter, they never actually released an official React 17 adapter, and it is not possible to create a React 18 enzyme adapter without major changes to the code base that are not going to happen.

Even before Enzyme died, React Testing Library was already well on its way to becoming the community standard (it is supported out of the box with Create React App) because unlike enzyme, Testing Library's API encourages you to avoid including component implementation details in your tests.

Mistake #3: Snapshot testing entire components

Snapshot tests are very alluring because they give you a lot of output while requiring you to write very little code. Jest says that:

Snapshot tests are a very useful tool whenever you want to make sure your UI does not change unexpectedly.

But unfortunately, that sense of security is a lie.

First and foremost, jest is wrong to say that snapshots "make sure your UI does not change"; what they actually do is let you know when your markup changes. And it's not necessarily problematic that the markup of your component changed. There are an infinite number of changes that I can make to your markup without changing your UI at all. You know how else I can determine if your markup is going to change? By actually reading the source code.

The other biggest problem with snapshots is that in real world applications they end up changing very frequently and quite dramatically. The diffs of snapshot files end up being so long that the people reviewing your code are 90% of the time going to completely ignore them, removing 100% of their value. And even when people do take the time to attempt to read your massive snapshot diff, what are they supposed to be looking for? It is an exercise in futility.

I made the title of this section about "entire components" instead of just "snapshot testing" because there is a newer way to use snapshots that I think is really neat and very effective; inline snapshots. They are inline because the snapshot is written directly into the source file instead of into a separate file. Basically it gives you the ability to write expect().toBe() tests without manually entering the expected result, and also gives you the ability to easily update those expected results if your code changes.

They look something like this:

it('formats integer', () => {
  expect(format.currency(1)).toMatchInlineSnapshot(`"$1"`)
  expect(format.currency(12)).toMatchInlineSnapshot(`"$12"`)
  expect(format.currency(123)).toMatchInlineSnapshot(`"$123"`)
})

it('formats decimal', () => {
  expect(format.currency(0.12)).toMatchInlineSnapshot(`"$0.12"`)
  expect(format.currency(0.123)).toMatchInlineSnapshot(`"$0.12"`)
  expect(format.currency(0.129)).toMatchInlineSnapshot(`"$0.13"`)
})

The toMatchInlineSnapshot() function starts out blank and is automatically filled in with that string template when the test is executed.

Mistake #4: Writing tests in TypeScript

TypeScript is great. Every single project that I create professionally or personally (my personal website included) is written in TypeScript. However, writing your tests in TypeScript provides little to no value.

In fact, more often than not your TypeScript test files end up having to define custom types of their own or do a bunch of funky typecasting in order to tell the TypeScript compiler to calm down and accept my fake data. Doing this makes the tests more difficult to maintain, harder to read, and simply creates cruft that does not add any value and has no reason to be there.

This is a minor point, but TypeScript tests also usually require more work to setup because they have to be compiled, and you always have to tell typescript to add all the global functions that your tests reference. It's not that these things are difficult, they're just more setup to do that again...adds no value to your project.

Mistake #5: Having a describe() that wraps the entire test module

I admit that this final point is more just annoying than anything else.

If you've ever worked with me you already know that I really hate repeating myself. I try quite hard to make my code as DRY as reasonably possible. So when I see duplication for the sake of duplication, I need to put a stop to it immediately.

Here's an example:

// get-link.test.js

describe('get link handler', () => {
  it('should do this', () => {})
  it('should do that', () => {})
})

What possible purpose could that describe() serve? When the test is run, this is the output

PASS  get-link.test.ts
  get link handler
    ✓ should do this (4 ms)
    ✓ should do that (4 ms)

The exact same information is repeated on lines 1 and 2! For Pete's sake, just remove the pointless describe().

The only defense of this tactic that I've heard is that it makes the code consistent if you later add a second describe() in the file. But it would not make sense for get-link.test.js to have any tests in it that didn't test "get link"; so the only way it could have another describe() would be inside of the root one, in which case you can STILL remove the root one. 🙃