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.

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

Added includeDeepMembers #589

Merged
merged 1 commit into from Jan 11, 2016
Merged

Conversation

@qbolec
Copy link
Contributor

@qbolec qbolec commented Jan 11, 2016

This is a proposition to add assert.includeDeepMembers.
It's a second attempt, as previous #588 was flawed due to my ignorance of git.
Hopefully now we can discus a the merit of the change, as I have some doubts:

  • is it ok that includeDeepMembers and includeMembers ignore duplicates in the subset argument? I'd expect the function to ignore order, but not quantity of elements to be a useful feature for some scenarios (same question for sameDeepMembers, and sameMembers). The motivating example is when I want to make sure that one array is permutation of another one.
  • I've added a testcase to explicitly document current behaviour described above. Is it ok to add such tests or is it unnecessarily constraining future modifications of implementation? I'm not sure if I describe my dilemma clearly, so let me paraphrase it: if the specification of a function is quite general, should the tests be more precise than the spec or be as general as the spec? I see benefits on both sides.
@keithamus
Copy link
Member

@keithamus keithamus commented Jan 11, 2016

Thanks @qbolec for the PR.

includeDeepMembers only checks that all members in the expected value are present in the actual value. So when it comes to duplicate values - I think this is acceptable. If you want a stricter checking - including that of length, then have.deep.members (haveDeepMembers) would be the more appropriate test. Unless you think we should add another one?

To answer your question about tests: yes - the tests are the spec for the assertion, so the more tests the better. If we decide that it is doing undesirable things then we can change the behaviour in a breaking version - we shouldn't be afraid to do that.

How do you feel about the above? Happy for me to merge this or want to discuss some more?

@qbolec
Copy link
Contributor Author

@qbolec qbolec commented Jan 11, 2016

Not sure if we understand each other.
Currently all of these assertions pass:

[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]);

while I think they should all fail, or at least there should be some switch, option, or other method which would make them fail, as otherwise it is not trivial to implement a check for being a permutation.

@keithamus
Copy link
Member

@keithamus keithamus commented Jan 11, 2016

[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]);

Hmm, as I understood the point, and in my opinion, include should be the only one that passes. This is something we definitely need to address - but let's get this PR merged, and raise a new issue to discuss the semantics of the above assertions, sound good?

@qbolec
Copy link
Contributor Author

@qbolec qbolec commented Jan 11, 2016

Sounds good to me :)

keithamus added a commit that referenced this pull request Jan 11, 2016
Added includeDeepMembers
@keithamus keithamus merged commit e614fee into chaijs:master Jan 11, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@qbolec qbolec deleted the qbolec:add-includedeepmembers branch Jan 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.