lib: AssertionError moved to internal/error by fhalde · Pull Request #12906 · nodejs/node

fhalde · 2017-05-08T20:14:58Z

AssertionError class is moved to internal/error
in reference to the TODO in assert.js. This was
suggested to get rid of the cyclic dependency
between assert.js and internal/error.js

Checklist

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

Affected core subsystem(s)

assert

fhalde · 2017-05-08T20:19:36Z

There is one test case which I've been trying to resolve but have been out of luck. Looks like the test suite regex matches the exception being thrown. Since AssertionError has moved to a different module, the fully qualified name doesn't match up which results in the test failure.

watilde · 2017-05-08T21:56:11Z

lib/internal/errors.js

Could we use const fail = require('assert').fail instead of inline codes?

hiroppy · 2017-05-09T11:59:59Z

lib/internal/errors.js

I think you can use const.

jasnell · 2017-05-09T20:09:13Z

lib/internal/errors.js

I don't think we're going to need the factory method in this case. The other ones are there just to reduce code duplication. Just define the class and export it directly.

Cool. Also there is a test case that depends on how the AssertionError object was created (regex matching). How do I fix that? Earlier AssertionError object were created like throw new AssertionError, but now in assert.js, I do it like this throw new error.AssertionError.

I can very well store the constructor reference in a variable named like the class, but that would be a hack!

jasnell · 2017-05-10T19:23:28Z

lib/internal/errors.js

This should fail linting... this needs a 2 character space indent

how can I just run the linter on my code?

make lint

jasnell · 2017-05-10T19:23:59Z

lib/internal/errors.js

just AssertionError, should suffice (it does not need the additional : AssertionError bit)

AssertionError class is moved to interna/error in reference to the TODO in assert.js. This was suggested to get rid of the cyclic dependency between assert.js and internal/error.js

fhalde · 2017-05-10T19:54:31Z

@jasnell done & done

jasnell · 2017-05-11T00:42:12Z

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

fhalde · 2017-05-11T08:36:51Z

Before I pushed my changes. There were 4 test cases that were failing. After my changes there are 5 test cases failing. I know the reason why the 5th one is failing. I'm just not sure where to do the changes!

release test-process-exit checks for the error being thrown by assert ( uses regex match to check the failure ). Since the AssertionError has been moved to a new file, the regex no longer succeeds.

addaleax · 2017-05-19T21:40:49Z

@fhalde I think you might be able to fix the CI by updating the third line in test/message/error_exit.out according to the failure, i.e. turn throw new AssertionError into throw new errors.AssertionError?

thefourtheye · 2017-05-22T10:41:55Z

lib/assert.js

    operator = '!=';
-  throw new AssertionError({
+  const errors = lazyErrors();
+  throw new errors.AssertionError({


Nit: This is anyway exported. Why not just throw new assert. AssertionError({...});

If I remember, there was a problem caused by cyclic dependency. Something wasn't initialized causing few test cases to fail. Let me check it again!

thefourtheye · 2017-05-22T10:44:54Z

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

fhalde · 2017-05-23T12:11:24Z

If everything looks good, i'll squash the commits. I guess GitHub has the feature of squashing the PR??

jasnell · 2017-05-23T14:41:24Z

The commits can be squashed by whomever lands the PR

AssertionError class is moved to interna/error in reference to the TODO in assert.js. This was suggested to get rid of the cyclic dependency between assert.js and internal/error.js PR-URL: #12906 Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell · 2017-05-23T15:11:26Z

Landed in 425aba4

AssertionError class is moved to interna/error in reference to the TODO in assert.js. This was suggested to get rid of the cyclic dependency between assert.js and internal/error.js PR-URL: #12906 Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2017-07-17T15:39:44Z

Opting to not land this on v6.x... lmk if we should backport

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 May 8, 2017

fhalde force-pushed the improv branch from 697f671 to 7efc70e Compare May 8, 2017 20:17

watilde reviewed May 8, 2017

View reviewed changes

lib/internal/errors.js Outdated

Copy link

Member

watilde May 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use const fail = require('assert').fail instead of inline codes?

fhalde force-pushed the improv branch from 7efc70e to c68cab4 Compare May 9, 2017 06:38

hiroppy reviewed May 9, 2017

View reviewed changes

lib/internal/errors.js Outdated

Copy link

Member

hiroppy May 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can use const.

fhalde force-pushed the improv branch from c68cab4 to 707aea9 Compare May 9, 2017 17:16

jasnell requested changes May 9, 2017

View reviewed changes

fhalde force-pushed the improv branch from 707aea9 to f3f1a86 Compare May 10, 2017 06:18

jasnell reviewed May 10, 2017

View reviewed changes

lib/internal/errors.js Outdated

Copy link

Member

jasnell May 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just AssertionError, should suffice (it does not need the additional : AssertionError bit)

lib: AssertionError moved to internal/error

cd70341

AssertionError class is moved to interna/error in reference to the TODO in assert.js. This was suggested to get rid of the cyclic dependency between assert.js and internal/error.js

fhalde force-pushed the improv branch from f3f1a86 to cd70341 Compare May 10, 2017 19:53

error message changed in error_exit.out

a9bd3f1

thefourtheye reviewed May 22, 2017

View reviewed changes

jasnell approved these changes May 22, 2017

View reviewed changes

jasnell closed this May 23, 2017

jasnell mentioned this pull request May 28, 2017

8.0.0 Release Proposal #12220

Closed

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

MylesBorins added the dont-land-on-v6.x label Jul 17, 2017

Uh oh!

Conversation

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

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fhalde commented May 10, 2017

Uh oh!

jasnell commented May 11, 2017

Uh oh!

fhalde commented May 11, 2017

Uh oh!

addaleax commented May 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thefourtheye commented May 22, 2017

Uh oh!

fhalde commented May 23, 2017

Uh oh!

jasnell commented May 23, 2017

Uh oh!

jasnell commented May 23, 2017

Uh oh!

MylesBorins commented Jul 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

fhalde commented May 8, 2017 •

edited

Loading

fhalde commented May 8, 2017 •

edited

Loading