-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
test: add optional throw fn to expectsError #14089
Conversation
Change LGTM, I don't think I've ever used an |
@nodejs/testing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs ttps://github.com/nodejs/node/blob/master/test/common/README.md
@BridgeAR You're the one who's gonna have to resolve all the conflicts in |
@refack I added the documentation. About resolving the conflicts: I think it would be best to first merge this as is and then change all those calls over time. |
PR-URL: nodejs#14089 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Refael Ackermann <refack@gmail.com>
67f5900
to
d6fece1
Compare
This doesn’t land cleanly on 8.x; if you can, please follow the guide and raise a backport PR, if you don’t think it’s worth it let me know and we’ll add the |
@BridgeAR IMHO if you apply the PRs to |
Should this be backported to |
This function does not exist in v.6 so backporting is obsolete. |
This is mainly a style thing but I think it's pretty nice to add the throwing function directly to
common.expectsError
instead of wrapping that into the throw function as it's done most of the time.I thought I just create this as a example and I only changed a few tests accordingly.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
test