From 4d0b5169ce4359fff7cf9bceeeae1e04afac3e71 Mon Sep 17 00:00:00 2001 From: "P. Roebuck" Date: Mon, 11 Feb 2019 14:07:52 -0600 Subject: [PATCH] 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 | 14 +++--- lib/runner.js | 24 ++++++--- lib/utils.js | 79 +++++++++++++++++++++++++++++- test/integration/reporters.spec.js | 8 ++- test/unit/mocha.spec.js | 2 +- test/unit/runner.spec.js | 23 ++++----- test/unit/utils.spec.js | 39 +++++++++++++++ 7 files changed, 155 insertions(+), 34 deletions(-) diff --git a/lib/mocha.js b/lib/mocha.js index 9886ded56d..1537196167 100644 --- a/lib/mocha.js +++ b/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 ); } diff --git a/lib/runner.js b/lib/runner.js index d9784a9672..647b4e5c92 100644 --- a/lib/runner.js +++ b/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,7 +329,7 @@ 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) { @@ -329,7 +337,7 @@ Runner.prototype.failHook = function(hook, err) { } else { parentTitle = hook.parent.root ? '{root}' : ''; } - hook.title = hook.originalTitle + ' in "' + parentTitle + '"'; + hook.title = hook.originalTitle + ' in ' + dQuote(parentTitle); } this.fail(hook, err); diff --git a/lib/utils.js b/lib/utils.js index 51d77668a8..82d4017603 100644 --- a/lib/utils.js +++ b/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. + * + * package 'foo' cannot be found + * + * @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. + * + * argument 'value' must be "string" or "number" + * + * @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 diff --git a/test/integration/reporters.spec.js b/test/integration/reporters.spec.js index d44a222324..944f94f552 100644 --- a/test/integration/reporters.spec.js +++ b/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') diff --git a/test/unit/mocha.spec.js b/test/unit/mocha.spec.js index 580533622c..beb5e2b5e0 100644 --- a/test/unit/mocha.spec.js +++ b/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' }); diff --git a/test/unit/runner.spec.js b/test/unit/runner.spec.js index f48144ee47..d524ea9ba2 100644 --- a/test/unit/runner.spec.js +++ b/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'), diff --git a/test/unit/utils.spec.js b/test/unit/utils.spec.js index bd0360362d..970c77c125 100644 --- a/test/unit/utils.spec.js +++ b/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);