should.have.deep.members diff

[qbolec]

I'm new to mocha and chai, so I'm not sure if my issue should be filed against one or the other.
The problem is that I often need to check if two arrays contain same elements up to permutation.
I use A.should.have.deep.members(B) for that.
Here are my issues:

1. I'm not sure if this particular combination of have, deep and members is even "official". The documentation at http://chaijs.com/api/bdd/ for members does not present this particular combo, but I guess, it's ok?
2. the mocha -R spec reporter does not show the diff for this assertions, when they fail. Instead, I get rather not helpful AssertionError: expected [ Array(3) ] to have the same members as [ Array(2) ].
   What would I expect? Something similar to A.sort().should.deep.equal(B.sort()). That is, I'd like to see '-' for each additional member, and '+' for each missing.
3. if this is out of scope of chai project, and should be solved by the mocha test runner, or one of it's reporters, could you please help me, by pointing me in the right direction. Is there a way I could fix this myself?

[keithamus]

Hey @qbolec thanks for the issue and welcome to the Chai community 🎉🍰! I'll address each point you have separately:

1. have is a language chain - it does nothing and can be used anywhere. members supports the deep flag and so .have.beep.members() is a perfectly valid assertion. .deep.members will expect you to provide the same array, but sort order wont matter (include.deep.members will allow you to assert on a subset, again unsorted). - in other words, A.should.have.deep.members(B) is the equivalent to A.sort().should.deep.equal(B.sort()) or thereabouts. We should probably document this better!
2. The message (AssertionError: expected [ Array(3) ] to have the same members as [ Array(2) ]) is part of Chai. Where it says Array(3) - this is chai truncating the output so you don't get really long error lines (which are not diffs). Chai does not do any of the diffing, that's all in Mocha's camp - but Chai tells Mocha that an assertion error should have a diff generated. In this case we don't do that, so we need to fix that, which leads me to point 3...
3. This is in scope for Chai! To fix this, it should be as simple a case as adding true to the two this.assert calls (here and here). If you'd like to take a go at fixing this, I can help you through the process. Everyone who contributes to Chai gets a place in our wonderful hall of fame plus a million internet points, so it's worth it 😀!

[qbolec]

I'll do my best :)
Are there any guidelines I should read before I start?
If not, then I'll go with the following plan:

1. fork the repo
2. learn how to run the tests (make sure they pass)
3. learn how to add a new test (where to put it, how to name it, what's the style)
4. write a new test for this particular change (decide how specific the test should be, what is the expected result)
5. make sure the test fails
6. fix the two places
7. make sure tests pass
8. commit
9. make a pull request

[keithamus]

To be honest @qbolec we don't have any tests for ensuring that true is passed - so skip worrying about the tests, in this instance I'll accept the PR without any - so you can skip 2-5.

The change will basically be adding true to here and here. Feel free to do a manual tests with Mocha to make sure the diff you want comes out okay, and then make the PR!

If you'd also like to add an example to the docs, showing how one can use to.have.deep.members, that'd be awesome. The docs are right here.

[qbolec]

Hi,
changing the three places mentioned by you is obviously trivial.
But, I'd really want to do it right in terms of testing and the whole process.

One way to test it would be to add following lines here https://github.com/chaijs/chai/blob/master/test/assert.js#L848 :

try{
  assert.sameDeepMembers([ {b: 3} ], [ {c: 3} ]);  
}catch(err){
  chai.expect(err.showDiff).to.equal(true);
}

I'm not sure though, if this style of performing assertions is the correct one.
Perhaps I should use assert.isTrue(err.showDiff) or err.showDiff.should.equal(true) or given go as far as to modify the implementation of the err() function from test/bootstrap/karma.js and test/bootstrap/index.js so that it accepts a callback to be run against the exception, like this:

err(function(){
  assert.sameDeepMembers([ {b: 3} ], [ {c: 3} ]);
},'expected [ { b: 3 } ] to have the same members as [ { c: 3 } ]',function(err){
  chai.expect(err.showDiff).to.equal(true)   
})

By the way, why is the err function defined twice in karma.js and in index.js?

Also, I've noticed that in the lib/chai/interface/assert.js library there is sameMembers, sameDeepMembers, includeMembers , but there is no includeDeepMembers.
I could add it for symmetry. (I can do that in a separate commit/issue to avoid scope creep).

My biggest concern though is that I'm not sure if adding err.showDiff=true is good enough - if I understand the architecture correct, this is up to the reporter to generate the diff between err.expected and err.actual, and chai will not provide any hint as to how it should be generated.
Since the intuition that the arrays should be "sorted" before comparison can not be conveyed in the single bit of err.showDiff, I guess the only other option would be to sort err.actual and err.expected before throwing it. But this would violate the (unwritten?) contract, that err.actual actually means actual. Is there some way to hint the generator of the diff, that this particular error should use sorted array for diffing?

[lucasfcosta]

I've also ran into this problem when creating tests for #515. I was using a try/catch to test the err.showDiff property, I think it would be nice to have err() to accept a callback so we can test the error thrown.

Maybe something like this:

global.err = function (fn, msg, callback) {
  try {
    fn();
    throw new chai.AssertionError({ message: 'Expected an error' });
  } catch (err) {
    if ('string' === typeof msg) {
      chai.expect(err.message).to.equal(msg);
    } else {
      chai.expect(err.message).to.match(msg);
    }
    if ('function' === typeof callback) callback(err);
  }
};

[keithamus]

I actually forgot about #515. This issue and #515 overlap in a lot of ways (in the sense that #515 would invalidate any work done for this issue). We should probably settle on a good plan of action for what to do here.

@lucasfcosta if you have made some progress on #515 then I think we should get @qbolec to hold off on making any changes to this issue before you work is done.

@qbolec to address your comments:

My biggest concern though is that I'm not sure if adding err.showDiff=true is good enough - if I understand the architecture correct, this is up to the reporter to generate the diff between err.expected and err.actual, and chai will not provide any hint as to how it should be generated.

This is true. Chai can tweak the given expected and actual properties though - which is the most important hint as to what a reporter needs to diff on, and covers 99% of cases. Here should be no different.

Since the intuition that the arrays should be "sorted" before comparison can not be conveyed in the single bit of err.showDiff, I guess the only other option would be to sort err.actual and err.expected before throwing it. But this would violate the (unwritten?) contract, that err.actual actually means actual. Is there some way to hint the generator of the diff, that this particular error should use sorted array for diffing?

I feel like the contract is slightly more nuanced than that. err.actual is defined more like "the thing that we made the assertion on" - we're sorting the values before asserting on them so having err.actual sorted is probably more true to the contract. To better illustrate this - have a look at, for example, the typeof assertions - where err.actual === utils.type(this._obj) - the actual is not the original value passed in, but instead a "view" of that data.

Ultimately I'm slowly reaching the conclusion that Chai should probably have diffing capabilities baked in, and present reporters a standardised diff output format. I'm thinking this because right now there are so many small edge cases around existing diffs, and we get so many issues about poor diff reporting from test runners, that it seems like it might be a good idea to centralise diffs within Chai.

[lucasfcosta]

@keithamus Actually I've made just a little progress on #515. I've commented there about a doubt I've had.

Can I include into that change the addition of a callback in err() so I can test it (as I said before on this issue)?

Thanks for your help!

[keithamus]

@lucasfcosta absolutely! Do you what you need to do to get the tests working 😉

[IanVS]

@qbolec, I am pining for the missing includeDeepMembers, as you mentioned in #572 (comment). Do you still think you can add it in? I sure could use it!

[qbolec]

I'm a github noobie, so I'm not sure if my PR is done properly, but you can find the code above.

[qbolec]

It landed in #589

[keithamus]

🎉

[scottbessler]

looks like this was closed accidentally as I believe @qbolec meant that the includeDeepMembers landed in #589 and not that the fix to this landed.. I'll send a pr over shortly.