Show diff only when it helps #802

Closed
wants to merge 2 commits into
from

Conversation

4 participants
@shivamdixit
Contributor

shivamdixit commented Mar 27, 2015

Follow up of #786

shivamdixit and others added some commits Mar 17, 2015

HTML Reporter: Show diff only when it helps
Don't show diff if expected and actual values are completely different
and there is nothing in common

Fixes #335
@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Mar 27, 2015

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.

Member

leobalter commented Mar 27, 2015

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

This comment has been minimized.

Show comment
Hide comment
@shivamdixit

shivamdixit Mar 27, 2015

Contributor

Seems like the test timed out.

Contributor

shivamdixit commented Mar 27, 2015

Seems like the test timed out.

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Mar 27, 2015

Member

it worked now

Member

leobalter commented Mar 27, 2015

it worked now

@shivamdixit

This comment has been minimized.

Show comment
Hide comment
@shivamdixit

shivamdixit Mar 28, 2015

Contributor

qunit7

Booleans inside objects/arrays are still displayed like falstrue.

Contributor

shivamdixit commented Mar 28, 2015

qunit7

Booleans inside objects/arrays are still displayed like falstrue.

@shivamdixit

This comment has been minimized.

Show comment
Hide comment
@shivamdixit

shivamdixit Apr 2, 2015

Contributor

Ping.

Contributor

shivamdixit commented Apr 2, 2015

Ping.

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Apr 9, 2015

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?

Member

jzaefferer commented Apr 9, 2015

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

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Apr 16, 2015

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)$/.

Member

leobalter commented Apr 16, 2015

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

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Apr 16, 2015

Member

btw, falstrue is a good name for a band.

Member

leobalter commented Apr 16, 2015

btw, falstrue is a good name for a band.

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Apr 22, 2015

Member

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

Member

leobalter commented Apr 22, 2015

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

@shivamdixit

This comment has been minimized.

Show comment
Hide comment
@shivamdixit

shivamdixit Apr 22, 2015

Contributor

Great. So this PR is ready to be merged?

Contributor

shivamdixit commented Apr 22, 2015

Great. So this PR is ready to be merged?

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Apr 22, 2015

Member

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

Member

leobalter commented Apr 22, 2015

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 " +

This comment has been minimized.

@jzaefferer

jzaefferer Apr 22, 2015

Member

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

@jzaefferer

jzaefferer Apr 22, 2015

Member

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

This comment has been minimized.

@shivamdixit

shivamdixit Apr 24, 2015

Contributor

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.

@shivamdixit

shivamdixit Apr 24, 2015

Contributor

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.

+ if ( !( /^(true|false)$/.test( actual ) ) &&
+ !( /^(true|false)$/.test( expected ) ) ) {
+ diff = QUnit.diff( expected, actual );
+ showDiff = stripHtml( diff ).length !==

This comment has been minimized.

@jzaefferer

jzaefferer Apr 22, 2015

Member

This will be false when the diff is exactly as long as actual length plus expected length, right? Is that really a good measure?

@jzaefferer

jzaefferer Apr 22, 2015

Member

This will be false when the diff is exactly as long as actual length plus expected length, right? Is that really a good measure?

This comment has been minimized.

@leobalter

leobalter Apr 22, 2015

Member
QUnit.diff( "1234567890", "abcdefghij" );
"<del>1234567890</del><ins>abcdefghij</ins>"

AFAIK, this happens when there's no diff, the objects are completely different.

@leobalter

leobalter Apr 22, 2015

Member
QUnit.diff( "1234567890", "abcdefghij" );
"<del>1234567890</del><ins>abcdefghij</ins>"

AFAIK, this happens when there's no diff, the objects are completely different.

This comment has been minimized.

@leobalter

leobalter Apr 22, 2015

Member

it makes sense when we already expose the actual and expected and there's nothing equal from them.

@leobalter

leobalter Apr 22, 2015

Member

it makes sense when we already expose the actual and expected and there's nothing equal from them.

This comment has been minimized.

@jzaefferer

jzaefferer Apr 24, 2015

Member

Okay, we can tune it later.

@jzaefferer

jzaefferer Apr 24, 2015

Member

Okay, we can tune it later.

This comment has been minimized.

@shivamdixit

shivamdixit Apr 24, 2015

Contributor

I agree with @leobalter , if objects are completely different then only the condition will be false.

@shivamdixit

shivamdixit Apr 24, 2015

Contributor

I agree with @leobalter , if objects are completely different then only the condition will be false.

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Apr 22, 2015

Member

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

Member

jzaefferer commented Apr 22, 2015

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

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer May 15, 2015

Member

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

Member

jzaefferer commented May 15, 2015

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

@leobalter leobalter deleted the leobalter:shivamdixit-335-show-diff-only-when-it-helps branch May 15, 2015

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