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

toEqual should list differences instead of pretty printing objects #675

Closed
mwisnicki opened this Issue Sep 18, 2014 · 40 comments

Comments

Projects
None yet
@mwisnicki
Copy link

mwisnicki commented Sep 18, 2014

Consider this simple example (jsfiddle):

var actual = { x: 1, y: { z: 2 } };
var expected = { x: 1, y: { z: 3 }};
expect(actual).toEqual(expected);

This outputs:
Expected { x : 1, y : { z : 2 } } to equal { x : 1, y : { z : 3 } }.

In other words it pretty prints both objects.

Now imagine we are in real world where objects are quite complex and toEqual quickly becomes useless.

I would expect output like:

Expected no differences but found 1:
1. Expected y.z = 2 to equal 3

i.e. list all differences with paths.

@slackersoft

This comment has been minimized.

Copy link
Member

slackersoft commented Sep 18, 2014

We have a story logged in tracker for this (https://www.pivotaltracker.com/story/show/1283792), but I don't think we actually want to remove the full print, for something like this. Probably just add some additional info for missing and extra elements.

We'd be happy to look at a pull request that solves this issue.

@mwisnicki

This comment has been minimized.

Copy link
Author

mwisnicki commented Sep 18, 2014

Showing nontrivial objects absolutely kills usefulness of ouput. At least limit it to single line or maybe don't show the object at all if it is longer than a line.

I'd like to work on this since I need it, though I don't know if I can migrate my project to 2.0 and without that I'll probably have to resort to writing some helper function in my own code since old matchers seemed to only return booleans.

BTW Is it possible to return something more sophisticated than string or is result.message = "..." the only way to override message ?

@slackersoft

This comment has been minimized.

Copy link
Member

slackersoft commented Sep 19, 2014

Because this output can be output either on the command line in various different contexts or in an html page, result.message can either be a string or a function that returns a string.

A fix for this probably also fixes #668

@Edward-Lombe

This comment has been minimized.

Copy link

Edward-Lombe commented Nov 5, 2014

I wrote something similar to this, only it doesn't print the path of the keys through the object, only the values in which it encounters a difference. I was working with parsed JSON objects and having a failed test take up two or three terminal windows just wasn't helpful.

Anyway here is a gist that you can drop into you /helpers/ folder which will do some of what you are saying. Some tests are included but they are far from comprehensive.

Edit: I updated my gist slightly to add path support, although it isn't really tested.

@mgol

This comment has been minimized.

Copy link

mgol commented Jan 22, 2015

This is what chai prints:

      AssertionError: expected { Object (id, issued_at, ...) } to deeply equal { Object (id, issued_at, ...) }
      + expected - actual

               "featuredCollectionsCount": 0
             }
             "avatar": {
               "origin": "gravatar"
      +        "url": "about:blankk"
      -        "url": "about:blank"
             }
             "email": "foo.bar@example.com"
             "fullname": "Foo Bar"
             "id": 42

So nice, I can immediately see what's wrong. It even has colors!
screen shot 2015-01-23 at 00 13 43

In large objects like that Jasmine output is basically useless, it's completely unreadable. I have to copy the output to a separate file, hunt for "equal to" and split the string by that, copy both parts to two files and compare using my own eyes.

@slackersoft

This comment has been minimized.

Copy link
Member

slackersoft commented Feb 11, 2015

We'd be happy to review a pull request for this that can work both in the HtmlReporter and ConsoleReporter (probably the one in jasmine-npm since core's is now deprecated).

@mgol

This comment has been minimized.

Copy link

mgol commented Apr 20, 2015

@slackersoft Mocha uses the diff package to display these readable diffs. Would you accept a PR that would use this package to display object differences in Jasmine?

@slackersoft

This comment has been minimized.

Copy link
Member

slackersoft commented Apr 20, 2015

We'd like to keep the core of jasmine with no dependencies. Aside from just not wanting to install a bunch of dependencies on your system just so you can test your javascript, this also means we can release a standalone version that doesn't need much setup, and the platform specific versions (ruby, python, npm) don't need to figure out how to get the same dependencies in each environment.

@mgol

This comment has been minimized.

Copy link

mgol commented Apr 21, 2015

@slackersoft

Aside from just not wanting to install a bunch of dependencies on your system just so you can test your javascript

The published diff package contains just 3 files: README.md, package.json & diff.js. This is not bloating anything. OTH, jasmine-core package contains a lot of files that are not necessary to run Jasmine, like GOALS.md, the images/ directory etc. so it shows it's not only about size.

this also means we can release a standalone version that doesn't need much setup

Don't you already have a build process? You don't keep everything in one file after all.

If you don't want to include diff as an explicit npm dependency, another option would be do declare it as a dev dependency and treat it in the build process as every other source file that you're currently merging anyway.

Yet another option would be to copy diff.js inside the Jasmine repository; the drawback is that it's easy to diverge from the source then and miss its fixes etc. Also, it gives smaller credit to the original author.

What do you think? I could try something from the proposals above if you agree. I don't know what's the status of the Ruby version but I don't know Ruby so I can only offer a PR for the JS version.

@slackersoft

This comment has been minimized.

Copy link
Member

slackersoft commented May 7, 2015

The concern from jasmine's perspective is around dependencies, not necessarily the size of the tarball that is downloaded for install. That said, the images folder is actually necessary for us to create a jasmine branded specRunner.html file in case a project using npm for package management wants to run browser based jasmine tests.

The dependency constraint hits us especially hard, considering the various different platforms that the jasmine javascript is released for (npm, ruby gem, python egg, standalone), which means that we can't just depend on a package to directly provide us other functionality.

This effectively leaves either the dev dependency route or inlining the diff.js file directly into jasmine iteself if we want to use the diff package. Inlining is obviously bad for updates, but both would actually require us to distribute the diff code and not just have it as a dependency, which isn't something I think we really want to do.

On top of all of this, the diff package wouldn't solve some of the more interesting cases like objectContaining or arrayContaining, so I think what we really want here is a more custom solution that supports the asymmetric matchers as well.

@mgol

This comment has been minimized.

Copy link

mgol commented May 8, 2015

Would it be ok to start from the code from the diff package & iterate from
there?

Michał Gołębiowski

@antoineverger

This comment has been minimized.

Copy link

antoineverger commented May 27, 2015

Having that feature would be really great. Is there any progress on a PR ?

@jchoca

This comment has been minimized.

Copy link

jchoca commented May 27, 2015

I think it would be a nice feature too. What would a dependency free solution look like?

@andymaster01

This comment has been minimized.

Copy link

andymaster01 commented Jul 27, 2015

If you're using karma to run your tests, you can use this one:
https://www.npmjs.com/package/karma-jasmine-diff-reporter

@mik01aj

This comment has been minimized.

Copy link

mik01aj commented Aug 3, 2015

I managed to have a highlighted diff in Jasmine (running in Node) by installing jsondiffpatch and by adding this in beforeEach:

        var jsondiffpatch = require('jsondiffpatch');
        jasmine.addMatchers({
            toEqualJSON: function(util, customEqualityTesters) {
                return {
                    compare: function(actual, expected) {
                        var result = {};
                        actual = JSON.parse(JSON.stringify(actual));
                        expected = JSON.parse(JSON.stringify(expected));
                        result.pass = util.equals(actual, expected, customEqualityTesters);
                        result.name = 'JSON objects ' + (result.pass ? '' :  'don\'t') + ' match';
                        if (result.pass) {
                            result.message = 'OMG Big Equal!';
                        } else {
                            result.message = '' + jsondiffpatch.formatters.console.format(jsondiffpatch.diff(expected, actual));
                        }
                        return result;
                    },
                };
            }
        });

The only thing I don't like about it is that, in case of an error, the diff gets displayed twice (once as message and once in stack).

Of course having this behaviour built-in in Jasmine, as @mzgol suggests would be way better. +1 for the issue :)

@startswithaj

This comment has been minimized.

Copy link

startswithaj commented Oct 20, 2015

+1

1 similar comment
@bcherny

This comment has been minimized.

Copy link

bcherny commented Dec 23, 2015

👍

@josketres

This comment has been minimized.

Copy link

josketres commented Dec 31, 2015

+1 this behaviour is very useful.
I tried to use unexpected.js for this, but the combination: unexpected + jasmine + karma + PhantomJS is still not stable (I keep getting errors).
It would be nice if it's built-in in jasmine.

@gitnik

This comment has been minimized.

Copy link

gitnik commented Jan 10, 2016

+1 for this.

cpetrov added a commit to eclipsesource/tabris-con that referenced this issue Jan 17, 2016

Use Mocha + Chai instead of Jasmine
Mocha outputs more helpful diffs when comparing objects, which is
particularly useful when testing behavior. See [1].

It is also better compatible with supertest [2], a module which can be
used to test a demo service for the feedback functionality, if one is
developed.

[1]: jasmine/jasmine#675
[2]: jasmine/jasmine-npm#31

benchristel pushed a commit to benchristel/jasmine that referenced this issue Feb 5, 2016

@jsdir

This comment has been minimized.

Copy link

jsdir commented Feb 22, 2016

👍

@vamsivarikuti

This comment has been minimized.

Copy link

vamsivarikuti commented Jun 27, 2016

For reference: ExUnit handles like this. Showing the diff is very useful when comparing large JSON structures

@benchristel

This comment has been minimized.

Copy link
Contributor

benchristel commented Jul 20, 2016

I've resumed work on this, at master...benchristel:eq-diffs

@brokentone

This comment has been minimized.

Copy link

brokentone commented Oct 21, 2016

We unfortunatly recently switched from Jasmine to Mocha (+Chai) for this functionality

@donald-s

This comment has been minimized.

Copy link

donald-s commented Oct 22, 2016

@benchristel I see that you've put quite a bit of work into the diff functionality. Are you still planning to work on it? I'd be happy to pitch in or pick up where you left off.

@benchristel

This comment has been minimized.

Copy link
Contributor

benchristel commented Oct 23, 2016

@donald-s the bulk of the functionality should be there already--at this point the bottleneck is getting the PR reviewed and merged. Unfortunately it doesn't seem like the maintainers have had much time to dedicate to Jasmine recently (and of course it doesn't help that the PR is enormous!)

If you're daring and want to beta-test the diff functionality, I'm sure that would be helpful. Real usage might uncover some issues that I didn't think to write tests for.

@donald-s

This comment has been minimized.

Copy link

donald-s commented Nov 21, 2016

@benchristel I see. Looks like it was just merged. I'm eager to try it out. Thanks for your work on this!

@giamir

This comment has been minimized.

Copy link

giamir commented Nov 29, 2016

@slackersoft It would be nice to know when the work done by @benchristel will be released.
This story has a 3.0 label in your pivotal tracker; does it mean we will have to wait a Jasmine 3.0 release? If that's the case any chance to have a 3.0 prerelease published in npm?
Thanks!

@nicbou

This comment has been minimized.

Copy link

nicbou commented Jan 18, 2017

Any update on this? I too would like to know when we can get our hands on this feature.

@nemoDreamer

This comment has been minimized.

Copy link

nemoDreamer commented Feb 21, 2017

@slackersoft what's the target release for this? I'm looking to migrate our test suites to mocha+chai, because our deep comparisons are just too hard to debug in Jasmine at the moment... 😢

@nemoDreamer

This comment has been minimized.

Copy link

nemoDreamer commented Feb 21, 2017

Btw, @slackersoft @mgol : Mocha solved the "too many dependencies on initial setup" problem by making this optional.
If you set mochaReporter: { showDiff: true } in Karma's conf, that's when you need to add the diff dependency to your package.json.
You'd really want to avoid re-inventing the wheel and copying something over into jasmine-core that's already been solved, no?

Note: I missed @andymaster01's comment about karma-jasmine-diff-reporter, so I'll give that a spin now.

@matthewadams

This comment has been minimized.

Copy link

matthewadams commented Apr 12, 2017

Is this still a no-go? As of this note, the issue still has the ready for work label, but the issue is closed. What's the status?

@benchristel

This comment has been minimized.

Copy link
Contributor

benchristel commented Apr 12, 2017

@matthewadams there was a PR for this feature that was merged a while ago.

Here's the merge commit: 54e7a82

I'm not sure what the timeline for release is.

@matthewadams

This comment has been minimized.

Copy link

matthewadams commented Apr 12, 2017

@slackersoft or @jasmine Can y'all give info on expected fix version that will contain 54e7a82 ? That'd be just jim-dandy! :)

@slackersoft

This comment has been minimized.

Copy link
Member

slackersoft commented Apr 12, 2017

It will be in the next release. I don't know when that will be right now.

@benjaminapetersen

This comment has been minimized.

Copy link

benjaminapetersen commented Sep 15, 2017

Check in on this! The issue is closed, but I don't believe it has been released? Feeling the pain and looking for something like what @mgol referenced in the comment above.

@benchristel

This comment has been minimized.

Copy link
Contributor

benchristel commented Sep 15, 2017

I see the code on the v2.8.0 tag, so I think it's out? What version do you have @benjaminapetersen ?

@benjaminapetersen

This comment has been minimized.

Copy link

benjaminapetersen commented Sep 15, 2017

I'm on jasmine-core v 2.8.0. I see this kind of output:

Expected $[0] to have properties
    candy: 'bars'
@test/spec/services/membership/membershipSpec.js:284:7

.... is this correct? If so, I'm current. However, I was expecting more like what was shown in the above comments this or this.

@benchristel

This comment has been minimized.

Copy link
Contributor

benchristel commented Sep 15, 2017

@benjaminapetersen yep, you're up-to-date. There were a couple different proposals for formatting the output and I went (more or less) with the one in the OP.

@benjaminapetersen

This comment has been minimized.

Copy link

benjaminapetersen commented Sep 16, 2017

ok thanks!

@ohjames

This comment has been minimized.

Copy link

ohjames commented Aug 8, 2018

Working with a team now that replaced jasmine with mocha because they couldn't stand jasmine's diff output or find a way to update it to be more like mocha's. I think mocha's "pretty printed diff" output is so much easier to parse.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment