Skip to content

Commit

Permalink
Fix Hook Test Names (#3573); closes #1638, closes #3291, closes #2134
Browse files Browse the repository at this point in the history
### Description of bug
Mocha shows an incorrect test-title if a nested `before` hook fails. For `after` hooks the second part of the message is completely missing.
```js
describe('Top-level describe', () => {
  it('Top-level it', () => { });       // the failing before-hook is pointing to this test
  describe('Nested describe', () => {
    before(() => {
      throw new Error();
    });
    it('Nested it', () => { });        // but it should point to this test
  });
});
```
```
Top-level describe
    ✓ Top-level it
    Nested describe
      1) "before all" hook for "Top-level it"       <<<incorrect
```
Root problem: see [#1638](#1638). `hook.ctx.currentTest` is set incorrectly in the hook before emitting the hook event:
- `before` hook: currentTest is undefined or points to the outer test already past successfully.
The runner hasn't reached the next test to come yet, so it's too early to derive currentTest from the runner. Instead the first test of hook's parent is used.
- `after` hook: currentTest is undefined.
The runner has already completed the latest test and runner.currentTest is deleted. Instead the last test of hook's parent is used.

### Description of the Change
#### 1. Setting currentTest correctly
- `before` hook: hook.ctx.currentTest is set to the first test of the parent suite
- `after hook`: hook.ctx.currentTest is set to the last test of the parent suite
- for nested suites just the current suite is searched for tests to come. When no tests can be found, currentTest remains undefined.

#### 2. Creating title in hook failure message
A correct `hook.ctx.currentTest` renders a correct title. 
When no currentTest can be found, the title of the parent suite is used for the hook failure message.

### Alternate Designs
PR [#3333](#3333)

### Benefits

This is a rather old bug back to 2015.

### Applicable issues
closes [#1638](#1638)
closes [#3291](#3291)
closes [#2134](#2134)

**EDITED by @boneskull**: added js syntax highlighting, comments
  • Loading branch information
juergba authored and boneskull committed Jan 28, 2019
1 parent 1e30501 commit 1f36ec5
Show file tree
Hide file tree
Showing 14 changed files with 271 additions and 13 deletions.
18 changes: 16 additions & 2 deletions lib/runner.js
Expand Up @@ -265,10 +265,18 @@ Runner.prototype.fail = function(test, err) {
* @param {Error} err
*/
Runner.prototype.failHook = function(hook, err) {
hook.originalTitle = hook.originalTitle || hook.title;
if (hook.ctx && hook.ctx.currentTest) {
hook.originalTitle = hook.originalTitle || hook.title;
hook.title =
hook.originalTitle + ' for "' + 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 + '"';
}

this.fail(hook, err);
Expand All @@ -294,7 +302,13 @@ Runner.prototype.hook = function(name, fn) {
}
self.currentRunnable = hook;

hook.ctx.currentTest = self.test;
if (name === 'beforeAll') {
hook.ctx.currentTest = hook.parent.tests[0];
} else if (name === 'afterAll') {
hook.ctx.currentTest = hook.parent.tests[hook.parent.tests.length - 1];
} else {
hook.ctx.currentTest = self.test;
}

self.emit('hook', hook);

Expand Down
47 changes: 47 additions & 0 deletions test/integration/fixtures/current-test-title.fixture.js
@@ -0,0 +1,47 @@
'use strict';
var assert = require('assert');

function getTitle(ctx) {
return ctx.currentTest && ctx.currentTest.title;
};

before(function () {
assert.equal(getTitle(this), undefined);
});

describe('suite A', () => {

before(function () {
assert.equal(getTitle(this), undefined);
});

describe('suite B', () => {

it('test1 B', () => {});

describe('suite C', function () {
var lap = 0;

before(function () {
assert.equal(getTitle(this), 'test1 C');
});
beforeEach(function () {
assert.equal(getTitle(this), ++lap === 1 ? 'test1 C' : 'test2 C');
});

it('test1 C', function () {});
it('test2 C', function () {});

afterEach(function () {
assert.equal(getTitle(this), lap === 1 ? 'test1 C' : 'test2 C');
});
after(function () {
assert.equal(getTitle(this), 'test2 C');
});
});
});
});

after(function () {
assert.equal(getTitle(this), undefined);
});
@@ -0,0 +1,13 @@
'use strict';

describe('spec 1', function () {
it('should pass', function () { });
describe('spec 2 nested - this title should be used', function () {
after(function() {
throw new Error('after hook nested error');
});
describe('spec 3 nested', function () {
it('it nested - this title should not be used', function () { });
});
});
});
14 changes: 14 additions & 0 deletions test/integration/fixtures/hooks/after-hook-nested-error.fixture.js
@@ -0,0 +1,14 @@
'use strict';

describe('spec 1', function () {
it('should pass', function () { });
describe('spec nested', function () {
after(function() {
throw new Error('after hook nested error');
});
it('it nested - this title should be used', function () { });
});
describe('spec 2 nested', function () {
it('it nested - not this title', function () { });
});
});
@@ -0,0 +1,13 @@
'use strict';

describe('spec 1', function () {
it('should pass', function () { });
describe('spec 2 nested - this title should be used', function () {
before(function() {
throw new Error('before hook nested error');
});
describe('spec 3 nested', function () {
it('it nested - this title should not be used', function () { });
});
});
});
@@ -0,0 +1,11 @@
'use strict';

describe('spec 1', function () {
it('should pass', function () { });
describe('spec nested', function () {
before(function() {
throw new Error('before hook nested error');
});
it('it nested - this title should be used', function () { });
});
});
@@ -0,0 +1,9 @@
'use strict';

before(function() {
throw new Error('before hook root error');
});

describe('spec 1', function () {
it('should not be called', function () { });
});
Expand Up @@ -50,7 +50,7 @@ describe('suite2', function () {
before('before suite2', function () {});
beforeEach('beforeEach suite2', function () {});
it('test suite2', function () {
runOrder.push('test suite2 - should not run');
console.log('test suite2 - should not run');
});
afterEach('afterEach suite2', function () {});
after('after suite2', function () {});
Expand Down
Expand Up @@ -37,7 +37,7 @@ describe('suite2', function () {
before('before suite2', function () {});
beforeEach('beforeEach suite2', function () {});
it('test suite2', function () {
runOrder.push('test suite2 - should not run');
console.log('test suite2 - should not run');
});
afterEach('afterEach suite2', function () {});
after('after suite2', function () {});
Expand Down
106 changes: 105 additions & 1 deletion test/integration/hook-err.spec.js
Expand Up @@ -2,6 +2,7 @@

var helpers = require('./helpers');
var runMocha = helpers.runMocha;
var runMochaJSON = require('./helpers').runMochaJSON;
var splitRegExp = helpers.splitRegExp;
var bang = require('../../lib/reporters/base').symbols.bang;

Expand All @@ -18,7 +19,63 @@ describe('hook error handling', function() {
describe('before hook error tip', function() {
before(run('hooks/before-hook-error-tip.fixture.js', onlyErrorTitle()));
it('should verify results', function() {
expect(lines, 'to equal', ['1) spec 2', '"before all" hook:']);
expect(lines, 'to equal', [
'1) spec 2',
'"before all" hook for "skipped":'
]);
});
});

describe('before hook root error', function() {
it('should verify results', function(done) {
var fixture = 'hooks/before-hook-root-error.fixture.js';
runMochaJSON(fixture, [], function(err, res) {
if (err) {
return done(err);
}
expect(res, 'to have failed with error', 'before hook root error')
.and('to have failed test', '"before all" hook in "{root}"')
.and('to have passed test count', 0);
done();
});
});
});

describe('before hook nested error', function() {
it('should verify results', function(done) {
var fixture = 'hooks/before-hook-nested-error.fixture.js';
runMochaJSON(fixture, [], function(err, res) {
if (err) {
return done(err);
}
expect(res, 'to have failed with error', 'before hook nested error')
.and(
'to have failed test',
'"before all" hook for "it nested - this title should be used"'
)
.and('to have passed test count', 1)
.and('to have passed test', 'should pass');
done();
});
});
});

describe('before hook deepnested error', function() {
it('should verify results', function(done) {
var fixture = 'hooks/before-hook-deepnested-error.fixture.js';
runMochaJSON(fixture, [], function(err, res) {
if (err) {
return done(err);
}
expect(res, 'to have failed with error', 'before hook nested error')
.and(
'to have failed test',
'"before all" hook in "spec 2 nested - this title should be used"'
)
.and('to have passed test count', 1)
.and('to have passed test', 'should pass');
done();
});
});
});

Expand All @@ -36,6 +93,53 @@ describe('hook error handling', function() {
});
});

describe('after hook nested error', function() {
it('should verify results', function(done) {
var fixture = 'hooks/after-hook-nested-error.fixture.js';
runMochaJSON(fixture, [], function(err, res) {
if (err) {
return done(err);
}
expect(res, 'to have failed with error', 'after hook nested error')
.and(
'to have failed test',
'"after all" hook for "it nested - this title should be used"'
)
.and('to have passed test count', 3)
.and(
'to have passed test order',
'should pass',
'it nested - this title should be used',
'it nested - not this title'
);
done();
});
});
});

describe('after hook deepnested error', function() {
it('should verify results', function(done) {
var fixture = 'hooks/after-hook-deepnested-error.fixture.js';
runMochaJSON(fixture, [], function(err, res) {
if (err) {
return done(err);
}
expect(res, 'to have failed with error', 'after hook nested error')
.and(
'to have failed test',
'"after all" hook in "spec 2 nested - this title should be used"'
)
.and('to have passed test count', 2)
.and(
'to have passed test order',
'should pass',
'it nested - this title should not be used'
);
done();
});
});
});

describe('after each hook error', function() {
before(run('hooks/afterEach-hook-error.fixture.js'));
it('should verify results', function() {
Expand Down
13 changes: 13 additions & 0 deletions test/integration/hooks.spec.js
Expand Up @@ -2,6 +2,7 @@

var assert = require('assert');
var runMocha = require('./helpers').runMocha;
var runMochaJSON = require('./helpers').runMochaJSON;
var splitRegExp = require('./helpers').splitRegExp;
var args = ['--reporter', 'dot'];

Expand Down Expand Up @@ -49,4 +50,16 @@ describe('hooks', function() {
done();
});
});

it('current test title of all hooks', function(done) {
runMochaJSON('current-test-title.fixture.js', [], function(err, res) {
if (err) {
return done(err);
}
expect(res, 'to have passed')
.and('to have passed test count', 3)
.and('to have passed test order', 'test1 B', 'test1 C', 'test2 C');
done();
});
});
});
10 changes: 8 additions & 2 deletions test/integration/multiple-done.spec.js
Expand Up @@ -90,7 +90,10 @@ describe('multiple calls to done()', function() {
});

it('correctly attributes the error', function() {
assert.strictEqual(res.failures[0].fullTitle, 'suite "before all" hook');
assert.strictEqual(
res.failures[0].fullTitle,
'suite "before all" hook in "suite"'
);
assert.strictEqual(
res.failures[0].err.message,
'done() called multiple times'
Expand All @@ -116,7 +119,10 @@ describe('multiple calls to done()', function() {
it('correctly attributes the errors', function() {
assert.strictEqual(res.failures.length, 2);
res.failures.forEach(function(failure) {
assert.strictEqual(failure.fullTitle, 'suite "before each" hook');
assert.strictEqual(
failure.fullTitle,
'suite "before each" hook in "suite"'
);
assert.strictEqual(failure.err.message, 'done() called multiple times');
});
});
Expand Down
10 changes: 8 additions & 2 deletions test/integration/options/bail.spec.js
Expand Up @@ -52,7 +52,10 @@ describe('--bail', function() {

expect(res, 'to have failed')
.and('to have failed test count', 1)
.and('to have failed test', '"before all" hook: before suite1')
.and(
'to have failed test',
'"before all" hook: before suite1 for "test suite1"'
)
.and('to have passed test count', 0);
done();
});
Expand Down Expand Up @@ -100,7 +103,10 @@ describe('--bail', function() {

expect(res, 'to have failed')
.and('to have failed test count', 1)
.and('to have failed test', '"after all" hook: after suite1A')
.and(
'to have failed test',
'"after all" hook: after suite1A for "test suite1A"'
)
.and('to have passed test count', 2)
.and('to have passed test order', 'test suite1', 'test suite1A');
done();
Expand Down

0 comments on commit 1f36ec5

Please sign in to comment.