assert: wrap original err on `ifError` fail by refack · Pull Request #12820 · nodejs/node · GitHub

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

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

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

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

We should add a test for this..

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.

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'

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

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

Bingo!

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

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

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

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

message isn't used.

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

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

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

Ping @nodejs/testing

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);
  }

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.

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.

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

Ping @refack

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

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

-              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}`;

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

assert: wrap original err on `ifError` fail by refack · Pull Request #12820 · nodejs/node · GitHub

Uh oh!

Uh oh!

assert: wrap original err on ifError fail - #12820

assert: wrap original err on ifError fail#12820refack wants to merge 1 commit intonodejs:masternodejs/node:masterfrom refack:assrt-iferrorrefack/node:assrt-iferrorCopy head branch name to clipboard

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

assert: wrap original err on ifError fail#12820
refack wants to merge 1 commit into
nodejs:masterfrom
refack:assrt-iferror

refack commented May 4, 2017 •
edited
Loading

refack commented May 4, 2017 •
edited
Loading

refack commented May 27, 2017 •
edited
Loading