bpo-38614: Add timeout constants to test.support by vstinner · Pull Request #16964 · python/cpython

vstinner · 2019-10-28T15:03:23Z

Add timeout constants to test.support:

LOOPBACK_TIMEOUT
INTERNET_TIMEOUT
SHORT_TIMEOUT
LONG_TIMEOUT

https://bugs.python.org/issue38614

vstinner · 2019-10-28T15:24:59Z

Somehow related, in 2014, I tried to make time.sleep() calls configurable in tests: https://bugs.python.org/issue20910

vstinner · 2019-10-28T15:47:07Z

@pablogsal: Would you mind to review this PR?

I'm not sure about the documentation of SHORT_TIMEOUT vs LONG_TIMEOUT.

LONG_TIMEOUT is 10 times larger than SHORT_TIMEOUT.

pablogsal

Thanks for doing this @vstinner ! 🎉

I left some comments regarding style and minor nits!

pablogsal · 2019-10-30T00:06:32Z

Nit: should we add markup links to connect(), recv()...?

Basically `connect` -> :meth:`socket.socket.connect`

Or perhaps :meth:~socket.socket.connect to avoid showing the fully-qualified name,

pablogsal · 2019-10-30T00:06:49Z

Markup link to socket.socket?

Perhaps not needed if the methods are linked.

pablogsal · 2019-10-30T00:07:26Z

Suggested change

Usually, a timeout using INTERNET_TIMEOUT should not mark a test a failed,

Usually, a timeout using INTERNET_TIMEOUT should not mark a test as failed,

pablogsal · 2019-10-30T00:07:52Z

Suggestion: link to transient_internet if it makes sense.

pablogsal · 2019-10-30T00:08:34Z

Suggested change

slower. It is not designed to measure a function performance.

It is not designed to measure a function performance.

pablogsal · 2019-10-30T00:09:17Z

Suggested change

enough to reduce the risk of test failure on the slowest Python buildbot

enough to reduce the risk of test failure on the slowest Python buildbot.

pablogsal · 2019-10-30T00:10:04Z

Suggested change

# For slowest buildbot worker, increase SHORT_TIMEOUT and LONG_TIMEOUT

# For a slow buildbot worker, increase SHORT_TIMEOUT and LONG_TIMEOUT

pablogsal · 2019-10-30T00:10:59Z

Suggested change

# long enough to prevent test failure: it takes an account that the client and

# long enough to prevent test failure: it takes into account that the client and

pablogsal · 2019-10-30T00:11:31Z

Suggested change

prevent test failure: it takes an account that the client and the server can

prevent test failure: it takes into account that the client and the server can

pablogsal · 2019-10-30T00:12:07Z

Suggested change

# Usually, a timeout using INTERNET_TIMEOUT should not mark a test a failed,

# Usually, a timeout using INTERNET_TIMEOUT should not mark a test as failed,

pablogsal · 2019-10-30T00:16:17Z

I'm not sure about the documentation of SHORT_TIMEOUT vs LONG_TIMEOUT.

Hummmm, maybe:

.. data:: SHORT_TIMEOUT
   Timeout in seconds that can be used to detect when a test hangs. It can be used
   in the average buildbots and is preferable over :data:`LONG_TIMEOUT` as it makes
   smaller pauses producing smaller run times for the test suite.

vsajip · 2019-10-30T09:22:18Z

Or perhaps :meth:~socket.socket.connect to avoid showing the fully-qualified name,

vsajip · 2019-10-30T09:24:31Z

Perhaps not needed if the methods are linked.

vsajip · 2019-10-30T09:29:20Z

Why this special value rather than None, as in the other cases? Is it to support None as an actual timeout value?

There is one test_socket test which requires timeout=None.

Add timeout constants to test.support: * LOOPBACK_TIMEOUT * INTERNET_TIMEOUT * SHORT_TIMEOUT * LONG_TIMEOUT

vstinner · 2019-10-30T11:17:47Z

@pablogsal: I rephased the documentation to better distinguish SHORT and LONG timeout. Does it sound better to you? I took your review in account. I checked manually the HTML documentation to ensure that it's rendered properly ;-)

I revert changes to use new timeouts, except test_socket and _test_multiprocessing to make this PR easier to review. I will write more PRs to use the new constants, I start to modify tons of tests, I prefer to have separated PRs to make them easier to review.

pablogsal · 2019-10-30T11:37:36Z

Does it sound better to you?

Much better indeed!

pablogsal

This looks great!

Add timeout constants to test.support: * LOOPBACK_TIMEOUT * INTERNET_TIMEOUT * SHORT_TIMEOUT * LONG_TIMEOUT

vstinner requested review from 1st1, asvetlov, tiran and vsajip as code owners October 28, 2019 15:03

the-knights-who-say-ni added the CLA signed label Oct 28, 2019

bedevere-bot added the awaiting core review label Oct 28, 2019

pablogsal reviewed Oct 30, 2019

View reviewed changes

vsajip reviewed Oct 30, 2019

View reviewed changes

bpo-38614: Add timeout constants to test.support

866731b

Add timeout constants to test.support: * LOOPBACK_TIMEOUT * INTERNET_TIMEOUT * SHORT_TIMEOUT * LONG_TIMEOUT

pablogsal approved these changes Oct 30, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Oct 30, 2019

vstinner merged commit 24c6258 into python:master Oct 30, 2019

vstinner deleted the support_timeout branch October 30, 2019 11:41

bedevere-bot removed the awaiting merge label Oct 30, 2019

jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019

bpo-38614: Add timeout constants to test.support (pythonGH-16964)

11fd0a2

Add timeout constants to test.support: * LOOPBACK_TIMEOUT * INTERNET_TIMEOUT * SHORT_TIMEOUT * LONG_TIMEOUT

vstinner mentioned this pull request Dec 10, 2019

bpo-38614: Use test.support.LOOPBACK_TIMEOUT constant #17554

Merged

shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020

bpo-38614: Add timeout constants to test.support (pythonGH-16964)

7096377

Add timeout constants to test.support: * LOOPBACK_TIMEOUT * INTERNET_TIMEOUT * SHORT_TIMEOUT * LONG_TIMEOUT

	Usually, a timeout using INTERNET_TIMEOUT should not mark a test a failed,
	Usually, a timeout using INTERNET_TIMEOUT should not mark a test as failed,

	slower. It is not designed to measure a function performance.
	It is not designed to measure a function performance.

	enough to reduce the risk of test failure on the slowest Python buildbot
	enough to reduce the risk of test failure on the slowest Python buildbot.

	# For slowest buildbot worker, increase SHORT_TIMEOUT and LONG_TIMEOUT
	# For a slow buildbot worker, increase SHORT_TIMEOUT and LONG_TIMEOUT

	# long enough to prevent test failure: it takes an account that the client and
	# long enough to prevent test failure: it takes into account that the client and

	prevent test failure: it takes an account that the client and the server can
	prevent test failure: it takes into account that the client and the server can

	# Usually, a timeout using INTERNET_TIMEOUT should not mark a test a failed,
	# Usually, a timeout using INTERNET_TIMEOUT should not mark a test as failed,

Uh oh!

Conversation

vstinner commented Oct 28, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Oct 28, 2019

Uh oh!

vstinner commented Oct 28, 2019

Uh oh!

pablogsal left a comment

Choose a reason for hiding this comment

Uh oh!

pablogsal Oct 30, 2019 • edited by vsajip Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablogsal commented Oct 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Oct 30, 2019

Uh oh!

pablogsal commented Oct 30, 2019

Uh oh!

pablogsal left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vstinner commented Oct 28, 2019 •

edited by bedevere-bot

Loading

pablogsal Oct 30, 2019 •

edited by vsajip

Loading

pablogsal commented Oct 30, 2019 •

edited

Loading