0

I have the below js for unit testing an error handler:

import assert from 'assert';
import deepClone from 'lodash.clonedeep';
import deepEqual from 'lodash.isequal';
import { spy } from 'sinon';
import errorHandler from './index';

function getValidError(constructor = SyntaxError) {
  let error = new constructor();
  error.status = 400;
  error.body = {};
  error.type = 'entity.parse.failed';
  return error;
}

describe('errorHandler', function() {
  let err;
  let req;
  let res;
  let next;
  let clonedRes;
  describe('When the error is not an instance of SyntaxError', function() {
    err = getValidError(Error);
    req = {};
    res = {};
    next = spy();
    clonedRes = deepClone(res);
    errorHandler(err, req, res, next);

    it('should not modify res', function() {
      assert(deepEqual(res, clonedRes));
    });

    it('should call next()', function() {
      assert(next.calledOnce);
    });
  });

  ...(#other test cases all similar to the first)

  describe('When the error is a SyntaxError, with a 400 status, has a `body` property set, and has type `entity.parse.failed`', function() {
    err = getValidError();
    req = {};
    let res = {
      status: spy(),
      set: spy(),
      json: spy()
    };
    let next = spy();
    errorHandler(err, req, res, next);

    it('should set res with a 400 status code', function() {
      assert(res.status.calledOnce);
      assert(res.status.calledWithExactly(400));
    });

    it('should set res with an application/json content-type header', function() {
      assert(res.set.calledOnce);
      assert(res.set.calledWithExactly('Content-Type', 'application/json'));
    });

    it('should set res.json with error code', function() {
      assert(res.json.calledOnce);
      assert(res.json.calledWithExactly({ message: 'Payload should be in JSON format' }));
    });
  });
});

Notice that I have let in front of res, next and clonedRes in the describe block for 'When the error is a SyntaxError...'.

Without the let in front of these, I get failures in my tests. I do not understand why I need to add the let for these again, but not for the err and req in that same block. Could anyone help me out with an explanation?

msm1089
  • 1,314
  • 1
  • 8
  • 19
  • It's always a good pratice to explicitly declare your variable ( using `var` or `let`). Javascript is supposed to understand implicitly declared variable but it can run into problem, like in your case. – Nicolas Mar 04 '19 at 02:01

1 Answers1

2

In strict mode (and in decently linted code in general), a variable must be declared before it's assigned to. Also, const and let variables must be declared once in a block, and no more. Re-declaring the err (or any other variable) which has already been declared will throw an error, which is why you should see let <varname> only once in your describe('errorHandler' function:

const describe = cb => cb();

let something;
describe(() => {
  something = 'foo';
});
let something;
describe(() => {
  something = 'bar';
});

Further describes inside of describe('errorHandler' already have scoped access to err.

Without declaring a variable first at all, assigning to it in sloppy mode will result in it being assigned to the global object, which is almost always undesirable and can introduce bugs and errors. For example:

// Accidentally implicitly referencing window.status, which can only be a string:

status = false;
if (status) {
  console.log('status is actually truthy!');
}

That said, it's often a good idea to keep variables scoped as narrowly as possible - only assign to an outside variable when you need the value in the outer scope. Consider declaring the variables only inside of the describes that assign to them, which has an additional bonus of allowing you to use const instead of let:

describe('When the error is not an instance of SyntaxError', function() {
  const err = getValidError(Error);
  const req = {};
  const res = {};
  const next = spy();
  const clonedRes = deepClone(res);
  errorHandler(err, req, res, next);
  // etc
});
// etc
describe('When the error is a SyntaxError, with a 400 status, has a `body` property set, and has type `entity.parse.failed`', function() {
  const err = getValidError();
  const req = {};
  const res = {
    status: spy(),
    set: spy(),
    json: spy()
  };
  const next = spy();
  // etc
CertainPerformance
  • 356,069
  • 52
  • 309
  • 320
  • Thanks for the answer, it is useful. I was basically trying to avoid having to type `const` everywhere. Why is it a bonus to use `const` rather than `let`? – msm1089 Mar 04 '19 at 02:33
  • 1
    It makes the code more readable when you *know* that a variable is not going to be reassigned. For example, consider `const foo = 'foo'; <30 lines of code> ` You *know* that `foo` will definitely be `'foo'` without having to carefully look through those 30 lines for any `foo = `. Reassignment should be avoided when possible. – CertainPerformance Mar 04 '19 at 02:36