bpo-38614: Add timeout constants to test.support #16964

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

Doc/library/test.rst

+   prevent test failure: it takes an account that the client and the server can
+   run in different threads or even different processes.
+
+   The timeout should be long enough for connect(), recv() and send() methods


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

Doc/library/test.rst

+   run in different threads or even different processes.
+
+   The timeout should be long enough for connect(), recv() and send() methods
+   of socket.socket.


Markup link to socket.socket?

Perhaps not needed if the methods are linked.

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

Doc/library/test.rst

+   is short enough to prevent a test to wait for too long if the Internet
+   request is blocked for whatever reason.
+
+   Usually, a timeout using INTERNET_TIMEOUT should not mark a test a failed,


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

Doc/library/test.rst

+   request is blocked for whatever reason.
+
+   Usually, a timeout using INTERNET_TIMEOUT should not mark a test a failed,
+   but skip the test instead: see transient_internet().


Suggestion: link to transient_internet if it makes sense.

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

Doc/library/test.rst

+
+   Timeout in seconds that can be used to detect when a test hangs. It is long
+   enough to reduce the risk of test failure on the slowest Python buildbot
+   slower. It is not designed to measure a function performance.


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

Doc/library/test.rst

+.. data:: LONG_TIMEOUT
+
+   Timeout in seconds that can be used to detect when a test hangs. It is long
+   enough to reduce the risk of test failure on the slowest Python buildbot


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

Lib/test/libregrtest/setup.py

@@ -81,6 +81,17 @@ def _test_audit_hook(name, args):

    setup_unraisable_hook()

+    if ns.timeout is not None:
+        # For slowest buildbot worker, increase SHORT_TIMEOUT and LONG_TIMEOUT


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

Lib/test/support/__init__.py

+
+# Timeout in seconds for tests using a network server listening on the network
+# local loopback interface like 127.0.0.1 (IPv4) or ::1 (IPv6). The timeout is
+# long enough to prevent test failure: it takes an account that the client and


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

Doc/library/test.rst

+
+   Timeout in seconds for tests using a network server listening on the network
+   local loopback interface like ``127.0.0.1``). The timeout is long enough to
+   prevent test failure: it takes an account that the client and the server can


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

Lib/test/support/__init__.py

+# short enough to prevent a test to wait for too long if the Internet request
+# is blocked for whatever reason.
+#
+# Usually, a timeout using INTERNET_TIMEOUT should not mark a test a failed,


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

Doc/library/test.rst

+   prevent test failure: it takes an account that the client and the server can
+   run in different threads or even different processes.
+
+   The timeout should be long enough for connect(), recv() and send() methods


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

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

Doc/library/test.rst

+   run in different threads or even different processes.
+
+   The timeout should be long enough for connect(), recv() and send() methods
+   of socket.socket.


Perhaps not needed if the methods are linked.

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

Lib/test/support/__init__.py

@@ -1542,9 +1585,12 @@ def get_socket_conn_refused_errs():


 @contextlib.contextmanager
-def transient_internet(resource_name, *, timeout=30.0, errnos=()):
+def transient_internet(resource_name, *, timeout=_NOT_SET, errnos=()):


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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-38614: Add timeout constants to test.support #16964

bpo-38614: Add timeout constants to test.support #16964

vstinner commented Oct 28, 2019 •

edited by bedevere-bot

vstinner commented Oct 28, 2019

vstinner commented Oct 28, 2019

pablogsal left a comment

pablogsal Oct 30, 2019 •

edited by vsajip

vsajip Oct 30, 2019

pablogsal Oct 30, 2019

vsajip Oct 30, 2019

pablogsal Oct 30, 2019

pablogsal Oct 30, 2019

pablogsal Oct 30, 2019

pablogsal Oct 30, 2019

pablogsal Oct 30, 2019

pablogsal Oct 30, 2019

pablogsal Oct 30, 2019

pablogsal Oct 30, 2019

pablogsal commented Oct 30, 2019 •

edited

vsajip Oct 30, 2019

vsajip Oct 30, 2019

vsajip Oct 30, 2019

vstinner Oct 30, 2019

vstinner commented Oct 30, 2019

pablogsal commented Oct 30, 2019

pablogsal left a comment

	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,

bpo-38614: Add timeout constants to test.support #16964

bpo-38614: Add timeout constants to test.support #16964

Conversation

vstinner commented Oct 28, 2019 • edited by bedevere-bot

vstinner commented Oct 28, 2019

vstinner commented Oct 28, 2019

pablogsal left a comment

Choose a reason for hiding this comment

pablogsal Oct 30, 2019 • edited by vsajip

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pablogsal commented Oct 30, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner commented Oct 30, 2019

pablogsal commented Oct 30, 2019

pablogsal left a comment

Choose a reason for hiding this comment

vstinner commented Oct 28, 2019 •

edited by bedevere-bot

pablogsal Oct 30, 2019 •

edited by vsajip

pablogsal commented Oct 30, 2019 •

edited