[WIP] Implementation of support for diff property in AssertionError #3251

Mingun · 2018-02-24T20:47:39Z

Description of the Change

The purpose of this PR -- check of hypotheses and testing of changes (because local execution of mocha tests -- pain, fear and horror) and continuation of the discussion begun in chaijs/chai#1120.

The first commit contains preparatory work for a possibility of getting the structured diff from AssertionError
The second commit changes the order actual and expected in diff output, for more logical, with expected as a base point. Meaningful of the diff -- changes which need to be applied to expected to get actual
The third commit adds a feature for get diff from an error. Its specific implementation -- a subject for discussion. For demonstration of a new ability I made demo branch in the https://github.com/Mingun/chai-like repository. Just clone this repo, switch to the demo branch. Last commit from it uses diff from an error, penultimate -- mocha built-in.

One of current problems with this solution -- the "List reporter on fail should immediately construct fail strings" test. It expects that expected and actual will be strings, but because of the lazy nature of diff generation they cannot be turned into strings, otherwise it is impossible to guarantee correct operation (see implementation in chai-like). As such conversion was carried out only if diff was required that I do not think that it important and it is worth saving such behavior.

Alternate Designs

Instead of diff function in an error it is possible to use the property containing both types of diffs (unified and inline), however such solution can decelerate diff generation (there were already issues with speed of diff generation)

Benefits

Correct diff can be constructed only by assertion library, test runner (mocha) has no enough information for this that the situation with chai-like shows. On the other hand, only test runner knows how diff will be displayed (coloring etc.) assertion library cannot do this.

Transmission of the structural information necessary for creation of text representation, is necessary also enough. After I did this implementation, I agree with this opinion of @keithamus, stated to them in discussion in chaijs/chai#1120.

Possible Drawbacks

No. mocha still uses built-in diff if it they not exist in an error.

Applicable issues

#3193

Is it an enhancement (minor release)

coveralls · 2018-02-24T20:55:04Z

Coverage increased (+0.1%) to 90.166% when pulling f6d63ca on Mingun:diff into d76f490 on mochajs:master.

boneskull · 2018-02-28T01:33:59Z

lib/reporters/base.js

-    err.expected = utils.stringify(err.expected);
-  }
+  return (utils.type(err.diff) === 'function') ||
+         (err.showDiff !== false && sameType(err.actual, err.expected) && err.expected !== undefined);


please use typeof err.expected === 'undefined' for safety

Ok. Maybe use utils.type(err.expected) === 'undefined' for uniformity with a code in the line above?

boneskull · 2018-02-28T01:34:16Z

test/integration/fixtures/diffs/output

@@ -1,91 +1,91 @@
 // DIFF
-      -foo rar baz
-      +foo bar baz
+      -foo bar baz


why did this have to happen?

It and similar changes because of the second commit which changes the order of actual and expected in diff generation

why did we change the order?

It is necessary for more natural message in diff. diff shows the reason for which actual does not correspond to expected. If the reason that at actual there is an excess element, then the reason shall show that this excess element is added, thus, I know that I need to find the place where the excess element is added and not to add it. Thus, for

expected = [1, 2]

actual = [1]

diff shall look so:

1 -2

and for

expected = [2]

actual = [1, 2]

diff shall look so:

+1 2

Without this change everything was on the contrary that at least confuses me a little

boneskull · 2018-03-01T01:20:46Z

lib/reporters/base.js

  }
+  lines.push('');


I'm having trouble following this diff. Why is this here?

This is add empty line after diff, just as in old code, but use more high-level abstraction, which allow write similar code in the inlineDiff and unifiedDiff functions

boneskull · 2018-03-01T01:20:54Z

lib/reporters/base.js

+
+    change.value.split('\n').forEach(function (line, i) {
+      line = colorize(line);
+      if (i === 0) {


what's this?

This prevent adding delimiters between hunks before first hunk

boneskull · 2018-03-01T01:21:29Z

lib/reporters/base.js

+    if (i !== 0) {
+      lines.push('--');
+    }
+    hunk.lines.forEach(function (line) {


maybe use reduce here..?

I don't see how it can be useful here?

boneskull · 2018-03-01T01:21:48Z

package.json

@@ -309,7 +309,7 @@
    "browser-stdout": "1.3.0",
    "commander": "2.11.0",
    "debug": "3.1.0",
-    "diff": "3.3.1",
+    "diff": "3.4.0",


Just to keep dependencies actual. The changes made between versions do not influence the functions used by us, but the fresh version of library will simplify the analysis of changes if such appear and they will influence our code

boneskull · 2018-03-01T01:23:26Z

lib/reporters/base.js

-    err.actual = utils.stringify(err.actual);
-    err.expected = utils.stringify(err.expected);
-  }
+  return (utils.type(err.diff) === 'function') ||


so it was agreed that err.diff is a function? what's its signature? can you point me to that discussion? again, I'm having trouble following / understanding what exactly people want here.

Function is used because we need one of two formats for different types of diff as one of possible solutions for this. Perhaps, it will be possible to integrate them in one format, in that case function is not required and this field will be able to be a simple object in this format. In this case this it will be necessary to make utility functions for conversion of results of diff library to this universal format. Since in chaijs/chai#1120 already expressed idea of object diff library, it can be done in there.

At the moment I only used existing formats which are provided by diff library that simplifies implementation of the user-defined diffs. Signature:

/** @param {boolean} inline Generate inline diffs if true, generate unified diff otherwise @return {Object} The same as return value of `diff.structuredPatch` from the `diff` library if `inline` is falsely The same as return value of `diff.diffWordsWithSpace` from the `diff` library if `inline` is truely */ function diff(inline) { ... }

Though use of universal format will give nothing -- for generation it is necessary to use different functions. As the choice of a format is carried out by mocha, and generation of diff by assertion library, it is necessary either to use function, or to generate directly 2 kinds of diff, which may be time expensive

… diff for colorize them

…ual added

Mingun · 2018-03-09T10:51:10Z

In the commit f6d63ca I add support for per-character diffs in unified diffs. In principle, existence of such information allows to manage only one type of diff therefore I have a question and suggestion: can be to delete support of inlineDiffs? With a new opportunity it does not look the especially useful, and existence only of one format simplifies support and will allow the err.diff field to be not function, but a diff object.

In image below you can see possible result. Before tildas you can see new unified format, below -- existent inline format:

There files on which diffs is generated:

Mingun · 2018-03-12T16:22:35Z

I find a way to use one data structure for both kinds of diff and push changes soon. Thus, a data structure, I think, is defined. Other changes:

The quantity of context lines is defined by the reporter. In the future this can be configurable if you wish
Also I implement concealment of big pieces of not changed lines in the inline diff mode
For inline diff line numbers now displayed correclty. Now it is not arbitrary values of the simple counter, but real line numbers, one for old and one for new version
Also I enable line numbers for unified diff too. In both diff modes line numbers shows if line count >= 4
In unified mode intra line diff is shown
The word diff algorithm is tuned (pay attention to text near parentheses)

Also I completely rework a method by which diff is displayed on the screen and now it looks as:

For comparing, the same files in the current implementation

The next thing -- tests. There are any ideas about what needs to be tested?

Mingun · 2018-03-20T20:05:29Z

package.json

    "escape-string-regexp": "1.0.5",
    "glob": "7.1.2",
    "growl": "1.10.3",
    "he": "1.1.1",
    "mkdirp": "0.5.1",
+    "structured-diff": "Mingun/structured-diff",


After the publication of a package I will replace the reference to a npm repository here

boneskull · 2018-03-21T20:06:01Z

The next thing -- tests. There are any ideas about what needs to be tested?

Everything that's changed, essentially. Tests should run in both browser and Node.js. You'll want unit tests for the basic functionality, then a handful of integration/functional tests to ensure that it enables the desired behavior.

boneskull · 2018-03-21T20:07:16Z

Also, because we have so many open PRs, I'm going to close this because it's a work-in-progress. This does not mean I'm rejecting the PR, but rather I prefer you hold it until it is "ready". Feel free to add any further questions to the associated open issue(s).

boneskull reviewed Feb 28, 2018

View reviewed changes

boneskull reviewed Mar 1, 2018

View reviewed changes

boneskull added type: feature enhancement proposal semver-minor implementation requires increase of "minor" version number; "features" labels Mar 1, 2018

Mingun mentioned this pull request Mar 2, 2018

Ability to show only changes and deletions in diffs, not additions chaijs/chai#1120

Closed

Mingun added 4 commits March 9, 2018 11:26

Improve diff handling code -- work with structures instead of parsing…

33f5704

… diff for colorize them

Change order of expected and actual in diffs -- expected removed, act…

f30c421

…ual added

Add support for external diffs

9b01dc7

Add support for per-character diffs in unified diffs

f6d63ca

Mingun force-pushed the diff branch from 10879ec to f6d63ca Compare March 9, 2018 10:24

Mingun added 5 commits March 13, 2018 21:52

Implement correct coloring with several colors in line

7f046de

Extract diff printers to own files

e0e77d1

Implement universal format for unified and inline diff

f5c3be5

Remove duplicated test

dd98eb8

Switch to structured-diff library for generating diffs

e2a7f6e

Mingun commented Mar 20, 2018

View reviewed changes

boneskull closed this Mar 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Implementation of support for diff property in AssertionError #3251

[WIP] Implementation of support for diff property in AssertionError #3251

Mingun commented Feb 24, 2018

coveralls commented Feb 24, 2018 •

edited

Loading

boneskull Feb 28, 2018

Mingun Feb 28, 2018

boneskull Feb 28, 2018

Mingun Feb 28, 2018

boneskull Mar 1, 2018

Mingun Mar 1, 2018

boneskull Mar 1, 2018

Mingun Mar 1, 2018

boneskull Mar 1, 2018

Mingun Mar 1, 2018

boneskull Mar 1, 2018

Mingun Mar 1, 2018

boneskull Mar 1, 2018

Mingun Mar 1, 2018

boneskull Mar 1, 2018

Mingun Mar 1, 2018 •

edited

Loading

Mingun Mar 1, 2018

Mingun commented Mar 9, 2018

Mingun commented Mar 12, 2018

Mingun Mar 20, 2018

boneskull commented Mar 21, 2018

boneskull commented Mar 21, 2018

[WIP] Implementation of support for diff property in AssertionError #3251

[WIP] Implementation of support for diff property in AssertionError #3251

Conversation

Mingun commented Feb 24, 2018

Description of the Change

Alternate Designs

Benefits

Possible Drawbacks

Applicable issues

coveralls commented Feb 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mingun Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mingun commented Mar 9, 2018

Mingun commented Mar 12, 2018

Choose a reason for hiding this comment

boneskull commented Mar 21, 2018

boneskull commented Mar 21, 2018

coveralls commented Feb 24, 2018 •

edited

Loading

Mingun Mar 1, 2018 •

edited

Loading