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.

Sign up for GitHub

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

Jump to bottom

add unified diff separator #2846

Merged
merged 1 commit into from Sep 29, 2017
+50 −2
Merged

add unified diff separator #2846

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/reporters/base.js
View file
Open in desktop
Expand Up @@ -404,7 +404,7 @@ function unifiedDiff (err, escape) {
return indent + colorLines('diff removed', line);
}
if (line.match(/@@/)) {
return null;
return '--';
}
if (line.match(/\\ No newline/)) {
return null;
Expand All @@ -415,7 +415,7 @@ function unifiedDiff (err, escape) {
return typeof line !== 'undefined' && line !== null;
}
var msg = diff.createPatch('string', err.actual, err.expected);
var lines = msg.split('\n').splice(4);
var lines = msg.split('\n').splice(5);
return '\n ' +
colorLines('diff added', '+ expected') + ' ' +
colorLines('diff removed', '- actual') +
Expand Down
48 changes: 48 additions & 0 deletions test/reporters/base.spec.js
View file
Open in desktop
Expand Up @@ -14,6 +14,14 @@ function makeTest (err) {
};
}

function createElements (argObj) {
var res = [];
for (var i = argObj.from; i <= argObj.to; i += 1) {
res.push('element ' + i);
}
return res;
}

describe('Base reporter', function () {
var stdout;
var stdoutWrite;
Expand Down Expand Up @@ -160,6 +168,46 @@ describe('Base reporter', function () {
});
});

describe('unified diff reporter', function () {
it('should separate diff hunks by two dashes', function () {
var err = new Error('test');
var errOut;

err.actual = createElements({ from: 2, to: 11 });
err.expected = createElements({ from: 1, to: 10 });
err.showDiff = true;
var test = makeTest(err);

Base.inlineDiffs = false;
Base.list([test]);

errOut = stdout.join('\n');

var regexesToMatch = [
/\[/,
/\+ {2}"element 1"/,
/"element 2"/,
/"element 3"/,
/"element 4"/,
/"element 5"/,
/--/,
/"element 7"/,
/"element 8"/,
/"element 9"/,
/"element 10"/,
/- {2}"element 11"/,
/]/,
/test/,
/expected/,
/actual/
];

regexesToMatch.forEach(function (aRegex) {
errOut.should.match(aRegex);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of matching each line's regex against the whole output string, which does not guarantee order as far as I am aware, let's either match a single regex covering multiple lines, or else use .forEach(function (aRegex, index) { <next line> errOut.split('\n')[index].should... instead of .forEach(function (aRegex) { <next line> errOut.should...

Copy link
Contributor Author

@olsonpm olsonpm Jun 4, 2017 •
edited

Choose a reason for hiding this comment

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

When I PR - I try to avoid corrections like these and just stick with the patterns in the existing codebase. I too thought this pattern was weird, but it's what's in the code.

If you really want me to make it different for this particular test I will, but I suggest refactoring them all in a separate PR for more accurate test coverage.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ouch! My bad; I hadn't noticed those tests before.

In that case I'd be fine merging this as-is for consistency and then doing a separate thorough sweep to correct all instances of that pattern.

Thanks for identifying that!

});
});

it('should stringify objects', function () {
var err = new Error('test');
var errOut;
Expand Down