Why are these tests passing?

Question

I have this function:

let removePresentation = function(presentationName, callback) {
  let rimraf = require('rimraf');

  callback();
  callback();
  callback();

  if(!presentationName || !presentationName.trim()) {
    callback();
    return;
  }

  presentationName = presentationName.replace('.zip', '');

  rimraf('./presentations/' + presentationName, function(err) {
    if(err) {
      console.log(err);
    }
    callback();
  });
};

exports.removePresentation = removePresentation;

and I am trying to test it with the following:

var chai = require('chai'),
expect = require('chai').expect,
sinonChai = require('sinon-chai'),
sinon = require('sinon'),
mock = require('mock-require');

chai.use(sinonChai);

describe('removePresentation', function() {

  var sandbox;
  var callback;
  var rimrafSpy;

  beforeEach(function() {
    sandbox = sinon.sandbox.create();
    mock('../business/communications_business', {});

    rimrafSpy = sinon.spy();
    callback = sinon.spy();

    mock('rimraf', rimrafSpy);
  });

  afterEach(function() {
    sandbox.restore();
  });

  it('should call rimraf if presentation name is valid', function(done) {
    let RoomStateBusiness = require('../business/roomstate_business');

    RoomStateBusiness.removePresentation('name.zip', callback);

    expect(rimrafSpy).to.have.been.calledWith('./presentations/name');
    expect(callback).to.have.been.called.once;
    done();
  });

  it('should not call rimraf if presentation name is null', function(done) {
    let RoomStateBusiness = require('../business/roomstate_business');

    RoomStateBusiness.removePresentation(null, callback);

    expect(rimrafSpy).not.to.have.been.called;
    expect(callback).to.have.been.called.once;
    done();
  });

  it('should not call rimraf if presentation name is whitespace', function(done) {
    let RoomStateBusiness = require('../business/roomstate_business');

    RoomStateBusiness.removePresentation('      ', callback);

    expect(rimrafSpy).not.to.have.been.called;
    expect(callback).to.have.been.called.once;
    done();
  });

  it('should not call rimraf if presentation name is empty string', function(done) {
    let RoomStateBusiness = require('../business/roomstate_business');

    RoomStateBusiness.removePresentation('', callback);

    expect(rimrafSpy).not.to.have.been.called;
    expect(callback).to.have.been.called.once;
    done();
  });

});

Even though I am clearly calling callback() multiple times (whilst testing only), expect(callback).to.have.been.called.once; is always asserting to true. I have checked on the Chai api that that expects the call to be exactly once, although it is always passing no matter how many times I call callback(). What am I doing wrong?

Answer 1

There is no such assertion as expect(fn).to.have.been.called.once.

As per sinon-chai docs, there is only:

• expect(fn).to.have.been.called
• expect(fn).to.have.been.calledOnce

The problem

This is a known problem with chai and why getter-only-assertions are a bad thing. Chai allows you to write a piece of code which looks like a property access (ie. the assertion itself does not end with a function call) to assert...whatever you want to assert. This uses property getters to execute the necessary code.

The problem is that if you make a typo or other mistake, the expression will simply evaluate to undefined (you are accessing a property which does not exist) and no assertion code is ever executed, resulting in a test passing (because tests only fail if an exception is thrown).

In your case, there is an assertion for called, and this most likely returns an object. Unfortunately, that object does not have assertion once, so there is no code executed and the test passes.

The solution

There are 2 options available to you:

• Upgrade to Chai 4 and Node.js version with Proxy support (not sure where Proxy support was added, probably Node.js 5 or 6) - chai introduced a safeguard against these issues by proxying all property access through a Proxy object which checks if you are using a valid assertion
• Never use getters for assertions and always end your assertions with a function call - this will make sure that if you ever make a mistake the test will fail with the infamous undefined is not a function error

The second option is highly preferred, in my opinion, as there can be no doubt on the correctness of the test case. Chai proxy support can still be turned off even on supported platforms.