Bug 78565 - webkitpy: create ports in Workers, not in manager_worker_broker
Summary: webkitpy: create ports in Workers, not in manager_worker_broker
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks: 78171
  Show dependency treegraph
 
Reported: 2012-02-13 19:12 PST by Dirk Pranke
Modified: 2019-05-02 16:23 PDT (History)
6 users (show)

See Also:


Attachments
Patch (7.77 KB, patch)
2012-02-13 19:14 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
add comment about the change in run_webkit_tests.py (7.83 KB, patch)
2012-02-13 19:16 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 2012-02-13 19:12:20 PST
webkitpy: create ports in Workers, not in manager_worker_broker
Comment 1 Dirk Pranke 2012-02-13 19:14:58 PST
Created attachment 126888 [details]
Patch
Comment 2 Dirk Pranke 2012-02-13 19:16:15 PST
Created attachment 126889 [details]
add comment about the change in run_webkit_tests.py
Comment 3 Dirk Pranke 2012-02-13 19:29:11 PST
Tony, can you take a look?
Comment 4 Tony Chang 2012-02-14 11:03:02 PST
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'.
Comment 5 Dirk Pranke 2012-02-14 11:10:01 PST
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.
Comment 6 Dirk Pranke 2012-02-14 11:25:52 PST
Committed r107718: <http://trac.webkit.org/changeset/107718>
Comment 7 Tony Chang 2012-02-14 14:52:24 PST
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 ...