Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add object diff #589

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

curvedmark commented Sep 25, 2012

I changed when to display invisible chars so object diffs won't be too crazy.

I'm also wondering should character diff always be used? Since we are testing js values, and whitespaces are almost always significant in this case, but I'm not sure, what do you think?

@curvedmark curvedmark referenced this pull request in NV/jsDump Sep 26, 2012

Open

Add an option to sort object keys #7

Contributor

tj commented Sep 26, 2012

well for example if you're doing css.should.equal(expected) for a preprocessor you're pretty screwed if you dont have a nice diff. what is jsDump?

Contributor

curvedmark commented Sep 27, 2012

You are right, for stylus unit tests, char diff is probably too fine-grained. Then maybe we can introduce a mocha option for that? Values like 'auto'(current behavior), 'char', 'word' and 'line' can determine how diff works.

jsDump is basically a more tolerant version of JSON.stringify(), so it won't complain when you throw undefined or objects containing methods at it.

QUnit also uses it to display diffs.

Contributor

curvedmark commented Sep 27, 2012

You would also want to accept LearnBoost/expect.js#34 if you accept this request. That pull request makes expect.js also display diffs.

Contributor

tj commented Sep 27, 2012

the problem with doing diffs this way is that we're obscuring the real error message, and just showing a diff, also not sure about jsDump, might have to think on that a bit im not sold

Contributor

curvedmark commented Sep 27, 2012

what do you mean by "real error message"? when two objects are different, isn't showing their diff the real error message?

Contributor

tj commented Sep 27, 2012

well for example if you had res.should.have.header('Content-Type', '...') it would give you "expected { ... } to have a Content-Type of ...' that sort of thing, but that would be crushed by the diff

Contributor

curvedmark commented Sep 27, 2012

assert libs shouldn't assign actual & expected props to the error object for these kinds of asserts. when they do this, it should mean that they want mocha to display a diff, which might not be suitable for all kinds of asserts.

speaking of which, I guess my commit for expect.js should leave .equal() along, which shouldn't display a diff.

Contributor

tj commented Sep 27, 2012

well those props aren't even specifically for mocha, they just hold the expected/actual values so you can choose to do other things with them. going mocha-specific wouldn't really be too great. Ideally I guess we have alternative messages or something, bit of a tough thing abstraction-wise I guess

Contributor

curvedmark commented Sep 27, 2012

then how about adding another prop to the error object like showDiff? so assert libs can specify if it's is ok to overridden the message.

Contributor

tj commented Sep 27, 2012

yeah that might be a reasonable alternative, sounds good to me for now at least!

Contributor

curvedmark commented Sep 27, 2012

cool. will send a new commit asap.

Contributor

curvedmark commented Sep 27, 2012

done, including the expect.js pulll request.

Contributor

curvedmark commented Sep 27, 2012

do you think a mocha option for diff methods still stands? not sure how to access mocha options in the base reporter. if you could shed some light, I'd like to take a stab at it.

Contributor

tj commented Sep 30, 2012

I'll add a prop like that to should.js as well and see how it goes

Contributor

travisjeffery commented Dec 7, 2013

this pr's pretty out of date. feel free to reopen if you want this and have made your changes up to date

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