Repository: mochajs/mocha

Problem statement:
bug fix: concat(undefined) returns [undefined]
### 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



Hints:

[![Coverage Status](https://coveralls.io/builds/21038001/badge)](https://coveralls.io/builds/21038001)

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

@givanse Seems plausible, but how did you encounter this?
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: https://github.com/mochajs/mocha/blob/6b5a7855110d9c493dc41aec9fb2cea15aaa42aa/lib/mocha.js#L112
and use `.globals(options.globals || []);`


Patch:
diff --git a/lib/mocha.js b/lib/mocha.js
--- a/lib/mocha.js
+++ b/lib/mocha.js
@@ -543,7 +543,9 @@ Mocha.prototype._growl = growl.notify;
  * mocha.globals(['jQuery', 'MyLib']);
  */
 Mocha.prototype.globals = function(globals) {
-  this.options.globals = (this.options.globals || []).concat(globals);
+  this.options.globals = (this.options.globals || [])
+    .concat(globals)
+    .filter(Boolean);
   return this;
 };
 


Test patch:
diff --git a/test/unit/mocha.spec.js b/test/unit/mocha.spec.js
--- a/test/unit/mocha.spec.js
+++ b/test/unit/mocha.spec.js
@@ -5,7 +5,7 @@ var Mocha = require('../../lib/mocha');
 var sinon = require('sinon');
 
 describe('Mocha', function() {
-  var opts = {reporter: function() {}}; // no output
+  var opts = {reporter: utils.noop}; // no output
   var sandbox;
 
   beforeEach(function() {
@@ -41,88 +41,59 @@ describe('Mocha', function() {
     });
   });
 
-  describe('.run(fn)', function() {
-    it('should not raise errors if callback was not provided', function() {
-      sandbox.stub(Mocha.Runner.prototype, 'run');
+  describe('#allowUncaught()', function() {
+    it('should set the allowUncaught option to true', function() {
       var mocha = new Mocha(opts);
-      expect(function() {
-        mocha.run();
-      }, 'not to throw');
+      mocha.allowUncaught();
+      expect(mocha.options, 'to have property', 'allowUncaught', true);
     });
 
-    it('should execute the callback when complete', function(done) {
+    it('should be chainable', function() {
       var mocha = new Mocha(opts);
-      sandbox.stub(Mocha.Runner.prototype, 'run').callsArg(0);
-      mocha.run(done);
-    });
-  });
-
-  describe('.reporter("xunit").run(fn)', function() {
-    it('should not raise errors if callback was not provided', function() {
-      var mocha = new Mocha();
-      expect(function() {
-        try {
-          mocha.reporter('xunit').run();
-        } catch (e) {
-          console.log(e);
-          expect.fail(e.message);
-        }
-      }, 'not to throw');
+      expect(mocha.allowUncaught(), 'to be', mocha);
     });
   });
 
-  describe('.invert()', function() {
-    it('should set the invert option to true', function() {
+  describe('#bail()', function() {
+    it('should set the suite._bail to true if there is no arguments', function() {
       var mocha = new Mocha(opts);
-      mocha.invert();
-      expect(mocha.options, 'to have property', 'invert', true);
+      mocha.bail();
+      expect(mocha.suite._bail, 'to be', true);
     });
 
     it('should be chainable', function() {
       var mocha = new Mocha(opts);
-      expect(mocha.invert(), 'to be', mocha);
+      expect(mocha.bail(), 'to be', mocha);
     });
   });
 
-  describe('.ignoreLeaks()', function() {
-    it('should set the ignoreLeaks option to true when param equals true', function() {
-      var mocha = new Mocha(opts);
-      mocha.ignoreLeaks(true);
-      expect(mocha.options, 'to have property', 'ignoreLeaks', true);
-    });
-
-    it('should set the ignoreLeaks option to false when param equals false', function() {
-      var mocha = new Mocha(opts);
-      mocha.ignoreLeaks(false);
-      expect(mocha.options, 'to have property', 'ignoreLeaks', false);
-    });
-
-    it('should set the ignoreLeaks option to false when the param is undefined', function() {
+  describe('#checkLeaks()', function() {
+    it('should set the ignoreLeaks option to false', function() {
       var mocha = new Mocha(opts);
-      mocha.ignoreLeaks();
+      mocha.checkLeaks();
       expect(mocha.options, 'to have property', 'ignoreLeaks', false);
     });
 
     it('should be chainable', function() {
       var mocha = new Mocha(opts);
-      expect(mocha.ignoreLeaks(), 'to be', mocha);
+      expect(mocha.checkLeaks(), 'to be', mocha);
     });
   });
 
-  describe('.checkLeaks()', function() {
-    it('should set the ignoreLeaks option to false', function() {
+  describe('#delay()', function() {
+    it('should set the delay option to true', function() {
       var mocha = new Mocha(opts);
-      mocha.checkLeaks();
-      expect(mocha.options, 'to have property', 'ignoreLeaks', false);
+      mocha.delay();
+      expect(mocha.options, 'to have property', 'delay', true);
     });
 
     it('should be chainable', function() {
       var mocha = new Mocha(opts);
-      expect(mocha.checkLeaks(), 'to be', mocha);
+      expect(mocha.delay(), 'to be', mocha);
     });
   });
 
-  describe('.fullTrace()', function() {
+  describe('#fullTrace()', function() {
     it('should set the fullStackTrace option to true', function() {
       var mocha = new Mocha(opts);
       mocha.fullTrace();
@@ -135,7 +106,53 @@ describe('Mocha', function() {
     });
   });
 
-  describe('.growl()', function() {
+  describe('#globals()', function() {
+    it('should be an empty array initially', function() {
+      var mocha = new Mocha();
+      expect(mocha.options.globals, 'to be empty');
+    });
+
+    it('should be chainable', function() {
+      var mocha = new Mocha(opts);
+      expect(mocha.globals(), 'to be', mocha);
+    });
+
+    describe('when argument is invalid', function() {
+      it('should not modify the whitelist when given empty string', function() {
+        var mocha = new Mocha(opts);
+        mocha.globals('');
+        expect(mocha.options.globals, 'to be empty');
+      });
+
+      it('should not modify the whitelist when given empty array', function() {
+        var mocha = new Mocha(opts);
+        mocha.globals([]);
+        expect(mocha.options.globals, 'to be empty');
+      });
+    });
+
+    describe('when argument is valid', function() {
+      var elem = 'foo';
+      var elem2 = 'bar';
+
+      it('should add string to the whitelist', function() {
+        var mocha = new Mocha(opts);
+        mocha.globals(elem);
+        expect(mocha.options.globals, 'to contain', elem);
+        expect(mocha.options.globals, 'to have length', 1);
+      });
+
+      it('should add contents of string array to the whitelist', function() {
+        var mocha = new Mocha(opts);
+        var elems = [elem, elem2];
+        mocha.globals(elems);
+        expect(mocha.options.globals, 'to contain', elem, elem2);
+        expect(mocha.options.globals, 'to have length', elems.length);
+      });
+    });
+  });
+
+  describe('#growl()', function() {
     describe('if capable of notifications', function() {
       it('should set the growl option to true', function() {
         var mocha = new Mocha(opts);
@@ -164,32 +181,45 @@ describe('Mocha', function() {
     });
   });
 
-  describe('.useInlineDiffs()', function() {
-    it('should set the useInlineDiffs option to true when param equals true', function() {
+  describe('#ignoreLeaks()', function() {
+    it('should set the ignoreLeaks option to true when param equals true', function() {
       var mocha = new Mocha(opts);
-      mocha.useInlineDiffs(true);
-      expect(mocha.options, 'to have property', 'useInlineDiffs', true);
+      mocha.ignoreLeaks(true);
+      expect(mocha.options, 'to have property', 'ignoreLeaks', true);
     });
 
-    it('should set the useInlineDiffs option to false when param equals false', function() {
+    it('should set the ignoreLeaks option to false when param equals false', function() {
       var mocha = new Mocha(opts);
-      mocha.useInlineDiffs(false);
-      expect(mocha.options, 'to have property', 'useInlineDiffs', false);
+      mocha.ignoreLeaks(false);
+      expect(mocha.options, 'to have property', 'ignoreLeaks', false);
     });
 
-    it('should set the useInlineDiffs option to false when the param is undefined', function() {
+    it('should set the ignoreLeaks option to false when the param is undefined', function() {
       var mocha = new Mocha(opts);
-      mocha.useInlineDiffs();
-      expect(mocha.options, 'to have property', 'useInlineDiffs', false);
+      mocha.ignoreLeaks();
+      expect(mocha.options, 'to have property', 'ignoreLeaks', false);
     });
 
     it('should be chainable', function() {
       var mocha = new Mocha(opts);
-      expect(mocha.useInlineDiffs(), 'to be', mocha);
+      expect(mocha.ignoreLeaks(), 'to be', mocha);
+    });
+  });
+
+  describe('#invert()', function() {
+    it('should set the invert option to true', function() {
+      var mocha = new Mocha(opts);
+      mocha.invert();
+      expect(mocha.options, 'to have property', 'invert', true);
+    });
+
+    it('should be chainable', function() {
+      var mocha = new Mocha(opts);
+      expect(mocha.invert(), 'to be', mocha);
     });
   });
 
-  describe('.noHighlighting()', function() {
+  describe('#noHighlighting()', function() {
     // :NOTE: Browser-only option...
     it('should set the noHighlighting option to true', function() {
       var mocha = new Mocha(opts);
@@ -203,57 +233,79 @@ describe('Mocha', function() {
     });
   });
 
-  describe('.allowUncaught()', function() {
-    it('should set the allowUncaught option to true', function() {
-      var mocha = new Mocha(opts);
-      mocha.allowUncaught();
-      expect(mocha.options, 'to have property', 'allowUncaught', true);
+  describe('#reporter()', function() {
+    it('should throw reporter error if an invalid reporter is given', function() {
+      var updatedOpts = {reporter: 'invalidReporter', reporterOptions: {}};
+      var throwError = function() {
+        // eslint-disable-next-line no-new
+        new Mocha(updatedOpts);
+      };
+      expect(throwError, 'to throw', {
+        message: "invalid reporter 'invalidReporter'",
+        code: 'ERR_MOCHA_INVALID_REPORTER',
+        reporter: 'invalidReporter'
+      });
     });
 
     it('should be chainable', function() {
       var mocha = new Mocha(opts);
-      expect(mocha.allowUncaught(), 'to be', mocha);
+      expect(mocha.reporter(), 'to be', mocha);
     });
   });
 
-  describe('.delay()', function() {
-    it('should set the delay option to true', function() {
+  describe('#run(fn)', function() {
+    it('should execute the callback when complete', function(done) {
       var mocha = new Mocha(opts);
-      mocha.delay();
-      expect(mocha.options, 'to have property', 'delay', true);
+      sandbox.stub(Mocha.Runner.prototype, 'run').callsArg(0);
+      mocha.run(done);
     });
 
-    it('should be chainable', function() {
+    it('should not raise errors if callback was not provided', function() {
+      sandbox.stub(Mocha.Runner.prototype, 'run');
       var mocha = new Mocha(opts);
-      expect(mocha.delay(), 'to be', mocha);
+      expect(function() {
+        mocha.run();
+      }, 'not to throw');
+    });
+
+    describe('#reporter("xunit")#run(fn)', function() {
+      // :TBD: Why does specifying reporter differentiate this test from preceding one
+      it('should not raise errors if callback was not provided', function() {
+        var mocha = new Mocha();
+        expect(function() {
+          try {
+            mocha.reporter('xunit').run();
+          } catch (e) {
+            console.log(e);
+            expect.fail(e.message);
+          }
+        }, 'not to throw');
+      });
     });
   });
 
-  describe('.bail()', function() {
-    it('should set the suite._bail to true if there is no arguments', function() {
+  describe('#useInlineDiffs()', function() {
+    it('should set the useInlineDiffs option to true when param equals true', function() {
       var mocha = new Mocha(opts);
-      mocha.bail();
-      expect(mocha.suite._bail, 'to be', true);
+      mocha.useInlineDiffs(true);
+      expect(mocha.options, 'to have property', 'useInlineDiffs', true);
     });
 
-    it('should be chainable', function() {
+    it('should set the useInlineDiffs option to false when param equals false', function() {
       var mocha = new Mocha(opts);
-      expect(mocha.bail(), 'to be', mocha);
+      mocha.useInlineDiffs(false);
+      expect(mocha.options, 'to have property', 'useInlineDiffs', false);
     });
-  });
 
-  describe('error handling', function() {
-    it('should throw reporter error if an invalid reporter is given', function() {
-      var updatedOpts = {reporter: 'invalidReporter', reporterOptions: {}};
-      var throwError = function() {
-        // eslint-disable-next-line no-new
-        new Mocha(updatedOpts);
-      };
-      expect(throwError, 'to throw', {
-        message: "invalid reporter 'invalidReporter'",
-        code: 'ERR_MOCHA_INVALID_REPORTER',
-        reporter: 'invalidReporter'
-      });
+    it('should set the useInlineDiffs option to false when the param is undefined', function() {
+      var mocha = new Mocha(opts);
+      mocha.useInlineDiffs();
+      expect(mocha.options, 'to have property', 'useInlineDiffs', false);
+    });
+
+    it('should be chainable', function() {
+      var mocha = new Mocha(opts);
+      expect(mocha.useInlineDiffs(), 'to be', mocha);
     });
   });
 });