tools: enforce two arguments in assert.throws - #12270
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Matching the complete error message seems fragile. What if V8 changes the wording of the error message?
Would matching the type of error object be sufficient, using the overload of assert.throws() that takes a constructor?
assert.throws(() => { /* some code that throws a TypeError */ }, TypeError);
Sorry, something went wrong.
There was a problem hiding this comment.
As above, consider just passing the AssertionError constructor here instead of a regexp.
Sorry, something went wrong.
There was a problem hiding this comment.
Will do. I actually tried to put AssertionError first but realized it's not globally available. Your comment made me look closer and I saw that it is exported by the assert module.
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM (with changes suggested by @jasongin). I'm slightly bummed about this, though, because I was going to hand all these changes out at Code + Learn later this month. :-D
Sorry, something went wrong.
Sorry, something went wrong.
This adds RegExp or error constructor arguments to the remaining places where it is missing in preparation for the commit that will enforce the presence of at least two arguments. PR-URL: nodejs#12270 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
Enables the requireTwo option of our custom rule. PR-URL: nodejs#12270 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
|
Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label. |
Sorry, something went wrong.
|
@targos when you merge multiple commits, could you still comment with Landed in HASH1 and HASH2 as per the Collaborator Guide? |
Sorry, something went wrong.
This adds RegExp or error constructor arguments to the remaining places where it is missing in preparation for the commit that will enforce the presence of at least two arguments. PR-URL: #12270 Backport-PR-URL: #13785 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
This adds RegExp or error constructor arguments to the remaining places where it is missing in preparation for the commit that will enforce the presence of at least two arguments. PR-URL: #12270 Backport-PR-URL: #13785 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
This adds RegExp or error constructor arguments to the remaining places where it is missing in preparation for the commit that will enforce the presence of at least two arguments. PR-URL: #12270 Backport-PR-URL: #13785 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
This adds RegExp or error constructor arguments to the remaining places where it is missing in preparation for the commit that will enforce the presence of at least two arguments. PR-URL: nodejs#12270 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
This adds RegExp or error constructor arguments to the remaining places where it is missing in preparation for the commit that will enforce the presence of at least two arguments. Backport-PR-URL: #19447 PR-URL: #12270 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
This adds RegExp or error constructor arguments to the remaining places where it is missing in preparation for the commit that will enforce the presence of at least two arguments. PR-URL: nodejs/node#12270 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
Enables the requireTwo option of our custom rule. PR-URL: nodejs/node#12270 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
First commit adds a RegExp argument to the remaining places where it is missing in preparation for the second commit that enforces the presence of at least two arguments in assert.throws().
Checklist
Affected core subsystem(s)
test, tools