chai: expect(array).to.not.include(obj) failing with constructors

This may be linked to https://github.com/chaijs/chai/issues/390, but I’m not sure so reporting this separately:

Environment:

Chai version: 3.5.0
Node version: 6.2.0

Test case:

function SomeConstructor() {}
const instance1 = new SomeConstructor();
const instance2 = new SomeConstructor();

expect(instance1).to.not.equal(instance2) // => passes as expected

const arr = [instance1];
expect(arr).to.include(instance1) // => passes as expected
expect(arr).to.not.include(instance2) // => fails; unexpected

As a workaround, I can change the constructor to:

function SomeConstructor() {
  this.uid = Symbol();
}

and expect(arr).to.not.include(instance2) will work fine.

About this issue

Original URL
State: closed
Created 8 years ago
Reactions: 1
Comments: 25 (23 by maintainers)

Most upvoted comments

I’ll likely knock this out over the weekend unless someone else chimes in wanting to do it.

meeber on Jul 1, 2016

Looked through the commit history. Here’s what happened:

Back in Chai 1.0, .include just used a simple indexOf test. Therefore, it used strict equality instead of deep.
A PR (#230) was introduced to enhance .include to also allow this type of assertion: expect({a: 1, b: 2}).to.include({a: 1});. It accomplishes this by using the existing .property assertion, aka, a strict comparison on the value of each key being tested for inclusion. (It was briefly mentioned how it’d be nice for a deep comparison to be possible, but not followed up on.)
An issue (#239) was raised stating that the PR mentioned above broke existing code, specifically when testing for inclusion of an object inside of an array.
A PR (#244) was introduced to fix the issue mentioned above. However, instead of merely fixing it by making it use indexOf like it did previously when testing for an object inside of an array, it was instead fixed in such a way that it now used deep equality instead of strict. There was no discussion or even casual acknowledgement of this difference in functionality; it was just accepted and merged.

Based on the above, I think the current functionality is a bug. After all, the .deep flag did exist back when #244 was introduced, so I don’t think #244 would’ve been merged if it’d been realized that the comparison method changed from strict to deep in the process of fixing the #239 regression.

This makes me feel a lot more comfortable about changing the current behavior. As I said before, the current functionality has always felt a bit strange to me, but I hadn’t looked into it deeply so I just kinda assumed it had been designed this way on purpose.

I propose the following (which I believe is in line with what has already been suggested):

Keep current functionality for string inclusion, which uses indexOf.
Keep current default functionality for non-negated property inclusion, which uses .property to assert that the expected key(s) are present in an object, and that their values are strictly equal.
Change current default functionality for negated property inclusion from using manual deep equality to using strict .property assertion.
Change current default functionality for array inclusion from deep equality to strict equality. This could be done using indexOf as it originally was.
Add deep flag support, which overrides the default functionality of both property and array inclusion in order to use deep equality instead of strict.

meeber on Jul 2, 2016

Cool thanks so much, @lucasfcosta and @meeber. I can confirm that include.members works without having to put a uid prop on the objects returned from the constructor.

For what it’s worth, @lucasfcosta, your proposed change to do an initial check if deep is flagged before using deep-eql to make the comparison is what I would have expected as a user of the library.

Thanks again.

GabeMedrash on Jun 30, 2016

@lucasfcosta I thought about it more this morning and have an idea for a different approach: #745

meeber on Jul 4, 2016

@meeber Makes sense, I even wondered how we were going to get a variable’s name to do this check 😆 Now the third option really seems the most correct. This reminds me of the problem we’ve had with the throws assertion, where only the Constructor or the message matching would be enough for the assertion to fail when using the negate flag. I totally agree with you, when using the negate flag a single condition being false (hasProperty or propertyValue) should be enough for the assertion to pass.

However, thinking that when we’ve got two arguments and the negate flag enabled only the value should make the assertion fail isn’t strange, I think many people would think this is the default behavior, so I think we need to make this behavior clear in the docStrings.

If anyone disagrees you’re welcome to share your point of view 😄

lucasfcosta on Jul 2, 2016

@GabeMedrash As a workaround I recommend using .members:

    expect(arr).to.include.members([instance1]);
    expect(arr).to.not.include.members([instance2]);

@lucasfcosta The behavior of .include as an assertion has always seemed a bit off to me, especially when compared to .include.members. I’d like to go back in time to read up on the PR that first introduced it (unless it was an original assertion). Will return to this later.

meeber on Jun 30, 2016