test: verify detail exception messages in addons-napi/test_constructor

[vincentcn]

[JSConf CN Code&Learn] Verify detail exception messages in addons-napi/test_constructor.

Checklist

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

Affected core subsystem(s)

no, just update tests.

[Trott]

LGTM if CI is green.

[gireeshpunathil]

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

[gabrielschulhof]

New CI: https://ci.nodejs.org/job/node-test-commit/11741/
Re-ran sub-jobs:

• https://ci.nodejs.org/job/node-test-commit-linuxone/7943/
• https://ci.nodejs.org/job/node-test-commit-osx/11706/

[gabrielschulhof]

Landed in e6eb5c0.

[MSLaguana]

What's the reasoning behind this change? When using node-chakracore this test was succeeding but now it fails since the specific error message reported by the engine is different. Does N-API intend to specify the particular error messages that are raised?

[Trott]

What's the reasoning behind this change?

The project treats Error message changes as breaking changes. This is because people often need to sniff error message text in production code to make a determination of what to do with a caught error.

The hope is that we can eventually drop that stance after there's been sufficient time given for the code property in the new internal error system to gain widespread usage.^*

But we're not there yet.

Until then, we encourage checking specific error messages in the tests so that the tests break when the messages change (and we can flag the change as a breaking change).

Can't speak for other Collaborators, but I'd certainly consider^** (for example) a proposal that we relax that for error types that are more specific than Error, if the tradeoff is making Chakracore maintenance less of a burden. In general, I think the interest of end users trump those of the maintainers, but a case could certainly be made that multiple VMs is in the end user's interest and that people who are relying on error message text likely know that it's a brittle practice to begin with.

^* At least, that seems to be the hope among most folks. I'm personally on the fence. A code for invalid argument type will still require the end user to sniff the message text to figure out which argument is invalid, thus making changes to messages still a potentially breaking change for responsibly-written code.

^** No promise of supporting the proposal, of course. But I'd certainly give it an honest look!

[MSLaguana]

Thanks for the response.

I've already tweaked the test in the node-chakracore repo to account for different error strings (see nodejs/node-chakracore@2f9535c?w=1) but it is good to know that error strings are considered to be part of the API surface. I'm not sure that sniffing strings will always be useful though, since as you can see from the change I made the error message that chakracore gives is significantly different to the error given by v8.

[digitalinfinity]

While I understand that error message changes are currently treated as breaking changes, I was under the impression (based on PRs like #13988 from @mhdawson) that the intent was to move the community away from relying on them and instead to rely on error codes. Is that not the case (apologies for hijacking this PR- please let me know if I should just open another issue for this discussion)

[Trott]

@digitalinfinity You're exactly right. That is the intention.

[Trott]

@MSLaguana Actually, this raises an important point. I think we concluded that error messages that were generated by V8 would be exempted from the "breaking change if the message changes" rule because we can't really control what V8 does with its error messages.

@nodejs/ctc Am I right to recall that?

If so, then a case could certainly be made that these really should just check for TypeError. (And I have to rethink a small number of my planned Code & Learn tasks for Vancouver in October. :-D )

[targos]

@Trott you are right. See the last paragraph of https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#breaking-changes:

Note that errors thrown, along with behaviors and APIs implemented by dependencies of Node.js (e.g. those originating from V8) are generally not under the control of Node.js and therefore are not directly subject to this policy. However, care should still be taken when landing updates to dependencies when it is known or expected that breaking changes to error handling may have been made. Additional CI testing may be required.