Changeset 121816 in webkit

Timestamp:

Jul 3, 2012 5:14:25 PM (12 years ago)

Author:

dpranke@chromium.org

Message:

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):

Location:

trunk/Tools

Files:

• ChangeLog (modified) (1 diff)
• Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py (modified) (3 diffs)
• Scripts/webkitpy/layout_tests/controllers/worker.py (modified) (1 diff)
• Scripts/webkitpy/layout_tests/port/factory.py (modified) (1 diff)
• Scripts/webkitpy/layout_tests/run_webkit_tests.py (modified) (6 diffs)
• Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py (modified) (1 diff)

trunk/Tools/ChangeLog

@@ +1 @@
+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>
 

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

@@ -256 +256 @@
 
     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)
@@ -306 +307 @@
         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):
@@ -457 +462 @@
         # 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
 
 

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

@@ -135 +135 @@
 
     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):

trunk/Tools/Scripts/webkitpy/layout_tests/port/factory.py

@@ -113 +113 @@
                 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):

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

@@ -38 +38 @@
 
 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
@@ -45 +46 @@
 
 _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
 
 
@@ -120 +129 @@
         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()
@@ -434 +448 @@
 
 
-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
@@ -444 +458 @@
     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)
@@ -452 +473 @@
     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)

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

@@ -905 +905 @@
         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):