assert: wrap original err on `ifError` fail #12820

refack · 2017-05-04T03:13:04Z

For assert.ifError fails, wrap the original err in a new AssertionError

Inspiration: #12803 (comment)
/cc @nodejs/testing

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)

test, assert

refack · 2017-05-04T03:14:18Z

~~The message will not change, but the stack trace will 🤔~~

Trott · 2017-05-04T04:49:49Z

It might be the smallest semver-major ever, but I would still say semver-major.

Imagine the impact on someone reading the stack traces if they run the same test on different versions of Node.js (which is what someone might do if they suspect they are triggering a bug in Node.js itself). If assert.ifError() returns a different stack trace in the same exact situation between Node.js 7.10.0 and Node.js 8.0.0, well, that's what happens between major versions. But different results between Node.js 7.10.0 and 7.11.0? I'd be less understanding about that...

addaleax · 2017-05-04T05:24:35Z

Is this a good idea, though? I think I would usually be more interested in the original stack trace (and apart from that, I think that this won’t work if somebody accessed err.stack before ifError() is called) to figure out why there was an error

joyeecheung

We should add a test for this..

jasnell · 2017-05-04T12:37:33Z

I'm not sure this is a good change to make. The errors are created elsewhere and passed here. Changing the stack trace would definitely be a breaking change (even if a mild one). I don't think this is behavior anyone would expect.

refack · 2017-05-04T12:39:30Z

Is this a good idea, though? I think I would usually be more interested in the original stack trace (and apart from that, I think that this won’t work if somebody accessed err.stack before ifError() is called) to figure out why there was an error

One could argue that if you want the original stack, just throw it. If you use assert, you want to knwo which asset failed...
It came to me after I sew this:

c:\code\node$ node --no-deprecation test/parallel/test-fs-chmod.js
33060
assert.js:378
assert.ifError = function ifError(err) { if (err) throw err; };
                                                  ^

Error: EPERM: operation not permitted, open 'd:\code\node\test\fixtures\a1.js'

cjihrig · 2017-05-04T12:45:45Z

I think we should either throw the existing error, as is, or throw a new assertion error. This hybrid seems confusing.

refack · 2017-05-04T12:46:12Z

I think we should either throw the existing error, as is, or throw a new assertion error. This hybrid seems confusing.

Bingo!

refack · 2017-05-04T12:58:38Z

Changed to wrapping the original err in a new AssertionError (per @cjihrig's comment).
Now It's definitely semver-major

jasnell · 2017-05-04T13:28:28Z

Wrapping in the AssertionError works. We will definitely want to run some CITGM tests on this tho

refack · 2017-05-04T14:24:59Z

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/760/

refack · 2017-05-26T02:56:37Z

New CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/833/

richardlau · 2017-05-27T05:53:13Z

lib/assert.js

@@ -546,4 +546,7 @@ function doesNotThrow(block, /*optional*/error, /*optional*/message) {
  _throws(false, block, error, message);
 }

-assert.ifError = function ifError(err) { if (err) throw err; };
+assert.ifError = function ifError(err, message) {


message isn't used.

Ack.
Forgot to carry it over into the last commit.

refack · 2017-05-27T13:51:19Z

CI: https://ci.nodejs.org/job/node-test-commit/10189/

refack · 2017-05-27T17:10:16Z

CI: ~~https://ci.nodejs.org/job/node-test-commit/10191/~~
https://ci.nodejs.org/job/node-test-commit/10192/

refack · 2017-06-02T17:48:10Z

Ping @nodejs/testing

refack · 2017-06-09T16:32:53Z

I now kinda hate the current ifError. Re:

1307	parallel/test-util-callbackify	
duration_ms	0.89
severity	fail
stack	->

assert.js:586
assert.ifError = function ifError(err) { if (err) throw err; };
                                                  ^
function () {
    context.actual++;
    return fn.apply(this, arguments);
  }

BridgeAR · 2017-08-25T02:54:43Z

doc/api/assert.md

-
+Throws an `AssertionError` if `value` is truthy. This is useful when testing the
+`error` argument in callbacks. If the `message` parameter is undefined, a
+default error message is assigned, and `value` is be appended to it.


I feel this is somewhat contradicting each other. If it should be used in callbacks, there should always either be an error or the result but never both at the same time.

BridgeAR · 2017-08-25T02:58:10Z

test/parallel/test-assert.js

+  common.expectsError({
+                code: 'ERR_ASSERTION',
+                type: a.AssertionError,
+                message: /^Error: test_error_slug$/m


I am not sure how this is any different from the test above. I actually would expect this test to fail because I would have expected it to be wrapped in the ifError failed part.

BridgeAR · 2017-08-25T02:59:37Z

lib/assert.js

-assert.ifError = function ifError(err) { if (err) throw err; };
+assert.ifError = function ifError(err, message) {
+  if (!err) return;
+  message = message || `ifError failed. Original error:\n${err}`;


Hm, is ifError really failing here? Maybe smth. like ifError encountered unexpected error: ... might be more intuitive?

BridgeAR · 2017-09-20T00:37:14Z

Ping @refack

BridgeAR · 2017-10-02T12:07:40Z

I am closing this due to the long inactivity. @refack please reopen if you would like to pursue this further.

refack added assert Issues and PRs related to the assert subsystem. test Issues and PRs related to the tests. labels May 4, 2017

nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label May 4, 2017

refack requested a review from addaleax May 4, 2017 03:15

joyeecheung reviewed May 4, 2017

View reviewed changes

refack force-pushed the assrt-iferror branch from d5e9f4d to 29eb4e4 Compare May 4, 2017 12:55

refack added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 4, 2017

refack changed the title ~~assert: generate interesting stack trace~~ assert: wrap original err on ifError fail May 4, 2017

refack mentioned this pull request May 20, 2017

doc: list macOS as supporting filename argument #13111

Closed

3 tasks

refack force-pushed the assrt-iferror branch from 765691d to f6394c0 Compare May 26, 2017 02:55

refack force-pushed the assrt-iferror branch from c626309 to 9726862 Compare May 26, 2017 03:23

refack self-assigned this May 26, 2017

richardlau reviewed May 27, 2017

View reviewed changes

refack force-pushed the assrt-iferror branch from cdc5440 to 62556d7 Compare May 27, 2017 17:06

refack force-pushed the assrt-iferror branch from 62556d7 to c43b3aa Compare May 27, 2017 17:20

assert: wrap original err on ifError fail

482d7b4

refack force-pushed the assrt-iferror branch from c43b3aa to 482d7b4 Compare May 27, 2017 21:14

refack mentioned this pull request May 28, 2017

assert: throw real error in doesNotThrow() #12079

Closed

BridgeAR reviewed Aug 25, 2017

View reviewed changes

BridgeAR closed this Oct 2, 2017

refack removed their assignment Oct 12, 2018

Uh oh!

assert: wrap original err on ifError fail #12820

assert: wrap original err on ifError fail #12820

Uh oh!

Conversation

refack commented May 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

refack commented May 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented May 4, 2017

Uh oh!

addaleax commented May 4, 2017

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented May 4, 2017

Uh oh!

refack commented May 4, 2017

Uh oh!

cjihrig commented May 4, 2017

Uh oh!

refack commented May 4, 2017

Uh oh!

refack commented May 4, 2017

Uh oh!

jasnell commented May 4, 2017

Uh oh!

refack commented May 4, 2017

Uh oh!

refack commented May 26, 2017

Uh oh!

richardlau May 27, 2017

Choose a reason for hiding this comment

Uh oh!

refack May 27, 2017

Choose a reason for hiding this comment

Uh oh!

refack commented May 27, 2017

Uh oh!

refack commented May 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented Jun 2, 2017

Uh oh!

refack commented Jun 9, 2017

Uh oh!

BridgeAR Aug 25, 2017

Choose a reason for hiding this comment

Uh oh!

BridgeAR Aug 25, 2017

Choose a reason for hiding this comment

Uh oh!

BridgeAR Aug 25, 2017

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Sep 20, 2017

Uh oh!

BridgeAR commented Oct 2, 2017

Uh oh!

Uh oh!

assert: wrap original err on `ifError` fail #12820

assert: wrap original err on `ifError` fail #12820

refack commented May 4, 2017 •

edited

Loading

refack commented May 4, 2017 •

edited

Loading

refack commented May 27, 2017 •

edited

Loading