Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make 'empty' throw on non-string primitives and functions #812

Merged
merged 5 commits into from
Oct 1, 2016
Merged

Make 'empty' throw on non-string primitives and functions #812

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
89b9865
Make 'empty' throw on non-string primitives and functions
shvaikalesh Sep 30, 2016
ccc00bc
Update jsdoc
shvaikalesh Oct 1, 2016
92a40d4
improve error messages
shvaikalesh Oct 1, 2016
79e6fba
fix symbol to string coercion for Node 0.12
shvaikalesh Oct 1, 2016
a1c1dc4
use utils.inspect
shvaikalesh Oct 1, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 20 additions & 5 deletions lib/chai/core/assertions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -528,10 +528,25 @@ 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':
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should handle generators too (because of recent updates to type-detect) and async function in the future.

throw new TypeError('.empty() was passed a function');
Copy link
Member

@vieiralucas vieiralucas Oct 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about replacing .empty() with .empty since it is a property assertion?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @vieiralucas said, I think .empty() should be changed to .empty since it's a property assertion.

default:
if (val !== Object(val)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other option is to add case clause for other primitives' types.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great use of Object(). 😍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That blew my mind to!

throw new TypeError('.empty() was passed non-string primitive');
Copy link
Member

@vieiralucas vieiralucas Oct 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about replacing .empty() with .empty since it is a property assertion?

}
itemsCount = Object.keys(val).length;
}

this.assert(
Object.keys(Object(obj)).length === 0
0 === itemsCount
Copy link
Contributor Author

@shvaikalesh shvaikalesh Sep 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any ESLint config we can add to the project? This will greatly improve contribution/review experience: yoda comparisons and comma-first are not quite popular and it would be awesome to automate codestyle checks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shvaikalesh Yes, we thought about that too, we already discussed that here and we came to the conclusion that by moving all our code to separate repos with their own linting rules would be better, however, if you disagree you're welcome to share your thoughts with us 😄 .

, 'expected #{this} to be empty'
, 'expected #{this} not to be empty'
);
Expand Down
37 changes: 31 additions & 6 deletions test/expect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -680,28 +680,53 @@ describe('expect', function () {

err(function(){
expect(null).to.be.empty;
}, "expected null to exist");
}, ".empty() was passed non-string primitive");

err(function(){
expect(undefined).to.be.empty;
}, "expected undefined to exist");
}, ".empty() was passed non-string primitive");

err(function(){
expect().to.be.empty;
}, "expected undefined to exist");
}, ".empty() was passed non-string primitive");

err(function(){
expect(null).to.not.be.empty;
}, "expected null to exist");
}, ".empty() was passed non-string primitive");

err(function(){
expect(undefined).to.not.be.empty;
}, "expected undefined to exist");
}, ".empty() was passed non-string primitive");

err(function(){
expect().to.not.be.empty;
}, "expected undefined to exist");
}, ".empty() was passed non-string primitive");

err(function(){
expect(0).to.be.empty;
}, ".empty() was passed non-string primitive");

err(function(){
expect(1).to.be.empty;
}, ".empty() was passed non-string primitive");

err(function(){
expect(true).to.be.empty;
}, ".empty() was passed non-string primitive");

err(function(){
expect(false).to.be.empty;
}, ".empty() was passed non-string primitive");

if (typeof Symbol !== 'undefined') {
err(function(){
expect(Symbol()).to.be.empty;
}, ".empty() was passed non-string primitive");
}

err(function(){
expect(function() {}).to.be.empty;
}, ".empty() was passed a function");
});

it('NaN', function() {
Expand Down
26 changes: 26 additions & 0 deletions test/should.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,6 +657,32 @@ 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");

err(function(){
(1).should.be.empty;
}, ".empty() was passed non-string primitive");

err(function(){
true.should.be.empty;
}, ".empty() was passed non-string primitive");

err(function(){
false.should.be.empty;
}, ".empty() was passed non-string primitive");

if (typeof Symbol !== 'undefined') {
err(function(){
Symbol().should.be.empty;
}, ".empty() was passed non-string primitive");
}

err(function(){
(function() {}).should.be.empty;
}, ".empty() was passed a function");
});

it('finite(value)', function() {
Expand Down