add some tests

- 100% coverage for `lib/serializer.js`
- add a trivial integration test for `--parallel`
- fix typo in integration test helper; output spawned command in a copy/pastable format
- remove some unused code
- rename `deserializeMessage` => `deserialize`
- rename `serializeObject` => `serialize`
- docstrings for `lib/serializer.js`
- rewrite `SerializableEvent.serialize` as a loop instead of recursive function due to possibility of exceeding max stack trace; other refactors
- do not freeze objects returned from various `Runnable`'s `serialize()` method, because `SerializableEvent#serialize` needs to mutate them.

Author: boneskull

lib/buffered-runner.js

@@ -6,7 +6,7 @@ const Runner = require('./runner');
 const {EVENT_RUN_BEGIN, EVENT_RUN_END} = Runner.constants;
 const debug = require('debug')('mocha:buffered-runner');
 const workerpool = require('workerpool');
-const {deserializeMessage} = require('./serializer');
+const {deserialize} = require('./serializer');
 
 /**
  * This `Runner` delegates tests runs to worker threads.  Does not execute any
@@ -48,12 +48,11 @@ class BufferedRunner extends Runner {
     this.emit(EVENT_RUN_BEGIN);
 
     const poolProxy = await pool.proxy();
-    // const tasks = new Set(
     const results = await allSettled(
       files.map(async file => {
         debug('enqueueing test file %s', file);
         try {
-          const {failures, events} = deserializeMessage(
+          const {failures, events} = deserialize(
             await poolProxy.run(file, opts)
           );
           debug(
@@ -91,6 +90,7 @@ class BufferedRunner extends Runner {
 
     await pool.terminate();
 
+    // XXX I'm not sure this is ever non-empty
     const uncaughtExceptions = results.filter(
       ({status}) => status === 'rejected'
     );

lib/hook.js

@@ -46,7 +46,7 @@ Hook.prototype.error = function(err) {
 };
 
 Hook.prototype.serialize = function serialize() {
-  return Object.freeze({
+  return {
     $$titlePath: this.titlePath(),
     ctx: {
       currentTest: {
@@ -59,5 +59,5 @@ Hook.prototype.serialize = function serialize() {
     },
     title: this.title,
     type: this.type
-  });
+  };
 };

lib/serializer.js

@@ -1,12 +1,17 @@
 'use strict';
 
+const {type} = require('./utils');
+const {createInvalidArgumentTypeError} = require('./errors');
 // const debug = require('debug')('mocha:serializer');
 
+const SERIALIZABLE_RESULT_NAME = 'SerializableWorkerResult';
+const SERIALIZABLE_TYPES = new Set(['object', 'array', 'function', 'error']);
+
 class SerializableWorkerResult {
   constructor(failures, events) {
     this.failures = failures;
     this.events = events;
-    this.__type = 'SerializableWorkerResult';
+    this.__type = SERIALIZABLE_RESULT_NAME;
   }
 
   static create(...args) {
@@ -24,90 +29,198 @@ class SerializableWorkerResult {
     obj.events.forEach(SerializableEvent.deserialize);
     return obj;
   }
+
+  /**
+   * Returns `true` if this is a {@link SerializableWorkerResult}, even if serialized
+   * (in other words, not an instance).
+   *
+   * @param {*} value - A value to check
+   */
+  static isSerializableWorkerResult(value) {
+    return (
+      type(value) === 'object' && value.__type === SERIALIZABLE_RESULT_NAME
+    );
+  }
 }
 
+/**
+ * Represents an event, emitted by a {@link Runner}, which is to be transmitted
+ * over IPC.
+ *
+ * Due to the contents of the event data, it's not possible to send them verbatim.
+ * When received by the main process--and handled by reporters--these objects are
+ * expected to contain {@link Runnable} instances.  This class provides facilities
+ * to perform the translation via serialization and deserialization.
+ */
 class SerializableEvent {
-  constructor(eventName, rawObject, error) {
+  /**
+   * Constructs a `SerializableEvent`, throwing if we receive unexpected data.
+   *
+   * Practically, events emitted from `Runner` have a minumum of zero (0) arguments--
+   * (for example, {@link Runnable.constants.EVENT_RUN_BEGIN}) and a maximum of two (2)
+   * (for example, {@link Runnable.constants.EVENT_TEST_FAIL}, where the second argument
+   * is an `Error`).  The first argument, if present, is a {@link Runnable}.
+   * This constructor's arguments adhere to this convention.
+   * @param {string} eventName - A non-empty event name.
+   * @param {any} [originalValue] - Some data. Corresponds to extra arguments passed to `EventEmitter#emit`.
+   * @param {Error} [originalError] - An error, if there's an error.
+   * @throws If `eventName` is empty, or `originalValue` is a non-object.
+   */
+  constructor(eventName, originalValue, originalError) {
+    if (!eventName) {
+      throw new Error('expected a non-empty `eventName` argument');
+    }
+    /**
+     * The event name.
+     * @memberof SerializableEvent
+     */
     this.eventName = eventName;
-    if (rawObject && typeof rawObject !== 'object') {
+    const originalValueType = type(originalValue);
+    if (originalValueType !== 'object' && originalValueType !== 'undefined') {
       throw new Error(
-        `expected object, received [${typeof rawObject}]: ${rawObject}`
+        `expected object, received [${originalValueType}]: ${originalValue}`
       );
     }
-    this.error = error;
-    // we don't want this value sent via IPC.
-    Object.defineProperty(this, 'rawObject', {
-      value: rawObject,
+    /**
+     * An error, if present.
+     * @memberof SerializableEvent
+     */
+    Object.defineProperty(this, 'originalError', {
+      value: originalError,
+      enumerable: false
+    });
+
+    /**
+     * The raw value.
+     *
+     * We don't want this value sent via IPC; making it non-enumerable will do that.
+     *
+     * @memberof SerializableEvent
+     */
+    Object.defineProperty(this, 'originalValue', {
+      value: originalValue,
       enumerable: false
     });
   }
 
+  /**
+   * In case you hated using `new` (I do).
+   *
+   * @param  {...any} args - Args for {@link SerializableEvent#constructor}.
+   * @returns {SerializableEvent} A new `SerializableEvent`
+   */
   static create(...args) {
     return new SerializableEvent(...args);
   }
 
+  /**
+   * Modifies this object *in place* (for theoretical memory consumption & performance
+   * reasons); serializes `SerializableEvent#originalValue` (placing the result in
+   * `SerializableEvent#data`) and `SerializableEvent#error`. Freezes this object.
+   * The result is an object that can be transmitted over IPC.
+   */
   serialize() {
-    const createError = err => {
-      const _serializeError = ([value, key]) => {
-        if (value) {
-          if (typeof value[key] === 'object') {
-            const obj = value[key];
-            Object.keys(obj)
-              .map(key => [obj[key], key])
-              .forEach(_serializeError);
-          } else if (typeof value[key] === 'function') {
-            delete value[key];
-          }
-        }
-      };
-      const error = {
-        message: err.message,
-        stack: err.stack,
-        __type: 'Error'
-      };
-
-      Object.keys(err)
-        .map(key => [err[key], key])
-        .forEach(_serializeError);
-      return error;
-    };
-    const obj = this.rawObject;
-    this.data = Object.create(null);
-    Object.assign(
-      this.data,
-      typeof obj.serialize === 'function' ? obj.serialize() : obj
-    );
-    Object.keys(this.data).forEach(key => {
-      if (this.data[key] instanceof Error) {
-        this.data[key] = createError(this.data[key]);
+    // list of types within values that we will attempt to serialize
+
+    // given a parent object and a key, inspect the value and decide whether
+    // to replace it, remove it, or add it to our `pairs` array to further process.
+    // this is recursion in loop form.
+    const _serialize = (parent, key) => {
+      let value = parent[key];
+      switch (type(value)) {
+        case 'error':
+          // we need to reference the stack prop b/c it's lazily-loaded.
+          // `__type` is necessary for deserialization to create an `Error` later.
+          // fall through to the 'object' branch below to further process & remove
+          // any junk that an assertion lib may throw in there.
+          // `message` is apparently not enumerable, so we must handle it specifically.
+          value = Object.assign(Object.create(null), value, {
+            stack: value.stack,
+            message: value.message,
+            __type: 'Error'
+          });
+          parent[key] = value;
+        // falls through
+        case 'object':
+          // by adding props to the `pairs` array, we will process it further
+          pairs.push(
+            ...Object.keys(value)
+              .filter(key => SERIALIZABLE_TYPES.has(type(value[key])))
+              .map(key => [value, key])
+          );
+          break;
+        case 'function':
+          // we _may_ want to dig in to functions for some assertion libraries
+          // that might put a usable property on a function.
+          // for now, just zap it.
+          delete parent[key];
+          break;
+        case 'array':
+          pairs.push(
+            ...value
+              .filter(value => SERIALIZABLE_TYPES.has(type(value)))
+              .map((value, index) => [value, index])
+          );
+          break;
       }
+    };
+
+    const result = Object.assign(Object.create(null), {
+      data:
+        type(this.originalValue) === 'object' &&
+        type(this.originalValue.serialize) === 'function'
+          ? this.originalValue.serialize()
+          : this.originalValue,
+      error: this.originalError
     });
-    if (this.error) {
-      this.error = createError(this.error);
+
+    const pairs = Object.keys(result).map(key => [result, key]);
+
+    let pair;
+    while ((pair = pairs.shift())) {
+      _serialize(...pair);
     }
+
+    this.data = result.data;
+    this.error = result.error;
+
     return Object.freeze(this);
   }
 
+  /**
+   * Deserialize value returned from a worker into something more useful.
+   * Does not return the same object.
+   * @todo - do this in a loop instead of with recursion (if necessary)
+   * @param {SerializedEvent} obj - Object returned from worker
+   * @returns {SerializedEvent} Deserialized result
+   */
   static deserialize(obj) {
     const createError = value => {
       const error = new Error(value.message);
       error.stack = value.stack;
       Object.assign(error, value);
+      delete error.__type;
       return error;
     };
     const _deserialize = ([object, key]) => {
-      const value = typeof key !== 'undefined' ? object[key] : object;
-      if (typeof key === 'string' && key.startsWith('$$')) {
+      if (key === '__proto__') {
+        delete object[key];
+        return;
+      }
+      const value = type(key) !== 'undefined' ? object[key] : object;
+      // keys beginning with `$$` are converted into functions returning the value
+      // and renamed, stripping the `$$` prefix
+      if (type(key) === 'string' && key.startsWith('$$')) {
         const newKey = key.slice(2);
         object[newKey] = () => value;
         delete object[key];
         key = newKey;
       }
-      if (Array.isArray(value)) {
+      if (type(value) === 'array') {
         value.forEach((_, idx) => {
           _deserialize([value, idx]);
         });
-      } else if (value && typeof value === 'object') {
+      } else if (type(value) === 'object') {
         if (value.__type === 'Error') {
           object[key] = createError(value);
         } else {
@@ -118,27 +231,60 @@ class SerializableEvent {
       }
     };
 
-    Object.keys(obj.data)
-      .map(key => [obj.data, key])
-      .forEach(_deserialize);
+    if (!obj) {
+      throw createInvalidArgumentTypeError('Expected value', obj);
+    }
+
+    obj = Object.assign(Object.create(null), obj);
+
+    if (obj.data) {
+      Object.keys(obj.data)
+        .map(key => [obj.data, key])
+        .forEach(_deserialize);
+    }
+
     if (obj.error) {
       obj.error = createError(obj.error);
     }
+
     return obj;
   }
 }
 
-exports.serializeObject = function serializeObject(obj) {
-  return obj instanceof SerializableWorkerResult ? obj.serialize() : obj;
+/**
+ * "Serializes" a value for transmission over IPC as a message.
+ *
+ * If value is an object and has a `serialize()` method, call that method; otherwise return the object and hope for the best.
+ *
+ * @param {*} obj - A value to serialize
+ */
+exports.serialize = function serialize(value) {
+  return type(value) === 'object' && type(value.serialize) === 'function'
+    ? value.serialize()
+    : value;
 };
 
-exports.deserializeMessage = function deserializeMessage(message) {
-  return message &&
-    typeof message === 'object' &&
-    message.__type === 'SerializableWorkerResult'
+/**
+ * "Deserializes" a "message" received over IPC.
+ *
+ * This could be expanded with other objects that need deserialization,
+ * but at present time we only care about {@link SerializableWorkerResult} objects.
+ *
+ * @param {*} message - A "message" to deserialize
+ */
+exports.deserialize = function deserialize(message) {
+  return SerializableWorkerResult.isSerializableWorkerResult(message)
     ? SerializableWorkerResult.deserialize(message)
     : message;
 };
 
 exports.SerializableEvent = SerializableEvent;
 exports.SerializableWorkerResult = SerializableWorkerResult;
+
+/**
+ * The result of calling `SerializableEvent.serialize`, as received
+ * by the deserializer.
+ * @typedef {Object} SerializedEvent
+ * @property {object?} data - Optional serialized data
+ * @property {object?} error - Optional serialized `Error`
+ */

lib/suite.js

@@ -555,12 +555,12 @@ Suite.prototype.cleanReferences = function cleanReferences() {
  * @returns {Object}
  */
 Suite.prototype.serialize = function serialize() {
-  return Object.freeze({
+  return {
     _bail: this._bail,
     $$fullTitle: this.fullTitle(),
     root: this.root,
     title: this.title
-  });
+  };
 };
 
 var constants = utils.defineConstants(