Chai: Handling of duplicates when comparing sets

[qbolec]

Current behaviour is that

[3,1,2,3].should.have.deep.members([3,2,1]);      
[1,2,3].should.have.deep.members([3,2,1,3]);
[1,2,3].should.deep.include.members([3,2,1,3]);
chai.assert.sameDeepMembers([1,2,3],[3,2,1,3]);

all pass.
The documentation uses language of "sets" not "multisets" so it is hard to judge what should be the good behaviour when the arguments contain duplicates.
One could argue that current implementation of should.deep.include.members is consistent with the documentation, as indeed every element of [3,2,1,3] belongs to [1,2,3].
Similarly, one could argue similarly for should.have.deep.members.
But in practice I found myself in situations where what I really want to test is if the actual is a permutation of expected. There are many situations in which an order in the array cannot be guaranteed (say, Object.keys(obj)).

The issue might seem not so controversial when the arguments are written as literals and one can easily see what is the problem.
But in a more realistic scenarios like:

emittedEvents.should.have.deep.members(expectedEvents)

one could easily be fooled into thinking that this assertion simply means "expectedEvents is a permutation of expectedEvents" only to be surprised that firing the same event twice does not violate this test.
Similarly one could have something like:

sorted.should.have.deep.members(inputArray)

and never spot an error in the implementation which caused removal of duplicates.

Perhaps it would be a good thing to have a separate should.be.permutation.of assertion for that case. But care must be taken to redact documentation for the other assertions so they do not suggest wrongly that they could be used for that.

[meeber]

It's my feeling that these should all fail:

[1,2,3].should.have.deep.members([3,2,1,3]);
[1,2,3].should.deep.include.members([3,2,1,3]);
chai.assert.sameDeepMembers([1,2,3],[3,2,1,3]);

@lucasfcosta @keithamus What do you guys think?

[lucasfcosta]

@meeber IMO include should only look for an occurrence instead of expecting a full-match (as @qbolec said it's written on our docs) while have should fail, as you said.
This way we'd have both behaviours available for the users and the word include will be more semantically correct.

[meeber]

@lucasfcosta: Good point. I agree.

Edit: What are your thoughts on this as a bug to be fixed soon, or a breaking change for 4.x.x? I'm slightly leaning toward breaking change, mainly because it doesn't feel urgent.

[lucasfcosta]

@meeber I totally agree, this doesn't seem urgent and changing behaviour seems to be the most adequate change for now, however as it's a breaking change (as you've said it yourself) it should be raised against 4.x.x.

But just to be sure let's wait to hear @keithamus' opinion, he may know what's going to be the optimal choice here :smile:

[meeber]

I plan on tackling this one this weekend.