From 78848932cef7e3feab9184a161b9899b7c024c82 Mon Sep 17 00:00:00 2001 From: Andrew Bradley Date: Wed, 29 Jul 2020 16:44:39 -0400 Subject: [PATCH] ensure hook titles are consistent; closes #4348 (PR #4383) * fix #4348 * Addressing PR feedback * rename "suite" to "suite1" to make test cases clearer * addressing PR feedback --- lib/runner.js | 76 +++++++++---------- .../multiple-done-before-each.fixture.js | 2 +- .../fixtures/multiple-done-before.fixture.js | 2 +- test/integration/multiple-done.spec.js | 27 +++---- test/unit/runner.spec.js | 70 ++++++++++++----- 5 files changed, 98 insertions(+), 79 deletions(-) diff --git a/lib/runner.js b/lib/runner.js index 1214121e1e..cd91173e10 100644 --- a/lib/runner.js +++ b/lib/runner.js @@ -360,6 +360,19 @@ Runner.prototype.checkGlobals = function(test) { /** * Fail the given `test`. * + * If `test` is a hook, failures work in the following pattern: + * - If bail, run corresponding `after each` and `after` hooks, + * then exit + * - Failed `before` hook skips all tests in a suite and subsuites, + * but jumps to corresponding `after` hook + * - Failed `before each` hook skips remaining tests in a + * suite and jumps to corresponding `after each` hook, + * which is run only once + * - Failed `after` hook does not alter execution order + * - Failed `after each` hook skips remaining tests in a + * suite and subsuites, but executes other `after each` + * hooks + * * @private * @param {Runnable} test * @param {Error} err @@ -398,44 +411,6 @@ Runner.prototype.fail = function(test, err, force) { this.emit(constants.EVENT_TEST_FAIL, test, err); }; -/** - * Fail the given `hook` with `err`. - * - * Hook failures work in the following pattern: - * - If bail, run corresponding `after each` and `after` hooks, - * then exit - * - Failed `before` hook skips all tests in a suite and subsuites, - * but jumps to corresponding `after` hook - * - Failed `before each` hook skips remaining tests in a - * suite and jumps to corresponding `after each` hook, - * which is run only once - * - Failed `after` hook does not alter execution order - * - Failed `after each` hook skips remaining tests in a - * suite and subsuites, but executes other `after each` - * hooks - * - * @private - * @param {Hook} hook - * @param {Error} err - */ -Runner.prototype.failHook = function(hook, err) { - hook.originalTitle = hook.originalTitle || hook.title; - if (hook.ctx && hook.ctx.currentTest) { - hook.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 ' + dQuote(parentTitle); - } - - this.fail(hook, err); -}; - /** * Run hook `name` callbacks and then invoke `fn()`. * @@ -464,13 +439,15 @@ Runner.prototype.hook = function(name, fn) { hook.ctx.currentTest = self.test; } + setHookTitle(hook); + hook.allowUncaught = self.allowUncaught; self.emit(constants.EVENT_HOOK_BEGIN, hook); if (!hook.listeners('error').length) { self._addEventListener(hook, 'error', function(err) { - self.failHook(hook, err); + self.fail(hook, err); }); } @@ -504,18 +481,35 @@ Runner.prototype.hook = function(name, fn) { } else { hook.pending = false; var errForbid = createUnsupportedError('`this.skip` forbidden'); - self.failHook(hook, errForbid); + self.fail(hook, errForbid); return fn(errForbid); } } else if (err) { - self.failHook(hook, err); + self.fail(hook, err); // stop executing hooks, notify callee of hook err return fn(err); } self.emit(constants.EVENT_HOOK_END, hook); delete hook.ctx.currentTest; + setHookTitle(hook); next(++i); }); + + function setHookTitle(hook) { + hook.originalTitle = hook.originalTitle || hook.title; + if (hook.ctx && hook.ctx.currentTest) { + hook.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 ' + dQuote(parentTitle); + } + } } Runner.immediately(function() { diff --git a/test/integration/fixtures/multiple-done-before-each.fixture.js b/test/integration/fixtures/multiple-done-before-each.fixture.js index 32de2bf442..6c93167cfb 100644 --- a/test/integration/fixtures/multiple-done-before-each.fixture.js +++ b/test/integration/fixtures/multiple-done-before-each.fixture.js @@ -1,6 +1,6 @@ 'use strict'; -describe('suite', function () { +describe('suite1', function () { beforeEach(function (done) { setTimeout(done, 10); setTimeout(done, 20); diff --git a/test/integration/fixtures/multiple-done-before.fixture.js b/test/integration/fixtures/multiple-done-before.fixture.js index 1e1bc71a16..cfa18de0c0 100644 --- a/test/integration/fixtures/multiple-done-before.fixture.js +++ b/test/integration/fixtures/multiple-done-before.fixture.js @@ -1,6 +1,6 @@ 'use strict'; -describe('suite', function () { +describe('suite1', function () { before(function (done) { setTimeout(done, 10); setTimeout(done, 30); diff --git a/test/integration/multiple-done.spec.js b/test/integration/multiple-done.spec.js index 2019df25c5..b7348ab488 100644 --- a/test/integration/multiple-done.spec.js +++ b/test/integration/multiple-done.spec.js @@ -95,9 +95,9 @@ describe('multiple calls to done()', function() { it('correctly attributes the error', function() { expect(res.failures[0], 'to satisfy', { - fullTitle: 'suite "before all" hook in "suite"', + fullTitle: 'suite1 "before all" hook in "suite1"', err: { - message: /done\(\) called multiple times in hook of file.+multiple-done-before\.fixture\.js/ + message: /done\(\) called multiple times in hook of file.+multiple-done-before\.fixture\.js/ } }); }); @@ -119,20 +119,17 @@ describe('multiple calls to done()', function() { }); it('correctly attributes the errors', function() { - expect(res.failures, 'to satisfy', [ - { - fullTitle: 'suite "before each" hook in "suite"', - err: { - message: /done\(\) called multiple times in hook of file.+multiple-done-before-each\.fixture\.js/ - } - }, - { - fullTitle: 'suite "before each" hook in "suite"', - err: { - message: /done\(\) called multiple times in hook of file.+multiple-done-before-each\.fixture\.js/ - } + expect(res.failures[0], 'to equal', res.failures[1]).and('to satisfy', { + fullTitle: 'suite1 "before each" hook in "suite1"', + err: { + message: /done\(\) called multiple times in hook of file.+multiple-done-before-each\.fixture\.js/, + multiple: [ + { + code: 'ERR_MOCHA_MULTIPLE_DONE' + } + ] } - ]); + }); }); }); diff --git a/test/unit/runner.spec.js b/test/unit/runner.spec.js index 28a152fc1e..730c40003d 100644 --- a/test/unit/runner.spec.js +++ b/test/unit/runner.spec.js @@ -12,7 +12,9 @@ var Hook = Mocha.Hook; var noop = Mocha.utils.noop; var errors = require('../../lib/errors'); var EVENT_HOOK_BEGIN = Runner.constants.EVENT_HOOK_BEGIN; +var EVENT_HOOK_END = Runner.constants.EVENT_HOOK_END; var EVENT_TEST_FAIL = Runner.constants.EVENT_TEST_FAIL; +var EVENT_TEST_PASS = Runner.constants.EVENT_TEST_PASS; var EVENT_TEST_RETRY = Runner.constants.EVENT_TEST_RETRY; var EVENT_TEST_END = Runner.constants.EVENT_TEST_END; var EVENT_RUN_END = Runner.constants.EVENT_RUN_END; @@ -252,6 +254,44 @@ describe('Runner', function() { runner.hook('afterEach', noop); runner.hook('afterAll', noop); }); + + it('should augment hook title with current test title', function(done) { + var expectedHookTitle; + function assertHookTitle() { + expect(hook.title, 'to be', expectedHookTitle); + } + var failHook = false; + var hookError = new Error('failed hook'); + suite.beforeEach(function() { + assertHookTitle(); + if (failHook) { + throw hookError; + } + }); + runner.on(EVENT_HOOK_BEGIN, assertHookTitle); + runner.on(EVENT_HOOK_END, assertHookTitle); + runner.on(EVENT_TEST_FAIL, assertHookTitle); + runner.on(EVENT_TEST_PASS, assertHookTitle); + var hook = suite._beforeEach[0]; + + suite.addTest(new Test('should behave', noop)); + suite.addTest(new Test('should obey', noop)); + runner.suite = suite; + + runner.test = suite.tests[0]; + expectedHookTitle = '"before each" hook for "should behave"'; + runner.hook('beforeEach', function(err) { + if (err && err !== hookError) return done(err); + + runner.test = suite.tests[1]; + failHook = true; + expectedHookTitle = '"before each" hook for "should obey"'; + runner.hook('beforeEach', function(err) { + if (err && err !== hookError) return done(err); + return done(); + }); + }); + }); }); describe('fail()', function() { @@ -418,31 +458,19 @@ describe('Runner', function() { }); }); - describe('.failHook(hook, err)', function() { + describe('.fail(hook, err)', function() { it('should increment .failures', function() { expect(runner.failures, 'to be', 0); var test1 = new Test('fail hook 1', noop); var test2 = new Test('fail hook 2', noop); suite.addTest(test1); suite.addTest(test2); - runner.failHook(test1, new Error('error1')); + runner.fail(test1, new Error('error1')); expect(runner.failures, 'to be', 1); - runner.failHook(test2, new Error('error2')); + runner.fail(test2, new Error('error2')); expect(runner.failures, 'to be', 2); }); - it('should augment hook title with current test title', function() { - var hook = new Hook('"before each" hook'); - hook.ctx = {currentTest: new Test('should behave', noop)}; - - runner.failHook(hook, {}); - expect(hook.title, 'to be', '"before each" hook for "should behave"'); - - hook.ctx.currentTest = new Test('should obey', noop); - runner.failHook(hook, {}); - expect(hook.title, 'to be', '"before each" hook for "should obey"'); - }); - it('should emit "fail"', function(done) { var hook = new Hook(); hook.parent = suite; @@ -452,7 +480,7 @@ describe('Runner', function() { expect(_err, 'to be', err); done(); }); - runner.failHook(hook, err); + runner.fail(hook, err); }); it('should not emit "end" if suite bail is not true', function(done) { @@ -462,7 +490,7 @@ describe('Runner', function() { suite.bail(false); expect( function() { - runner.failHook(hook, err); + runner.fail(hook, err); }, 'not to emit from', hook, @@ -727,7 +755,7 @@ describe('Runner', function() { expect(_err.stack, 'to be', stack.slice(0, 3).join('\n')); done(); }); - runner.failHook(hook, err); + runner.fail(hook, err); }); }); @@ -745,7 +773,7 @@ describe('Runner', function() { expect(_err.stack, 'to be', stack.join('\n')); done(); }); - runner.failHook(hook, err); + runner.fail(hook, err); }); }); @@ -796,7 +824,7 @@ describe('Runner', function() { ); done(); }); - runner.failHook(hook, err); + runner.fail(hook, err); }); it('should not hang if overlong error message is multiple lines', function(done) { @@ -816,7 +844,7 @@ describe('Runner', function() { ); done(); }); - runner.failHook(hook, err); + runner.fail(hook, err); }); }); });