Bug 170186 – webkitpy: Add target host concept

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

A target host concept will allow tests to be run on different hosts than the machine driving the test suite.  This target host would be accessed from the port class by the driver, allowing drivers to run layout tests on a remote host.

[Jonathan Bedard]

Created attachment 305605 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/31301797>

[Jonathan Bedard]

Created attachment 305640 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 305640 [details]
Patch

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

I'd like to better understand the difference between "host", "device" and "port". Perhaps the code could be more clear about these.

Looks good overall, please address the comments about the Port class member functions with unused arguments.

> Tools/Scripts/webkitpy/port/base.py:809
> -    def abspath_for_test(self, test_name):
> +    def abspath_for_test(self, test_name, target_host=None):

It would be cleaner to implement the functionality for non-None target host here, not in subclasses. As it is, the signature misleads the reader into believing that passing a target_host is always supported.

> Tools/Scripts/webkitpy/port/base.py:1277
> +    def _driver_tempdir(self, target_host=None):

Ditto.

> Tools/Scripts/webkitpy/port/base.py:1349
> +    def sample_process(self, name, pid, target_host=None):

Ditto.

> Tools/Scripts/webkitpy/port/darwin.py:125
> +    def sample_process(self, name, pid, target_host=None):

Ditto.

> Tools/Scripts/webkitpy/port/ios.py:79
> +    # Devices are the target host

Please add the sentence with a period.

Is it right that multiple "devices" are a singular "host"?

[Jonathan Bedard]

Created attachment 305778 [details]
Patch

[Jonathan Bedard]

Created attachment 305916 [details]
Patch

[Jonathan Bedard]

Created attachment 305981 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 305981 [details]
Patch

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

> Tools/Scripts/webkitpy/common/system/filesystem.py:315
> +        """Returns a path from the base host localized for this host.  By default,

Please don't use French spaces.

> Tools/Scripts/webkitpy/common/system/filesystem.py:320
> +        """Moves a file from this host to the base host.  Be default, this host

Ditto. Typo: Be default.

It may be better to have a copy operation, as move can't be atomic in he general case anyway. As far as I understand, we don't need to copy or move for Simulator much anyway.

> Tools/Scripts/webkitpy/common/system/filesystem.py:325
> +        """Moves a file from the base host to this host.  Be default, this host

Ditto.

> Tools/Scripts/webkitpy/port/darwin.py:157
> +    def sample_file_path(self, name, pid, dir):

dir -> directory

> Tools/Scripts/webkitpy/port/darwin.py:160
> +    def spindump_file_path(self, name, pid, dir):

Ditto.

[Jonathan Bedard]

Created attachment 306010 [details]
Patch

[Jonathan Bedard]

Created attachment 306017 [details]
Patch

[Jonathan Bedard]

Created attachment 306026 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 306026 [details]
Patch for landing

Clearing flags on attachment: 306026

Committed r214705: <http://trac.webkit.org/changeset/214705>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.