Skip to content

Commit 24c22be

Browse files
juergbajuergba
authored
Fix hook pattern of this.skip() in beforeEach hooks (#3741)
1 parent 1412dc8 commit 24c22be

File tree

5 files changed

+157
-33
lines changed

5 files changed

+157
-33
lines changed

‎lib/runner.js‎

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -315,8 +315,7 @@ Runner.prototype.fail = function(test, err) {
315315
* - Failed `before each` hook skips remaining tests in a
316316
* suite and jumps to corresponding `after each` hook,
317317
* which is run only once
318-
* - Failed `after` hook does not alter
319-
* execution order
318+
* - Failed `after` hook does not alter execution order
320319
* - Failed `after each` hook skips remaining tests in a
321320
* suite and subsuites, but executes other `after each`
322321
* hooks
@@ -386,19 +385,28 @@ Runner.prototype.hook = function(name, fn) {
386385
if (testError) {
387386
self.fail(self.test, testError);
388387
}
389-
// conditional this.skip()
388+
// conditional skip
390389
if (hook.pending) {
391390
if (name === HOOK_TYPE_AFTER_ALL) {
392391
utils.deprecate(
393392
'Skipping a test within an "after all" hook is DEPRECATED and will throw an exception in a future version of Mocha. ' +
394393
'Use a return statement or other means to abort hook execution.'
395394
);
396395
}
397-
if (name === HOOK_TYPE_BEFORE_EACH || name === HOOK_TYPE_AFTER_EACH) {
396+
if (name === HOOK_TYPE_AFTER_EACH) {
397+
// TODO define and implement use case
398398
if (self.test) {
399399
self.test.pending = true;
400400
}
401+
} else if (name === HOOK_TYPE_BEFORE_EACH) {
402+
if (self.test) {
403+
self.test.pending = true;
404+
}
405+
self.emit(constants.EVENT_HOOK_END, hook);
406+
hook.pending = false; // activates hook for next test
407+
return fn(new Error('abort hookDown'));
401408
} else {
409+
// TODO throw error for afterAll
402410
suite.tests.forEach(function(test) {
403411
test.pending = true;
404412
});
@@ -619,6 +627,7 @@ Runner.prototype.runTests = function(suite, fn) {
619627
return;
620628
}
621629

630+
// static skip, no hooks are executed
622631
if (test.isPending()) {
623632
if (self.forbidPending) {
624633
test.isPending = alwaysFalse;
@@ -634,6 +643,7 @@ Runner.prototype.runTests = function(suite, fn) {
634643
// execute test and hook(s)
635644
self.emit(constants.EVENT_TEST_BEGIN, (self.test = test));
636645
self.hookDown(HOOK_TYPE_BEFORE_EACH, function(err, errSuite) {
646+
// conditional skip within beforeEach
637647
if (test.isPending()) {
638648
if (self.forbidPending) {
639649
test.isPending = alwaysFalse;
@@ -643,15 +653,21 @@ Runner.prototype.runTests = function(suite, fn) {
643653
self.emit(constants.EVENT_TEST_PENDING, test);
644654
}
645655
self.emit(constants.EVENT_TEST_END, test);
646-
return next();
656+
// skip inner afterEach hooks below errSuite level
657+
var origSuite = self.suite;
658+
self.suite = errSuite;
659+
return self.hookUp(HOOK_TYPE_AFTER_EACH, function(e, eSuite) {
660+
self.suite = origSuite;
661+
next(e, eSuite);
662+
});
647663
}
648664
if (err) {
649665
return hookErr(err, errSuite, false);
650666
}
651667
self.currentRunnable = self.test;
652668
self.runTest(function(err) {
653669
test = self.test;
654-
// conditional this.skip()
670+
// conditional skip within it
655671
if (test.pending) {
656672
if (self.forbidPending) {
657673
test.isPending = alwaysFalse;

‎test/integration/fixtures/pending/skip-async-beforeEach.fixture.js‎

Lines changed: 37 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,48 @@
11
'use strict';
2+
var assert = require('assert');
23

3-
describe('skip in beforeEach', function () {
4-
beforeEach(function (done) {
4+
describe('skip in beforeEach', function() {
5+
var runOrder = [];
6+
beforeEach(function(done) {
7+
runOrder.push('beforeEach');
58
var self = this;
6-
setTimeout(function () {
9+
setTimeout(function() {
710
self.skip(); // done() is not required
8-
}, 0);
11+
}, 10);
912
});
1013

11-
it('should skip this test-1', function () {
14+
it('should skip this test-1', function() {
1215
throw new Error('never run this test');
1316
});
14-
it('should skip this test-2', function () {
15-
throw new Error('never run this test');
17+
18+
describe('inner', function() {
19+
beforeEach(function() {
20+
runOrder.push('should not run');
21+
});
22+
23+
it('should skip this test-2', function() {
24+
throw new Error('never run this test');
25+
});
26+
it('should skip this test-3', function() {
27+
throw new Error('never run this test');
28+
});
29+
30+
afterEach(function() {
31+
runOrder.push('should not run');
32+
});
1633
});
17-
it('should skip this test-3', function () {
18-
throw new Error('never run this test');
34+
35+
afterEach(function() {
36+
runOrder.push('afterEach');
37+
});
38+
after(function() {
39+
runOrder.push('after');
40+
assert.deepStrictEqual(runOrder, [
41+
'beforeEach', 'afterEach',
42+
'beforeEach', 'afterEach',
43+
'beforeEach', 'afterEach',
44+
'after'
45+
]);
46+
throw new Error('should throw this error');
1947
});
2048
});

‎test/integration/fixtures/pending/skip-sync-beforeEach-cond.fixture.js‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
'use strict';
2+
3+
describe('skip conditionally in beforeEach', function() {
4+
var n = 1;
5+
beforeEach(function() {
6+
if (n !== 2) {
7+
this.skip();
8+
}
9+
});
10+
11+
it('should skip this test-1', function() {
12+
throw new Error('never run this test');
13+
});
14+
it('should run this test-2', function() {});
15+
16+
describe('inner suite', function() {
17+
it('should skip this test-3', function() {
18+
throw new Error('never run this test');
19+
});
20+
});
21+
22+
afterEach(function() { n++; });
23+
after(function() {
24+
if (n === 4) {
25+
throw new Error('should throw this error');
26+
}
27+
});
28+
});

‎test/integration/fixtures/pending/skip-sync-beforeEach.fixture.js‎

Lines changed: 35 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,45 @@
11
'use strict';
2+
var assert = require('assert');
23

3-
describe('skip in beforeEach', function () {
4-
beforeEach(function () {
4+
describe('skip in beforeEach', function() {
5+
var runOrder = [];
6+
beforeEach(function() {
7+
runOrder.push('beforeEach');
58
this.skip();
69
});
710

8-
it('should never run this test', function () {
11+
it('should skip this test-1', function() {
912
throw new Error('never run this test');
1013
});
1114

12-
it('should never run this test', function () {
13-
throw new Error('never run this test');
15+
describe('inner', function() {
16+
beforeEach(function() {
17+
runOrder.push('should not run');
18+
});
19+
20+
it('should skip this test-2', function() {
21+
throw new Error('never run this test');
22+
});
23+
it('should skip this test-3', function() {
24+
throw new Error('never run this test');
25+
});
26+
27+
afterEach(function() {
28+
runOrder.push('should not run');
29+
});
30+
});
31+
32+
afterEach(function() {
33+
runOrder.push('afterEach');
34+
});
35+
after(function() {
36+
runOrder.push('after');
37+
assert.deepStrictEqual(runOrder, [
38+
'beforeEach', 'afterEach',
39+
'beforeEach', 'afterEach',
40+
'beforeEach', 'afterEach',
41+
'after'
42+
]);
43+
throw new Error('should throw this error');
1444
});
1545
});

‎test/integration/pending.spec.js‎

Lines changed: 35 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -170,18 +170,40 @@ describe('pending', function() {
170170

171171
describe('in beforeEach', function() {
172172
it('should skip all suite specs', function(done) {
173-
run('pending/skip-sync-beforeEach.fixture.js', args, function(
174-
err,
175-
res
176-
) {
173+
var fixture = 'pending/skip-sync-beforeEach.fixture.js';
174+
run(fixture, args, function(err, res) {
177175
if (err) {
178-
done(err);
179-
return;
176+
return done(err);
180177
}
181-
assert.strictEqual(res.stats.pending, 2);
182-
assert.strictEqual(res.stats.passes, 0);
183-
assert.strictEqual(res.stats.failures, 0);
184-
assert.strictEqual(res.code, 0);
178+
expect(res, 'to have failed with error', 'should throw this error')
179+
.and('to have failed test count', 1)
180+
.and('to have pending test count', 3)
181+
.and(
182+
'to have pending test order',
183+
'should skip this test-1',
184+
'should skip this test-2',
185+
'should skip this test-3'
186+
)
187+
.and('to have passed test count', 0);
188+
done();
189+
});
190+
});
191+
it('should skip only two suite specs', function(done) {
192+
var fixture = 'pending/skip-sync-beforeEach-cond.fixture.js';
193+
run(fixture, args, function(err, res) {
194+
if (err) {
195+
return done(err);
196+
}
197+
expect(res, 'to have failed with error', 'should throw this error')
198+
.and('to have failed test count', 1)
199+
.and('to have pending test count', 2)
200+
.and(
201+
'to have pending test order',
202+
'should skip this test-1',
203+
'should skip this test-3'
204+
)
205+
.and('to have passed test count', 1)
206+
.and('to have passed test', 'should run this test-2');
185207
done();
186208
});
187209
});
@@ -287,16 +309,16 @@ describe('pending', function() {
287309
if (err) {
288310
return done(err);
289311
}
290-
expect(res, 'to have passed')
291-
.and('to have passed test count', 0)
312+
expect(res, 'to have failed with error', 'should throw this error')
313+
.and('to have failed test count', 1)
292314
.and('to have pending test count', 3)
293315
.and(
294316
'to have pending test order',
295317
'should skip this test-1',
296318
'should skip this test-2',
297319
'should skip this test-3'
298320
)
299-
.and('to have failed test count', 0);
321+
.and('to have passed test count', 0);
300322
done();
301323
});
302324
});

0 commit comments

Comments
 (0)