nrwt: clean up exception handling and make sure we log some more failures
authordpranke@chromium.org <dpranke@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 4 Jul 2012 00:14:25 +0000 (00:14 +0000)
committerdpranke@chromium.org <dpranke@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 4 Jul 2012 00:14:25 +0000 (00:14 +0000)
https://bugs.webkit.org/show_bug.cgi?id=90503

Reviewed by Ojan Vafai.

There were several places where exceptions weren't getting
logged, most notably if you passed a bad value to --platform.
This change tests that and cleans things up a bit; more cleanup
will be possible when we rework the manager_worker_broker code.

* Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:
(_BrokerConnection.raise_exception):
(_InlineWorkerConnection.raise_exception):
* Scripts/webkitpy/layout_tests/controllers/worker.py:
(Worker.run):
(Worker.kill_driver):
* Scripts/webkitpy/layout_tests/port/factory.py:
(PortFactory.get):
* Scripts/webkitpy/layout_tests/run_webkit_tests.py:
(run):
(main):
* Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:
(MainTest.test_unsupported_platfrom):

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

Tools/ChangeLog
Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py
Tools/Scripts/webkitpy/layout_tests/controllers/worker.py
Tools/Scripts/webkitpy/layout_tests/port/factory.py
Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py
Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py

index 899e769..0e80d30 100644 (file)
@@ -1,5 +1,31 @@
 2012-07-03  Dirk Pranke  <dpranke@chromium.org>
 
+        nrwt: clean up exception handling and make sure we log some more failures
+        https://bugs.webkit.org/show_bug.cgi?id=90503
+
+        Reviewed by Ojan Vafai.
+
+        There were several places where exceptions weren't getting
+        logged, most notably if you passed a bad value to --platform.
+        This change tests that and cleans things up a bit; more cleanup
+        will be possible when we rework the manager_worker_broker code.
+
+        * Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:
+        (_BrokerConnection.raise_exception):
+        (_InlineWorkerConnection.raise_exception):
+        * Scripts/webkitpy/layout_tests/controllers/worker.py:
+        (Worker.run):
+        (Worker.kill_driver):
+        * Scripts/webkitpy/layout_tests/port/factory.py:
+        (PortFactory.get):
+        * Scripts/webkitpy/layout_tests/run_webkit_tests.py:
+        (run):
+        (main):
+        * Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:
+        (MainTest.test_unsupported_platfrom):
+
+2012-07-03  Dirk Pranke  <dpranke@chromium.org>
+
         nrwt: fix mock port
         https://bugs.webkit.org/show_bug.cgi?id=90500
 
index 7e83384..f7baced 100755 (executable)
@@ -255,9 +255,10 @@ class _BrokerConnection(object):
                                   message_name, *message_args)
 
     def raise_exception(self, exc_info):
-        # Since tracebacks aren't picklable, send the extracted stack instead.
+        # Since tracebacks aren't picklable, send the extracted stack instead,
+        # but at least log the full traceback.
         exception_type, exception_value, exception_traceback = sys.exc_info()
-        stack_utils.log_traceback(_log.debug, exception_traceback)
+        stack_utils.log_traceback(_log.error, exception_traceback)
         stack = traceback.extract_tb(exception_traceback)
         self._broker.post_message(self._client, self._post_topic, 'exception', exception_type, exception_value, stack)
 
@@ -305,8 +306,12 @@ class AbstractWorker(BrokerClient):
             self._worker_connection.raise_exception(sys.exc_info())
         finally:
             _log.debug("%s done with message loop%s" % (self._name, exception_msg))
-            self.worker.cleanup()
-            self._worker_connection.post_message('done')
+            try:
+                self.worker.cleanup()
+            finally:
+                # Make sure we post a done so that we can flush the log messages
+                # and clean up properly even if we raise an exception in worker.cleanup().
+                self._worker_connection.post_message('done')
 
     def handle_stop(self, source):
         self._done = True
@@ -456,8 +461,11 @@ class _InlineWorkerConnection(_WorkerConnection):
     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]
+        # the queue. This allows us to preserve the traceback, but we log
+        # it anyway for consistency with the multiprocess case.
+        exception_type, exception_value, exception_traceback = sys.exc_info()
+        stack_utils.log_traceback(_log.error, exception_traceback)
+        raise exception_type, exception_value, exception_traceback
 
 
 class _Process(multiprocessing.Process):
index 1f6d0b5..c689159 100644 (file)
@@ -134,10 +134,13 @@ class Worker(object):
         return thread_timeout_sec
 
     def kill_driver(self):
-        if self._driver:
+        # Be careful about how and when we kill the driver; if driver.stop()
+        # raises an exception, this routine may get re-entered via __del__.
+        driver = self._driver
+        self._driver = None
+        if driver:
             _log.debug("%s killing driver" % self._name)
-            self._driver.stop()
-            self._driver = None
+            driver.stop()
 
     def run_test_with_timeout(self, test_input, timeout):
         if self._options.run_singly:
index 5da5bc5..9b5cf27 100644 (file)
@@ -112,7 +112,7 @@ class PortFactory(object):
             if port_name.startswith(cls.port_name):
                 port_name = cls.determine_full_port_name(self._host, options, port_name)
                 return cls(self._host, port_name, options=options, **kwargs)
-        raise NotImplementedError('unsupported port: %s' % port_name)
+        raise NotImplementedError('unsupported platform: "%s"' % port_name)
 
     def all_port_names(self):
         """Return a list of all valid, fully-specified, "real" port names.
index c1043e8..e3a13c2 100755 (executable)
@@ -37,7 +37,8 @@ import signal
 import sys
 
 from webkitpy.common.host import Host
-from webkitpy.layout_tests.controllers.manager import Manager
+from webkitpy.common.system import stack_utils
+from webkitpy.layout_tests.controllers.manager import Manager, WorkerException, TestRunInterruptedException
 from webkitpy.layout_tests.models import test_expectations
 from webkitpy.layout_tests.port import port_options
 from webkitpy.layout_tests.views import printing
@@ -46,6 +47,14 @@ from webkitpy.layout_tests.views import printing
 _log = logging.getLogger(__name__)
 
 
+# This mirrors what the shell normally does.
+INTERRUPTED_EXIT_STATUS = signal.SIGINT + 128
+
+# This is a randomly chosen exit code that can be tested against to
+# indicate that an unexpected exception occurred.
+EXCEPTIONAL_EXIT_STATUS = 254
+
+
 def lint(port, options):
     host = port.host
     if options.platform:
@@ -119,6 +128,11 @@ def run(port, options, args, regular_output=sys.stderr, buildbot_output=sys.stdo
 
         unexpected_result_count = manager.run()
         _log.debug("Testing completed, Exit status: %d" % unexpected_result_count)
+    except Exception:
+        exception_type, exception_value, exception_traceback = sys.exc_info()
+        if exception_type not in (KeyboardInterrupt, TestRunInterruptedException, WorkerException):
+            stack_utils.log_traceback(_log.error, exception_traceback)
+        raise
     finally:
         printer.cleanup()
 
@@ -433,8 +447,8 @@ def parse_args(args=None):
     return option_parser.parse_args(args)
 
 
-def main():
-    options, args = parse_args()
+def main(argv=None):
+    options, args = parse_args(argv)
     if options.platform and 'test' in options.platform:
         # It's a bit lame to import mocks into real code, but this allows the user
         # to run tests against the test platform interactively, which is useful for
@@ -443,7 +457,14 @@ def main():
         host = MockHost()
     else:
         host = Host()
-    port = host.port_factory.get(options.platform, options)
+
+    try:
+        port = host.port_factory.get(options.platform, options)
+    except NotImplementedError, e:
+        # FIXME: is this the best way to handle unsupported port names?
+        print >> sys.stderr, str(e)
+        return EXCEPTIONAL_EXIT_STATUS
+
     logging.getLogger().setLevel(logging.DEBUG if options.verbose else logging.INFO)
     return run(port, options, args)
 
@@ -451,12 +472,7 @@ def main():
 if '__main__' == __name__:
     try:
         sys.exit(main())
-    except KeyboardInterrupt:
-        # This mirrors what the shell normally does.
-        INTERRUPTED_EXIT_STATUS = signal.SIGINT + 128
-        sys.exit(INTERRUPTED_EXIT_STATUS)
-    except Exception:
-        # This is a randomly chosen exit code that can be tested against to
-        # indicate that an unexpected exception occurred.
-        EXCEPTIONAL_EXIT_STATUS = 254
+    except Exception, e:
+        if e.__class__ in (KeyboardInterrupt, TestRunInterruptedException):
+            sys.exit(INTERRUPTED_EXIT_STATUS)
         sys.exit(EXCEPTIONAL_EXIT_STATUS)
index 04bc5e5..477a98f 100755 (executable)
@@ -904,6 +904,20 @@ class MainTest(unittest.TestCase, StreamTestingMixin):
         self.assertEquals(full_results['has_wdiff'], False)
         self.assertEquals(full_results['has_pretty_patch'], False)
 
+    def test_unsupported_platform(self):
+        oc = outputcapture.OutputCapture()
+        try:
+            oc.capture_output()
+            res = run_webkit_tests.main(['--platform', 'foo'])
+        finally:
+            stdout, stderr, logs = oc.restore_output()
+
+        self.assertEquals(res, run_webkit_tests.EXCEPTIONAL_EXIT_STATUS)
+        self.assertEquals(stdout, '')
+        self.assertTrue('unsupported platform' in stderr)
+
+        # This is empty because we don't even get a chance to configure the logger before failing.
+        self.assertEquals(logs, '')
 
 class EndToEndTest(unittest.TestCase):
     def parse_full_results(self, full_results_text):