-
-
Notifications
You must be signed in to change notification settings - Fork 326
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
Update some more tests relying on timeouts #341
Conversation
In addition it's probably better to use a t.setTimeout(..) than assertion for a timeout because it will come with a clear and informative error message.
Avoid relying on node being able to execute its setTimeouts within a reasonable time to account for the low performance VMs of the tests environments.
wooow! Good spot. Could you fix retimer? Another way might be to just drop its usage. The cost of allocating a timer almost disappeared in latest Node.js |
It makes sense to update retimer as it does add some functionality over the standard setTimeout(). |
@mcollina the Pr has been updated with the latest retimer and the code updated. |
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.
lgtm
const opts = lazyServer.address() | ||
opts.pipelining = 2 | ||
const client = new Client(opts) | ||
let count = 0 | ||
|
||
client.on('response', (statusCode, length, duration) => { | ||
t.equal(statusCode, 200, 'status code matches') | ||
t.ok(duration > 500 && duration < 800) | ||
t.ok(duration > delayResponse, `Expected response delay > ${delayResponse}ms but got ${duration}ms`) |
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.
FYI the original reason for checking if duration < 800 was this.
our calculations were giving ~half results, so not checking if it takes less than 600ms might not catch if the calculations in future break and start giving 2x time. We can increase it to 700 if builds start to fail.
I know it wasn't the right fix and there should be a more predictable way to assert this but removing the upper limit check brings the risk I explained in the comment.
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.
@nherment could you handle this?
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, I'll add a unit test for these calculations.
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.
@salmanm could you please point me to the 'calculations' you are referring to?
The code around the delayed response is pretty straightforward and I don't see anything that I would classify as calculations.
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.
@mcollina this should help further.
However, I don't like the fix here because as a user of
httpClient
I would expect forclient.destroy()
to work regardless of when it is called. In the current implementation,client.destroy()
will not work as expected if invoked synchronously in a timeout handler. It does not clear the retry mechanism of thehttpClient
.I think the "right" fix consists in using a different version of
retimer
that allows to reset the timer after it has been triggered. This way there is no need to change the instance of retimer within thehttpClient
.Cf.
autocannon/lib/httpClient.js
Line 73 in f8b598e