Bug 78171 – webkitpy: start cleaning up manager_worker_broker in prep for reuse by test-webkitpy

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

webkitpy: start cleaning up manager_worker_broker in prep for reuse by test-webkitpy

[Dirk Pranke]

Created attachment 126172 [details]
Patch

[Dirk Pranke]

Created attachment 126185 [details]
move more logic into self._finish_initializing()

[Eric Seidel (no email)]

Comment on attachment 126185 [details]
move more logic into self._finish_initializing()

I'm confused as to what exactly you're doing here.  Perhaps this patch is too big, or perhaps I'm just not up-to-speed on this section of webkitpy.

I'm also not sure we want to run test-webkitpy tests in parallel.  Excepting the integration tests, the execute in 23s.

[Eric Seidel (no email)]

They run in 51s with the integration tests.

[Dirk Pranke]

(In reply to comment #3)
> (From update of attachment 126185 [details])
> I'm confused as to what exactly you're doing here.  Perhaps this patch is too big, or perhaps I'm just not up-to-speed on this section of webkitpy.
> 

I think it's probably the latter. Tony might be the best person to review this. 

Note that this change is worth doing even if we don't want to run stuff in parallel; it is just generally making things cleaner.

Also, as far as running in 53s or not, I don't know about you, but I would rather things ran faster if possible :). Any particular reason you'd be concerned about stuff running in parallel?

[Tony Chang]

Comment on attachment 126185 [details]
move more logic into self._finish_initializing()

View in context: https://bugs.webkit.org/attachment.cgi?id=126185&action=review

> Tools/ChangeLog:19
> +        * Scripts/webkitpy/layout_tests/controllers/manager.py:
> +        (Manager._run_tests):
> +        * Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:
> +        (get):
> +        (AbstractWorker.__init__):
> +        (AbstractWorker.run):

I also skimmed this yesterday and it wasn't obvious to me what the change was doing (lots of code is moving). It would be helpful if you filled in this part and below with the specific changes.

[Dirk Pranke]

(In reply to comment #6)
> I also skimmed this yesterday and it wasn't obvious to me what the change was doing (lots of code is moving). It would be helpful if you filled in this part and below with the specific changes.

Will do. I will take another look; it may be better to split this into multiple patches.

[Dirk Pranke]

Created attachment 126401 [details]
clean up, annotate changelog

[Dirk Pranke]

Okay, turns out that it doesn't make much sense to try and split this into multiple patches, so I have attempted to annotate the heck out of the changelog instead. Please take another look, and let me know if that is enough of a field guide?

[Tony Chang]

Comment on attachment 126401 [details]
clean up, annotate changelog

View in context: https://bugs.webkit.org/attachment.cgi?id=126401&action=review

I mostly skimmed the patch (I don't have a big picture sense of the code moves), but tried to give some ideas on how to split it up.

> Tools/ChangeLog:12
> +        I want to be able to re-use the worker queue logic from
> +        new-run-webkit-tests in test-webkitpy. In order to do this I
> +        need to remove all of the NRWT-specific logic from the worker
> +        queue, and refactor things a bit so the interface is simpler.
> +        This is the first of several patches for that.

Maybe drop most of this text and focus on how this simplifies the interface.

> Tools/ChangeLog:20
> +        With this patch message_broker.py and manager_worker_broker.py
> +        no longer depend on an options argument (it was only using the
> +        worker_model flag, which is now a string argument to get()), no
> +        longer depends on being handed a Port object, no longer constructs
> +        Host objects in the child processes, and now passes a set of
> +        keyword args to start_worker() instead of a hard-coded list of
> +        NRWT-specific args.

Could each of these (or maybe a couple at a time) be a separate patch?

> Tools/ChangeLog:24
> +

Nit: blank line?

> Tools/ChangeLog:27
> +        (get): take worker_model as an arg instead of options, remove
> +        the runtime_options() function which wasn't used anyway.

Removing runtime_options could probably be a standalone patch.

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:764
> +            worker_connection = manager_connection.start_worker(worker_number, port=self._port,
> +                options=self._options, results_directory=self.results_directory(),
> +                real_port_name=self._port.real_name())

Why do we pass port and port.real_name()?

Rather than passing a dict as worker_args, I think it would be better to have a class.  It's hard for me to track what's in worker_args.

> Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:144
> +        self._inline_worker = _InlineWorkerConnection(self._broker,
> +            self._client, self._worker_class, worker_number, port, **worker_args)

It looks like port is in worker_args.  It seems weird that we also pass it as a separate param.

> Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:38
> +# These are needed when workers are launched in new child processes.
> +from webkitpy.common.host import Host
> +from webkitpy.common.host_mock import MockHost

Do we normally comment why we have imports?

> Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:74
> +        if not port:
> +            if 'test' in self._real_port_name:
> +                host = MockHost()
> +            else:
> +                host = Host()
> +            host._initialize_scm()
> +            port = host.port_factory.get(self._real_port_name, self._options)

This feels like testing code in the main class.  Is there a way to keep this in test files (maybe as a separate patch before this one)?

> Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:93
> +        self._driver = None

Why did this move?

> Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:126
>              self.cleanup()
> +            self._printer.cleanup()

Should _printer.cleanup() be moved into cleanup()?

[Dirk Pranke]

(In reply to comment #10)
> (From update of attachment 126401 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126401&action=review
> 
> I mostly skimmed the patch (I don't have a big picture sense of the code moves), but tried to give some ideas on how to split it up.
> 

Good feedback, thanks for the review!

> > Tools/ChangeLog:12
> > +        I want to be able to re-use the worker queue logic from
> > +        new-run-webkit-tests in test-webkitpy. In order to do this I
> > +        need to remove all of the NRWT-specific logic from the worker
> > +        queue, and refactor things a bit so the interface is simpler.
> > +        This is the first of several patches for that.
> 
> Maybe drop most of this text and focus on how this simplifies the interface.
>

Will do.
 
> > Tools/ChangeLog:20
> > +        With this patch message_broker.py and manager_worker_broker.py
> > +        no longer depend on an options argument (it was only using the
> > +        worker_model flag, which is now a string argument to get()), no
> > +        longer depends on being handed a Port object, no longer constructs
> > +        Host objects in the child processes, and now passes a set of
> > +        keyword args to start_worker() instead of a hard-coded list of
> > +        NRWT-specific args.
> 
> Could each of these (or maybe a couple at a time) be a separate patch?
>

Yeah, I can probably get at least two or three patches out of this.
 
> > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:764
> > +            worker_connection = manager_connection.start_worker(worker_number, port=self._port,
> > +                options=self._options, results_directory=self.results_directory(),
> > +                real_port_name=self._port.real_name())
> 
> Why do we pass port and port.real_name()?
> 

I don't think they're both needed now (this is left over from an earlier rev. Will clean up).

> Rather than passing a dict as worker_args, I think it would be better to have a class.  It's hard for me to track what's in worker_args.
>

Okay; it's a little weird in my mind since there's no required base interface for this, but I can do that for now.
 
> > Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:144
> > +        self._inline_worker = _InlineWorkerConnection(self._broker,
> > +            self._client, self._worker_class, worker_number, port, **worker_args)
> 
> It looks like port is in worker_args.  It seems weird that we also pass it as a separate param.
> 

Will dig into this. port's role in this interface is unfortunately awkward :(

> > Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:38
> > +# These are needed when workers are launched in new child processes.
> > +from webkitpy.common.host import Host
> > +from webkitpy.common.host_mock import MockHost
> 
> Do we normally comment why we have imports?
> 

These particular ones have caused confusion in the past, since code shouldn't normally need to create Hosts.

> > Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:74
> > +        if not port:
> > +            if 'test' in self._real_port_name:
> > +                host = MockHost()
> > +            else:
> > +                host = Host()
> > +            host._initialize_scm()
> > +            port = host.port_factory.get(self._real_port_name, self._options)
> 
> This feels like testing code in the main class.  Is there a way to keep this in test files (maybe as a separate patch before this one)?
> 

No, unfortunately. That's part of the MockHost() issue, above

> > Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:93
> > +        self._driver = None
> 
> Why did this move?
> 

To clarify that it wasn't needed in __init__ (along with the other lines I deleted).

> > Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:126
> >              self.cleanup()
> > +            self._printer.cleanup()
> 
> Should _printer.cleanup() be moved into cleanup()?

Probably a good idea.

[Dirk Pranke]

Okay, I've split this patch up into the three dependent bugs ... please take a look (bug 78565, bug 78568, bug 78572).

[Dirk Pranke]

closing ... the sub-bugs have all landed.