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

deepEqual doesn't always show an object diff #469

Closed
lambdabaa opened this issue Jun 14, 2015 · 20 comments
Closed

deepEqual doesn't always show an object diff #469

lambdabaa opened this issue Jun 14, 2015 · 20 comments

Comments

@lambdabaa
Copy link

I have seen lots of things like AssertionError: expected [ Array(2) ] to deeply equal [ Array(2) ] when the objects being compared are very similar and a diff would be useful. I think I have seen chai do this often in the past but not at 3.0.0.

@csnover
Copy link
Contributor

csnover commented Jun 15, 2015

Chai provides actual and expected information as part of the thrown assertion error, it is up to the test system you are using to display an object diff. Intern for example provides an accurate object diff on assertion failure against objects.

@lambdabaa
Copy link
Author

So if I do something like

try {
  require('chai').expect({ x: { y: 0 } }).to.deep.equal({ x: { y: 1 } });
} catch (error) {
  // then error has diff somewhere?
}

@lambdabaa
Copy link
Author

This is what I see

{ [AssertionError: expected { x: { y: 0 } } to deeply equal { x: { y: 1 } }]
  message: 'expected { x: { y: 0 } } to deeply equal { x: { y: 1 } }',
  showDiff: true,
  actual: { x: { y: 0 } },
  expected: { x: { y: 1 } } }

I think it would be useful for the regular Error message to contain the diff

@keithamus
Copy link
Member

Hey @gaye thanks for the issue.

As @csnover mentioned, Chai's AssertionError has actual and expected properties, which test runners like Mocha can (and do) use. If you're not seeing diffs, it's likely a problem with the test runner (e.g. Mocha) as we always throw AssertionErrors with the properties set.

What test runner are you using?

What do you mean about "regular Error"? We don't throw any Errors, only AssertionErrors.

@lambdabaa
Copy link
Author

Right so I guess what I am asking for is a feature where test runners like mocha wouldn't have to know specific information about your assertions/expectations library in order to show useful output. Ideally, mocha just shows the error message and doesn't know anything specific about the errors that chai is throwing.

@csnover
Copy link
Contributor

csnover commented Jun 16, 2015

@gaye Unfortunately different test systems output to different formats so would have different ways of representing that diff. ANSI? HTML?
? \n? It’s not possible for the assertion library to know.

@lambdabaa
Copy link
Author

Hrm that's a very good point.

@keithamus
Copy link
Member

@gaye Mocha (among others) actually already knows specific information about our error messages, and has for as long as I can remember. Here's Mocha's base reporter, picking up err.actual and err.expected.

By the way, the .actual and .expected aren't conventions specific to Chai, they're in Node proper, for example:

> var assert = require('assert');
> var err; try { assert.ok(false) } catch(e) { err = e }
{ [AssertionError: false == true]
  name: 'AssertionError',
  actual: false,
  expected: true,
  operator: '==',
  message: 'false == true',
  generatedMessage: true }

Your ideal of having Mocha's responsibility of just showing an error message is not the current situation, in fact it is the opposite. Chai has no diffing engine, chai does not do diffs. It creates an AssertionError which has the standard .message, .stack, .expected, and .actual properties - and then throws that error. Mocha (and others) do the heavy lifting there end, by showing diff output, stack traces etc.

This is precisely how it should be - as @csnover explained. Chai is sending the raw info (the values) to the test harness (e.g. Mocha), and the test harness does things that best suit its environment. This might be outputting XML, or fancy diffs to console, or it might be something entirely new. The point being, this output is the responsibility of the Reporter.

@keithamus
Copy link
Member

To go back to your original issue @gaye - I think there are two subtly different problems here.

One is to do with diffing - we've already solved this, it is in the test harness domain, so if you are not seeing useful diffs, I'd kindly ask you raise the issue with your test harness, for example Mocha.

There is another part of the puzzle though, and that's chai's inspection engine. The message which says 'AssertionError: expected [ Array(2) ] to deeply equal [ Array(2) ]' is from Chai, and we use our own inspection library (found here) to create these.

I think here we have a lot of room to make things better. [ Array(2) ] isn't that useful, [ [1, 2] ] would be more useful I'm guessing - but we need to strike a good balance between too much info and too little.

Can I suggest we move the topic of conversation on how to deal with these small arrays and objects within messages? Here's the code for formatting Arrays - we could place in some length checks and try to output the full contents of short Arrays, or truncate them in the middle and only show [0..2, n-2..n].

@oveddan
Copy link

oveddan commented Oct 7, 2015

It would be great if this detailed diff showed up in karma!

@fabiosussetto
Copy link

This currently works for me (using Karma as the test runner):

chai.config.truncateThreshold = 0

Without it I only see "expected [ Array(1) ] to deeply equal [ Array(1) ]"

@oveddan
Copy link

oveddan commented Oct 20, 2015

The issue lies in the test runner itself

@keithamus
Copy link
Member

Yes, @oveddan is right, diffs should be dealt with by the test runner, mocha's html reporter (and karma-mocha) do not support diffs. Please support this issue: mochajs/mocha#1348.

As this issue isn't going toward the direction I hoped, I'll close this now - diffs are Mocha's problem, not ours. However, I've opened an issue in loupe (our object inspection tool) about getting better quality error messages, which is over in chaijs/loupe#1. If you care about better quality error messages (and not diffs), please add your input there.

Summary:

Chai does not produce diffs, it is the test reporters responsibility.

Mocha's lack of diffing in some reporters is Mocha's issue.

Chai's error messages could be better, we have another issue for that now.

@capaj
Copy link

capaj commented Nov 4, 2015

@fabiosussetto thanks, saved me a lot of trouble today. Why the hell isn't this a default value?

@keithamus
Copy link
Member

@capaj because it's not actually a diff. It's just outputting the whole objects inside of an Error message, which is very messy. Test reporters need to support diffs to solve this problem properly.

@capaj
Copy link

capaj commented Nov 4, 2015

@keithamus ok, that is all great, but is there any reason not to have this value as 0 by default? Because I see one big reason for it to be 0 by default.

@keithamus
Copy link
Member

@capaj yes, there are good reasons for having it to not be 0 by default. The error message is meant to be just that: an error message. If you're using it to eyeball diffs between two objects, thats not a desirable solution for you and for those who have reporters that support diffs, they get information overload.

Any sufficiently large amount of data, let's say an array buffer or even a complex object will completely blow up error messages to grotesque proportions. It's not the information you want, it's just the information you think you want.

Here's a contrived example:

> chai.should(); chai.config.truncateThreshold = 0;
0
> chai.expect(crypto.randomBytes(40).toJSON()).to.deep.equal(crypto.randomBytes(40).toJSON());
AssertionError: expected { type: 'Buffer',
  data:
   [ 212,
     8,
     156,
     165,
     190,
     67,
     27,
     15,
     196,
     115,
     177,
     237,
     224,
     183,
     139,
     205,
     166,
     178,
     61,
     167,
     185,
     182,
     87,
     145,
     151,
     82,
     213,
     36,
     42,
     154,
     0,
     224,
     212,
     143,
     42,
     185,
     249,
     227,
     166,
     80 ] } to deeply equal { type: 'Buffer',
  data:
   [ 103,
     232,
     65,
     234,
     108,
     128,
     49,
     30,
     195,
     42,
     245,
     132,
     231,
     73,
     91,
     172,
     234,
     131,
     165,
     121,
     96,
     238,
     37,
     191,
     77,
     224,
     184,
     4,
     246,
     191,
     127,
     135,
     154,
     108,
     165,
     213,
     67,
     109,
     21,
     197 ] }
    at Assertion.assertEqual (/Users/keith/Projects/chai/chai/lib/chai/core/assertions.js:468:19)
    at Assertion.ctx.(anonymous function) [as equal] (/Users/keith/Projects/chai/chai/lib/chai/utils/addMethod.js:40:25)
    at repl:1:54
    at REPLServer.defaultEval (repl.js:164:27)
    at bound (domain.js:250:14)
    at REPLServer.runBound [as eval] (domain.js:263:12)
    at REPLServer.<anonymous> (repl.js:393:12)
    at emitOne (events.js:82:20)
    at REPLServer.emit (events.js:169:7)
    at REPLServer.Interface._onLine (readline.js:210:10)
>

That is not really helpful to anyone. Instead of being concerned about the truncation of naive output, let's focus on getting more intelligent output. Please feedback on chaijs/loupe#1, come up with ideas and continue the discussion there.

@keithamus
Copy link
Member

Also, I'm going to reiterate this point again:

Chai does not do diffs. If you're not seeing diffs, then that is an issue with your test runner (e.g. mocha). You should totally file an issue with your test runner about this, here is one for mocha's html reporter: mochajs/mocha#1348

@fluggo
Copy link

fluggo commented Mar 23, 2016

I came across this issue because I was running into the same problem. There's an important thing you need to check for: did the error report happen outside of your tests?

That was my problem: I had an expect() statement sitting inside a describe() block, when it should have been sitting inside an it() block. That makes all the difference to mocha.

@advance512
Copy link

I had a similar issue - it ended up being an two different ObjectId (from mongoose) objects with the same identifier.

JSON.parse(JSON.stringify(obj)) solved the issue. Hackish but it worked for me for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants