Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ Katerina Koukiou
Kevin Cox
Kodi B. Arfer
Kostis Anagnostopoulos
Kyle Altendorf
Lawrence Mitchell
Lee Kamentsky
Lev Maximov
Expand Down
1 change: 1 addition & 0 deletions changelog/4073.feature.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Allow specification of timeout for ``Testdir.runpytest_subprocess()`` and ``Testdir.run()``.
62 changes: 59 additions & 3 deletions src/_pytest/pytester.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,11 @@ def pytest_configure(config):
config.pluginmanager.register(checker)


def raise_on_kwargs(kwargs):
if kwargs:
raise TypeError("Unexpected arguments: {}".format(", ".join(sorted(kwargs))))


class LsofFdLeakChecker(object):
def get_open_files(self):
out = self._exec_lsof()
Expand Down Expand Up @@ -482,6 +487,9 @@ class Testdir(object):

"""

class TimeoutExpired(Exception):
pass

def __init__(self, request, tmpdir_factory):
self.request = request
self._mod_collections = WeakKeyDictionary()
Expand Down Expand Up @@ -1039,14 +1047,23 @@ def popen(self, cmdargs, stdout, stderr, **kw):

return popen

def run(self, *cmdargs):
def run(self, *cmdargs, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

should this be timeout=None instead of allowing arbitrary kwargs? I notice they aren't checked so it could potentially allow .run(foo='bar') even when foo isn't used / would be a TypeError otherwise

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd love to do that, and did do that, but py2. This re-surprises me each time I find it.

https://repl.it/@altendky/pytest-devpytest4078-001

def f(*args, something=None):
    pass
Python 2.7.10 (default, Jul 14 2015, 19:46:27)
[GCC 4.8.2] on linux
   
Traceback (most recent call last):
  File "python", line 1
    def f(*args, something=None):
                         ^
SyntaxError: invalid syntax

But yes, if we are concerned about extras getting lost I can add some validation. I was maybe being a bit loose with it given elsewhere in the file there's a random **kwargs that is (was?) entirely unused.

Copy link
Member

Choose a reason for hiding this comment

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

oh, here's keyword-only arguments in python2+3:

def f(**kwargs):
    arg = kwargs.pop('arg', None)
    if kwargs:
        raise TypeError('Unexpected arguments: {}'.format(', '.join(sorted(kwargs))))

"""Run a command with arguments.

Run a process using subprocess.Popen saving the stdout and stderr.

:param args: the sequence of arguments to pass to `subprocess.Popen()`
:param timeout: the period in seconds after which to timeout and raise
:py:class:`Testdir.TimeoutExpired`

Returns a :py:class:`RunResult`.

"""
__tracebackhide__ = True

timeout = kwargs.pop("timeout", None)
raise_on_kwargs(kwargs)

cmdargs = [
str(arg) if isinstance(arg, py.path.local) else arg for arg in cmdargs
]
Expand All @@ -1061,7 +1078,40 @@ def run(self, *cmdargs):
popen = self.popen(
cmdargs, stdout=f1, stderr=f2, close_fds=(sys.platform != "win32")
)
ret = popen.wait()

def handle_timeout():
__tracebackhide__ = True

timeout_message = (
"{seconds} second timeout expired running:"
" {command}".format(seconds=timeout, command=cmdargs)
)

popen.kill()
popen.wait()
raise self.TimeoutExpired(timeout_message)

if timeout is None:
ret = popen.wait()
elif six.PY3:
try:
ret = popen.wait(timeout)
except subprocess.TimeoutExpired:
handle_timeout()
else:
end = time.time() + timeout

resolution = min(0.1, timeout / 10)

while True:
ret = popen.poll()
if ret is not None:
break

if time.time() > end:
handle_timeout()

time.sleep(resolution)
finally:
f1.close()
f2.close()
Expand Down Expand Up @@ -1108,9 +1158,15 @@ def runpytest_subprocess(self, *args, **kwargs):
with "runpytest-" so they do not conflict with the normal numbered
pytest location for temporary files and directories.

:param args: the sequence of arguments to pass to the pytest subprocess
:param timeout: the period in seconds after which to timeout and raise
:py:class:`Testdir.TimeoutExpired`

Returns a :py:class:`RunResult`.

"""
__tracebackhide__ = True

p = py.path.local.make_numbered_dir(
prefix="runpytest-", keep=None, rootdir=self.tmpdir
)
Expand All @@ -1119,7 +1175,7 @@ def runpytest_subprocess(self, *args, **kwargs):
if plugins:
args = ("-p", plugins[0]) + args
args = self._getpytestargs() + args
return self.run(*args)
return self.run(*args, timeout=kwargs.get("timeout"))

def spawn_pytest(self, string, expect_timeout=10.0):
"""Run pytest using pexpect.
Expand Down
30 changes: 30 additions & 0 deletions testing/test_pytester.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import py.path
import pytest
import sys
import time
import _pytest.pytester as pytester
from _pytest.pytester import HookRecorder
from _pytest.pytester import CwdSnapshot, SysModulesSnapshot, SysPathsSnapshot
Expand Down Expand Up @@ -401,3 +402,32 @@ def test_testdir_subprocess(testdir):
def test_unicode_args(testdir):
result = testdir.runpytest("-k", u"💩")
assert result.ret == EXIT_NOTESTSCOLLECTED


def test_testdir_run_no_timeout(testdir):
testfile = testdir.makepyfile("def test_no_timeout(): pass")
assert testdir.runpytest_subprocess(testfile).ret == EXIT_OK


def test_testdir_run_with_timeout(testdir):
testfile = testdir.makepyfile("def test_no_timeout(): pass")

start = time.time()
result = testdir.runpytest_subprocess(testfile, timeout=120)
Copy link
Contributor

Choose a reason for hiding this comment

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

@altendky
Wouldn't that still fail with assert duration < 5 below?

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately the build failure is gone, likely because it was restarted?! (would be good to copy'n'paste those in the future). (it was for py27-pluggymaster)

Copy link
Member Author

Choose a reason for hiding this comment

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

:[ Yes, I should have extended that as well. Or, really, made one number calculated from the other to avoid this error in the future.

How should I proceed? Push a new commit to the branch? Start a new branch?

And :[ again. I did double check that my link to the broken build was for a specific build. I didn't realize that Travis was going to reuse that number and effectively break the link. Lesson learned. sigh Don't restart builds, trigger a new build (empty commit, close/reopen PR, something).

end = time.time()
duration = end - start

assert result.ret == EXIT_OK
assert duration < 5


def test_testdir_run_timeout_expires(testdir):
Copy link
Member

Choose a reason for hiding this comment

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

👍

testfile = testdir.makepyfile(
"""
import time

def test_timeout():
time.sleep(10)"""
)
with pytest.raises(testdir.TimeoutExpired):
testdir.runpytest_subprocess(testfile, timeout=1)