Bug 92877 – [Chromium-Android] Run layout tests on multiple devices in parallel

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Xianzhu Wang]

Because of the nature of apk, it's difficult to run layout tests in parallel on a single device. However, we can use multiple devices to speed up.

[Xianzhu Wang]

Created attachment 155871 [details]
Patch

[Marcus Bulach]

Comment on attachment 155871 [details]
Patch

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

really good stuff, many thanks!
informal review here, looking forward to have this sharding enabled on our bots!

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:326
> +        self._run_adb_command(['root'])

nit: not for now, but we'll eventually run all tests in non-rooted devices...

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:344
> +        self._run_adb_command(['shell', '%s %s' % (DEVICE_FORWARDER_PATH, FORWARD_PORTS)])

all other _run_adb_command have one param per item, so maybe this should be:
'shell', DEVICE_FORWARDER_PATH, str(FORWARD_PORTS)
?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:347
> +        drt_host_path = self._port._path_to_driver()

nit: here, the line below and 360-363 block and a few other places, it's accessing "protected" methods from _port..
should they be public, i.e., without the _ prefix?

> Tools/Scripts/webkitpy/layout_tests/port/driver.py:169
> +            self.error_from_test, crash_log = self._get_crash_log(text, self.error_from_test, newer_than=start_time)

nit: here newer_than doesn't have a default param, so no need to name it..

> Tools/Scripts/webkitpy/layout_tests/port/driver.py:191
> +        return self._port._get_crash_log(self._crashed_process_name, self._crashed_pid, stdout, stderr, newer_than)

nit: I don't know here, but to keep the previous behavior it'd probably be better to name the newer_than param (in case there are other params after stderr with default values..)

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:-249
> -        optparse.make_option("--adb-args", type="string",

nit: one thing that we implemented was --device (or here, --adb-device)..
then, way up on _get_devices, it'd return just this param if passed..

reasons:
1. allow testing a specific device.
2. in the future, allow a single bot to have multiple device types, so that we can run a set of tests on device A, then the same set of tests on device B, etc..

[Xianzhu Wang]

Comment on attachment 155871 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:326
>> +        self._run_adb_command(['root'])
> 
> nit: not for now, but we'll eventually run all tests in non-rooted devices...

Yes. This is tracked in https://bugs.webkit.org/show_bug.cgi?id=91872.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:344
>> +        self._run_adb_command(['shell', '%s %s' % (DEVICE_FORWARDER_PATH, FORWARD_PORTS)])
> 
> all other _run_adb_command have one param per item, so maybe this should be:
> 'shell', DEVICE_FORWARDER_PATH, str(FORWARD_PORTS)
> ?

'adb shell' accepts both multiple parameters or a whole command line in a single parameter.
Because FORWARD_PORTS is a string ('8000 8080 8443 8880 9323'), we use the second form.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:347
>> +        drt_host_path = self._port._path_to_driver()
> 
> nit: here, the line below and 360-363 block and a few other places, it's accessing "protected" methods from _port..
> should they be public, i.e., without the _ prefix?

In layout test python code, this seems an existing style that the driver classes can access the protected methods of the port classes. I think we'd better keep it in this change and discuss this separately :)

>> Tools/Scripts/webkitpy/layout_tests/port/driver.py:169
>> +            self.error_from_test, crash_log = self._get_crash_log(text, self.error_from_test, newer_than=start_time)
> 
> nit: here newer_than doesn't have a default param, so no need to name it..

Here I'm keeping the original style. The newer_than parameter in Port._get_crash_log() also doesn't have a default value. I guess the author's intention was to express the meaning of the parameter explicitly at the call site.

Dirk, what's your opinion?

>> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:-249
>> -        optparse.make_option("--adb-args", type="string",
> 
> nit: one thing that we implemented was --device (or here, --adb-device)..
> then, way up on _get_devices, it'd return just this param if passed..
> 
> reasons:
> 1. allow testing a specific device.
> 2. in the future, allow a single bot to have multiple device types, so that we can run a set of tests on device A, then the same set of tests on device B, etc..

Good suggestion. Will do it.

[Xianzhu Wang]

Created attachment 155892 [details]
Patch

[Dirk Pranke]

Comment on attachment 155871 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:228
> +        pass

is this really desired? It seems at least messy ...

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

Nit: consider using @memoized here instead.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:297
> +    def _get_device_serial(self, work_number):

probably better to call this worker_number ...

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:318
> +        self._teardown_performance()

isn't this a method on the port? do you need to move that to the driver?

>>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:347
>>> +        drt_host_path = self._port._path_to_driver()
>> 
>> nit: here, the line below and 360-363 block and a few other places, it's accessing "protected" methods from _port..
>> should they be public, i.e., without the _ prefix?
> 
> In layout test python code, this seems an existing style that the driver classes can access the protected methods of the port classes. I think we'd better keep it in this change and discuss this separately :)

There's no good way to declare "friends" in python, so either we have to either make methods public that shouldn't be, or make methods private that some other classes use. I consider Drivers friends of Ports, so the latter usage is okay to me.

>>> Tools/Scripts/webkitpy/layout_tests/port/driver.py:169
>>> +            self.error_from_test, crash_log = self._get_crash_log(text, self.error_from_test, newer_than=start_time)
>> 
>> nit: here newer_than doesn't have a default param, so no need to name it..
> 
> Here I'm keeping the original style. The newer_than parameter in Port._get_crash_log() also doesn't have a default value. I guess the author's intention was to express the meaning of the parameter explicitly at the call site.
> 
> Dirk, what's your opinion?

You are right, that was my intent, but I don't feel strongly that it needs to be here. I'd probably leave it, but I'd be fine if you deleted it, too.

>> Tools/Scripts/webkitpy/layout_tests/port/driver.py:191
>> +        return self._port._get_crash_log(self._crashed_process_name, self._crashed_pid, stdout, stderr, newer_than)
> 
> nit: I don't know here, but to keep the previous behavior it'd probably be better to name the newer_than param (in case there are other params after stderr with default values..)

in this case, it's clear which parameter newer_than is being mapped onto. I'd leave this alone.

[Xianzhu Wang]

Comment on attachment 155871 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:228
>> +        pass
> 
> is this really desired? It seems at least messy ...

In the new patch, http server is stopped here, and forwarder is stopped in ChromiumAndroidDriver.stop().

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:288
>> +    def _get_devices(self):
> 
> Nit: consider using @memoized here instead.

I tried but found it not very beneficial because we have the self._devices to store the values passed from the command line and it can also store the result from 'adb devices'.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:297
>> +    def _get_device_serial(self, work_number):
> 
> probably better to call this worker_number ...

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:318
>> +        self._teardown_performance()
> 
> isn't this a method on the port? do you need to move that to the driver?

Because now there are multiple drivers each managing one device, the method needs to be in driver to restore the performance governor of one device.

>>> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:-249
>>> -        optparse.make_option("--adb-args", type="string",
>> 
>> nit: one thing that we implemented was --device (or here, --adb-device)..
>> then, way up on _get_devices, it'd return just this param if passed..
>> 
>> reasons:
>> 1. allow testing a specific device.
>> 2. in the future, allow a single bot to have multiple device types, so that we can run a set of tests on device A, then the same set of tests on device B, etc..
> 
> Good suggestion. Will do it.

Done.

[Xianzhu Wang]

Created attachment 156116 [details]
Patch

[Dirk Pranke]

(In reply to comment #6)
> >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:318
> >> +        self._teardown_performance()
> > 
> > isn't this a method on the port? do you need to move that to the driver?
> 
> Because now there are multiple drivers each managing one device, the method needs to be in driver to restore the performance governor of one device.
> 

Sorry, I meant that I think this method is missing: you deleted the port's method, but forgot to add it to the driver.

[Xianzhu Wang]

(In reply to comment #8)
> 
> Sorry, I meant that I think this method is missing: you deleted the port's method, but forgot to add it to the driver.

Oh, my sorry too for misunderstanding :)

The method is just not shown in the diff, because it is unchanged. The diff shows that ChromiumAndroidDriver.__init__ is moved up of these methods.

[Dirk Pranke]

(In reply to comment #9)
> (In reply to comment #8)
> > 
> > Sorry, I meant that I think this method is missing: you deleted the port's method, but forgot to add it to the driver.
> 
> Oh, my sorry too for misunderstanding :)
> 
> The method is just not shown in the diff, because it is unchanged. The diff shows that ChromiumAndroidDriver.__init__ is moved up of these methods.

Oh, I see. right. I missed that __init__ was moving above it. Ok.

[WebKit Review Bot]

Comment on attachment 156116 [details]
Patch

Clearing flags on attachment: 156116

Committed r124481: <http://trac.webkit.org/changeset/124481>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.