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

assert.equal doesn't give nice output if truncate threshold is reached #7

Closed
faassen opened this issue Mar 6, 2015 · 12 comments
Closed
Labels

Comments

@faassen
Copy link

faassen commented Mar 6, 2015

Consider this assertion:

        assert.equal(Immutable.Set([Immutable.Map({ id: "one", propName: "subref" })]),
                     Immutable.Set([1, 2]));

This fails with this ugly error:

AssertionError: expected { Object (size, _map, ...) } to equal Set { 1, 2 }

I debugged why I sometimes got this kind of error. It's because the length of the string representation of the actual value exceeds Chai's truncate threshold. Chai then proceeds to fall back to another method and displays the object in a truncated fashion.

I don't think it looks like Chai's objDisplay function can be easily taught about truncating Immutable.js's values. So I think it would be best to detect whether what we pass into actual (or expected) is an Immutable object, and if so, pass in the string instead of the object itself. It should then pass through without harm.

@faassen
Copy link
Author

faassen commented Mar 6, 2015

Hm, I looked into a fix along these lines:

       var collectionRepr = (collection instanceof Collection ?
                              collection.toString() :
                              collection);

and then passing CollectionRepr, obj.toString() as the last two arguments into this.assert.

This failed and debugging it I found out that obj instanceOf Collection does not always hold true; there are Immutable objects where this is false. How to replicate it yet I don't know, but I have a set here that gives true to Immutable.isSet() but false toobj instanceof Collection`...

@faassen
Copy link
Author

faassen commented Mar 6, 2015

Note that:

chai.config.truncateThreshold = 0;

is a workaround. It still tries to show me useless diffs on how the underlying objects differ, though.

@faassen
Copy link
Author

faassen commented Mar 6, 2015

But I suspect due to the Collection instance not being reliable I get an assertion failure where two immutable objects that should be the same are considered not to be...

@faassen
Copy link
Author

faassen commented Mar 6, 2015

In the end I decided to forgo chai entirely for this kind of testing and replace it with a simple extension to node's assert:

assert.is = function(actual, expected, message) {
    if (!Immutable.is(actual, expected)) {
        assert.fail(actual, expected, message, "is", assert.is);
    }
};

so far it's much more reliable.

You could consider something similar: add an 'is' which assumes immutable objects, instead of trying to override equal to mean something in addition to its original meaning.

@astorije
Copy link
Owner

astorije commented Mar 6, 2015

Waouh @faassen, that's a detailed bug report! :-)
Thanks for sharing, give me a few days to finish what's on my priority queue and I'll get back to this to come up with a solution!

@kmckee
Copy link

kmckee commented Nov 14, 2015

Yes this is the same issue I reported on 24. Not sure how I missed this issue.

Here's what I've observed:

        it('does not give helpful test output?', () => {
            const expected = Map({
                entries: List.of('Trainspotting')
            });
            const actual = Map({
                entries: List.of('28 Days Later')
            });
            expect(expected).to.eql(actual);
        });

Yields the following output:

AssertionError: expected { Object (size, _root, ...) } to equal { Object (size, _root, ...) }

If I convert my immutables to regular JS objects, like below, I get output like what I was expecting:

        it('has helpful output with JS objects', () => {
            const expected = Map({
                entries: List.of('Trainspotting')
            });
            const actual = Map({
                entries: List.of('28 Days Later')
            });
            expect(expected.toJS()).to.eql(actual.toJS());
        });

Output:

AssertionError: expected { entries: [ 'Trainspotting' ] } to deeply equal { entries: [ '28 Days Later' ] }
      + expected - actual

       {
         "entries": [
      -    "Trainspotting"
      +    "28 Days Later"
         ]
       }

@astorije
Copy link
Owner

@kmckee I'm working on a fix and it should land soon. Stay tuned.

@kmckee
Copy link

kmckee commented Nov 15, 2015

Awesome!

In the mean time, if anyone else is having the same problem, I've started to just call toJS() on my immutables when asserting with them, and then using POJOs for the expected values. I get nice output and I can easily remove them later with something like: grep -r expect.*toJS ./test

@astorije
Copy link
Owner

@kmckee, 08edf4a should fix this issue. Can you confirm that this commit and subsequent commits after that do work in your case?
I'll release a patch version when you can confirm.

I know it's been a while, but that would be awesome to also have @faassen give his feedback on this. Let me know if you have anything to add, and thanks again fore reporting this in the first place :-)
Sorry I dropped the ball on you for months!

@kmckee
Copy link

kmckee commented Nov 17, 2015

Confirmed. Thanks a ton!!

Error message displayed is:
AssertionError: expected 'Map { "entries": List [ "Trainspotting", "28 Days Later" ] }' to equal 'Map { "entries": List [ "Traiinspotting", "28 Days Later" ] }'

@rogeriopvl
Copy link

Great to see this fixed! 👍

When will this fix be published to npm?

@astorije
Copy link
Owner

Oops, sorry @rogeriopvl, I was so busy I guess I dropped the ball after @kmckee replied...

Anyway, done: chai-immutable@1.5.3 is now published!

Thanks all for your patience, your helpful reports and encouragement, especially @faassen whom I ended up ignoring for 8+ months!

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

No branches or pull requests

4 participants