Bug 54068

Created attachment 81755 [details] Patch

Created attachment 81756 [details] clean up signatures, comments a bit

Created attachment 81892 [details] add interaction diagram, fix signatures, unit tests for _ThreadedManager, _MultiProcessManager

Comment on attachment 81892 [details] add interaction diagram, fix signatures, unit tests for _ThreadedManager, _MultiProcessManager View in context: https://bugs.webkit.org/attachment.cgi?id=81892&action=review > Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:42 > + | \ / | Nit: I would get rid of this middle line. > Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:89 > + client - object to dispatch replies to. This object must implement > + the methods in message_broker2.BrokerClient. nit: message_broker2.BrokerClient implementation to dispatch replies to > Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:168 > + def start_worker(self, worker_number, **kwargs): > + worker_connection = self._make_worker_connection() > + worker = self._make_worker(worker_connection, worker_number, **kwargs) > + worker_connection.set_client(worker) > + return worker_connection I find this confusing. I think it's because I expect the classes that own objects to create the objects they own. If I understand things correctly, worker_connection owns a worker. Is it possible to keep worker construction code out of ManagerConnection? Maybe rather than passing _worker_class around, we can have a worker connection factory that takes the a string ('inline', 'process', 'threads') and creates the right woker type?

Created attachment 81905 [details] revise w/ tony's feedback, shuffle constructors of worker_connection, worker around

(In reply to comment #4) > (From update of attachment 81892 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=81892&action=review > > > Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:42 > > + | \ / | > > Nit: I would get rid of this middle line. > Done. > > Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:89 > > + client - object to dispatch replies to. This object must implement > > + the methods in message_broker2.BrokerClient. > > nit: message_broker2.BrokerClient implementation to dispatch replies to > Done. > > Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:168 > > + def start_worker(self, worker_number, **kwargs): > > + worker_connection = self._make_worker_connection() > > + worker = self._make_worker(worker_connection, worker_number, **kwargs) > > + worker_connection.set_client(worker) > > + return worker_connection > > I find this confusing. I think it's because I expect the classes that own objects to create the objects they own. I suppose it depends on your definition of own :) The way things are coded now, the ManagerConnection owns both the workers and the worker_connections, and the worker_connection just has a reference to the worker, it doesn't create things. The concept of ownership is a bit loose here since we rely on garbage collection to destroy the objects, and neither workers nor their connections have a specified lifetime. That said, I can understand how it is a bit confusing. I think I can restructure it so that the worker_connection does create and own the worker. If I understand what you are suggesting properly, though, it would be a bit awkward to create a separate worker_connection factory, since the signatures of the constructors can vary (_InlineWorker has one more argument than either _MultiProcessWorker or _ThreadedWorker, although that could be fixed easily enough). And, the workers themselves are always created the same way (as you see in start_worker). I've posted a slightly revised patch and you can see if this makes things better or worse. I'm kinda on the fence, because I don't care for the really long string of arguments that need to be added to the worker_connection constructor, but it does simplify some other things.

Created attachment 81906 [details] incremental diff of the prior two attachments

Comment on attachment 81905 [details] revise w/ tony's feedback, shuffle constructors of worker_connection, worker around View in context: https://bugs.webkit.org/attachment.cgi?id=81905&action=review > Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:160 > + def _make_worker_connection(self, worker_class, worker_number, **kwargs): > + raise NotImplementedError > + > + def start_worker(self, worker_number, **kwargs): > + return self._make_worker_connection(self._worker_class, worker_number, **kwargs) I'm not sure _make_worker_connection buys you much. You could just have start_worker be pure virtual. > Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:172 > + def _make_worker_connection(self, worker_class, worker_number, **kwargs): > + self._inline_worker = _InlineWorker(self._broker, self._port, self._client, > + worker_class, worker_number, self._options, **kwargs) > + return self._inline_worker And this would become start_worker. > Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:195 > + def __init__(self, broker, worker_class, worker_number, options, **kwargs): > + self._client = worker_class(self, worker_number, options, **kwargs) > + self.name = self._client.name() > + message_broker2.BrokerConnection.__init__(self, broker, self._client, Yes, this is definitely improved. Do we have to pass around worker_class or will it always be worker.Worker? > Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:206 > +class _InlineWorker(_WorkerConnection): > + def __init__(self, broker, port, manager_client, worker_class, worker_number, options, **kwargs): Do we have to pass in options? Looks like we can get options from port? > Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner2.py:41 > +from webkitpy.layout_tests.layout_package import worker > +from webkitpy.layout_tests.layout_package import test_runner sort > Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner2.py:67 > + Return: A tuple (keyboard_interrupted, thread_timings, test_timings, > + individual_test_timings) I would prefer if this returned a class. > Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner2.py:80 > + test_lists = self._shard_tests(file_list, False) I would also prefer if this returned a class so below you don't have to do test_list[0], test_list[1].

Created attachment 82029 [details] update w/ more review feedback from tony - make start_worker() virtual

(In reply to comment #8) > (From update of attachment 81905 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=81905&action=review > > > Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:160 > > + def _make_worker_connection(self, worker_class, worker_number, **kwargs): > > + raise NotImplementedError > > + > > + def start_worker(self, worker_number, **kwargs): > > + return self._make_worker_connection(self._worker_class, worker_number, **kwargs) > > I'm not sure _make_worker_connection buys you much. You could just have start_worker be pure virtual. > Yeah, at this point, I'd agree with you. Done. Also, it turns out I don't actually use the kwargs at this point (I did once), so I've removed them. > > Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:172 > > + def _make_worker_connection(self, worker_class, worker_number, **kwargs): > > + self._inline_worker = _InlineWorker(self._broker, self._port, self._client, > > + worker_class, worker_number, self._options, **kwargs) > > + return self._inline_worker > > And this would become start_worker. > Yup. > > Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:195 > > + def __init__(self, broker, worker_class, worker_number, options, **kwargs): > > + self._client = worker_class(self, worker_number, options, **kwargs) > > + self.name = self._client.name() > > + message_broker2.BrokerConnection.__init__(self, broker, self._client, > > Yes, this is definitely improved. > > Do we have to pass around worker_class or will it always be worker.Worker? > The unit tests will use a different class, and I'd strongly prefer not to hard-code it. > > Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:206 > > +class _InlineWorker(_WorkerConnection): > > + def __init__(self, broker, port, manager_client, worker_class, worker_number, options, **kwargs): > > Do we have to pass in options? Looks like we can get options from port? > We can, and I've made the change, but I'm on the fence about it, since options isn't technically a public property of the port. > > Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner2.py:41 > > +from webkitpy.layout_tests.layout_package import worker > > +from webkitpy.layout_tests.layout_package import test_runner > > sort > Done. > > Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner2.py:67 > > + Return: A tuple (keyboard_interrupted, thread_timings, test_timings, > > + individual_test_timings) > > I would prefer if this returned a class. > This code matches the signature used by test_runner.py as well. I'd prefer not to change this in this patch. I'll add a fixme. > > Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner2.py:80 > > + test_lists = self._shard_tests(file_list, False) > > I would also prefer if this returned a class so below you don't have to do test_list[0], test_list[1]. Same thing.

Comment on attachment 82029 [details] update w/ more review feedback from tony - make start_worker() virtual View in context: https://bugs.webkit.org/attachment.cgi?id=82029&action=review I think it's ok to pass around worker_class for testability, but a bit unfortunate. I wonder if it would be possible to have a function on some object that can be overridden with a sub class during testing. If there's only Worker and TestWorker, I'd suggest removing AbstractWorker and just use duck typing for testing. That seems to be what we do for other mocks. > Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:204 > + _WorkerConnection.__init__(self, broker, worker_class, worker_number, port._options) I would just add a public accessor for options() in base.py.

(In reply to comment #11) > (From update of attachment 82029 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=82029&action=review > > I think it's ok to pass around worker_class for testability, but a bit unfortunate. I wonder if it would be possible to have a function on some object that can be overridden with a sub class during testing. > Possibly. However, I really don't want to hard-code the class, because logically this module does not depend on the types of the workers. > If there's only Worker and TestWorker, I'd suggest removing AbstractWorker and just use duck typing for testing. That seems to be what we do for other mocks. > I think having Worker inherit from AbstractWorker() makes the interface contract clearer. I suppose this could be considered unpythonic, but I don't particularly like this aspect of python ;) > > Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:204 > > + _WorkerConnection.__init__(self, broker, worker_class, worker_number, port._options) > > I would just add a public accessor for options() in base.py. Yeah, this is probably what I'll do eventually.

Committed r78302: <http://trac.webkit.org/changeset/78302>