Skip to content
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

Unify timeouts in tests #1717

Merged
merged 6 commits into from Oct 31, 2019
Merged

Unify timeouts in tests #1717

merged 6 commits into from Oct 31, 2019

Conversation

pquentin
Copy link
Member

@pquentin pquentin commented Oct 29, 2019

Please review #1712 first, as I included it in this pull request to avoid conflicts in the future.

Closes #1706

Copy link
Member

@sethmlarson sethmlarson left a comment

Thanks! Preliminary comments for you.

# want to use a long timeout, even more so on CI where tests can be really slow
# 3. To test our timeout logic by using two different values, eg. by using different
# values at the pool level and at the request level.
SHORT_TIMEOUT = 0.001
Copy link
Member

@sethmlarson sethmlarson Oct 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like Travis (which is faster than AppVeyor usually) timed out on this 0.001 value. Maybe we need to have two separate values for the SHORT_TIMEOUT as well?

Copy link
Member Author

@pquentin pquentin Oct 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, test_ssl_read_timeout failed, I had not noticed that, sorry. I think the better fix is to switch to LONG_TIMEOUT here. Here's my reasoning.

In this test, we:

  1. read the headers -- this should not timeout, so using LONG_TIMEOUT makes sense
  2. read the body -- this should timeout, so using SHORT_TIMEOUT makes sense

However, both timeouts are read timeouts and are controlled by a single value, so we have to compromise. Switching to LONG_TIMEOUT is only a compromise for this test, while bumping SHORT_TIMEOUT would be a compromise for all tests.

What do you think?

Copy link
Member

@sethmlarson sethmlarson Oct 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LONG_TIMEOUT works for that test.

@@ -46,7 +47,7 @@ def setup_method(self, method):
def test_exceptions(self):
# DeadlineExceededError -> TimeoutError
with pytest.raises(urllib3.exceptions.TimeoutError):
self.pool.request("GET", "/sleep?seconds=0.005", timeout=0.001)
self.pool.request("GET", "/sleep?seconds=0.005", timeout=SHORT_TIMEOUT)
Copy link
Member

@sethmlarson sethmlarson Oct 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we implement the above comment, can we have the seconds=X call be dynamic? Maybe 2 * SHORT_TIMEOUT?

Copy link
Member Author

@pquentin pquentin Oct 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, great catch! I think I'll continue to use 5 * SHORT_TIMEOUT

pquentin added 3 commits Oct 29, 2019
The goal is to be able to to tweak the timeout values globally, and to
use larger timeouts on CI where running time is less predictable.
A part of the test is not supposed to time out, so a short timeout does
not make sense here.
@codecov-io
Copy link

codecov-io commented Oct 29, 2019

Codecov Report

Merging #1717 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1717   +/-   ##
=======================================
  Coverage   99.65%   99.65%           
=======================================
  Files          22       22           
  Lines        2006     2006           
=======================================
  Hits         1999     1999           
  Misses          7        7

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 11d68ef...8447aed. Read the comment docs.

@pquentin
Copy link
Member Author

pquentin commented Oct 29, 2019

Rebased on top of master and added two commits for your first two remarks.

@pquentin
Copy link
Member Author

pquentin commented Oct 29, 2019

I broke the lint, sorry. Will fix as soon as I can.

pquentin added 2 commits Oct 29, 2019
A part of the test is not supposed to time out, so a short timeout
does not make sense here.
@pquentin
Copy link
Member Author

pquentin commented Oct 29, 2019

I can't say that I've eradicated all flaky tests here, but I find this approach really promising. The separation between the two types of timeouts allows me to reason about the timeouts and justify switching to longer timeouts when it makes sense.

This is ready for another review!

@pquentin
Copy link
Member Author

pquentin commented Oct 30, 2019

Closing/reopening to see if I just got lucky or not.

@pquentin pquentin closed this Oct 30, 2019
@pquentin pquentin reopened this Oct 30, 2019
@pquentin
Copy link
Member Author

pquentin commented Oct 30, 2019

The AppVeyor failure is due to my test from #1715, I'll fix it later. The Travis failure is due to 100ms not being enough, I bumped LONG_TIMEOUT to 500ms.

@pquentin
Copy link
Member Author

pquentin commented Oct 30, 2019

Closing/reopening because it was a good idea the last time. :)

@pquentin pquentin closed this Oct 30, 2019
@pquentin pquentin reopened this Oct 30, 2019
@pquentin
Copy link
Member Author

pquentin commented Oct 30, 2019

One last try.

@pquentin pquentin closed this Oct 30, 2019
@pquentin pquentin reopened this Oct 30, 2019
@codecov-io
Copy link

codecov-io commented Oct 30, 2019

Codecov Report

Merging #1717 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1717   +/-   ##
=======================================
  Coverage   99.65%   99.65%           
=======================================
  Files          22       22           
  Lines        2006     2006           
=======================================
  Hits         1999     1999           
  Misses          7        7

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 11d68ef...8447aed. Read the comment docs.

@pquentin pquentin requested a review from sethmlarson Oct 30, 2019
Copy link
Member

@sethmlarson sethmlarson left a comment

LGTM

@sethmlarson sethmlarson merged commit b2fe7be into urllib3:master Oct 31, 2019
2 checks passed
@pquentin pquentin deleted the unify-timeouts branch Oct 31, 2019
Dobatymo pushed a commit to Dobatymo/urllib3 that referenced this issue Mar 16, 2022
The goal is to be able to to tweak the timeout values globally, and to
use larger timeouts on CI where running time is less predictable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants