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.

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

Base reporter, add unified diff separator #2295

Closed
olsonpm opened this issue Jun 3, 2016 · 10 comments
Closed

Base reporter, add unified diff separator #2295

olsonpm opened this issue Jun 3, 2016 · 10 comments
Labels
area: usability concerning user experience or interface semver-major implementation requires increase of "major" version number; "breaking changes" status: accepting prs Mocha can use your help with this one!

Comments

@olsonpm
Copy link
Contributor

olsonpm commented Jun 3, 2016

Right now the unified diff output has no separators. Resulting in output similar to this:

no-diff-seperator

Having not used mocha in some time, I had completely forgotten it splits before/after 4 lines from the affected text and spent a while trying to figure out how "PD" could possibly be having values of "15/30" or "50/100".

I don't know what kind of separator to add, but a blank line or two should suffice for majority of cases. I'd have to check to see if other libraries have tackled this problem how they went about it.

The code to modify would be this:

mocha/lib/reporters/base.js

Lines 161 to 207 in 7493bca

failures.forEach(function(test, i){
// format
var fmt = color('error title', ' %s) %s:\n')
+ color('error message', ' %s')
+ color('error stack', '\n%s\n');
// msg
var err = test.err
, message = err.message || ''
, stack = err.stack || message
, index = stack.indexOf(message)
, actual = err.actual
, expected = err.expected
, escape = true;
if (index === -1) {
msg = message;
} else {
index += message.length;
msg = stack.slice(0, index);
// remove msg from stack
stack = stack.slice(index + 1);
}
// uncaught
if (err.uncaught) {
msg = 'Uncaught ' + msg;
}
// explicitly show diff
if (err.showDiff !== false && sameType(actual, expected)
&& expected !== undefined) {
escape = false;
if (!(utils.isString(actual) && utils.isString(expected))) {
err.actual = actual = utils.stringify(actual);
err.expected = expected = utils.stringify(expected);
}
fmt = color('error title', ' %s) %s:\n%s') + color('error stack', '\n%s\n');
var match = message.match(/^([^:]+): expected/);
msg = '\n ' + color('error message', match ? match[1] : msg);
if (exports.inlineDiffs) {
msg += inlineDiff(err, escape);
} else {
msg += unifiedDiff(err, escape);
}
}

@ScottFreeCode
Copy link
Contributor

Git has a format something like this: @@ -<line number of start of this segment in previous file version>,<number of lines removed> +<line number of start of this segment in next file version>,<number of lines added> @@
It's blue, where the + are green and the - are red. And I'm not sure I'm interpreting those numbers exactly right -- maybe the numbers after the commas include the unchanged lines shown?

@olsonpm
Copy link
Contributor Author

olsonpm commented Jun 12, 2016

hmm - the problem with generating line numbers is that these diffs are for formatted objects, thus line numbers don't seem appropriate.

However I did notice git grep separates sections with \n--\n if multiple matches are found. That seems intuitive enough.

Edit: especially if they were colored something beside green/red

@boneskull
Copy link
Member

@olsonpm Can you show what should be output?

@boneskull boneskull added the status: waiting for author waiting on response from OP - more information needed label Jun 27, 2016
@olsonpm
Copy link
Contributor Author

olsonpm commented Jun 27, 2016

As I said I'm open for discussion on what makes the most sense, but my vote leans toward the output of git grep -C<number>, where sections are delimited by a double dash. E.g.

asdf

@stale stale bot added the stale this has been inactive for a while... label May 24, 2017
@stale
Copy link

stale bot commented May 24, 2017

I am a bot that watches issues for inactivity.
This issue hasn't had any recent activity, and I'm labeling it stale. In 14 days, if there are no further comments or activity, I will close this issue.
Thanks for contributing to Mocha!

@olsonpm
Copy link
Contributor Author

olsonpm commented May 24, 2017

@boneskull - trying to prevent the issue from being closed. I provided the requested feedback, so at least remove that label please.

@stale stale bot removed the stale this has been inactive for a while... label May 24, 2017
@ScottFreeCode ScottFreeCode removed the status: waiting for author waiting on response from OP - more information needed label May 25, 2017
@ScottFreeCode
Copy link
Contributor

I'd be fine with just -- as a separator if no one has better suggestions or other concerns. Do we want to put this behind a flag for compatibility? (Another advantage to a flag would be we could provide the ability to override -- with some other non-dynamic string...)

@boneskull boneskull added the area: usability concerning user experience or interface label May 25, 2017
@boneskull
Copy link
Member

unexpected may have a good solution we can pilfer; let's take a look at what their messaging looks like.

Otherwise, -- is just as reasonable as any other string.

@ScottFreeCode I don't think this needs an option to change the separator, and a flag also feels like overkill.

If we're erring on the site of caution, this is semver-major, because it could confuse anything that's programmatically consuming the output. No, nothing should consume it, but something probably does.

@boneskull boneskull added status: accepting prs Mocha can use your help with this one! semver-major implementation requires increase of "major" version number; "breaking changes" labels May 25, 2017
@olsonpm
Copy link
Contributor Author

olsonpm commented May 25, 2017

Sweet - I'll get a pr in next week. Thanks for the responses.

olsonpm pushed a commit to olsonpm/mocha that referenced this issue May 31, 2017
 - fix issue mochajs#2295
 - the purpose of this feature is to make the unified diff
   format more readable.  Without it, different portions
   of the same actual/expected diff look contiguous even
   though they are actually separated by the default
   context of four lines.
olsonpm pushed a commit to olsonpm/mocha that referenced this issue Jun 4, 2017
 - fix issue mochajs#2295
 - the purpose of this feature is to make the unified diff
   format more readable.  Without it, different portions
   of the same actual/expected diff look contiguous even
   though they are actually separated by the default
   context of four lines.
ScottFreeCode pushed a commit that referenced this issue Sep 29, 2017
- fix issue #2295
 - the purpose of this feature is to make the unified diff
   format more readable.  Without it, different portions
   of the same actual/expected diff look contiguous even
   though they are actually separated by the default
   context of four lines.
@ScottFreeCode
Copy link
Contributor

This went out in 4; thanks again!

sgilroy pushed a commit to TwineHealth/mocha that referenced this issue Feb 27, 2019
- fix issue mochajs#2295
 - the purpose of this feature is to make the unified diff
   format more readable.  Without it, different portions
   of the same actual/expected diff look contiguous even
   though they are actually separated by the default
   context of four lines.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: usability concerning user experience or interface semver-major implementation requires increase of "major" version number; "breaking changes" status: accepting prs Mocha can use your help with this one!
Projects
None yet
Development

No branches or pull requests

3 participants