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

Author: cspotcode

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() {

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);

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);

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 <suite "before all" hook> of file.+multiple-done-before\.fixture\.js/
+          message: /done\(\) called multiple times in hook <suite1 "before all" hook in "suite1"> 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 <suite "before each" hook> of file.+multiple-done-before-each\.fixture\.js/
-          }
-        },
-        {
-          fullTitle: 'suite "before each" hook in "suite"',
-          err: {
-            message: /done\(\) called multiple times in hook <suite "before each" 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 <suite1 "before each" hook in "suite1"> of file.+multiple-done-before-each\.fixture\.js/,
+          multiple: [
+            {
+              code: 'ERR_MOCHA_MULTIPLE_DONE'
+            }
+          ]
         }
-      ]);
+      });
     });
   });
 

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);
       });
     });
   });