Refactor checkGlobals() error message creation (#3711)

* Added sQuote() and dQuote() functions for quoting text inline
* Added ngettext() function for message translation for dealing with plurality
* Refactored portions of code to use the aforementioned functions
* Various fixes to tests

lib/mocha.js

@@ -20,6 +20,7 @@ var createInvalidInterfaceError = errors.createInvalidInterfaceError;
 var EVENT_FILE_PRE_REQUIRE = Suite.constants.EVENT_FILE_PRE_REQUIRE;
 var EVENT_FILE_POST_REQUIRE = Suite.constants.EVENT_FILE_POST_REQUIRE;
 var EVENT_FILE_REQUIRE = Suite.constants.EVENT_FILE_REQUIRE;
+var sQuote = utils.sQuote;
 
 exports = module.exports = Mocha;
 
@@ -227,24 +228,23 @@ Mocha.prototype.reporter = function(reporter, reporterOptions) {
           } catch (_err) {
             _err.code !== 'MODULE_NOT_FOUND' ||
             _err.message.indexOf('Cannot find module') !== -1
-              ? console.warn('"' + reporter + '" reporter not found')
+              ? console.warn(sQuote(reporter) + ' reporter not found')
               : console.warn(
-                  '"' +
-                    reporter +
-                    '" reporter blew up with error:\n' +
+                  sQuote(reporter) +
+                    ' reporter blew up with error:\n' +
                     err.stack
                 );
           }
         } else {
           console.warn(
-            '"' + reporter + '" reporter blew up with error:\n' + err.stack
+            sQuote(reporter) + ' reporter blew up with error:\n' + err.stack
           );
         }
       }
     }
     if (!_reporter) {
       throw createInvalidReporterError(
-        'invalid reporter "' + reporter + '"',
+        'invalid reporter ' + sQuote(reporter),
         reporter
       );
     }
@@ -273,7 +273,7 @@ Mocha.prototype.ui = function(name) {
       this._ui = require(name);
     } catch (err) {
       throw createInvalidInterfaceError(
-        'invalid interface "' + name + '"',
+        'invalid interface ' + sQuote(name),
         name
       );
     }

lib/runner.js

@@ -1,5 +1,9 @@
 'use strict';
 
+/**
+ * Module dependencies.
+ */
+var util = require('util');
 var EventEmitter = require('events').EventEmitter;
 var Pending = require('./pending');
 var utils = require('./utils');
@@ -14,6 +18,9 @@ var HOOK_TYPE_BEFORE_ALL = Suite.constants.HOOK_TYPE_BEFORE_ALL;
 var EVENT_ROOT_SUITE_RUN = Suite.constants.EVENT_ROOT_SUITE_RUN;
 var STATE_FAILED = Runnable.constants.STATE_FAILED;
 var STATE_PASSED = Runnable.constants.STATE_PASSED;
+var dQuote = utils.dQuote;
+var ngettext = utils.ngettext;
+var sQuote = utils.sQuote;
 var stackFilter = utils.stackTraceFilter();
 var stringify = utils.stringify;
 var type = utils.type;
@@ -257,13 +264,14 @@ Runner.prototype.checkGlobals = function(test) {
   leaks = filterLeaks(ok, globals);
   this._globals = this._globals.concat(leaks);
 
-  if (leaks.length > 1) {
-    this.fail(
-      test,
-      new Error('global leaks detected: ' + leaks.join(', ') + '')
+  if (leaks.length) {
+    var format = ngettext(
+      leaks.length,
+      'global leak detected: %s',
+      'global leaks detected: %s'
     );
-  } else if (leaks.length) {
-    this.fail(test, new Error('global leak detected: ' + leaks[0]));
+    var error = new Error(util.format(format, leaks.map(sQuote).join(', ')));
+    this.fail(test, error);
   }
 };
 
@@ -321,15 +329,15 @@ Runner.prototype.failHook = function(hook, err) {
   hook.originalTitle = hook.originalTitle || hook.title;
   if (hook.ctx && hook.ctx.currentTest) {
     hook.title =
-      hook.originalTitle + ' for "' + hook.ctx.currentTest.title + '"';
+      hook.originalTitle + ' for ' + dQuote(hook.ctx.currentTest.title);
   } else {
     var parentTitle;
     if (hook.parent.title) {
       parentTitle = hook.parent.title;
     } else {
       parentTitle = hook.parent.root ? '{root}' : '';
     }
-    hook.title = hook.originalTitle + ' in "' + parentTitle + '"';
+    hook.title = hook.originalTitle + ' in ' + dQuote(parentTitle);
   }
 
   this.fail(hook, err);

lib/utils.js

@@ -13,6 +13,7 @@ var debug = require('debug')('mocha:watch');
 var fs = require('fs');
 var glob = require('glob');
 var path = require('path');
+var util = require('util');
 var join = path.join;
 var he = require('he');
 var errors = require('./errors');
@@ -530,7 +531,7 @@ exports.lookupFiles = function lookupFiles(filepath, extensions, recursive) {
       files = glob.sync(filepath);
       if (!files.length) {
         throw createNoFilesMatchPatternError(
-          'Cannot find any files matching pattern "' + filepath + '"',
+          'Cannot find any files matching pattern ' + exports.dQuote(filepath),
           filepath
         );
       }
@@ -564,7 +565,11 @@ exports.lookupFiles = function lookupFiles(filepath, extensions, recursive) {
     }
     if (!extensions) {
       throw createMissingArgumentError(
-        'Argument "extensions" required when argument "filepath" is a directory',
+        util.format(
+          'Argument %s required when argument %s is a directory',
+          exports.sQuote('extensions'),
+          exports.sQuote('filepath')
+        ),
         'extensions',
         'array'
       );
@@ -715,6 +720,76 @@ exports.clamp = function clamp(value, range) {
   return Math.min(Math.max(value, range[0]), range[1]);
 };
 
+/**
+ * Single quote text by combining with undirectional ASCII quotation marks.
+ *
+ * @description
+ * Provides a simple means of markup for quoting text to be used in output.
+ * Use this to quote names of variables, methods, and packages.
+ *
+ * <samp>package 'foo' cannot be found</samp>
+ *
+ * @private
+ * @param {string} str - Value to be quoted.
+ * @returns {string} quoted value
+ * @example
+ * sQuote('n') // => 'n'
+ */
+exports.sQuote = function(str) {
+  return "'" + str + "'";
+};
+
+/**
+ * Double quote text by combining with undirectional ASCII quotation marks.
+ *
+ * @description
+ * Provides a simple means of markup for quoting text to be used in output.
+ * Use this to quote names of datatypes, classes, pathnames, and strings.
+ *
+ * <samp>argument 'value' must be "string" or "number"</samp>
+ *
+ * @private
+ * @param {string} str - Value to be quoted.
+ * @returns {string} quoted value
+ * @example
+ * dQuote('number') // => "number"
+ */
+exports.dQuote = function(str) {
+  return '"' + str + '"';
+};
+
+/**
+ * Provides simplistic message translation for dealing with plurality.
+ *
+ * @description
+ * Use this to create messages which need to be singular or plural.
+ * Some languages have several plural forms, so _complete_ message clauses
+ * are preferable to generating the message on the fly.
+ *
+ * @private
+ * @param {number} n - Non-negative integer
+ * @param {string} msg1 - Message to be used in English for `n = 1`
+ * @param {string} msg2 - Message to be used in English for `n = 0, 2, 3, ...`
+ * @returns {string} message corresponding to value of `n`
+ * @example
+ * var sprintf = require('util').format;
+ * var pkgs = ['one', 'two'];
+ * var msg = sprintf(
+ *   ngettext(
+ *     pkgs.length,
+ *     'cannot load package: %s',
+ *     'cannot load packages: %s'
+ *   ),
+ *   pkgs.map(sQuote).join(', ')
+ * );
+ * console.log(msg); // => cannot load packages: 'one', 'two'
+ */
+exports.ngettext = function(n, msg1, msg2) {
+  if (typeof n === 'number' && n >= 0) {
+    return n === 1 ? msg1 : msg2;
+  }
+};
+
 /**
  * It's a noop.
  * @public

test/integration/reporters.spec.js

@@ -5,6 +5,8 @@ var fs = require('fs');
 var crypto = require('crypto');
 var path = require('path');
 var run = require('./helpers').runMocha;
+var utils = require('../../lib/utils');
+var dQuote = utils.dQuote;
 
 describe('reporters', function() {
   describe('markdown', function() {
@@ -213,13 +215,9 @@ describe('reporters', function() {
             return;
           }
 
-          function dquote(s) {
-            return '"' + s + '"';
-          }
-
           var pattern =
             '^Error: invalid or unsupported TAP version: ' +
-            dquote(invalidTapVersion);
+            dQuote(invalidTapVersion);
           expect(res, 'to satisfy', {
             code: 1,
             output: new RegExp(pattern, 'm')

test/unit/mocha.spec.js

@@ -234,7 +234,7 @@ describe('Mocha', function() {
         new Mocha(updatedOpts);
       };
       expect(throwError, 'to throw', {
-        message: 'invalid reporter "invalidReporter"',
+        message: "invalid reporter 'invalidReporter'",
         code: 'ERR_MOCHA_INVALID_REPORTER',
         reporter: 'invalidReporter'
       });

test/unit/runner.spec.js

@@ -117,7 +117,7 @@ describe('Runner', function() {
       global.foo = 'bar';
       runner.on(EVENT_TEST_FAIL, function(_test, _err) {
         expect(_test, 'to be', test);
-        expect(_err, 'to have message', 'global leak detected: foo');
+        expect(_err, 'to have message', "global leak detected: 'foo'");
         delete global.foo;
         done();
       });
@@ -171,7 +171,7 @@ describe('Runner', function() {
       global.bar = 'baz';
       runner.on(EVENT_TEST_FAIL, function(_test, _err) {
         expect(_test, 'to be', test);
-        expect(_err, 'to have message', 'global leaks detected: foo, bar');
+        expect(_err, 'to have message', "global leaks detected: 'foo', 'bar'");
         delete global.foo;
         delete global.bar;
         done();
@@ -201,22 +201,23 @@ describe('Runner', function() {
 
       suite.addTest(test);
 
-      global.foo = 'bar';
-      global.bar = 'baz';
+      global.foo = 'whitelisted';
+      global.bar = 'detect-me';
       runner.on(EVENT_TEST_FAIL, function(_test, _err) {
         expect(_test.title, 'to be', 'im a test about lions');
-        expect(_err, 'to have message', 'global leak detected: bar');
+        expect(_err, 'to have message', "global leak detected: 'bar'");
         delete global.foo;
+        delete global.bar;
         done();
       });
       runner.checkGlobals(test);
     });
 
-    it('should emit "fail" when a global beginning with d is introduced', function(done) {
+    it('should emit "fail" when a global beginning with "d" is introduced', function(done) {
       global.derp = 'bar';
-      runner.on(EVENT_TEST_FAIL, function(test, err) {
-        expect(test.title, 'to be', 'herp');
-        expect(err.message, 'to be', 'global leak detected: derp');
+      runner.on(EVENT_TEST_FAIL, function(_test, _err) {
+        expect(_test.title, 'to be', 'herp');
+        expect(_err, 'to have message', "global leak detected: 'derp'");
         delete global.derp;
         done();
       });
@@ -569,7 +570,7 @@ describe('Runner', function() {
         // Fake stack-trace
         err.stack = [message].concat(stack).join('\n');
 
-        runner.on('fail', function(_hook, _err) {
+        runner.on(EVENT_TEST_FAIL, function(_hook, _err) {
           var filteredErrStack = _err.stack.split('\n').slice(1);
           expect(
             filteredErrStack.join('\n'),
@@ -589,7 +590,7 @@ describe('Runner', function() {
         // Fake stack-trace
         err.stack = [message].concat(stack).join('\n');
 
-        runner.on('fail', function(_hook, _err) {
+        runner.on(EVENT_TEST_FAIL, function(_hook, _err) {
           var filteredErrStack = _err.stack.split('\n').slice(-3);
           expect(
             filteredErrStack.join('\n'),

test/unit/utils.spec.js

@@ -706,6 +706,45 @@ describe('lib/utils', function() {
     });
   });
 
+  describe('sQuote/dQuote', function() {
+    var str = 'xxx';
+
+    it('should return its input as string wrapped in single quotes', function() {
+      var expected = "'xxx'";
+      expect(utils.sQuote(str), 'to be', expected);
+    });
+
+    it('should return its input as string wrapped in double quotes', function() {
+      var expected = '"xxx"';
+      expect(utils.dQuote(str), 'to be', expected);
+    });
+  });
+
+  describe('ngettext', function() {
+    var singular = 'singular';
+    var plural = 'plural';
+
+    it("should return plural string if 'n' is 0", function() {
+      expect(utils.ngettext(0, singular, plural), 'to be', plural);
+    });
+
+    it("should return singular string if 'n' is 1", function() {
+      expect(utils.ngettext(1, singular, plural), 'to be', singular);
+    });
+
+    it("should return plural string if 'n' is greater than 1", function() {
+      var arr = ['aaa', 'bbb'];
+      expect(utils.ngettext(arr.length, singular, plural), 'to be', plural);
+    });
+
+    it("should return undefined if 'n' is not a non-negative integer", function() {
+      expect(utils.ngettext('', singular, plural), 'to be undefined');
+      expect(utils.ngettext(-1, singular, plural), 'to be undefined');
+      expect(utils.ngettext(true, singular, plural), 'to be undefined');
+      expect(utils.ngettext({}, singular, plural), 'to be undefined');
+    });
+  });
+
   describe('createMap', function() {
     it('should return an object with a null prototype', function() {
       expect(Object.getPrototypeOf(utils.createMap()), 'to be', null);