toEqual reports detailed comparison of objects and arrays - take 2 #1163
Conversation
benchristel
force-pushed
the
eq-diffs
branch
2 times, most recently
from
fc82f87
to
b89305c
Compare
|
@slackersoft, @Gerg —it looks like the discussion on #675 never reached a consensus about whether to pretty-print the entire object if there's a deeply-nested mismatch. The code in this PR currently does not (in general) print the whole object. I considered the idea of adding the pretty-print but greatly limiting the length and depth to which the objects would be printed, by passing |
|
I know "plus one-ing" is not the most helpful comment to a conversation, but getting detailed comparisons to expected data would really be helpful. This is a real pain point when working with large objects, sometimes deeply nested. So +1. ;-) |
|
@benchristel even if there is no concensus I would argue that anything is better than the current state. Manually comparing large objects is a real pain right now. I am eagerly looking forward to this feature. |
|
Any update on this? Sorely needed when comparing large objects. |
|
Any update on this? |
|
How can we beta test this before it's released ? Tried to npm install this as the jasmine-core in my local jasmine installation, but doesn't appear to work as I still see the old style diff. Also, will this also work for strings ? |
benchristel
force-pushed
the
eq-diffs
branch
2 times, most recently
from
fc82f87
to
295551c
Compare
|
@surdu sorry, I hadn't run Currently, it does not do anything special for strings. I don't think it would be too hard to add string-diffing, but maybe it should wait for a future PR. |
|
|
||
| expect(compareEquals(actual, expected).message).toEqual(message); | ||
| }); | ||
|
|
There was a problem hiding this comment.
There are a bunch of xitd specs below here. We should either implement them or pull them out as not relevant
There was a problem hiding this comment.
The xitd specs have been implemented. The one for custom equality checkers turned into an integration test.
| return false; | ||
| } | ||
| var iterA = a.values(), iterB = b.values(); | ||
| var valA, valB; | ||
| do { | ||
| valA = iterA.next(); | ||
| valB = iterB.next(); | ||
| if (!eq(valA.value, valB.value, aStack, bStack, customTesters)) { | ||
| if (!eq(valA.value, valB.value, aStack, bStack, customTesters, j$.NullDiffBuilder())) { |
There was a problem hiding this comment.
Shouldn't this be the diffBuilder local and not grabbing a new NullDiffBuilder?
There was a problem hiding this comment.
Passing in the NullDiffBuilder will cause both Sets to be pretty-printed in the failure message of their containing object. Basically, I was uncertain about how to display set diffs, so I just punted on this one.
Ideally, I'd like something similar to the diff that's displayed for objects when a and b have different sets of keys. However, the way set equality is implemented right now, the insertion order of items in the set matters. Given that that's the case, we could treat sets like arrays. But that, too, presents problems, because then you'd get output like
Expected $[0] to equal 'bar', but got 'baz'
when the objects in question are Sets, not Arrays. This output seems a bit misleading since set items can't actually be accessed by index.
benchristel
force-pushed
the
eq-diffs
branch
3 times, most recently
from
d452d0f
to
9600e2c
Compare
- Mismatches deep within object/array structures are pinpointed with a JsonPath-like syntax.
benchristel
force-pushed
the
eq-diffs
branch
from
9600e2c
to
d5e6bf4
Compare
- Merges #1163 from @benchristel - Fixes #675
|
Merged! 🎉 So, did it finally also included big strings in the diff or only big objects ? |
|
@benchristel It looks like this actually has some failing tests in the various IE browsers. If you, or someone else, have some time to look into the failures (https://travis-ci.org/jasmine/jasmine/builds/176577479) we need to get these fixed. |
This implements #675. It addresses @slackersoft's comments on #1042, which was my first attempt at implementing this feature.
EDIT: This is now "done" and ready for review.
I'm opening a PR for it because I'd love to get some intermediate feedback on the approach. I keep discovering edge cases that seem to require refactoring withineqto enable adding the diff functionality, and want to make sure I'm not going down the wrong path or inadvertently changing behavior. Also, any feedback about the preferred format for these error messages would be immensely helpful at this stage.Thanks in advance for any feedback you have time to provide. I'll keep working on this in the meantime since I'm between projects at Pivotal Labs for the next week.