From c16a7e7f17bc256394e8e9ca11e7adbb86d72d52 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Fri, 15 Feb 2019 09:08:47 -0500 Subject: [PATCH] Refactor out usages of Suite#_onlyTests and Suite#_onlyTests (#3689) (#3707) * Refactor out usages of Suite#_onlyTests and Suite#_onlyTests (#3689) * Mark new Suite methods as private * Make hasOnly() and filterOnly() consistently return boolean * Add test coverage for Suite#hasOnly() and Suite#filterOnly() * Refactor tests re: code review comments --- jsdoc.conf.json | 2 +- lib/interfaces/common.js | 4 +- lib/runner.js | 56 +++---------------------- lib/suite.js | 67 ++++++++++++++++++++++++++++++ test/unit/suite.spec.js | 89 ++++++++++++++++++++++++++++++++++++++++ 5 files changed, 164 insertions(+), 54 deletions(-) diff --git a/jsdoc.conf.json b/jsdoc.conf.json index 1407bf170e..161b1fd252 100644 --- a/jsdoc.conf.json +++ b/jsdoc.conf.json @@ -27,7 +27,7 @@ "default": { "includeDate": false, "outputSourceFiles": true - }, + }, "monospaceLinks": false } } diff --git a/lib/interfaces/common.js b/lib/interfaces/common.js index 6384c7d477..976b4d7a65 100644 --- a/lib/interfaces/common.js +++ b/lib/interfaces/common.js @@ -130,7 +130,7 @@ module.exports = function(suites, context, mocha) { throw new Error('`.only` forbidden'); } - suite.parent._onlySuites = suite.parent._onlySuites.concat(suite); + suite.parent.appendOnlySuite(suite); } if (suite.pending) { if (mocha.options.forbidPending && shouldBeTested(suite)) { @@ -175,7 +175,7 @@ module.exports = function(suites, context, mocha) { * @returns {*} */ only: function(mocha, test) { - test.parent._onlyTests = test.parent._onlyTests.concat(test); + test.parent.appendOnlyTest(test); return test; }, diff --git a/lib/runner.js b/lib/runner.js index 647b4e5c92..fc69a687cf 100644 --- a/lib/runner.js +++ b/lib/runner.js @@ -505,7 +505,9 @@ Runner.prototype.runTest = function(fn) { if (!test) { return; } - if (this.forbidOnly && hasOnly(this.parents().reverse()[0] || this.suite)) { + + var suite = this.parents().reverse()[0] || this.suite; + if (this.forbidOnly && suite.hasOnly()) { fn(new Error('`.only` forbidden')); return; } @@ -874,8 +876,8 @@ Runner.prototype.run = function(fn) { function start() { // If there is an `only` filter - if (hasOnly(rootSuite)) { - filterOnly(rootSuite); + if (rootSuite.hasOnly()) { + rootSuite.filterOnly(); } self.started = true; if (self._delay) { @@ -932,54 +934,6 @@ Runner.prototype.abort = function() { return this; }; -/** - * Filter suites based on `isOnly` logic. - * - * @param {Array} suite - * @returns {Boolean} - * @private - */ -function filterOnly(suite) { - if (suite._onlyTests.length) { - // If the suite contains `only` tests, run those and ignore any nested suites. - suite.tests = suite._onlyTests; - suite.suites = []; - } else { - // Otherwise, do not run any of the tests in this suite. - suite.tests = []; - suite._onlySuites.forEach(function(onlySuite) { - // If there are other `only` tests/suites nested in the current `only` suite, then filter that `only` suite. - // Otherwise, all of the tests on this `only` suite should be run, so don't filter it. - if (hasOnly(onlySuite)) { - filterOnly(onlySuite); - } - }); - // Run the `only` suites, as well as any other suites that have `only` tests/suites as descendants. - suite.suites = suite.suites.filter(function(childSuite) { - return ( - suite._onlySuites.indexOf(childSuite) !== -1 || filterOnly(childSuite) - ); - }); - } - // Keep the suite only if there is something to run - return suite.tests.length || suite.suites.length; -} - -/** - * Determines whether a suite has an `only` test or suite as a descendant. - * - * @param {Array} suite - * @returns {Boolean} - * @private - */ -function hasOnly(suite) { - return ( - suite._onlyTests.length || - suite._onlySuites.length || - suite.suites.some(hasOnly) - ); -} - /** * Filter leaks with the given globals flagged as `ok`. * diff --git a/lib/suite.js b/lib/suite.js index e2fb75bd41..13c6ec792a 100644 --- a/lib/suite.js +++ b/lib/suite.js @@ -434,6 +434,73 @@ Suite.prototype.run = function run() { } }; +/** + * Determines whether a suite has an `only` test or suite as a descendant. + * + * @private + * @returns {Boolean} + */ +Suite.prototype.hasOnly = function hasOnly() { + return ( + this._onlyTests.length > 0 || + this._onlySuites.length > 0 || + this.suites.some(function(suite) { + return suite.hasOnly(); + }) + ); +}; + +/** + * Filter suites based on `isOnly` logic. + * + * @private + * @returns {Boolean} + */ +Suite.prototype.filterOnly = function filterOnly() { + if (this._onlyTests.length) { + // If the suite contains `only` tests, run those and ignore any nested suites. + this.tests = this._onlyTests; + this.suites = []; + } else { + // Otherwise, do not run any of the tests in this suite. + this.tests = []; + this._onlySuites.forEach(function(onlySuite) { + // If there are other `only` tests/suites nested in the current `only` suite, then filter that `only` suite. + // Otherwise, all of the tests on this `only` suite should be run, so don't filter it. + if (onlySuite.hasOnly()) { + onlySuite.filterOnly(); + } + }); + // Run the `only` suites, as well as any other suites that have `only` tests/suites as descendants. + var onlySuites = this._onlySuites; + this.suites = this.suites.filter(function(childSuite) { + return onlySuites.indexOf(childSuite) !== -1 || childSuite.filterOnly(); + }); + } + // Keep the suite only if there is something to run + return this.tests.length > 0 || this.suites.length > 0; +}; + +/** + * Adds a suite to the list of subsuites marked `only`. + * + * @private + * @param {Suite} suite + */ +Suite.prototype.appendOnlySuite = function(suite) { + this._onlySuites.push(suite); +}; + +/** + * Adds a test to the list of tests marked `only`. + * + * @private + * @param {Test} test + */ +Suite.prototype.appendOnlyTest = function(test) { + this._onlyTests.push(test); +}; + /** * Returns the array of hooks by hook name; see `HOOK_TYPE_*` constants. * @private diff --git a/test/unit/suite.spec.js b/test/unit/suite.spec.js index 971208b9b9..2a1d639c53 100644 --- a/test/unit/suite.spec.js +++ b/test/unit/suite.spec.js @@ -545,6 +545,95 @@ describe('Suite', function() { expect(suite.timeout(), 'to be', 100); }); }); + + describe('hasOnly()', function() { + it('should return true if a test has `only`', function() { + var suite = new Suite('foo'); + var test = new Test('bar'); + + suite.appendOnlyTest(test); + + expect(suite.hasOnly(), 'to be', true); + }); + + it('should return true if a suite has `only`', function() { + var suite = new Suite('foo'); + var nested = new Suite('bar'); + + suite.appendOnlySuite(nested); + + expect(suite.hasOnly(), 'to be', true); + }); + + it('should return true if nested suite has `only`', function() { + var suite = new Suite('foo'); + var nested = new Suite('bar'); + var test = new Test('baz'); + + nested.appendOnlyTest(test); + // `nested` has a `only` test, but `suite` doesn't know about it + suite.suites.push(nested); + + expect(suite.hasOnly(), 'to be', true); + }); + + it('should return false if no suite or test is marked `only`', function() { + var suite = new Suite('foo'); + var nested = new Suite('bar'); + var test = new Test('baz'); + + suite.suites.push(nested); + nested.tests.push(test); + + expect(suite.hasOnly(), 'to be', false); + }); + }); + + describe('.filterOnly()', function() { + it('should filter out all other tests and suites if a test has `only`', function() { + var suite = new Suite('a'); + var nested = new Suite('b'); + var test = new Test('c'); + var test2 = new Test('d'); + + suite.suites.push(nested); + suite.appendOnlyTest(test); + suite.tests.push(test2); + + suite.filterOnly(); + + expect(suite, 'to satisfy', { + suites: expect.it('to be empty'), + tests: expect + .it('to have length', 1) + .and('to have an item satisfying', {title: 'c'}) + }); + }); + + it('should filter out all other tests and suites if a suite has `only`', function() { + var suite = new Suite('a'); + var nested1 = new Suite('b'); + var nested2 = new Suite('c'); + var test = new Test('d'); + var nestedTest = new Test('e'); + + nested1.appendOnlyTest(nestedTest); + + suite.tests.push(test); + suite.suites.push(nested1); + suite.appendOnlySuite(nested1); + suite.suites.push(nested2); + + suite.filterOnly(); + + expect(suite, 'to satisfy', { + suites: expect + .it('to have length', 1) + .and('to have an item satisfying', {title: 'b'}), + tests: expect.it('to be empty') + }); + }); + }); }); describe('Test', function() {