Repository: unexpectedjs/unexpected
Pull request #482 (2018-06-02 20:16:50)

Include non-enumerable properties
This branch adds support for including all properties regardless of enumerability for object comparisons. This is definitely further than a prototype but I imagine will want a thorough review both on the code itself but also 'philosophically'. Note some interesting cleanups/simplifications in a few places were possible around key handling.


Patch:
diff --git a/documentation/api/addType.md b/documentation/api/addType.md
index 6e02de1d..9f858232 100644
--- a/documentation/api/addType.md
+++ b/documentation/api/addType.md
@@ -299,13 +299,13 @@ block. The outputs below shows the contrast between setting the
 inlineDiff = true;
 expect(
   {
-    'John Doe': new Person('John Doe', 42),
-    'Jane Doe': new Person('Janie Doe', 24)
+    JohnDoe: new Person('John Doe', 42),
+    JaneDoe: new Person('Janie Doe', 24)
   },
   'to equal',
   {
-    'John Doe': new Person('John Doe', 42),
-    'Jane Doe': new Person('Jane Doe', 24)
+    JohnDoe: new Person('John Doe', 42),
+    JaneDoe: new Person('Jane Doe', 24)
   }
 );
 ```
@@ -313,18 +313,18 @@ expect(
 ```output
 expected
 {
-  'John Doe': new Person('John Doe', 42),
-  'Jane Doe': new Person('Janie Doe', 24)
+  JohnDoe: new Person('John Doe', 42),
+  JaneDoe: new Person('Janie Doe', 24)
 }
 to equal
 {
-  'John Doe': new Person('John Doe', 42),
-  'Jane Doe': new Person('Jane Doe', 24)
+  JohnDoe: new Person('John Doe', 42),
+  JaneDoe: new Person('Jane Doe', 24)
 }
 
 {
-  'John Doe': new Person('John Doe', 42),
-  'Jane Doe': new Person(
+  JohnDoe: new Person('John Doe', 42),
+  JaneDoe: new Person(
     'Janie Doe', // should be 'Jane Doe'
                  // -Janie Doe
                  // +Jane Doe
@@ -337,13 +337,13 @@ to equal
 inlineDiff = false;
 expect(
   {
-    'John Doe': new Person('John Doe', 42),
-    'Jane Doe': new Person('Janie Doe', 24)
+    JohnDoe: new Person('John Doe', 42),
+    JaneDoe: new Person('Janie Doe', 24)
   },
   'to equal',
   {
-    'John Doe': new Person('John Doe', 42),
-    'Jane Doe': new Person('Jane Doe', 24)
+    JohnDoe: new Person('John Doe', 42),
+    JaneDoe: new Person('Jane Doe', 24)
   }
 );
 ```
@@ -351,25 +351,25 @@ expect(
 ```output
 expected
 {
-  'John Doe': new Person('John Doe', 42),
-  'Jane Doe': new Person('Janie Doe', 24)
+  JohnDoe: new Person('John Doe', 42),
+  JaneDoe: new Person('Janie Doe', 24)
 }
 to equal
 {
-  'John Doe': new Person('John Doe', 42),
-  'Jane Doe': new Person('Jane Doe', 24)
+  JohnDoe: new Person('John Doe', 42),
+  JaneDoe: new Person('Jane Doe', 24)
 }
 
 {
-  'John Doe': new Person('John Doe', 42),
-  'Jane Doe': new Person('Janie Doe', 24) // should equal new Person('Jane Doe', 24)
-                                          //
-                                          // new Person(
-                                          //   'Janie Doe', // should be 'Jane Doe'
-                                          //                // -Janie Doe
-                                          //                // +Jane Doe
-                                          //   24
-                                          // )
+  JohnDoe: new Person('John Doe', 42),
+  JaneDoe: new Person('Janie Doe', 24) // should equal new Person('Jane Doe', 24)
+                                       //
+                                       // new Person(
+                                       //   'Janie Doe', // should be 'Jane Doe'
+                                       //                // -Janie Doe
+                                       //                // +Jane Doe
+                                       //   24
+                                       // )
 }
 ```
 
diff --git a/lib/assertions.js b/lib/assertions.js
index 32cb2c1c..cfaa6c10 100644
--- a/lib/assertions.js
+++ b/lib/assertions.js
@@ -1796,27 +1796,35 @@ module.exports = expect => {
       if (valueType.is('array-like') && !subjectIsArrayLike) {
         expect.fail();
       }
-      const promiseByKey = {};
-      const keys = valueType.getKeys(value);
+
       const subjectKeys = subjectType.getKeys(subject);
+      const valueKeys = valueType.getKeys(value);
+      // calculate the unique keys early given enumerability no
+      // longer affects what is included in the list of keys
+      const uniqueKeys = subjectType.uniqueKeys(subjectKeys, valueKeys);
 
-      if (!subjectIsArrayLike) {
-        // Find all non-enumerable subject keys present in value, but not returned by subjectType.getKeys:
-        keys.forEach(key => {
-          if (
-            Object.prototype.hasOwnProperty.call(subject, key) &&
-            subjectKeys.indexOf(key) === -1
-          ) {
-            subjectKeys.push(key);
+      const promiseByKey = {};
+      let forceExhaustivelyComparison = false;
+      uniqueKeys.forEach((key, index) => {
+        const subjectKey = subjectType.valueForKey(subject, key);
+        const valueKey = valueType.valueForKey(value, key);
+        const valueKeyType = expect.findTypeOf(valueKey);
+        const isDefinedSubjectKey = typeof subjectKey !== 'undefined';
+        const isDefinedValueKey = typeof valueKey !== 'undefined';
+
+        if (expect.flags.exhaustively) {
+          if (valueKeyType.is('expect.it') && !isDefinedSubjectKey) {
+            // ensure value only expect.it key is marked missing
+            forceExhaustivelyComparison = true;
           }
-        });
-      }
+        } else {
+          if (isDefinedSubjectKey && !isDefinedValueKey) {
+            // ignore subject only keys unless we are being exhaustive
+            return;
+          }
+        }
 
-      keys.forEach((key, index) => {
         promiseByKey[key] = expect.promise(() => {
-          const subjectKey = subjectType.valueForKey(subject, key);
-          const valueKey = valueType.valueForKey(value, key);
-          const valueKeyType = expect.findTypeOf(valueKey);
           if (valueKeyType.is('expect.it')) {
             expect.context.thisObject = subject;
             return valueKey(subjectKey, expect.context);
@@ -1831,25 +1839,8 @@ module.exports = expect => {
       return expect.promise
         .all([
           expect.promise(() => {
-            if (expect.flags.exhaustively) {
-              const nonOwnKeysWithDefinedValues = keys.filter(
-                key =>
-                  !Object.prototype.hasOwnProperty.call(subject, key) &&
-                  typeof subjectType.valueForKey(subject, key) !== 'undefined'
-              );
-              const valueKeysWithDefinedValues = keys.filter(
-                key => typeof valueType.valueForKey(value, key) !== 'undefined'
-              );
-              const subjectKeysWithDefinedValues = subjectKeys.filter(
-                key =>
-                  typeof subjectType.valueForKey(subject, key) !== 'undefined'
-              );
-              expect(
-                valueKeysWithDefinedValues.length -
-                  nonOwnKeysWithDefinedValues.length,
-                'to equal',
-                subjectKeysWithDefinedValues.length
-              );
+            if (forceExhaustivelyComparison) {
+              throw new Error('exhaustive comparison failure');
             }
           }),
           expect.promise.all(promiseByKey)
@@ -1860,16 +1851,13 @@ module.exports = expect => {
               diff(output, diff, inspect, equal) {
                 output.inline = true;
                 const subjectIsArrayLike = subjectType.is('array-like');
-                const valueKeys = valueType.getKeys(value);
-                const keys = subjectType
-                  .uniqueKeys(subjectKeys, valueKeys)
-                  .filter(key => {
-                    // Skip missing keys expected to be missing so they don't get rendered in the diff
-                    return (
-                      subjectType.hasKey(subject, key) ||
-                      typeof valueType.valueForKey(value, key) !== 'undefined'
-                    );
-                  });
+                // Skip missing keys expected to be missing so they don't get rendered in the diff
+                const keys = uniqueKeys.filter(key => {
+                  return (
+                    subjectType.hasKey(subject, key) ||
+                    typeof valueType.valueForKey(value, key) !== 'undefined'
+                  );
+                });
                 const prefixOutput = subjectType.prefix(
                   output.clone(),
                   subject
@@ -2354,14 +2342,19 @@ module.exports = expect => {
             }
           });
         },
-        err =>
+        err => {
+          if (err.isOperational && !err.propertyIsEnumerable('isOperational')) {
+            delete err.isOperational;
+          }
+
           expect.withError(
             () => expect.shift(err),
             e => {
               e.originalError = err;
               throw e;
             }
-          )
+          );
+        }
       );
     }
   );
diff --git a/lib/types.js b/lib/types.js
index 132942b0..e7002d6d 100644
--- a/lib/types.js
+++ b/lib/types.js
@@ -114,7 +114,7 @@ module.exports = function(expect) {
     },
     getKeys: Object.getOwnPropertySymbols
       ? obj => {
-          const keys = Object.keys(obj);
+          const keys = Object.getOwnPropertyNames(obj);
           const symbols = Object.getOwnPropertySymbols(obj);
           if (symbols.length > 0) {
             return keys.concat(symbols);
@@ -122,7 +122,7 @@ module.exports = function(expect) {
             return keys;
           }
         }
-      : Object.keys,
+      : Object.getOwnPropertyNames,
     // If Symbol support is not detected default to undefined which, when
     // passed to Array.prototype.sort, means "natural" (asciibetical) sort.
     keyComparator:
@@ -154,44 +154,7 @@ module.exports = function(expect) {
           }
         : undefined,
     equal(a, b, equal) {
-      if (a === b) {
-        return true;
-      }
-
-      if (b.constructor !== a.constructor) {
-        return false;
-      }
-
-      const actualKeys = this.getKeys(a).filter(
-        key => typeof this.valueForKey(a, key) !== 'undefined'
-      );
-      const expectedKeys = this.getKeys(b).filter(
-        key => typeof this.valueForKey(b, key) !== 'undefined'
-      );
-
-      // having the same number of owned properties (keys incorporates hasOwnProperty)
-      if (actualKeys.length !== expectedKeys.length) {
-        return false;
-      }
-      //the same set of keys (although not necessarily the same order),
-      actualKeys.sort(this.keyComparator);
-      expectedKeys.sort(this.keyComparator);
-      // cheap key test
-      for (let i = 0; i < actualKeys.length; i += 1) {
-        if (actualKeys[i] !== expectedKeys[i]) {
-          return false;
-        }
-      }
-
-      //equivalent values for every corresponding key, and
-      // possibly expensive deep test
-      for (let j = 0; j < actualKeys.length; j += 1) {
-        const key = actualKeys[j];
-        if (!equal(this.valueForKey(a, key), this.valueForKey(b, key))) {
-          return false;
-        }
-      }
-      return true;
+      return utils.checkObjectEqualityUsingType(a, b, this, equal);
     },
     hasKey(obj, key) {
       return key in obj;
@@ -799,12 +762,20 @@ module.exports = function(expect) {
     'sourceId',
     'sourceURL',
     'stack',
-    'stackArray'
+    'stackArray',
+    '__stackCleaned__',
+    'isOperational' // added by the promise implementation
   ].reduce((result, prop) => {
     result[prop] = true;
     return result;
   }, {});
 
+  if (new Error().hasOwnProperty('arguments')) {
+    // node.js 0.10 adds two extra non-enumerable properties to Error instances:
+    errorMethodBlacklist.arguments = true;
+    errorMethodBlacklist.type = true;
+  }
+
   expect.addType({
     base: 'object',
     name: 'Error',
@@ -826,7 +797,9 @@ module.exports = function(expect) {
     },
     equal(a, b, equal) {
       return (
-        a === b || (equal(a.message, b.message) && this.baseType.equal(a, b))
+        a === b ||
+        (equal(a.message, b.message) &&
+          utils.checkObjectEqualityUsingType(a, b, this, equal))
       );
     },
     inspect(value, depth, output, inspect) {
diff --git a/lib/utils.js b/lib/utils.js
index 6b8217f8..20c23bb0 100644
--- a/lib/utils.js
+++ b/lib/utils.js
@@ -27,6 +27,48 @@ const utils = (module.exports = {
       return a === b;
     }),
 
+  checkObjectEqualityUsingType(a, b, type, isEqual) {
+    if (a === b) {
+      return true;
+    }
+
+    if (b.constructor !== a.constructor) {
+      return false;
+    }
+
+    const actualKeys = type
+      .getKeys(a)
+      .filter(key => typeof type.valueForKey(a, key) !== 'undefined');
+    const expectedKeys = type
+      .getKeys(b)
+      .filter(key => typeof type.valueForKey(b, key) !== 'undefined');
+
+    // having the same number of owned properties (keys incorporates hasOwnProperty)
+    if (actualKeys.length !== expectedKeys.length) {
+      return false;
+    }
+
+    //the same set of keys (although not necessarily the same order),
+    actualKeys.sort(type.keyComparator);
+    expectedKeys.sort(type.keyComparator);
+    // cheap key test
+    for (let i = 0; i < actualKeys.length; i += 1) {
+      if (actualKeys[i] !== expectedKeys[i]) {
+        return false;
+      }
+    }
+
+    //equivalent values for every corresponding key, and
+    // possibly expensive deep test
+    for (let j = 0; j < actualKeys.length; j += 1) {
+      const key = actualKeys[j];
+      if (!isEqual(type.valueForKey(a, key), type.valueForKey(b, key))) {
+        return false;
+      }
+    }
+    return true;
+  },
+
   duplicateArrayLikeUsingType(obj, type) {
     const keys = type.getKeys(obj);
 


Test patch:
diff --git a/test/api/inspect.spec.js b/test/api/inspect.spec.js
index bd6877ac..92a941b5 100644
--- a/test/api/inspect.spec.js
+++ b/test/api/inspect.spec.js
@@ -207,9 +207,9 @@ describe('inspect', function() {
             deeply: {
               nested: { object: 'This should not be shown' },
               string: 'should be shown',
-              'a list': [1, 2, 3]
+              list: [1, 2, 3]
             },
-            'a list': [1, 2, 3]
+            list: [1, 2, 3]
           }
         }
       }
@@ -269,8 +269,8 @@ describe('inspect', function() {
         '    circular: { self: [Circular] },\n' +
         '    this: {\n' +
         '      is: {\n' +
-        "        deeply: { nested: ..., string: 'should be shown', 'a list': ... },\n" +
-        "        'a list': [ 1, 2, 3 ]\n" +
+        "        deeply: { nested: ..., string: 'should be shown', list: ... },\n" +
+        '        list: [ 1, 2, 3 ]\n' +
         '      }\n' +
         '    }\n' +
         '  }\n' +
@@ -321,7 +321,7 @@ describe('inspect', function() {
         "      { id: 4, name: 'Barbara Lynn' },\n" +
         "      { id: 5, name: 'Sharpe Downs' }\n" +
         '    ],\n' +
-        "    circular: { self: [Circular] }, this: { is: { deeply: { nested: ..., string: 'should be shown', 'a list': ... }, 'a list': [ 1, 2, 3 ] } }\n" +
+        "    circular: { self: [Circular] }, this: { is: { deeply: { nested: ..., string: 'should be shown', list: ... }, list: [ 1, 2, 3 ] } }\n" +
         '  }\n' +
         ']'
     );
diff --git a/test/assertions/to-have-property.spec.js b/test/assertions/to-have-property.spec.js
index 6c255fc4..5460683a 100644
--- a/test/assertions/to-have-property.spec.js
+++ b/test/assertions/to-have-property.spec.js
@@ -42,21 +42,24 @@ describe('to have property assertion', function() {
           expect(subject, 'to have enumerable property', 'enumFalse');
         },
         'to throw exception',
-        "expected { a: 'b' } to have enumerable property 'enumFalse'"
+        "expected { a: 'b', enumFalse: 't', configFalse: 't', writableFalse: 't' }\n" +
+          "to have enumerable property 'enumFalse'"
       );
       expect(
         function() {
           expect(subject, 'to have configurable property', 'configFalse');
         },
         'to throw exception',
-        "expected { a: 'b' } to have configurable property 'configFalse'"
+        "expected { a: 'b', enumFalse: 't', configFalse: 't', writableFalse: 't' }\n" +
+          "to have configurable property 'configFalse'"
       );
       expect(
         function() {
           expect(subject, 'to have writable property', 'writableFalse');
         },
         'to throw exception',
-        "expected { a: 'b' } to have writable property 'writableFalse'"
+        "expected { a: 'b', enumFalse: 't', configFalse: 't', writableFalse: 't' }\n" +
+          "to have writable property 'writableFalse'"
       );
     });
   });
diff --git a/test/assertions/to-satisfy.spec.js b/test/assertions/to-satisfy.spec.js
index 458b29d1..a0a7e0bd 100644
--- a/test/assertions/to-satisfy.spec.js
+++ b/test/assertions/to-satisfy.spec.js
@@ -2194,7 +2194,8 @@ describe('to satisfy assertion', function() {
               });
             },
             'to throw',
-            "expected {} to exhaustively satisfy { nonEnumerable: 'wrong' }\n" +
+            "expected { nonEnumerable: 'theValue' }\n" +
+              "to exhaustively satisfy { nonEnumerable: 'wrong' }\n" +
               '\n' +
               '{\n' +
               '  nonEnumerable:\n' +
@@ -2208,10 +2209,6 @@ describe('to satisfy assertion', function() {
       });
 
       describe('when not matching the non-enumerable property', function() {
-        it('should succeed', function() {
-          expect(bar, 'to exhaustively satisfy', {});
-        });
-
         it('should fail with a diff', function() {
           expect(
             function() {
@@ -2220,9 +2217,11 @@ describe('to satisfy assertion', function() {
               });
             },
             'to throw',
-            "expected {} to exhaustively satisfy { somethingElse: 'wrong' }\n" +
+            "expected { nonEnumerable: 'theValue' }\n" +
+              "to exhaustively satisfy { somethingElse: 'wrong' }\n" +
               '\n' +
               '{\n' +
+              "  nonEnumerable: 'theValue' // should be removed\n" +
               "  // missing somethingElse: 'wrong'\n" +
               '}'
           );
diff --git a/test/types/error-type.spec.js b/test/types/error-type.spec.js
index 10650341..230f3ab7 100644
--- a/test/types/error-type.spec.js
+++ b/test/types/error-type.spec.js
@@ -61,8 +61,14 @@ describe('Error type', function() {
     }
 
     function MyError(message) {
-      Error.call(this);
-      this.message = message;
+      var instance = new Error(message);
+      var proto = Object.getPrototypeOf(this);
+      if (Object.setPrototypeOf) {
+        Object.setPrototypeOf(instance, proto);
+      } else {
+        instance.__proto__ = proto; // eslint-disable-line no-proto
+      }
+      return instance;
     }
 
     inherits(MyError, Error);
@@ -140,4 +146,16 @@ describe('Error type', function() {
       });
     });
   });
+
+  describe('when comparing Error objects with differing enumerable keys', () => {
+    it('should not break', () => {
+      var e1 = new Error('foo');
+      var e2 = new Error();
+      e2.message = 'foo';
+
+      expect(() => {
+        expect(e1, 'to equal', e2);
+      }, 'not to throw');
+    });
+  });
 });