-
-
Notifications
You must be signed in to change notification settings - Fork 99
Fix: Increase keepalive and HTTP request timeouts to prevent CI failures #777
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #777 +/- ##
=======================================
Coverage 79.31% 79.31%
=======================================
Files 29 29
Lines 4197 4197
Branches 538 538
=======================================
Hits 3329 3329
Misses 726 726
Partials 142 142 |
53df248 to
0f1d213
Compare
|
@julianz- could you also link the example failures you were seeing in actual CI logs? |
adae5be to
8848593
Compare
These fixes address: 1. The `test_keepalive_conn_management` test was intermittently failing in CI with `http.client.NotConnected`. This was due to the combined execution time of test logic and the explicit `time.sleep()` exceeding the server timeout (2.0s). 2. The http_request_timeout was also too short and causing failures in test_http_over_https_error(). This commit addresses the flakiness by increasing both timeout values.
Here's a link for the failure with test_keepalive_conn_management(). Unfortunately, I can't find a log for the other timeout failure now. I saw it many times but it's bit of a nightmare rummaging through all these builds. I can drop this change and see it if comes back again. |
webknjaz
left a 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.
Alright, I've skimmed through the second test and even though there's no log to check, the timeout change doesn't seem harmful so I'll just go with it. Thanks!
For the Debian package of cheroot we need Python 3.14 support, and I noticed that applying cherrypy#800 cherrypy#801 cherrypy#777 on top of the 11.1.2 release was enough to get the tests to pass with Python 3.14. Fixes: cherrypy#767
For the Debian package of cheroot we need Python 3.14 support, and I noticed that applying cherrypy#800 cherrypy#801 cherrypy#777 on top of the 11.1.2 release was enough to get the tests to pass with Python 3.14. Fixes: cherrypy#767
For the Debian package of cheroot we need Python 3.14 support, and I noticed that applying cherrypy#800 cherrypy#801 cherrypy#777 on top of the 11.1.2 release was enough to get the tests to pass with Python 3.14. Fixes: cherrypy#767
For the Debian package of cheroot we need Python 3.14 support, and I noticed that applying cherrypy#800 cherrypy#801 cherrypy#777 on top of the 11.1.2 release was enough to get the tests to pass with Python 3.14. Fixes: cherrypy#767
These fixes address:
The
test_keepalive_conn_managementtest was intermittently failing in CI withhttp.client.NotConnected. This was due to the combined execution time of test logic and the explicittime.sleep()exceeding the server timeout (2.0s).The http_request_timeout was also too short and causing failures in test_http_over_https_error().
This commit addresses the flakiness by increasing both timeout values.
This change is