Skip to content

Commit 9fe8493

Browse files
committed
refactor: address PR review feedback
- Extract error message creation into createHookSkipError() helper function to eliminate duplication across three locations - Simplify recursive forEach call in failAffectedTests() - Update test fixture and suite descriptions to use backticks for clarity (e.g., 'error in `before` hook')
1 parent 6d3d64e commit 9fe8493

File tree

4 files changed

+31
-29
lines changed

4 files changed

+31
-29
lines changed

lib/runner.js

Lines changed: 23 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -442,6 +442,22 @@ Runner.prototype.checkGlobals = function (test) {
442442
}
443443
};
444444

445+
/**
446+
* Create an error object for a test that was skipped due to a hook failure.
447+
*
448+
* @private
449+
* @param {string} hookTitle - The title of the failed hook
450+
* @param {Error} hookError - The error from the failed hook
451+
* @returns {Error} The error object for the skipped test
452+
*/
453+
function createHookSkipError(hookTitle, hookError) {
454+
var errorMessage =
455+
'Test skipped due to failure in "' + hookTitle + '": ' + hookError.message;
456+
var testError = new Error(errorMessage);
457+
testError.stack = hookError.stack;
458+
return testError;
459+
}
460+
445461
/**
446462
* Fail all tests that are affected by a hook failure.
447463
* This is used when the `failHookAffectedTests` option is enabled.
@@ -457,10 +473,7 @@ Runner.prototype.failAffectedTests = function (suite, hookError, hookTitle) {
457473
}
458474

459475
var self = this;
460-
var errorMessage =
461-
'Test skipped due to failure in "' + hookTitle + '": ' + hookError.message;
462-
var testError = new Error(errorMessage);
463-
testError.stack = hookError.stack;
476+
var testError = createHookSkipError(hookTitle, hookError);
464477

465478
// Recursively fail all tests in this suite and its child suites
466479
function failTestsInSuite(s) {
@@ -475,9 +488,7 @@ Runner.prototype.failAffectedTests = function (suite, hookError, hookTitle) {
475488
}
476489
});
477490

478-
s.suites.forEach(function (childSuite) {
479-
failTestsInSuite(childSuite);
480-
});
491+
s.suites.forEach(failTestsInSuite);
481492
}
482493

483494
failTestsInSuite(suite);
@@ -632,13 +643,7 @@ Runner.prototype.hook = function (name, fn) {
632643
} else if (name === HOOK_TYPE_BEFORE_EACH) {
633644
// Fail the current test
634645
if (self.test && !self.test.state) {
635-
var errorMessage =
636-
'Test skipped due to failure in "' +
637-
hook.title +
638-
'": ' +
639-
err.message;
640-
var testError = new Error(errorMessage);
641-
testError.stack = err.stack;
646+
var testError = createHookSkipError(hook.title, err);
642647

643648
self.test.state = STATE_FAILED;
644649
self.failures++;
@@ -819,13 +824,10 @@ Runner.prototype.runTests = function (suite, fn) {
819824
var remainingTests = tests.slice();
820825
remainingTests.forEach(function (t) {
821826
if (!t.state) {
822-
var errorMessage =
823-
'Test skipped due to failure in "' +
824-
self._failedBeforeEachHook.title +
825-
'": ' +
826-
self._failedBeforeEachHook.error.message;
827-
var testError = new Error(errorMessage);
828-
testError.stack = self._failedBeforeEachHook.error.stack;
827+
var testError = createHookSkipError(
828+
self._failedBeforeEachHook.title,
829+
self._failedBeforeEachHook.error
830+
);
829831

830832
t.state = STATE_FAILED;
831833
self.failures++;

test/integration/fixtures/hooks/before-each-hook-error-with-fail-affected.fixture.js

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
'use strict';
22

3-
describe('spec 1', function () {
3+
describe('fails `beforeEach` hook', function () {
44
beforeEach(function () {
5-
throw new Error('before each hook error');
5+
throw new Error('error in `beforeEach` hook');
66
});
77
it('test 1', function () {
88
// This should be reported as failed due to beforeEach hook failure
@@ -11,7 +11,7 @@ describe('spec 1', function () {
1111
// This should be reported as failed due to beforeEach hook failure
1212
});
1313
});
14-
describe('spec 2', function () {
14+
describe('passes normally', function () {
1515
it('test 3', function () {
1616
// This should pass normally
1717
});

test/integration/fixtures/hooks/before-hook-error-with-fail-affected.fixture.js

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
'use strict';
22

3-
describe('spec 1', function () {
3+
describe('fails `before` hook', function () {
44
before(function () {
5-
throw new Error('before hook error');
5+
throw new Error('error in `before` hook');
66
});
77
it('test 1', function () {
88
// This should be reported as failed due to before hook failure
@@ -11,7 +11,7 @@ describe('spec 1', function () {
1111
// This should be reported as failed due to before hook failure
1212
});
1313
});
14-
describe('spec 2', function () {
14+
describe('passes normally', function () {
1515
it('test 3', function () {
1616
// This should pass normally
1717
});

test/integration/hook-err.spec.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -268,7 +268,7 @@ describe("hook error handling", function () {
268268
});
269269

270270
describe("--fail-hook-affected-tests", function () {
271-
describe("before hook error", function () {
271+
describe("error in `before` hook", function () {
272272
it("should fail all affected tests", function (done) {
273273
runMochaJSON(
274274
"hooks/before-hook-error-with-fail-affected",
@@ -290,7 +290,7 @@ describe("hook error handling", function () {
290290
});
291291
});
292292

293-
describe("beforeEach hook error", function () {
293+
describe("error in `beforeEach` hook", function () {
294294
it("should fail all affected tests", function (done) {
295295
runMochaJSON(
296296
"hooks/before-each-hook-error-with-fail-affected",

0 commit comments

Comments
 (0)