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

Show diff only when it helps #802

Conversation

shivamdixit
Copy link
Contributor

Follow up of #786

shivamdixit and others added 2 commits March 27, 2015 14:59
Don't show diff if expected and actual values are completely different
and there is nothing in common

Fixes qunitjs#335
@leobalter
Copy link
Member

Note: this is a follow up to #786 and also address an issue introduced on the last changes showing a whacky falstrue diff when one of the assertion values (actual or expected) was a Boolean.

@shivamdixit
Copy link
Contributor Author

Seems like the test timed out.

@leobalter
Copy link
Member

it worked now

@shivamdixit
Copy link
Contributor Author

qunit7

Booleans inside objects/arrays are still displayed like falstrue.

@shivamdixit
Copy link
Contributor Author

Ping.

@jzaefferer
Copy link
Member

The hack to show true/false differently is bad. This is likely to blow up with a similar combination of single word pairs, or maybe with numbers.

We want to character-by-character diff for syntax, but maybe not for literals like booleans?

Since we already landed the new diff, can we keep the "show diff only when it helps" and "show better diff for booleans" apart?

@leobalter
Copy link
Member

the regexp works only for Booleans, as strings are represented between double quotes and the regexp will search from the start to the end of the value: /^(true|false)$/.

@leobalter
Copy link
Member

btw, falstrue is a good name for a band.

@leobalter
Copy link
Member

as told on the biweekly meeting, the boolean regexp check is good as-is and @jzaefferer agreed to land it.

@shivamdixit
Copy link
Contributor Author

Great. So this PR is ready to be merged?

@leobalter
Copy link
Member

LGTM, I'll just wait for @jzaefferer 👍 here to confirm. Then I'll squash the commits and merge them.

expected.indexOf( "[object Object]" ) !== -1 ) {
message += "<tr class='test-message'><th>Message: </th><td>" +
"Diff suppressed as the depth of object is more than current max depth (" +
QUnit.config.maxDepth + ").<p>Hint: Use <code>QUnit.dump.maxDepth</code> to " +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's fix the <code>QUnit.dump.maxDepth</code> to use QUnit.config.maxDepth as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tried to change it to QUnit.config.maxDepth but strangely it din't work, whereas using QUnit.dump.maxDepth works fine. I guess this is happening because dump.maxDepth is initialized with default config.maxDepth in file dump.js and then the value of config.maxDepth is changed. Hence I got a message which looks like:

Diff suppressed as the depth of object is more than current max depth (infinity)

However specifying maxDepth in GET parameters works fine because if parameters are present, the value of config.maxDepth is updated in file core.js before initialization in dump.js.

@jzaefferer
Copy link
Member

Noticed two things, not sure why I missed them before.

@jzaefferer
Copy link
Member

Squashed the two commits into one and merged. Thanks again!

@leobalter leobalter deleted the shivamdixit-335-show-diff-only-when-it-helps branch May 15, 2015 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants