Add option to time request/response cycle (including rollup of redirects) #1459
Conversation
|
👍 It seems like your new test is broken though. |
|
@nicolasmccurdy Good catch, thanks! Odd that that assertion passes locally but fails in CI - in any case, the check around the mock delay on redirect renders the failing assertion moot, so I just removed it. |
|
👍 |
|
The original tests sometimes fail for me. |
|
If you still want that test you would probably need to increase the Update: more specifically Node's |
|
I doubt it's a V8 issue since the test was reliable locally, and only failed in CI. Aaron Miller
|
|
Not really; the intent was to ensure that, when following redirects, the time reported is the sum of all request times, instead of only the time taken by the last request made. Aaron Miller
|
|
What I was getting at is that running your original test locally for me was not reliable, it would fail or succeed on and off. |
|
The original test was just checking the redirect elapsed time was greater than the non redirect elapsed time right? Not the sum of each request's elapsed time correct? |
|
I've also have had different results depending on the order of the tests. |
| self.req = self.httpModule.request(reqOptions) | ||
|
|
||
| if (self.timing) { | ||
| self.startTime = startTime |
There was a problem hiding this comment.
Could we just define self.startTime to be new Date().getTime() ?
|
@nylen @simov @FredKSchott Do you guys have any opinion about adding this? |
|
Seems like the tests are failing because an error that was thrown by Karma Spec Runner Update: And only in Node 0.12 |
|
@seanstrom this is my first experience with travis; i have no idea what to do about that, sorry. |
|
It seems to me like this might be an issue with Karma occasionally failing on Travis. Could someone please restart the job so we can see if that helps? |
|
That's okay, i just wanted that info to stated on the PR. |
|
I attempted to restart the build, and Travis says the job has been restarted. |
|
Okay so closing and opening the PR will restart the PR build in Travis CI |
|
@aaron-em okay everything looks good, but I made one other line comment. |
|
@seanstrom I don't think this should be part of the core as it doesn't add any value to the module. You can wrap request in a function and achieve the exact same result without passing additional option to request itself. But if you want it merge it, it's your call :) |
|
Actually now when I think about it, why not, we have tons of other options as well. So @seanstrom I'm not against adding it. |
|
@simov yeah that was my feeling at first, but like you said we have a bunch of useful options. |
|
@aaron-em any other changes you want to make? |
|
I've done all the damage I mean to do; feel free to merge, and thanks! Aaron Miller
|
Add option to time request/response cycle (including rollup of redirects)
|
@seanstrom check out the names listed below the maintainers section here https://www.npmjs.com/package/request |
|
when do we expect an npm release for this? it would be very helpful for us. |
|
The next release on NPM is scheduled for this week, I'll post an issue in the tracker when it's done 👍 |
|
👍 look forward |
Sure, it'd be fairly trivial to grab timestamps before initiating the request, and in the response callback, and compare them; on the other hand, I'll eventually have a lot of tests where I want to use the functionality, and writing a wrapper around the request library doesn't appeal to me.