Ava: Better string and object diffs

[jamestalmage]

The power-assert output is great except for long strings and moderate to large sized JSON objects.

I think for those cases, it is probably better to just output a diff. Something similar to mocha.

(I'm almost certain there was an issue raised about this already, but I couldn't find it quickly).

[sindresorhus]

Hah, opening an issue like this was already on my todo list. :+1: :+1:

Is this maybe something the power-assert reporter should handle?

[TrySound]

Related https://github.com/power-assert-js/power-assert/issues/31

[jamestalmage]

Hmm. I don't know. It doesn't depend on powerAssertContext at all, just assertionErrror.expected and assertionError.actual, it certainly could be implemented as a custom reporter. I just don't know if that's the best choice.

// @twada

[twada]

It doesn't depend on powerAssertContext at all, just assertionErrror.expected and assertionError.actual

It makes sense when the assertion has two args (t.is, t.not, t.same, t.notSame).
With one arg (t.ok, t.notOk), power-assert should handle string diff and that's what BinaryExpressionRenderer does.

Rendering diffs between expected and actual could be handled by power-assert, and I'm thinking about implementing it though.

[jamestalmage]

Oh yeah, forgot about BinaryExpressionRenderer - we definitely want good diffs if they do:

t.true(someLongString === someOtherLongString)

How about extending BinaryExpressionRenderer with a "minimumLength" parameter that prevents short diffs from being displayed, a diff for t.true("foo" === "bar") is unnecessarily verbose, and it's easy to spot the difference looking at the power assert graph output.

[sindresorhus]

From Gitter, for posterity:

jamestalmage
@ben-eb power-asserts BinaryExpressionRenderer will output pretty diffs. So it would just be a matter of adding that in enhance.js.

ben-eb
@jamestalmage here? https://github.com/sindresorhus/ava/blob/master/lib/enhance-assert.js#L47-L52

jamestalmage 01:05
@ben-eb See the discussion in that PR. We don’t want BinaryExpressionRenderer to always show a diff. It’s too verbose.

ben-eb 01:06
i wouldn't either, no

jamestalmage 01:06
yep - that’s where.
I think the difficulty in fixing this is figuring out how to decide when to enable the BinaryExpressionRenderer.

ben-eb 01:07
so, is it possible to set a length at where diffs would render?
right

jamestalmage 01:08
It’s not possible in BinaryExpressionRenderer itself, but if you make a good PR that is general use (i.e. not exclusively targeted towards AVA). @twada will accept it. Our other option is to just wrap the renderer and delegate to it only if certain conditions are met.
I’m not sure if it’s as simple as string.length. It may be. Or you may need to examine the AST that is available in powerAssertContext that gets passed to the renderer.

ben-eb 01:10
what about this option? https://www.npmjs.com/package/power-assert-formatter#optionslinediffthreshold

jamestalmage 01:12
Yep - that looks key. Note that we don’t use that module. But build a formatter from individual renderers.
err - I’m wrong on that.

ben-eb 01:13
was going to say it's required in that file :smile:

jamestalmage 01:14
We only use two of these five renderers
This may be doable with existing power-assert options.

ben-eb 01:18
think my only issue is that my string diffs don't have newlines, so I would want to be able to test against the length instead of the number of lines :smile:

ben-eb 01:40
i didn't read that documentation properly, says it supports character level for lineDiffThreshold

ben-eb 03:17
sorry james, i'm completely lost with this.
i don't even know how i can break AVA's default assertions so that i can always show a diff

[twada]

@sindresorhus Oh thanks

[ben-eb]

I'd love this feature by the way, but I'm not at all familiar with AVA's code base...

[paulyoung]

As a workaround I've been doing this:

export const same = (t, actual, expected, message) => {
  return t.same(actual, expected, `

    ${message}

    Actual:
    ${JSON.stringify(actual, null, 2).split("\n").join("\n    ")}

    Expected:
    ${JSON.stringify(expected, null, 2).split("\n").join("\n    ")}

  `);
};

and then using:

same(t, actual, expected, message);

instead of:

t.same(actual, expected, message);

[vadimdemedes]

@paulyoung Good trick!

[novemberborn]

I've run into this lately. It makes it getting tests to pass quite cumbersome. Maybe we should step up the priority?

@twada are you saying this should be fixed in power-assert-renderers? Is it this issue: https://github.com/twada/power-assert-renderers/issues/1? What can we do to help?

[twada]

@novemberborn I've started a new power-assert-runtime modules in a monorepo style and deprecate power-assert-renderers.

First alpha version is already published and the first beta version will be released soon, maybe this weekend.

@twada are you saying this should be fixed in power-assert-renderers? Is it this issue: twada/power-assert-renderers#1? What can we do to help?

Therefore, this issue would be fixed in new power-assert-runtime repo. I'll migrate power-assert-renderers issues to power-assert-runtime.

Sorry for inconvenience :bow: Let's go forward together.

[novemberborn]

@twada cool, I just started watching the new repo :smile:

[MoOx]

I have to admit that long multilines string can be a pain

  ✖ Button render a MaterialUI <FlatButton />   
  t.deepEqual(actual, expected)
              |       |        
              |       "<FlatButton\n  backgroundColor=\"transparent\"\n  disabled={false}\n  hoverColor={{values: {alpha: 1, cmyk: [0, 0, 0, 7], hsl: [0, 0, 93], hsv: [0, 0, 93], hwb: [0, 93, 7], rgb: [238, 238, 238]}}}\n  label=\"Click\"\n  labelPosition=\"after\"\n  labelStyle={{color: {values: {alpha: 1, cmyk: [100, 56, 0, 45], hsl: [214, 100, 28], hsv: [214, 100, 55], hwb: [214, 0, 45], rgb: [0, 62, 141]}}}}\n  onKeyboardFocus={function noRefCheck() {}}\n  onMouseDown={function noRefCheck() {}}\n  onMouseEnter={function noRefCheck() {}}\n  onMouseLeave={function noRefCheck() {}}\n  onMouseUp={function noRefCheck() {}}\n  onTouchEnd={function noRefCheck() {}}\n  onTouchStart={function noRefCheck() {}}\n  primary={false}\n  rippleColor={{values: {alpha: 1, cmyk: [0, 0, 0, 39], hsl: [0, 0, 61], hsv: [0, 0, 61], hwb: [0, 61, 39], rgb: [155, 155, 155]}}}\n  secondary={false}\n  style={{}}\n/>"
              "<FlatButton\n  backgroundColor=\"transparent\"\n  disabled={false}\n  hoverColor=\"rgb(238, 238, 238)\"\n  label=\"Click\"\n  labelPosition=\"after\"\n  labelStyle={{color: 'rgb(0, 62, 141)'}}\n  onKeyboardFocus={function noRefCheck() {}}\n  onMouseDown={function noRefCheck() {}}\n  onMouseEnter={function noRefCheck() {}}\n  onMouseLeave={function noRefCheck() {}}\n  onMouseUp={function noRefCheck() {}}\n  onTouchEnd={function noRefCheck() {}}\n  onTouchStart={function noRefCheck() {}}\n  primary={false}\n  rippleColor=\"rgb(155, 155, 155)\"\n  secondary={false}\n  style={{}}\n/>"


  1 test failed

  1. Button render a MaterialUI <FlatButton />
  AssertionError: '<FlatButton\n  backgroundColor="transparent"\n  disabled={false}\n  hoverColor="rgb(238, 238, 238)"\n  label="Click"\n  labelPo === '<FlatButton\n  backgroundColor="transparent"\n  disabled={false}\n  hoverColor={{values: {alpha: 1, cmyk: [0, 0, 0, 7], hsl: [0
    Test.fn (index.js:39:5)

And in the terminal, it's even worst because you cannot scroll horizontally :)

[wenzowski]

Doing something similar to @paulyoung

import chalk from 'chalk';
import * as JsDiff from 'diff';

// warning!
// t.deepEqual() tests attribute order while prettyDiff() does not
export function deepEqual(t, actual, expected) {
  t.deepEqual(actual, expected, prettyDiff(actual, expected));
}

export function prettyDiff(actual, expected) {
  const diff = JsDiff.diffJson(expected, actual).map(part => {
    if (part.added) return chalk.green(part.value.replace(/.+/g, '    - $&'));
    if (part.removed) return chalk.red(part.value.replace(/.+/g, '    + $&'));
    return chalk.gray(part.value.replace(/.+/g, '    | $&'));
  }).join('');
  return `\n${diff}\n`;
}

diff 2.2.2
chalk 1.1.3

[Whoaa512]

Also something similar to @wenzowski and @paulyoung but with less moving parts:

import difflet from 'difflet'

const diff = difflet({ indent: 2 })

export default function deepEqual(t, actual, expected) {
    t.deepEqual(actual, expected, diff.compare(actual, expected))
}

[email protected]

[calebmer]

Is anyone working on this?

[sotojuan]

@calebmer I don't think so, want to help?

[iamstarkov]

@sotojuan what was the last idea to fix this?

[creeperyang]

SO no one is working on this?

Long string diff is really hard to read with [email protected].

Just a pic:

[sindresorhus]

@creeperyang It's being worked on in https://github.com/avajs/ava/pull/1154.

[creeperyang]

Great! 👍

[hughrawlinson]

1154 is merged! Is this issue closeable now? Or are you waiting for the release?

[novemberborn]

@hughrawlinson that's in the release, so yup, this issue can be closed. Thanks for reminding us.