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

Seperate diff generation code from Base reporter; closes #3011 #3201

Closed
wants to merge 2 commits into from

Conversation

harrysarson
Copy link
Contributor

Description of the Change

Moves the code used to generate "nice diffs" out of lib/reporters/base.js and into a new file which I have called lib/formatDiffs.js. This allows access to "nice diffs" by IDE's.

To make this change I also had to move move code which deals with the coloring of mocha output out of lib/reporters/base.js as it is needed to generate the diffs. I put this into lib/colorUtils.js.

Alternate Designs

This is a first step towards exposing a public API, I have left function definitions as unchanged as possible but they should be changed if this API is to be exposed.

Benefits

Allows IDE to produce prettier output (#3011)

Possible Drawbacks

Doing Base.colors = { /* ... */ } will no longer change the colors used by mocha as Base.colors is now a reference to the object defined in lib/colorUtils.js.

If this is a problem Base.colors could be defined using getters/setters or made readonly.

Applicable issues

#3011

This should be semver patch.

@jsf-clabot
Copy link

jsf-clabot commented Jan 13, 2018

CLA assistant check
All committers have signed the CLA.

@harrysarson harrysarson changed the title Seperate diff Seperate diff generation code from Base reporter; closes #3011 Jan 13, 2018
@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 90.054% when pulling d2c2eb5 on HarrySarson:seperate-diff into c7730a6 on mochajs:master.

@boneskull boneskull added the type: cleanup a refactor label Jan 17, 2018
@boneskull
Copy link
Contributor

Thanks! @harrysarson Does this actually close #3011? It looks like this is just part of the solution, right?

Copy link
Contributor

@boneskull boneskull left a comment

Choose a reason for hiding this comment

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

Where would you suggest the public API lives here?
Not exactly sure of the intent.

@boneskull
Copy link
Contributor

cc @segrey

@harrysarson
Copy link
Contributor Author

@boneskull yeah sorry this is not a complete solution to #3011. It was my intention to ask for feedback and about the desired shape of the public API.

I think the exported functions inlineDiff and unifiedDiff (in fornatError.js) are what were requested by @segrey. These two functions generate coloured diffs based on the error. Would love feedback about whether this is alone the right lines to solve this use case.

@boneskull
Copy link
Contributor

boneskull commented Jan 17, 2018

@segrey I'm not sure this is really going to solve your issue, because Mocha is not the only place diffs come from. Some assertion libraries provide their own diffs, appended to the message prop of whatever Error they give us.

We may overhaul all of this. See chaijs/chai#1120 for more discussion.

I think we should probably sit on #3011 unless @segrey is fine w/ supporting a temporary change.

@segrey
Copy link
Contributor

segrey commented Jan 18, 2018

@harrysarson Thanks! AFAIU, a reporter can use this API in the following way:

var BaseReporter = requireMochaModule('./lib/reporters/base');
var formatDiffs = requireMochaModule('./lib/formatDiffs');

function extractErrorDetails(err) {
  var message = err.message, stack = err.stack;
  var index = stack.indexOf(message);
  if (index >= 0) {
    message = stack.slice(0, index + message.length);
    stack = stack.slice(message.length);
  }
  var diff;
  if (BaseReporter.inlineDiffs) {
    diff = formatDiffs.inlineDiff(err, {useColors: BaseReporter.useColors});
  } else {
    diff = formatDiffs.unifiedDiff(err, {useColors: BaseReporter.useColors});
  }
  return {
    message : message,
    diff: diff,
    stack : stack
  }
}

Since mocha already does something similar in Base reporter (it extracts message, diff and stack from Error object), it seems possible to expose its result as err.details with the following properties: {message, diff, stack}?
BTW, this err.details object can be expanded with {actualStr, expectedStr} properties (so IntelliJ reporter may no longer call stringify from ./lib/utils).
Seems this API allows to leverage diffs provided by assertion libraries: mocha can simply set err.details.diff to a needed diff. @boneskull what do you think?

@segrey
Copy link
Contributor

segrey commented Jan 19, 2018

Anyway, I'm fine with a quick temporal solution too. Seems it can be smaller, like this one:

// lib/reporters/base.js
exports.unifiedDiff = unifiedDiff;
exports.inlineDiff = inlineDiff;

Then, IntelliJ reporter could use it in this way:

  var diff;
  if (BaseReporter.inlineDiffs) {
    diff = BaseReporter.inlineDiff(err);
  } else {
    diff = BaseReporter.unifiedDiff(err);
  }

@harrysarson
Copy link
Contributor Author

@segrey how does #3213 look?

Closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: cleanup a refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants