Merge pull request #109 from astorije/astorije/deep-keys

Re-sync `.keys` with Chai's native assertion, and add tests/support for `.deep.keys`
astorije · Nov 27, 2017 · 4719e0f · 4719e0f
2 parents e92c116 + db3bf84
commit 4719e0f
Show file tree

Hide file tree

Showing 4 changed files with 264 additions and 82 deletions.
diff --git a/.eslintrc.yml b/.eslintrc.yml
@@ -150,7 +150,7 @@ rules:
     - error
     - tabWidth: 2
       ignoreUrls: true
-      ignorePattern: \* expect\(.*\)\.to\.have\.(keys|nested\.property)
+      ignorePattern: \* expect\(.*\)\.to\.have\.[a-z.]*(keys|nested\.property)
   max-nested-callbacks:
     - error
     - max: 6
@@ -228,7 +228,6 @@ rules:
     - as-needed
   arrow-spacing: error
   generator-star-spacing: error
-  no-confusing-arrow: error
   no-duplicate-imports: error
   no-restricted-imports: error
   no-useless-computed-key: error

diff --git a/README.md b/README.md
@@ -129,42 +129,100 @@ expect(new List([1, 2, 3])).to.include(2);
 expect(new Map({ foo: 'bar', hello: 'world' })).to.include.keys('foo');
 ```
 
-### .keys(key1[, key2, ...[, keyN]])
+### .keys(key1[, key2[, ...]])
 
 - **@param** *{ String... | Array | Object | Collection }* key*N*
 
-Asserts that the keyed collection contains any or all of the passed-in
-keys. Use in combination with `any`, `all`, `contains`, or `have` will
-affect what will pass.
+Asserts that the target collection has the given keys.
 
-When used in conjunction with `any`, at least one key that is passed in
-must exist in the target object. This is regardless whether or not
-the `have` or `contain` qualifiers are used. Note, either `any` or `all`
-should be used in the assertion. If neither are used, the assertion is
-defaulted to `all`.
+When the target is an object or array, keys can be provided as one or more
+string arguments, a single array argument, a single object argument, or an
+immutable collection. In the last 2 cases, only the keys in the given
+object/collection matter; the values are ignored.
 
-When both `all` and `contain` are used, the target object must have at
-least all of the passed-in keys but may have more keys not listed.
+```js
+expect(new Map({ foo: 1, bar: 2 })).to.have.all.keys('foo', 'bar');
+expect(new Map({ foo: 1, bar: 2 })).to.have.all.keys(new List(['bar', 'foo']));
+expect(new Map({ foo: 1, bar: 2 })).to.have.all.keys(new Set(['bar', 'foo']));
+expect(new Map({ foo: 1, bar: 2 })).to.have.all.keys(new Stack(['bar', 'foo']));
+expect(new List(['x', 'y'])).to.have.all.keys(0, 1);
+
+expect(new Map({ foo: 1, bar: 2 })).to.have.all.keys(['foo', 'bar']);
+expect(new List(['x', 'y'])).to.have.all.keys([0, 1]);
+
+// Values in the passed object are ignored:
+expect(new Map({ foo: 1, bar: 2 })).to.have.all.keys({ bar: 6, foo: 7 });
+expect(new Map({ foo: 1, bar: 2 })).to.have.all.keys(new Map({ bar: 6, foo: 7 }));
+expect(new List(['x', 'y'])).to.have.all.keys({ 0: 4, 1: 5 });
+```
+
+Note that `deep.property` behaves exactly like `property` in the context of
+immutable data structures.
 
-When both `all` and `have` are used, the target object must both contain
-all of the passed-in keys AND the number of keys in the target object must
-match the number of keys passed in (in other words, a target object must
-have all and only all of the passed-in keys).
+By default, the target must have all of the given keys and no more. Add
+`.any` earlier in the chain to only require that the target have at least
+one of the given keys. Also, add `.not` earlier in the chain to negate
+`.keys`. It's often best to add `.any` when negating `.keys`, and to use
+`.all` when asserting `.keys` without negation.
 
-`key` is an alias to `keys`.
+When negating `.keys`, `.any` is preferred because `.not.any.keys` asserts
+exactly what's expected of the output, whereas `.not.all.keys` creates
+uncertain expectations.
+
+```js
+// Recommended; asserts that target doesn't have any of the given keys
+expect(new Map({a: 1, b: 2})).to.not.have.any.keys('c', 'd');
+
+// Not recommended; asserts that target doesn't have all of the given
+// keys but may or may not have some of them
+expect(new Map({a: 1, b: 2})).to.not.have.all.keys('c', 'd');
+```
+
+When asserting `.keys` without negation, `.all` is preferred because
+`.all.keys` asserts exactly what's expected of the output, whereas
+`.any.keys` creates uncertain expectations.
+
+```js
+// Recommended; asserts that target has all the given keys
+expect(new Map({a: 1, b: 2})).to.have.all.keys('a', 'b');
+
+// Not recommended; asserts that target has at least one of the given
+// keys but may or may not have more of them
+expect(new Map({a: 1, b: 2})).to.have.any.keys('a', 'b');
+```
+
+Note that `.all` is used by default when neither `.all` nor `.any` appear
+earlier in the chain. However, it's often best to add `.all` anyway because
+it improves readability.
+
+```js
+// Both assertions are identical
+expect(new Map({a: 1, b: 2})).to.have.all.keys('a', 'b'); // Recommended
+expect(new Map({a: 1, b: 2})).to.have.keys('a', 'b'); // Not recommended
+```
+
+Add `.include` earlier in the chain to require that the target's keys be a
+superset of the expected keys, rather than identical sets.
+
+```js
+// Target object's keys are a superset of ['a', 'b'] but not identical
+expect(new Map({a: 1, b: 2, c: 3})).to.include.all.keys('a', 'b');
+expect(new Map({a: 1, b: 2, c: 3})).to.not.have.all.keys('a', 'b');
+```
+
+However, if `.any` and `.include` are combined, only the `.any` takes
+effect. The `.include` is ignored in this case.
+
+```js
+// Both assertions are identical
+expect(new Map({a: 1})).to.have.any.keys('a', 'b');
+expect(new Map({a: 1})).to.include.any.keys('a', 'b');
+```
+
+The alias `.key` can be used interchangeably with `.keys`.
 
 ```js
 expect(new Map({ foo: 1 })).to.have.key('foo');
-expect(new Map({ foo: 1, bar: 2 })).to.have.keys('foo', 'bar');
-expect(new Map({ foo: 1, bar: 2 })).to.have.keys(new List(['bar', 'foo']));
-expect(new Map({ foo: 1, bar: 2 })).to.have.keys(new Set(['bar', 'foo']));
-expect(new Map({ foo: 1, bar: 2 })).to.have.keys(new Stack(['bar', 'foo']));
-expect(new Map({ foo: 1, bar: 2 })).to.have.keys(['bar', 'foo']);
-expect(new Map({ foo: 1, bar: 2 })).to.have.keys({ 'bar': 6, 'foo': 7 });
-expect(new Map({ foo: 1, bar: 2 })).to.have.keys(new Map({ 'bar': 6, 'foo': 7 }));
-expect(new Map({ foo: 1, bar: 2 })).to.have.any.keys('foo', 'not-foo');
-expect(new Map({ foo: 1, bar: 2 })).to.have.all.keys('foo', 'bar');
-expect(new Map({ foo: 1, bar: 2 })).to.contain.key('foo');
 ```
 
 ### .property(path[, val])

diff --git a/chai-immutable.js b/chai-immutable.js
@@ -169,45 +169,104 @@
   });
 
   /**
-   * ### .keys(key1[, key2, ...[, keyN]])
+   * ### .keys(key1[, key2[, ...]])
    *
-   * Asserts that the keyed collection contains any or all of the passed-in
-   * keys. Use in combination with `any`, `all`, `contains`, or `have` will
-   * affect what will pass.
+   * Asserts that the target collection has the given keys.
    *
-   * When used in conjunction with `any`, at least one key that is passed in
-   * must exist in the target object. This is regardless whether or not
-   * the `have` or `contain` qualifiers are used. Note, either `any` or `all`
-   * should be used in the assertion. If neither are used, the assertion is
-   * defaulted to `all`.
+   * When the target is an object or array, keys can be provided as one or more
+   * string arguments, a single array argument, a single object argument, or an
+   * immutable collection. In the last 2 cases, only the keys in the given
+   * object/collection matter; the values are ignored.
    *
-   * When both `all` and `contain` are used, the target object must have at
-   * least all of the passed-in keys but may have more keys not listed.
+   * ```js
+   * expect(new Map({ foo: 1, bar: 2 })).to.have.all.keys('foo', 'bar');
+   * expect(new Map({ foo: 1, bar: 2 })).to.have.all.keys(new List(['bar', 'foo']));
+   * expect(new Map({ foo: 1, bar: 2 })).to.have.all.keys(new Set(['bar', 'foo']));
+   * expect(new Map({ foo: 1, bar: 2 })).to.have.all.keys(new Stack(['bar', 'foo']));
+   * expect(new List(['x', 'y'])).to.have.all.keys(0, 1);
+   *
+   * expect(new Map({ foo: 1, bar: 2 })).to.have.all.keys(['foo', 'bar']);
+   * expect(new List(['x', 'y'])).to.have.all.keys([0, 1]);
+   *
+   * // Values in the passed object are ignored:
+   * expect(new Map({ foo: 1, bar: 2 })).to.have.all.keys({ 'bar': 6, 'foo': 7 });
+   * expect(new Map({ foo: 1, bar: 2 })).to.have.all.keys(new Map({ 'bar': 6, 'foo': 7 }));
+   * expect(new List(['x', 'y'])).to.have.all.keys({0: 4, 1: 5});
+   * ```
+   *
+   * Note that `deep.property` behaves exactly like `property` in the context of
+   * immutable data structures.
    *
-   * When both `all` and `have` are used, the target object must both contain
-   * all of the passed-in keys AND the number of keys in the target object must
-   * match the number of keys passed in (in other words, a target object must
-   * have all and only all of the passed-in keys).
+   * By default, the target must have all of the given keys and no more. Add
+   * `.any` earlier in the chain to only require that the target have at least
+   * one of the given keys. Also, add `.not` earlier in the chain to negate
+   * `.keys`. It's often best to add `.any` when negating `.keys`, and to use
+   * `.all` when asserting `.keys` without negation.
    *
-   * `key` is an alias to `keys`.
+   * When negating `.keys`, `.any` is preferred because `.not.any.keys` asserts
+   * exactly what's expected of the output, whereas `.not.all.keys` creates
+   * uncertain expectations.
+   *
+   * ```js
+   * // Recommended; asserts that target doesn't have any of the given keys
+   * expect(new Map({a: 1, b: 2})).to.not.have.any.keys('c', 'd');
+   *
+   * // Not recommended; asserts that target doesn't have all of the given
+   * // keys but may or may not have some of them
+   * expect(new Map({a: 1, b: 2})).to.not.have.all.keys('c', 'd');
+   * ```
+   *
+   * When asserting `.keys` without negation, `.all` is preferred because
+   * `.all.keys` asserts exactly what's expected of the output, whereas
+   * `.any.keys` creates uncertain expectations.
+   *
+   * ```js
+   * // Recommended; asserts that target has all the given keys
+   * expect(new Map({a: 1, b: 2})).to.have.all.keys('a', 'b');
+   *
+   * // Not recommended; asserts that target has at least one of the given
+   * // keys but may or may not have more of them
+   * expect(new Map({a: 1, b: 2})).to.have.any.keys('a', 'b');
+   * ```
+   *
+   * Note that `.all` is used by default when neither `.all` nor `.any` appear
+   * earlier in the chain. However, it's often best to add `.all` anyway because
+   * it improves readability.
+   *
+   * ```js
+   * // Both assertions are identical
+   * expect(new Map({a: 1, b: 2})).to.have.all.keys('a', 'b'); // Recommended
+   * expect(new Map({a: 1, b: 2})).to.have.keys('a', 'b'); // Not recommended
+   * ```
+   *
+   * Add `.include` earlier in the chain to require that the target's keys be a
+   * superset of the expected keys, rather than identical sets.
+   *
+   * ```js
+   * // Target object's keys are a superset of ['a', 'b'] but not identical
+   * expect(new Map({a: 1, b: 2, c: 3})).to.include.all.keys('a', 'b');
+   * expect(new Map({a: 1, b: 2, c: 3})).to.not.have.all.keys('a', 'b');
+   * ```
+   *
+   * However, if `.any` and `.include` are combined, only the `.any` takes
+   * effect. The `.include` is ignored in this case.
+   *
+   * ```js
+   * // Both assertions are identical
+   * expect(new Map({a: 1})).to.have.any.keys('a', 'b');
+   * expect(new Map({a: 1})).to.include.any.keys('a', 'b');
+   * ```
+   *
+   * The alias `.key` can be used interchangeably with `.keys`.
    *
    * ```js
    * expect(new Map({ foo: 1 })).to.have.key('foo');
-   * expect(new Map({ foo: 1, bar: 2 })).to.have.keys('foo', 'bar');
-   * expect(new Map({ foo: 1, bar: 2 })).to.have.keys(new List(['bar', 'foo']));
-   * expect(new Map({ foo: 1, bar: 2 })).to.have.keys(new Set(['bar', 'foo']));
-   * expect(new Map({ foo: 1, bar: 2 })).to.have.keys(new Stack(['bar', 'foo']));
-   * expect(new Map({ foo: 1, bar: 2 })).to.have.keys(['bar', 'foo']);
-   * expect(new Map({ foo: 1, bar: 2 })).to.have.keys({ 'bar': 6, 'foo': 7 });
-   * expect(new Map({ foo: 1, bar: 2 })).to.have.keys(new Map({ 'bar': 6, 'foo': 7 }));
-   * expect(new Map({ foo: 1, bar: 2 })).to.have.any.keys('foo', 'not-foo');
-   * expect(new Map({ foo: 1, bar: 2 })).to.have.all.keys('foo', 'bar');
-   * expect(new Map({ foo: 1, bar: 2 })).to.contain.key('foo');
    * ```
    *
    * @name keys
-   * @param {String...|Array|Object|Collection} keyN
    * @alias key
+   * @alias deep.key
+   * @param {...String|Array|Object|Collection} keys
    * @namespace BDD
    * @api public
    */
@@ -216,7 +275,9 @@
     return function (keys) {
       const obj = this._obj;
 
-      if (Immutable.Iterable.isKeyed(obj)) {
+      if (Immutable.Iterable.isIterable(obj)) {
+        const ssfi = utils.flag(this, 'ssfi');
+
         switch (utils.type(keys)) {
           case 'Object':
             if (Immutable.Iterable.isIndexed(keys)) {
@@ -229,10 +290,12 @@
             // `keys` is now an array so this statement safely falls through
           case 'Array':
             if (arguments.length > 1) {
-              throw new Error(
-                'keys must be given single argument of ' +
-                'Array|Object|String|Collection, ' +
-                'or multiple String arguments'
+              throw new chai.AssertionError(
+                'when testing keys against an immutable collection, you must ' +
+                'give a single Array|Object|String|Collection argument or ' +
+                'multiple String arguments',
+                null,
+                ssfi
               );
             }
             break;
@@ -241,19 +304,28 @@
             break;
         }
 
+        // Only stringify non-Symbols because Symbols would become "Symbol()"
+        keys = keys.map(val => typeof val === 'symbol' ? val : String(val));
+
         if (!keys.length) {
-          throw new Error('keys required');
+          throw new chai.AssertionError('keys required', null, ssfi);
         }
 
+        let all = utils.flag(this, 'all');
         const any = utils.flag(this, 'any');
         const contains = utils.flag(this, 'contains');
         let ok;
-        let str = `${contains ? 'contain' : 'have'} `;
+        let str = contains ? 'contain ' : 'have ';
+
+        if (!any && !all) {
+          all = true;
+        }
 
         if (any) {
           ok = keys.some(key => obj.has(key));
         } else {
           ok = keys.every(key => obj.has(key));
+
           if (!contains) {
             ok = ok && keys.length === obj.count();
           }
@@ -272,8 +344,9 @@
           ok,
           `expected #{act} to ${str}`,
           `expected #{act} to not ${str}`,
-          keys,
-          obj.toString()
+          keys.slice(0).sort(utils.compareByInspect),
+          obj.toString(),
+          true
         );
       } else {
         _super.apply(this, arguments);