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

Jump to bottom

Add ES6 collection support to include() #994

Merged
merged 7 commits into from
Jun 23, 2017
Merged

Add ES6 collection support to include() #994

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
07de33e
fix error messages tests
shvaikalesh Jun 21, 2017
86a9ea3
add tests
shvaikalesh Jun 21, 2017
4d56c3d
add implementation
shvaikalesh Jun 21, 2017
0e809c3
performance tweaks
shvaikalesh Jun 22, 2017
defc699
add tests for SameValueZero
shvaikalesh Jun 22, 2017
039564d
drop weakmap support
shvaikalesh Jun 23, 2017
df6badd
update docs
shvaikalesh Jun 23, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
53 changes: 40 additions & 13 deletions lib/chai/core/assertions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,21 +453,16 @@ module.exports = function (chai, _) {
flag(this, 'contains', true);
}

function isDeepIncluded (arr, val) {
return arr.some(function (arrVal) {
return _.eql(arrVal, val);
});
}

function include (val, msg) {
if (msg) flag(this, 'message', msg);

_.expectTypes(this, ['array', 'object', 'string']);
_.expectTypes(this, [
'array', 'object', 'string',
'map', 'set', 'weakmap', 'weakset',
]);

var obj = flag(this, 'object')
, objType = _.type(obj).toLowerCase()
, isDeep = flag(this, 'deep')
, descriptor = isDeep ? 'deep ' : '';
, objType = _.type(obj).toLowerCase();

// This block is for asserting a subset of properties in an object.
if (objType === 'object') {
Expand Down Expand Up @@ -504,10 +499,42 @@ module.exports = function (chai, _) {
return;
}

// Assert inclusion in an array or substring in a string.
var isDeep = flag(this, 'deep')
, descriptor = isDeep ? 'deep ' : ''
, included = false;

switch (objType) {
case 'string':
included = obj.indexOf(val) !== -1;
break;

case 'weakmap':
case 'weakset':
if (isDeep) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should consider containing code like this into a private method: it is easy to forget to process flagMsg, pass ssfi, and skip second argument.

Copy link
Member

@keithamus keithamus Jun 22, 2017 •
edited

Choose a reason for hiding this comment

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

Agreed, but I'd like to see focus on #585 to get the assertion functions to be pure functions returning objects, so we can wrap them up in AssertionErrors

var flagMsg = flag(this, 'message')
, ssfi = flag(this, 'ssfi');
flagMsg = flagMsg ? flagMsg + ': ' : '';

throw new AssertionError(
flagMsg + 'unable to use .deep.include with weak collection',
undefined,
ssfi
);
}

included = obj.has(val);
break;

default:
var isEql = isDeep ? _.eql : function(a, b) { return a === b; };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like we should use SameValue here, but it is minor breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

Yes - we should look to change many of these instances for chai@5 i think

obj.forEach(function (item) {
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, we might be able to optimize this by getting out of the loop when we found the included item.

In order to support IE and older versions of it we can throw an error inside the forEach callback and catch it.

This is a cross-platform compatible implementation I've made on Sinon for the every method. We can inspire our some implementation in that. However, even though I'd like it to be done, I won't block merging this PR because of that.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lucasfcosta I think this is worth exploring. I'd prefer this kind of optimization technique be implemented in a separate PR as a refactor.

Copy link
Member

Choose a reason for hiding this comment

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

@meeber agreed.

LGTM! Let's get this merged.

included = included || isEql(item, val);
});
Copy link
Member

Choose a reason for hiding this comment

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

Can we do something here to optimize this by breaking out the loop once included is true?

I realize that then we would need to have 3 implementations, one for Array, another for Map and another for Set.

Do you think it is worth it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can optimize away looping through set for non-deep include and use some + indexOf on arrays to avoid extra functions. Code:

case 'map':
  let isEql = isDeep ? _.eql : function(a, b) { return a === b; };
  // I believe SameValueZero should be ^^ here to be on par with Set#has
  obj.forEach(function (item) {
    included = included || isEql(item, val);
  });
  break;

case 'set':
  if (isDeep) {
    obj.forEach(function (item) {
      included = included || _.eql(item, val);
    });
  } else {
    included = obj.has(val);
  }
  break;

case 'array':
  if (isDeep) {
    included = obj.some(function (item) {
      return _.eql(item, val);
    });
  } else {
    included = obj.indexOf(val) !== -1;
  }
  break;

I am not sure the benefits outweigh code amount & complexity.

EDIT: sorry, email readers.

Copy link
Member

Choose a reason for hiding this comment

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

This looks good but I'd be a little careful here with IE11 - which has terrible support for Map/Set. It probably will work but I've often been surprised how seemingly obvious features are missing from IE11's Map/Set

}

// Assert inclusion in collection or substring in a string.
this.assert(
objType === 'string' || !isDeep ? ~obj.indexOf(val)
: isDeepIncluded(obj, val)
included
, 'expected #{this} to ' + descriptor + 'include ' + _.inspect(val)
, 'expected #{this} to not ' + descriptor + 'include ' + _.inspect(val));
}
Expand Down
119 changes: 111 additions & 8 deletions test/assert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -633,6 +633,42 @@ describe('assert', function () {
assert.include({foo: obj1, bar: obj2}, {foo: obj1});
assert.include({foo: obj1, bar: obj2}, {foo: obj1, bar: obj2});

if (typeof Map === 'function') {
var map = new Map();
var val = [{a: 1}];
map.set('a', val);
map.set('b', 2);

assert.include(map, val);
assert.include(map, 2);
}

if (typeof Set === 'function') {
var set = new Set();
var val = [{a: 1}];
set.add(val);
set.add(2);

assert.include(set, val);
assert.include(set, 2);
}

if (typeof WeakMap === 'function') {
var wm = new WeakMap();
var val = [{a: 1}];
wm.set(val, 1);

assert.include(wm, val);
}

if (typeof WeakSet === 'function') {
var ws = new WeakSet();
var val = [{a: 1}];
ws.add(val);

assert.include(ws, val);
}

if (typeof Symbol === 'function') {
var sym1 = Symbol()
, sym2 = Symbol();
Expand All @@ -653,19 +689,19 @@ describe('assert', function () {

err(function(){
assert.include(true, true, 'blah');
}, "blah: object tested must be an array, an object, or a string, but boolean given");
}, "blah: object tested must be an array, a map, an object, a set, a string, a weakmap, or a weakset, but boolean given");

err(function () {
assert.include(42, 'bar');
}, "object tested must be an array, an object, or a string, but number given");
}, "object tested must be an array, a map, an object, a set, a string, a weakmap, or a weakset, but number given");

err(function(){
assert.include(null, 42);
}, "object tested must be an array, an object, or a string, but null given");
}, "object tested must be an array, a map, an object, a set, a string, a weakmap, or a weakset, but null given");

err(function () {
assert.include(undefined, 'bar');
}, "object tested must be an array, an object, or a string, but undefined given");
}, "object tested must be an array, a map, an object, a set, a string, a weakmap, or a weakset, but undefined given");
});

it('notInclude', function () {
Expand All @@ -678,6 +714,47 @@ describe('assert', function () {
assert.notInclude({foo: obj1, bar: obj2}, {foo: {a: 1}});
assert.notInclude({foo: obj1, bar: obj2}, {foo: obj1, bar: {b: 2}});

if (typeof Map === 'function') {
var map = new Map();
var val = [{a: 1}];
map.set('a', val);
map.set('b', 2);

assert.notInclude(map, [{a: 1}]);
assert.notInclude(map, 3);
}

if (typeof Set === 'function') {
var set = new Set();
var val = [{a: 1}];
set.add(val);
set.add(2);

assert.include(set, val);
assert.include(set, 2);

assert.notInclude(set, [{a: 1}]);
assert.notInclude(set, 3);
}

if (typeof WeakMap === 'function') {
var wm = new WeakMap();
var val = [{a: 1}];
wm.set(val, 1);

assert.notInclude(wm, [{a: 1}]);
assert.notInclude(wm, {});
}

if (typeof WeakSet === 'function') {
var ws = new WeakSet();
var val = [{a: 1}];
ws.add(val);

assert.notInclude(ws, [{a: 1}]);
assert.notInclude(ws, {});
}

if (typeof Symbol === 'function') {
var sym1 = Symbol()
, sym2 = Symbol()
Expand All @@ -699,19 +776,19 @@ describe('assert', function () {

err(function(){
assert.notInclude(true, true, 'blah');
}, "blah: object tested must be an array, an object, or a string, but boolean given");
}, "blah: object tested must be an array, a map, an object, a set, a string, a weakmap, or a weakset, but boolean given");

err(function () {
assert.notInclude(42, 'bar');
}, "object tested must be an array, an object, or a string, but number given");
}, "object tested must be an array, a map, an object, a set, a string, a weakmap, or a weakset, but number given");

err(function(){
assert.notInclude(null, 42);
}, "object tested must be an array, an object, or a string, but null given");
}, "object tested must be an array, a map, an object, a set, a string, a weakmap, or a weakset, but null given");

err(function () {
assert.notInclude(undefined, 'bar');
}, "object tested must be an array, an object, or a string, but undefined given");
}, "object tested must be an array, a map, an object, a set, a string, a weakmap, or a weakset, but undefined given");

err(function () {
assert.notInclude('foobar', 'bar');
Expand All @@ -731,6 +808,32 @@ describe('assert', function () {
assert.notDeepInclude({foo: obj1, bar: obj2}, {baz: {a: 1}});
assert.notDeepInclude({foo: obj1, bar: obj2}, {foo: {a: 1}, bar: {b: 9}});

if (typeof Map === 'function') {
var map = new Map();
map.set(1, [{a: 1}]);

assert.deepInclude(map, [{a: 1}]);
}

if (typeof Set === 'function') {
var set = new Set();
set.add([{a: 1}]);

assert.deepInclude(set, [{a: 1}]);
}

if (typeof WeakMap === 'function') {
err(function() {
assert.deepInclude(new WeakMap(), {}, 'foo');
}, 'foo: unable to use .deep.include with weak collection');
}

if (typeof WeakSet === 'function') {
err(function() {
assert.deepInclude(new WeakSet(), {});
}, 'unable to use .deep.include with weak collection');
}

err(function () {
assert.deepInclude([obj1, obj2], {a: 9}, 'blah');
}, "blah: expected [ { a: 1 }, { b: 2 } ] to deep include { a: 9 }");
Expand Down
88 changes: 79 additions & 9 deletions test/expect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1694,6 +1694,50 @@ describe('expect', function () {
expect({foo: obj1, bar: obj2}).to.not.include({foo: {a: 1}});
expect({foo: obj1, bar: obj2}).to.not.include({foo: obj1, bar: {b: 2}});

if (typeof Map === 'function') {
var map = new Map();
var val = [{a: 1}];
map.set('a', val);
map.set('b', 2);

expect(map).to.include(val);
expect(map).to.not.include([{a: 1}]);
expect(map).to.include(2);
expect(map).to.not.include(3);
}

if (typeof Set === 'function') {
var set = new Set();
var val = [{a: 1}];
set.add(val);
set.add(2);

expect(set).to.include(val);
expect(set).to.not.include([{a: 1}]);
expect(set).to.include(2);
expect(set).to.not.include(3);
}

if (typeof WeakMap === 'function') {
var wm = new WeakMap();
var val = [{a: 1}];
wm.set(val, 1);

expect(wm).to.include(val);
expect(wm).to.not.include([{a: 1}]);
expect(wm).to.not.include({});
}

if (typeof WeakSet === 'function') {
var ws = new WeakSet();
var val = [{a: 1}];
ws.add(val);

expect(ws).to.include(val);
expect(ws).to.not.include([{a: 1}]);
expect(ws).to.not.include({});
}

if (typeof Symbol === 'function') {
var sym1 = Symbol()
, sym2 = Symbol()
Expand Down Expand Up @@ -1748,39 +1792,39 @@ describe('expect', function () {

err(function(){
expect(true).to.include(true, 'blah');
}, "blah: object tested must be an array, an object, or a string, but boolean given");
}, "blah: object tested must be an array, a map, an object, a set, a string, a weakmap, or a weakset, but boolean given");

err(function(){
expect(true, 'blah').to.include(true);
}, "blah: object tested must be an array, an object, or a string, but boolean given");
}, "blah: object tested must be an array, a map, an object, a set, a string, a weakmap, or a weakset, but boolean given");

err(function(){
expect(42.0).to.include(42);
}, "object tested must be an array, an object, or a string, but number given");
}, "object tested must be an array, a map, an object, a set, a string, a weakmap, or a weakset, but number given");

err(function(){
expect(null).to.include(42);
}, "object tested must be an array, an object, or a string, but null given");
}, "object tested must be an array, a map, an object, a set, a string, a weakmap, or a weakset, but null given");

err(function(){
expect(undefined).to.include(42);
}, "object tested must be an array, an object, or a string, but undefined given");
}, "object tested must be an array, a map, an object, a set, a string, a weakmap, or a weakset, but undefined given");

err(function(){
expect(true).to.not.include(true);
}, "object tested must be an array, an object, or a string, but boolean given");
}, "object tested must be an array, a map, an object, a set, a string, a weakmap, or a weakset, but boolean given");

err(function(){
expect(42.0).to.not.include(42);
}, "object tested must be an array, an object, or a string, but number given");
}, "object tested must be an array, a map, an object, a set, a string, a weakmap, or a weakset, but number given");

err(function(){
expect(null).to.not.include(42);
}, "object tested must be an array, an object, or a string, but null given");
}, "object tested must be an array, a map, an object, a set, a string, a weakmap, or a weakset, but null given");

err(function(){
expect(undefined).to.not.include(42);
}, "object tested must be an array, an object, or a string, but undefined given");
}, "object tested must be an array, a map, an object, a set, a string, a weakmap, or a weakset, but undefined given");
});

it('deep.include()', function () {
Expand All @@ -1796,6 +1840,32 @@ describe('expect', function () {
expect({foo: obj1, bar: obj2}).to.not.deep.include({baz: {a: 1}});
expect({foo: obj1, bar: obj2}).to.not.deep.include({foo: {a: 1}, bar: {b: 9}});

if (typeof Map === 'function') {
var map = new Map();
map.set(1, [{a: 1}]);

expect(map).to.deep.include([{a: 1}]);
}

if (typeof Set === 'function') {
var set = new Set();
set.add([{a: 1}]);

expect(set).to.deep.include([{a: 1}]);
}

if (typeof WeakMap === 'function') {
err(function() {
expect(new WeakMap()).to.deep.include({}, 'foo');
}, 'foo: unable to use .deep.include with weak collection');
}

if (typeof WeakSet === 'function') {
err(function() {
expect(new WeakSet()).to.deep.include({});
}, 'unable to use .deep.include with weak collection');
}

err(function () {
expect([obj1, obj2]).to.deep.include({a: 9}, 'blah');
}, "blah: expected [ { a: 1 }, { b: 2 } ] to deep include { a: 9 }");
Expand Down