Changeset 90679 in webkit

Timestamp:

Jul 9, 2011 4:21:30 AM (13 years ago)

Author:

dpranke@chromium.org

Message:

nrwt: stack traces from worker-side exceptions aren't very useful inside test-webkitpy
​https://bugs.webkit.org/show_bug.cgi?id=64218

Reviewed by Eric Seidel.

Exceptions aren't picklable and can't be sent across the
manager/worker message queue without losing information. NRWT
handles this by turning the stack trace into a set of strings,
and logging the strings when we receive an exception from the
worker. However, when you are running tests and something
crashes on the worker side, test-webkitpy prints the
manager-side stack trace, which is just confusing and useless.

This patch changes the logic so that exceptions are passed
through as-is when the worker and manager are in the same
process (the --worker-model=inline option). This increases the
code paths slightly but makes crashes much more useful.

• Scripts/webkitpy/layout_tests/controllers/manager.py:
• Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:
• Scripts/webkitpy/layout_tests/controllers/message_broker.py:
• Scripts/webkitpy/layout_tests/controllers/worker.py:
• Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:

Location:

trunk/Tools

Files:

• ChangeLog (modified) (1 diff)
• Scripts/webkitpy/layout_tests/controllers/manager.py (modified) (1 diff)
• Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py (modified) (1 diff)
• Scripts/webkitpy/layout_tests/controllers/message_broker.py (modified) (2 diffs)
• Scripts/webkitpy/layout_tests/controllers/worker.py (modified) (3 diffs)
• Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py (modified) (2 diffs)

trunk/Tools/ChangeLog

@@ +1 @@
+2011-07-09  Dirk Pranke  <dpranke@chromium.org>
+
+        nrwt: stack traces from worker-side exceptions aren't very useful inside test-webkitpy
+        https://bugs.webkit.org/show_bug.cgi?id=64218
+
+        Reviewed by Eric Seidel.
+
+        Exceptions aren't picklable and can't be sent across the
+        manager/worker message queue without losing information. NRWT
+        handles this by turning the stack trace into a set of strings,
+        and logging the strings when we receive an exception from the
+        worker. However, when you are running tests and something
+        crashes on the worker side, test-webkitpy prints the
+        manager-side stack trace, which is just confusing and useless.
+
+        This patch changes the logic so that exceptions are passed
+        through as-is when the worker and manager are in the same
+        process (the --worker-model=inline option). This increases the
+        code paths slightly but makes crashes much more useful.
+
+        * Scripts/webkitpy/layout_tests/controllers/manager.py:
+        * Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:
+        * Scripts/webkitpy/layout_tests/controllers/message_broker.py:
+        * Scripts/webkitpy/layout_tests/controllers/worker.py:
+        * Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:
+
 2011-07-09  Darin Fisher  <darin@chromium.org>
 

trunk/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py

@@ -779 +779 @@
             keyboard_interrupted = True
         except TestRunInterruptedException, e:
-            _log.info(e.reason)
+            _log.warning(e.reason)
             self.cancel_workers()
             interrupted = True

trunk/Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py

@@ -236 +236 @@
         self._broker.run_all_pending(MANAGER_TOPIC, self._manager_client)
 
+    def raise_exception(self, exc_info):
+        # Since the worker is in the same process as the manager, we can
+        # raise the exception directly, rather than having to send it through
+        # the queue. This allows us to preserve the traceback.
+        raise exc_info[0], exc_info[1], exc_info[2]
+
 
 if multiprocessing:

trunk/Tools/Scripts/webkitpy/layout_tests/controllers/message_broker.py

@@ -62 +62 @@
 import logging
 import Queue
+import sys
 import time
-
+import traceback
+
+from webkitpy.common.system import stack_utils
 
 _log = logging.getLogger(__name__)
@@ -195 +198 @@
         self._broker.post_message(self._client, self._post_topic,
                                   message_name, *message_args)
+
+    def raise_exception(self, exc_info):
+        # Since tracebacks aren't picklable, send the extracted stack instead.
+        exception_type, exception_value, exception_traceback = sys.exc_info()
+        stack_utils.log_traceback(_log.debug, exception_traceback)
+        stack = traceback.extract_tb(exception_traceback)
+        self._broker.post_message(self._client, self._post_topic, 'exception', exception_type, exception_value, stack)

trunk/Tools/Scripts/webkitpy/layout_tests/controllers/worker.py

@@ -33 +33 @@
 import threading
 import time
-import traceback
-
-from webkitpy.common.system import stack_utils
+
 from webkitpy.layout_tests.controllers import manager_worker_broker
 from webkitpy.layout_tests.controllers import single_test_runner
@@ -96 +94 @@
         except KeyboardInterrupt:
             exception_msg = ", interrupted"
-            self.post_exception()
+            self._worker_connection.raise_exception(sys.exc_info())
         except:
             exception_msg = ", exception raised"
-            self.post_exception()
+            self._worker_connection.raise_exception(sys.exc_info())
         finally:
             _log.debug("%s done with message loop%s" % (self._name, exception_msg))
@@ -105 +103 @@
             self.cleanup()
             _log.debug("%s exiting" % self._name)
-
-    def post_exception(self):
-        # Since tracebacks aren't picklable, send the extracted stack instead.
-        exception_type, exception_value, exception_traceback = sys.exc_info()
-        stack_utils.log_traceback(_log.debug, exception_traceback)
-        stack = traceback.extract_tb(exception_traceback)
-        self._worker_connection.post_message('exception', exception_type, exception_value, stack)
 
     def handle_test_list(self, src, list_name, test_list):

trunk/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py

@@ -237 +237 @@
 
     def test_exception_raised(self):
+        # Exceptions raised by a worker are treated differently depending on
+        # whether they are in-process or out. inline exceptions work as normal,
+        # which allows us to get the full stack trace and traceback from the
+        # worker. The downside to this is that it could be any error, but this
+        # is actually useful in testing, which is what --worker-model=inline is
+        # usually used for.
+        #
+        # Exceptions raised in a separate process are re-packaged into
+        # WorkerExceptions, which have a string capture of the stack which can
+        # be printed, but don't display properly in the unit test exception handlers.
+        self.assertRaises(ValueError, logging_run,
+            ['failures/expected/exception.html'], tests_included=True)
+
         self.assertRaises(run_webkit_tests.WorkerException, logging_run,
-            ['failures/expected/exception.html'], tests_included=True)
+            ['--worker-model', 'processes', 'failures/expected/exception.html'], tests_included=True)
 
     def test_full_results_html(self):
@@ -326 +339 @@
         # This raises an exception because we run
         # failures/expected/exception.html, which is normally SKIPped.
-        self.assertRaises(run_webkit_tests.WorkerException, logging_run, ['--force'])
+
+        # See also the comments in test_exception_raised() about ValueError vs. WorkerException.
+        self.assertRaises(ValueError, logging_run, ['--force'])
 
     def test_run_part(self):