From cdda8c707be155cfb88eacab09601029366ee0a0 Mon Sep 17 00:00:00 2001 From: juergba Date: Mon, 6 Apr 2020 08:21:58 +0200 Subject: [PATCH 1/2] fix before pending hook pattern --- lib/runnable.js | 14 ++++++++------ lib/runner.js | 41 ++++++++++++++++------------------------- 2 files changed, 24 insertions(+), 31 deletions(-) diff --git a/lib/runnable.js b/lib/runnable.js index 7d3011dc86..9e43da3100 100644 --- a/lib/runnable.js +++ b/lib/runnable.js @@ -295,6 +295,8 @@ Runnable.prototype.run = function(fn) { var finished; var emitted; + if (this.isPending()) return fn(); + // Sometimes the ctx exists, but it is not runnable if (ctx && ctx.runnable) { ctx.runnable(this); @@ -376,11 +378,7 @@ Runnable.prototype.run = function(fn) { // sync or promise-returning try { - if (this.isPending()) { - done(); - } else { - callFn(this.fn); - } + callFn(this.fn); } catch (err) { emitted = true; if (err instanceof Pending) { @@ -481,7 +479,11 @@ var constants = utils.defineConstants( /** * Value of `state` prop when a `Runnable` has passed */ - STATE_PASSED: 'passed' + STATE_PASSED: 'passed', + /** + * Value of `state` prop when a `Runnable` has been skipped by user + */ + STATE_PENDING: 'pending' } ); diff --git a/lib/runner.js b/lib/runner.js index 8e7c8736c0..0285315649 100644 --- a/lib/runner.js +++ b/lib/runner.js @@ -18,6 +18,7 @@ var HOOK_TYPE_BEFORE_ALL = Suite.constants.HOOK_TYPE_BEFORE_ALL; var EVENT_ROOT_SUITE_RUN = Suite.constants.EVENT_ROOT_SUITE_RUN; var STATE_FAILED = Runnable.constants.STATE_FAILED; var STATE_PASSED = Runnable.constants.STATE_PASSED; +var STATE_PENDING = Runnable.constants.STATE_PENDING; var dQuote = utils.dQuote; var ngettext = utils.ngettext; var sQuote = utils.sQuote; @@ -122,8 +123,7 @@ module.exports = Runner; * @public * @class * @param {Suite} suite Root suite - * @param {boolean} [delay] Whether or not to delay execution of root suite - * until ready. + * @param {boolean} [delay] Whether to delay execution of root suite until ready. */ function Runner(suite, delay) { var self = this; @@ -285,11 +285,13 @@ Runner.prototype.checkGlobals = function(test) { * Fail the given `test`. * * @private - * @param {Test} test + * @param {Runnable} test * @param {Error} err + * @param {boolean} [force=false] - Whether to fail a pending test. */ -Runner.prototype.fail = function(test, err) { - if (test.isPending()) { +Runner.prototype.fail = function(test, err, force) { + force = force === true; + if (test.isPending() && !force) { return; } @@ -386,7 +388,7 @@ Runner.prototype.hook = function(name, fn) { }); } - hook.run(function(err) { + hook.run(function cbHookRun(err) { var testError = hook.error(); if (testError) { self.fail(self.test, testError); @@ -412,6 +414,7 @@ Runner.prototype.hook = function(name, fn) { suite.suites.forEach(function(suite) { suite.pending = true; }); + hooks = []; } else { hook.pending = false; var errForbid = createUnsupportedError('`this.skip` forbidden'); @@ -533,9 +536,6 @@ Runner.prototype.runTest = function(fn) { test.asyncOnly = true; } test.on('error', function(err) { - if (err instanceof Pending) { - return; - } self.fail(test, err); }); if (this.allowUncaught) { @@ -634,10 +634,9 @@ Runner.prototype.runTests = function(suite, fn) { // static skip, no hooks are executed if (test.isPending()) { if (self.forbidPending) { - test.isPending = alwaysFalse; - self.fail(test, new Error('Pending test forbidden')); - delete test.isPending; + self.fail(test, new Error('Pending test forbidden'), true); } else { + test.state = STATE_PENDING; self.emit(constants.EVENT_TEST_PENDING, test); } self.emit(constants.EVENT_TEST_END, test); @@ -650,10 +649,9 @@ Runner.prototype.runTests = function(suite, fn) { // conditional skip within beforeEach if (test.isPending()) { if (self.forbidPending) { - test.isPending = alwaysFalse; - self.fail(test, new Error('Pending test forbidden')); - delete test.isPending; + self.fail(test, new Error('Pending test forbidden'), true); } else { + test.state = STATE_PENDING; self.emit(constants.EVENT_TEST_PENDING, test); } self.emit(constants.EVENT_TEST_END, test); @@ -674,10 +672,9 @@ Runner.prototype.runTests = function(suite, fn) { // conditional skip within it if (test.pending) { if (self.forbidPending) { - test.isPending = alwaysFalse; - self.fail(test, new Error('Pending test forbidden')); - delete test.isPending; + self.fail(test, new Error('Pending test forbidden'), true); } else { + test.state = STATE_PENDING; self.emit(constants.EVENT_TEST_PENDING, test); } self.emit(constants.EVENT_TEST_END, test); @@ -714,10 +711,6 @@ Runner.prototype.runTests = function(suite, fn) { next(); }; -function alwaysFalse() { - return false; -} - /** * Run the given `suite` and invoke the callback `fn()` when complete. * @@ -850,9 +843,7 @@ Runner.prototype.uncaught = function(err) { return; } else if (runnable.isPending()) { // report 'pending test' retrospectively as failed - runnable.isPending = alwaysFalse; - this.fail(runnable, err); - delete runnable.isPending; + this.fail(runnable, err, true); return; } From 48bb3be7b0a774d46a214cb04a5b326a23a9bd74 Mon Sep 17 00:00:00 2001 From: juergba Date: Mon, 6 Apr 2020 08:21:03 +0200 Subject: [PATCH 2/2] additional tests --- .../skip-async-before-hooks.fixture.js | 71 ++++++----- .../pending/skip-sync-before-hooks.fixture.js | 66 ++++++---- .../pending/skip-sync-before-inner.fixture.js | 38 ++++++ test/integration/pending.spec.js | 119 +++++++----------- 4 files changed, 164 insertions(+), 130 deletions(-) create mode 100644 test/integration/fixtures/pending/skip-sync-before-inner.fixture.js diff --git a/test/integration/fixtures/pending/skip-async-before-hooks.fixture.js b/test/integration/fixtures/pending/skip-async-before-hooks.fixture.js index 7afd6bc670..0aa1409e6a 100644 --- a/test/integration/fixtures/pending/skip-async-before-hooks.fixture.js +++ b/test/integration/fixtures/pending/skip-async-before-hooks.fixture.js @@ -1,59 +1,72 @@ 'use strict'; +var assert = require('assert'); -describe('outer suite', function () { - - before(function () { - console.log('outer before'); +describe('outer suite', function() { + var runOrder = []; + before(function() { + runOrder.push('outer before'); }); - it('should run this test', function () { }); - - describe('inner suite', function () { + it('should run test-1', function() { + runOrder.push('should run test-1'); + }); - before(function (done) { - console.log('inner before'); + describe('inner suite', function() { + before(function(done) { + runOrder.push('inner before'); var self = this; - setTimeout(function () { + setTimeout(function() { self.skip(); // done() is not required }, 0); }); - beforeEach(function () { - throw new Error('beforeEach should not run'); + before(function() { + runOrder.push('inner before-2 should not run'); }); - afterEach(function () { - throw new Error('afterEach should not run'); + beforeEach(function() { + runOrder.push('beforeEach should not run'); }); - it('should not run this test', function () { - throw new Error('inner suite test should not run'); + afterEach(function() { + runOrder.push('afterEach should not run'); + }); + + after(function() { + runOrder.push('inner after'); }); - after(function () { - console.log('inner after'); + it('should not run this test', function() { + throw new Error('inner suite test should not run'); }); - describe('skipped suite', function () { - before(function () { - console.log('skipped before'); + describe('skipped suite', function() { + before(function() { + runOrder.push('skipped suite before should not run'); }); - it('should not run this test', function () { + it('should not run this test', function() { throw new Error('skipped suite test should not run'); }); - after(function () { - console.log('skipped after'); + after(function() { + runOrder.push('skipped suite after should not run'); }); }); - }); - it('should run this test', function () { }); - - after(function () { - console.log('outer after'); + it('should run test-2', function() { + runOrder.push('should run test-2'); }); + after(function() { + runOrder.push('outer after'); + assert.deepStrictEqual(runOrder, [ + 'outer before', + 'should run test-1', 'should run test-2', + 'inner before', 'inner after', + 'outer after' + ]); + throw new Error('should throw this error'); + }); }); diff --git a/test/integration/fixtures/pending/skip-sync-before-hooks.fixture.js b/test/integration/fixtures/pending/skip-sync-before-hooks.fixture.js index 1649656202..629ba29552 100644 --- a/test/integration/fixtures/pending/skip-sync-before-hooks.fixture.js +++ b/test/integration/fixtures/pending/skip-sync-before-hooks.fixture.js @@ -1,57 +1,69 @@ 'use strict'; +var assert = require('assert'); -describe('outer suite', function () { - - before(function () { - console.log('outer before'); +describe('outer suite', function() { + var runOrder = []; + before(function() { + runOrder.push('outer before'); }); - it('should run this test', function () { }); + it('should run test-1', function() { + runOrder.push('should run test-1'); + }); - describe('inner suite', function () { - before(function () { + describe('inner suite', function() { + before(function() { + runOrder.push('inner before'); this.skip(); }); - before(function () { - console.log('inner before'); + before(function() { + runOrder.push('inner before-2 should not run'); }); - beforeEach(function () { - throw new Error('beforeEach should not run'); + beforeEach(function() { + runOrder.push('beforeEach should not run'); }); - afterEach(function () { - throw new Error('afterEach should not run'); + afterEach(function() { + runOrder.push('afterEach should not run'); }); - after(function () { - console.log('inner after'); + after(function() { + runOrder.push('inner after'); }); - it('should never run this test', function () { + it('should never run this test', function() { throw new Error('inner suite test should not run'); }); - describe('skipped suite', function () { - before(function () { - console.log('skipped before'); + describe('skipped suite', function() { + before(function() { + runOrder.push('skipped suite before should not run'); }); - it('should never run this test', function () { + it('should never run this test', function() { throw new Error('skipped suite test should not run'); }); - after(function () { - console.log('skipped after'); + after(function() { + runOrder.push('skipped suite after should not run'); }); }); }); - it('should run this test', function () { }); - - after(function () { - console.log('outer after'); - }) + it('should run test-2', function() { + runOrder.push('should run test-2'); + }); + after(function() { + runOrder.push('outer after'); + assert.deepStrictEqual(runOrder, [ + 'outer before', + 'should run test-1', 'should run test-2', + 'inner before', 'inner after', + 'outer after' + ]); + throw new Error('should throw this error'); + }); }); diff --git a/test/integration/fixtures/pending/skip-sync-before-inner.fixture.js b/test/integration/fixtures/pending/skip-sync-before-inner.fixture.js new file mode 100644 index 0000000000..a5389a43e5 --- /dev/null +++ b/test/integration/fixtures/pending/skip-sync-before-inner.fixture.js @@ -0,0 +1,38 @@ +'use strict'; +var assert = require('assert'); + +describe('outer suite', function() { + var runOrder = []; + before(function() { + runOrder.push('outer before'); + this.skip(); + }); + + it('should never run this outer test', function() { + throw new Error('outer suite test should not run'); + }); + + describe('inner suite', function() { + before(function() { runOrder.push('no inner before'); }); + before(function(done) { runOrder.push('no inner before'); done(); }); + before(async function() { runOrder.push('no inner before'); }); + before(function() { return Promise.resolve(runOrder.push('no inner before')) }); + + after(function() { runOrder.push('no inner after'); }); + after(function(done) { runOrder.push('no inner after'); done(); }); + after(async function() { runOrder.push('no inner after'); }); + after(function() { return Promise.resolve(runOrder.push('no inner after')) }); + + it('should never run this inner test', function() { + throw new Error('inner suite test should not run'); + }); + }); + + after(function() { + runOrder.push('outer after'); + assert.deepStrictEqual(runOrder, [ + 'outer before', 'outer after' + ]); + throw new Error('should throw this error'); + }); +}); diff --git a/test/integration/pending.spec.js b/test/integration/pending.spec.js index ae6a57f0ca..51a7bc9b16 100644 --- a/test/integration/pending.spec.js +++ b/test/integration/pending.spec.js @@ -3,8 +3,6 @@ var assert = require('assert'); var helpers = require('./helpers'); var run = helpers.runMochaJSON; -var runMocha = helpers.runMocha; -var splitRegExp = helpers.splitRegExp; var invokeNode = helpers.invokeNode; var toJSONRunResult = helpers.toJSONRunResult; var args = []; @@ -111,43 +109,38 @@ describe('pending', function() { }); }); it('should run before and after hooks', function(done) { - runMocha( - 'pending/skip-sync-before-hooks.fixture.js', - args.concat(['--reporter', 'dot']), - function(err, res) { - if (err) { - done(err); - return; - } - - var lines = res.output - .split(splitRegExp) - .map(function(line) { - return line.trim(); - }) - .filter(function(line) { - return line.length; - }) - .slice(0, -1); - - var expected = [ - 'outer before', - 'inner before', - 'inner after', - 'outer after' - ]; - - assert.strictEqual(res.pending, 2); - assert.strictEqual(res.passing, 2); - assert.strictEqual(res.failing, 0); - assert.strictEqual(res.code, 0); - expected.forEach(function(line, i) { - assert.strictEqual(true, lines[i].includes(line)); - }); - - done(); + run('pending/skip-sync-before-hooks.fixture.js', function(err, res) { + if (err) { + return done(err); } - ); + expect(res, 'to have failed with error', 'should throw this error') + .and('to have failed test count', 1) + .and('to have pending test count', 2) + .and('to have passed test count', 2) + .and( + 'to have passed test order', + 'should run test-1', + 'should run test-2' + ); + done(); + }); + }); + it('should skip all sync/async inner before/after hooks', function(done) { + run('pending/skip-sync-before-inner.fixture.js', function(err, res) { + if (err) { + return done(err); + } + expect(res, 'to have failed with error', 'should throw this error') + .and('to have failed test count', 1) + .and('to have pending test count', 2) + .and('to have passed test count', 0) + .and( + 'to have pending test order', + 'should never run this outer test', + 'should never run this inner test' + ); + done(); + }); }); }); @@ -245,43 +238,21 @@ describe('pending', function() { }); }); it('should run before and after hooks', function(done) { - runMocha( - 'pending/skip-async-before-hooks.fixture.js', - args.concat(['--reporter', 'dot']), - function(err, res) { - if (err) { - done(err); - return; - } - - var lines = res.output - .split(splitRegExp) - .map(function(line) { - return line.trim(); - }) - .filter(function(line) { - return line.length; - }) - .slice(0, -1); - - var expected = [ - 'outer before', - 'inner before', - 'inner after', - 'outer after' - ]; - - assert.strictEqual(res.pending, 2); - assert.strictEqual(res.passing, 2); - assert.strictEqual(res.failing, 0); - assert.strictEqual(res.code, 0); - expected.forEach(function(line, i) { - assert.strictEqual(true, lines[i].includes(line)); - }); - - done(); + run('pending/skip-async-before-hooks.fixture.js', function(err, res) { + if (err) { + return done(err); } - ); + expect(res, 'to have failed with error', 'should throw this error') + .and('to have failed test count', 1) + .and('to have pending test count', 2) + .and('to have passed test count', 2) + .and( + 'to have passed test order', + 'should run test-1', + 'should run test-2' + ); + done(); + }); }); });