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: restore TypeError if no arguments #12843

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@Trott
Member

Trott commented May 5, 2017

In Node 7.x, calling throw new assert.AssertionError() resulted in a
TypeError.

In current master, the same call does not result in an error but, due to
the default option, it results in uninformative output ("undefined
undefined undefined").

This change removes the default argument, restoring a TypeError if there
is no argument. This also will restore our test coverage to 100%. (The
default argument is not tested in our current test suite.)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

assert

@Trott Trott added the assert label May 5, 2017

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell May 5, 2017

Member

I think I'd rather keep the default and fix the message so that it's more helpful in the default case

Member

jasnell commented May 5, 2017

I think I'd rather keep the default and fix the message so that it's more helpful in the default case

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott May 5, 2017

Member

I think I'd rather keep the default and fix the message so that it's more helpful in the default case

There is no such thing as a "default case" for an assertion. By definition, an assertion needs to be about something specific. A default argument here isn't fulfilling some unmet need. It's just extra surface area we don't need. Although this does make me realize I should add a test...

Member

Trott commented May 5, 2017

I think I'd rather keep the default and fix the message so that it's more helpful in the default case

There is no such thing as a "default case" for an assertion. By definition, an assertion needs to be about something specific. A default argument here isn't fulfilling some unmet need. It's just extra surface area we don't need. Although this does make me realize I should add a test...

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell May 5, 2017

Member

Ok. I'm not going to object to the change over a simple personal preference ;-) ... the code LGTM

Member

jasnell commented May 5, 2017

Ok. I'm not going to object to the change over a simple personal preference ;-) ... the code LGTM

assert: restore TypeError if no arguments
In Node 7.x, calling `throw new assert.AssertionError()` resulted in a
TypeError.

In current master, the same call does not result in an error but, due to
the default option, it results in uninformative output ("undefined
undefined undefined").

This change removes the default argument, restoring a TypeError if there
is no argument. This also will restore our test coverage to 100%. (The
default argument is not tested in our current test suite.)
@cjihrig

cjihrig approved these changes May 5, 2017

@lpinca

lpinca approved these changes May 5, 2017

@Trott

This comment has been minimized.

Show comment
Hide comment
@refack

refack approved these changes May 6, 2017

});
{
// bad args to AssertionError constructor should throw TypeError
const args = [1, true, false, '', null, Infinity, Symbol('test'), undefined];

This comment has been minimized.

@refack

refack May 6, 2017

Member

If you've went to the trouble of making it more readable, why not call it badArgs or invalidArgs. I've seen the line is 77 chars long, but maybe... Maybe just invalids or bad?

@refack

refack May 6, 2017

Member

If you've went to the trouble of making it more readable, why not call it badArgs or invalidArgs. I've seen the line is 77 chars long, but maybe... Maybe just invalids or bad?

This comment has been minimized.

@refack

refack May 6, 2017

Member

nonArgs

@refack

refack May 6, 2017

Member

nonArgs

Trott added a commit to Trott/io.js that referenced this pull request May 8, 2017

assert: restore TypeError if no arguments
In Node 7.x, calling `throw new assert.AssertionError()` resulted in a
TypeError.

In current master, the same call does not result in an error but, due to
the default option, it results in uninformative output ("undefined
undefined undefined").

This change removes the default argument, restoring a TypeError if there
is no argument. This also will restore our test coverage to 100%. (The
default argument is not tested in our current test suite.)

PR-URL: nodejs#12843
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott May 8, 2017

Member

Landed in f6247a9

Member

Trott commented May 8, 2017

Landed in f6247a9

@Trott Trott closed this May 8, 2017

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

assert: restore TypeError if no arguments
In Node 7.x, calling `throw new assert.AssertionError()` resulted in a
TypeError.

In current master, the same call does not result in an error but, due to
the default option, it results in uninformative output ("undefined
undefined undefined").

This change removes the default argument, restoring a TypeError if there
is no argument. This also will restore our test coverage to 100%. (The
default argument is not tested in our current test suite.)

PR-URL: nodejs#12843
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@jasnell jasnell referenced this pull request May 11, 2017

Closed

8.0.0 Release Proposal #12220

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete
@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Jun 20, 2017

Member

In Node 7.x, calling throw new assert.AssertionError() resulted in a
TypeError.

In current master, the same call does not result in an error but, due to
the default option, it results in uninformative output ("undefined
undefined undefined").

I think this means that this isn't needed on v6.x, correct me if I'm wrong @Trott

Member

gibfahn commented Jun 20, 2017

In Node 7.x, calling throw new assert.AssertionError() resulted in a
TypeError.

In current master, the same call does not result in an error but, due to
the default option, it results in uninformative output ("undefined
undefined undefined").

I think this means that this isn't needed on v6.x, correct me if I'm wrong @Trott

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Jun 21, 2017

Member

I think this means that this isn't needed on v6.x, correct me if I'm wrong @Trott

I think that's right.

Member

Trott commented Jun 21, 2017

I think this means that this isn't needed on v6.x, correct me if I'm wrong @Trott

I think that's right.

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