[Piglit] [PATCH] framework: Add support for a test timeout
Dylan Baker
baker.dylan.c at gmail.com
Wed Nov 13 13:03:49 PST 2013
On Wednesday, November 13, 2013 06:16:34 PM Daniel Vetter wrote:
> i-g-t tests can take a long time, and for a bunch of reasons (bad
> tuning on disparate set of platforms, stuck in the kernel which often
> can be recovered by interrupting with a signal, ...) that sometimes
> extends to a good approximation of forever.
>
> Hence this adds a per-test timeout value and a bit of infrastructure
> to adjust the results. Test results adjusting is done after calling
> interpretResult so that we don't have to replicate this all over the
> place. This might need to be adjusted for the piglit-native subtest
> stuff, but otoh igt is a bit special with it's crazy long-running
> tests. So I think we can fix this once it's actually needed.
>
> The default timeout is None, so this is purely opt-in.
>
> Note on the implementation: SIG_ALARM doesn't exists on Windows and
> stackoverflow overwhelmingly recommended to go with this thread-based
> approach here. But only tested on my Linux box here.
>
> I've also timed quick.tests run a bit and the overhead due to the
> additional thread we launch seems to be in the wash. So I didn't opt
> to make the thread launching optional if there's no timeout limit.
>
> v2: Also add all the boilerplate needed to handle the new test status
> in summaries. For the color I've opted for two shades of light blue,
> they nicely stick out from the current selection.
>
> v3: Fix GLSLParserTest and ShaderTest. They both derive from
> PlainExecTest but only call the __init__ function of the Test
> baseclass. I haven't really figured why this is and also not really
> what command I should pass to PlainExecTest.__init__, so just
> replicated the init code for now.
This should be fine, I'm working on refactoring the Test, ExecTest, and
PlainExecTest classes anyway, I'd like to get down to just one or two of them
>
> v4: Initialize timeout earlier for tests where we use the ENOENT
> handling to skip them.
>
> Fix up the out, err passing from the thread. Apparently my idea of how
> python closures work was completely misguided - like with a function
> call a closure captures a copy of of a reference pointing at the
> original object. So assinging anything to them won't have any effect
> outside of the lambda. Hence we need to jump through hoops and pass an
> array around. Nicer fix would be to create a class or something, but
> that seems like overkill.
>
> Cc: Dylan Baker <baker.dylan.c at gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> ---
> framework/exectest.py | 28 +++++++++++++++++++++++++---
> framework/glsl_parser_test.py | 1 +
> framework/shader_test.py | 1 +
> framework/status.py | 39 ++++++++++++++++++++++++++-------------
> templates/index.css | 5 ++++-
> 5 files changed, 57 insertions(+), 17 deletions(-)
>
> diff --git a/framework/exectest.py b/framework/exectest.py
> index 986d8032cb37..e239940fe891 100644
> --- a/framework/exectest.py
> +++ b/framework/exectest.py
> @@ -23,6 +23,7 @@
> import errno
> import os
> import subprocess
> +import threading
> import shlex
> import types
> import re
> @@ -72,6 +73,7 @@ class ExecTest(Test):
> self.command = command
> self.split_command = os.path.split(self.command[0])[1]
> self.env = {}
> + self.timeout = None
>
> if isinstance(self.command, basestring):
> self.command = shlex.split(str(self.command))
> @@ -113,7 +115,7 @@ class ExecTest(Test):
> else:
> if env.dmesg:
> old_dmesg = read_dmesg()
> - (out, err, returncode) = \
> + (out, err, returncode, timeout) = \
> self.get_command_result(command, fullenv)
> if env.dmesg:
> dmesg_diff = get_dmesg_diff(old_dmesg,
> read_dmesg()) @@ -172,6 +174,9 @@ class ExecTest(Test):
> elif returncode != 0:
> results['note'] = 'Returncode was {0}'.format(returncode)
>
> + if timeout:
> + results['result'] = 'timeout'
> +
> if env.valgrind:
> # If the underlying test failed, simply report
> # 'skip' for this valgrind test.
> @@ -196,6 +201,7 @@ class ExecTest(Test):
> results['returncode'] = returncode
> results['command'] = ' '.join(self.command)
> results['dmesg'] = dmesg_diff
> + results['timeout'] = timeout
>
> self.handleErr(results, err)
>
> @@ -217,13 +223,29 @@ class ExecTest(Test):
>
> def get_command_result(self, command, fullenv):
> try:
> + timeout = False
> proc = subprocess.Popen(command,
> stdout=subprocess.PIPE,
> stderr=subprocess.PIPE,
> env=fullenv,
> universal_newlines=True)
> - out, err = proc.communicate()
> + output = ['', '']
> +
> + def thread_fn():
> + output[0], output[1] = proc.communicate()
> +
> + thread = threading.Thread(target=thread_fn)
> + thread.start()
> +
> + thread.join(self.timeout)
> +
> + if thread.is_alive():
> + proc.terminate()
> + thread.join()
> + timeout = True
> +
> returncode = proc.returncode
> + out, err = output
> except OSError as e:
> # Different sets of tests get built under
> # different build configurations. If
> @@ -237,7 +259,7 @@ class ExecTest(Test):
> returncode = None
> else:
> raise e
> - return out, err, returncode
> + return out, err, returncode, timeout
>
>
> class PlainExecTest(ExecTest):
> diff --git a/framework/glsl_parser_test.py b/framework/glsl_parser_test.py
> index 3b1a8936c65a..74034e871dcf 100755
> --- a/framework/glsl_parser_test.py
> +++ b/framework/glsl_parser_test.py
> @@ -197,6 +197,7 @@ class GLSLParserTest(PlainExecTest):
> self.__command = None
> self.__filepath = filepath
> self.result = None
> + self.timeout = None
>
> def __get_config(self):
> """Extract the config section from the test file.
> diff --git a/framework/shader_test.py b/framework/shader_test.py
> index ea1d46265e25..f5a2bf95c5e7 100755
> --- a/framework/shader_test.py
> +++ b/framework/shader_test.py
> @@ -145,6 +145,7 @@ class ShaderTest(PlainExecTest):
> self.__gl_api = None
>
> self.env = {}
> + self.timeout = None
>
> def __report_failure(self, message):
> if self.__run_standalone:
> diff --git a/framework/status.py b/framework/status.py
> index 2fe153eeb01a..c9509db42a40 100644
> --- a/framework/status.py
> +++ b/framework/status.py
> @@ -27,27 +27,30 @@ warn
> dmesg-fail
> fail
> crash
> +timeout
> skip
>
>
> The following are regressions:
>
> -pass|warn|dmesg-warn|fail|dmesg-fail|crash -> skip
> -pass|warn|dmesg-warn|fail|dmesg-fail -> crash|skip
> -pass|warn|dmesg-warn|fail -> dmesg-fail|crash|skip
> -pass|warn|dmesg-warn -> fail|dmesg-fail|crash|skip
> -pass|warn -> dmesg-warn|fail|dmesg-fail|crash|skip
> -pass -> warn|dmesg-warn|fail|dmesg-fail|crash|skip
> +pass|warn|dmesg-warn|fail|dmesg-fail|crash|timeout -> skip
> +pass|warn|dmesg-warn|fail|dmesg-fail|crash -> timeout|skip
> +pass|warn|dmesg-warn|fail|dmesg-fail -> crash|timeout|skip
> +pass|warn|dmesg-warn|fail -> dmesg-fail|crash|timeout|skip
> +pass|warn|dmesg-warn -> fail|dmesg-fail|crash|timeout|skip
> +pass|warn -> dmesg-warn|fail|dmesg-fail|crash|timeout|skip
> +pass -> warn|dmesg-warn|fail|dmesg-fail|crash|timeout|skip
>
>
> The following are fixes:
>
> -skip -> pass|warn|dmesg-warn|fail|dmesg-fail|crash
> -crash|skip - >pass|warn|dmesg-warn|fail|dmesg-fail
> -dmesg-fail|crash|skip -> pass|warn|dmesg-warn|fail
> -fail|dmesg-fail|crash|skip -> pass|warn|dmesg-warn
> -dmesg-warn|fail|dmesg-fail|crash|skip -> pass|warn
> -warn|dmesg-warn|fail|dmesg-fail|crash|skip -> pass
> +skip -> pass|warn|dmesg-warn|fail|dmesg-fail|crash|timeout
> +timeout|skip -> pass|warn|dmesg-warn|fail|dmesg-fail|crash
> +crash|timeout|skip - >pass|warn|dmesg-warn|fail|dmesg-fail
> +dmesg-fail|crash|timeout|skip -> pass|warn|dmesg-warn|fail
> +fail|dmesg-fail|crash|timeout|skip -> pass|warn|dmesg-warn
> +dmesg-warn|fail|dmesg-fail|crash|timeout|skip -> pass|warn
> +warn|dmesg-warn|fail|dmesg-fail|crash|timeout|skip -> pass
>
>
> NotRun -> * and * -> NotRun is a change, but not a fix or a regression.
> This is @@ -71,6 +74,7 @@ def status_lookup(status):
> 'crash': Crash,
> 'dmesg-warn': DmesgWarn,
> 'dmesg-fail': DmesgFail,
> + 'timeout' : Timeout,
> 'notrun': NotRun}
>
> try:
> @@ -205,9 +209,18 @@ class Crash(Status):
> pass
>
>
> +class Timeout(Status):
> + name = 'timeout'
> + value = 50
> + fraction = (0, 0)
Are you sure that you want this to be (0, 0)? That means that tests with this
status will not be counted toward the total number of tests. I don't think
you want to change this from the default (0, 1), but if you're sure I wont
argue :)
> +
> + def __init__(self):
> + pass
> +
> +
> class Skip(Status):
> name = 'skip'
> - value = 50
> + value = 60
> fraction = (0, 0)
>
> def __init__(self):
> diff --git a/templates/index.css b/templates/index.css
> index 577370cec677..3389738abb55 100644
> --- a/templates/index.css
> +++ b/templates/index.css
> @@ -36,7 +36,7 @@ td:first-child > div {
> background-color: #c8c838
> }
>
> -td.skip, td.warn, td.fail, td.pass, td.trap, td.abort, td.crash,
> td.dmesg-warn, td.dmesg-fail { +td.skip, td.warn, td.fail, td.pass,
> td.trap, td.abort, td.crash, td.dmesg-warn, td.dmesg-fail, td.timeout {
> text-align: right;
> }
>
> @@ -67,6 +67,9 @@ tr:nth-child(even) td.fail { background-color: #e00505; }
> tr:nth-child(odd) td.dmesg-fail { background-color: #ff2020; }
> tr:nth-child(even) td.dmesg-fail { background-color: #e00505; }
>
> +tr:nth-child(odd) td.timeout { background-color: #83bdf6; }
> +tr:nth-child(even) td.timeout { background-color: #4a9ef2; }
> +
> tr:nth-child(odd) td.trap { background-color: #111111; }
> tr:nth-child(even) td.trap { background-color: #000000; }
> tr:nth-child(odd) td.abort { background-color: #111111; }
Thanks for making these additions. With the one nit about the status
considered, this is:
reviewed-by: Dylan Baker <baker.dylan.c at gmail.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20131113/f6a8b5fa/attachment-0001.pgp>
More information about the Piglit
mailing list