Patchwork [010,of,179,tests-refactor] run-tests: capture execution results in a TestResult class

login
register
mail settings
Submitter Gregory Szorc
Date May 2, 2014, 6:37 p.m.
Message ID <22f12dcef025b280eb1b.1399055847@vm-ubuntu-main.gateway.sonic.net>
Download mbox | patch
Permalink /patch/4505/
State Accepted
Commit d7a7825ff2cfcf512f7ad469e77ff39bbef50a34
Headers show

Comments

Gregory Szorc - May 2, 2014, 6:37 p.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1397940625 25200
#      Sat Apr 19 13:50:25 2014 -0700
# Branch stable
# Node ID 22f12dcef025b280eb1b8f22280167f562ebc46d
# Parent  b56fd9035cad3f850b8e921a4177b007659ffef6
run-tests: capture execution results in a TestResult class

Some implementation details of test execution still live outside of
Test. These include determining what a result means and cleaning up
after the test.

To move to the world where more of this logic can live inside Test or a
derived object, the logic for test execution needs to be refactored.
Specifically, exception trapping and opportunities for result processing
need to be moved into Test.

This patch starts the process by establishing a TestResult class for
holding the results of a test execution. In order to actually use this
class, exception trapping and execution time recording needed to be
moved into Test.run().
Pierre-Yves David - May 7, 2014, 8:39 p.m.
On 05/02/2014 11:37 AM, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1397940625 25200
> #      Sat Apr 19 13:50:25 2014 -0700
> # Branch stable
> # Node ID 22f12dcef025b280eb1b8f22280167f562ebc46d
> # Parent  b56fd9035cad3f850b8e921a4177b007659ffef6
> run-tests: capture execution results in a TestResult class
>
> Some implementation details of test execution still live outside of
> Test. These include determining what a result means and cleaning up
> after the test.
>
> To move to the world where more of this logic can live inside Test or a
> derived object, the logic for test execution needs to be refactored.
> Specifically, exception trapping and opportunities for result processing
> need to be moved into Test.
>
> This patch starts the process by establishing a TestResult class for
> holding the results of a test execution. In order to actually use this
> class, exception trapping and execution time recording needed to be
> moved into Test.run().
>
> diff --git a/tests/run-tests.py b/tests/run-tests.py
> --- a/tests/run-tests.py
> +++ b/tests/run-tests.py
> @@ -556,20 +556,37 @@ class Test(object):
>           os.mkdir(self._testtmp)
>
>           self._setreplacements(count)
>
>       def run(self):
>           env = self._getenv()
>           createhgrc(env['HGRCPATH'], self._options)
>
> +        result = TestResult()
> +        starttime = time.time()
> +
> +        def updateduration():
> +            result.duration = time.time() - starttime

Not sure why you need this closure here (#ihateclosure)

> +
>           try:
> -            return self._run(self._replacements, env)
> -        finally:
> -            killdaemons(env['DAEMON_PIDS'])
> +            ret, out = self._run(self._replacements, env)
> +            updateduration()
> +            result.ret = ret
> +            result.out = out
> +        except KeyboardInterrupt:
> +            updateduration()
> +            result.interrupted = True
> +        except Exception, e:
> +            updateduration()
> +            result.exception = e

And not sure why you call it in tree different place instead of once. at 
the ends.

Note: This is the end of my review path. Will look at the other 10 when 
those will be queued.

Patch

diff --git a/tests/run-tests.py b/tests/run-tests.py
--- a/tests/run-tests.py
+++ b/tests/run-tests.py
@@ -556,20 +556,37 @@  class Test(object):
         os.mkdir(self._testtmp)
 
         self._setreplacements(count)
 
     def run(self):
         env = self._getenv()
         createhgrc(env['HGRCPATH'], self._options)
 
+        result = TestResult()
+        starttime = time.time()
+
+        def updateduration():
+            result.duration = time.time() - starttime
+
         try:
-            return self._run(self._replacements, env)
-        finally:
-            killdaemons(env['DAEMON_PIDS'])
+            ret, out = self._run(self._replacements, env)
+            updateduration()
+            result.ret = ret
+            result.out = out
+        except KeyboardInterrupt:
+            updateduration()
+            result.interrupted = True
+        except Exception, e:
+            updateduration()
+            result.exception = e
+
+        killdaemons(env['DAEMON_PIDS'])
+
+        return result
 
     def _run(self, replacements, env):
         raise NotImplemented('Subclasses must implement Test.run()')
 
     def _setreplacements(self, count):
         port = self._options.port + count * 3
         r = [
             (r':%s\b' % port, ':$HGPORT'),
@@ -618,16 +635,26 @@  class Test(object):
 
         # unset env related to hooks
         for k in env.keys():
             if k.startswith('HG_'):
                 del env[k]
 
         return env
 
+class TestResult(object):
+    """Holds the result of a test execution."""
+
+    def __init__(self):
+        self.ret = None
+        self.out = None
+        self.duration = None
+        self.interrupted = False
+        self.exception = None
+
 def pytest(test, wd, options, replacements, env):
     py3kswitch = options.py3k_warnings and ' -3' or ''
     cmd = '%s%s "%s"' % (PYTHON, py3kswitch, test)
     vlog("# Running", cmd)
     if os.name == 'nt':
         replacements.append((r'\r\n', '\n'))
     return run(cmd, wd, options, replacements, env)
 
@@ -1018,26 +1045,29 @@  def runone(options, test, count):
         return skip("unknown test type")
 
     vlog("# Test", test)
 
     if os.path.exists(err):
         os.remove(err)       # Remove any previous output files
 
     t = runner(testpath, options, count)
+    res = t.run()
 
-    starttime = time.time()
-    try:
-        ret, out = t.run()
-    except KeyboardInterrupt:
-        endtime = time.time()
-        log('INTERRUPTED: %s (after %d seconds)' % (test, endtime - starttime))
-        raise
-    endtime = time.time()
-    times.append((test, endtime - starttime))
+    if res.interrupted:
+        log('INTERRUPTED: %s (after %d seconds)' % (test, res.duration))
+        raise KeyboardInterrupt()
+
+    if res.exception:
+        return fail('Exception during execution: %s' % res.exception, 255)
+
+    ret = res.ret
+    out = res.out
+
+    times.append((test, res.duration))
     vlog("# Ret was:", ret)
 
     skipped = (ret == SKIPPED_STATUS)
 
     # If we're not in --debug mode and reference output file exists,
     # check test output against it.
     if options.debug:
         refout = None                   # to match "out is None"