Repository: chaijs/chai
Pull request #812 (2016-09-30 20:26:32)

Make 'empty' throw on non-string primitives and functions
Follow-up of #763.

meeber: @shvaikalesh Thank you for your continued work on this! Implementation looks fine to me. However, I think we should update the jsdoc to clarify that only non-function objects are allowed.

Other thoughts:

- Agreed on ESLint. I'll start a new discussion on this after v4 is released.
- You mentioned previously that you'll be following up this PR with another one that adds support for `Map` and `Set`. At that time, I'm wondering if we should begin throwing a `TypeError` if a `WeakMap` or `WeakSet` is passed. It seems to me that it'd likely be a programmer error to attempt to check if one of those two were empty without realizing that you can't do that.
- It looks like we'll need a 3rd PR to add `empty` to the `assert` interface. Lemme know if you're interested in tackling that as well. Otherwise, I'll open a new issue for it.


shvaikalesh: @meeber I have updated jsdoc. Throwing on weak collections is nice idea, absolutely agree. Totally interested to add `empty` to `assert` interface.

One more thought: it seems like tests for most assertions are repeated for every interface they are available for. Would be great to have only one test suite for each assertion.
meeber: @shvaikalesh Yeah adding tests is painful right now due to the multiple interfaces. There was a discussion about this somewhere but I can't find it. Anyway, the long term idea as we move into a modular system as described in #457 is to only have to write tests once for each assertion. But we're not quite there yet.

Regarding this PR: LGTM!
vieiralucas: I'll just leave my LGTM in case you guys think that my comment is just nitpicking.

Agreed with you and @meeber about `Map`, `Set`, `WekMap` and `WeakSet`.
Agreed about adding `empty` to the `assert` interface
Agreed about the problem with code style and missing eslint thing. 😸 

P.S. This just made me realize that we might be throwing `TypeError` instead of plain `Error` on [closeTo](https://github.com/chaijs/chai/blob/5dd96a0a3d3bd711ddc688bcd395588aa17a8819/lib/chai/core/assertions.js#L1756), [above](https://github.com/chaijs/chai/blob/5dd96a0a3d3bd711ddc688bcd395588aa17a8819/lib/chai/core/assertions.js#L680), [least](https://github.com/chaijs/chai/blob/5dd96a0a3d3bd711ddc688bcd395588aa17a8819/lib/chai/core/assertions.js#L741) and [below](https://github.com/chaijs/chai/blob/5dd96a0a3d3bd711ddc688bcd395588aa17a8819/lib/chai/core/assertions.js#L802).
meeber: @lucasfcosta @vieiralucas UH OH THE LUCAS SYNDICATE IS IN FULL FORCE TODAY. Just a reminder to whoever ends up merging: Squash and merge! :D
lucasfcosta: @meeber hahaha yes, thanks for remembering us to do that 😄 
Let's just wait for the `.empty()` change to make the error more self-explanatory (which IMO is a very important thing). Are you guys okay with that or am I being too nitpicky?
shvaikalesh: I have removed `()` and added function names and primitives values to messages for better debugging experience (is that OK?).
vieiralucas: LGTM ?
lucasfcosta: Okay, LGTM, feel free to do the honors and squash and merge if you agree @vieiralucas.

Patch:
diff --git a/lib/chai/core/assertions.js b/lib/chai/core/assertions.js
index d276111..829c174 100644
--- a/lib/chai/core/assertions.js
+++ b/lib/chai/core/assertions.js
@@ -515,8 +515,8 @@ module.exports = function (chai, _) {
    * ### .empty
    *
    * Asserts that the target's length is `0`. For arrays and strings, it checks
-   * the `length` property. For objects, it gets the count of
-   * enumerable keys.
+   * the `length` property. For non-function objects, it gets the count of own
+   * enumerable string keys.
    *
    *     expect([]).to.be.empty;
    *     expect('').to.be.empty;
@@ -528,10 +528,26 @@ module.exports = function (chai, _) {
    */
 
   Assertion.addProperty('empty', function () {
-    var obj = flag(this, 'object');
-    new Assertion(obj).to.exist;
+    var val = flag(this, 'object')
+      , itemsCount;
+
+    switch (_.type(val)) {
+      case 'array':
+      case 'string':
+        itemsCount = val.length;
+        break;
+      case 'function':
+        var name = val.name ? ' ' + val.name : '';
+        throw new TypeError('.empty was passed a function' + name);
+      default:
+        if (val !== Object(val)) {
+          throw new TypeError('.empty was passed non-string primitive ' + _.inspect(val));
+        }
+        itemsCount = Object.keys(val).length;
+    }
+
     this.assert(
-        Object.keys(Object(obj)).length === 0
+        0 === itemsCount
       , 'expected #{this} to be empty'
       , 'expected #{this} not to be empty'
     );


Test patch:
diff --git a/test/expect.js b/test/expect.js
index d33d016..c706021 100644
--- a/test/expect.js
+++ b/test/expect.js
@@ -680,28 +680,63 @@ describe('expect', function () {
 
     err(function(){
       expect(null).to.be.empty;
-    }, "expected null to exist");
+    }, ".empty was passed non-string primitive null");
 
     err(function(){
       expect(undefined).to.be.empty;
-    }, "expected undefined to exist");
+    }, ".empty was passed non-string primitive undefined");
 
     err(function(){
       expect().to.be.empty;
-    }, "expected undefined to exist");
+    }, ".empty was passed non-string primitive undefined");
 
     err(function(){
       expect(null).to.not.be.empty;
-    }, "expected null to exist");
+    }, ".empty was passed non-string primitive null");
 
     err(function(){
       expect(undefined).to.not.be.empty;
-    }, "expected undefined to exist");
+    }, ".empty was passed non-string primitive undefined");
 
     err(function(){
       expect().to.not.be.empty;
-    }, "expected undefined to exist");
+    }, ".empty was passed non-string primitive undefined");
+
+    err(function(){
+      expect(0).to.be.empty;
+    }, ".empty was passed non-string primitive 0");
 
+    err(function(){
+      expect(1).to.be.empty;
+    }, ".empty was passed non-string primitive 1");
+
+    err(function(){
+      expect(true).to.be.empty;
+    }, ".empty was passed non-string primitive true");
+
+    err(function(){
+      expect(false).to.be.empty;
+    }, ".empty was passed non-string primitive false");
+
+    if (typeof Symbol !== 'undefined') {
+      err(function(){
+        expect(Symbol()).to.be.empty;
+      }, ".empty was passed non-string primitive Symbol()");
+
+      err(function(){
+        expect(Symbol.iterator).to.be.empty;
+      }, ".empty was passed non-string primitive Symbol(Symbol.iterator)");
+    }
+
+    err(function(){
+      expect(function() {}).to.be.empty;
+    }, ".empty was passed a function");
+
+    if (FakeArgs.name === 'FakeArgs') {
+      err(function(){
+        expect(FakeArgs).to.be.empty;
+      }, ".empty was passed a function FakeArgs");
+    }
   });
 
   it('NaN', function() {
diff --git a/test/should.js b/test/should.js
index e99a1fb..b827784 100644
--- a/test/should.js
+++ b/test/should.js
@@ -657,6 +657,42 @@ describe('should', function() {
     err(function(){
       ({foo: 'bar'}).should.be.empty;
     }, "expected { foo: \'bar\' } to be empty");
+
+    err(function(){
+      (0).should.be.empty;
+    }, ".empty was passed non-string primitive 0");
+
+    err(function(){
+      (1).should.be.empty;
+    }, ".empty was passed non-string primitive 1");
+
+    err(function(){
+      true.should.be.empty;
+    }, ".empty was passed non-string primitive true");
+
+    err(function(){
+      false.should.be.empty;
+    }, ".empty was passed non-string primitive false");
+
+    if (typeof Symbol !== 'undefined') {
+      err(function(){
+        Symbol().should.be.empty;
+      }, ".empty was passed non-string primitive Symbol()");
+
+      err(function(){
+        Symbol.iterator.should.to.be.empty;
+      }, ".empty was passed non-string primitive Symbol(Symbol.iterator)");
+    }
+
+    err(function(){
+      (function() {}).should.be.empty;
+    }, ".empty was passed a function");
+
+    if (FakeArgs.name === 'FakeArgs') {
+      err(function(){
+        FakeArgs.should.be.empty;
+      }, ".empty was passed a function FakeArgs");
+    }
   });
 
   it('finite(value)', function() {