Bug 78565 – webkitpy: create ports in Workers, not in manager_worker_broker

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

webkitpy: create ports in Workers, not in manager_worker_broker

[Dirk Pranke]

Created attachment 126888 [details]
Patch

[Dirk Pranke]

Created attachment 126889 [details]
add comment about the change in run_webkit_tests.py

[Dirk Pranke]

Tony, can you take a look?

[Tony Chang]

Comment on attachment 126889 [details]
add comment about the change in run_webkit_tests.py

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

> Tools/ChangeLog:13
> +        bug 78171.
> +
> +        * Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:

Is this covered by existing tests? Please name the tests that cover this in the ChangeLog.

> Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:90
> +            # we are running in a child process and need to create a new Host.

Nit: Capitalize 'we'.

> Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:97
> +            host._initialize_scm()

For my education, do we always have to call _initialize_scm() on host objects?  I noticed that running layout tests always calls this, which runs svn and git to determine the checkout type, even though we never use the information (e.g., if all the tests pass).  It's slow on my Windows machine.

> Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:107
> +            # FIXME: this won't work if the calling process is logging

Nit: Capitalize 'this'.

[Dirk Pranke]

Comment on attachment 126889 [details]
add comment about the change in run_webkit_tests.py

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

>> Tools/ChangeLog:13
>> +        * Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:
> 
> Is this covered by existing tests? Please name the tests that cover this in the ChangeLog.

Yes, this is well-covered by run_webkit_tests_integrationtest.py; will note that.

>> Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:90
>> +            # we are running in a child process and need to create a new Host.
> 
> Nit: Capitalize 'we'.

Will do.

>> Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:97
>> +            host._initialize_scm()
> 
> For my education, do we always have to call _initialize_scm() on host objects?  I noticed that running layout tests always calls this, which runs svn and git to determine the checkout type, even though we never use the information (e.g., if all the tests pass).  It's slow on my Windows machine.

You only need to call initialize_scm() if you need to actually access scm/checkout objects. We need this in the manager process in order to properly generate the json result information (although I thought Ojan had patched the code at some point to only do this during that routine). 

I don't think we need it at all in the child process ... this code was cloned from run_webkit_tests.py, and I will remove it. It is slow.

>> Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:107
>> +            # FIXME: this won't work if the calling process is logging
> 
> Nit: Capitalize 'this'.

Will do.

[Dirk Pranke]

Committed r107718: <http://trac.webkit.org/changeset/107718>

[Tony Chang]

When I run "new-run-webkit-tests --pixel --release svg" on the command line, I get:

No handlers could be found for logger "webkitpy.layout_tests.port.mac"
Starting 1 worker ...Process _Process-1:
Traceback (most recent call last):
  File "/System/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/multiprocessing/process.py", line 231, in _bootstrap
    self.run()
  File "/Users/tc/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py", line 246, in run
    self._client.run(port=None)
  File "/Users/tc/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/worker.py", line 91, in run
    if 'test' in self._options.platform:
TypeError: argument of type 'NoneType' is not iterable

Starting testing ...