- Revision
- 90679
- Author
- [email protected]
- Date
- 2011-07-09 04:21:30 -0700 (Sat, 09 Jul 2011)
Log 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:
Modified Paths
Diff
Modified: trunk/Tools/ChangeLog (90678 => 90679)
--- trunk/Tools/ChangeLog 2011-07-09 10:55:19 UTC (rev 90678)
+++ trunk/Tools/ChangeLog 2011-07-09 11:21:30 UTC (rev 90679)
@@ -1,3 +1,29 @@
+2011-07-09 Dirk Pranke <[email protected]>
+
+ 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 <[email protected]>
Eliminate bad dependency on gfx::Point.
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (90678 => 90679)
--- trunk/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py 2011-07-09 10:55:19 UTC (rev 90678)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py 2011-07-09 11:21:30 UTC (rev 90679)
@@ -778,7 +778,7 @@
self.cancel_workers()
keyboard_interrupted = True
except TestRunInterruptedException, e:
- _log.info(e.reason)
+ _log.warning(e.reason)
self.cancel_workers()
interrupted = True
except WorkerException:
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py (90678 => 90679)
--- trunk/Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py 2011-07-09 10:55:19 UTC (rev 90678)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py 2011-07-09 11:21:30 UTC (rev 90679)
@@ -235,7 +235,13 @@
def yield_to_broker(self):
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:
class _Process(multiprocessing.Process):
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/controllers/message_broker.py (90678 => 90679)
--- trunk/Tools/Scripts/webkitpy/layout_tests/controllers/message_broker.py 2011-07-09 10:55:19 UTC (rev 90678)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/controllers/message_broker.py 2011-07-09 11:21:30 UTC (rev 90679)
@@ -61,8 +61,11 @@
import cPickle
import logging
import Queue
+import sys
import time
+import traceback
+from webkitpy.common.system import stack_utils
_log = logging.getLogger(__name__)
@@ -194,3 +197,10 @@
def post_message(self, message_name, *message_args):
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)
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/controllers/worker.py (90678 => 90679)
--- trunk/Tools/Scripts/webkitpy/layout_tests/controllers/worker.py 2011-07-09 10:55:19 UTC (rev 90678)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/controllers/worker.py 2011-07-09 11:21:30 UTC (rev 90679)
@@ -32,9 +32,7 @@
import sys
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
from webkitpy.layout_tests.models import test_expectations
@@ -95,23 +93,16 @@
% self._name)
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))
self._worker_connection.post_message('done')
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):
start_time = time.time()
num_tests = 0
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py (90678 => 90679)
--- trunk/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py 2011-07-09 10:55:19 UTC (rev 90678)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py 2011-07-09 11:21:30 UTC (rev 90679)
@@ -236,9 +236,22 @@
self.assertEqual(batch_tests_run, [])
def test_exception_raised(self):
- self.assertRaises(run_webkit_tests.WorkerException, logging_run,
+ # 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,
+ ['--worker-model', 'processes', 'failures/expected/exception.html'], tests_included=True)
+
def test_full_results_html(self):
# FIXME: verify html?
res, out, err, user = logging_run(['--full-results-html'])
@@ -325,8 +338,10 @@
def test_run_force(self):
# 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):
# Test that we actually select the right part
tests_to_run = ['passes/error.html', 'passes/image.html', 'passes/platform_image.html', 'passes/text.html']
