String diff output starts red and ends white

[lukechilds]

I think this is a bug, although maybe it's intentional and I'm missing something.

The text output for the string diff always starts red for neutral text (as in text that has been neither added or removed in the diff). Then elsewhere the neutral text is white not red. You can see what I mean in the screenshot from the readme:

Notice how "this is so is red, but the last " is white.

I think all the text should be white apart from cool|bad.

[vadimdemedes]

I agree, we should change the string color from red to white. Thanks for reporting this!

[lukechilds]

No probs, happy to submit a PR but I don't seem to be able to figure out where it's coming from.

This is the code for the diff:

if (err.actualType === 'string' && err.expectedType === 'string') {
  const patch = diff.diffChars(err.actual, err.expected);
  const msg = patch
    .map(part => {
      if (part.added) {
        return chalk.bgGreen.black(part.value);
      }

      if (part.removed) {
        return chalk.bgRed.black(part.value);
      }

      return part.value;
    })
    .join('');

  return `Difference:\n\n${msg}\n`;
}

As you can see the section for the neutral text applies no colouring: return part.value;. So it's weird that it's red the first time it's returned but not the other times. That makes me think that it's inheriting it from a previous unclosed ANSI colour code, however the string is interpolated into the same string that starts with the word Difference::

return `Difference:\n\n${msg}\n`;

But Difference: isn't red. Any ideas?

[vadimdemedes]

The red color for strings is a default value set in @ava/pretty-format module (at the end of index.js in default props for 'theme' prop, if I'm not mistaken).

Now that I think of it, it'd be inconsistent if strings in other types of output (comparing objects, for example) were red, but white in string diff?

[lukechilds]

Ok, so that flips the problem round, it's the white text that is incorrect. Instead of the neutral colour being white it should be red.

[lukechilds]

Hmmn, I'm struggling to think of a clean way to fix this. Obviously a really simple fix would be to just colour the neutral text red, but then if the string pretty-format colour changes then it'll be wrong.

So we could check the default pretty-format colour and use that, but then if pretty-format was ran with custom colour options we won't know and again the colour will be wrong.

Ideally we need to run the diff on an unformatted string, and then run the output through pretty-format. But the diffs for objects and arrays need to be ran through pretty-format first. I think the neatest solution would be to save a copy of the raw value in the err object when it's getting formatted. So inside the diff we can access the raw value. The error would be exposed as:

{ name: 'AssertionError',
  actualRawValue: 'foobuzz',
  actualType: 'string',
  actual: '\u001b[31m"foobuzz"\u001b[39m',
  expectedRawValue: 'foobar',
  expectedType: 'string',
  expected: '\u001b[31m"foobar"\u001b[39m',
  operator: '===',
  message: ' ',
  generatedMessage: false,
  stack: 'AssertionError:  \n    Test.t [as fn] (test/visual/console-log.js:4:4)\n    ',
  showOutput: true,
  source:
   { isDependency: false,
     isWithinProject: true,
     file: '/Users/lukechilds/Dev/oss/ava/test/visual/console-log.js',
     line: 4 } }

actualRawValue and expectedRawValue are my additions. Thoughts?

[vadimdemedes]

How about the following:

1. Before generating a string diff, strip the red color from the string using strip-ansi module, which we already depend on.
2. Generate a string diff, colorize actual/expected chars (red/green background) and use a red color for unchanged values here https://github.com/avajs/ava/blob/master/lib/format-assert-error.js#L59.

That way the red color won't interfere with the highlighting of actual/expected values.

[lukechilds]

If you're ok with hardcoding the red colour in format-assert-error.js then that's all that's needed. It's not necessary to strip any ANSI codes out first.

[vadimdemedes]

If you're ok with hardcoding the red colour in format-assert-error.js then that's all that's needed.

Yeah, hardcoding is rarely a great solution. You can hardcode it for now and I'll take care of it later by creating a "theme.js" with colors passed to pretty-format.

No need to strip any ANSI codes out first.

Wouldn't it end up with duplicate red color ansi codes?

[lukechilds]

I'll take care of it later by creating a "theme.js" with colors passed to pretty-format.

Perfect 👌

Wouldn't it end up with duplicate red color ansi codes?

Yes of course sorry, I've just got in after a few 🍻

[vadimdemedes]

Haha, no worries, I totally forgot we already have a colors.js as well, so no need for a theme.js!

[lukechilds]

Sweet, just force pushed the strip-ansi update to the PR 👍