Bug 99588 – Use forwarder2 in Chrome for Android layout tests.

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Tools / Tests

[felipe]

Use the new forwarder2

[felipe]

Created attachment 170105 [details]
Patch

[WebKit Review Bot]

Comment on attachment 170105 [details]
Patch

Attachment 170105 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14500500

[Peter Beverloo]

Comment on attachment 170105 [details]
Patch

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

Hi Felipe, thank you for the patch! Here are some comments.

Dirk, Adam, I proposed moving the AndroidCommands and Forwarder classes to separate files, as these will be shared for running w_u_t and TWA as well, and this file is becoming way too bloated. But I'm not sure where it should be.

> Tools/ChangeLog:1
> [phone]  Felipe Goldstein  <[e-mail]>

nit: Your Bugzilla account says @chromium.org.

> Tools/ChangeLog:3
> +        Use the new forwarder2

I'm afraid that the rest of the WebKit community won't know what "forwarder2" is: it's a highly Android-specific term. In general, for platform specific code such as this, we'd use a prefix such as [Chromium] to tell other vendors that this doesn't concern their ports. Maybe a title such as "[Chromium] Use the new forwarder2 for running layout tests on Android" would work better?

> Tools/ChangeLog:5
> +

The "Reviewed by NOBODY (OOPS!)." line needs to be here.
Furthermore, commit messages in WebKit tend to be quite verbose. Could you add one or two paragraphs explaining *why* we are switching to forwarder2?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:33
> +import pexpect

I don't think we can rely on the pexpect library to be available on all ports, as the cr-linux failure indicates. Chromium has the dependency in third_party/pexpect for that reason, and loads it through this file: src/build/android/pylib/pexpect.py. Since it's MIT licensed and WebKit only accepts LGPL and BST, maybe it's an option to pull it in through Tools/Scripts/webkitpy/thirdparty/__init__.py?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:166
> +        'mac',

This seems unintentional, Chromium no longer falls back to mac baselines.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:345
> +class AndroidCommands():

nit: inherit from object? Other classes seem to do that.

I think it'd be worthwhile to split AndroidCommands (and forwarder?) to their own files, removing all code related to them from this file. We'll also need to use them in order to be able to run webkit_unit_tests and TestWebKitAPI. I'm not entirely sure about the ideal directory, maybe Tools/Scripts/webkitpy/common/system/? One worry I have here is that the forwarder is only available for Chromium, which kind of would be a layering violation there...

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:354
> +    def push_md5sum(self):

Should we maybe have a setup_device() method which also executes _setup_performance (and, maybe, a teardown_device() method for _teardown_performance)? Removing all device-logic from the ChromiumAndroidDriver seems preferable. We may be able to address this in later patches..

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:398
> +        return self._port

The Forwarder class only needs the path to the forwarders, exposing the whole Port seems a bit excessive.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:414
> +    _TIMEOUT_SECS = 30

The mixture between global constants and class-local ones is a bit strange. Is there a reason we have both?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:419
> +        Works like adb forward, but in reverse.

I think it'd be fine to remove this line, as 417 already covers what it does.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:422
> +            adb: Instance of AndroidCommands for talking to the device.

nit: the argument is named android_cmd.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:423
> +            port_pairs: A list of tuples (device_port, host_port) to forward. Note

nit: worker_number is missing. Maybe we should remove the argument explanation altogether?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:448
> +        forward_string = ['%d:%d:%s' %

nit: this would fit on a single line.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:450
> +        logging.info('Forwarding ports: %s', forward_string)

Should this be str(forward_string)?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:454
> +        # TODO(felipeg): Rather than using a blocking kill() here, the device

nit: WebKit uses unattributed FIXMEs.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:518
> +                logging.info("Forwarding device port: %d to host port: %d." %

nit: this could fit on one line (no line length limits).

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:525
> +                raise Exception('Failed to forward port %d to %d' % (device_port,

nit: dito.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:530
> +                raise Exception('Unexpected EOF while trying to forward ports %s' %

nit: dito.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:542
> +        self._android_cmd.run_host_command(['pkill', '-f', host_pattern])

pkill and pgrep are not available on Mac systems.. Ideally we'd be able to run layout tests on any platform, not just Linux. Not a huge issue now, though.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:545
> +        while not self._android_cmd.run_host_command(['pgrep', '-f', host_pattern]) and (

Not splitting the line would make this more readable.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:550
> +                raise Exception('Timed out while killing ' + host_pattern)

nit: indenting.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:560
> +        while self._android_cmd.run_adb_command(['shell', 'ps']).find(

Not splitting the line would make this more readable.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:565
> +                raise Exception('Timed out while killing device_forwarder.')

nit: indenting.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:579
> +        """Get the device port that corresponds to a given host port."""

We don't need this comment, the name already describes it :-).

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:583
> +        """Terminate the forwarder process."""

We don't need this comment, the name already describes it :-).

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:602
> +        self._android = AndroidCommands(port, worker_number)

The AndroidCommands instance is named "_android_cmd" in the Forwarder class, but "_android" here. Being more verbose is a good thing, so I'd prefer using "_android_cmd" (or maybe even "_android_commands") for both.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:780
> +        self._forwarder = Forwarder(self._android, self._worker_number, port_pairs, '127.0.0.1')

Should there be a try/catch here in case the Forwarder.__init__ throws?

[felipe]

Comment on attachment 170105 [details]
Patch

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

>> Tools/ChangeLog:1
>> [phone]  Felipe Goldstein  <[e-mail]>
> 
> nit: Your Bugzilla account says @chromium.org.

done

>> Tools/ChangeLog:3
>> +        Use the new forwarder2
> 
> I'm afraid that the rest of the WebKit community won't know what "forwarder2" is: it's a highly Android-specific term. In general, for platform specific code such as this, we'd use a prefix such as [Chromium] to tell other vendors that this doesn't concern their ports. Maybe a title such as "[Chromium] Use the new forwarder2 for running layout tests on Android" would work better?

done

>> Tools/ChangeLog:5
>> +
> 
> The "Reviewed by NOBODY (OOPS!)." line needs to be here.
> Furthermore, commit messages in WebKit tend to be quite verbose. Could you add one or two paragraphs explaining *why* we are switching to forwarder2?

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:33
>> +import pexpect
> 
> I don't think we can rely on the pexpect library to be available on all ports, as the cr-linux failure indicates. Chromium has the dependency in third_party/pexpect for that reason, and loads it through this file: src/build/android/pylib/pexpect.py. Since it's MIT licensed and WebKit only accepts LGPL and BST, maybe it's an option to pull it in through Tools/Scripts/webkitpy/thirdparty/__init__.py?

dpne

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:166
>> +        'mac',
> 
> This seems unintentional, Chromium no longer falls back to mac baselines.

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:345
>> +class AndroidCommands():
> 
> nit: inherit from object? Other classes seem to do that.
> 
> I think it'd be worthwhile to split AndroidCommands (and forwarder?) to their own files, removing all code related to them from this file. We'll also need to use them in order to be able to run webkit_unit_tests and TestWebKitAPI. I'm not entirely sure about the ideal directory, maybe Tools/Scripts/webkitpy/common/system/? One worry I have here is that the forwarder is only available for Chromium, which kind of would be a layering violation there...

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:354
>> +    def push_md5sum(self):
> 
> Should we maybe have a setup_device() method which also executes _setup_performance (and, maybe, a teardown_device() method for _teardown_performance)? Removing all device-logic from the ChromiumAndroidDriver seems preferable. We may be able to address this in later patches..

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:398
>> +        return self._port
> 
> The Forwarder class only needs the path to the forwarders, exposing the whole Port seems a bit excessive.

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:414
>> +    _TIMEOUT_SECS = 30
> 
> The mixture between global constants and class-local ones is a bit strange. Is there a reason we have both?

I dont like the global constants. It has no reason be global, but I didn't want to change that, and since this is the pattern I found in the file, I will keep it and move my local constants to the global namespace.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:419
>> +        Works like adb forward, but in reverse.
> 
> I think it'd be fine to remove this line, as 417 already covers what it does.

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:422
>> +            adb: Instance of AndroidCommands for talking to the device.
> 
> nit: the argument is named android_cmd.

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:423
>> +            port_pairs: A list of tuples (device_port, host_port) to forward. Note
> 
> nit: worker_number is missing. Maybe we should remove the argument explanation altogether?

The argument explanation came from Chromium code style requirement. Should I remove ?

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:448
>> +        forward_string = ['%d:%d:%s' %
> 
> nit: this would fit on a single line.

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:450
>> +        logging.info('Forwarding ports: %s', forward_string)
> 
> Should this be str(forward_string)?

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:454
>> +        # TODO(felipeg): Rather than using a blocking kill() here, the device
> 
> nit: WebKit uses unattributed FIXMEs.

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:518
>> +                logging.info("Forwarding device port: %d to host port: %d." %
> 
> nit: this could fit on one line (no line length limits).

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:525
>> +                raise Exception('Failed to forward port %d to %d' % (device_port,
> 
> nit: dito.

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:530
>> +                raise Exception('Unexpected EOF while trying to forward ports %s' %
> 
> nit: dito.

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:542
>> +        self._android_cmd.run_host_command(['pkill', '-f', host_pattern])
> 
> pkill and pgrep are not available on Mac systems.. Ideally we'd be able to run layout tests on any platform, not just Linux. Not a huge issue now, though.

But I thought this was meant to be used with android.
The file is called chromium_android.py and everything is related to running the tests on android devices, which should happen on linux.
If we try to run it from a Mac or Windows we have a much bigger problem. For example, the forwarder binary only works on linux.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:545
>> +        while not self._android_cmd.run_host_command(['pgrep', '-f', host_pattern]) and (
> 
> Not splitting the line would make this more readable.

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:550
>> +                raise Exception('Timed out while killing ' + host_pattern)
> 
> nit: indenting.

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:560
>> +        while self._android_cmd.run_adb_command(['shell', 'ps']).find(
> 
> Not splitting the line would make this more readable.

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:565
>> +                raise Exception('Timed out while killing device_forwarder.')
> 
> nit: indenting.

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:579
>> +        """Get the device port that corresponds to a given host port."""
> 
> We don't need this comment, the name already describes it :-).

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:583
>> +        """Terminate the forwarder process."""
> 
> We don't need this comment, the name already describes it :-).

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:602
>> +        self._android = AndroidCommands(port, worker_number)
> 
> The AndroidCommands instance is named "_android_cmd" in the Forwarder class, but "_android" here. Being more verbose is a good thing, so I'd prefer using "_android_cmd" (or maybe even "_android_commands") for both.

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:780
>> +        self._forwarder = Forwarder(self._android, self._worker_number, port_pairs, '127.0.0.1')
> 
> Should there be a try/catch here in case the Forwarder.__init__ throws?

The exceptions from the __init__ in the Forwarder are quite clear and it is preferred to fail and show the exception from there instead of catching it here.

[felipe]

Created attachment 170423 [details]
Patch

[WebKit Review Bot]

Attachment 170423 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1
Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:423:  indentation is not a multiple of four  [pep8/E111] [5]
Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:426:  indentation is not a multiple of four  [pep8/E111] [5]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tony Chang]

Comment on attachment 170423 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:42
> +import webkitpy.thirdparty.autoinstalled.pexepct as pexpect

Why not 'from webkitpy.thirdparty.autoinstalled import pexpect' like the other import statements?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:78
> +FORWARD_PORTS = [8000, 8080, 8443, 8880, 9323]

Use a tuple instead of a list since this is a constant.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:348
> +class AndroidCommands(Object):

Object? I think you mean object.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:353
> +        self._original_governors = {}

Maybe _original_governors_file_contents?  It wasn't obvious to me what this dictionary is for.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:368
> +        # Disable CPU scaling and drop ram cache to reduce noise in tests

Nit: Please end the sentence with a period.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:416
> +        result = self._port._executive.run_command(cmd, error_handler=error_handler, return_exit_code=return_exit_code)
> +        return result

Nit: Maybe return directly and remove the temp variable?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:450
> +            port_pairs: A list of tuples (device_port, host_port) to forward. Note
> +                        that you can specify 0 as a device_port, in which case a
> +                        port will by dynamically assigned on the device. You can
> +                        get the number of the assigned port using the
> +                        device_port_for_host_port method.

Rather than passing in a list of tuples, can you make a small class for device port and host port and pass in a list of the class? E.g.,

class Ports(object):
    def __init__(device_port, host_port):
        self.device_port = device_port
        self.host_port = host_port

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:458
> +        self._host_to_device_port_map = dict()

Nit: Why dict() rather than {}?  You use {} above.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:473
> +        timeout_sec = 5

This looks like a constant. Maybe name it TIMEOUT_SECONDS or KILL_TIMEOUT_SECONDS?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:505
> +            # Failure

I would remove this comment.  It seems obvious from the exception being raised.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:522
> +        self._host_process = pexpect.spawn(self._host_forwarder_path,
> +                                           ['--adb_port=%s' % (
> +                                               self._host_adb_control_port)] +
> +                                           forward_string)

The wrapping makes this hard for me to read. Maybe put it on a single line (WebKit doesn't have an 80 col limit) or just do a 4 space indent rather than trying to align with the open paren.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:536
> +                # Success

I would remove this comment. It seems obvious from the lack of exception being raised.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:542
> +                # Failure

I would remove this comment.  It seems obvious from the exception being raised.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:563
> +        elapsed = 0
> +        wait_period = 0.1

Nit: elapsed_seconds and wait_period_seconds.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:577
> +        elapsed = 0
> +        wait_period = 0.1

Nit: elapsed_seconds and wait_period_seconds.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:599
> +    def close(self):
> +        self._close_process()

Do we need both close and _close_process?  I would just have a single close_process.

> Tools/Scripts/webkitpy/thirdparty/__init__.py:96
> +    def _install_pexpect(self):
> +        return self._install("http://pexpect.sourceforge.net/pexpect-2.3.tar.gz",
> +                             "pexpect-2.3")

It looks like pexpect is MIT licensed: http://www.noah.org/wiki/Pexpect#License
That means we can just check in the files rather than using autoinstall.  It'll be much less flaky to check the files in.

[Tony Chang]

Maybe land pexpect as a separate change first.

[Peter Beverloo]

Comment on attachment 170423 [details]
Patch

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

>> Tools/Scripts/webkitpy/thirdparty/__init__.py:96
>> +                             "pexpect-2.3")
> 
> It looks like pexpect is MIT licensed: http://www.noah.org/wiki/Pexpect#License
> That means we can just check in the files rather than using autoinstall.  It'll be much less flaky to check the files in.

I thought that we could only check code licensed with BSD or LGPL 2.1 in. Is this documented somewhere? It would indeed be easier if we can check the pexpect library in.

[felipe]

Created attachment 170600 [details]
Patch

[felipe]

Comment on attachment 170423 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:42
>> +import webkitpy.thirdparty.autoinstalled.pexepct as pexpect
> 
> Why not 'from webkitpy.thirdparty.autoinstalled import pexpect' like the other import statements?

done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:78
>> +FORWARD_PORTS = [8000, 8080, 8443, 8880, 9323]
> 
> Use a tuple instead of a list since this is a constant.

Done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:348
>> +class AndroidCommands(Object):
> 
> Object? I think you mean object.

Done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:353
>> +        self._original_governors = {}
> 
> Maybe _original_governors_file_contents?  It wasn't obvious to me what this dictionary is for.

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:368
>> +        # Disable CPU scaling and drop ram cache to reduce noise in tests
> 
> Nit: Please end the sentence with a period.

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:416
>> +        return result
> 
> Nit: Maybe return directly and remove the temp variable?

dpne

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:423
>> +      return self._port._path_to_device_forwarder()
> 
> indentation is not a multiple of four  [pep8/E111] [5]

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:426
>> +      return self._port._path_to_host_forwarder()
> 
> indentation is not a multiple of four  [pep8/E111] [5]

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:450
>> +                        device_port_for_host_port method.
> 
> Rather than passing in a list of tuples, can you make a small class for device port and host port and pass in a list of the class? E.g.,
> 
> class Ports(object):
>     def __init__(device_port, host_port):
>         self.device_port = device_port
>         self.host_port = host_port

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:458
>> +        self._host_to_device_port_map = dict()
> 
> Nit: Why dict() rather than {}?  You use {} above.

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:473
>> +        timeout_sec = 5
> 
> This looks like a constant. Maybe name it TIMEOUT_SECONDS or KILL_TIMEOUT_SECONDS?

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:505
>> +            # Failure
> 
> I would remove this comment.  It seems obvious from the exception being raised.

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:522
>> +                                           forward_string)
> 
> The wrapping makes this hard for me to read. Maybe put it on a single line (WebKit doesn't have an 80 col limit) or just do a 4 space indent rather than trying to align with the open paren.

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:536
>> +                # Success
> 
> I would remove this comment. It seems obvious from the lack of exception being raised.

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:542
>> +                # Failure
> 
> I would remove this comment.  It seems obvious from the exception being raised.

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:563
>> +        wait_period = 0.1
> 
> Nit: elapsed_seconds and wait_period_seconds.

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:577
>> +        wait_period = 0.1
> 
> Nit: elapsed_seconds and wait_period_seconds.

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:599
>> +        self._close_process()
> 
> Do we need both close and _close_process?  I would just have a single close_process.

done

>>> Tools/Scripts/webkitpy/thirdparty/__init__.py:96
>>> +                             "pexpect-2.3")
>> 
>> It looks like pexpect is MIT licensed: http://www.noah.org/wiki/Pexpect#License
>> That means we can just check in the files rather than using autoinstall.  It'll be much less flaky to check the files in.
> 
> I thought that we could only check code licensed with BSD or LGPL 2.1 in. Is this documented somewhere? It would indeed be easier if we can check the pexpect library in.

Ok,
How should I do that ?
Should I make a separate CL ?

[WebKit Review Bot]

Comment on attachment 170600 [details]
Patch

Attachment 170600 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14559587

[Peter Beverloo]

Comment on attachment 170600 [details]
Patch

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

Thanks Felipe! A few more comments.

> Tools/ChangeLog:8
> +        Itâs currently not possible to run the Clank test suite on

nit: It's.

"Clank"? I'm fairly sure you mean WebKit's layout tests..

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:42
> +from webkitpy.thirdparty.autoinstalled import pexepct

s/pexepct/pexpect. Please be sure to test the change locally as well.

> Tools/Scripts/webkitpy/thirdparty/__init__.py:77
> +        if '.pexpect' in fullname:

Assuming we can indeed land MIT code, it should be fine to add the pexpect library in Tools/Scripts/webkitpy/thirdparty/ directly. This should be done in a separate change, however.

[Tony Chang]

https://lists.webkit.org/pipermail/webkit-dev/2010-January/011406.html

Please make a new bug and attach the patch for pexpect to it. The bug could block this bug.

Also, did you run test-webkitpy?  You should also be adding some unittests for your new classes.

[felipe]

Created attachment 171471 [details]
Patch

[Peter Beverloo]

Comment on attachment 171471 [details]
Patch

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

Thanks for the updated patch, Felipe! Be sure to reply to Tony's remarks in comment 14 as well.

> Tools/ChangeLog:6
> +        "Reviewed by NOBODY (OOPS!)."

nit: no need for the quotes here :).

> Tools/ChangeLog:8
> +        Itâs currently not possible to run the Clank test suite on

Clank?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:88
> +FORWARDER_TIMEOUT_SECS = 30

nit: Should we place this line after line 84 for additional clarity?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:451
> +    _TIMEOUT = 3

nit: Other constants don't have the underscore prefix, I'd prefer if we could stay consistent here.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:455
> +        self._port = android_port

self._port is not used elsewhere in this class.

[felipe]

(In reply to comment #14)
> https://lists.webkit.org/pipermail/webkit-dev/2010-January/011406.html
> 
> Please make a new bug and attach the patch for pexpect to it. The bug could block this bug.
> 
> Also, did you run test-webkitpy?  You should also be adding some unittests for your new classes.

Hey tony I gave up with the pexpect.
I did run the test-webkitpy, all passes.
I did not added unittests to my classes because those classes will be radically changed soon.
Pliard@ is working on a patch on Chromium that will change the way we use the Forwarder2 and the python scripts will be way much simpler than they are now.
Once he makes it on Chromium, he will also need to change it here.
My change is just a step so we can move on while he works on his change in chromium.
Please let me know if you think I should add the unittests now or if we can wait for pliard@ changes

[felipe]

Comment on attachment 171471 [details]
Patch

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

>> Tools/ChangeLog:6
>> +        "Reviewed by NOBODY (OOPS!)."
> 
> nit: no need for the quotes here :).

done

>> Tools/ChangeLog:8
>> +        Itâs currently not possible to run the Clank test suite on
> 
> Clank?

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:88
>> +FORWARDER_TIMEOUT_SECS = 30
> 
> nit: Should we place this line after line 84 for additional clarity?

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:451
>> +    _TIMEOUT = 3
> 
> nit: Other constants don't have the underscore prefix, I'd prefer if we could stay consistent here.

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:455
>> +        self._port = android_port
> 
> self._port is not used elsewhere in this class.

done

[felipe]

Created attachment 171871 [details]
Patch

[Peter Beverloo]

Tony, could you take another look please?

[felipe]

Ping ?

[Tony Chang]

Comment on attachment 171871 [details]
Patch

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

Sorry, just got back from vacation.  Feel free to ask someone else for a review if I don't respond in a day.

Overall this seems fine, but I suspect the code will break if you don't add unit tests.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:228
>          result = super(ChromiumAndroidPort, self).check_build(needs_http)
>          result = self._check_file_exists(self._path_to_md5sum(), 'md5sum utility') and result
> -        result = self._check_file_exists(self._path_to_forwarder(), 'forwarder utility') and result
> +        result = self._check_file_exists(self._path_to_device_forwarder(), 'device_forwarder utility') and result
> +        result = self._check_file_exists(self._path_to_host_forwarder(), 'host_forwarder utility') and result

Nit: I would probably merge these lines.
result = (super(...)
    and self._check_file_exists(...)
    and self._check_file_exists(...)
    ...)

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:487
> +            failure_m = failure_re.match(line)

Nit: failure_m -> failure_match

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:491
> +            success_m = success_re.match(line)

Nit: success_m -> success_match

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:501
> +    def close(self):
> +        self._process.stop(0.0)

This is OK, but it would be cleaner to use __enter__ and __exit__ to start/stop the process and use the 'with' statement in the calling code.  Then you wouldn't need to manually remember to call close() everywhere.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:629
> +        output = self._android_cmd.run_host_command(['lsof', '-nPi:%d' % host_port], ignore_error=True, return_exit_code=False)

I guess this is OK, but it requires that lsof is installed.  I assume you verified this was the case on our bots?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:639
> +        self._android_cmd.run_host_command(['pkill', '-f', host_pattern])

Same comment about pkill and pgrep (procps package) as for lsof.  Maybe these are installed with the other android specific dependencies?

[Philippe Liard]

Quick update on this guys: Felipe's patch didn't land since we significantly changed on the Chromium side the way forwarder2's setup/tear down happens.

I'm going to upload a new patch (attached to this bug if this is ok) compatible with the latest version of forwarder2.

[Peter Beverloo]

(In reply to comment #23)
> Quick update on this guys: Felipe's patch didn't land since we significantly changed on the Chromium side the way forwarder2's setup/tear down happens.
> 
> I'm going to upload a new patch (attached to this bug if this is ok) compatible with the latest version of forwarder2.

If the approach changed significantly from Felipe's approach, it may be easier to open a new bug considering the rest of the discussion on this thread won't be relevant anymore. I don't have a strong preference, though.

[Philippe Liard]

I think this should be pretty close to Felipe's patch from a design perspective (e.g. separate AndroidCommands class).
I think we could keep this bug but let me know if you feel strongly after you looked at my patch.

[Philippe Liard]

Created attachment 177474 [details]
Patch

[Peter Beverloo]

Comment on attachment 177474 [details]
Patch

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

Thanks for the patch, Philippe! I left comments inline.

> Tools/ChangeLog:3
> +        Use forwarder2 in Chrome for Android layout tests.

Please also update the bug's title to reflect this change.

> Tools/ChangeLog:5
> +

Please add a description to the ChangeLog. Why is this change necessary? What kind of impact on functionality does it have, etcetera.

> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:114
> +            self._port.pre_sharding_init()

Can we stop the host forwarder as part of Port.setup_test_run()? The consequence would be that it won't be run separately for the failed tests' second run.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:377
> +        self._adb_command = ['adb', '-s', device_serial]

Did you rebase before uploading this change? We no longer require 'adb' to be in the path, and a utility method was added to get the path to use (ChromiumAndroidDriver.path_to_adb()).

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:384
> +                raise AssertionError('Could not push md5sum to device')

nit: to THE device.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:389
> +    def run_command(self, cmd):

This is only used in the AndroidCommands class, you should prefix it with _ to indicate visibility.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:391
> +        process = subprocess.Popen(cmd, stderr=subprocess.STDOUT, stdout=subprocess.PIPE)

You should use the host's executive here.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:396
> +        _log.debug('Run adb result: ' + str(output[:80]))

The worry I have here is that the current logging output includes the device's serial for the output. We'd loose that coverage here.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:414
> +                self.run_command(['%s_host' % self._md5sum_path, host_file]))

Would it be worth to assert for making sure that _md5sum_path has been setup (i.e. setup_md5sum() must have been invoked)?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:427
> +    DEVICE_FORWARDER_PATH = '/data/local/tmp/device_forwarder'

Could we just append "device_forwarder" to DEVICE_SOURCE_ROOT_DIR?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:432
> +        self._device_forwarder_path = path_builder('device_forwarder')

Since there also is Forwarder.DEVICE_FORWARDER_PATH, I'd prefer to clarify here that it's the host path.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:433
> +        self._host_forwarder_path = path_builder('host_forwarder')

For consistency's sake, maybe invoke Forwarder.path_to_{device,host}_forwarder()?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:439
> +        _log.debug('Killing host daemon ')

nit: ends with a space.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:447
> +        _log.debug('Killing device daemon ')

nit: ends with a space.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:451
> +                '%s shell %s kill-server' % (Forwarder._get_adb_command_string(android_cmd), Forwarder.DEVICE_FORWARDER_PATH))

It wouldn't be hard to convert either of the calls to _run_shell_command_and_get_exit_code() to use a list of arguments instead of a string, while removing the need for Forwarder._get_adb_command_string().

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:453
> +            raise AssertionError('Could not kill device_forwarder:\n%s' % output)

nit: You print output on a new line here, but not in the other error messages (lines 474, 479, 485).

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:456
> +        assert(FORWARDER_CONTROL_PORT_BEGIN + worker_number < Forwarder.ADB_CONTROL_PORT)

nit: no need for the parenthesis.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:458
> +        adb_control_port = self._worker_number_to_adb_control_port_map.get(worker_number)

adb_control_port is something entirely different than Forwarder.ADB_CONTROL_PORT. Could you please use a clearer name?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:465
> +        self._android_cmd.push_file_if_needed(self._device_forwarder_path, Forwarder.DEVICE_FORWARDER_PATH)

Again: distinguishing between the names for host and device paths would make this significantly clearer.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:466
> +        # Command format: <ADB port>:<Device port>:<Host port>:<Host address>

nit: could you add a blank line before this command and lowercase the parts (/s/Device/device etc)? That'd make it a lot easier to read.
"ADB port" should probably read "ADB control port".

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:471
> +                        Forwarder._get_adb_command_string(self._android_cmd), adb_control_port, Forwarder.ADB_CONTROL_PORT)

You seem to indent continuations with either four, eight or an arbitrary amount of spaces. There is no line length limit in WebKit, if you do decide to wrap a line (for example, to improve readability), please indent with four spaces.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:489
> +        while port < FORWARDER_CONTROL_PORT_END and self._is_port_used(port):

Theoretically, with a thousand control ports, we'd open the /proc/net/tcp an equal amount of times. Since _is_port_used() is only used for this purpose, wouldn't it be easier to read /proc/net/tcp once and then find the first available port between [FORWARDER_CONTROL_PORT_BEGIN + worker_number, FORWARDER_CONTROL_PORT_END]?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:497
> +        for line in open('/proc/net/tcp').xreadlines():

Instead of just calling open(), could we go through webkitpy.common.system.filesystem?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:506
> +                _log.debug('  Port %d is used.' % host_port)

nit: Why are these outputs indented?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:516
> +    def _run_shell_command_and_get_exit_code(cmd):

Since Executive.run_command() knows both the output and exit code, it would be a lot easier to add an return_exit_code_and_output argument. That'd be three lines of code, as opposed to bypassing the entire executive.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:749
> +        forwarder = Forwarder(self._port._build_path, self._android_cmd)

Is forwarder intentionally not being stored?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:750
> +        forwarder.setup_redirections(self._worker_number, [(port, port) for port in FORWARD_PORTS], '127.0.0.1')

If host_address is always 127.0.0.1 we shouldn't pass it as an argument.

[Peter Beverloo]

Comment on attachment 171871 [details]
Patch

Marking Felipe's patch as obsolete.

[Philippe Liard]

Created attachment 177729 [details]
Patch

[Philippe Liard]

Hi Peter, thanks for the quick review and the relevant comments! Let me double check that my patch is aligned with my replies to your comments before I publish them.

[Philippe Liard]

Comment on attachment 177474 [details]
Patch

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

>> Tools/ChangeLog:3
>> +        Use forwarder2 in Chrome for Android layout tests.
> 
> Please also update the bug's title to reflect this change.

I will see if Felipe can do it. I don't think I can given that he created the bug.

>> Tools/ChangeLog:5
>> +
> 
> Please add a description to the ChangeLog. Why is this change necessary? What kind of impact on functionality does it have, etcetera.

Done.

>> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:114
>> +            self._port.pre_sharding_init()
> 
> Can we stop the host forwarder as part of Port.setup_test_run()? The consequence would be that it won't be run separately for the failed tests' second run.

Nice, thanks. I wasn't aware of this method. My only constraint was to kill the host daemon only once before sharding happens.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:377
>> +        self._adb_command = ['adb', '-s', device_serial]
> 
> Did you rebase before uploading this change? We no longer require 'adb' to be in the path, and a utility method was added to get the path to use (ChromiumAndroidDriver.path_to_adb()).

Done. Good catch.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:384
>> +                raise AssertionError('Could not push md5sum to device')
> 
> nit: to THE device.

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:389
>> +    def run_command(self, cmd):
> 
> This is only used in the AndroidCommands class, you should prefix it with _ to indicate visibility.

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:391
>> +        process = subprocess.Popen(cmd, stderr=subprocess.STDOUT, stdout=subprocess.PIPE)
> 
> You should use the host's executive here.

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:396
>> +        _log.debug('Run adb result: ' + str(output[:80]))
> 
> The worry I have here is that the current logging output includes the device's serial for the output. We'd loose that coverage here.

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:414
>> +                self.run_command(['%s_host' % self._md5sum_path, host_file]))
> 
> Would it be worth to assert for making sure that _md5sum_path has been setup (i.e. setup_md5sum() must have been invoked)?

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:427
>> +    DEVICE_FORWARDER_PATH = '/data/local/tmp/device_forwarder'
> 
> Could we just append "device_forwarder" to DEVICE_SOURCE_ROOT_DIR?

Done. Good point.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:432
>> +        self._device_forwarder_path = path_builder('device_forwarder')
> 
> Since there also is Forwarder.DEVICE_FORWARDER_PATH, I'd prefer to clarify here that it's the host path.

Done. I renamed path_builder to host_path_builder.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:433
>> +        self._host_forwarder_path = path_builder('host_forwarder')
> 
> For consistency's sake, maybe invoke Forwarder.path_to_{device,host}_forwarder()?

Done. Good point.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:439
>> +        _log.debug('Killing host daemon ')
> 
> nit: ends with a space.

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:447
>> +        _log.debug('Killing device daemon ')
> 
> nit: ends with a space.

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:451
>> +                '%s shell %s kill-server' % (Forwarder._get_adb_command_string(android_cmd), Forwarder.DEVICE_FORWARDER_PATH))
> 
> It wouldn't be hard to convert either of the calls to _run_shell_command_and_get_exit_code() to use a list of arguments instead of a string, while removing the need for Forwarder._get_adb_command_string().

Done. Good point.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:453
>> +            raise AssertionError('Could not kill device_forwarder:\n%s' % output)
> 
> nit: You print output on a new line here, but not in the other error messages (lines 474, 479, 485).

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:456
>> +        assert(FORWARDER_CONTROL_PORT_BEGIN + worker_number < Forwarder.ADB_CONTROL_PORT)
> 
> nit: no need for the parenthesis.

Done. I removed this assert.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:458
>> +        adb_control_port = self._worker_number_to_adb_control_port_map.get(worker_number)
> 
> adb_control_port is something entirely different than Forwarder.ADB_CONTROL_PORT. Could you please use a clearer name?

Done. Indeed.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:465
>> +        self._android_cmd.push_file_if_needed(self._device_forwarder_path, Forwarder.DEVICE_FORWARDER_PATH)
> 
> Again: distinguishing between the names for host and device paths would make this significantly clearer.

I noticed that I forgot to address this point. I will come back to it in a next patch set.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:466
>> +        # Command format: <ADB port>:<Device port>:<Host port>:<Host address>
> 
> nit: could you add a blank line before this command and lowercase the parts (/s/Device/device etc)? That'd make it a lot easier to read.
> "ADB port" should probably read "ADB control port".

Done. I also moved this part to a more appropriate place (right before it's used).

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:471
>> +                        Forwarder._get_adb_command_string(self._android_cmd), adb_control_port, Forwarder.ADB_CONTROL_PORT)
> 
> You seem to indent continuations with either four, eight or an arbitrary amount of spaces. There is no line length limit in WebKit, if you do decide to wrap a line (for example, to improve readability), please indent with four spaces.

Indeed, thanks.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:489
>> +        while port < FORWARDER_CONTROL_PORT_END and self._is_port_used(port):
> 
> Theoretically, with a thousand control ports, we'd open the /proc/net/tcp an equal amount of times. Since _is_port_used() is only used for this purpose, wouldn't it be easier to read /proc/net/tcp once and then find the first available port between [FORWARDER_CONTROL_PORT_BEGIN + worker_number, FORWARDER_CONTROL_PORT_END]?

Good point. Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:497
>> +        for line in open('/proc/net/tcp').xreadlines():
> 
> Instead of just calling open(), could we go through webkitpy.common.system.filesystem?

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:506
>> +                _log.debug('  Port %d is used.' % host_port)
> 
> nit: Why are these outputs indented?

No obvious reason. This was taken from Chromium.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:516
>> +    def _run_shell_command_and_get_exit_code(cmd):
> 
> Since Executive.run_command() knows both the output and exit code, it would be a lot easier to add an return_exit_code_and_output argument. That'd be three lines of code, as opposed to bypassing the entire executive.

Good point. Note that I still need the '; echo *$?' trick used below since adb shell doesn't return the exit code from the command which was run on the device.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:749
>> +        forwarder = Forwarder(self._port._build_path, self._android_cmd)
> 
> Is forwarder intentionally not being stored?

Yes we don't need to store it. I can make setup_redirections a static method if you prefer. What do you think?

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:750
>> +        forwarder.setup_redirections(self._worker_number, [(port, port) for port in FORWARD_PORTS], '127.0.0.1')
> 
> If host_address is always 127.0.0.1 we shouldn't pass it as an argument.

Done.

[Philippe Liard]

I'm going to upload another patch after I did a rebase.

[Philippe Liard]

Created attachment 177733 [details]
Patch

[felipe]

LGTM

[Tony Chang]

Comment on attachment 177733 [details]
Patch

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

> Tools/Scripts/webkitpy/common/system/executive.py:388
> +                    return_exit_code_and_output=False,

Is this necessary?  Would it be sufficient to include an error_handler?  The error_handler gets both the exit_code and the output and will always be called before |run_command| returns the output.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:33
> +import sets

Nit: This appears to be unused (and should be unnecessary because of the built-in "set" type).

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:450
> +        (exit_code, output) = Forwarder._run_shell_command_and_get_exit_code(

Nit: No () on the left side, although if you use an error handler, I don't think you need to check the return value at all.  Same with the other calls to _run_adb_shell_command_and_get_exit_code below.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:521
> +    @staticmethod
> +    def _run_adb_shell_command_and_get_exit_code(command_executor, android_cmd, cmd_array):
> +        # Note that the extra '*' character below is used to handle the case where the process' output is not newline terminated.

Can we add some unit tests for this function?  It looks pretty fragile.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:528
> +        if status_separator_pos == 0:

Nit: if not status_separator_pos:

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:563
> +        self._android_cmd = AndroidCommands(

Nit: android_cmd -> android_commands. We don't normally use abbreviations for variable names in WebKit.

[Philippe Liard]

Thanks Tony for the review. I will address your comments in the next few days. I'm busy right now with some P1 bugs and sheriffing.

[Eric Seidel (no email)]

I wrote this up last night on a whim:
https://github.com/eseidel/webkit/compare/device_connection

We should combine our efforts!  I was not aware of your patch at the time.  Note that the tip of that branch does not work, as I was not aware of the md5sum dependency.  Having DeviceConnection know how to push md5sum onto the device would be a layering violtion (since DeviceConnection shouldn't need to know about chromium build locations).

[Philippe Liard]

Created attachment 179740 [details]
Patch

[Philippe Liard]

Comment on attachment 177733 [details]
Patch

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

>> Tools/Scripts/webkitpy/common/system/executive.py:388
>> +                    return_exit_code_and_output=False,
> 
> Is this necessary?  Would it be sufficient to include an error_handler?  The error_handler gets both the exit_code and the output and will always be called before |run_command| returns the output.

Indeed I didn't get that ScriptError was wrapping all the necessary information.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:33
>> +import sets
> 
> Nit: This appears to be unused (and should be unnecessary because of the built-in "set" type).

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:450
>> +        (exit_code, output) = Forwarder._run_shell_command_and_get_exit_code(
> 
> Nit: No () on the left side, although if you use an error handler, I don't think you need to check the return value at all.  Same with the other calls to _run_adb_shell_command_and_get_exit_code below.

The error handler used with partial application does the trick indeed in a much nicer way.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:521
>> +        # Note that the extra '*' character below is used to handle the case where the process' output is not newline terminated.
> 
> Can we add some unit tests for this function?  It looks pretty fragile.

We have been using it for quite a while in Chromium. I'm not very familiar with testing in Python/WebKit to be honest.
I think we will be notified early enough if it fails :)

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:528
>> +        if status_separator_pos == 0:
> 
> Nit: if not status_separator_pos:

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:563
>> +        self._android_cmd = AndroidCommands(
> 
> Nit: android_cmd -> android_commands. We don't normally use abbreviations for variable names in WebKit.

Done.

[Tony Chang]

Comment on attachment 179740 [details]
Patch

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

This looks much better.  Thanks!

> Tools/ChangeLog:40
> +        * DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp:
> +        * Scripts/webkitpy/common/system/executive.py:
> +        (Executive.run_command):
> +        * Scripts/webkitpy/layout_tests/port/chromium_android.py:
> +        (ChromiumAndroidPort.setup_test_run):
> +        (ChromiumAndroidPort.check_build):
> +        (ChromiumAndroidPort.path_to_host_forwarder):
> +        (ChromiumAndroidPort):
> +        (ChromiumAndroidPort.path_to_device_forwarder):

Please regenerate the ChangeLog.  You can run prepare-ChangeLog to do that.

[Eric Seidel (no email)]

Comment on attachment 179740 [details]
Patch

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

Have to run, more comments soon.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:498
> +    def setup_md5sum(self):
> +        self._md5sum_path = self._path_builder(MD5SUM_DEVICE_FILE_NAME)
> +        if not self.file_exists_on_device(MD5SUM_DEVICE_PATH):
> +            if not self.push_to_device(self._md5sum_path, MD5SUM_DEVICE_PATH):
> +                raise AssertionError('Could not push md5sum to the device')

Seems these paths should be members of this class, if this information is going to be used here?  Or passed in from teh chromium port?  I had trouble with this layering in my attempt at this too.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:501
> +    def get_adb_command(self):
> +        return self._adb_command

WebKit style doesn't normally use get_ for getters.

[Eric Seidel (no email)]

Comment on attachment 179740 [details]
Patch

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

I think you and I should compare notes.  Are you reachable on #webkit?  I'm eseidel there.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:39
> +from webkitpy.common.system.filesystem import FileSystem

Including this is rarely correct. :)

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:529
> +        assert os.path.exists(host_file)

Please try to use filesystem.exists instead of os.path.exists so you can mock/unittest your funtions more easily.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:547
> +class Forwarder:

All classes should inherit from object in modern python. :)  (At least that's my understanding.)

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:570
> +    @staticmethod
> +    def kill_host_daemon(command_executor, host_path_builder):

Do you reall want these static?  I find @staticmethod to be deceptive in python.  Intially I think "oh, this doesn't need any members" so I make something static... and then I realize it's not easily mockable and cry.  Even @classmethods are more mockable than statics. :)

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:624
> +        for line in FileSystem().open_text_file_for_reading('/proc/net/tcp').xreadlines():

Bad.  This makes this method unmockable.  Are you sure you can't pass in a filesystem or Host?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:651
> +    @staticmethod

Generally I've found a single shared instance works out better than a bunch of static methods.  You could split these off into a singleton if that would work better?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:774
> +        install_result = self._android_commands.run_adb_command(['install', drt_host_path])

In my version of this I named this DeviceConnection with the idea that we might some day make it generic not just for android.  That may have been over-design on my part.  But I do think the idea of teaching a "hosted" driver how to talk to the target is generic and not necessarily specific to ADB or android.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:914
> +        forwarder = Forwarder(self._port._executive.run_command, self._port._build_path, self._android_commands)

Why not just pass it an executive instead of a run_command method pointer?

[Philippe Liard]

Created attachment 179921 [details]
Patch

[Philippe Liard]

Comment on attachment 179740 [details]
Patch

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

>> Tools/ChangeLog:40
>> +        (ChromiumAndroidPort.path_to_device_forwarder):
> 
> Please regenerate the ChangeLog.  You can run prepare-ChangeLog to do that.

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:39
>> +from webkitpy.common.system.filesystem import FileSystem
> 
> Including this is rarely correct. :)

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:498
>> +                raise AssertionError('Could not push md5sum to the device')
> 
> Seems these paths should be members of this class, if this information is going to be used here?  Or passed in from teh chromium port?  I had trouble with this layering in my attempt at this too.

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:501
>> +        return self._adb_command
> 
> WebKit style doesn't normally use get_ for getters.

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:529
>> +        assert os.path.exists(host_file)
> 
> Please try to use filesystem.exists instead of os.path.exists so you can mock/unittest your funtions more easily.

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:547
>> +class Forwarder:
> 
> All classes should inherit from object in modern python. :)  (At least that's my understanding.)

Done. I'm still pretty new to Python :)

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:570
>> +    def kill_host_daemon(command_executor, host_path_builder):
> 
> Do you reall want these static?  I find @staticmethod to be deceptive in python.  Intially I think "oh, this doesn't need any members" so I make something static... and then I realize it's not easily mockable and cry.  Even @classmethods are more mockable than statics. :)

I'm concerned about the maintenance cost. I would like to keep this consistent with the Forwarder class in Chromium at least from the design perspective if that's ok with you.

We can still improve things later on in a follow up when we also start really benefiting from mocking. It seems to me that this is a bit premature for now since this is not unit tested yet.

To be honest I also would like to get this in soon so that I don't have to deal with nasty rebases anymore :)

Thanks for sharing your experience anyway. This will be useful later on.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:624
>> +        for line in FileSystem().open_text_file_for_reading('/proc/net/tcp').xreadlines():
> 
> Bad.  This makes this method unmockable.  Are you sure you can't pass in a filesystem or Host?

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:651
>> +    @staticmethod
> 
> Generally I've found a single shared instance works out better than a bunch of static methods.  You could split these off into a singleton if that would work better?

See my reply to your comment line 570.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:774
>> +        install_result = self._android_commands.run_adb_command(['install', drt_host_path])
> 
> In my version of this I named this DeviceConnection with the idea that we might some day make it generic not just for android.  That may have been over-design on my part.  But I do think the idea of teaching a "hosted" driver how to talk to the target is generic and not necessarily specific to ADB or android.

I agree with you although I think this is a bit premature for now :)

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:914
>> +        forwarder = Forwarder(self._port._executive.run_command, self._port._build_path, self._android_commands)
> 
> Why not just pass it an executive instead of a run_command method pointer?

I'm injecting only what Forwarder actually needs. It doesn't need to know about executive and this should also make mocking easier since you seem to like it (I can see why) :)

[Philippe Liard]

Thanks Eric!

[Peter Beverloo]

Comment on attachment 179921 [details]
Patch

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

Hi Philippe, a few more comments. I tested your patch and verified that it works both for layout tests and for performance tests. Thanks!

For reference's sake, when are you going on holiday?

> Tools/ChangeLog:5
> +

This needs the "Reviewed by NOBODY (OOPS!)." line, otherwise it won't be able to pass through the commit queue.

> Tools/ChangeLog:-397
> -

Accidental change?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:679
> +            self._port.host.filesystem, self._port._executive.run_command, self._adb_command(), self._port._build_path, self._log_debug)

This means that it'll be required for the user to either have "adb" installed or have a Chromium WebKit checkout if they want to instantiate ChromiumAndroidDriver. This is why the _adb_command_base property exists, so it can be determined lazily.

Can we remove ChromiumAndroidDriver._adb_command() and ChromiumAndroidDriver._adb_command_base altogether and move this logic to AndroidCommands?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:730
> +        self._android_commands.setup_md5sum()

Regarding Eric's plans to have a more generic DeviceConnection class, it may make sense to have an intermediate class to encapsulate md5sum/push_if_needed... Not sure, something to address later, though I agree Eric's abstraction is cleaner.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:759
> +    def _push_data_if_needed(self):

Can we perhaps just call these in _setup_test()? It doesn't really add much to have a separate method.

[Peter Beverloo]

Also, please run the webkitpy unit tests. You can do so by typing:
$ Tools/Scripts/test-webkitpy

I'm seeing the following error:


[446/1615] webkitpy.layout_tests.port.chromium_android_unittest.AndroidPerfTest.test_perf_output_regexp erred:
  Traceback (most recent call last):
    File "/b/WebKit/Tools/Scripts/webkitpy/layout_tests/port/chromium_android_unittest.py", line 312, in test_perf_output_regexp
      profiler = chromium_android.AndroidPerf(host, '/bin/executable', '/tmp/output', 'adb-path', 'device-serial', 'foo')
    File "/b/WebKit/Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py", line 379, in __init__
      self._device_serial = device_serial
  NameError: global name 'device_serial' is not defined

[Philippe Liard]

Created attachment 180136 [details]
Patch

[Philippe Liard]

Comment on attachment 179921 [details]
Patch

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

>> Tools/ChangeLog:5
>> +
> 
> This needs the "Reviewed by NOBODY (OOPS!)." line, otherwise it won't be able to pass through the commit queue.

Done.

>> Tools/ChangeLog:-397
>> -
> 
> Accidental change?

Good catch.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:679
>> +            self._port.host.filesystem, self._port._executive.run_command, self._adb_command(), self._port._build_path, self._log_debug)
> 
> This means that it'll be required for the user to either have "adb" installed or have a Chromium WebKit checkout if they want to instantiate ChromiumAndroidDriver. This is why the _adb_command_base property exists, so it can be determined lazily.
> 
> Can we remove ChromiumAndroidDriver._adb_command() and ChromiumAndroidDriver._adb_command_base altogether and move this logic to AndroidCommands?

Good catch!

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:730
>> +        self._android_commands.setup_md5sum()
> 
> Regarding Eric's plans to have a more generic DeviceConnection class, it may make sense to have an intermediate class to encapsulate md5sum/push_if_needed... Not sure, something to address later, though I agree Eric's abstraction is cleaner.

I will address this later.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:759
>> +    def _push_data_if_needed(self):
> 
> Can we perhaps just call these in _setup_test()? It doesn't really add much to have a separate method.

Indeed.

[Philippe Liard]

FYI, the last patch also fixes the error you mentioned in comment #47.

[Philippe Liard]

Eric, can you please take another look?

[Eric Seidel (no email)]

Comment on attachment 180136 [details]
Patch

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

I think this is better than what we have, but I don't entirely agree with the AndroidCommands design.  Since mostly I think you're just tryihng to move us off of the deprecated forwarder and onto forwarder2 (which sounds like a good thing!) the AndroidCommands stuff is largely unrelated (but waaaay overdue).  I'll see if I can come up with a working alternative tomrorow and rebase your patch on top of it for you.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:499
> +    def setup_md5sum(self):
> +        self._md5sum_path = self._path_builder(AndroidCommands.MD5SUM_DEVICE_FILE_NAME)
> +        if not self.file_exists_on_device(AndroidCommands.MD5SUM_DEVICE_PATH):
> +            if not self.push_to_device(self._md5sum_path, AndroidCommands.MD5SUM_DEVICE_PATH):
> +                raise AssertionError('Could not push md5sum to the device')

I don't see why we're even installing md5 on the device.  Doesn't the device have something which can already produce an md5?

The whole Md5 thing is a layer on top of this commands object IMO.  Its unfortunate that it's tied in this way.

[Philippe Liard]

Android doesn't provide any sort of file hashing command unfortunately. That's why we have to push our own.

If Forwarder is a separate class, then it has to receive an object like AndroidCommands to be able to talk to the device.
The other alternative would have been not to introduce any new class (which seems more than reasonable to me). I don't think having a separate AndroidCommands class is totally ugly to be honest (it didn't shock previous reviewers at least :)). md5sum is only an implementation detail used to push files to the device (which can be seen as a command). I don't think having the file push functionality in a separate layer/class would bring us anything for now.

What do you think?

[Eric Seidel (no email)]

Comment on attachment 180136 [details]
Patch

I don't 100% agree with the design, but I clearly have not found time for this.

[Philippe Liard]

Thanks Eric. I will do a rebase and upload an updated patch.

Cheers,
Philippe.

[Peter Beverloo]

Resolving as WontFix given that Chromium moved to Blink.