Bug 54068 - nrwt multiprocessing: implement a full set of stubs for testrunner, worker
Summary: nrwt multiprocessing: implement a full set of stubs for testrunner, worker
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on: 54067
Blocks: 49566 54070
  Show dependency treegraph
 
Reported: 2011-02-08 22:50 PST by Dirk Pranke
Modified: 2011-02-10 19:18 PST (History)
4 users (show)

See Also:


Attachments
Patch (14.59 KB, patch)
2011-02-08 22:56 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
clean up signatures, comments a bit (16.84 KB, patch)
2011-02-08 23:18 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
add interaction diagram, fix signatures, unit tests for _ThreadedManager, _MultiProcessManager (18.11 KB, patch)
2011-02-09 16:22 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
revise w/ tony's feedback, shuffle constructors of worker_connection, worker around (17.87 KB, patch)
2011-02-09 18:01 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
incremental diff of the prior two attachments (4.99 KB, patch)
2011-02-09 18:04 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
update w/ more review feedback from tony - make start_worker() virtual (18.22 KB, patch)
2011-02-10 12:45 PST, Dirk Pranke
tony: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2011-02-08 22:50:23 PST
nrwt multiprocessing: implement a full set of stubs for testrunner, worker
Comment 1 Dirk Pranke 2011-02-08 22:56:51 PST
Created attachment 81755 [details]
Patch
Comment 2 Dirk Pranke 2011-02-08 23:18:44 PST
Created attachment 81756 [details]
clean up signatures, comments a bit
Comment 3 Dirk Pranke 2011-02-09 16:22:48 PST
Created attachment 81892 [details]
add interaction diagram, fix signatures, unit tests for _ThreadedManager, _MultiProcessManager
Comment 4 Tony Chang 2011-02-09 16:45:13 PST
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?
Comment 5 Dirk Pranke 2011-02-09 18:01:39 PST
Created attachment 81905 [details]
revise w/ tony's feedback, shuffle constructors of worker_connection, worker around
Comment 6 Dirk Pranke 2011-02-09 18:03:03 PST
(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.
Comment 7 Dirk Pranke 2011-02-09 18:04:33 PST
Created attachment 81906 [details]
incremental diff of the prior two attachments
Comment 8 Tony Chang 2011-02-10 11:53:17 PST
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].
Comment 9 Dirk Pranke 2011-02-10 12:45:25 PST
Created attachment 82029 [details]
update w/ more review feedback from tony - make start_worker() virtual
Comment 10 Dirk Pranke 2011-02-10 12:46:27 PST
(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 11 Tony Chang 2011-02-10 13:34:09 PST
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.
Comment 12 Dirk Pranke 2011-02-10 16:35:13 PST
(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.
Comment 13 Dirk Pranke 2011-02-10 19:18:26 PST
Committed r78302: <http://trac.webkit.org/changeset/78302>