bug fix: concat(undefined) returns [undefined]

[givanse]

Requirements

Description of the Change

This line: this.options.globals = (this.options.globals || []).concat(globals); has the potential to produce arrays that contain the undefined element.

The result of concat(undefined) is [undefined], an array of length 1 that contains undefined.

Later in lib/runner there is a filter that iterates on the globals list and raises an error when trying to reference undefined.

Alternate Designs

There is different syntax that could be used to achieve the same validation and fallback. If there is a preferred style I'm happy to adjust to it.

Why should this be in core?

It is fixing an edge case within the core libs.

Benefits

Protects users from an edge case where globals is has falsy value.

Possible Drawbacks

The impact is trivial, it increases the set up overhead by a little bit (if at all).

Applicable issues

patch release

[coveralls]

Coverage increased (+0.04%) to 90.814% when pulling 3efcc84 on givanse:concat-undefined into 0a86e6f on mochajs:master.

[boneskull]

@givanse Seems plausible, but how did you encounter this?

[givanse]

I encountered this by using Mocha programmatically with Testem. Right now I don't have access to the codebase. So it is going to take me a few days (maybe Wednesday) to get back to you with more details.

Maybe the fallback could be moved up, to:

mocha/lib/mocha.js

Line 112 in 6b5a785

.globals(options.globals);

and use .globals(options.globals || []);

[plroebuck]

Since we shouldn't allow empty strings either...

Mocha.prototype.globals = function(globals) {
  this.options.globals = (this.options.globals || []).concat(globals).filter(Boolean);
  return this;
};