Add regex to text-crypto-random #10020
Conversation
|
Please see the commit message guidelines here. Also, there is a typo in the commit message. |
|
@NFoxley May I kindly ask you to format the commit message as described in CONTRIBUTING guidelines. |
imyller
added
the
code-and-learn
|
Yes, I will reformat. |
| assert.throws(function() { f(value); }); | ||
| assert.throws(function() { f(value, function() {}); }); | ||
| assert.throws(function() { f(value); }, /size must be a number >= 0/); | ||
| assert.throws(function() { f(value, function() {}); }, /size must be a number >= 0/); |
There was a problem hiding this comment.
possibly long line. does this pass make lint?
There was a problem hiding this comment.
To pass make lint this should be wrapped and indented to assert.throws( on the next line. :)
|
@NFoxley Also if you could |
|
Should all be taken care of, thanks for the feedback everyone. |
|
In CI run smartos failure was #10166 so unrelated. Arm failure looks to be machine issue so this should be good to land. @Fishrock123 can you check if you are happy your changes have been addressed ? |
|
@NFoxley looks like the change needs to be rebased since there have been other changes to the file landed. There are changes to the same part of the test so you'll need to look to see how the change in the other commmit related to the change you have here. |
Trott
force-pushed
the
test-crypto-random-regex
branch
from
19c540e to
78e1fa8
Compare
|
@mhdawson @Fishrock123 @NFoxley I did the rebase/conflict resolution and pushed. |
Trott
force-pushed
the
test-crypto-random-regex
branch
from
78e1fa8 to
d8e3670
Compare
Trott
force-pushed
the
test-crypto-random-regex
branch
from
d8e3670 to
c834c7f
Compare
|
@Fishrock123 I think your comments have been addressed. If so, can you please update your review so this can land? Thanks! |
|
Only CI failure is a known flaky test that is fixed in another PR that should land today or tomorrow. |
|
@Fishrock123 Looks good to you? |
PR-URL: nodejs#10020 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: #10020 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: #10020 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
|
Thoughts regarding a backport? |
|
ping |
|
@MylesBorins This PR applies cleanly for me to v6.x-staging. |
PR-URL: #10020 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: nodejs/node#10020 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Checklist
make -j8 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
Description of change
test: add RegExp as a second argument to assert.throws()
27:5 and 28:5 error assert.throws() should include RegExp as a second argument