From 50e6753fd2eb1a074d5b9a5be7fd8f61d23f6274 Mon Sep 17 00:00:00 2001 From: "dpranke@chromium.org" Date: Fri, 29 Jun 2012 02:37:04 +0000 Subject: [PATCH] nrwt: clean up how arguments are passed to workers https://bugs.webkit.org/show_bug.cgi?id=90126 Reviewed by Ojan Vafai. The way arguments are passed to workers has been crufty. It turns out it can be a lot cleaner via two things: 1) using a factory method instead of instantiating objects directly in manager_worker_broker removes the need for passing 'worker arguments' to the broker. 2) it turns out that since mock hosts and test ports are purely in-memory constructions, they can be pickled and passed to child workers, meaning that the worker no longer needs hacky code to pass the port in a special case or to guess what to do if we don't have a host - all of the test-specific logic can move to the test file, where we can stub out the mock host's port_factory to return the same already-created port when it needs to be shared. This change also moves WorkerException to manager_worker_broker.py where it belongs, and removes several useless tests that were just a maintenance burden (and would've needed rewriting when we change the rest of the broker implementation). * Scripts/webkitpy/layout_tests/controllers/manager.py: (Manager._run_tests.worker_factory): (Manager._run_tests): * Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py: (get): (WorkerException): (AbstractWorker.__init__): (_ManagerConnection.__init__): (_ManagerConnection.start_worker): (_InlineManager.__init__): (_InlineManager.start_worker): (_MultiProcessManager._can_pickle_host): (_MultiProcessManager): (_MultiProcessManager.start_worker): (_WorkerConnection.__init__): (_InlineWorkerConnection.__init__): (_InlineWorkerConnection.join): (_InlineWorkerConnection.run): (_Process.run): (_MultiProcessWorkerConnection.__init__): (_MultiProcessWorkerConnection.start): (_MultiProcessWorkerConnection): (_MultiProcessWorkerConnection.run): * Scripts/webkitpy/layout_tests/controllers/manager_worker_broker_unittest.py: (_TestWorker.__init__): (_TestWorker.run): (_TestsMixin.test_name): (_TestsMixin.test_cancel): (_TestsMixin.test_done): (_TestsMixin.test_unknown_message): (InlineBrokerTests.setUp): (MultiProcessBrokerTests.setUp): * Scripts/webkitpy/layout_tests/controllers/worker.py: (Worker.__init__): (Worker.run): * Scripts/webkitpy/layout_tests/controllers/worker_unittest.py: Removed. * Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py: (passing_run): (logging_run): (run_and_capture): (MainTest.test_child_processes_2): (MainTest.test_child_processes_min): (MainTest.test_exception_raised): (MainTest.test_keyboard_interrupt): (MainTest.test_retrying_and_flaky_tests): (MainTest.test_run_order__inline): git-svn-id: http://svn.webkit.org/repository/webkit/trunk@121509 268f45cc-cd09-0410-ab3c-d52691b4dbfc --- Tools/ChangeLog | 73 +++++++++++++++++ .../webkitpy/layout_tests/controllers/manager.py | 22 ++--- .../controllers/manager_worker_broker.py | 94 +++++++++++----------- .../controllers/manager_worker_broker_unittest.py | 59 ++------------ .../webkitpy/layout_tests/controllers/worker.py | 39 +++------ .../layout_tests/controllers/worker_unittest.py | 54 ------------- .../run_webkit_tests_integrationtest.py | 43 +++++----- 7 files changed, 167 insertions(+), 217 deletions(-) delete mode 100644 Tools/Scripts/webkitpy/layout_tests/controllers/worker_unittest.py diff --git a/Tools/ChangeLog b/Tools/ChangeLog index 7859d01..aa9553b 100644 --- a/Tools/ChangeLog +++ b/Tools/ChangeLog @@ -1,5 +1,78 @@ 2012-06-28 Dirk Pranke + nrwt: clean up how arguments are passed to workers + https://bugs.webkit.org/show_bug.cgi?id=90126 + + Reviewed by Ojan Vafai. + + The way arguments are passed to workers has been crufty. It + turns out it can be a lot cleaner via two things: + 1) using a factory method instead of instantiating objects + directly in manager_worker_broker removes the need for passing + 'worker arguments' to the broker. + 2) it turns out that since mock hosts and test ports are purely + in-memory constructions, they can be pickled and passed to child + workers, meaning that the worker no longer needs hacky code to + pass the port in a special case or to guess what to do if we + don't have a host - all of the test-specific logic can move to + the test file, where we can stub out the mock host's + port_factory to return the same already-created port when it + needs to be shared. + + This change also moves WorkerException to manager_worker_broker.py + where it belongs, and removes several useless tests that were + just a maintenance burden (and would've needed rewriting when we + change the rest of the broker implementation). + + * Scripts/webkitpy/layout_tests/controllers/manager.py: + (Manager._run_tests.worker_factory): + (Manager._run_tests): + * Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py: + (get): + (WorkerException): + (AbstractWorker.__init__): + (_ManagerConnection.__init__): + (_ManagerConnection.start_worker): + (_InlineManager.__init__): + (_InlineManager.start_worker): + (_MultiProcessManager._can_pickle_host): + (_MultiProcessManager): + (_MultiProcessManager.start_worker): + (_WorkerConnection.__init__): + (_InlineWorkerConnection.__init__): + (_InlineWorkerConnection.join): + (_InlineWorkerConnection.run): + (_Process.run): + (_MultiProcessWorkerConnection.__init__): + (_MultiProcessWorkerConnection.start): + (_MultiProcessWorkerConnection): + (_MultiProcessWorkerConnection.run): + * Scripts/webkitpy/layout_tests/controllers/manager_worker_broker_unittest.py: + (_TestWorker.__init__): + (_TestWorker.run): + (_TestsMixin.test_name): + (_TestsMixin.test_cancel): + (_TestsMixin.test_done): + (_TestsMixin.test_unknown_message): + (InlineBrokerTests.setUp): + (MultiProcessBrokerTests.setUp): + * Scripts/webkitpy/layout_tests/controllers/worker.py: + (Worker.__init__): + (Worker.run): + * Scripts/webkitpy/layout_tests/controllers/worker_unittest.py: Removed. + * Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py: + (passing_run): + (logging_run): + (run_and_capture): + (MainTest.test_child_processes_2): + (MainTest.test_child_processes_min): + (MainTest.test_exception_raised): + (MainTest.test_keyboard_interrupt): + (MainTest.test_retrying_and_flaky_tests): + (MainTest.test_run_order__inline): + +2012-06-28 Dirk Pranke + nrwt: don't try to catch worker exceptions in run_webkit_tests.__main__ https://bugs.webkit.org/show_bug.cgi?id=90125 diff --git a/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py b/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py index f526135..f2fed3f 100644 --- a/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py +++ b/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py @@ -266,9 +266,7 @@ class TestRunInterruptedException(Exception): return self.__class__, (self.reason,) -class WorkerException(Exception): - """Raised when we receive an unexpected/unknown exception from a worker.""" - pass +WorkerException = manager_worker_broker.WorkerException class TestShard(object): @@ -771,25 +769,17 @@ class Manager(object): num_workers = min(num_workers, len(all_shards)) self._log_num_workers(num_workers, len(all_shards), len(locked_shards)) - manager_connection = manager_worker_broker.get(num_workers, self, worker.Worker) + def worker_factory(worker_connection, worker_number): + return worker.Worker(worker_connection, worker_number, self.results_directory(), self._options) + + manager_connection = manager_worker_broker.get(num_workers, self, worker_factory, self._port.host) if self._options.dry_run: return (keyboard_interrupted, interrupted, thread_timings, self._group_stats, self._all_results) self._printer.print_update('Starting %s ...' % grammar.pluralize('worker', num_workers)) for worker_number in xrange(num_workers): - worker_arguments = worker.WorkerArguments(worker_number, self.results_directory(), self._options) - worker_connection = manager_connection.start_worker(worker_arguments) - if num_workers == 1: - # FIXME: We need to be able to share a port with the work so - # that some of the tests can query state on the port; ideally - # we'd rewrite the tests so that this wasn't necessary. - # - # Note that this only works because in the inline case - # the worker hasn't really started yet and won't start - # running until we call run_message_loop(), below. - worker_connection.set_inline_arguments(self._port) - + worker_connection = manager_connection.start_worker(worker_number) worker_state = _WorkerState(worker_number, worker_connection) self._worker_states[worker_connection.name()] = worker_state diff --git a/Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py b/Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py index 7047d93..9281f7d 100755 --- a/Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py +++ b/Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py @@ -87,15 +87,15 @@ MANAGER_TOPIC = 'managers' ANY_WORKER_TOPIC = 'workers' -def get(max_workers, client, worker_class): +def get(max_workers, client, worker_factory, host=None): """Return a connection to a manager/worker message_broker Args: max_workers - max # of workers to run concurrently. client - BrokerClient implementation to dispatch replies to. - worker_class - type of workers to create. This class should override - the methods in AbstractWorker. + worker_factory: factory method for creatin objects that implement the AbstractWorker interface. + host: optional picklable host object that can be passed to workers for testing. Returns: A handle to an object that will talk to a message broker configured for the normal manager/worker communication.""" @@ -107,7 +107,12 @@ def get(max_workers, client, worker_class): manager_class = _MultiProcessManager broker = _Broker(queue_class) - return manager_class(broker, client, worker_class) + return manager_class(broker, client, worker_factory, host) + + +class WorkerException(Exception): + """Raised when we receive an unexpected/unknown exception from a worker.""" + pass class BrokerClient(object): @@ -255,7 +260,7 @@ class _BrokerConnection(object): class AbstractWorker(BrokerClient): - def __init__(self, worker_connection, worker_arguments=None): + def __init__(self, worker_connection, worker_number): """The constructor should be used to do any simple initialization necessary, but should not do anything that creates data structures that cannot be Pickled or sent across processes (like opening @@ -265,10 +270,11 @@ class AbstractWorker(BrokerClient): Args: worker_connection - handle to the _BrokerConnection object creating the worker and that can be used for messaging. - worker_arguments - (optional, Picklable) object passed to the worker from the manager""" + worker_number - the number/index of the current worker.""" BrokerClient.__init__(self) self._worker_connection = worker_connection - self._name = 'worker' + self._worker_number = worker_number + self._name = 'worker/%d' % worker_number self._done = False self._canceled = False @@ -308,42 +314,25 @@ class AbstractWorker(BrokerClient): class _ManagerConnection(_BrokerConnection): - def __init__(self, broker, client, worker_class): - """Base initialization for all Manager objects. - - Args: - broker: handle to the message_broker object - client: callback object (the caller) - worker_class: class object to use to create workers. - """ + def __init__(self, broker, client, worker_factory, host): _BrokerConnection.__init__(self, broker, client, MANAGER_TOPIC, ANY_WORKER_TOPIC) - self._worker_class = worker_class - - def start_worker(self, worker_arguments=None): - """Starts a new worker. + self._worker_factory = worker_factory + self._host = host - Args: - worker_arguments - an optional Picklable object that is passed to the worker constructor - """ + def start_worker(self, worker_number): raise NotImplementedError class _InlineManager(_ManagerConnection): - def __init__(self, broker, client, worker_class): - _ManagerConnection.__init__(self, broker, client, worker_class) + def __init__(self, broker, client, worker_factory, host): + _ManagerConnection.__init__(self, broker, client, worker_factory, host) self._inline_worker = None - def start_worker(self, worker_arguments=None): - self._inline_worker = _InlineWorkerConnection(self._broker, - self._client, self._worker_class, worker_arguments) + def start_worker(self, worker_number): + host = self._host + self._inline_worker = _InlineWorkerConnection(host, self._broker, self._client, self._worker_factory, worker_number) return self._inline_worker - def set_inline_arguments(self, arguments=None): - # Note that this method only exists here, and not on all - # ManagerConnections; calling this method on a MultiProcessManager - # will deliberately result in a runtime error. - self._inline_worker.set_inline_arguments(arguments) - def run_message_loop(self, delay_secs=None): # Note that delay_secs is ignored in this case since we can't easily # implement it. @@ -352,16 +341,26 @@ class _InlineManager(_ManagerConnection): class _MultiProcessManager(_ManagerConnection): - def start_worker(self, worker_arguments=None): - worker_connection = _MultiProcessWorkerConnection(self._broker, - self._worker_class, worker_arguments) + def _can_pickle_host(self): + try: + cPickle.dumps(self._host) + return True + except TypeError: + return False + + def start_worker(self, worker_number): + host = None + if self._can_pickle_host(): + host = self._host + worker_connection = _MultiProcessWorkerConnection(host, self._broker, self._worker_factory, worker_number) worker_connection.start() return worker_connection class _WorkerConnection(_BrokerConnection): - def __init__(self, broker, worker_class, worker_arguments=None): - self._client = worker_class(self, worker_arguments) + def __init__(self, host, broker, worker_factory, worker_number): + self._client = worker_factory(self, worker_number) + self._host = host _BrokerConnection.__init__(self, broker, self._client, ANY_WORKER_TOPIC, MANAGER_TOPIC) def name(self): @@ -381,8 +380,8 @@ class _WorkerConnection(_BrokerConnection): class _InlineWorkerConnection(_WorkerConnection): - def __init__(self, broker, manager_client, worker_class, worker_arguments): - _WorkerConnection.__init__(self, broker, worker_class, worker_arguments) + def __init__(self, host, broker, manager_client, worker_factory, worker_number): + _WorkerConnection.__init__(self, host, broker, worker_factory, worker_number) self._alive = False self._manager_client = manager_client @@ -395,13 +394,10 @@ class _InlineWorkerConnection(_WorkerConnection): def join(self, timeout): assert not self._alive - def set_inline_arguments(self, arguments): - self._client.set_inline_arguments(arguments) - def run(self): self._alive = True try: - self._client.run() + self._client.run(self._host, set_up_logging=False) finally: self._alive = False @@ -422,12 +418,12 @@ class _Process(multiprocessing.Process): self._client = client def run(self): - self._client.run() + self._worker_connection.run() class _MultiProcessWorkerConnection(_WorkerConnection): - def __init__(self, broker, worker_class, worker_arguments): - _WorkerConnection.__init__(self, broker, worker_class, worker_arguments) + def __init__(self, host, broker, worker_factory, worker_number): + _WorkerConnection.__init__(self, host, broker, worker_factory, worker_number) self._proc = _Process(self, self._client) def cancel(self): @@ -441,3 +437,7 @@ class _MultiProcessWorkerConnection(_WorkerConnection): def start(self): self._proc.start() + + def run(self): + # FIXME: set_up_logging shouldn't be needed. + self._client.run(self._host, set_up_logging=True) diff --git a/Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker_unittest.py b/Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker_unittest.py index 0464256..6fda171 100644 --- a/Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker_unittest.py +++ b/Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker_unittest.py @@ -44,8 +44,7 @@ starting_queue = None stopping_queue = None -WORKER_NAME = 'TestWorker' - +WORKER_NAME = 'worker/1' def make_broker(manager, max_workers, start_queue=None, stop_queue=None): global starting_queue @@ -56,16 +55,12 @@ def make_broker(manager, max_workers, start_queue=None, stop_queue=None): class _TestWorker(manager_worker_broker.AbstractWorker): - def __init__(self, worker_connection, worker_arguments=None): - super(_TestWorker, self).__init__(worker_connection) - self._name = WORKER_NAME + def __init__(self, worker_connection, worker_number=1): + super(_TestWorker, self).__init__(worker_connection, worker_number) self._thing_to_greet = 'everybody' self._starting_queue = starting_queue self._stopping_queue = stopping_queue - def set_inline_arguments(self, thing_to_greet): - self._thing_to_greet = thing_to_greet - def handle_stop(self, src): self.stop_handling_messages() @@ -74,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): + def run(self, host, set_up_logging): if self._starting_queue: self._starting_queue.put('') @@ -131,7 +126,7 @@ class _TestsMixin(object): def test_name(self): self.make_broker() - worker = self._broker.start_worker() + worker = self._broker.start_worker(1) self.assertEquals(worker.name(), WORKER_NAME) worker.cancel() worker.join(0.1) @@ -140,7 +135,7 @@ class _TestsMixin(object): def test_cancel(self): self.make_broker() - worker = self._broker.start_worker() + worker = self._broker.start_worker(1) self._broker.post_message('test', 1, 'hello, world') worker.cancel() worker.join(0.1) @@ -149,7 +144,7 @@ class _TestsMixin(object): def test_done(self): self.make_broker() - worker = self._broker.start_worker() + worker = self._broker.start_worker(1) self._broker.post_message('test', 1, 'hello, world') self._broker.post_message('stop') self._broker.run_message_loop() @@ -162,7 +157,7 @@ class _TestsMixin(object): def test_unknown_message(self): self.make_broker() - worker = self._broker.start_worker() + worker = self._broker.start_worker(1) self._broker.post_message('unknown') try: self._broker.run_message_loop() @@ -181,15 +176,6 @@ class InlineBrokerTests(_TestsMixin, unittest.TestCase): _TestsMixin.setUp(self) self._max_workers = 1 - def test_inline_arguments(self): - self.make_broker() - worker = self._broker.start_worker() - worker.set_inline_arguments('me') - self._broker.post_message('test', 1, 'hello, world') - self._broker.post_message('stop') - self._broker.run_message_loop() - self.assertEquals(self._a_str, 'hi, me') - # FIXME: https://bugs.webkit.org/show_bug.cgi?id=54520. if sys.platform not in ('cygwin', 'win32'): @@ -200,35 +186,6 @@ if sys.platform not in ('cygwin', 'win32'): self._max_workers = 2 -class InterfaceTest(unittest.TestCase): - # These tests mostly exist to pacify coverage. - - # FIXME: There must be a better way to do this and also verify - # that classes do implement every abstract method in an interface. - def test_brokerclient_is_abstract(self): - # Test that all the base class methods are abstract and have the - # signature we expect. - obj = manager_worker_broker.BrokerClient() - self.assertRaises(NotImplementedError, obj.is_done) - self.assertRaises(NotImplementedError, obj.name) - - def test_managerconnection_is_abstract(self): - # Test that all the base class methods are abstract and have the - # signature we expect. - broker = make_broker(self, 1) - obj = manager_worker_broker._ManagerConnection(broker._broker, self, None) - self.assertRaises(NotImplementedError, obj.start_worker) - - def test_workerconnection_is_abstract(self): - # Test that all the base class methods are abstract and have the - # signature we expect. - broker = make_broker(self, 1) - obj = manager_worker_broker._WorkerConnection(broker._broker, _TestWorker, None) - self.assertRaises(NotImplementedError, obj.cancel) - self.assertRaises(NotImplementedError, obj.is_alive) - self.assertRaises(NotImplementedError, obj.join, None) - - class MessageTest(unittest.TestCase): def test__no_body(self): msg = manager_worker_broker._Message('src', 'topic_name', 'message_name', None) diff --git a/Tools/Scripts/webkitpy/layout_tests/controllers/worker.py b/Tools/Scripts/webkitpy/layout_tests/controllers/worker.py index 5e522cb..a8611a8 100644 --- a/Tools/Scripts/webkitpy/layout_tests/controllers/worker.py +++ b/Tools/Scripts/webkitpy/layout_tests/controllers/worker.py @@ -45,20 +45,11 @@ from webkitpy.layout_tests.views import metered_stream _log = logging.getLogger(__name__) -class WorkerArguments(object): - def __init__(self, worker_number, results_directory, options): - self.worker_number = worker_number - self.results_directory = results_directory - self.options = options - - class Worker(manager_worker_broker.AbstractWorker): - def __init__(self, worker_connection, worker_arguments): - super(Worker, self).__init__(worker_connection, worker_arguments) - self._worker_number = worker_arguments.worker_number - self._name = 'worker/%d' % self._worker_number - self._results_directory = worker_arguments.results_directory - self._options = worker_arguments.options + def __init__(self, worker_connection, worker_number, results_directory, options): + super(Worker, self).__init__(worker_connection, worker_number) + self._results_directory = results_directory + self._options = options self._port = None self._batch_size = None self._batch_count = None @@ -100,24 +91,16 @@ class Worker(manager_worker_broker.AbstractWorker): self._log_handler = _WorkerLogHandler(self) self._logger.addHandler(self._log_handler) - def _set_up_host_and_port(self): - options = self._options - if options.platform and 'test' in options.platform: - # It is lame to import mocks into real code, but this allows us to use the test port in multi-process tests as well. - from webkitpy.common.host_mock import MockHost - host = MockHost() - else: + def run(self, host, set_up_logging): + if not host: host = Host() - self._port = host.port_factory.get(options.platform, options) - def set_inline_arguments(self, port): - self._port = port - - def run(self): - if not self._port: - # We are running in a child process and need to initialize things. + # FIXME: this should move into manager_worker_broker.py. + if set_up_logging: self._set_up_logging() - self._set_up_host_and_port() + + options = self._options + self._port = host.port_factory.get(options.platform, options) self.safe_init() try: diff --git a/Tools/Scripts/webkitpy/layout_tests/controllers/worker_unittest.py b/Tools/Scripts/webkitpy/layout_tests/controllers/worker_unittest.py deleted file mode 100644 index ebb4e7a..0000000 --- a/Tools/Scripts/webkitpy/layout_tests/controllers/worker_unittest.py +++ /dev/null @@ -1,54 +0,0 @@ -# Copyright (c) 2012 Google Inc. All rights reserved. -# -# Redistribution and use in source and binary forms, with or without -# modification, are permitted provided that the following conditions are -# met: -# -# * Redistributions of source code must retain the above copyright -# notice, this list of conditions and the following disclaimer. -# * Redistributions in binary form must reproduce the above -# copyright notice, this list of conditions and the following disclaimer -# in the documentation and/or other materials provided with the -# distribution. -# * Neither the name of Google Inc. nor the names of its -# contributors may be used to endorse or promote products derived from -# this software without specific prior written permission. -# -# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS -# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT -# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR -# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT -# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, -# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT -# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, -# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY -# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT -# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE -# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. - -import unittest - -from webkitpy.layout_tests.controllers.worker import Worker, WorkerArguments -from webkitpy.tool.mocktool import MockOptions - - -class FakeConnection(object): - def run_message_loop(self): - pass - - def post_message(self, message_name, *message_args): - pass - - -class WorkerTest(unittest.TestCase): - def test_default_platform_in_worker(self): - # This checks that we got a port and didn't raise an exception - # if we didn't specify a port with the --platform flag. - worker_connection = FakeConnection() - worker = Worker(worker_connection, WorkerArguments(1, '/tmp', MockOptions(platform=None, print_options=None, verbose=False, batch_size=0))) - worker._set_up_host_and_port() - self.assertNotEquals(worker._port, None) - - -if __name__ == '__main__': - unittest.main() diff --git a/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py b/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py index e06468d..28e1a14 100755 --- a/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py +++ b/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py @@ -50,7 +50,7 @@ from webkitpy.common.host_mock import MockHost from webkitpy.layout_tests import port from webkitpy.layout_tests import run_webkit_tests -from webkitpy.layout_tests.controllers.manager import WorkerException +from webkitpy.layout_tests.controllers.manager_worker_broker import WorkerException from webkitpy.layout_tests.port import Port from webkitpy.layout_tests.port.test import TestPort, TestDriver from webkitpy.test.skip import skip_if @@ -82,18 +82,22 @@ def parse_args(extra_args=None, record_results=False, tests_included=False, new_ return run_webkit_tests.parse_args(args) -def passing_run(extra_args=None, port_obj=None, record_results=False, tests_included=False, host=None): +def passing_run(extra_args=None, port_obj=None, record_results=False, tests_included=False, host=None, shared_port=True): options, parsed_args = parse_args(extra_args, record_results, tests_included) if not port_obj: host = host or MockHost() port_obj = host.port_factory.get(port_name=options.platform, options=options) + + if shared_port: + port_obj.host.port_factory.get = lambda *args, **kwargs: port_obj + buildbot_output = StringIO.StringIO() regular_output = StringIO.StringIO() res = run_webkit_tests.run(port_obj, options, parsed_args, buildbot_output=buildbot_output, regular_output=regular_output) return res == 0 and not regular_output.getvalue() and not buildbot_output.getvalue() -def logging_run(extra_args=None, port_obj=None, record_results=False, tests_included=False, host=None, new_results=False): +def logging_run(extra_args=None, port_obj=None, record_results=False, tests_included=False, host=None, new_results=False, shared_port=True): options, parsed_args = parse_args(extra_args=extra_args, record_results=record_results, tests_included=tests_included, @@ -102,11 +106,13 @@ def logging_run(extra_args=None, port_obj=None, record_results=False, tests_incl if not port_obj: port_obj = host.port_factory.get(port_name=options.platform, options=options) - res, buildbot_output, regular_output = run_and_capture(port_obj, options, parsed_args) + res, buildbot_output, regular_output = run_and_capture(port_obj, options, parsed_args, shared_port) return (res, buildbot_output, regular_output, host.user) -def run_and_capture(port_obj, options, parsed_args): +def run_and_capture(port_obj, options, parsed_args, shared_port=True): + if shared_port: + port_obj.host.port_factory.get = lambda *args, **kwargs: port_obj oc = outputcapture.OutputCapture() try: oc.capture_output() @@ -303,14 +309,14 @@ class MainTest(unittest.TestCase, StreamTestingMixin): def test_child_processes_2(self): if self.should_test_processes: _, _, regular_output, _ = logging_run( - ['--print', 'config', '--child-processes', '2']) + ['--print', 'config', '--child-processes', '2'], shared_port=False) self.assertTrue(any(['Running 2 ' in line for line in regular_output.buflist])) def test_child_processes_min(self): if self.should_test_processes: _, _, regular_output, _ = logging_run( ['--print', 'config', '--child-processes', '2', 'passes'], - tests_included=True) + tests_included=True, shared_port=False) self.assertTrue(any(['Running 1 ' in line for line in regular_output.buflist])) def test_dryrun(self): @@ -335,7 +341,7 @@ class MainTest(unittest.TestCase, StreamTestingMixin): if self.should_test_processes: self.assertRaises(WorkerException, logging_run, - ['--child-processes', '2', '--force', 'failures/expected/exception.html', 'passes/text.html'], tests_included=True) + ['--child-processes', '2', '--force', 'failures/expected/exception.html', 'passes/text.html'], tests_included=True, shared_port=False) def test_full_results_html(self): # FIXME: verify html? @@ -365,7 +371,7 @@ class MainTest(unittest.TestCase, StreamTestingMixin): if self.should_test_processes: self.assertRaises(KeyboardInterrupt, logging_run, - ['failures/expected/keyboard.html', 'passes/text.html', '--child-processes', '2', '--force'], tests_included=True) + ['failures/expected/keyboard.html', 'passes/text.html', '--child-processes', '2', '--force'], tests_included=True, shared_port=False) def test_no_tests_found(self): res, out, err, user = logging_run(['resources'], tests_included=True) @@ -755,6 +761,7 @@ class MainTest(unittest.TestCase, StreamTestingMixin): # Now we test that --clobber-old-results does remove the old entries and the old retries, # and that we don't retry again. + host = MockHost() res, out, err, _ = logging_run(['--no-retry-failures', '--clobber-old-results', 'failures/flaky'], tests_included=True, host=host) self.assertEquals(res, 1) self.assertTrue('Clobbering old results' in err.getvalue()) @@ -764,22 +771,16 @@ class MainTest(unittest.TestCase, StreamTestingMixin): self.assertTrue(host.filesystem.exists('/tmp/layout-test-results/failures/flaky/text-actual.txt')) self.assertFalse(host.filesystem.exists('retries')) - - # These next tests test that we run the tests in ascending alphabetical - # order per directory. HTTP tests are sharded separately from other tests, - # so we have to test both. - def assert_run_order(self, child_processes='1'): - tests_run = get_tests_run(['--child-processes', child_processes, 'passes'], - tests_included=True, flatten_batches=True) + def test_run_order__inline(self): + # These next tests test that we run the tests in ascending alphabetical + # order per directory. HTTP tests are sharded separately from other tests, + # so we have to test both. + tests_run = get_tests_run(['passes'], tests_included=True, flatten_batches=True) self.assertEquals(tests_run, sorted(tests_run)) - tests_run = get_tests_run(['--child-processes', child_processes, 'http/tests/passes'], - tests_included=True, flatten_batches=True) + tests_run = get_tests_run(['http/tests/passes'], tests_included=True, flatten_batches=True) self.assertEquals(tests_run, sorted(tests_run)) - def test_run_order__inline(self): - self.assert_run_order() - def test_tolerance(self): class ImageDiffTestPort(TestPort): def diff_image(self, expected_contents, actual_contents, tolerance=None): -- 2.7.4