Addresses Python style issues in test_configuration.py and test_case.py

tools/testing/test_configuration.py

Index: tools/testing/test_configuration.py
diff --git a/tools/testing/test_configuration.py b/tools/testing/test_configuration.py
index 1b5bca3b07ade573195477d1b740e0afc1f7cc3b..ea7f0adc863611c9c43bb09282c61ca6b7103737 100644
--- a/tools/testing/test_configuration.py
+++ b/tools/testing/test_configuration.py
@@ -2,31 +2,30 @@
 # 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.
 
+"""Common Testconfiguration subclasses used to define a class of tests."""
+
 import atexit
 import fileinput
 import os
-import test
-import platform
 import re
 import shutil
-import sys
-import tempfile
 
-from testing import test_case
+
 import test
+from testing import test_case
 import utils
 
-from os.path import join, exists, basename
-
-import utils
 
 class Error(Exception):
   pass
 
+
 class TestConfigurationError(Error):
   pass
 
+
 class StandardTestConfiguration(test.TestConfiguration):
+  """Configuration that looks for .dart files in the tests/*/src dirs."""

[ngeoffray, 2011/10/13 08:03:38]

*Test.dart?

[zundel, 2011/10/13 11:07:44]

Done.

   LEGAL_KINDS = set(['compile-time error',
                      'runtime error',
                      'static type error',
@@ -35,24 +34,27 @@ class StandardTestConfiguration(test.TestConfiguration):
   def __init__(self, context, root):
     super(StandardTestConfiguration, self).__init__(context, root)
 
-  def _cleanup(self, tests):
-    if self.context.keep_temporary_files: return
+  def _Cleanup(self, tests):
+    """Remove any temporary files created by running the test."""
+    if self.context.keep_temporary_files:
+      return
 
     dirs = []
     for t in tests:
-      if t.run_arch != None:
+      if t.run_arch:
         temp_dir = t.run_arch.temp_dir
-        if temp_dir != None: dirs.append(temp_dir)
-
-    if len(dirs) == 0: return
-    if not utils.Daemonize(): return
-
+        if temp_dir:
+          dirs.append(temp_dir)
+    if not dirs:
+      return
+    if not utils.Daemonize():
+      return
     os.execlp('rm', *(['rm', '-rf'] + dirs))
 
-
   def CreateTestCases(self, test_path, path, filename, mode, arch):
+    """Given a .dart filename, create a StandardTestCase from it."""
     tags = {}
-    if filename.endswith(".dart"):
+    if filename.endswith('.dart'):
       tags = self.SplitMultiTest(test_path, filename)
     if arch in ['dartium', 'chromium']:
       if tags:
@@ -68,36 +70,48 @@ class StandardTestConfiguration(test.TestConfiguration):
           if not self.Contains(path, test_path + [tag]):
             continue
           tests.append(test_case.MultiTestCase(self.context,
-              test_path + [tag], test_source, kind, mode, arch))
+                                               test_path + [tag],
+                                               test_source,
+                                               kind,
+                                               mode, arch))
       else:
         tests.append(test_case.StandardTestCase(self.context,
-            test_path, filename, mode, arch))
+                                                test_path,
+                                                filename,
+                                                mode, arch))
       return tests
 
   def ListTests(self, current_path, path, mode, arch):
+    """Searches for .dart files that qualify and returns list of TestCases."""

[ngeoffray, 2011/10/13 08:03:38]

ditto

[zundel, 2011/10/13 11:07:44]

Done.

     tests = []
-    for root, dirs, files in os.walk(join(self.root, 'src')):
+    for root, unused_dirs, files in os.walk(os.path.join(self.root, 'src')):
       for f in [x for x in files if self.IsTest(x)]:
-        if f.endswith(".dart"):
-          test_path = current_path + [ f[:-5] ]  # Remove .dart suffix.
-        elif f.endswith(".app"):
-          test_path = current_path + [ f[:-4] ]  # Remove .app suffix.
+        if f.endswith('.dart'):
+          test_path = current_path + [f[:-5]]  # Remove .dart suffix.
+        elif f.endswith('.app'):
+          # TODO(zundel): .app files are used only the dromaeo test
+          # and should be removed.
+          test_path = current_path + [f[:-4]]  # Remove .app suffix.
         if not self.Contains(path, test_path):
           continue
-        tests.extend(self.CreateTestCases(test_path, path, join(root, f),
+        tests.extend(self.CreateTestCases(test_path, path,
+                                          os.path.join(root, f),
                                           mode, arch))
-    atexit.register(lambda: self._cleanup(tests))
+    atexit.register(lambda: self._Cleanup(tests))
     return tests
 
   def IsTest(self, name):
-    return name.endswith('Test.dart') or name.endswith("Test.app")
+    """Returns True if the file name looks like a test file."""

[ngeoffray, 2011/10/13 08:03:38]

looks like -> is?

[zundel, 2011/10/13 11:07:44]

Done.

+    return name.endswith('Test.dart') or name.endswith('Test.app')
 
   def GetTestStatus(self, sections, defs):
-    status = join(self.root, basename(self.root) + '.status')
-    if exists(status):
+    """Populates the sections and defs parameters if the file exists."""

[ngeoffray, 2011/10/13 08:03:38]

Change to """Reads the status file of the test suite."""?

[zundel, 2011/10/13 11:07:44]

Done.

+    status = os.path.join(self.root, os.path.basename(self.root) + '.status')
+    if os.path.exists(status):
       test.ReadConfigurationInto(status, sections, defs)
 
   def FindReferencedFiles(self, lines):
+    """Scours the lines containing source code for include directives."""
     referenced_files = []
     for line in lines:
       m = re.match("#(source|import)\(['\"](.*)['\"]\);", line)
@@ -106,19 +120,34 @@ class StandardTestConfiguration(test.TestConfiguration):
     return referenced_files
 
   def SplitMultiTest(self, test_path, filename):
+    """Takes a file with multiple test case defined.
+
+    Splits the file into multiple TestCase instances.
+
+    Args:
+      test_path: temporary dir to write split test case data.
+      filename: name of the file to split.
+
+    Returns:
+      sequence of test cases split from file.
+
+    Raises:
+      TestConfigurationError: when a problem with the multi-test-case
+          syntax is encountered.
+    """
     (name, extension) = os.path.splitext(os.path.basename(filename))
     with open(filename, 'r') as s:
       source = s.read()
     lines = source.splitlines()
     tags = {}
     for line in lines:
-      (code, sep, info) = line.partition(' /// ')
+      (unused_code, sep, info) = line.partition(' /// ')
       if sep:
         (tag, sep, kind) = info.partition(': ')
-        if tags.has_key(tag):
-          raise utils.Error('duplicated tag %s' % tag)
+        if tag in tags:
+          raise TestConfigurationError('duplicated tag %s' % tag)
         if kind not in StandardTestConfiguration.LEGAL_KINDS:
-          raise utils.Error('unrecognized kind %s' % kind)
+          raise TestConfigurationError('unrecognized kind %s' % kind)
         tags[tag] = kind
     if not tags:
       return {}
@@ -161,65 +190,29 @@ class StandardTestConfiguration(test.TestConfiguration):
     tests['none'] = ('', test_filename)
     return tests
 
-class BrowserTestCase(test_case.StandardTestCase):
-  def __init__(self, context, path, filename,
-               fatal_static_type_errors, mode, arch):
-    super(test_case.BrowserTestCase, self).__init__(context, path, filename, mode, arch)
-    self.fatal_static_type_errors = fatal_static_type_errors
-
-  def IsBatchable(self):
-    return True
-      
-  def Run(self):
-    command = self.run_arch.GetCompileCommand(self.fatal_static_type_errors)
-    if command != None:
-      # We change the directory where dartc will be launched because
-      # it is not predictable on the location of the compiled file. In
-      # case the test is a web test, we make sure the app file is not
-      # in a subdirectory.
-      cwd = None
-      if self.run_arch.is_web_test: cwd = self.run_arch.temp_dir
-      command = command[:1] + self.context.flags + command[1:]
-      test_output = self.RunCommand(command, cwd=cwd)
-
-      # If errors were found, fail fast and show compile errors:
-      if test_output.output.exit_code != 0:
-        if not self.context.keep_temporary_files:
-          self.run_arch.Cleanup()
-        return test_output
-
-    command = self.run_arch.GetRunCommand();
-    test_output = self.RunCommand(command)
-    # The return value of DumpRenderedTree does not indicate test failing, but
-    # the output does.
-    if self.run_arch.HasFailed(test_output.output.stdout):
-      test_output.output.exit_code = 1
-
-    # TODO(ngeoffray): We run out of space on the build bots for these tests if
-    # the temp directories are not removed right after running the test.
-    if not self.context.keep_temporary_files:
-      self.run_arch.Cleanup()
-
-    return test_output
-
 
 class BrowserTestConfiguration(StandardTestConfiguration):
+  """A configuration used to run tests inside a browser."""
+
   def __init__(self, context, root, fatal_static_type_errors=False):
     super(BrowserTestConfiguration, self).__init__(context, root)
     self.fatal_static_type_errors = fatal_static_type_errors
 
   def ListTests(self, current_path, path, mode, arch):
+    """Searches for .dart files that qualify and returns list of TestCases."""

[ngeoffray, 2011/10/13 08:03:38]

*Test.dart

[zundel, 2011/10/13 11:07:44]

Done.

     tests = []
-    for root, dirs, files in os.walk(self.root):
+    for root, unused_dirs, files in os.walk(self.root):
       for f in [x for x in files if self.IsTest(x)]:
         relative = os.path.relpath(root, self.root).split(os.path.sep)
         test_path = current_path + relative + [os.path.splitext(f)[0]]
         if not self.Contains(path, test_path):
           continue
         tests.append(test_case.BrowserTestCase(self.context,
-            test_path, join(root, f), self.fatal_static_type_errors, mode,
-            arch))
-    atexit.register(lambda: self._cleanup(tests))
+                                               test_path,
+                                               os.path.join(root, f),
+                                               self.fatal_static_type_errors,
+                                               mode, arch))
+    atexit.register(lambda: self._Cleanup(tests))
     return tests
 
   def IsTest(self, name):
@@ -227,14 +220,16 @@ class BrowserTestConfiguration(StandardTestConfiguration):
 
 
 class CompilationTestConfiguration(test.TestConfiguration):
-  """ Configuration that searches specific directories for apps to compile
+  """Configuration that searches specific directories for apps to compile.
 
-      Expects a status file named  dartc.status
+  Expects a status file named  dartc.status
   """
+
   def __init__(self, context, root):
     super(CompilationTestConfiguration, self).__init__(context, root)
 
   def ListTests(self, current_path, path, mode, arch):
+    """Searches for .dart files that qualify and returns list of TestCases."""

[ngeoffray, 2011/10/13 08:03:38]

*Test.dart

[zundel, 2011/10/13 11:07:44]

Done.

     tests = []
     client_path = os.path.normpath(os.path.join(self.root, '..', '..'))
 
@@ -246,32 +241,33 @@ class CompilationTestConfiguration(test.TestConfiguration):
         for f in files:
           filename = [os.path.basename(client_path)]
           filename.extend(root[len(client_path) + 1:].split(os.path.sep))
-          filename.append(f) # Remove .lib or .app suffix.
+          filename.append(f)  # Remove .lib or .app suffix.
           test_path = current_path + filename
           test_dart_file = os.path.join(root, f)
-          if ((not self.Contains(path, test_path)) 
-              or (not self.IsTest(test_dart_file))):
+          if (not self.Contains(path, test_path)
+              or not self.IsTest(test_dart_file)):
             continue
           tests.append(test_case.CompilationTestCase(test_path,
-                                           self.context,
-                                           test_dart_file,
-                                           mode,
-                                           arch))
-    atexit.register(lambda: self._cleanup(tests))
+                                                     self.context,
+                                                     test_dart_file,
+                                                     mode,
+                                                     arch))
+    atexit.register(lambda: self._Cleanup(tests))
     return tests
 
   def SourceDirs(self):
-    """ Returns a list of directories to scan for files to compile """
+    """Returns a list of directories to scan for files to compile."""
     raise TestConfigurationError(
-        "Subclasses must implement SourceDirs()")
+        'Subclasses must implement SourceDirs()')
 
   def IsTest(self, name):
-    if (not name.endswith('.dart')):
+    """Returns True if name looks like a test case to be compiled."""

[ngeoffray, 2011/10/13 08:03:38]

looks like -> is

[zundel, 2011/10/13 11:07:44]

Done.

+    if not name.endswith('.dart'):
       return False
-    if (os.path.exists(name)):
+    if os.path.exists(name):
       # TODO(dgrove): can we end reading the input early?
       for line in fileinput.input(name):
-        if (re.match('#', line)):
+        if re.match('#', line):
           fileinput.close()
           return True
       fileinput.close()
@@ -283,7 +279,7 @@ class CompilationTestConfiguration(test.TestConfiguration):
     if os.path.exists(status):
       test.ReadConfigurationInto(status, sections, defs)
 
-  def _cleanup(self, tests):
+  def _Cleanup(self, tests):
     if not utils.Daemonize(): return
     os.execlp('rm', *(['rm', '-rf'] + [t.temp_dir for t in tests]))
     raise