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

Fix flaky test_timeout #1729

Merged
merged 3 commits into from Nov 5, 2019
Merged

Conversation

pquentin
Copy link
Member

@pquentin pquentin commented Nov 3, 2019

We had an interesting test failure in CI where this test failed twice in a row: we were expecting a timeout in 0.001 seconds, but both times it took more than one second!

How can this happen? Well, when we ask a request to timeout after N seconds, here's what we know:

  • the request will time out, eventually
  • it won't timeout before those N seconds
  • it could timeout a long time after those N seconds, especially on a busy CI service

Let's take advantage of those facts by specifying a request-specific timeout that's longer than the pool-level timeout and making sure that the timeout was long indeed.

We still test that the request-specific timeout overrides the pool-level timeout, but will stop failing on busy CI services.

(I'd like to think that this is easier to read and merge commit by commit.)

pquentin added 3 commits Oct 31, 2019
This test is about timeouts, we don't need to test _make_request
specifically here.
It's used throughout the test so it's better to explain that it's a
short timeout.
We had an interesting test failure in CI where this test failed twice in
a row: we were expecting a timeout in 0.001 seconds, but both times it
took more than one second!

How can this happen? Well, when we ask a request to timeout after N
seconds, here's what we know:

 * the request will time out, eventually
 * it won't timeout before those N seconds
 * it could timeout a long time after those N seconds, especially on a
   busy CI service

Let's take advantage of those facts by specifying a request-specific
timeout that's longer than the pool-level timeout and making sure that
the timeout was long indeed.

We still test that the request-specific timeout overrides the pool-level
timeout, but will stop failing on busy CI services.
@codecov-io
Copy link

codecov-io commented Nov 3, 2019

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1729   +/-   ##
=======================================
  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 f4b36ad...a9e1b86. Read the comment docs.

@sethmlarson sethmlarson merged commit 29041a2 into urllib3:master Nov 5, 2019
2 checks passed
@pquentin pquentin deleted the flaky-test-timeout branch Nov 5, 2019
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.

None yet

3 participants