lib: throw a special error in internal/assert #26635

joyeecheung · 2019-03-13T15:21:41Z

Instead of using the public AssertionError, use a simplified
error that describes potential causes of these assertions
and suggests the user to open an issue.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

nodejs-github-bot · 2019-03-13T15:21:44Z

@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2892/pipeline

BridgeAR

LGTM, I would just rather not use the helper function.

BridgeAR · 2019-03-13T15:40:17Z

test/common/index.js

+    name: 'Error [ERR_INTERNAL_ASSERTION]',
+    code: 'ERR_INTERNAL_ASSERTION'
+  });
+}


I would rather not add this helper function in common. Common is AFAIC pretty overboarded.

That's where common.expectsError is...I guess we could move these and other expectsSomething to one file when the time comes

Also, surprisingly, test/common.js is even less than 1000 lines? That's even smaller than a lot of our tests :S

It's not about having a lot of lines but this file is loaded for every single test file and I would rather completely get rid of something that we always use.

Having something more specific would be fine but I actually doubt that this will often be tested for and currently there are only for places in which this helper would be used in.

About common.expectsError() it is already used significantly less as we can always use assert.throws() instead. Only callbacks should always use common.expectsError().

Do you strongly prefer the helper function? To me it still does not seem necessary.

I believe the consensus is you don't need to validate the message of errors in more then one place (in this case test/message/internal_assert.js is enough), so there's no need for the value that you are DRYing in the first place.

@refack This is special in that the message is what we want to test for in the test cases, because the code is constant across all internal assertions and we intentionally do not give error codes for assertions coming out of completely different type of bugs.

For instance, we want to verify that we tell the user handle must be a FSEvent if the user monkey patches watcher._handle incorrectly, but we do not want to create a code for that special type of bugs because the user is simply violating the API contract and it gives us little to classify all these violations and assign permanent error codes instead of just telling them no. However it's still useful to verify that we hint something helpful in the message.

I think it's important to make sure we provide useful information in the assertions, even for our own sake - just making sure that we say "you are hitting a bug, please open an issue" is not enough, see all the useless error messages in #26798 before #26859 (though that one is not verifiable in user land).

This is special in that the message is what we want to test for in the test cases,

What I mean there's no need to validate the message formatting mechanism more then once.
For example you can store the specific prefix behind a Symbol field and validate just it.

My concern about common/index is only about the requirement of a common helper function. I would love to remove the requirement for our test files at some point.

I already gave my LG, so from my side this can land as is.

doc/api/errors.md

joyeecheung · 2019-03-18T17:04:03Z

Rebased & addressed doc reviews: https://ci.nodejs.org/job/node-test-pull-request/21638/

BridgeAR · 2019-03-25T17:16:51Z

@joyeecheung this requires a rebase.

nodejs-github-bot · 2019-03-26T20:42:29Z

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

nodejs-github-bot · 2019-03-27T14:28:42Z

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

joyeecheung · 2019-03-27T14:28:44Z

Made the paths in message tests play nice with Windows

nodejs-github-bot · 2019-03-29T20:26:01Z

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

refack · 2019-03-31T02:53:57Z

I'm thinking about this some more... I believe this is a big change that deserves some design review. For example:

Are these exceptions recoverable? Maybe they should crash the process?
If the message is unique we are circling back to the if (e.message is something) { something specific } issue... How do we avoid that?

So essentially IMO this is a semver-major change...

joyeecheung · 2019-03-31T04:13:55Z

TBH this is just a drive-by change for me, what I had in mind is more along the lines of “oh, this is entirely internal, why not just say something more helpful, we should be free to change this and are totally free to regret about it if it’s a bad idea”. I don’t personally think it’s worth the time to be pedantic about it, so if anyone has any strong opinions about what’s changed here, feel free to take it on.

BridgeAR · 2019-04-03T02:41:20Z

@refack I definitely disagree that this is semver-major. If anyone runs into one of these assertions, something must be seriously broken. We only use the assertions as safeguards without documenting any of them as in giving a guarantee for the error message.

It is also out of scope of this PR if the exceptions are recoverable or not. That could be looked at in another PR or issue but that's definitely something independent for me.

BridgeAR · 2019-04-07T09:34:18Z

@refack is your comment blocking? If not, I'd actually like to go ahead and land this as is.

BridgeAR · 2019-04-09T14:51:59Z

I would actually like to land this some time soon. @refack is your comment blocking?

BridgeAR · 2019-04-14T12:56:38Z

I'll land this on the 16.04. if there's no hard objection until then. // @refack

Instead of using the public AssertionError, use a simplified error that describes potential causes of these assertions and suggests the user to open an issue. PR-URL: nodejs#26635 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

BridgeAR · 2019-04-22T02:07:24Z

~~Landed in e9022df 🎉~~

BridgeAR · 2019-04-22T02:56:19Z

I had to force push this out again, since the tests failed due to recent changes.

Instead of using the public AssertionError, use a simplified error that describes potential causes of these assertions and suggests the user to open an issue.

nodejs-github-bot · 2019-04-22T16:16:42Z

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

BridgeAR · 2019-04-24T23:32:51Z

Landed in b9f1e57 🎉

Instead of using the public AssertionError, use a simplified error that describes potential causes of these assertions and suggests the user to open an issue. PR-URL: nodejs#26635 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Instead of using the public AssertionError, use a simplified error that describes potential causes of these assertions and suggests the user to open an issue. PR-URL: #26635 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

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. labels Mar 13, 2019

BridgeAR approved these changes Mar 13, 2019

View reviewed changes

vsemozhetbyt reviewed Mar 14, 2019

View reviewed changes

doc/api/errors.md Outdated Show resolved Hide resolved

joyeecheung force-pushed the internal-assert branch from 52c6873 to ac92395 Compare March 18, 2019 17:03

joyeecheung force-pushed the internal-assert branch from ac92395 to f38b910 Compare March 26, 2019 20:41

This was referenced Apr 3, 2019

stream: use an assertion for weird error cases #27073

Closed

stream: remove TODO and add a description instead #27086

Closed

BridgeAR added the review wanted PRs that need reviews. label Apr 9, 2019

BridgeAR closed this Apr 22, 2019

BridgeAR reopened this Apr 22, 2019

joyeecheung and others added 2 commits April 22, 2019 17:47

lib: throw a special error in internal/assert

902c69f

Instead of using the public AssertionError, use a simplified error that describes potential causes of these assertions and suggests the user to open an issue.

fixup! lib: throw a special error in internal/assert

98e873f

BridgeAR force-pushed the internal-assert branch from 9888451 to 98e873f Compare April 22, 2019 16:15

BridgeAR closed this Apr 24, 2019

targos mentioned this pull request Apr 27, 2019

v12.1.0 proposal #27440

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: throw a special error in internal/assert #26635

lib: throw a special error in internal/assert #26635

joyeecheung commented Mar 13, 2019

nodejs-github-bot commented Mar 13, 2019

BridgeAR left a comment

BridgeAR Mar 13, 2019

joyeecheung Mar 13, 2019

joyeecheung Mar 13, 2019

BridgeAR Mar 13, 2019

BridgeAR Mar 25, 2019

refack Mar 31, 2019

joyeecheung Mar 31, 2019 •

edited

Loading

joyeecheung Mar 31, 2019 •

edited

Loading

refack Mar 31, 2019

BridgeAR Apr 3, 2019

joyeecheung commented Mar 18, 2019

BridgeAR commented Mar 25, 2019

nodejs-github-bot commented Mar 26, 2019

nodejs-github-bot commented Mar 27, 2019

joyeecheung commented Mar 27, 2019

nodejs-github-bot commented Mar 29, 2019

refack commented Mar 31, 2019

joyeecheung commented Mar 31, 2019

BridgeAR commented Apr 3, 2019

BridgeAR commented Apr 7, 2019

BridgeAR commented Apr 9, 2019

BridgeAR commented Apr 14, 2019

BridgeAR commented Apr 22, 2019 •

edited

Loading

BridgeAR commented Apr 22, 2019

nodejs-github-bot commented Apr 22, 2019

BridgeAR commented Apr 24, 2019

lib: throw a special error in internal/assert #26635

lib: throw a special error in internal/assert #26635

Conversation

joyeecheung commented Mar 13, 2019

Checklist

nodejs-github-bot commented Mar 13, 2019

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

Choose a reason for hiding this comment

joyeecheung Mar 31, 2019 • edited Loading

Choose a reason for hiding this comment

joyeecheung Mar 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung commented Mar 18, 2019

BridgeAR commented Mar 25, 2019

nodejs-github-bot commented Mar 26, 2019

nodejs-github-bot commented Mar 27, 2019

joyeecheung commented Mar 27, 2019

nodejs-github-bot commented Mar 29, 2019

refack commented Mar 31, 2019

joyeecheung commented Mar 31, 2019

BridgeAR commented Apr 3, 2019

BridgeAR commented Apr 7, 2019

BridgeAR commented Apr 9, 2019

BridgeAR commented Apr 14, 2019

BridgeAR commented Apr 22, 2019 • edited Loading

BridgeAR commented Apr 22, 2019

nodejs-github-bot commented Apr 22, 2019

BridgeAR commented Apr 24, 2019

joyeecheung Mar 31, 2019 •

edited

Loading

joyeecheung Mar 31, 2019 •

edited

Loading

BridgeAR commented Apr 22, 2019 •

edited

Loading