Apr 25 2020

Better Coverage with DI

Global variables actually prevent us from increasing coverage.

Compare functions that use global variables with pure input/output functions. It is very easy to alter the runtime path in a pure function: simply alter the input. When testing globals, altering runtime paths becomes much more difficult. There is too much “magic” happening inside of the function.

Likewise, consider a runtime environment being different from its test environment. For example, a node environment could be testing code for a browser. What happens when node attempts to access variables only found in a browser? Now we get defective tests!

Let’s reference a code snippet from the open source project validate.js.

function warn(msg) {
  if (typeof console !== "undefined" && console.warn) {
    console.warn("[validate.js] " + msg)
  }
}

This refactor is more of a thought experiement than anything. This is not terribly bad code. However it is difficult to get 100% code coverage:

  1. There is a reference to a global variable.
  2. We cannot assert the non-existance of a global variable (or any variable for that matter).

The first issue is a fairly trivial fix. It’s common to reach for dependency injection. However, the second issue is a little more nefarious.

What if a user should be warned, but does not have the console variable? These attempts to cry out to the user would be silenced. If the intent is to warn the user, it’s not much of a stretch to throw an error.

import assert from 'assert'
function warn(msg) {
  assert(console, '`console` is not supported in this environment')
  assert(console.warn, '`console.warn` is not supported in this environment')
  console.warn("[validate.js] " + msg)
}

Now we can be sure that our concerns meet the user. If they have console.warn, we can give them our custom error. Otherwise we can at least tell them to change environments.

Now we can go after the global variable issue.

import assert from 'assert'
export const warn = (logger) => (msg) => {
  assert(logger, `'${logger}' is not supported in this environment`)
  assert(logger.warn, `'${logger}.warn' is not supported in this environment`)
  logger.warn("[validate.js] " + msg)
}

This gives sufficient access to complete branch coverage. In order to verify branch coverage, we can inject a spy. The spy asserts the callCount.

import sinon from 'sinon'
import { expect } from 'chai'
import { warn } from './validate'

describe('warn', () => {
  it('logs a warning', () => {
    const logger = warn({ warn: sinon.spy() })
    logger('some message')
    expect(logger.warn).to.have.property('callCount', 1)
  })

  it('throws for missing warn', () => {
    const logger = warn({})
    const badFn = () => { logger.warn() }
    expect(badFn).to.throw(/is not supported in this environment/)
  })

  it('throws for missing logger', () => {
    const logger = warn()
    const badFn = () => { logger.warn() }
    expect(badFn).to.throw(/is not supported in this environment/)
  })
})

Beyond this, I would only have one other recommendation. It would make more sense to rename the imported warn function to something like createLogger. This name change is necessitated from the introduction of DI.

What did we “unlock” as a result of the refactor?