Our struggles with testing.

What do I want to say.

Where we started coming from PHPUnit.

Our theory about keeping tests isolated.

The difficulty and inflexibility of our tests. Difficulty of getting full coverage.
Our crazy use of mocks, and how they actually just end up testing implementation details

"These tests prove you wrote the code you wrote, not that it works"

evolution of plan, to maybe we just shouldn't care about coverage as much.
I think maybe we just shouldn't care quite as much about mocking and smashing all dependencies. If you are having to
proxyquire 5 dependencies, you could have a code smell and need redesign, or you
might just not want to mock them all.

---
layout: post
title: 
author: Will
---

In my last post about [Transitioning from PHP to Node.js]() I mentioned that one of our largest challenges had been learning how to unit test in Node.js. Over the last few weeks almost every one of us had grumbled to another member of the team that, "Boy! These tests are difficult to write." Everyone was feeling like the effort they were putting into unit testing was not paying the dividends that we had experienced from testing in PHP. We talked about how difficult tests were to read, and how painful it often is to get the last 10% of coverage. We started questioning their usefulness because of how brittle they are. The smallest code change resulted in full refactoring of tests, that's too much overhead! We started having discussions about lowering our standard for test coverage, which is admittedly kind of insane. We mandate 100% unit test coverage. There is a really long story about why we're so demanding in this regard, it boils down to we inherited a lot of untested code a while back and have been paying the technical debt price ever since.

My initial reaction to the discussion of writing less unit tests, or loosening up on coverage as a solution to our testing woes was a frank "I think you're wrong about that." We ultimately decided to start experimenting more with our testing methodology, maybe mock things less, aim for less isolation in the tests, and try to make them easier to write.

### Stuck in a PHPUnit mindset
We had become very comfortable testing with PHPUnit. In that ecosystem you use a lot of Mocks and spy's. You assert that this method was called X number of times, with arguments Y and Z. You mock all of your outside dependencies in order to isolate your "Subject Under Test". This is all excellent stuff, and PHPUnit is a phenomenal tool for testing classical object-oriented PHP.

    $y = 'y';
    $z = 'z'
    $expected = 'bar';
    
    $sut = $this->getMock('MyClass');
    $sut
        ->expects($this->once())
        ->method('myMethod')
        ->with($y, $z)
        ->willReturn($expected)
       
When we started testing in JavaScript we naturally began looking for tools which could give us the things we were used to. For dependency isolation we found [proxyquire](https://github.com/thlorenz/proxyquire), for mocking we use [sinon](http://sinonjs.org). These are both excellent tools and I have nothing bad to say about them.

However, it wasn't until I took the time to actually sit down with the sole purpose of finding a way to simplify a JavaScript test that I realized how badly we had been abusing them. It's kind of embarrassing. I don't even want to show you guys these tests, here live on the internet. But I'm going to do it anyway.

## The subject
Here's the code to test, it's pretty simple. It checks if a security group exists in AWS.

    'use strict'
    
    var _ = require('lodash')
    var Bluebird = require('bluebird')
    
    function doesSecurityGroupExist (ec2, securityGroupName) {
      function inspectError (err) {
        if (err.code === 'InvalidGroup.NotFound') {
          return false
        }
    
        return Bluebird.reject(err)
      }
    
      return ec2
        .describeSecurityGroupsPromised({
          GroupNames: [ securityGroupName ]
        })
        .then(_.constant(true))
        .error(inspectError)
    }
    
    module.exports = doesSecurityGroupExist
    
This isn't too bad, it's just a function. You give it an ec2 client from [aws-promised](https://www.npmjs.com/package/aws-promised), and the name of security group you want to look for. You expect to have that promise resolved with true when the security group exists, and false when it doesn't.

## Unit Test Attempt (filename "Ewwwww.js")
First, here's the convoluted, burdensome, way I had been testing. I'll explain below certain aspects of it. Even when writing this example I couldn't push myself to test the entire file for 100% coverage. It was too annoying. I'm done with it, this is the last test I will write like this.

    'use strict';
    
    var proxyquire = require('proxyquire')
    var should = require('should')
    var sinon = require('sinon')
    
    describe('ec2/doesAutoScalingGroupExist', function () {
      var _
      var constantFunc;
      var ec2
      var Bluebird
      var doesSecurityGroupExist
    
      beforeEach(function () {
        constantFunc = 'test.constant';
        _ = { constant: sinon.stub().returns(constantFunc) }
        ec2 = {
          describeSecurityGroupsPromised: sinon.stub().returnThis()
          then: sinon.stub().returnsThis(),
          error: sinon.stub()
        }
        Bluebird = {
          reject: sinon.stub()
        }
    
        doesSecurityGroupExist = proxyquire(
          '../../lib/ec2/doesSecurityGroupExist',
          {
            lodash: _,
            bluebird: Bluebird
          }
        )
      })
    
      it('should resolve true if the security group exists', function () {
        var securityGroupName = 'test.sg'
    
        doesSecurityGroupExist(ec2, securityGroupName)
    
        ec2.describeSecurityGroupsPromised.calledOnce.should.equal(true);
        ec2.describeSecurityGroupsPromised.args[0].should.eql([{
          GroupNames: [ securityGroupName ]
        }])
    
        _.constant.calledOnce.should.equal(true)
        _.constant.args[0].should.eql([true])
    
        ec2.then.calledOnce.should.equal(true)
        ec2.then.args[0].should.equal(constantFunc)
      })
    })

Did you try to follow it? It's difficult right? You can see we have a lot of setup code in the `beforeEach`. Setting up stubs and proxyquiring in our dependencies for `bluebird` and `lodash`. Our theory here was that we wanted to isolate our unit of code, the actual `doesSecurityGroupExist` function we're testing, from it's dependence on lodash and bluebird. We're essentially overriding their methods with mocks, smashing them with our own versions using proxyquire, and then controlling the entire flow of our code with stubs. There's a "hack" in there to get `ec2` to have promise methods `then` and `error` with the use of `sinon.stub().returnsThis()`. I think that's a deceptive way to do this, because `ec2` doesn't actually have those methods, it returns a promise. We're faking async behavior through stub magic. And beyond the setup, the test itself is confusing. There's an invocation of `doesSecurityGroupExist` and then a lot of sinon assertions on who got passed what. The really horrible thing about this test I just wrote is you can't understand the `it` block without deeply examining the `beforeEach`. It's all jumbled together. There's like three more confusing tests to write, but you can get to 100% coverage continuing on this way. 

The thing that disturbs me most:
 
> This test only proves I wrote the code I wrote, not that it works.

If I change anything about the implementation of our subject code, it will break the test because all of those stubs are expecting to be called and they'll throws errors if they don't get what they want. This is just wrong. I wrote code, and then I wrote code that does nothing valuable except check that I did in fact write the code I intended to write word for word.

## Behavior Test Attempt
I'm going to argue that the subject isolation we're shooting for, is actually our foot. Yes, it probably makes the tests faster if they don't load the entire dependency tree. Yes, our behavior tests will call through to the real `_.constant` and `Blubird` methods and depend on them to work correctly so if something is wrong with them, they'll break our test. That doesn't sound bad! That's what we want! This was just way easier to write, and I believe it's easier to follow as well.

    'use strict'
    
    var assert = require('assert')
    var Bluebird = require('bluebird')
    var doesSecurityGroupExist = require('../../lib/ec2/doesSecurityGroupExist')
    
    describe('ec2/doesSecurityGroupExist', function () {
      var ec2 = {}
      var securityGroupName = 'test.sg'
    
      it('should resolve true if security group exists', function () {
        ec2.describeSecurityGroupsPromised = function (params) {
          assert.deepEqual(params, { GroupNames: [securityGroupName] })
          return Bluebird.resolve()
        }
    
        doesSecurityGroupExist(ec2, securityGroupName)
          .then(function (result) {
            assert.equal(result, true)
          })
      })
    
      it('should resolve false if security group does not exist', function () {
        ec2.describeSecurityGroupsPromised = function (params) {
          var err = { code: 'InvalidGroup.NotFound' }
          assert.deepEqual(params, { GroupNames: [securityGroupName] })
          return Bluebird.reject(err)
        }
    
        doesSecurityGroupExist(ec2, securityGroupName)
          .then(function (result) {
            assert.equal(result, false)
          })
      })
    
      it('should reject on all errors except Invalid.NotFound', function () {
        var err = { code: 'OtherError.test' }
    
        ec2.describeSecurityGroupsPromised = function (params) {
          assert.deepEqual(params, { GroupNames: [securityGroupName] })
          return Bluebird.reject(err)
        }
    
        doesSecurityGroupExist(ec2, securityGroupName)
          .error(function (reason) {
            assert.deepEqual(reason, err)
          })
      })
    })

This is quite different. Not only am I not isolating my subject from Bluebird, but I'm actually using Bluebird in the test! We're saying we trust our module choices as much as I trust mocha, should, assert, or any of the rest. The nice thing is. If I change the implementation slightly. Like let's say I get rid of `_.constant` and just use an inline function:

    .then(function () { return true })
    
 Test still passes.
 
I think I could completely replace bluebird with another Promises/A+ library like [Q](https://github.com/kriskowal/q) in the subject, and it would still pass without changing my use of bluebird in the tests.