-
Notifications
You must be signed in to change notification settings - Fork 46
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
Solving issue #89: #110
Solving issue #89: #110
Conversation
mazoni
commented
Feb 21, 2017
- Special treatment to customHeader keyword to be added as a list to the metadata
- Specs covering single and multiple customHeader keywords
- README file updated to match the above changes
- Special treatment to customHeader keyword to be added as a list to the metadata - Specs covering single and multiple customHeader keywords - README file updated to match the above 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.
Thanks for all your work, some comments to update but I'm happy to merge this! 👍
@@ -144,7 +143,14 @@ Canned.prototype.parseMetaData = function(response) { | |||
return parts.join(':') | |||
}).join(',') | |||
var opts = JSON.parse('{' + content + '}') | |||
cannedUtils.extend(metaData, opts) | |||
if(opts.hasOwnProperty('customHeader')) { | |||
if(metaData.hasOwnProperty('customHeaders')) metaData.customHeaders.push(opts.customHeader) |
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.
Is that a common way to format? Just wondering as I've never seen if
without curly but else
with curly.
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.
Do we need both customHeaders
and customHeader
?
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.
You're right, @sideshowcoder looks weird. Will fix it.
About customHeaders and customHeader the thing is that to take advantage of all the work regarding metaData having a customHeadears attribute and this was already set to the response.
So customHeaders is only the name of the list in metadata not an accepted header in canned.
@@ -10,6 +10,8 @@ describe('canned', function () { | |||
can = canned('./spec/test_responses') | |||
req = { method: 'GET' } | |||
res = { setHeader: function () {}, end: function () {} } | |||
spyOn(res, 'setHeader') |
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.
Is the spy needed here? Overriding the setHeader
function should do fine or am I missing something.
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, in this particular case. In lines 139, 148 and 149 I have some cases where the setHeader will be called multiple times and setting the setHeader function fails since it ends up testing the wrong header.
So to cover the case that a certain header was set independently from what was set before, I needed the spy.
Sorry for the delay, but the new version (0.3.10) has been released. |