[webkit-reviews] review denied: [Bug 54068] nrwt multiprocessing: implement a full set of stubs for testrunner, worker : [Attachment 81905] revise w/ tony's feedback, shuffle constructors of worker_connection, worker around

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 54068: nrwt multiprocessing: implement a full set of stubs for testrunner,
worker
https://bugs.webkit.org/show_bug.cgi?id=54068

Attachment 81905: revise w/ tony's feedback, shuffle constructors of
worker_connection, worker around
https://bugs.webkit.org/attachment.cgi?id=81905&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
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].