nrwt: moving child process logging code into manager_worker_broker
authordpranke@chromium.org <dpranke@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 3 Jul 2012 23:11:18 +0000 (23:11 +0000)
committerdpranke@chromium.org <dpranke@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 3 Jul 2012 23:11:18 +0000 (23:11 +0000)
https://bugs.webkit.org/show_bug.cgi?id=90408

Reviewed by Ojan Vafai.

Users of manager_worker_broker should not have to be aware of
whether they're in the same process or different processes and
configure logging themselves; mwb should hide this complexity.
We can't quite do this completely/correctly yet, since the
manager expects to get a list of messages to log, but this
change fixes the worker side of it, at least.

This is just moving code around, there is no new functionality
and this should be covered by existing tests.

* Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:
(AbstractWorker.__init__):
(_WorkerConnection.__init__):
(_WorkerConnection.post_message):
(_WorkerConnection):
(_WorkerConnection.set_up_logging):
(_WorkerConnection.clean_up_logging):
(_InlineWorkerConnection.run):
(_MultiProcessWorkerConnection.run):
(_WorkerLogHandler):
(_WorkerLogHandler.__init__):
(_WorkerLogHandler.emit):
* Scripts/webkitpy/layout_tests/controllers/manager_worker_broker_unittest.py:
(_TestWorker.run):
(_TestsMixin.handle_done):
* Scripts/webkitpy/layout_tests/controllers/worker.py:
(Worker.__init__):
(Worker.run):
(Worker._run_test):
(Worker.cleanup):
(Worker.run_single_test):

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

Tools/ChangeLog
Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py
Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker_unittest.py
Tools/Scripts/webkitpy/layout_tests/controllers/worker.py

index c760568..17d2eaf 100644 (file)
@@ -1,3 +1,42 @@
+2012-07-03  Dirk Pranke  <dpranke@chromium.org>
+
+        nrwt: moving child process logging code into manager_worker_broker
+        https://bugs.webkit.org/show_bug.cgi?id=90408
+
+        Reviewed by Ojan Vafai.
+
+        Users of manager_worker_broker should not have to be aware of
+        whether they're in the same process or different processes and
+        configure logging themselves; mwb should hide this complexity.
+        We can't quite do this completely/correctly yet, since the
+        manager expects to get a list of messages to log, but this
+        change fixes the worker side of it, at least.
+
+        This is just moving code around, there is no new functionality
+        and this should be covered by existing tests.
+
+        * Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:
+        (AbstractWorker.__init__):
+        (_WorkerConnection.__init__):
+        (_WorkerConnection.post_message):
+        (_WorkerConnection):
+        (_WorkerConnection.set_up_logging):
+        (_WorkerConnection.clean_up_logging):
+        (_InlineWorkerConnection.run):
+        (_MultiProcessWorkerConnection.run):
+        (_WorkerLogHandler):
+        (_WorkerLogHandler.__init__):
+        (_WorkerLogHandler.emit):
+        * Scripts/webkitpy/layout_tests/controllers/manager_worker_broker_unittest.py:
+        (_TestWorker.run):
+        (_TestsMixin.handle_done):
+        * Scripts/webkitpy/layout_tests/controllers/worker.py:
+        (Worker.__init__):
+        (Worker.run):
+        (Worker._run_test):
+        (Worker.cleanup):
+        (Worker.run_single_test):
+
 2012-07-03  Tony Chang  <tony@chromium.org>
 
         [chromium] Don't archive build files generated by VS2010
index 9281f7d..790df93 100755 (executable)
@@ -69,12 +69,14 @@ import cPickle
 import logging
 import multiprocessing
 import optparse
+import os
 import Queue
 import sys
 import traceback
 
 
 from webkitpy.common.system import stack_utils
+from webkitpy.layout_tests.views import metered_stream
 
 
 _log = logging.getLogger(__name__)
@@ -277,6 +279,7 @@ class AbstractWorker(BrokerClient):
         self._name = 'worker/%d' % worker_number
         self._done = False
         self._canceled = False
+        self._options = optparse.Values({'verbose': False})
 
     def name(self):
         return self._name
@@ -361,6 +364,9 @@ class _WorkerConnection(_BrokerConnection):
     def __init__(self, host, broker, worker_factory, worker_number):
         self._client = worker_factory(self, worker_number)
         self._host = host
+        self._log_messages = []
+        self._logger = None
+        self._log_handler = None
         _BrokerConnection.__init__(self, broker, self._client, ANY_WORKER_TOPIC, MANAGER_TOPIC)
 
     def name(self):
@@ -378,6 +384,33 @@ class _WorkerConnection(_BrokerConnection):
     def yield_to_broker(self):
         pass
 
+    def post_message(self, *args):
+        # FIXME: This is a hack until we can remove the log_messages arg from the manager.
+        if args[0] in ('finished_test', 'done'):
+            log_messages = self._log_messages
+            self._log_messages = []
+            args = args + tuple([log_messages])
+        super(_WorkerConnection, self).post_message(*args)
+
+    def set_up_logging(self):
+        self._logger = logging.root
+        # The unix multiprocessing implementation clones the MeteredStream log handler
+        # into the child process, so we need to remove it to avoid duplicate logging.
+        for h in self._logger.handlers:
+            # log handlers don't have names until python 2.7.
+            if getattr(h, 'name', '') == metered_stream.LOG_HANDLER_NAME:
+                self._logger.removeHandler(h)
+                break
+        self._logger.setLevel(logging.DEBUG if self._client._options.verbose else logging.INFO)
+        self._log_handler = _WorkerLogHandler(self)
+        self._logger.addHandler(self._log_handler)
+
+    def clean_up_logging(self):
+        if self._log_handler and self._logger:
+            self._logger.removeHandler(self._log_handler)
+        self._log_handler = None
+        self._logger = None
+
 
 class _InlineWorkerConnection(_WorkerConnection):
     def __init__(self, host, broker, manager_client, worker_factory, worker_number):
@@ -397,7 +430,7 @@ class _InlineWorkerConnection(_WorkerConnection):
     def run(self):
         self._alive = True
         try:
-            self._client.run(self._host, set_up_logging=False)
+            self._client.run(self._host)
         finally:
             self._alive = False
 
@@ -439,5 +472,18 @@ class _MultiProcessWorkerConnection(_WorkerConnection):
         self._proc.start()
 
     def run(self):
-        # FIXME: set_up_logging shouldn't be needed.
-        self._client.run(self._host, set_up_logging=True)
+        self.set_up_logging()
+        try:
+            self._client.run(self._host)
+        finally:
+            self.clean_up_logging()
+
+
+class _WorkerLogHandler(logging.Handler):
+    def __init__(self, worker):
+        logging.Handler.__init__(self)
+        self._worker = worker
+        self._pid = os.getpid()
+
+    def emit(self, record):
+        self._worker._log_messages.append(record)
index 6fda171..f8be07b 100644 (file)
@@ -69,7 +69,7 @@ class _TestWorker(manager_worker_broker.AbstractWorker):
         assert a_str == "hello, world"
         self._worker_connection.post_message('test', 2, 'hi, ' + self._thing_to_greet)
 
-    def run(self, host, set_up_logging):
+    def run(self, host):
         if self._starting_queue:
             self._starting_queue.put('')
 
@@ -102,7 +102,7 @@ class _TestsMixin(object):
     def is_done(self):
         return self._done
 
-    def handle_done(self, src):
+    def handle_done(self, src, log_messages):
         self._done = True
 
     def handle_test(self, src, an_int, a_str):
index a8611a8..22a96aa 100644 (file)
@@ -29,8 +29,6 @@
 """Handle messages from the Manager and executes actual tests."""
 
 import logging
-import os
-import sys
 import threading
 import time
 
@@ -39,7 +37,6 @@ 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
 from webkitpy.layout_tests.models import test_results
-from webkitpy.layout_tests.views import metered_stream
 
 
 _log = logging.getLogger(__name__)
@@ -57,9 +54,6 @@ class Worker(manager_worker_broker.AbstractWorker):
         self._driver = None
         self._tests_run_file = None
         self._tests_run_filename = None
-        self._log_messages = []
-        self._logger = None
-        self._log_handler = None
 
     def __del__(self):
         self.cleanup()
@@ -76,29 +70,10 @@ class Worker(manager_worker_broker.AbstractWorker):
         tests_run_filename = self._filesystem.join(self._results_directory, "tests_run%d.txt" % self._worker_number)
         self._tests_run_file = self._filesystem.open_text_file_for_writing(tests_run_filename)
 
-    def _set_up_logging(self):
-        self._logger = logging.getLogger()
-
-        # The unix multiprocessing implementation clones the MeteredStream log handler
-        # into the child process, so we need to remove it to avoid duplicate logging.
-        for h in self._logger.handlers:
-            # log handlers don't have names until python 2.7.
-            if getattr(h, 'name', '') == metered_stream.LOG_HANDLER_NAME:
-                self._logger.removeHandler(h)
-                break
-
-        self._logger.setLevel(logging.DEBUG if self._options.verbose else logging.INFO)
-        self._log_handler = _WorkerLogHandler(self)
-        self._logger.addHandler(self._log_handler)
-
-    def run(self, host, set_up_logging):
+    def run(self, host):
         if not host:
             host = Host()
 
-        # FIXME: this should move into manager_worker_broker.py.
-        if set_up_logging:
-            self._set_up_logging()
-
         options = self._options
         self._port = host.port_factory.get(options.platform, options)
 
@@ -110,7 +85,7 @@ class Worker(manager_worker_broker.AbstractWorker):
             self.kill_driver()
             _log.debug("%s exiting" % self._name)
             self.cleanup()
-            self._worker_connection.post_message('done', self._log_messages)
+            self._worker_connection.post_message('done')
 
     def handle_test_list(self, src, list_name, test_list):
         start_time = time.time()
@@ -147,9 +122,7 @@ class Worker(manager_worker_broker.AbstractWorker):
         result = self.run_test_with_timeout(test_input, test_timeout_sec)
 
         elapsed_time = time.time() - start
-        log_messages = self._log_messages
-        self._log_messages = []
-        self._worker_connection.post_message('finished_test', result, elapsed_time, log_messages)
+        self._worker_connection.post_message('finished_test', result, elapsed_time)
 
         self.clean_up_after_test(test_input, result)
 
@@ -159,10 +132,6 @@ class Worker(manager_worker_broker.AbstractWorker):
         if self._tests_run_file:
             self._tests_run_file.close()
             self._tests_run_file = None
-        if self._log_handler and self._logger:
-            self._logger.removeHandler(self._log_handler)
-        self._log_handler = None
-        self._logger = None
 
     def timeout(self, test_input):
         """Compute the appropriate timeout value for a test."""
@@ -279,13 +248,3 @@ class Worker(manager_worker_broker.AbstractWorker):
     def run_single_test(self, driver, test_input):
         return single_test_runner.run_single_test(self._port, self._options,
             test_input, driver, self._name)
-
-
-class _WorkerLogHandler(logging.Handler):
-    def __init__(self, worker):
-        logging.Handler.__init__(self)
-        self._worker = worker
-        self._pid = os.getpid()
-
-    def emit(self, record):
-        self._worker._log_messages.append(record)