New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

assert: fix AssertException, assign error code #12651

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@jasnell
Member

jasnell commented Apr 25, 2017

Using assert.AssertException() without the new keyword results in a non-intuitive error:

> assert.AssertionError({})
TypeError: Cannot assign to read only property 'name' of function 'function ok(value, message) {
  if (!value) fail(value, true, message, '==', assert.ok);
}'
    at Function.AssertionError (assert.js:45:13)
    at repl:1:8
    at realRunInThisContextScript (vm.js:22:35)
    at sigintHandlersWrap (vm.js:98:12)
    at ContextifyScript.Script.runInThisContext (vm.js:24:12)
    at REPLServer.defaultEval (repl.js:346:29)
    at bound (domain.js:280:14)
    at REPLServer.runBound [as eval] (domain.js:293:12)
    at REPLServer.onLine (repl.js:545:10)
    at emitOne (events.js:101:20)
>

The assert.AssertionError() can only be used correctly with new, so this converts it into a proper ES6 class that will give an appropriate error message.

This also associates the appropriate internal/errors code with all assert.AssertionError instances and updates the appropriate test cases.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

assert

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Apr 25, 2017

Member

If I'm understanding correctly, this makes new AssertionError() work if assert module has been required. Otherwise, one will get a ReferenceError when trying to create an AssertionError?

Are we OK with that sort of magic? (AssertionError exists if assert has been required but otherwise not.)

Member

Trott commented Apr 25, 2017

If I'm understanding correctly, this makes new AssertionError() work if assert module has been required. Otherwise, one will get a ReferenceError when trying to create an AssertionError?

Are we OK with that sort of magic? (AssertionError exists if assert has been required but otherwise not.)

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Apr 25, 2017

Member

Ooops! No, I misread it entirely and I'm totally wrong, I think. Ignore my comment! :-D

Member

Trott commented Apr 25, 2017

Ooops! No, I misread it entirely and I'm totally wrong, I think. Ignore my comment! :-D

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 25, 2017

Member

@Trott ... heh... AssertionError is already exported by assert... it's already a requirement to require('assert') to use it.

Member

jasnell commented Apr 25, 2017

@Trott ... heh... AssertionError is already exported by assert... it's already a requirement to require('assert') to use it.

function truncate(s, n) {
return s.slice(0, n);
// TODO(jasnell): Consider moving AssertionError into internal/errors.js

This comment has been minimized.

@Trott

Trott Apr 25, 2017

Member

Nit: I dislike TODO comments because they tend to confuse others rather than communicate clearly what's going on. See all the longstanding TODOs that we just can't seem to address and all the confusion it sows for people reading them in #4641 #4642 #4640

In this case, what are the considerations? I'd rather see this opened as a separate issue with full sentences and paragraphs explaining the issue.

On the other hand, bonus points for putting your name there so at least the reader knows who to ask.

You can totally ignore this nit, of course. But in the off-chance you agree, then please open an issue.

@Trott

Trott Apr 25, 2017

Member

Nit: I dislike TODO comments because they tend to confuse others rather than communicate clearly what's going on. See all the longstanding TODOs that we just can't seem to address and all the confusion it sows for people reading them in #4641 #4642 #4640

In this case, what are the considerations? I'd rather see this opened as a separate issue with full sentences and paragraphs explaining the issue.

On the other hand, bonus points for putting your name there so at least the reader knows who to ask.

You can totally ignore this nit, of course. But in the off-chance you agree, then please open an issue.

This comment has been minimized.

@jasnell

jasnell Apr 25, 2017

Member

I expected at least one person to ask about this one :-) ... if we think it's a good idea, then I can just add a commit to this PR that moves it. It would be a minimally invasive change and would allow us to simplify some of the code. I just didn't do it yet because I want to know if there are any objections before doing so.

@jasnell

jasnell Apr 25, 2017

Member

I expected at least one person to ask about this one :-) ... if we think it's a good idea, then I can just add a commit to this PR that moves it. It would be a minimally invasive change and would allow us to simplify some of the code. I just didn't do it yet because I want to know if there are any objections before doing so.

This comment has been minimized.

@Fishrock123

Fishrock123 Apr 27, 2017

Member

TODO comments like this are still better than not having them, IMO.

@Fishrock123

Fishrock123 Apr 27, 2017

Member

TODO comments like this are still better than not having them, IMO.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Apr 25, 2017

Member

SGTM in principle. Since this involves significant changes to lib/assert.js and that happens to be one of the files we have complete test coverage for, it would be great if a coverage run could confirm that we still have 100% coverage across the board for it after these changes. I haven't tried to do a coverage run locally myself so I'm actually not sure how much work that is. If it's onerous, never mind. :-D

Member

Trott commented Apr 25, 2017

SGTM in principle. Since this involves significant changes to lib/assert.js and that happens to be one of the files we have complete test coverage for, it would be great if a coverage run could confirm that we still have 100% coverage across the board for it after these changes. I haven't tried to do a coverage run locally myself so I'm actually not sure how much work that is. If it's onerous, never mind. :-D

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 25, 2017

Member

I'll give the coverage run a go, but considering the fact that the tests had not previously caught that calling assert.AssertionError() without new was throwing a not-so-useful error, the 100% coverage is just a tad bit dubious ;-)

Member

jasnell commented Apr 25, 2017

I'll give the coverage run a go, but considering the fact that the tests had not previously caught that calling assert.AssertionError() without new was throwing a not-so-useful error, the 100% coverage is just a tad bit dubious ;-)

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Apr 25, 2017

Member

I'll give the coverage run a go, but considering the fact that the tests had not previously caught that calling assert.AssertionError() without new was throwing a not-so-useful error, the 100% coverage is just a tad bit dubious ;-)

I know you're being facetious (I hope), but that won't stop me from saying: 100% coverage just means that all the code gets exercised, not that there are no unhelpful error messages or (for that matter) bugs. 100% coverage isn't a guarantee that everything is tested. But 90% coverage is a guarantee that not everything is tested. So let's keep it at 100%. :-)

Member

Trott commented Apr 25, 2017

I'll give the coverage run a go, but considering the fact that the tests had not previously caught that calling assert.AssertionError() without new was throwing a not-so-useful error, the 100% coverage is just a tad bit dubious ;-)

I know you're being facetious (I hope), but that won't stop me from saying: 100% coverage just means that all the code gets exercised, not that there are no unhelpful error messages or (for that matter) bugs. 100% coverage isn't a guarantee that everything is tested. But 90% coverage is a guarantee that not everything is tested. So let's keep it at 100%. :-)

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 25, 2017

Member

Yes I am being facetious ;-) ... I'm running the coverage report now

Member

jasnell commented Apr 25, 2017

Yes I am being facetious ;-) ... I'm running the coverage report now

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 26, 2017

Member

@Trott ... updated! This should have 100% coverage on the assert module now

Member

jasnell commented Apr 26, 2017

@Trott ... updated! This should have 100% coverage on the assert module now

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 27, 2017

Member

Ping @Trott (and other @nodejs/ctc)

Member

jasnell commented Apr 27, 2017

Ping @Trott (and other @nodejs/ctc)

@jasnell

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Apr 27, 2017

Member

No comment on the code but +1 to the idea.

Member

Fishrock123 commented Apr 27, 2017

No comment on the code but +1 to the idea.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Apr 27, 2017

Member

CI is failing. Something needs some adjusting....

Member

Trott commented Apr 27, 2017

CI is failing. Something needs some adjusting....

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 27, 2017

Member

Ah... yeah, non-obvious rebase conflict. Rebasing and updating now.

Member

jasnell commented Apr 27, 2017

Ah... yeah, non-obvious rebase conflict. Rebasing and updating now.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 27, 2017

Member

CI is green.

Member

jasnell commented Apr 27, 2017

CI is green.

assert: fix AssertException, assign error code
Using `assert.AssertException()` without the `new` keyword results
in a non-intuitive error:

```js
> assert.AssertionError({})
TypeError: Cannot assign to read only property 'name' of function 'function ok(value, message) {
  if (!value) fail(value, true, message, '==', assert.ok);
}'
    at Function.AssertionError (assert.js:45:13)
    at repl:1:8
    at realRunInThisContextScript (vm.js:22:35)
    at sigintHandlersWrap (vm.js:98:12)
    at ContextifyScript.Script.runInThisContext (vm.js:24:12)
    at REPLServer.defaultEval (repl.js:346:29)
    at bound (domain.js:280:14)
    at REPLServer.runBound [as eval] (domain.js:293:12)
    at REPLServer.onLine (repl.js:545:10)
    at emitOne (events.js:101:20)
>
```

The `assert.AssertionError()` can only be used correctly with `new`,
so this converts it into a proper ES6 class that will give an
appropriate error message.

This also associates the appropriate internal/errors code with all
`assert.AssertionError` instances and updates the appropriate test
cases.

@jasnell jasnell added this to the 8.0.0 milestone Apr 28, 2017

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 29, 2017

Member

ping @nodejs/ctc ... I'd like to get this landed in time for 8.0.0 :-)

Member

jasnell commented Apr 29, 2017

ping @nodejs/ctc ... I'd like to get this landed in time for 8.0.0 :-)

@mhdawson

LGTM and nice to see ongoing progress on supporting error codes.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell May 4, 2017

Member

Landed in 3f34ede

Member

jasnell commented May 4, 2017

Landed in 3f34ede

@jasnell jasnell closed this May 4, 2017

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos May 4, 2017

Member

Shouldn't it be "AssertionError" instead of "AssertException" in the commit message?

Member

targos commented May 4, 2017

Shouldn't it be "AssertionError" instead of "AssertException" in the commit message?

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell May 4, 2017

Member

doh! ok, we're still within the ten minutes so I'm going to push a fix

Member

jasnell commented May 4, 2017

doh! ok, we're still within the ten minutes so I'm going to push a fix

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell May 4, 2017

Member

fixed!

Member

jasnell commented May 4, 2017

fixed!

jasnell added a commit that referenced this pull request May 4, 2017

assert: fix AssertionError, assign error code
Using `assert.AssertionError()` without the `new` keyword results
in a non-intuitive error:

```js
> assert.AssertionError({})
TypeError: Cannot assign to read only property 'name' of function 'function ok(value, message) {
  if (!value) fail(value, true, message, '==', assert.ok);
}'
    at Function.AssertionError (assert.js:45:13)
    at repl:1:8
    at realRunInThisContextScript (vm.js:22:35)
    at sigintHandlersWrap (vm.js:98:12)
    at ContextifyScript.Script.runInThisContext (vm.js:24:12)
    at REPLServer.defaultEval (repl.js:346:29)
    at bound (domain.js:280:14)
    at REPLServer.runBound [as eval] (domain.js:293:12)
    at REPLServer.onLine (repl.js:545:10)
    at emitOne (events.js:101:20)
>
```

The `assert.AssertionError()` can only be used correctly with `new`,
so this converts it into a proper ES6 class that will give an
appropriate error message.

This also associates the appropriate internal/errors code with all
`assert.AssertionError` instances and updates the appropriate test
cases.

PR-URL: #12651
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

anchnk added a commit to anchnk/node that referenced this pull request May 6, 2017

assert: fix AssertionError, assign error code
Using `assert.AssertionError()` without the `new` keyword results
in a non-intuitive error:

```js
> assert.AssertionError({})
TypeError: Cannot assign to read only property 'name' of function 'function ok(value, message) {
  if (!value) fail(value, true, message, '==', assert.ok);
}'
    at Function.AssertionError (assert.js:45:13)
    at repl:1:8
    at realRunInThisContextScript (vm.js:22:35)
    at sigintHandlersWrap (vm.js:98:12)
    at ContextifyScript.Script.runInThisContext (vm.js:24:12)
    at REPLServer.defaultEval (repl.js:346:29)
    at bound (domain.js:280:14)
    at REPLServer.runBound [as eval] (domain.js:293:12)
    at REPLServer.onLine (repl.js:545:10)
    at emitOne (events.js:101:20)
>
```

The `assert.AssertionError()` can only be used correctly with `new`,
so this converts it into a proper ES6 class that will give an
appropriate error message.

This also associates the appropriate internal/errors code with all
`assert.AssertionError` instances and updates the appropriate test
cases.

PR-URL: nodejs#12651
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

@jasnell jasnell referenced this pull request May 11, 2017

Closed

8.0.0 Release Proposal #12220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment