-
Notifications
You must be signed in to change notification settings - Fork 28.8k
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 a RegExp #10058
test: add a RegExp #10058
Conversation
@@ -4,5 +4,7 @@ const assert = require('assert'); | |||
const dns = require('dns'); | |||
|
|||
// Should not raise assertion error. Issue #7070 | |||
assert.throws(() => dns.resolveNs([])); // bad name | |||
assert.throws(() => dns.resolveNs('')); // bad callback | |||
//assert.throws(() => dns.resolveNs([]), /^\"name\" argument must be a string$/); // bad name |
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.
I think this line was committed by accident?
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.
Yes. Thanks for catching it. Now it is fixed.
assert.throws(() => dns.resolveNs([])); // bad name | ||
assert.throws(() => dns.resolveNs('')); // bad callback | ||
assert.throws(() => dns.resolveNs([]), /"name" argument must be a string/); // bad name | ||
assert.throws(() => dns.resolveNs(''), /"callback" argument must be a function/); // bad callback |
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.
These lines are longer than 80 characters, which is something we want to avoid – could you move the second argument to a new line (and align that below the first argument)?
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.
If you're checking the entire error message, you can add ^
and $
to the regular expression as well.
thanks for the suggestions. made the changes. |
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.
The changes LGTM but the commits should be squashed and a bit more descriptive.. for instance:
test: test error messages in test-dns-regress-7070.js
@@ -4,5 +4,14 @@ const assert = require('assert'); | |||
const dns = require('dns'); | |||
|
|||
// Should not raise assertion error. Issue #7070 | |||
assert.throws(() => dns.resolveNs([])); // bad name | |||
assert.throws(() => dns.resolveNs('')); // bad callback | |||
<<<<<<< HEAD |
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.
Looks like some merge conflicts got through.
It took me several commits to get it right. I think it should be good now. Let me know if there is anything else I need to change. |
assert.throws(() => dns.resolveNs([]), // bad name | ||
/^Error: "name" argument must be a string$/); | ||
assert.throws(() => dns.resolveNs(''), // bad callback | ||
/^Error: "callback" argument must be a function$/); |
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.
@zjyw can you please add a final new line? The linter will fail without it.
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.
yeah. just added it. thanks for pointing it out. i was wondering about it after my last commit.
|
@Fishrock123 why this shouldn't land on v7.x ? |
@addaleax LGTY? |
@lpinca The changes don't apply cleanly (even on master) |
@targos I see, the green merge button tricked me, thanks. |
@targos I am new to the node.js change system. When you say it won't apply cleanly, do you mean the unit test will fail or something else? |
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.
yes, lgtm!
@zjyw if you could rebase this PR against master and optionally squash your commits that would be awesome. |
Next time please try to do a backport before removing the label, thanks. This does not apply cleanly on v7.x |
@Fishrock123 I tried and it does apply cleanly if the commits are squashed. |
Rebased and squashed using 'git rebase -i HEAD~7'. Let me know if I did it right. |
Add a RegExp as a second argument to assert.throws().
@zjyw it seems something went wrong. I went ahead and fixed it. Here is what I did, with the |
@targos thanks for help! |
CI failure on FreeBSD seems entirely unrelated to this change, but just to be sure... |
Add a RegExp as a second argument to assert.throws(). PR-URL: nodejs#10058 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Landed in be6de1a. |
Add a RegExp as a second argument to assert.throws(). PR-URL: #10058 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Add a RegExp as a second argument to assert.throws(). PR-URL: #10058 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Add a RegExp as a second argument to assert.throws(). PR-URL: #10058 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Checklist
make -j8 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected core subsystem(s)
test
Description of change
Add a RegExp as a second argument to assert.throws().