assert: add error diffs #17615

BridgeAR · 2017-12-11T18:19:00Z

The current assert errors are actually pretty bad when it comes to objects. This implements
a simple way to add diffs as default but only in assert strict mode. It relies on another PR that changes the util.inspect output as I use that for the diff. Building the diff differently would likely yield better results but it would also mean more work and this on its own should already be a significant improvement.

So far a few spots are not optimized for performance and I would like to get some general feedback on the solution. I especially do not like the way to distinguish asserts strict mode and legacy mode.
Activating the diff always would mean we have to change all our assert tests and I did not want to do that, besides the fact that some people might partially rely on the message output.

Refs #17576

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

assert, lib, util, errors

Trott · 2017-12-12T00:16:27Z

doc/api/assert.md

@@ -26,12 +29,33 @@ When using the `strict mode`, any `assert` function will use the equality used i
 the strict function mode. So [`assert.deepEqual()`][] will, for example, work the
 same as [`assert.deepStrictEqual()`][].

+Besides that error messages that involve objects produce a error diff instead of


Nit: a errror diff -> an error diff

The sentence seems to be missing some punctuation for clarity. Probably a comma after the second word?

vsemozhetbyt · 2017-12-12T00:42:32Z

doc/api/assert.md

 It can be accessed using:

 ```js
 const assert = require('assert').strict;
 ```

+Example error diff:
+
+```diff


I am not sure, but maybe it is worth to separate the input with js label (for better highlighting and linting) and the output with diff label.

I cannot find any ```diff insets in .md files in this repo or nodejs.org repo. Are we sure they are rendered to the .html docs properly?

BridgeAR · 2017-12-28T05:38:24Z

I just updated the code. I also improved the diffing algorithm from the first draft. It will now try to optimize the output rudimentary by removing identical lines from the back first, limits the output lines and some other smaller changes.

So PTAL

jasnell · 2017-12-28T17:58:15Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12331/

BridgeAR · 2017-12-29T18:41:13Z

Rebased due to conflicts.

I actually changed the tests to use the new assert.throws(fn, obj) pattern instead of try {} catch {} or common.expectsError.

BridgeAR · 2017-12-29T18:41:40Z

CI https://ci.nodejs.org/job/node-test-pull-request/12348/

BridgeAR · 2018-01-03T15:49:24Z

@nodejs/collaborators PTAL. This should be ready but it would be nice to get another LG.

mcollina · 2018-01-15T19:23:02Z

lib/internal/errors.js

+  const expectedLines = util
+    .inspect(expected, { compact: false, depth: 10 }).split('\n');
+  const msg = `Input A expected to ${operator} input B:\n` +
+        '\u001b[32m+ expected\u001b[39m \u001b[31m- actual\u001b[39m';


We should probably be testing that unix color codes are supported on the current terminal.

I don’t think we can test that but it might make sense to:

Check whether process.stderr is a TTY

Make coloring overridable in some way

That would indeed be nice... I will see what I can do but I am not sure if we can properly detect that.

addaleax · 2018-01-15T19:42:46Z

doc/api/assert.md

@@ -26,12 +29,42 @@ When using the `strict mode`, any `assert` function will use the equality used i
 the strict function mode. So [`assert.deepEqual()`][] will, for example, work the
 same as [`assert.deepStrictEqual()`][].

+Besides that error messages that involve objects produce an error diff instead of


grammar nit: comma after the first that? Or maybe just start with Error messages which involve objects …?

addaleax · 2018-01-15T19:53:04Z

lib/assert.js

@@ -152,7 +152,8 @@ assert.deepStrictEqual = function deepStrictEqual(actual, expected, message) {
      expected,
      message,
      operator: 'deepStrictEqual',
-      stackStartFn: deepStrictEqual
+      stackStartFn: deepStrictEqual,
+      errorDiff: this === strict ? 2 : 0


Can you name the different error diff modes? It’s really hard to keep track of constants 0, 1 and 2 with distinct meanings, especially in code that spans across files.

addaleax · 2018-01-15T19:53:52Z

lib/internal/errors.js

+  const expectedLines = util
+    .inspect(expected, { compact: false, depth: 10 }).split('\n');
+  const msg = `Input A expected to ${operator} input B:\n` +
+        '\u001b[32m+ expected\u001b[39m \u001b[31m- actual\u001b[39m';


I don’t think we can test that but it might make sense to:

Check whether process.stderr is a TTY

Make coloring overridable in some way

BridgeAR · 2018-01-16T13:04:06Z

@mcollina @addaleax I added a util.hasColors function. It is a very basic way to detect this but it seems like it is better than anything we had so far.

As this is probably also something that other users might want to rely on, I added it to the util module, even though I am not sure if it is the best place for it. What do you think about that?
And about adding it as a dedicated function in general: if we expose this, other users will report things to improve the current implementation :-).

@jasnell @Trott @vsemozhetbyt @TimothyGu please also take another look.

vsemozhetbyt · 2018-01-16T13:35:49Z

doc/api/util.md

+added: REPLACEME
+-->
+
+* `stream` {Stream} A [stream][]. Defaults to `process.stdout`.


[stream][] — Missing bottom reference?

BridgeAR · 2018-01-16T15:39:43Z

Rebased and new CI https://ci.nodejs.org/job/node-test-pull-request/12554/

I also addressed the doc comment.

mcollina

LGTM

PR-URL: nodejs#17615 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

BridgeAR · 2018-03-08T16:24:48Z

Backport opened in #19230

From now on all error messages produced by `assert` in strict mode will produce a error diff. Backport-PR-URL: #19230 PR-URL: #17615 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Right now it is very difficult to determine if a terminal supports colors or not. This function adds this functionality by detecting environment variables and checking process. Backport-PR-URL: #19230 PR-URL: #17615 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Backport-PR-URL: #19230 PR-URL: #17615 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

From now on all error messages produced by `assert` in strict mode will produce a error diff. Backport-PR-URL: #19230 PR-URL: #17615 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Right now it is very difficult to determine if a terminal supports colors or not. This function adds this functionality by detecting environment variables and checking process. Backport-PR-URL: #19230 PR-URL: #17615 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Backport-PR-URL: #19230 PR-URL: #17615 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Notable changes: * assert: - From now on all error messages produced by `assert` in strict mode will produce a error diff. (Ruben Bridgewater) #17615 - From now on it is possible to use a validation object in throws instead of the other possibilities. (Ruben Bridgewater) #17584 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * fs: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * tty: - Add getColorDepth function to determine if terminal supports colors (Ruben Bridgewater) #17615 * util: - add util.inspect compact option (Ruben Bridgewater) #17576 * **Added new collaborators** - [watson](https://github.com/watson) Thomas Watson PR-URL: #19428

sindresorhus · 2018-03-23T19:09:03Z

lib/tty.js

+    return COLORS_16;
+
+  return COLORS_2;
+};


This code is mostly copy-pasted from https://github.com/chalk/supports-color/blob/master/index.js and https://github.com/isaacs/color-support/blob/master/index.js with minor style tweaks. I don't see any attribution.

@BridgeAR can you comment on the above?

Hey, I just came home and saw this, so I could not respond any earlier.

When writing on this I got the #17615 (comment) from @mcollina that I should check for the colors. So I googled a lot about checking color support in terminals and I tried to gather as much information as I could (I actually tried hard not to rely on environment variables at all). In the end I got inspired by multiple sources including chalk, color-support, different gists, stackoverflow and more that I do not remember. I did not think of anything bad when adding this function. I do see the similarities though and I am more than happy to give the attributions. I am just not sure how to do this properly right now as there are multiple licenses.

@sindresorhus shall I just move the function in a new file and add both license texts? I am not sure about the other sources though. Or would it be fine to add something like: This functionality got inspired by multiple sources including 'chalk' by @sindresorhus and 'color-support' by @isaacs?

Ping @isaacs

I guess also cc @nodejs/tsc FYI

@BridgeAR
I think moving the color detection code to an internal lib file and put attributions there would be better than doing it in tty.js. But it still comes down to whether @sindresorhus and @isaacs accept it or not.

TTY tests should almost never be placed in `/parallel/`. Skipping TTY tests there due to missing tty fds just means they will never be run, ever, on any system. This moves the tty-get-color-depth test to `/pseudo-tty/` where the test runner will actually make a pty fd. Refs: nodejs#17615 PR-URL: nodejs#18800 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

From now on all error messages produced by `assert` in strict mode will produce a error diff. PR-URL: nodejs#17615 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Instead of having three times the same RegExp, just use a helper. PR-URL: nodejs#17615 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Right now it is very difficult to determine if a terminal supports colors or not. This function adds this functionality by detecting environment variables and checking process. PR-URL: nodejs#17615 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

PR-URL: nodejs#17615 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

TTY tests should almost never be placed in `/parallel/`. Skipping TTY tests there due to missing tty fds just means they will never be run, ever, on any system. This moves the tty-get-color-depth test to `/pseudo-tty/` where the test runner will actually make a pty fd. Refs: nodejs#17615 PR-URL: nodejs#18800 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins · 2018-07-31T18:27:30Z

Requested backport to 8.x in #19230

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Dec 11, 2017

addaleax added assert Issues and PRs related to the assert subsystem. semver-minor PRs that contain new features and should be released in the next minor version. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Dec 11, 2017

Trott reviewed Dec 12, 2017

View reviewed changes

Trott added the wip Issues and PRs that are still a work in progress. label Dec 12, 2017

vsemozhetbyt reviewed Dec 12, 2017

View reviewed changes

BridgeAR added the notable-change PRs with changes that should be highlighted in changelogs. label Dec 15, 2017

BridgeAR force-pushed the improve-error-msg branch 2 times, most recently from 1777777 to 8b75434 Compare December 28, 2017 05:25

BridgeAR removed the wip Issues and PRs that are still a work in progress. label Dec 28, 2017

BridgeAR changed the title ~~[WIP] assert: add error diffs~~ assert: add error diffs Dec 28, 2017

jasnell approved these changes Dec 28, 2017

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 28, 2017

BridgeAR force-pushed the improve-error-msg branch from 8b75434 to b1a5d42 Compare December 29, 2017 18:39

TimothyGu added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jan 13, 2018

mcollina reviewed Jan 15, 2018

View reviewed changes

addaleax reviewed Jan 15, 2018

View reviewed changes

vsemozhetbyt reviewed Jan 16, 2018

View reviewed changes

BridgeAR force-pushed the improve-error-msg branch from 9ece1bf to 820ba9a Compare January 16, 2018 15:36

mcollina approved these changes Jan 16, 2018

View reviewed changes

jasnell approved these changes Jan 16, 2018

View reviewed changes

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 8, 2018

tty: refactor to es6

073cf69

PR-URL: nodejs#17615 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

BridgeAR mentioned this pull request Mar 8, 2018

[9.x backport] assert & util PRs #19230

Closed

4 tasks

MylesBorins pushed a commit that referenced this pull request Mar 15, 2018

tty: refactor to es6

4517e7a

Backport-PR-URL: #19230 PR-URL: #17615 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

targos mentioned this pull request Mar 18, 2018

v9.9.0 proposal #19428

Merged

MylesBorins pushed a commit that referenced this pull request Mar 20, 2018

tty: refactor to es6

74f0d1a

Backport-PR-URL: #19230 PR-URL: #17615 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

oliversalzburg mentioned this pull request Mar 21, 2018

Changelog links to incorrect GH ticket #19516

Closed

sindresorhus reviewed Mar 23, 2018

View reviewed changes

targos added backported-to-v9.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-requested-v9.x labels Mar 24, 2018

papandreou mentioned this pull request Mar 28, 2018

Move the assert module to userland #19652

Closed

addaleax mentioned this pull request Mar 28, 2018

console: add color support #19372

Closed

4 tasks

targos mentioned this pull request Apr 2, 2018

test: improve tty.getColorDepth coverage #19446

Closed

4 tasks

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018

tty: refactor to es6

f0bddf2

PR-URL: nodejs#17615 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

MylesBorins added the backport-requested-v8.x label Jul 31, 2018

BridgeAR deleted the improve-error-msg branch April 1, 2019 23:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

assert: add error diffs #17615

assert: add error diffs #17615

BridgeAR commented Dec 11, 2017 •

edited

Trott Dec 12, 2017 •

edited

vsemozhetbyt Dec 12, 2017

BridgeAR commented Dec 28, 2017 •

edited

jasnell commented Dec 28, 2017

BridgeAR commented Dec 29, 2017

BridgeAR commented Dec 29, 2017

BridgeAR commented Jan 3, 2018

mcollina Jan 15, 2018

addaleax Jan 15, 2018

BridgeAR Jan 15, 2018

addaleax Jan 15, 2018

addaleax Jan 15, 2018

addaleax Jan 15, 2018

BridgeAR commented Jan 16, 2018

vsemozhetbyt Jan 16, 2018 •

edited

BridgeAR commented Jan 16, 2018

mcollina left a comment

BridgeAR commented Mar 8, 2018

sindresorhus Mar 23, 2018

richardlau Mar 23, 2018

BridgeAR Mar 24, 2018 •

edited

BridgeAR Mar 24, 2018

richardlau Mar 24, 2018

joyeecheung Mar 28, 2018

MylesBorins commented Jul 31, 2018

assert: add error diffs #17615

assert: add error diffs #17615

Conversation

BridgeAR commented Dec 11, 2017 • edited

Checklist

Affected core subsystem(s)

Trott Dec 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Dec 28, 2017 • edited

jasnell commented Dec 28, 2017

BridgeAR commented Dec 29, 2017

BridgeAR commented Dec 29, 2017

BridgeAR commented Jan 3, 2018

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

BridgeAR commented Jan 16, 2018

vsemozhetbyt Jan 16, 2018 • edited

Choose a reason for hiding this comment

BridgeAR commented Jan 16, 2018

mcollina left a comment

Choose a reason for hiding this comment

BridgeAR commented Mar 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR Mar 24, 2018 • edited

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

MylesBorins commented Jul 31, 2018

BridgeAR commented Dec 11, 2017 •

edited

Trott Dec 12, 2017 •

edited

BridgeAR commented Dec 28, 2017 •

edited

vsemozhetbyt Jan 16, 2018 •

edited

BridgeAR Mar 24, 2018 •

edited