[Webkit-unassigned] [Bug 99588] Use the new forwarder2

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=99588





--- Comment #27 from Peter Beverloo <peter at chromium.org>  2012-12-04 08:01:25 PST ---
(From update of attachment 177474)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.