test: convert new tests to use error types - #18581
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
I'd keep this one as common.expectsError()
Sorry, something went wrong.
|
Updated to expectsError. Just for my own understanding, when is it preferred to use common.expectsError(func, { type }) over assert.throws(func, type)? |
Sorry, something went wrong.
|
It was originally recommended to use common.expectsError as it was much more powerful than assert.throws. This changed recently and now it is possible to also use a object for the validation. common.expectsError is still better in two small cases: it validates the error type and it allows a RegExp for the messages. There will always be a valid use case for common.expectsError even if those things would be overcome as well: it is possible to use it as callback and that will not be possible with assert.throws. I personally recommend to always use assert.throws if it is suitable. I might update the docs about it some time soon. |
Sorry, something went wrong.
Sorry, something went wrong.
|
I have looked at a handful of the CI errors and none of them look related to this change, is that expected? OS X had a missing tool, Linux had a YAML issue, and some others didn't look like they reported why they failed at all. |
Sorry, something went wrong.
|
@jackhorton to some extend: yes. But not as bad as it has happened here. We have to many flakes and other issues with our CI at the moment as it seems. |
Sorry, something went wrong.
Sorry, something went wrong.
PR-URL: nodejs#18581 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
|
Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label. |
Sorry, something went wrong.
|
Not sure what the status quo is for this -- theres no functional update, it just makes for fewer special cases in Node-ChakraCore. I think im fine with not backporting it? |
Sorry, something went wrong.
PR-URL: nodejs#18581 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs#18581 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs#18581 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs#18581 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Checklist
Affected core subsystem(s)
test
Some new tests were added recently that rely on error messages that are incompatible with Node-ChakraCore. This PR simply reverts the test to only relying on error type, rather than error message. vcbuild test is actually failing for me on an unrelated test, sequential/test-inspector-port-cluster, but I might make a new issue for that separately.