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.

Sign up for GitHub

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

Jump to bottom

lib: Use regex to compare error message #11854

Closed
wants to merge 3 commits into from
Closed

lib: Use regex to compare error message #11854

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
562be6d
lib: Use regex to compare error message
kunalspathak Mar 15, 2017
83109a9
lib: Addressed review comments
kunalspathak Mar 17, 2017
4d1d901
lib: Addressed review comments
kunalspathak Mar 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,15 @@ const inspectDefaultOptions = Object.seal({
breakLength: 60
});

const CIRCULAR_ERROR_MESSAGE = 'Converting circular structure to JSON';
const CIRCULAR_ERROR_MESSAGE = /Circular/i;

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 •
edited
Loading

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.

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.

return '[Circular]';
throw err;
}
Expand Down