assert: improve ifError #18247

BridgeAR · 2018-01-19T09:52:49Z

Commit:

It is hard to know where ifError is actually triggered due to the
original error being thrown.
This changes it by wrapping the original error in a AssertionError.
This has the positive effect of also making clear that it is indeed
a assertion function that triggered that error.

The original stack can still be accessed by checking the actual
property.

Commit:

Show only the Error class and not errors.AssertionError.

Commit:

Make ifError stricter by only accepting null and undefined from now on.
Before any truthy value was accepted.

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

BridgeAR · 2018-01-19T09:54:28Z

test/parallel/test-assert-if-error.js

+assert.doesNotThrow(() => { assert.ifError(null); });
+assert.doesNotThrow(() => { assert.ifError(); });
+assert.doesNotThrow(() => { assert.ifError(undefined); });
+assert.doesNotThrow(() => { assert.ifError(false); });


What do others think about actually throwing in such a case? I personally think it would be best to only accept null and undefined.

I agree, it's already semver-major so let's do it. Can you also update the PR description with this change?

BridgeAR · 2018-01-19T09:54:39Z

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

mcollina

I'm thinking this might not be a good idea.
If an error we didn't want is passed, I think we prefer to see the original stacktrace and not the new one.

mcollina · 2018-01-19T14:52:30Z

test/fixtures/uncaught-exceptions/callbackify2.js

 // Calling test will use `stdout` to assert value of `err.message`
 console.log(err.message);
 });

 async function fn() {
- return await Promise.reject(sentinel);
+ return Promise.reject(sentinel);


if you remove await there is no need for this to be an async function.

Hm, true. I am not sure why it was a async function in the first place. I guess a oversight.

BridgeAR · 2018-01-19T15:11:12Z

@mcollina about the changed stack trace: you should have both. You can access the original stack trace by checking the actual property.

I could also combine the stack traces (I thought about it before but I rejected it again) if that feels better to others.

mcollina · 2018-01-19T15:16:30Z

If you put it in actual, most testing frameworks would not print it by default. Then, the information will be lost.

BridgeAR · 2018-01-22T23:59:35Z

@mcollina I rewrote this to extend the actual error with the necessary frames. So now you get a combined stack that is way more informative than before.

BridgeAR · 2018-01-23T00:06:51Z

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

mcollina · 2018-01-23T12:04:56Z

lib/assert.js

+ }
+ }
+ newErr.stack = `${tmp1.join('\n')}\n${tmp2.join('\n')}`;
+ }


the goal of this code is not really clear, can you add some comments?

mcollina · 2018-01-23T12:05:53Z

test/parallel/test-assert-if-error.js

+ assert(threw);
+ })();
+ })();
+})();


I would prefer a test showing a full stack trace, rather than this check. It would mean future edits are easier to understand and fix.

I added a separate test to check for the stack trace and minimized this test to the necessary parts.

mcollina · 2018-01-23T12:06:40Z

test/parallel/test-assert-if-error.js

+assert.doesNotThrow(() => { assert.ifError(null); });
+assert.doesNotThrow(() => { assert.ifError(); });
+assert.doesNotThrow(() => { assert.ifError(undefined); });
+assert.doesNotThrow(() => { assert.ifError(false); });


I agree, it's already semver-major so let's do it. Can you also update the PR description with this change?

It is hard to know where ifError is actually triggered due to the original error being thrown. This changes it by wrapping the original error in a AssertionError. This has the positive effect of also making clear that it is indeed a assertion function that triggered that error. The original stack can still be accessed by checking the `actual` property.

Destructure the necessary Error classes from internal/errors. This improves the readability of the error creation.

This makes `assert.ifError` stricter by only accepting `null` and `undefined` from now on. Before any truthy value was accepted.

mcollina

LGTM

It is hard to know where ifError is actually triggered due to the original error being thrown. This changes it by wrapping the original error in a AssertionError. This has the positive effect of also making clear that it is indeed a assertion function that triggered that error. The original stack can still be accessed by checking the `actual` property. PR-URL: nodejs#18247 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Destructure the necessary Error classes from internal/errors. This improves the readability of the error creation. PR-URL: nodejs#18247 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This makes `assert.ifError` stricter by only accepting `null` and `undefined` from now on. Before any truthy value was accepted. PR-URL: nodejs#18247 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

BridgeAR · 2018-01-24T12:16:36Z

Landed in 7a23fc0...e65a6e8

Trott · 2018-01-25T17:21:08Z

Looks like this may have landed without a clean CI run? In any event, the message test added here seems to be failing 100% of the time on Windows CI. (Am I wrong somehow about that?) Is there a PR to fix that or should we revert until it's fixed?

Trott · 2018-01-25T17:23:50Z

Oh, I see the problem. It's expecting forward slashes, but on Windows, the path separator is (of course) backslashes. Opening a PR to fix.

apapirovski · 2018-01-25T17:24:26Z

@Trott fix already in #18378

Destructure the necessary Error classes from internal/errors. This improves the readability of the error creation. PR-URL: nodejs#18247 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Destructure the necessary Error classes from internal/errors. This improves the readability of the error creation. Backport-PR-URL: #19230 PR-URL: #18247 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

It is hard to know where ifError is actually triggered due to the original error being thrown. This changes it by wrapping the original error in a AssertionError. This has the positive effect of also making clear that it is indeed a assertion function that triggered that error. The original stack can still be accessed by checking the `actual` property. PR-URL: nodejs#18247 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Destructure the necessary Error classes from internal/errors. This improves the readability of the error creation. PR-URL: nodejs#18247 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This makes `assert.ifError` stricter by only accepting `null` and `undefined` from now on. Before any truthy value was accepted. PR-URL: nodejs#18247 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

BridgeAR added assert Issues and PRs related to the assert subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. notable-change PRs with changes that should be highlighted in changelogs. labels Jan 19, 2018

nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label Jan 19, 2018

BridgeAR commented Jan 19, 2018

View reviewed changes

mcollina requested changes Jan 19, 2018

View reviewed changes

BridgeAR requested review from apapirovski, jasnell and addaleax January 19, 2018 15:19

mcollina reviewed Jan 23, 2018

View reviewed changes

BridgeAR added 3 commits January 23, 2018 14:09

assert: use destructuring for errors

56403f7

Destructure the necessary Error classes from internal/errors. This improves the readability of the error creation.

assert: stricter ifError

34f02d9

This makes `assert.ifError` stricter by only accepting `null` and `undefined` from now on. Before any truthy value was accepted.

BridgeAR force-pushed the improve-assert-iferror branch from 58d658c to 34f02d9 Compare January 23, 2018 13:09

BridgeAR changed the title ~~assert: wrap original error in ifError~~ assert: improve ifError Jan 23, 2018

mcollina approved these changes Jan 23, 2018

View reviewed changes

jasnell approved these changes Jan 23, 2018

View reviewed changes

BridgeAR closed this Jan 24, 2018

targos mentioned this pull request Mar 18, 2018

v9.9.0 proposal #19428

Merged

BridgeAR deleted the improve-assert-iferror branch April 1, 2019 23:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

assert: improve ifError #18247

assert: improve ifError #18247

BridgeAR commented Jan 19, 2018 •

edited

Loading

BridgeAR Jan 19, 2018

mcollina Jan 23, 2018

BridgeAR Jan 23, 2018

BridgeAR commented Jan 19, 2018

mcollina left a comment

mcollina Jan 19, 2018

BridgeAR Jan 19, 2018

BridgeAR commented Jan 19, 2018

mcollina commented Jan 19, 2018

BridgeAR commented Jan 22, 2018

BridgeAR commented Jan 23, 2018

mcollina Jan 23, 2018

BridgeAR Jan 23, 2018

mcollina Jan 23, 2018

BridgeAR Jan 23, 2018

mcollina Jan 23, 2018

mcollina left a comment

BridgeAR commented Jan 24, 2018

Trott commented Jan 25, 2018

Trott commented Jan 25, 2018

apapirovski commented Jan 25, 2018

assert: improve ifError #18247

assert: improve ifError #18247

Conversation

BridgeAR commented Jan 19, 2018 • edited Loading

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Jan 19, 2018

mcollina left a 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 19, 2018

mcollina commented Jan 19, 2018

BridgeAR commented Jan 22, 2018

BridgeAR commented Jan 23, 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

mcollina left a comment

Choose a reason for hiding this comment

BridgeAR commented Jan 24, 2018

Trott commented Jan 25, 2018

Trott commented Jan 25, 2018

apapirovski commented Jan 25, 2018

BridgeAR commented Jan 19, 2018 •

edited

Loading