oc-client warmup improvements #288
Conversation
matteofigus
changed the title
|
|
||
| it('should return an error with all the details', function(){ | ||
|
|
||
| var expectedError = 'Error: Error warming up oc-client: request ' + |
There was a problem hiding this comment.
what about reusing the settings.js file here as well as the "template" for the error?
There was a problem hiding this comment.
This is consistent with the other test. I don't mind tests being a bit verbose and dirty if it improves readability.
There was a problem hiding this comment.
I cleaned up that to be a bit more readable btw hopefully it is better
| var injectr = require('injectr'); | ||
| var sinon = require('sinon'); | ||
|
|
||
| describe('client : warmup', function(){ |
There was a problem hiding this comment.
well done w/ the tests; they helped me to understand what the feature does
| @@ -17,12 +17,12 @@ var _ = { | |||
| eachAsync: function(obj, fn, cb){ | |||
| var c = obj.length; | |||
|
|
|||
| var next = function(){ | |||
| var next = function(err){ | |||
There was a problem hiding this comment.
in this case, the variable names are not helping very much w/ readability aren't they?
There was a problem hiding this comment.
True. This is really just a little custom polyfill for some little async functions that I use - mostly to keep the client as much "dependency free" and small as possible. The behaviour I changed here is that for a group of parallel async tasks, I want the cycle to exit not only when all the tasks are done - but also when any of the tasks errors via "calling back" an error.
There was a problem hiding this comment.
Renamed vars a bit to improve readability, hopefully this is a bit better
| callbacksLeft--; | ||
| if(callbacksLeft === 0 || !!err){ | ||
|
|
||
| var cbCopy = cb; |
There was a problem hiding this comment.
I do not quite understand why you do cbCopy (not entirely sure you do copy it btw) and also you do _.noop ... can you explain as I am really curious about it?
There was a problem hiding this comment.
This 2 lines are to ensure main callback is called just once. So basically I am overriding main callback with noop (function(){}) so that future executions do nothing. This is mostly for handling the scenario of when I fire the callback as soon as the error happens (so, other things could complete later but I ignore them).
There was a problem hiding this comment.
I understand, all good to me then; thanks!
| timeout: 5 | ||
| }; | ||
|
|
||
| var expectedError = 'Error: Error warming up oc-client: request ' + JSON.stringify(expectedRequest) + ' failed (timeout)'; |
| callbacksLeft--; | ||
| if(callbacksLeft === 0 || !!err){ | ||
|
|
||
| var cbCopy = cb; |
There was a problem hiding this comment.
I understand, all good to me then; thanks!
|
merging as I got the reviewers 👍 |
This PR is for improving the warmup mechanism in the oc node.js client