Cleans up style problems in architecture.py and test_runner.py

tools/testing/test_runner.py

Index: tools/testing/test_runner.py
diff --git a/tools/testing/test_runner.py b/tools/testing/test_runner.py
index c3cde9f85eec47759b3dc2b8e581ab843d5ddf1d..8cf4f52f4e8782388dcf71d1257903711fa531b8 100755
--- a/tools/testing/test_runner.py
+++ b/tools/testing/test_runner.py
@@ -1,18 +1,24 @@
-#!/usr/bin/env python
-#
 # Copyright (c) 2011, the Dart project authors.  Please see the AUTHORS file
 # for details. All rights reserved. Use of this source code is governed by a
 # BSD-style license that can be found in the LICENSE file.
 #
+"""Classes and methods for executing tasks for the test.py framework.
+
+This module includes:
+  - Windows and Unix specific code for spawning tasks and retrieving results
+  - Managing parallel execution for threads
+"""
 
+import ctypes
 import os
+import Queue
+import signal
 import subprocess
 import sys
 import tempfile
-import time
 import threading
+import time
 import traceback
-import Queue
 
 import testing
 import utils
@@ -22,14 +28,8 @@ class Error(Exception):
   pass
 
 
-def _CheckedUnlink(name):
-  try:
-    os.unlink(name)
-  except OSError, e:
-    PrintError("os.unlink() " + str(e))
-
-
 class CommandOutput(object):
+  """Represents the output of running a command."""
 
   def __init__(self, pid, exit_code, timed_out, stdout, stderr):
     self.pid = pid
@@ -41,13 +41,26 @@ class CommandOutput(object):
 
 
 class TestOutput(object):
+  """Represents the output of running a TestCase."""
 
   def __init__(self, test, command, output):
+    """Represents the output of running a TestCase.
+
+    Args:
+      test: A TestCase instance.
+      command: the command line that was run
+      output: A CommandOutput instance.
+    """
     self.test = test
     self.command = command
     self.output = output
 
   def UnexpectedOutput(self):
+    """Compare the result of running the expected from the TestConfiguration.
+
+    Returns:
+      True if the test had an unexpected output.
+    """
     if self.HasCrashed():
       outcome = testing.CRASH
     elif self.HasTimedOut():
@@ -59,12 +72,14 @@ class TestOutput(object):
     return not outcome in self.test.outcomes
 
   def HasCrashed(self):
+    """Returns True if the test should be considered testing.CRASH."""
     if utils.IsWindows():
       if self.output.exit_code == 3:
         # The VM uses std::abort to terminate on asserts.
         # std::abort terminates with exit code 3 on Windows.
         return True
-      return 0x80000000 & self.output.exit_code and not (0x3FFFFF00 & self.output.exit_code)
+      return (0x80000000 & self.output.exit_code
+              and not 0x3FFFFF00 & self.output.exit_code)
     else:
       # Timed out tests will have exit_code -signal.SIGTERM.
       if self.output.timed_out:
@@ -75,9 +90,11 @@ class TestOutput(object):
       return self.output.exit_code < 0
 
   def HasTimedOut(self):
-    return self.output.timed_out;
+    """Returns True if the test should be considered as testing.TIMEOUT."""
+    return self.output.timed_out
 
   def HasFailed(self):
+    """Returns True if the test should be considered as testing.FAIL."""
     execution_failed = self.test.DidFail(self.output)
     if self.test.IsNegative():
       return not execution_failed
@@ -86,28 +103,35 @@ class TestOutput(object):
 
 
 def Execute(args, context, timeout=None, cwd=None):
+  """Executes the specified command.
+
+  Args:
+    args: sequence of the executable name + arguments.
+    context: An instance of Context object with global settings for test.py.
+    timeout: optional timeout to wait for results in seconds.
+    cwd: optionally change to this working directory.
+
+  Returns:
+    An instance of CommandOutput with the collected results.
+  """
   (fd_out, outname) = tempfile.mkstemp()
   (fd_err, errname) = tempfile.mkstemp()
-  (process, exit_code, timed_out) = RunProcess(
-    context,
-    timeout,
-    args = args,
-    stdout = fd_out,
-    stderr = fd_err,
-    cwd = cwd
-  )
+  (process, exit_code, timed_out) = RunProcess(context, timeout, args=args,
+                                               stdout=fd_out, stderr=fd_err,
+                                               cwd=cwd)
   os.close(fd_out)
   os.close(fd_err)
   output = file(outname).read()
   errors = file(errname).read()
-  _CheckedUnlink(outname)
-  _CheckedUnlink(errname)
+  utils.CheckedUnlink(outname)
+  utils.CheckedUnlink(errname)
   result = CommandOutput(process.pid, exit_code, timed_out,
                          output, errors)
   return result
 
 
 def KillProcessWithID(pid):
+  """Stop a process (with SIGTERM on Unix)."""
   if utils.IsWindows():
     os.popen('taskkill /T /F /PID %d' % pid)
   else:
@@ -117,39 +141,39 @@ def KillProcessWithID(pid):
 MAX_SLEEP_TIME = 0.1
 INITIAL_SLEEP_TIME = 0.0001
 SLEEP_TIME_FACTOR = 1.25
-
 SEM_INVALID_VALUE = -1
-SEM_NOGPFAULTERRORBOX = 0x0002 # Microsoft Platform SDK WinBase.h
+SEM_NOGPFAULTERRORBOX = 0x0002  # Microsoft Platform SDK WinBase.h
 
 
 def Win32SetErrorMode(mode):
+  """Some weird Windows stuff you just have to do."""

[ahe, 2011/10/12 12:14:11]

:-)

[zundel, 2011/10/12 14:13:49]

I see you didn't sleep through this review!

   prev_error_mode = SEM_INVALID_VALUE
   try:
-    import ctypes
-    prev_error_mode = ctypes.windll.kernel32.SetErrorMode(mode);
+    prev_error_mode = ctypes.windll.kernel32.SetErrorMode(mode)
   except ImportError:
     pass
   return prev_error_mode
 
+
 def RunProcess(context, timeout, args, **rest):
-  if context.verbose: print "#", " ".join(args)
+  """Handles the OS specific details of running a task and saving results."""
+  if context.verbose: print '#', ' '.join(args)
   popen_args = args
-  prev_error_mode = SEM_INVALID_VALUE;
+  prev_error_mode = SEM_INVALID_VALUE
   if utils.IsWindows():
     popen_args = '"' + subprocess.list2cmdline(args) + '"'
     if context.suppress_dialogs:
       # Try to change the error mode to avoid dialogs on fatal errors. Don't
       # touch any existing error mode flags by merging the existing error mode.
       # See http://blogs.msdn.com/oldnewthing/archive/2004/07/27/198410.aspx.
-      error_mode = SEM_NOGPFAULTERRORBOX;
-      prev_error_mode = Win32SetErrorMode(error_mode);
-      Win32SetErrorMode(error_mode | prev_error_mode);
-  process = subprocess.Popen(
-    shell = utils.IsWindows(),
-    args = popen_args,
-    **rest
-  )
-  if utils.IsWindows() and context.suppress_dialogs and prev_error_mode != SEM_INVALID_VALUE:
+      error_mode = SEM_NOGPFAULTERRORBOX
+      prev_error_mode = Win32SetErrorMode(error_mode)
+      Win32SetErrorMode(error_mode | prev_error_mode)
+  process = subprocess.Popen(shell=utils.IsWindows(),
+                             args=popen_args,
+                             **rest)
+  if (utils.IsWindows() and context.suppress_dialogs
+      and prev_error_mode != SEM_INVALID_VALUE):
     Win32SetErrorMode(prev_error_mode)
   # Compute the end time - if the process crosses this limit we
   # consider it timed out.
@@ -171,14 +195,15 @@ def RunProcess(context, timeout, args, **rest):
     else:
       exit_code = process.poll()
       time.sleep(sleep_time)
-      sleep_time = sleep_time * SLEEP_TIME_FACTOR
+      sleep_time *= SLEEP_TIME_FACTOR
       if sleep_time > MAX_SLEEP_TIME:
         sleep_time = MAX_SLEEP_TIME
   return (process, exit_code, timed_out)
 
 
 class TestRunner(object):
-  """Base class for runners """
+  """Base class for runners."""
+
   def __init__(self, work_queue, tasks, progress):
     self.work_queue = work_queue
     self.tasks = tasks
@@ -197,7 +222,6 @@ class BatchRunner(TestRunner):
     self.last_activity = {}
     self.context = progress.context
 
-
     # Scale the number of tasks to the nubmer of CPUs on the machine
     # 1:1 is too much of an overload on many machines in batch mode,
     # so scale the ratio of threads to CPUs back.
@@ -222,33 +246,33 @@ class BatchRunner(TestRunner):
                                   stdout=subprocess.PIPE)
         self.runners[thread_number] = runner
         self.FeedTestRunner(runner, thread_number)
-        if self.last_activity.has_key(thread_number):
+        if thread_number in self.last_activity:
           del self.last_activity[thread_number]
 
-        # cleanup
+        # Cleanup
         self.EndRunner(runner)
 
     except:
       self.Shutdown()
       raise
     finally:
-      if self.last_activity.has_key(thread_number):
+      if thread_number in self.last_activity:
         del self.last_activity[thread_number]
       if runner: self.EndRunner(runner)
 
   def EndRunner(self, runner):
-    """ Cleans up a single runner, killing the child if necessary"""
+    """Cleans up a single runner, killing the child if necessary."""
     with self.shutdown_lock:
       if runner:
         returncode = runner.poll()
-        if returncode == None:
+        if returncode is None:
           runner.kill()
       for (found_runner, thread_number) in self.runners.items():
         if runner == found_runner:
           del self.runners[thread_number]
           break
     try:
-      runner.communicate();
+      runner.communicate()
     except ValueError:
       pass
 
@@ -259,7 +283,7 @@ class BatchRunner(TestRunner):
         self.runners[thread_number].kill()
 
   def WaitForCompletion(self):
-    """ Wait for threads to finish, and monitor test runners for timeouts."""
+    """Wait for threads to finish, and monitor test runners for timeouts."""
     for t in self.threads:
       while True:
         self.CheckForTimeouts()
@@ -282,8 +306,9 @@ class BatchRunner(TestRunner):
           self.RecordPassFail(last_case, buf, testing.CRASH)
         else:
           with self.progress.lock:
-            print >>sys. stderr, ("%s: runner unexpectedly exited: %d"
-                   % (threading.currentThread().name, returninfo))
+            print >>sys. stderr, ('%s: runner unexpectedly exited: %d'
+                                  % (threading.currentThread().name,
+                                     returninfo))
             print 'Crash Output: '
             print
             print buf
@@ -297,7 +322,7 @@ class BatchRunner(TestRunner):
       except Queue.Empty:
         return
       test_case = case.case
-      cmd = " ".join(test_case.GetCommand()[1:])
+      cmd = ' '.join(test_case.GetCommand()[1:])
 
       try:
         print >>runner.stdin, cmd
@@ -313,22 +338,22 @@ class BatchRunner(TestRunner):
         self.work_queue.put(case)
         return
 
-      buf = ""
+      buf = ''
       self.last_activity[thread_number] = time.time()
       while not self.terminate:
         line = runner.stdout.readline()
         if self.terminate:
-          break;
-        case.case.duration = time.time() - self.last_activity[thread_number];
+          break
+        case.case.duration = time.time() - self.last_activity[thread_number]
         if not line:
-         # EOF. Child has exited.
-         if case.case.duration > self.context.timeout:
-           with self.progress.lock:
-             print "Child timed out after %d seconds" % self.context.timeout
-           self.RecordPassFail(case, buf, testing.TIMEOUT)
-         elif buf:
-           self.RecordPassFail(case, buf, testing.CRASH)
-         return
+          # EOF. Child has exited.
+          if case.case.duration > self.context.timeout:
+            with self.progress.lock:
+              print 'Child timed out after %d seconds' % self.context.timeout
+            self.RecordPassFail(case, buf, testing.TIMEOUT)
+          elif buf:
+            self.RecordPassFail(case, buf, testing.CRASH)
+          return
 
         # Look for TestRunner batch status escape sequence.  e.g.
         # >>> TEST PASS
@@ -368,13 +393,13 @@ class BatchRunner(TestRunner):
     elif outcome == testing.FAIL or outcome == testing.TIMEOUT:
       exit_code = 1
     else:
-      assert false, "Unexpected outcome: %s" % outcome
+      assert False, 'Unexpected outcome: %s' % outcome
 
     cmd_output = CommandOutput(0, exit_code,
-                                    outcome == testing.TIMEOUT, stdout_buf, "")
+                               outcome == testing.TIMEOUT, stdout_buf, '')
     test_output = TestOutput(case.case,
-                                  case.case.GetCommand(),
-                                  cmd_output)
+                             case.case.GetCommand(),
+                             cmd_output)
     with self.progress.lock:
       if test_output.UnexpectedOutput():
         self.progress.failed.append(test_output)
@@ -386,9 +411,9 @@ class BatchRunner(TestRunner):
       self.progress.HasRun(test_output)
 
   def Shutdown(self):
-    """Kill all active runners"""
-    print "Shutting down remaining runners"
-    self.terminate = True;
+    """Kill all active runners."""
+    print 'Shutting down remaining runners.'
+    self.terminate = True
     for runner in self.runners.values():
       runner.kill()
     # Give threads a chance to exit gracefully