Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix #4348 #4383

Merged
merged 13 commits into from Jul 29, 2020
Merged

Fix #4348 #4383

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
fe806f4
fix #4348
cspotcode Jul 26, 2020
2e03e57
fix
cspotcode Jul 26, 2020
597597e
fix
cspotcode Jul 26, 2020
002c432
fix
cspotcode Jul 26, 2020
680b14f
fix
cspotcode Jul 26, 2020
aea7b65
fix
cspotcode Jul 27, 2020
3ad9fa3
fix
cspotcode Jul 27, 2020
31febbf
fix
cspotcode Jul 27, 2020
c149bdb
fix
cspotcode Jul 27, 2020
46cf90a
fix
cspotcode Jul 27, 2020
299cfa4
Addressing PR feedback
cspotcode Jul 28, 2020
564b189
rename "suite" to "suite1" to make test cases clearer
cspotcode Jul 28, 2020
fe88c03
addressing PR feedback
cspotcode Jul 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 19 additions & 14 deletions lib/runner.js
View file
Open in desktop
Expand Up @@ -419,20 +419,6 @@ Runner.prototype.fail = function(test, err, force) {
* @param {Error} err
*/
Runner.prototype.failHook = function(hook, err) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could probably just remove failHook and replace its usage with calls directly to fail

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

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

Expand Down Expand Up @@ -464,6 +450,8 @@ Runner.prototype.hook = function(name, fn) {
hook.ctx.currentTest = self.test;
}

setHookTitle(hook);

hook.allowUncaught = self.allowUncaught;

self.emit(constants.EVENT_HOOK_BEGIN, hook);
Expand Down Expand Up @@ -514,8 +502,25 @@ Runner.prototype.hook = function(name, fn) {
}
self.emit(constants.EVENT_HOOK_END, hook);
delete hook.ctx.currentTest;
setHookTitle(hook);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When hook.ctx.currentTest is deleted, this changes the hook's title so that it no longer mentions the test case and only mentions the test suite. It affects the "multiple done calls" errors.

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() {
Expand Down
26 changes: 12 additions & 14 deletions test/integration/multiple-done.spec.js
View file
Open in desktop
Expand Up @@ -97,7 +97,7 @@ describe('multiple calls to done()', function() {
expect(res.failures[0], 'to satisfy', {
fullTitle: 'suite "before all" hook in "suite"',
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 <suite "before all" hook in "suite"> of file.+multiple-done-before\.fixture\.js/
boneskull marked this conversation as resolved.
Show resolved Hide resolved
}
});
});
Expand All @@ -119,20 +119,18 @@ 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]);
expect(res.failures[0], 'to satisfy', {
fullTitle: 'suite "before each" hook in "suite"',
err: {
message: /done\(\) called multiple times in hook <suite "before each" hook in "suite"> of file.+multiple-done-before-each\.fixture\.js/,
multiple: [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this error object is weird, but I guess that's what it does.

you can combine this into a single assertion:

expect(res.failures[0], 'to equal', res.failures[1])
  .and('to satisfy', {
    // etc.
  });

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(likewise the assertion(s) in the new test may be able to be combined this way)

Copy link
Contributor Author

@cspotcode cspotcode Jul 28, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you probably already know this, but I rewrote the assertion to point out that both items of the array actually refer to the same object. I can't do a === check in the tests, because it's been converted to/from JSON, so the assertions are running against 2 different parsed copies of the same object. Does 'to equal' handle that? EDIT: disregard; I'm already using 'to equal' I see what you're saying.

This happens because when a single test/hook has multiple failures, the first error is reused, and the multiple array is created to store the others. Ideally I think mocha should be creating a new "hookinvocation" instance for each time the hook runs, where each "hookinvocation" can store a reference to the test case it's paired to. But I don't think I'll have time for that size of code change.

{
code: 'ERR_MOCHA_MULTIPLE_DONE'
}
]
}
]);
});
});
});

Expand Down
52 changes: 40 additions & 12 deletions test/unit/runner.spec.js
View file
Open in desktop
Expand Up @@ -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;
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -431,18 +471,6 @@ describe('Runner', function() {
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;
Expand Down