Skip to content
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

lib: Use regex to compare error message #11854

Closed
wants to merge 3 commits into from

Conversation

kunalspathak
Copy link
Member

To make node Engine Agnostic, use better comparison method for error
message.

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

lib

To make node engine agnostic, use better comparison method for error
message.
@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Mar 15, 2017
@jasnell
Copy link
Member

jasnell commented Mar 15, 2017

/cc @nodejs/node-chakracore

lib/util.js Outdated

var Debug;

function tryStringify(arg) {
try {
return JSON.stringify(arg);
} catch (err) {
if (err.name === 'TypeError' && err.message === CIRCULAR_ERROR_MESSAGE)
if (err.name === 'TypeError' && err.message.match(CIRCULAR_ERROR_MESSAGE))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the only error message comparison in lib?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, usually error message comparison is done in test for which we added common.engineSpecificMessage API. See this for example.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really like this particular change because it makes the test less specific, even if we are unlikely to get burned by it. I think the common.engineSpecificMessage API is a good solution in the long term. In the shorter term, you could generate this particular error dynamically like this:

try { const a = {}; a.a = a; JSON.stringify(a); } catch (err) { console.log(err.message); }

Copy link
Member Author

Choose a reason for hiding this comment

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

I would love to use common.engineSpecificMessage() API for this case and that's why it is written. We are using it frequently in test to get around similar issues. But this particular check is specific to v8 and is inside product code which I felt should be relaxed. Alternatively I can modify the test case in node-chakracore repo to have a check for engine, but I thought this was more appropriate fix.

Copy link
Contributor

@mscdex mscdex Mar 15, 2017

Choose a reason for hiding this comment

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

I think between the current regexp and @cjihrig's suggestion, I would vote for the latter (followed by an assert to make sure the value is a non-empty string during startup).

The regexp might match a TypeError that includes the name of a property named 'circular', which would match the wrong error.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay. I understand the concerns. How about solving this problem broadly by having engine *specific string templates for various error messages?

// v8-error-msg.js
var errorMsg = [];
...
errorMsg[engineName]['SyntaxError_Unexpected_Id'] = "SyntaxError: Unexpected identifier";
...

We could update test infrastructure to fetch the appropriate error message based on template. Of-course some error message might need parameters like variable names, function names, etc. which we will have to add support for.

// test.js
assert.strictEquals(stdErr, common.engineSpecific('SyntaxError_Unexpected_Id'));

Currently the way it is achieved in node-chakracore is

assert.strictEquals(stdErr, common.engineSpecific({
    v8: "SyntaxError: Unexpected identifier",
    chakracore: "SyntaxError: Expected ';'"})
}));

Additional benefit we get is if engine changes the error message, there will be one common place where the messages will have to be changed. I know this is non-trivial work since it will involve going through every test case that relies on error message, make the entry of error message in central engine specific file and then update the test to fetch that error message, but wanted to get thoughts of community to make node test cases more engine neutral.

cc: @addaleax, @jasnell

*Roughly adopted from #4311 .

Copy link
Member

Choose a reason for hiding this comment

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

Fwiw I like @cjihrig’s suggestion for this particular problem too, just generating the error dynamically (either lazily or when at the top level of util.js) seems okay to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the PR with the change proposed by @cjihrig .

We can have separate discussion for error message centralization.

lib/util.js Outdated

var Debug;

function tryStringify(arg) {
try {
return JSON.stringify(arg);
} catch (err) {
if (err.name === 'TypeError' && err.message === CIRCULAR_ERROR_MESSAGE)
if (err.name === 'TypeError' && err.message.match(CIRCULAR_ERROR_MESSAGE))
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really like this particular change because it makes the test less specific, even if we are unlikely to get burned by it. I think the common.engineSpecificMessage API is a good solution in the long term. In the shorter term, you could generate this particular error dynamically like this:

try { const a = {}; a.a = a; JSON.stringify(a); } catch (err) { console.log(err.message); }

Lazily populate the `circular reference` error message thrown
by `JSON.stringify()` which can be used to compare the error
message thrown.
lib/util.js Outdated
CIRCULAR_ERROR_MESSAGE = err.message;
}
}
if (err.name === 'TypeError' && err.message.match(CIRCULAR_ERROR_MESSAGE))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the changes to this line can be reverted now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure.

@mscdex
Copy link
Contributor

mscdex commented Mar 19, 2017

LGTM if CI is ok with it: https://ci.nodejs.org/job/node-test-pull-request/6915/

jasnell pushed a commit that referenced this pull request Mar 22, 2017
To make node engine agnostic, use better comparison method for error
message.

Lazily populate the `circular reference` error message thrown
by `JSON.stringify()` which can be used to compare the error
message thrown.

PR-URL: #11854
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@jasnell
Copy link
Member

jasnell commented Mar 22, 2017

Landed in 4eb194a

@jasnell jasnell closed this Mar 22, 2017
kunalspathak added a commit to kunalspathak/node-chakracore that referenced this pull request Mar 23, 2017
Reverted nodejs@374a1d8 because
nodejs/node#11854 is present in upstream.

Updated v8-version
kunalspathak added a commit to kunalspathak/node-chakracore that referenced this pull request Mar 24, 2017
Reverted nodejs@374a1d8 because
nodejs/node#11854 is present in upstream.

Updated v8-version

PR-URL: nodejs#198
Signed-off-by: Hitesh Kanwathirtha <hiteshk@microsoft.com>
@MylesBorins
Copy link
Contributor

This will need to be manually backported to v7.x

@gibfahn
Copy link
Member

gibfahn commented Jun 17, 2017

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants