assert: improve error messages #19467

BridgeAR · 2018-03-20T01:58:07Z

This switches all assert error messages to use the new diffing output as in the assert strict mode.

It also makes sure not reference equal objects are now visualised as such. Before it would e.g. have been [1, 2] strictEqual [1, 2] and that would be quite confusing. That is now detected and expressed.

I also added support in util.inspect to visualize arguments to distinguish them from a regular object.

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

BridgeAR · 2018-03-20T01:59:49Z

~~CI https://ci.nodejs.org/job/node-test-pull-request/13754/~~
~~CI https://ci.nodejs.org/job/node-test-pull-request/13756/~~
CI https://ci.nodejs.org/job/node-test-pull-request/13759/

BridgeAR · 2018-03-20T11:38:52Z

~~I am uncertain why the CI fails. Could someone please take a look?~~

BridgeAR · 2018-03-20T11:42:22Z

CITGM PR https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1338/
CITGM master https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1339/

BridgeAR · 2018-03-20T16:42:07Z

Seems like this breaks eslint tests. @not-an-aardvark can you PTAL?

It does not seem like a big problem to me but I might be wrong.

Here is a small patch that might fix it (without verifying it for real):

@@ -439,7 +439,7 @@ class RuleTester {
                     `Expected '${actual}' to match ${expected}`
                 );
             } else {
-                assert.strictEqual(actual, expected);
+                assert.strictEqual(actual, expected, util.format("%s strictEqual %s", actual, expected));
             }
         }

Looking at it further, it might make sense to change some more assert things as well like e.g.:

diff --git a/lib/testers/rule-tester.js b/lib/testers/rule-tester.js
index de218a875..798253368 100644
--- a/lib/testers/rule-tester.js
+++ b/lib/testers/rule-tester.js
@@ -396,11 +396,11 @@ class RuleTester {
          * @private
          */
         function assertASTDidntChange(beforeAST, afterAST) {
-            if (!lodash.isEqual(beforeAST, afterAST)) {
-
-                // Not using directly to avoid performance problem in node 6.1.0. See #6111
-                // eslint-disable-next-line no-restricted-properties
-                assert.deepEqual(beforeAST, afterAST, "Rule should not modify AST.");
+            if (util.isDeepStrictEqual) {
+                assert.deepStrictEqual(beforeAST, afterAST, "Rule should not modify AST.");
+                // Not using directly to avoid performance problem in Node.js 6 and 7. See #6111
+            } else if (!lodash.isEqual(beforeAST, afterAST)) {
+                assert.fail("Rule should not modify AST.");
             }
         }
 
@@ -539,7 +539,7 @@ class RuleTester {
                     } else {
 
                         // Message was an unexpected type
-                        assert.fail(message, null, "Error should be a string, object, or RegExp.");
+                        assert.fail("Error should be a string, object, or RegExp.");
                     }
                 }
             }
@@ -554,8 +554,7 @@ class RuleTester {
                 } else {
                     const fixResult = SourceCodeFixer.applyFixes(item.code, messages);
 
-                    // eslint-disable-next-line no-restricted-properties
-                    assert.equal(fixResult.output, item.output, "Output is incorrect.");
+                    assert.strictEqual(fixResult.output, item.output, "Output is incorrect.");
                 }
             }

not-an-aardvark · 2018-03-21T06:08:48Z

I agree that the eslint test failure is not a big problem (it looks like no significant functionality is broken).

Presumably, eslint users would also benefit from improved assertion error messages, so I'm not sure it would be a good idea to lock the error messages within eslint just to get the existing tests to pass. I suppose we could do feature-detection in the tests to check what error message to expect, e.g.:

assert.strictEqual(message, new assert.AssertionError({ actual: 5, expected: 'foo', operator: '===' }).message);

Trott · 2018-03-21T14:02:35Z

If we do feature detection, maybe include a comment about when we can drop the feature detection. (In other words, "this feature detection code won't be needed anymore after we drop support for Node.js 8.0.0" or whatever. That assumes it's possible to know that version number at this time...)

mcollina

LGTM

BridgeAR · 2018-03-25T02:24:41Z

Rebased due to conflicts.

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

BridgeAR

I updated the code (rebased due to conflicts) and fixed some small bugs (e.g. errors with extra properties did not have a proper error message), removed the errorDiff property` and added some proxy tests.

BridgeAR · 2018-04-02T20:24:44Z

lib/internal/errors.js

+      breakLength: Infinity,
+      // Assert does not detect proxies currently.
+      showProxy: false
+    }


Note: this is actually relatively important to make sure the errors are actually printed properly, no matter how the default options where changed. Only the color setting is not important.

BridgeAR · 2018-04-02T20:25:29Z

lib/internal/errors.js

+  Object.defineProperty(target, 'message', { value: source.message });
+  Object.setPrototypeOf(target, Object.getPrototypeOf(source));
+  return target;
+}


This was the easiest and best way to pretend to be an error without a stack trace.

BridgeAR · 2018-04-02T20:25:58Z

lib/internal/errors.js

+  notDeepStrictEqual: 'Input A expected to strictly not deep equal input B',
+  strictEqual: 'Input A expected to strictly equal input B',
+  notStrictEqual: 'Input A expected to strictly not equal input B'
+};


@Trott @vsemozhetbyt PTAL.

BridgeAR · 2018-04-02T20:26:52Z

lib/internal/errors.js

+      // This code path should not be possible to reach.
+      // The output is identical but it is not clear why.
+      base = 'Input objects not identical:';
+    }


This has happened before and there might still be some rare cases that I am not aware about that could produce this weird output. I am not sure how to handle that better though.

BridgeAR · 2018-04-02T20:27:29Z

lib/internal/errors.js

+          'stack' in actual && actual instanceof Error &&
+          'stack' in expected && expected instanceof Error) {
+        actual = copyError(actual);
+        expected = copyError(expected);
      }


I think it is best to actually show the stack trace in the comparison in case there is only a single error. That way it is clearer in what way the input is different from each other.

BridgeAR · 2018-04-02T20:28:47Z

test/parallel/test-assert-deep.js

+  const arrProxy = new Proxy([1, 2], {});
+  assert.deepStrictEqual(arrProxy, [1, 2]);
+  const tmp = util.inspect.defaultOptions;
+  util.inspect.defaultOptions = { showProxy: true };


Any opinions from others about checking for proxies?

BridgeAR · 2018-04-02T22:10:57Z

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

BridgeAR · 2018-04-02T22:12:33Z

@jasnell @mcollina PTAL

jasnell · 2018-04-02T22:16:54Z

Still lgtm

Trott · 2018-04-02T22:59:58Z

lib/internal/errors.js

@@ -19,6 +19,13 @@ let green = '';
 let red = '';
 let white = '';

+const READABLE_OPERATOR = {
+  deepStrictEqual: 'Input A expected to strictly deep equal input B',


maybe strictly deep-equal?

Trott · 2018-04-02T23:00:16Z

lib/internal/errors.js

@@ -19,6 +19,13 @@ let green = '';
 let red = '';
 let white = '';

+const READABLE_OPERATOR = {
+  deepStrictEqual: 'Input A expected to strictly deep equal input B',
+  notDeepStrictEqual: 'Input A expected to strictly not deep equal input B',


Same here? deep equal -> deep-equal?

Trott · 2018-04-02T23:02:02Z

Not sure how I feel about Input A and Input B being used for "first argument" and "second argument". Maybe Argument 1 and Argument 2 would be less problematic? (Or even just going full verbose and using first argument and second argument?)

BridgeAR · 2018-04-04T13:26:29Z

I addressed the comment. @Trott I would like to keep Input A / B for now as that could be changed in a different PR. Are you OK with that, so this can land?

Trott · 2018-04-04T17:42:02Z

I addressed the comment. @Trott I would like to keep Input A / B for now as that could be changed in a different PR. Are you OK with that, so this can land?

Yes, that's fine by me.

BridgeAR · 2018-04-04T17:51:02Z

It would be nice to get another LG before landing.

Trott · 2018-04-04T18:21:20Z

test/pseudo-tty/test-assert-colors.js

@@ -7,7 +7,7 @@ try {
  process.env.COLORTERM = '1';
  assert.deepStrictEqual([1, 2], [2, 2]);
 } catch (err) {
-  const expected = 'Input A expected to deepStrictEqual input B:\n' +
+  const expected = 'Input A expected to strictly deep equal input B:\n' +


Same question as above: Shouldn't this fail because it should be expecting deep-equal?

BridgeAR · 2018-04-04T19:43:56Z

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

BridgeAR · 2018-04-09T17:48:55Z

@nodejs/testing @nodejs/tsc PTAL. I would like to land this soon.

Trott · 2018-04-09T21:07:59Z

Since it's semver-major, it needs a CITGM run, if I'm not mistaken.

BridgeAR · 2018-04-10T01:00:16Z

@Trott I did that already in the beginning. It breaks the eslint tests and I opened a PR in eslint to address that. Besides that there was nothing and that should not be blocking.

Trott · 2018-04-10T02:53:18Z

@Trott I did that already in the beginning.

Not sure how I missed that. Sorry!

BridgeAR · 2018-04-12T11:15:11Z

If there is no objection until then, I am going to land this in ~30 hours.

targos · 2018-04-12T16:16:04Z

No objections from me

Trott · 2018-04-12T20:51:09Z

Re-running failed Windows CI: https://ci.nodejs.org/job/node-test-commit-windows-fanned/17190/

Right now it is not possible to distinguish arguments from a regular object. This adds a arguments indicator.

This improves the error messages for: - assert.notDeepStrictEqual - assert.deepStrictEqual - assert.notStrictEqual - assert.strictEqual Those will now always use the same error message as used in the strict mode.

The property is not necessary as it is possible to check for the operator instead.

BridgeAR · 2018-04-13T19:07:37Z

Rebased due to minor conflicts in the tests.

One more CI before landing: https://ci.nodejs.org/job/node-test-pull-request/14266/

mcollina · 2018-04-14T09:12:50Z

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1361/
CITGM (master): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1362/

mcollina · 2018-04-14T09:20:05Z

Still LGTM

jasnell · 2018-04-14T17:44:51Z

@nodejs/tsc ... any objections to this landing in 10.0.0?

Right now it is not possible to distinguish arguments from a regular object. This adds a arguments indicator. PR-URL: #19467 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This improves the error messages for: - assert.notDeepStrictEqual - assert.deepStrictEqual - assert.notStrictEqual - assert.strictEqual Those will now always use the same error message as used in the strict mode. PR-URL: #19467 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

The property is not necessary as it is possible to check for the operator instead. PR-URL: #19467 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell · 2018-04-14T17:46:12Z

Landed in f2d112c, bfe54df, and 2b0825e

mcollina · 2018-04-14T17:52:19Z

+1 for me

Right now it is not possible to distinguish arguments from a regular object. This adds a arguments indicator. PR-URL: #19467 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This improves the error messages for: - assert.notDeepStrictEqual - assert.deepStrictEqual - assert.notStrictEqual - assert.strictEqual Those will now always use the same error message as used in the strict mode. PR-URL: #19467 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

The property is not necessary as it is possible to check for the operator instead. PR-URL: #19467 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 20, 2018

nodejs-github-bot added assert Issues and PRs related to the assert subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. util Issues and PRs related to the built-in util module. labels Mar 20, 2018

not-an-aardvark mentioned this pull request Mar 21, 2018

Update tests to be more resilient against assert message changes in Node core eslint/eslint#10111

Closed

mcollina approved these changes Mar 22, 2018

View reviewed changes

jasnell approved these changes Mar 23, 2018

View reviewed changes

BridgeAR force-pushed the improve-error-messages branch from 802d12e to bc52f4d Compare March 25, 2018 02:24

BridgeAR force-pushed the improve-error-messages branch 3 times, most recently from 4b06f51 to ec5e5c1 Compare April 2, 2018 22:07

BridgeAR commented Apr 2, 2018

View reviewed changes

BridgeAR requested a review from joyeecheung April 2, 2018 22:11

Trott reviewed Apr 2, 2018

View reviewed changes

BridgeAR requested a review from a team April 4, 2018 17:50

Trott reviewed Apr 4, 2018

View reviewed changes

BridgeAR mentioned this pull request Apr 5, 2018

test: improve common.expectsError #19797

Closed

4 tasks

BridgeAR requested a review from targos April 12, 2018 16:00

BridgeAR added 3 commits April 13, 2018 21:04

util: inspect arguments properly

d808ca2

Right now it is not possible to distinguish arguments from a regular object. This adds a arguments indicator.

assert: improve default error messages

87a3657

This improves the error messages for: - assert.notDeepStrictEqual - assert.deepStrictEqual - assert.notStrictEqual - assert.strictEqual Those will now always use the same error message as used in the strict mode.

assert: remove errorDiff property

611ddd2

The property is not necessary as it is possible to check for the operator instead.

BridgeAR force-pushed the improve-error-messages branch from 6ae2061 to 611ddd2 Compare April 13, 2018 19:05

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

jasnell closed this Apr 14, 2018

BridgeAR deleted the improve-error-messages branch January 20, 2020 11:30

assert: improve error messages #19467

assert: improve error messages #19467

Conversation

BridgeAR commented Mar 20, 2018 • edited Loading

Checklist

BridgeAR commented Mar 20, 2018 • edited Loading

BridgeAR commented Mar 20, 2018 • edited Loading

BridgeAR commented Mar 20, 2018

BridgeAR commented Mar 20, 2018

not-an-aardvark commented Mar 21, 2018

Trott commented Mar 21, 2018 • edited Loading

mcollina left a comment

Choose a reason for hiding this comment

BridgeAR commented Mar 25, 2018

BridgeAR left a comment • 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

BridgeAR commented Apr 2, 2018

BridgeAR commented Apr 2, 2018

jasnell commented Apr 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Apr 2, 2018 • edited Loading

BridgeAR commented Apr 4, 2018

Trott commented Apr 4, 2018

BridgeAR commented Apr 4, 2018

Choose a reason for hiding this comment

BridgeAR commented Apr 4, 2018

BridgeAR commented Apr 9, 2018

Trott commented Apr 9, 2018

BridgeAR commented Apr 10, 2018

Trott commented Apr 10, 2018

BridgeAR commented Apr 12, 2018

targos commented Apr 12, 2018

Trott commented Apr 12, 2018

BridgeAR commented Apr 13, 2018

mcollina commented Apr 14, 2018

mcollina commented Apr 14, 2018

jasnell commented Apr 14, 2018

jasnell commented Apr 14, 2018

mcollina commented Apr 14, 2018

BridgeAR commented Mar 20, 2018 •

edited

Loading

BridgeAR commented Mar 20, 2018 •

edited

Loading

BridgeAR commented Mar 20, 2018 •

edited

Loading

Trott commented Mar 21, 2018 •

edited

Loading

BridgeAR left a comment •

edited

Loading

Trott commented Apr 2, 2018 •

edited

Loading