2010-09-20 Dirk Pranke <dpranke@chromium.org>
authordpranke@chromium.org <dpranke@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 21 Sep 2010 01:26:10 +0000 (01:26 +0000)
committerdpranke@chromium.org <dpranke@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 21 Sep 2010 01:26:10 +0000 (01:26 +0000)
        Reviewed by Ojan Vafai.

        new-run-webkit-tests: refactor command line args getting passed to DRT

        This change cleans up some argument parsing between functions to get
        rid of some overlapping data structures. There should be no functional
        changes in this patch; it is pure refactoring in preparation for
        landing the Chrome GPU port and adding a generic way to pass
        args to DRT/TestShell.

        https://bugs.webkit.org/show_bug.cgi?id=46135

        * Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:
          - pass the options argument explicitly to the threads and drivers,
            also consolidate the passing of options to the driver.
          - pass options directly to process_output() to remove a couple
            parameters (minor cleanup).
        * Scripts/webkitpy/layout_tests/port/base.py:
          - pass the options argument to Port.create_driver().
        * Scripts/webkitpy/layout_tests/port/base_unittest.py:
          - update Port.create_driver() test
        * Scripts/webkitpy/layout_tests/port/chromium.py:
          - pass the options argument to Port.create_driver(), and clean up
            building of the cmd line for DRT.
        * Scripts/webkitpy/layout_tests/port/dryrun.py:
          - pass the options argument to Port.create_driver()
        * Scripts/webkitpy/layout_tests/port/test.py:
          - pass the options argument to Port.create_driver()
        * Scripts/webkitpy/layout_tests/port/webkit.py:
          - pass the options argument to Port.create_driver(), and clean up
            building of the cmd line for DRT.
        * Scripts/webkitpy/layout_tests/run_webkit_tests.py:
          - consolidate args in _get_dump_render_tree_args and rename to
            _get_test_args(); move all of the command-line args to the
            Port implementations.

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@67905 268f45cc-cd09-0410-ab3c-d52691b4dbfc

WebKitTools/ChangeLog
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py
WebKitTools/Scripts/webkitpy/layout_tests/port/base.py
WebKitTools/Scripts/webkitpy/layout_tests/port/base_unittest.py
WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py
WebKitTools/Scripts/webkitpy/layout_tests/port/dryrun.py
WebKitTools/Scripts/webkitpy/layout_tests/port/test.py
WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py
WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py

index 2701c7dfc6032e3a7f20e686a823d3951d2c11b6..eace217f5637316caf9f149ac6f2b20e98959123 100644 (file)
@@ -1,3 +1,41 @@
+2010-09-20  Dirk Pranke  <dpranke@chromium.org>
+
+        Reviewed by Ojan Vafai.
+
+        new-run-webkit-tests: refactor command line args getting passed to DRT
+
+        This change cleans up some argument parsing between functions to get
+        rid of some overlapping data structures. There should be no functional
+        changes in this patch; it is pure refactoring in preparation for
+        landing the Chrome GPU port and adding a generic way to pass
+        args to DRT/TestShell.
+
+        https://bugs.webkit.org/show_bug.cgi?id=46135
+
+        * Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:
+          - pass the options argument explicitly to the threads and drivers,
+            also consolidate the passing of options to the driver.
+          - pass options directly to process_output() to remove a couple
+            parameters (minor cleanup).
+        * Scripts/webkitpy/layout_tests/port/base.py:
+          - pass the options argument to Port.create_driver().
+        * Scripts/webkitpy/layout_tests/port/base_unittest.py:
+          - update Port.create_driver() test
+        * Scripts/webkitpy/layout_tests/port/chromium.py:
+          - pass the options argument to Port.create_driver(), and clean up
+            building of the cmd line for DRT.
+        * Scripts/webkitpy/layout_tests/port/dryrun.py:
+          - pass the options argument to Port.create_driver()
+        * Scripts/webkitpy/layout_tests/port/test.py:
+          - pass the options argument to Port.create_driver()
+        * Scripts/webkitpy/layout_tests/port/webkit.py:
+          - pass the options argument to Port.create_driver(), and clean up
+            building of the cmd line for DRT.
+        * Scripts/webkitpy/layout_tests/run_webkit_tests.py:
+          - consolidate args in _get_dump_render_tree_args and rename to
+            _get_test_args(); move all of the command-line args to the
+            Port implementations.
+
 2010-09-20  Andrew Wilson  <atwilson@chromium.org>
 
         Revert change which was accidentally committed along with some expectation changes.
index 9b963ca9f7dd0c5333ca2b8dadff94bccb745247..970de60cc61c0fd33ad9c2dbcfff14368eaf5625 100644 (file)
@@ -72,20 +72,19 @@ def log_stack(stack):
             _log.error('  %s' % line.strip())
 
 
-def _process_output(port, test_info, test_types, test_args, configuration,
-                    output_dir, crash, timeout, test_run_time, actual_checksum,
+def _process_output(port, options, test_info, test_types, test_args,
+                    crash, timeout, test_run_time, actual_checksum,
                     output, error):
     """Receives the output from a DumpRenderTree process, subjects it to a
     number of tests, and returns a list of failure types the test produced.
 
     Args:
       port: port-specific hooks
+      options: command line options argument from optparse
       proc: an active DumpRenderTree process
       test_info: Object containing the test filename, uri and timeout
       test_types: list of test types to subject the output to
       test_args: arguments to be passed to each test
-      configuration: Debug or Release
-      output_dir: directory to put crash stack traces into
 
     Returns: a TestResult object
     """
@@ -106,7 +105,8 @@ def _process_output(port, test_info, test_types, test_args, configuration,
         _log.debug("Stacktrace for %s:\n%s" % (test_info.filename, error))
         # Strip off "file://" since RelativeTestFilename expects
         # filesystem paths.
-        filename = os.path.join(output_dir, port.relative_test_filename(
+        filename = os.path.join(options.results_directory,
+                                port.relative_test_filename(
                                 test_info.filename))
         filename = os.path.splitext(filename)[0] + "-stack.txt"
         port.maybe_make_directory(os.path.split(filename)[0])
@@ -122,7 +122,7 @@ def _process_output(port, test_info, test_types, test_args, configuration,
         start_diff_time = time.time()
         new_failures = test_type.compare_output(port, test_info.filename,
                                                 output, local_test_args,
-                                                configuration)
+                                                options.configuration)
         # Don't add any more failures if we already have a crash, so we don't
         # double-report those tests. We do double-report for timeouts since
         # we still want to see the text and image output.
@@ -166,25 +166,23 @@ class TestResult(object):
 class SingleTestThread(threading.Thread):
     """Thread wrapper for running a single test file."""
 
-    def __init__(self, port, image_path, shell_args, test_info,
-        test_types, test_args, configuration, output_dir):
+    def __init__(self, port, options, test_info, test_types, test_args):
         """
         Args:
           port: object implementing port-specific hooks
+          options: command line argument object from optparse
           test_info: Object containing the test filename, uri and timeout
-          output_dir: Directory to put crash stacks into.
-          See TestShellThread for documentation of the remaining arguments.
+          test_types: A list of TestType objects to run the test output
+              against.
+          test_args: A TestArguments object to pass to each TestType.
         """
 
         threading.Thread.__init__(self)
         self._port = port
-        self._image_path = image_path
-        self._shell_args = shell_args
+        self._options = options
         self._test_info = test_info
         self._test_types = test_types
         self._test_args = test_args
-        self._configuration = configuration
-        self._output_dir = output_dir
         self._driver = None
 
     def run(self):
@@ -194,17 +192,17 @@ class SingleTestThread(threading.Thread):
         # FIXME: this is a separate routine to work around a bug
         # in coverage: see http://bitbucket.org/ned/coveragepy/issue/85.
         test_info = self._test_info
-        self._driver = self._port.create_driver(self._image_path,
-                                                self._shell_args)
+        self._driver = self._port.create_driver(self._test_args.png_path,
+                                                self._options)
         self._driver.start()
         start = time.time()
         crash, timeout, actual_checksum, output, error = \
             self._driver.run_test(test_info.uri.strip(), test_info.timeout,
                                   test_info.image_hash())
         end = time.time()
-        self._test_result = _process_output(self._port,
+        self._test_result = _process_output(self._port, self._options,
             test_info, self._test_types, self._test_args,
-            self._configuration, self._output_dir, crash, timeout, end - start,
+            crash, timeout, end - start,
             actual_checksum, output, error)
         self._driver.stop()
 
@@ -248,12 +246,13 @@ class WatchableThread(threading.Thread):
 
 
 class TestShellThread(WatchableThread):
-    def __init__(self, port, filename_list_queue, result_queue,
-                 test_types, test_args, image_path, shell_args, options):
+    def __init__(self, port, options, filename_list_queue, result_queue,
+                 test_types, test_args):
         """Initialize all the local state for this DumpRenderTree thread.
 
         Args:
           port: interface to port-specific hooks
+          options: command line options argument from optparse
           filename_list_queue: A thread safe Queue class that contains lists
               of tuples of (filename, uri) pairs.
           result_queue: A thread safe Queue class that will contain tuples of
@@ -261,22 +260,17 @@ class TestShellThread(WatchableThread):
           test_types: A list of TestType objects to run the test output
               against.
           test_args: A TestArguments object to pass to each TestType.
-          shell_args: Any extra arguments to be passed to DumpRenderTree.
-          options: A property dictionary as produced by optparse. The
-              command-line options should match those expected by
-              run_webkit_tests; they are typically passed via the
-              run_webkit_tests.TestRunner class."""
+
+        """
         WatchableThread.__init__(self)
         self._port = port
+        self._options = options
         self._filename_list_queue = filename_list_queue
         self._result_queue = result_queue
         self._filename_list = []
         self._test_types = test_types
         self._test_args = test_args
         self._driver = None
-        self._image_path = image_path
-        self._shell_args = shell_args
-        self._options = options
         self._directory_timing_stats = {}
         self._test_results = []
         self._num_tests = 0
@@ -433,13 +427,11 @@ class TestShellThread(WatchableThread):
           A TestResult
 
         """
-        worker = SingleTestThread(self._port, self._image_path,
-                                  self._shell_args,
+        worker = SingleTestThread(self._port,
+                                  self._options,
                                   test_info,
                                   self._test_types,
-                                  self._test_args,
-                                  self._options.configuration,
-                                  self._options.results_directory)
+                                  self._test_args)
 
         worker.start()
 
@@ -503,11 +495,11 @@ class TestShellThread(WatchableThread):
            self._driver.run_test(test_info.uri, test_info.timeout, image_hash)
         end = time.time()
 
-        result = _process_output(self._port, test_info, self._test_types,
-                                self._test_args, self._options.configuration,
-                                self._options.results_directory, crash,
-                                timeout, end - start, actual_checksum,
-                                output, error)
+        result = _process_output(self._port, self._options,
+                                 test_info, self._test_types,
+                                 self._test_args, crash,
+                                 timeout, end - start, actual_checksum,
+                                 output, error)
         self._test_results.append(result)
         return result
 
@@ -521,7 +513,8 @@ class TestShellThread(WatchableThread):
         # poll() is not threadsafe and can throw OSError due to:
         # http://bugs.python.org/issue1731717
         if (not self._driver or self._driver.poll() is not None):
-            self._driver = self._port.create_driver(self._image_path, self._shell_args)
+            self._driver = self._port.create_driver(self._test_args.png_path,
+                                                    self._options)
             self._driver.start()
 
     def _kill_dump_render_tree(self):
index c21c88947bc97c06ca70d946618c50f2d597f875..70beac32493bb685ed06c6b6abe9ff7ead817fa2 100644 (file)
@@ -374,7 +374,7 @@ class Port(object):
         results_filename in a users' browser."""
         raise NotImplementedError('Port.show_html_results_file')
 
-    def create_driver(self, png_path, options):
+    def create_driver(self, image_path, options):
         """Return a newly created base.Driver subclass for starting/stopping
         the test driver."""
         raise NotImplementedError('Port.create_driver')
@@ -688,7 +688,7 @@ class Port(object):
 class Driver:
     """Abstract interface for the DumpRenderTree interface."""
 
-    def __init__(self, port, png_path, options):
+    def __init__(self, port, png_path, options, executive):
         """Initialize a Driver to subsequently run tests.
 
         Typically this routine will spawn DumpRenderTree in a config
@@ -698,7 +698,10 @@ class Driver:
         png_path - an absolute path for the driver to write any image
             data for a test (as a PNG). If no path is provided, that
             indicates that pixel test results will not be checked.
-        options - any port-specific driver options."""
+        options - command line options argument from optparse
+        executive - reference to the process-wide Executive object
+
+        """
         raise NotImplementedError('Driver.__init__')
 
     def run_test(self, uri, timeout, checksum):
index 407b9061c1f47ca1e79b7da9d4305e2a8a12c9d6..780cd22f94515256cf7bf6bc54d3f0d7d905d47d 100644 (file)
@@ -250,8 +250,8 @@ class VirtualTest(unittest.TestCase):
         self.assertVirtual(port._shut_down_http_server, None)
 
     def test_virtual_driver_method(self):
-        self.assertRaises(NotImplementedError, base.Driver, base.Port, "", None)
-        self.assertVirtual(base.Driver, base.Port, "", None)
+        self.assertRaises(NotImplementedError, base.Driver, base.Port(),
+                          "", None, None)
 
     def test_virtual_driver_methods(self):
         class VirtualDriver(base.Driver):
index 896eab15e5b126055f8a07e60400b5422027afc2..3fc46134714ad4d2efc1c88c6b0b3f928342d380 100644 (file)
@@ -194,13 +194,11 @@ class ChromiumPort(base.Port):
 
     def create_driver(self, image_path, options):
         """Starts a new Driver and returns a handle to it."""
-        if self._options.use_drt and sys.platform == 'darwin':
-            return webkit.WebKitDriver(self, image_path, options, executive=self._executive)
-        if self._options.use_drt:
-            options += ['--test-shell']
-        else:
-            options += ['--layout-tests']
-        return ChromiumDriver(self, image_path, options, executive=self._executive)
+        if options.use_drt and sys.platform == 'darwin':
+            return webkit.WebKitDriver(self, image_path, options,
+                                       executive=self._executive)
+        return ChromiumDriver(self, image_path, options,
+                              executive=self._executive)
 
     def start_helper(self):
         helper_path = self._path_to_helper()
@@ -337,22 +335,32 @@ class ChromiumDriver(base.Driver):
 
     def __init__(self, port, image_path, options, executive=Executive()):
         self._port = port
-        self._configuration = port._options.configuration
-        # FIXME: _options is very confusing, because it's not an Options() element.
-        # FIXME: These don't need to be passed into the constructor, but could rather
-        # be passed into .start()
         self._options = options
         self._image_path = image_path
         self._executive = executive
 
+    def _driver_args(self):
+        driver_args = []
+        if self._image_path:
+            driver_args.append("--pixel-tests=" + self._image_path)
+
+        if self._options.use_drt:
+            driver_args.append('--test-shell')
+        else:
+            driver_args.append('--layout-tests')
+
+        if self._options.startup_dialog:
+            driver_args.append('--testshell-startup-dialog')
+
+        if self._options.gp_fault_error_box:
+            driver_args.append('--gp-fault-error-box')
+        return driver_args
+
     def start(self):
         # FIXME: Should be an error to call this method twice.
-        cmd = []
-        # FIXME: We should not be grabbing at self._port._options.wrapper directly.
-        cmd += self._command_wrapper(self._port._options.wrapper)
-        cmd += [self._port._path_to_driver()]
-        if self._options:
-            cmd += self._options
+        cmd = self._command_wrapper(self._options.wrapper)
+        cmd.append(self._port._path_to_driver())
+        cmd += self._driver_args()
 
         # We need to pass close_fds=True to work around Python bug #2320
         # (otherwise we can hang when we kill DumpRenderTree when we are running
index 1af01ad90833d21270225c11c5f4bd81f6033c7e..4940e4c22c94ee8497cfcd2e3d774278509381fb 100644 (file)
@@ -86,6 +86,7 @@ class DryRunPort(object):
             port_name = port_name[len(pfx):]
         else:
             port_name = None
+        self._options = options
         self.__delegate = factory.get(port_name, options)
 
     def __getattr__(self, name):
@@ -116,16 +117,17 @@ class DryRunPort(object):
         pass
 
     def create_driver(self, image_path, options):
-        return DryrunDriver(self, image_path, options)
+        return DryrunDriver(self, image_path, options, executive=None)
 
 
 class DryrunDriver(base.Driver):
     """Dryrun implementation of the DumpRenderTree / Driver interface."""
 
-    def __init__(self, port, image_path, test_driver_options):
+    def __init__(self, port, image_path, options, executive):
         self._port = port
-        self._driver_options = test_driver_options
+        self._options = options
         self._image_path = image_path
+        self._executive = executive
         self._layout_tests_dir = None
 
     def poll(self):
index a3a16c34a5a2266746af5e080ddc6452019ccf47..2ccddb072952329fd3fe9ca92221e10002305fc4 100644 (file)
@@ -97,7 +97,7 @@ class TestPort(base.Port):
         pass
 
     def create_driver(self, image_path, options):
-        return TestDriver(image_path, options, self)
+        return TestDriver(self, image_path, options, executive=None)
 
     def start_http_server(self):
         pass
@@ -139,10 +139,11 @@ class TestPort(base.Port):
 class TestDriver(base.Driver):
     """Test/Dummy implementation of the DumpRenderTree interface."""
 
-    def __init__(self, image_path, test_driver_options, port):
-        self._driver_options = test_driver_options
-        self._image_path = image_path
+    def __init__(self, port, image_path, options, executive):
         self._port = port
+        self._image_path = image_path
+        self._options = options
+        self._executive = executive
         self._image_written = False
 
     def poll(self):
@@ -204,7 +205,7 @@ class TestDriver(base.Driver):
             crash = False
             timeout = False
             output = basename + '-txt\n'
-            if self._port.options().pixel_tests and (
+            if self._options.pixel_tests and (
                 'image' in basename or 'check' in basename):
                 checksum = basename + '-checksum\n'
                 with open(self._image_path, "w") as f:
index b085ceb7e68fc6bdfc25bea2aafae8ab8bc92db8..88c9bdf982d52d33b2109a80fe2cc7e88b15119b 100644 (file)
@@ -199,7 +199,8 @@ class WebKitPort(base.Port):
         webbrowser.open(uri, new=1)
 
     def create_driver(self, image_path, options):
-        return WebKitDriver(self, image_path, options, executive=self._executive)
+        return WebKitDriver(self, image_path, options,
+                            executive=self._executive)
 
     def test_base_platform_names(self):
         # At the moment we don't use test platform names, but we have
@@ -405,22 +406,31 @@ class WebKitPort(base.Port):
 class WebKitDriver(base.Driver):
     """WebKit implementation of the DumpRenderTree interface."""
 
-    def __init__(self, port, image_path, driver_options, executive=Executive()):
+    def __init__(self, port, image_path, options, executive=Executive()):
         self._port = port
-        # FIXME: driver_options is never used.
         self._image_path = image_path
+        self._options = options
+        self._executive = executive
         self._driver_tempdir = tempfile.mkdtemp(prefix='DumpRenderTree-')
 
     def __del__(self):
         shutil.rmtree(self._driver_tempdir)
 
+    def _driver_args(self):
+        driver_args = []
+        if self._image_path:
+            driver_args.append('--pixel-tests')
+
+        # These are used by the Chromium DRT port
+        if self._options.use_drt:
+            driver_args.append('--test-shell')
+        return driver_args
+
     def start(self):
-        command = []
-        # FIXME: We should not be grabbing at self._port._options.wrapper directly.
-        command += self._command_wrapper(self._port._options.wrapper)
+        command = self._command_wrapper(self._options.wrapper)
         command += [self._port._path_to_driver(), '-']
-        if self._image_path:
-            command.append('--pixel-tests')
+        command += self._driver_args()
+
         environment = self._port.setup_environ_for_server()
         environment['DYLD_FRAMEWORK_PATH'] = self._port._build_path()
         environment['DUMPRENDERTREE_TEMP'] = self._driver_tempdir
index 2e2da6d587377180c5bfbd1748db8594efaaa225..14d4f0e99fa619d8af46b591b57a0f94341ba4ce 100755 (executable)
@@ -564,27 +564,18 @@ class TestRunner:
             filename_queue.put(item)
         return filename_queue
 
-    def _get_dump_render_tree_args(self, index):
+    def _get_test_args(self, index):
         """Returns the tuple of arguments for tests and for DumpRenderTree."""
-        shell_args = []
         test_args = test_type_base.TestArguments()
-        png_path = None
+        test_args.png_path = None
         if self._options.pixel_tests:
             png_path = os.path.join(self._options.results_directory,
                                     "png_result%s.png" % index)
-            shell_args.append("--pixel-tests=" + png_path)
             test_args.png_path = png_path
-
         test_args.new_baseline = self._options.new_baseline
         test_args.reset_results = self._options.reset_results
 
-        if self._options.startup_dialog:
-            shell_args.append('--testshell-startup-dialog')
-
-        if self._options.gp_fault_error_box:
-            shell_args.append('--gp-fault-error-box')
-
-        return test_args, png_path, shell_args
+        return test_args
 
     def _contains_tests(self, subdir):
         for test_file in self._test_files:
@@ -610,11 +601,10 @@ class TestRunner:
                 test_types.append(test_type(self._port,
                                     self._options.results_directory))
 
-            test_args, png_path, shell_args = \
-                self._get_dump_render_tree_args(i)
+            test_args = self._get_test_args(i)
             thread = dump_render_tree_thread.TestShellThread(self._port,
-                filename_queue, self._result_queue, test_types, test_args,
-                png_path, shell_args, self._options)
+                self._options, filename_queue, self._result_queue,
+                test_types, test_args)
             if self._is_single_threaded():
                 thread.run_in_main_thread(self, result_summary)
             else: