move max worker defaults into buffered-worker-pool

- add some missing coverage in `test/node-unit/worker.spec.js` - remove a noisy debug in `lib/nodejs/worker.js`
mochajs · May 27, 2020 · fd92e62 · fd92e62
1 parent 155841a
commit fd92e62
Show file tree

Hide file tree

Showing 6 changed files with 52 additions and 37 deletions.
diff --git a/lib/cli/run.js b/lib/cli/run.js
@@ -25,7 +25,6 @@ const {ONE_AND_DONES, ONE_AND_DONE_ARGS} = require('./one-and-dones');
 const debug = require('debug')('mocha:cli:run');
 const defaults = require('../mocharc');
 const {types, aliases} = require('./run-option-metadata');
-const coreCount = require('os').cpus().length;
 
 /**
  * Logical option groups
@@ -157,8 +156,7 @@ exports.builder = yargs =>
           'Number of concurrent jobs for --parallel; use 1 to run in serial',
         defaultDescription: '(number of CPU cores - 1)',
         requiresArg: true,
-        group: GROUPS.RULES,
-        default: Math.max(2, coreCount - 1)
+        group: GROUPS.RULES
       },
       'list-interfaces': {
         conflicts: Array.from(ONE_AND_DONE_ARGS),

diff --git a/lib/mocha.js b/lib/mocha.js
@@ -197,13 +197,12 @@ function Mocha(options) {
    */
   this.isWorker = Boolean(options.isWorker);
 
-  if (options.parallel) {
-    if (options.jobs > 1) {
-      debug('attempting to enable parallel mode');
-      this.parallelMode(true);
-    } else {
-      debug('parallel mode requested, but job count < 2');
-    }
+  if (
+    options.parallel &&
+    (typeof options.jobs === 'undefined' || options.jobs > 1)
+  ) {
+    debug('attempting to enable parallel mode');
+    this.parallelMode(true);
   }
 }
 

diff --git a/lib/nodejs/buffered-worker-pool.js b/lib/nodejs/buffered-worker-pool.js
@@ -11,7 +11,6 @@ const serializeJavascript = require('serialize-javascript');
 const workerpool = require('workerpool');
 const {deserialize} = require('./serializer');
 const debug = require('debug')('mocha:parallel:buffered-worker-pool');
-const {cpus} = require('os');
 const {createInvalidArgumentTypeError} = require('../errors');
 
 const WORKER_PATH = require.resolve('./worker.js');
@@ -25,20 +24,6 @@ const WORKER_PATH = require.resolve('./worker.js');
  */
 let optionsCache = new WeakMap();
 
-/**
- * Count of CPU cores
- */
-const CPU_COUNT = cpus().length;
-
-/**
- * Default max number of workers.
- *
- * We are already using one core for the main process, so assume only _n - 1_ are left.
- *
- * This is a reasonable default, but YMMV.
- */
-const DEFAULT_MAX_WORKERS = CPU_COUNT - 1;
-
 /**
  * These options are passed into the [workerpool](https://npm.im/workerpool) module.
  * @type {Partial<WorkerPoolOptions>}
@@ -49,29 +34,38 @@ const WORKER_POOL_DEFAULT_OPTS = {
   // ensure the same flags sent to `node` for this `mocha` invocation are passed
   // along to children
   forkOpts: {execArgv: process.execArgv},
-  maxWorkers: DEFAULT_MAX_WORKERS
+  maxWorkers: workerpool.cpus - 1
 };
 
 /**
  * A wrapper around a third-party worker pool implementation.
  * @private
  */
 class BufferedWorkerPool {
+  /**
+   * Creates an underlying worker pool instance; determines max worker count
+   * @param {Partial<WorkerPoolOptions>} [opts] - Options
+   */
   constructor(opts = {}) {
-    const maxWorkers = Math.max(1, opts.maxWorkers || 0);
+    const maxWorkers = Math.max(
+      1,
+      typeof opts.maxWorkers === 'undefined'
+        ? WORKER_POOL_DEFAULT_OPTS.maxWorkers
+        : opts.maxWorkers
+    );
 
-    if (maxWorkers < 2) {
-      /* istanbul ignore next */
+    /* istanbul ignore next */
+    if (workerpool.cpus < 2) {
+      // TODO: decide whether we should warn
       debug(
-        'not enough CPU cores available (%d) to run multiple jobs; avoid --parallel on this machine',
-        CPU_COUNT
+        'not enough CPU cores available to run multiple jobs; avoid --parallel on this machine'
       );
-    } else if (maxWorkers >= CPU_COUNT) {
-      /* istanbul ignore next */
+    } else if (maxWorkers >= workerpool.cpus) {
+      // TODO: decide whether we should warn
       debug(
         '%d concurrent job(s) requested, but only %d core(s) available',
         maxWorkers,
-        CPU_COUNT
+        workerpool.cpus
       );
     }
     /* istanbul ignore next */

diff --git a/lib/nodejs/worker.js b/lib/nodejs/worker.js
@@ -90,7 +90,6 @@ async function run(filepath, serializedOptions = '{}') {
     );
   }
 
-  debug('run(): deserialized options to %O', argv);
   const opts = Object.assign({ui: 'bdd'}, argv, {
     // workers only use the `Buffered` reporter.
     reporter: BUFFERED_REPORTER_PATH,

diff --git a/test/node-unit/buffered-worker-pool.spec.js b/test/node-unit/buffered-worker-pool.spec.js
@@ -30,7 +30,8 @@ describe('class BufferedWorkerPool', function() {
       require.resolve('../../lib/nodejs/buffered-worker-pool'),
       {
         workerpool: {
-          pool: sandbox.stub().returns(pool)
+          pool: sandbox.stub().returns(pool),
+          cpus: 8
         },
         '../../lib/nodejs/serializer': serializer,
         'serialize-javascript': serializeJavascript

diff --git a/test/node-unit/worker.spec.js b/test/node-unit/worker.spec.js
@@ -76,7 +76,7 @@ describe('worker', function() {
     });
 
     describe('function', function() {
-      describe('run', function() {
+      describe('run()', function() {
         describe('when called without arguments', function() {
           it('should reject', async function() {
             return expect(worker.run, 'to be rejected with error satisfying', {
@@ -85,6 +85,30 @@ describe('worker', function() {
           });
         });
 
+        describe('when passed a non-string `options` value', function() {
+          it('should reject', async function() {
+            return expect(
+              () => worker.run('foo.js', 42),
+              'to be rejected with error satisfying',
+              {
+                code: 'ERR_MOCHA_INVALID_ARG_TYPE'
+              }
+            );
+          });
+        });
+
+        describe('when passed an invalid string `options` value', function() {
+          it('should reject', async function() {
+            return expect(
+              () => worker.run('foo.js', 'tomfoolery'),
+              'to be rejected with error satisfying',
+              {
+                code: 'ERR_MOCHA_INVALID_ARG_VALUE'
+              }
+            );
+          });
+        });
+
         describe('when called with empty "filepath" argument', function() {
           it('should reject', async function() {
             return expect(