[Webkit-unassigned] [Bug 99588] Use the new forwarder2
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Dec 5 05:07:51 PST 2012
https://bugs.webkit.org/show_bug.cgi?id=99588
--- Comment #31 from Philippe Liard <pliard at chromium.org> 2012-12-05 05:10:17 PST ---
(From update of attachment 177474)
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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list