Bad Test, Bad
Tags: testing, javascript, webdev
Published: 2019-07-30 12:01:38

---
title: Bad Test, Bad
published: true
description: These tests should never have been written. They provide no or little value. Testing code as been described as an "art form." It is, but it should not be. There are many good patterns that should be followed when writing tests.
tags: Testing, JavaScript, WebDev
series: Front-End Testing Patterns
cover_image: https://thepracticaldev.s3.amazonaws.com/i/b8k7xtj52as21kk8bjb4.jpg
---

> These tests should never have been written. They provide no or little value.

Testing code as been described as an "art form." It is, but it should not be. There are many good patterns that should be followed when writing tests. There is even a lifecycle of tests that should be paid some attention. There are also many BAD patterns that developers should be aware of so that they can be avoided (or cleaned up).

Over the years, I've heard of and come across many examples of tests that are incorrect, violate basic principles of testing, do too much or too little. What I've had a challenge with is finding actual examples supporting the testing principles I follow.

The reason for this article is to provide concrete code patterns that are BAD and where possible, ways to correct these BAD tests.

Code used here is located on github at [https://github.com/bob-fornal/bad-test-bad](https://github.com/bob-fornal/bad-test-bad).

## Use of BAD Testing Patterns

Given that we are showing code that is BAD: There are always reasons to violate these rules, but the logic behind violating the rules should be well thought out and described in the test code.

## Tests Should Not Be "Commented Out"

Tests should NEVER be pushed in to get code through the pipeline. Only when code is removed should the corresponding tests be removed.

## Tests Should Not Be "Well-Factored"

```javascript
/* BAD PATTERN: Well-Factored */
var testableCode = {
  user: {},
  getUserRating: function() {
    return testableCode.user.rating;
  },
  getUserScore: function() {
    return testableCode.user.score;
  }
};

beforeEach(function() {
  testableCode.user = {
    username: 'Bob',
    rating: 100,
    score: 1.1
  };
});

// more code and tests here...

it('expects user rating to be returned', function() {
  var result = testableCode.getUserRating('Bob');
  expect(result).toEqual(100);
});

it('expects user score to be returned', function() {
  var result = testableCode.getUserScore('Bob');
  expect(result).toEqual(1.1);
});
```

What does the test in this code do? It retrieves a rating and verified that the value is 100. Looking at just the test, a few questions should come to mind ...

1. Where did 'Bob' come from?
2. Why is the rating 100?

This test is well-factored, meaning in this case that obvious information is moved out of the test. Examining this using a slightly different design ...

```javascript
/* GOOD PATTERN: Keep the reader in the test */
/* GOOD PATTERN: Violate the DRY Principle */
var testableCode = {
  user: {},
  getUserRating: function() {
    return testableCode.user.rating;
  },
  getUserScore: function() {
    return testableCode.user.score;
  }
};

afterEach(function() {
  testableCode.user = {};
});

// more code and tests here...

it('expects user rating to be returned', function() {
  testableCode.user = {
    username: 'Bob',
    rating: 100,
    score: 1.1
  };

  var result = testableCode.getUserRating('Bob');
  expect(result).toEqual(100);
});

it('expects user score to be returned', function() {
  testableCode.user = {
    username: 'Bob',
    rating: 100,
    score: 1.1
  };

  var result = testableCode.getUserScore('Bob');
  expect(result).toEqual(1.1);
});
```

### Keep The Reader In The Test

When writing a test, think about the next developer who will see the test. They do not want to read the entire test suite, and they certainly do not want to read through a suite of test utilities.

If a test breaks, the reader should be able to diagnose the problem by reading the test in a straight line from top to bottom. If they have to jump out of the test to read additional code, the test has not done its job.

### Violate The DRY Principle

This pattern of including meaningful information means that code will be repeated, most likely using COPY/PASTE. This is good; the tests include all necessary information.

The goal here is clear, simple tests.

Before blindly applying the DRY Principle to tests, think about what will make the problem obvious when a test fails. Refactoring may reduce duplication, but it also increases complexity and can obscure information when things break.

## Testing Too Much

Given some code ...

```javascript
var testableCode = {
  strings: [],
  getString: function() {
    if (testableCode.strings.length === 0) {
      return "";
    }
    return testableCode.strings.pop();
  },
  setString: function(value) {
    testableCode.string.push(value);
  }
};
```

Now, examining a possible test ...

```javascript
/* BAD PATTERN: Testing Too Much */
it('expects getString to return an empty string', function() {
  testableCode.strings = [];
  expect(testableCode.getString()).toEqual("");

  testableCode.strings = ["Bob"];
  expect(testableCode.getString()).toEqual("Bob");
  expect(testableCode.getString()).toEqual("");
});
```

The test above is doing too much. There are at least two distinct scenarios shown. Cleaning up these tests should result in something like the following ...

```javascript
/* GOOD PATTERN: Each test expects once */
describe('it expects getString to return', function() {
  it('an empty string', function() {
    testableCode.strings = [];
    expect(testableCode.getString()).toEqual("");
  });

  it('the "last" value', function() {
    testableCode.strings = ["Dan", "Bob"];
    expect(testableCode.getString()).toEqual("Bob");
  });

  it('an empty string after all other values are removed', function() {
    testableCode.strings = ["Bob"];
    testableCode.getString(); // removes "Bob"
    expect(testableCode.getString()).toEqual("");
  });
});
```

### Each Test Expects Once

Having more than one assert is not automatically a problem (even though having too many is a CODE SMELL). But more important than the quantity of asserts is their quality; the semantics. The test name promises that it tests for just a single scenario. But the test goes way beyond that and tests other behaviors, each one deserving of its own focused and well-named test case.

### Breaking Single Responsibility Principle
(in code-under-test)

This type of test ...

```javascript
/* BAD PATTERN: Code-under-test can break SRP */
describe("removeSomething", function () {
  describe("where when called", function () {
    beforeEach(function () {
      this.module.remove.and.returnValue(jasmine.helpers.deferredDone());
      this.module.removeSomething();
    });

    it("should call remove action to remove something", function () {
      expect(this.module.remove).toHaveBeenCalledWith({
        "subPathId": "removeSomething"
      });
    });
  });
});
```

... would allow a developer to follow the "Each Test Expects Once" rule above with code-under-test that is doing too much, violating the Single Responsibility Principle.

See [A Different Unit Test Pattern](https://dev.to/rfornal/a-different-unit-test-pattern-39m)

## Duplication Of Code Logic

Given some code ...

```javascript
var testableCode = {
  getString: function(a, b, len) {
    return (a + b).substr(0, len);
  }
};
```

Now, examining a possible test ...

```javascript
/* BAD PATTERN: Duplication of code logic */
it('expects getString to return a + b at len', function() {
  var a = "abc";
  var b = "def";
  var len = 4;
  var result = (a + b).substr(len);

  expect(testableCode.getString(a, b, len)).toEqual(result);
});
```

The test contains the same logic that the code under test uses. If this logic contained a flaw, the test might still succeed. Cleaning up these tests should result in something like the following ...

```javascript
/* GOOD PATTERN: Use Magic Numbers */
it('expects getString to return a + b at len', function() {
  var a = "abc";
  var b = "def";
  var len = 4;

  expect(testableCode.getString(a, b, len)).toEqual("abcd");
});
```

This way, if the logic within the code is wrong (or changed to something incorrect) the test will fail.

## Bad Test Double

This is a case for testing with a "faithful" test-double.

Given some code (there's a bit more setup here) ...

```javascript
var apiService = {
  base: 'http://www.example.com/',
  testApi: 'testApi/',
  getSimple: function() {
    return fetch(apiService.base + apiService.testApi{
        method: 'GET'
      }).then(function(response) {
        return response.json();
      });
  },
  getMore: function() {
    return fetch(apiService.base + apiService.testApi{
        method: 'GET'
      }).then(function(response) {
        var result = response.json();
        result.checked = true;
        return result;
      });
  }
};

var testableCode = {
  getData: function(service) {
    return service.getSimple();
  }
};
```

This code sets us up to examine some testing paths where the test-double can cause problems.

```javascript
/* BAD PATTERN: Testing with a mock */
var mockService, mockResult;
beforeEach(function() {
  mockResult = {
    data: "Bob"
  };
  mockService = {
    getSimple: function() {
      return Promise().resolve(mockResult);
    }
  };
});

describe('expect getData to', function() {
  it('return the correct object', function(done) {
    testableCode.getData(mockService)
      .then(function(data) {
        expect(data).toEqual(mockResult);
        done();
      }).catch(function() {
        expect(true).toEqual(false);
        done();
      });
  });
});
```

In the previous test, if the service changes or the data returned changes, the setup for these tests have to be changed. These tests would continue to pass.

Looking at the next example ...

```javascript
/* OK PATTERN: Hooking into the service */
var mockService, mockResult;
beforeEach(function(_apiService_) {
  mockResult = {
    data: "Bob"
  };
  mockService = _apiService_;
  mockService.getSimple = function() {
    return Promise().resolve(mockResult);
  };
});

describe('expect getData to', function() {
  it('return the correct object', function(done) {
    testableCode.getData(mockService)
      .then(function(data) {
        expect(data).toEqual(mockResult);
        done();
      }).catch(function() {
        expect(true).toEqual(false);
        done();
      });
  });
});
```

Here, the prior example will continue to pass if the service is changed or the data returned changes. It is an improvement on the previous test because the remainder of the internals of the ***apiService*** are exposed for use, if needed. This exposure would allow for changes in ***testableCode*** and the other parts of the service that remain unchanged.

```javascript
/* GOOD PATTERN: Hooking into HTTP Request */
var mockService, mockResult;
beforeEach(function(_apiService_) {
  mockResult = {
    data: "Bob"
  };
  mockService = _apiService_;

  // Hook into HTTP Requests here ...
  // Specifically: GET on http://www.example.com/testApi/
  // Leaving this code out; will depend on packages
  //   or frameworks used
});

describe('expect getData to', function() {
  it('return the correct object', function(done) {
    testableCode.getData(mockService)
      .then(function(data) {
        expect(data).toEqual(mockResult);
        done();
      }).catch(function() {
        expect(true).toEqual(false);
        done();
      });
  });
});
```

This code should be much more resilient to change. For example, ***getData*** in testable code could be changed to use ***getMore*** rather than ***getSimple*** from the service without failure.

Here, the test would only be vulnerable to changes in the API data coming back.

## Testing Against Bad Data

Given some code ...

```javascript
var testableCode = {
  isDateAfterToday: (givenDate) => {
    var currentDate = new Date();
    return givenDate > currentDate;
  }
};
```

Now, examining a possible test ...

```javascript
/* BAD PATTERN: Testing against bad data */
it('expects isDateAfterToday to return true', function() {
  var futureDate = new Date('2019-10-22');
  expect(testableCode.isDateAfterToday(futureDate))
    .toEqual(true);
});

/* BETTER PATTERN: Testing against bad data */
it('expects isDateAfterToday to return true', function() {
  var futureDate = new Date('3019-10-22');
  expect(testableCode.isDateAfterToday(futureDate))
    .toEqual(true);
});

/* BEST PATTERN: Testing against bad data */
it('expects isDateAfterToday to return true', function() {
  var tomorrow = new Date();
  tomorrow.setDate(tomorrow.getDate() + 1);
  futureDate = new Date(tomorrow);
  expect(testableCode.isDateAfterToday(tomorrow))
    .toEqual(true);
});
```
In the BAD PATTERN, the date is hard coded; we will reach this date (yes, the date used here was found in a recently failing test).

In the BETTER PATTERN, a year is used that we will most likely not reach.

In the BEST PATTERN, we are calculating a value that will not be reached, tomorrow.


## Testing The Mock

Given some code ...

```javascript
var testableCode = {
  getString: function(a, b, len) {
    return (a + b).substr(0, len);
  },
  getShortName: function(first, last) {
    return testableCode.getString(last + ", ", first, 10);
  }
};
```

Now, examining a possible test ...

```javascript
/* BAD PATTERN: Testing the mock */
beforeEach(function() {
  jasmine.spyOn(testableCode, "getShortName")
    .and.returnValue("Bob45678901234567890");
});

describe('expects getShortName to return', function() {
  it('a name truncated to 10 characters', function() {
    expect(testableCode.getShortName("Bob", "Last"))
      .toEqual("Bob45678901234567890");
  });
});
```

The only thing that gets tested in this example is the mock created in the **beforeEach**. The true **getString** functionality does not get exercised here. The only thing determined here is that the **getString** function is what is actually called within the **getShortName** functionality (this is a form of Gray-Box Testing; some knowledge of the internals).

Cleaning up these tests should result in something like the following ...

```javascript
/* GOOD PATTERN: Testing appropriate code */
describe('expects getString to return', function() {
  it('a + b at len', function() {
    var a = "abc";
    var b = "def";
    var len = 4;

    expect(testableCode.getString(a, b, len)).toEqual("abcd");
  });
});

describe('expects getShortName to return', function() {
  it('a name truncated to 10 characters', function() {
    expect(testableCode.getShortName("Bob4567890", "Last"))
      .toEqual("Last, Bob4");
  });
});
```

Here, it is clear that we are testing the code, not the test framework itself.

## False Positives

Examining a possible test ...

```javascript
/* BAD PATTERN: False positive */
it('expect the code inside setTimeout to be ignored', function() {
  setTimeout(function() {
    expect(true).toEqual(false);
  }, 1000);
});
```

The code inside the **setTimeout** will not run before the test completes.

In most javascript test suites, **specs with no expectations simply pass.

```javascript
/* BAD PATTERN: False positive */
it('test with no expect will always pass', function() {
  const hello = 'World';
  const life = 42;
});
```

Dealing with this issue is simple: both in mocha and jasmine, an extra parameter can be passed into the spec (usually called ***done***).
This flags the test as asynchronous, and the test engine will wait for the parameter (function) to be called before flagging the test as passed.

Looking at the examples above in this light ...

```javascript
/* GOOD PATTERN: Handling Asynchronous Behavior */
it('expect the code inside setTimeout to run', function(done) {
  setTimeout(function() {
    expect(true).toEqual(false);
    done();
  }, 1000);
});

/* GOOD PATTERN: Include expect */
it('test with an expect can pass or fail', function() {
  const hello = 'World';
  const life = 42;
  expect(true).toEqual(false);
});
```

These tests will now fail.

### Promises

Looking at some code ...

```javascript
var testableCode = {
  getRejectedPromise: function() {
    return new Promise((resolve, reject) => { 
      setTimeout(() => {
          reject('fail');
      }, 1000);
    });
  }
};
```

And now, looking at the test ...

```javascript
/* BAD PATTERN: False positive */
it('expects rejection to occur (should pass)', function() {
  testableCode.getRejectedPromise().then(function(result) {
    expect(result).toEqual('fail');
  });
});
```

As shown above, this test will give a false positive. Using the resolution seen ...

```javascript
/* GOOD PATTERN: Handling Asynchronous Behavior */
it('expects rejection to occur (should follow catch)', function(done) {
  testableCode.getRejectedPromise()
    .then(function(result) {
      expect(result).toEqual('pass');
      done();
    })
    .catch(function(result) {
      expect(result).toEqual('fail');
      done();
    });
});
```

So, at this point, the code would fail if it went through the **resolve**, but since it gets **rejected**, it will correctly pass.

## Testing Private Functionality Directly

Looking at some code ...

```javascript
function testableCode() {
  function privateFnA() { ... };
  function privateFnB() { ... };

  function publicFnC() { ... };
  function publicFnD() { ... };

  return {
    publicFnC,
    publicFnD,

    testable: {
      privateFnA,
      privateFnB,
      publicFnC,
      publicFnD
    }
  };
}
```

And here, the tests can now directly test all functionality within this codebase. The issue here is that the private functionality should not be exposed and if there is some functional shift in the code, this can lead to significant test refactoring along with the code refactoring.

The private functionality should have been tested indirectly through the publicly exposed functionality.

## Excessive Setup

Excessive setup is more of a CODE SMELL than something where code can be shown that is incorrect versus correct. Just be aware that this is a case where care should be taken to examine why the setup is so lengthy and document the reasoning if it should exist.

## Conclusions

The reason for this article is to provide concrete code patterns that are BAD and where possible, ways to correct these BAD tests.

> These tests should never have been written. They provide no or little value.

Testing code as been described as an "art form." It is, but it should not be. There are many good patterns that should be followed when writing tests. There is even a lifecycle of tests that should be paid some attention. There are also many BAD patterns that developers should be aware of so that they can be avoided (or cleaned up).