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

@@ -27,7 +27,7 @@
     "default": {
       "includeDate": false,
       "outputSourceFiles": true
-  },
+    },
     "monospaceLinks": false
   }
 }

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

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`.
  *

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

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() {