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

assert: add error diffs #17615

Closed
wants to merge 5 commits into from
Closed

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Dec 11, 2017

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

@nodejs-github-bot 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 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
@@ -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
Copy link
Member

@Trott Trott Dec 12, 2017

Choose a reason for hiding this comment

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

Nit: a errror diff -> an error diff

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

@Trott Trott added the wip Issues and PRs that are still a work in progress. label Dec 12, 2017
It can be accessed using:

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

Example error diff:

```diff
Copy link
Contributor

Choose a reason for hiding this comment

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

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

  2. 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 BridgeAR added the notable-change PRs with changes that should be highlighted in changelogs. label Dec 15, 2017
@BridgeAR BridgeAR force-pushed the improve-error-msg branch 2 times, most recently from 1777777 to 8b75434 Compare December 28, 2017 05:25
@BridgeAR BridgeAR removed the wip Issues and PRs that are still a work in progress. label Dec 28, 2017
@BridgeAR BridgeAR changed the title [WIP] assert: add error diffs assert: add error diffs Dec 28, 2017
@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 28, 2017

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
Copy link
Member

jasnell commented Dec 28, 2017

@BridgeAR 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
Copy link
Member Author

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
Copy link
Member Author

@BridgeAR
Copy link
Member Author

BridgeAR commented Jan 3, 2018

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

@TimothyGu 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
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';
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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.

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';
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member Author

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

doc/api/util.md Outdated
added: REPLACEME
-->

* `stream` {Stream} A [stream][]. Defaults to `process.stdout`.
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Jan 16, 2018

Choose a reason for hiding this comment

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

[stream][] — Missing bottom reference?

@BridgeAR
Copy link
Member Author

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

I also addressed the doc comment.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 8, 2018
PR-URL: nodejs#17615
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Mar 8, 2018
4 tasks
@BridgeAR
Copy link
Member Author

BridgeAR commented Mar 8, 2018

Backport opened in #19230

MylesBorins pushed a commit that referenced this pull request Mar 15, 2018
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>
MylesBorins pushed a commit that referenced this pull request Mar 15, 2018
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>
MylesBorins pushed a commit that referenced this pull request Mar 15, 2018
Backport-PR-URL: #19230
PR-URL: #17615
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@targos targos mentioned this pull request Mar 18, 2018
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
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>
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
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>
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
Backport-PR-URL: #19230
PR-URL: #17615
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins added a commit that referenced this pull request Mar 21, 2018
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
MylesBorins added a commit that referenced this pull request Mar 21, 2018
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
return COLORS_16;

return COLORS_2;
};

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@BridgeAR can you comment on the above?

Copy link
Member Author

@BridgeAR BridgeAR Mar 24, 2018

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ping @isaacs

Copy link
Member

Choose a reason for hiding this comment

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

I guess also cc @nodejs/tsc FYI

Copy link
Member

Choose a reason for hiding this comment

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

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

@targos 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
@addaleax addaleax mentioned this pull request Mar 28, 2018
4 tasks
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 1, 2018
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>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
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>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
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>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
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>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
PR-URL: nodejs#17615
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
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
Copy link
Contributor

Requested backport to 8.x in #19230

@BridgeAR BridgeAR deleted the improve-error-msg branch April 1, 2019 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.