Repository navigation
C++ errors and JS errors format mismatch #26669
Description
Activity
I think there was some previous discussion (somewhere?) that it’s a bad idea to modify the
.nameproperty of errors like we do. I would agree, and if we do something, prefer to change it to the C++-style formatting.Reacted by Denys OtrishkoIt indeed looks like a duplicate of #20253. I personally would also like to change it to only contain the class name.
One way that work relatively well is: #20253 (comment)
An alternative would be to move it in the message property as suggested in #20253 (comment)
I tried restoring the name property and moving the code out of them once, but there were too many tests failing because we explicitly test for the name property with this format (there are still 77 tests on the current master if you search for
Error [ERR), and I did not really want to change them in bulk.Maybe we should start migrating those tests? We already disable this format for WPT with
internalErrors.useOriginalName = true, since WPT uses the name property instead ofinstanceof(because of realms) to verify the error types.Reacted by Rich Trott and Denys Otrishko@joyeecheung I would be rigorous in this case and use regular expressions to replace these cases. It would otherwise indeed be a lot of work to change all these tests. I am not sure how you would like to migrate the tests without having to change all at once (if we want to keep on using
assert.throws()and check the name property in these tests).@BridgeAR Pretty sure for most cases (errors that are not thrown from a different vm context), we could just replace
assert.throwsoptions like{ name: 'TypeError [ERR_SOMETHING]', code: 'ERR_SOMETHING' }with
{ type: TypeError, code: 'ERR_SOMETHING' }For errors thrown from another context, we could switch to regular expressions, but those are rare.
No matter how we change those tests, there are still going to be 70+ files to be changed so I don't see a huge difference.
@joyeecheung I don't think it is a good idea to use
common.expectsError()at all anymore for these cases (and I originally changed the function to support this use case in the first place). I definitely think we should useassert.throws.The only reason why I originally added this functionality was that
assert.throws()at that point did not support anything the like but that is not the case anymore.Right,
typeseems to be acommon.expectsErroroption, an easy way to do this is to simply replace allassert.throwswithcommon.expectsError, and in the implementation ofcommon.expectsError, transform the options passed toassert.throwsas mentioned above.
Then when we finish the migration and change the implementation of the name properties, we just need to go back to strict matching incommon.expectsErrorand the test will be stricter.@BridgeAR The issue with
assert.throwsis that it's a public API, butcommon.expectsErroris not, so whenever we make a decision like this, if we continue usingassert.throws, all the tests need to be migrated again before the implementation can change, whereas it would easier if we just usecommon.expectsErrorall the time and tweak it to our liking.@joyeecheung this is the only special case and we'd run into the same issue with
expectsErrorif we planned on changing anything else. I doubt that if we switch back to the having the "regular" name in the name property that we'd ever change that again.@BridgeAR But we could extend
common.expectsErrorwith more features easily in the future if we decide to make any more changes to our errors: e.g. testing cross-realm errors, or actually providing names (e.g.NetworkError) for different categories of errors with a hierarchy. In those cases, it will be easier to changecommon.expectsErrorthan to changeassert.throws, and sincecommon.expectsErroralready usesassert.throws, if we decide to surface those changes toassert.throwswe just need to move the logic. IMO that's a healthier development flow than having to rely onassert.throwsdirectly to make any changes for internal testing purposes.I like the common. APIs. Every time I type
assert.strict...I wish we didn't useassertat all in our unit tests, and just had an internal assertion package that did exactly what we want and that could be changed at our convenience, rather than having eslint rules that force us to use the long cumbersome names.@sam-github you could use
const assert = require('assert').strict;. All assert functions are going to behave strict afterwards (so you can use the short names). But this has never become a convention in core.@BridgeAR Nice idea, I just tried, but it triggers our eslint error.
Yes, we should improve the rule to detect that.
I have a fix with which the errors will always print the same as they do right now but with which the name is set to the class name. I'll open a PR for it later on.
- added 3 commits that reference this issue
on Mar 23, 2019
The format of the
nameproperty differs between errors generated by C++ and those generated by JS.For example, JS errors will contain a
namelike'TypeError [ERR_FOO_BAR_BAZ]', while C++ errors will contain anamewith just'TypeError'.It's probably a good idea to have these two match.