From 709a09efd74ee4a83fbfbe0236fab3443c65cda0 Mon Sep 17 00:00:00 2001 From: 38elements <38elements@users.noreply.github.com> Date: Thu, 7 Dec 2017 09:16:51 +0900 Subject: [PATCH 1/4] remove unused code in ms module * Remove unused argument of lib/ms * Remove unnecessary comment --- lib/ms.js | 48 ++++---------------------------------------- test/unit/ms.spec.js | 21 ------------------- 2 files changed, 4 insertions(+), 65 deletions(-) diff --git a/lib/ms.js b/lib/ms.js index 9590856052..2fdcaf3242 100644 --- a/lib/ms.js +++ b/lib/ms.js @@ -13,22 +13,15 @@ var y = d * 365.25; /** * Parse or format the given `val`. * - * Options: - * - * - `long` verbose formatting [false] - * * @api public * @param {string|number} val - * @param {Object} options * @return {string|number} */ -module.exports = function (val, options) { - options = options || {}; +module.exports = function (val) { if (typeof val === 'string') { return parse(val); } - // https://github.com/mochajs/mocha/pull/1035 - return options['long'] ? longFormat(val) : shortFormat(val); + return format(val); }; /** @@ -74,13 +67,13 @@ function parse (str) { } /** - * Short format for `ms`. + * Format for `ms`. * * @api private * @param {number} ms * @return {string} */ -function shortFormat (ms) { +function format (ms) { if (ms >= d) { return Math.round(ms / d) + 'd'; } @@ -95,36 +88,3 @@ function shortFormat (ms) { } return ms + 'ms'; } - -/** - * Long format for `ms`. - * - * @api private - * @param {number} ms - * @return {string} - */ -function longFormat (ms) { - return plural(ms, d, 'day') || - plural(ms, h, 'hour') || - plural(ms, m, 'minute') || - plural(ms, s, 'second') || - ms + ' ms'; -} - -/** - * Pluralization helper. - * - * @api private - * @param {number} ms - * @param {number} n - * @param {string} name - */ -function plural (ms, n, name) { - if (ms < n) { - return; - } - if (ms < n * 1.5) { - return Math.floor(ms / n) + ' ' + name; - } - return Math.ceil(ms / n) + ' ' + name + 's'; -} diff --git a/test/unit/ms.spec.js b/test/unit/ms.spec.js index e7b1d358be..0844391258 100644 --- a/test/unit/ms.spec.js +++ b/test/unit/ms.spec.js @@ -21,45 +21,24 @@ describe('.ms()', function () { it('should return short format', function () { expect(ms(2000)).to.equal('2s'); }); - - it('should return long format', function () { - expect(ms(2000, { long: true })).to.equal('2 seconds'); - expect(ms(1000, { long: true })).to.equal('1 second'); - expect(ms(1010, { long: true })).to.equal('1 second'); - }); }); describe('minutes representation', function () { it('should return short format', function () { expect(ms(time.minutes(1))).to.equal('1m'); }); - - it('should return long format', function () { - expect(ms(time.minutes(1), { long: true })).to.equal('1 minute'); - expect(ms(time.minutes(3), { long: true })).to.equal('3 minutes'); - }); }); describe('hours representation', function () { it('should return short format', function () { expect(ms(time.hours(1))).to.equal('1h'); }); - - it('should return long format', function () { - expect(ms(time.hours(1), { long: true })).to.equal('1 hour'); - expect(ms(time.hours(3), { long: true })).to.equal('3 hours'); - }); }); describe('days representation', function () { it('should return short format', function () { expect(ms(time.days(1))).to.equal('1d'); }); - - it('should return long format', function () { - expect(ms(time.days(1), { long: true })).to.equal('1 day'); - expect(ms(time.days(3), { long: true })).to.equal('3 days'); - }); }); describe('Getting string value', function () { From 45083862119f88586f9a2fa54f7253fe2a9641a6 Mon Sep 17 00:00:00 2001 From: Aaron Brady Date: Wed, 6 Dec 2017 17:39:53 -0800 Subject: [PATCH 2/4] fix inaccurate diff output due to post-assertion object mutation (#3075) * have Base generate the strings for error messages immediately instead of delaying until epilogue, the reason for this is that it is possible to reference objects in errors that could be mutated after the error occurs, causing the printed error message to be misleading * re-add stringify during list to appease tests, open for discussion if this is correct behavior --- lib/reporters/base.js | 24 ++++++++++++++++-------- test/reporters/list.spec.js | 22 ++++++++++++++++++++++ 2 files changed, 38 insertions(+), 8 deletions(-) diff --git a/lib/reporters/base.js b/lib/reporters/base.js index 8b68a3d51d..0ebc5e7db3 100644 --- a/lib/reporters/base.js +++ b/lib/reporters/base.js @@ -155,6 +155,17 @@ exports.cursor = { } }; +function showDiff (err) { + return err && err.showDiff !== false && sameType(err.actual, err.expected) && err.expected !== undefined; +} + +function stringifyDiffObjs (err) { + if (!utils.isString(err.actual) || !utils.isString(err.expected)) { + err.actual = utils.stringify(err.actual); + err.expected = utils.stringify(err.expected); + } +} + /** * Output the given `failures` as a list. * @@ -183,8 +194,6 @@ exports.list = function (failures) { } var stack = err.stack || message; var index = message ? stack.indexOf(message) : -1; - var actual = err.actual; - var expected = err.expected; if (index === -1) { msg = message; @@ -200,12 +209,8 @@ exports.list = function (failures) { msg = 'Uncaught ' + msg; } // explicitly show diff - if (err.showDiff !== false && sameType(actual, expected) && expected !== undefined) { - if (!(utils.isString(actual) && utils.isString(expected))) { - err.actual = actual = utils.stringify(actual); - err.expected = expected = utils.stringify(expected); - } - + if (showDiff(err)) { + stringifyDiffObjs(err); fmt = color('error title', ' %s) %s:\n%s') + color('error stack', '\n%s\n'); var match = message.match(/^([^:]+): expected/); msg = '\n ' + color('error message', match ? match[1] : msg); @@ -290,6 +295,9 @@ function Base (runner) { runner.on('fail', function (test, err) { stats.failures = stats.failures || 0; stats.failures++; + if (showDiff(err)) { + stringifyDiffObjs(err); + } test.err = err; failures.push(test); }); diff --git a/test/reporters/list.spec.js b/test/reporters/list.spec.js index 4b3704204d..87978de011 100644 --- a/test/reporters/list.spec.js +++ b/test/reporters/list.spec.js @@ -192,6 +192,28 @@ describe('List reporter', function () { Base.cursor = cachedCursor; }); + it('should immediately construct fail strings', function () { + var actual = { a: 'actual' }; + var expected = { a: 'expected' }; + var test = {}; + var checked = false; + var err; + runner.on = function (event, callback) { + if (!checked && event === 'fail') { + err = new Error('fake failure object with actual/expected'); + err.actual = actual; + err.expected = expected; + err.showDiff = true; + callback(test, err); + checked = true; + } + }; + List.call({epilogue: function () {}}, runner); + + process.stdout.write = stdoutWrite; + expect(typeof err.actual).to.equal('string'); + expect(typeof err.expected).to.equal('string'); + }); }); describe('on end', function () { From 2dc09bc6c7970bc5f3ea42a0e7924876f5e1f184 Mon Sep 17 00:00:00 2001 From: Christopher Hiller Date: Wed, 6 Dec 2017 20:00:58 -0800 Subject: [PATCH 3/4] add diff tool artifacts to .gitignore [ci skip] --- .gitignore | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.gitignore b/.gitignore index 8298390254..d922a61222 100644 --- a/.gitignore +++ b/.gitignore @@ -15,3 +15,8 @@ coverage/ yarn.lock package-lock.json mocha.js +# artifacts from various diff tools +*_BACKUP_* +*_BASE_* +*_LOCAL_* +*_REMOTE_* From 2f8173a33812361c49a7e60a354f1ad5b66f5af4 Mon Sep 17 00:00:00 2001 From: Christopher Hiller Date: Wed, 6 Dec 2017 19:59:06 -0800 Subject: [PATCH 4/4] refactor "forbid pending" tests for DRY --- test/integration/options.spec.js | 84 ++++++++++---------------------- 1 file changed, 26 insertions(+), 58 deletions(-) diff --git a/test/integration/options.spec.js b/test/integration/options.spec.js index f77da38bb8..e4ecc96cd6 100644 --- a/test/integration/options.spec.js +++ b/test/integration/options.spec.js @@ -274,63 +274,28 @@ describe('options', function () { }); }); - it('fails if there are tests marked skip', function (done) { - run('options/forbid-pending/skip.js', args, function (err, res) { - if (err) { - done(err); - return; - } - assert.equal(res.code, 1); - assert.equal(res.failures[0].err.message, pendingErrorMessage); - done(); - }); - }); - - it('fails if there are pending tests', function (done) { - run('options/forbid-pending/pending.js', args, function (err, res) { - if (err) { - done(err); - return; - } - assert.equal(res.code, 1); - assert.equal(res.failures[0].err.message, pendingErrorMessage); - done(); - }); - }); - - it('fails if tests call `skip()`', function (done) { - run('options/forbid-pending/this.skip.js', args, function (err, res) { - assert(!err); - assert.equal(res.code, 1); - assert.equal(res.failures[0].err.message, pendingErrorMessage); - done(); - }); - }); - - it('fails if beforeEach calls `skip()`', function (done) { - run('options/forbid-pending/beforeEach-this.skip.js', args, function (err, res) { - assert(!err); - assert.equal(res.code, 1); - assert.equal(res.failures[0].err.message, pendingErrorMessage); - done(); - }); - }); - - it('fails if before calls `skip()`', function (done) { - run('options/forbid-pending/before-this.skip.js', args, function (err, res) { - assert(!err); - assert.equal(res.code, 1); - assert.equal(res.failures[0].err.message, pendingErrorMessage); - done(); - }); - }); + var forbidPendingFailureTests = { + 'fails if there are tests marked skip': 'skip.js', + 'fails if there are pending tests': 'pending.js', + 'fails if tests call `skip()`': 'this.skip.js', + 'fails if beforeEach calls `skip()`': 'beforeEach-this.skip.js', + 'fails if before calls `skip()`': 'before-this.skip.js', + 'fails if there are tests in suites marked skip': 'skip-suite.js' + }; - it('fails if there are tests in suites marked skip', function (done) { - run('options/forbid-pending/skip-suite.js', args, function (err, res) { - assert(!err); - assert.equal(res.code, 1); - assert.equal(res.failures[0].err.message, pendingErrorMessage); - done(); + Object.keys(forbidPendingFailureTests).forEach(function (title) { + it(title, function (done) { + run(path.join('options', 'forbid-pending', forbidPendingFailureTests[title]), + args, + function (err, res) { + if (err) { + done(err); + return; + } + assert.equal(res.code, 1); + assert.equal(res.failures[0].err.message, pendingErrorMessage); + done(); + }); }); }); }); @@ -344,7 +309,8 @@ describe('options', function () { /** * Returns a test that executes Mocha in a subprocess with either * `--exit`, `--no-exit`, or default behavior. - * @param {boolean} shouldExit - Expected result; `true` if Mocha should have force-killed the process. + * @param {boolean} shouldExit - Expected result; `true` if Mocha should + * have force-killed the process. * @param {string} [behavior] - 'enabled' or 'disabled' * @returns {Function} */ @@ -392,7 +358,9 @@ describe('options', function () { describe('--help', function () { it('works despite the presence of mocha.opts', function (done) { directInvoke(['-h'], function (error, result) { - if (error) { return done(error); } + if (error) { + return done(error); + } expect(result.output).to.contain('Usage:'); done(); }, path.join(__dirname, 'fixtures', 'options', 'help'));