Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Improve multi-line diff output #657

Closed
wants to merge 1 commit into from

3 participants

@simenbrekken

This patch improves multi-line diff output by utilizing diff.diffLines instead of diff.diffWords which ignores whitespace.

Here's an image showing the differences before and after the patch:

Patch result animation

@tj tj commented on the diff
lib/reporters/base.js
@@ -171,17 +171,19 @@ exports.list = function(failures){
// actual / expected diff
if ('string' == typeof actual && 'string' == typeof expected) {
var len = Math.max(actual.length, expected.length);
+ var lines = (actual.length > expected.length ? actual : expected).match(/\n/g).length;
@tj Owner
tj added a note

this fails if there's no newlines, something like .split(/\r\n|\r|\n/).length - 1 is probably better

@tj Owner
tj added a note

I have to test it anyway though I'll pull it down and make the change(s)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tj tj commented on the diff
lib/reporters/base.js
((12 lines not shown))
- // linenos
- var lines = msg.split('\n');
- if (lines.length > 4) {
- var width = String(lines.length).length;
- msg = lines.map(function(str, i){
+ // linenos
+ msg = errorDiff(err, 'Lines', escape).split('\n').map(function(str, i) {
@tj Owner
tj added a note

we should also display the same lineno since we're introducing a new line

Maybe move the line number logic to errorDiff along the lines of errorDiff(err, type, escape, lineNumbers)

@tj Owner
tj added a note

yeah we'll definitely have to refactor that part, I thought I was going to have time but I lied hahaha. might be able to get to it today

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

I was having issues with the line break diffs as well and solved with a simpler solution:

diff --git a/lib/reporters/base.js b/lib/reporters/base.js
index 24072eb..e9045b2 100644
--- a/lib/reporters/base.js
+++ b/lib/reporters/base.js
@@ -352,19 +352,24 @@ function pad(str, len) {
  */

 function errorDiff(err, type, escape) {
+  if (escape) {
+    err.actual = replaceInvisibleChars(err.actual);
+    err.expected = replaceInvisibleChars(err.expected);
+  }
   return diff['diff' + type](err.actual, err.expected).map(function(str){
-    if (escape) {
-      str.value = str.value
-        .replace(/\t/g, '<tab>')
-        .replace(/\r/g, '<CR>')
-        .replace(/\n/g, '<LF>\n');
-    }
     if (str.added) return colorLines('diff added', str.value);
     if (str.removed) return colorLines('diff removed', str.value);
     return str.value;
   }).join('');
 }

+function replaceInvisibleChars(str) {
+  return str
+    .replace(/\t/g, '<tab>')
+    .replace(/\r/g, '<CR>')
+    .replace(/\n/g, '<LF>\n');
+}
+
 /**
  * Color lines for `str`, using the color `name`.
  *

Here is a screenshot of before/after:

image diff

@millermedeiros

BTW using screenshots on the pull request was a very good idea. very easy to see the differences, I guess @jeremyckahn was right on his last post.

PS: the diff was the main reason why I decided to use mocha instead of Jasmine on my current project.

cheers.

@tj
Owner
tj commented

@millermedeiros nice I like that

@tj
Owner
tj commented

@millermedeiros hm actually it already behaves like that for me at least

@millermedeiros

@visionmedia it's strange, I can't replicate the issue with a simple expect("foo\nbar").to.equal("foo\n\nbar") but it does happen when I compare with a file loaded with fs.readFileSync().toString(). If I clone this repo, revert the changes to mocha (npm uninstall mocha && npm install mocha), add a line break into any of the files inside test/compare/default and run npm test the diff doesn't show up correctly to me. After my changes it does work as expected.

@millermedeiros millermedeiros referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@millermedeiros

I created a branch on esformatter to exemplify the issue, compare branch mocha_iss and master. master contain my changes to the base reporter, mocha_iss I did a clean install of mocha and introduced a line break on one of the tests.

PS: I'm on a mac.

@tj
Owner
tj commented

the real pain-point right now is spaces

@millermedeiros millermedeiros referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@tj
Owner
tj commented

lots of changes lately, closing this one for now

@tj tj closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 15, 2012
  1. @simenbrekken
This page is out of date. Refresh to see the latest.
Showing with 9 additions and 7 deletions.
  1. +9 −7 lib/reporters/base.js
View
16 lib/reporters/base.js
@@ -171,17 +171,19 @@ exports.list = function(failures){
// actual / expected diff
if ('string' == typeof actual && 'string' == typeof expected) {
var len = Math.max(actual.length, expected.length);
+ var lines = (actual.length > expected.length ? actual : expected).match(/\n/g).length;
@tj Owner
tj added a note

this fails if there's no newlines, something like .split(/\r\n|\r|\n/).length - 1 is probably better

@tj Owner
tj added a note

I have to test it anyway though I'll pull it down and make the change(s)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
- if (len < 20) msg = errorDiff(err, 'Chars', escape);
- else msg = errorDiff(err, 'Words', escape);
+ if (len < 20) {
+ msg = errorDiff(err, 'Chars', escape);
+ } else if (lines > 4) {
+ var width = String(lines).length;
- // linenos
- var lines = msg.split('\n');
- if (lines.length > 4) {
- var width = String(lines.length).length;
- msg = lines.map(function(str, i){
+ // linenos
+ msg = errorDiff(err, 'Lines', escape).split('\n').map(function(str, i) {
@tj Owner
tj added a note

we should also display the same lineno since we're introducing a new line

Maybe move the line number logic to errorDiff along the lines of errorDiff(err, type, escape, lineNumbers)

@tj Owner
tj added a note

yeah we'll definitely have to refactor that part, I thought I was going to have time but I lied hahaha. might be able to get to it today

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
return pad(++i, width) + ' |' + ' ' + str;
}).join('\n');
+ } else {
+ msg = errorDiff(err, 'Words', escape);
}
// legend
Something went wrong with that request. Please try again.