-
Notifications
You must be signed in to change notification settings - Fork 30k
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
test: fix flaky HTTP server tests #42846
test: fix flaky HTTP server tests #42846
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
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.
✅
The |
Fast-track has been requested by @tniessen. Please 👍 to approve. |
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.
If this is supposed to fix the flakiness entirely, I think we should add Fixes: https://github.com/nodejs/node/issues/42741
to the description. Otherwise, maybe rename the PR to test: reduce flakiness in HTTP server tests
?
I wasn't aware of the open issue. Edited the description in the hope that this will fix the flakiness. |
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, I had the fix for this in another PR which will take a while for me to land.
This one is even better, LGTM!
Landed in 0288844 |
Refs: nodejs#41263 PR-URL: nodejs#42846 Fixes: nodejs#42741 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>
@tniessen this doesn't land cleanly in v16.x-staging - do you mind backporting this to v16.x? |
These two tests have been causing lots of CI failures, both in Jenkins and GitHub actions. They incorrectly use
common.platformTimeout()
, which scales a given amount of time by a factor that depends on the platform. However, these tests compute some times ascommon.platformTimeout(x * common.platformTimeout(y))
, effectively squaring the platform-dependent factor.I also increased the base timeout from 1000ms to 2000ms. As a reference, I ran one of the tests (
test-http-server-headers-timeout-keepalive.js
) with different base timeouts, 8000 times each, using 200 concurrent processes across 16 cores.Refs: #41263
Fixes: #42741