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

[GabeMedrash]

This may be linked to #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.

[lucasfcosta]

Hi @GabeMedrash, thanks for the issue!

I think you are right and this is linked to #390.
I'm now reading this code here and it seems like if we've got an object inside an Array, we look for val inside every index of the Array using deep-eql from utils. The problem is that deep-eql compares an object's primitive keys one by one instead of doing an strict equality check (===), this means that if your instances have the same properties Chai will think that they're the same (because that is what deep-eql does).

I think that we should fix this by adding the possibility of using the deep flag for the includes assertion. However, doing this would be a breaking change, since the current default behavior is to use deep-eql. This could break a lot of people. If we want this to be a more 'soft' change maybe we could have an strict flag instead of deep flag.

What do you think @meeber and @keithamus (anyone else is also welcome to share your thoughts on this, I'm just tagging these two guys so they can see it 😄 )?

In any case, implementing that should be reasonably easy, all we've gotta do is add a check before running _.eql and then running a strict comparison (indexOf or anything like that) instead of _.eql when we don't have the deep flag turned on (or when we have the strict flag turned on, whatever you guys prefer).

[meeber]

@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.

[GabeMedrash]

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.

[meeber]

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

1. Back in Chai 1.0, .include just used a simple indexOf test. Therefore, it used strict equality instead of deep.
2. A PR (support {a:1,b:2}.should.include({a:1}) #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.)
3. An issue (contain/include bug introduced in v1.9 #239) was raised stating that the PR mentioned above broke existing code, specifically when testing for inclusion of an object inside of an array.
4. A PR (fixing #239 (without changing chai.js) #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.

[keithamus]

Agreed with @meeber. Sounds like a straightforward fix and falls inline with other assertions.

[lucasfcosta]

I also agree with @meeber.
Well, since we're all on the same page I'm going to tag this as PR Wanted. 😄

[meeber]

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

[meeber]

Just noting that I was half-wrong about this statement:

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 only uses .property, and thus strict comparison, when the negate flag isn't set. When the negate is set then it uses a manual deep comparison instead.

Therefore, I'm changing the plan of action from:

• Keep current default functionality for property inclusion, which uses .property to assert that the expected key(s) are present in an object, and that their values are strictly equal.

To:

• 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.

[meeber]

I ran into a problem while refactoring .not.include to use .not.property to match how .include uses .property.

This is with the current version of Chai, prior to my changes:

expect({a: 1, b: 2}).to.not.include({c: 3}); // Passes
expect({a: 1, b: 2}).to.not.have.property('c', 3); // Error: { a: 1, b: 2 } has no property 'c'

In my opinion, there are three options:

1. The two assertions above are behaving exactly as desired. Therefore, instead of refactoring .not.include to use .not.property, we should refactor .include to stop using .property. (We'd still refactor .not.include to only use deep equality when .deep flag is present; separate issue.)
2. The two assertions above should both fail. Therefore, we should proceed as planned to refactor .not.include to use .not.property.
3. The two assertions above should both pass. Therefore, we should proceed as planned to refactor .not.include to use .not.property, and we should also modify the behavior of .not.property to pass in the above situation.

I feel pretty strongly that the third option is the most intuitive. To me, expect(obj).to.not.have.property('c', 3) should mean "expect obj to not have a property named c that's equal to 3" instead of its current meaning of "expect obj to have a property named c that's not equal to 3".

However, this argument was made before in #16, and was ultimately rejected.

@keithamus @lucasfcosta Your thoughts?

[lucasfcosta]

@meeber Well, IMO the first one is the most correct.
I think that if c has not been defined its value should be undefined and therefore the object being tested won't have this key, I think that is what most people would expect to happen given the inner working of the language.

However, if that second assertion was expect({a: 1, b: 2}).to.not.have.property('c', 3) then I would expect it to pass, since 'c' is a string representing the name of the property I want to check. When using only a c variable I would like it to assume the value assigned to it and not the name of the variable ('c' in this case).

If we used option 3 I feel like we would be going against the language standards, it would behave different from the language and there would be no way of asserting for a property dinamically (using the value hold by c in this case, for example).

Let me know if you need any help. I'm also open to talk about what is the optimal choice here so we can all agree about what would be better 😄

[meeber]

@lucasfcosta Whoops, screwed up my example. I just corrected my post. I meant expect({a: 1, b: 2}).to.not.have.property('c', 3)

[lucasfcosta]

@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 😄

[meeber]

@lucasfcosta Yes, it's exactly like the issue we ran into with the throws assertion, right down to the part where we'll need to jump through a couple of hoops to make sure that the negated assertion logic doesn't short-circuit too early.

[meeber]

Ran into yet another problem. Applying the deep flag to a property assertion doesn't cause it to use deep comparison instead of strict. Instead:

• If the deep flag is set, you can use dot- and bracket-notation for deep
• references into objects and arrays.

This is highly problematic in terms of enabling deep equality support on the .include assertion while internally leveraging .property.

At this point I'm inclined to say that .include shouldn't use .property at all. The .property assertion seems to have a bunch of quirks that make it unsuitable for anything except direct usage. In this case, merging #744 isn't necessary. I'd rather just leave it alone at this point.

[lucasfcosta]

@meeber I'm afraid I didn't understand your problem with the deep flag, sorry 😓 Would you mind explaining it with an example or showing me some code?

IMO the behavior we have agreed upon before seems to be the most correct, but if we really can't do this without much trouble maybe we should make the current behavior more explicit on the DocStrings.

[meeber]

The problem I'm having with the deep flag is that it currently means something completely different for .property than what we wanted it to mean for .include.

The deep flag when used with the .property assertion doesn't change the equality comparison from strict to deep. Instead, what it does is let the developer write their assertions like the following examples, using brackets and dots:

var deepObj = {
    green: { tea: 'matcha' }
  , teas: [ 'chai', 'matcha', { tea: 'konacha' } ]
};

expect(deepObj).to.have.deep.property('green.tea', 'matcha');
expect(deepObj).to.have.deep.property('teas[1]', 'matcha');
expect(deepObj).to.have.deep.property('teas[2].tea', 'konacha');

It hurts my heart that the deep flag means something completely different when used with .property than it does when used for other assertions like .equal or .members. It also makes it impossible for .include to continue leveraging .property internally without resulting in a huge breaking change to .property.

We can still make .include do everything we agreed upon. Just need to divorce it completely from .property.

Edit: We can still also merge #744 if we believe that to be the desired behavior for .property; it's just no longer necessary to do so in order to complete the changes to .include, since .include will no longer use .property at all.

[lucasfcosta]

@meeber makes sense to me now, thanks for the explanation.
Well, so this means we will still be able to use deep for property with the behavior you have just described above but now the include assertion will change its comparison criteria based on that flag (and won't use .propertyanymore), am I right?
If this is what we're gonna do, I'm +1 for that.

Great job buddy (and thanks again for taking your time to explain the details to me 😄 )

[meeber]

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

[lukashoegh]

This issue just made me feel confused for a couple of hours. Could you perhaps change the documentation to make it more clear what include does; more specifically perhaps add examples to clarify that [{a:1}].should.include({a:1}) passes?

[meeber]

@lukashoegh I'm working on overhauling the .include assertion so that it's more consistent with the rest of Chai. One result of this overhaul is that the example you provided will only pass when the deep flag is used. Documentation will be updated accordingly.

[meeber]

@lucasfcosta @keithamus Big changes ready for review. I submitted these changes in the form of 5 separate PRs with 1 commit each that build upon one another; I'm interested to know if this approach is easier/harder/identical from a reviewer's perspective than a single PR with 5 commits.

The PRs in order are: #744, #757, #758, #760, #761

What I was trying to achieve with these PRs is best summarized by this comment. Basically trying to make the .property and .include assertions more consistent with the rest of Chai, especially in regard to the .deep flag, and fix a couple of bugs as well.

Summary of breaking changes across these PRs:

• Previously, expect(obj).not.property(name, val) would throw an Error if obj didn't have a property named name. This change causes the assertion to pass instead.
• assert.propertyNotVal renamed to assert.notPropertyVal
• assert.deepPropertyNotVal renamed to assert.notDeepPropertyVal
• Previously, the deep flag had unexpected behavior when used with the property assertion: Instead of switching from strict to deep equality, it unlocked the ability to use dot and bracket notation when referencing property names. This behavior now belongs to the new nested flag.
• assert.deepProperty renamed to assert.nestedProperty
• assert.notDeepProperty renamed to assert.notNestedProperty
• assert.deepPropertyVal renamed to assert.nestedPropertyVal
• assert.notDeepPropertyVal renamed to assert.notNestedPropertyVal
• Previously, .include was using strict equality for non-negated property inclusion, but deep equality for negated property inclusion and array inclusion. This fix causes .include to always use strict equality.

In addition to these breaking changes, .deep support was added to both the .property and .include assertions for deep equality comparisons.

[keithamus]

Reviewed and agree with the changes. Although it changes lots for our users, its a useful change and makes deep a lot cleaner and more precise in its use.

@meeber - I will say that, because of the impact of this change, we should also consider releasing a tool to help migrate code from <4 to 4.0 - using something like jscodeshift to automatically change our user's code to switch to nested over deep.

[meeber]

@keithamus Thanks for reviewing! Just out of curiosity, did it help having the commits split between multiple PRs, even though only the last one is actually needed?

I'll take a look at jscodeshift. Were you envisioning packaging such a fixer tool with Chai 4.x, or having it be a separate repo within the chai org?

Regarding 4.x: This was the last major change I was working on. I know we still have the deep-eql refactor and possibly loupe, although I'm thinking now that maybe it's best to hold off on loupe until next release? Regardless, I feel like we're close enough to start merging in the 4.x branch. Thoughts?

[keithamus]

Just out of curiosity, did it help having the commits split between multiple PRs, even though only the last one is actually needed?

In this instance because they were multiple discrete changes it makes sense for individual PRs. So 👍

Were you envisioning packaging such a fixer tool with Chai 4.x, or having it be a separate repo within the chai org?

One of those, not sure which. Just wanted to spur a conversation about it

Regarding 4.x: This was the last major change I was working on. I know we still have the deep-eql refactor and possibly loupe, although I'm thinking now that maybe it's best to hold off on loupe until next release? Regardless, I feel like we're close enough to start merging in the 4.x branch. Thoughts?

I need to get some time to really push on deep-eql and ideally I'd like loupe too, but we can let it slide to concentrate on getting 4.0 out the door. deep-eql is, I feel, mandatory for 4.0.

[lucasfcosta]

@keithamus @meeber Using jscodeshift seems like a great idea, I've read lots of good things about it and it seems to work very fine but IMO it is not a mandatory thing to release alongside 4.0.

I think a migration guide with detailed explanation would be enough, I'd love to help writing it. We can also inspire ourselves into the react-router migration guides. Those are really good materials, full of examples and explanations about why things changed and how to upgrade codebases while still maintaining good practices.