-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
Adding a test for undefined value in OutgoingMessage.setHeader #970
Conversation
As a result of 979d0ca there is a new check for undefined values on OutgoingMessage.setHeader. This commit introduces a test for this case.
lgtm |
@@ -18,6 +18,17 @@ var s = http.createServer(function(req, res) { | |||
} | |||
assert.ok(threw, 'Non-string names should throw'); | |||
|
|||
// undefined value should throw, via 979d0ca8 | |||
threw = false; | |||
try { |
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.
Why not just use assert.throws()
?
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 just quickly duplicated the section above. I'm open to suggestions here.
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.
IMO it would make more sense to use assert.throws()
with a validation function like:
function(err) {
return err instanceof Error && err.message === '`value` required in setHeader("foo", value).';
}
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.
Truth be told, I think the try/catch is more readable.
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.
as it is the same style as the above section, @cjihrig would you object to me merging this in as-is?
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.
@brendanashworth go for it.
looks good to me |
As a result of 979d0ca there is a new check for undefined values on OutgoingMessage.setHeader. This commit introduces a test for this case. PR-URL: #970 Reviewed-By: Rod Vagg <[email protected]> Reviewed-By: Brendan Ashworth <[email protected]>
Thanks @kenperkins, merged in |
As a result of 979d0ca there is a new check for
undefined
values onOutgoingMessage.setHeader. This commit introduces a test for this case.