assert: add error diffs by BridgeAR · Pull Request #17615 · nodejs/node · GitHub

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

Nit: a errror diff -> an error diff

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

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?

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

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

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.

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

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

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.

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

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.

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

@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.

[stream][] — Missing bottom reference?

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

I also addressed the doc comment.

LGTM

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

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

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>

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 outstanding review comments, 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 outstanding review comments, and a CI started. and removed author ready PRs that have at least one approval, no outstanding review comments, 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 outstanding review comments, 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

+                  return COLORS_16;
+                return COLORS_2;
+              };

assert: add error diffs by BridgeAR · Pull Request #17615 · nodejs/node · GitHub

Uh oh!

Uh oh!

assert: add error diffs - #17615

assert: add error diffs#17615BridgeAR wants to merge 5 commits intonodejs:masternodejs/node:masterfrom BridgeAR:improve-error-msgBridgeAR/node:improve-error-msgCopy head branch name to clipboard

Conversation

BridgeAR commented Dec 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Trott Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Dec 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Dec 28, 2017

Uh oh!

BridgeAR commented Dec 29, 2017

Uh oh!

BridgeAR commented Dec 29, 2017

Uh oh!

BridgeAR commented Jan 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Jan 16, 2018

Uh oh!

vsemozhetbyt Jan 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Jan 16, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Mar 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR Mar 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MylesBorins commented Jul 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

assert: add error diffs#17615
BridgeAR wants to merge 5 commits into
nodejs:masterfrom
BridgeAR:improve-error-msg

BridgeAR commented Dec 11, 2017 •
edited
Loading

Trott Dec 12, 2017 •
edited
Loading

BridgeAR commented Dec 28, 2017 •
edited
Loading

vsemozhetbyt Jan 16, 2018 •
edited
Loading

BridgeAR Mar 24, 2018 •
edited
Loading