[Chromium-Android] Run layout tests on unrooted devices

• Component: Tools / Tests
• Status: RESOLVED WONTFIX

[Xianzhu Wang]

For now Android run layout tests can only run on rooted devices with the rooted adb shell. It's necessary to let it run on unrooted devices.
This is a meta bug.

[Xianzhu Wang]

Though all depending bugs are resolved, there are still following works to do to achieve the goal to run on unrooted devices:

1) Unrooted forwarder
2) Unrooted way to synchronize date-time
and other things that we don't know yet

[Philippe Liard]

(In reply to comment #1)
> Though all depending bugs are resolved, there are still following works to do to achieve the goal to run on unrooted devices:
> 
> 1) Unrooted forwarder
> 2) Unrooted way to synchronize date-time
> and other things that we don't know yet

I'm going to send a CL addressing the second point.

[Philippe Liard]

Created attachment 161190 [details]
Patch

[Philippe Liard]

Note that this patch depends on https://chromiumcodereview.appspot.com/10867008/ which is being reviewed.

[Peter Beverloo]

Comment on attachment 161190 [details]
Patch

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

Thanks, Philippe! Some comments in-line..

> Tools/ChangeLog:3
> +        [Chromium-Android] Support LayoutTests on non-rooted devices.

Since this also touches a non-Chromium for Android specific test, I wouldn't use the [Chromium-Android] prefix here and append "for Chromium Android" to the title.

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:410
> +                                 'Please pass --%s.' % (configuration, most_recent_binary, most_recent_binary.lower()))

nit: The warning() method accepts multiple arguments for formatting, we should probably prefer that over string formatting.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:44
> +    os.path.join(os.environ['CHROME_SRC'], 'build', 'android', 'pylib'))

I'm not comfortable introducing this dependency. We're trying to get rid of dependencies on environment variable (like CHROME_SRC), and since we need adb commands in order parts of WebKit as well, I'd prefer to have a smaller but similar runner on the WebKit side. This is out of scope, but is there any way we can work around needing this right now?

> LayoutTests/ChangeLog:5
> +        http://crbug.com/143114

nit: it's uncommon to for Chromium to mention our own bug tracker, but it's harmless too. In general, you could use the URL field in the bug for this.

> LayoutTests/http/tests/cache/resources/cache-simulator.cgi:-6
> -print "Content-type: text/javascript\n"; 

I'd avoid touching this file if you're just removing a space.

> LayoutTests/http/tests/cache/resources/subresource-test.js:31
> +    var url = document.URL.replace(/subresource-expiration-..html/, "resources/current-time.cgi");

This seems rather fragile, can we rely on relative paths to work? Requesting "resources/current-time.cgi" should work fine.

> LayoutTests/http/tests/cache/resources/subresource-test.js:40
> +    return new Date((parseInt(req.responseText) * 1000) + elapsedTime);

nit: elapsedTime is the time the round trip took, while I guess we only care about half of that (server -> client)?

> LayoutTests/http/tests/cache/resources/subresource-test.js:85
> +    now = getServerDate();

This introduces 24 additional synchronous requests for the subresource-expiration-{1,2}.html test cases. While this probably isn't a huge deal considering it's (for most platforms) local, could we maybe determine the difference between the server and client times once during initialization, and then correct the "new Date()" statement here with the offset?

[Peter Beverloo]

Also, you probably should have opened a separate bug (blocking this one) for the synchronization :-).

[Philippe Liard]

Comment on attachment 161190 [details]
Patch

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

>> Tools/ChangeLog:3
>> +        [Chromium-Android] Support LayoutTests on non-rooted devices.
> 
> Since this also touches a non-Chromium for Android specific test, I wouldn't use the [Chromium-Android] prefix here and append "for Chromium Android" to the title.

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:410
>> +                                 'Please pass --%s.' % (configuration, most_recent_binary, most_recent_binary.lower()))
> 
> nit: The warning() method accepts multiple arguments for formatting, we should probably prefer that over string formatting.

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:44
>> +    os.path.join(os.environ['CHROME_SRC'], 'build', 'android', 'pylib'))
> 
> I'm not comfortable introducing this dependency. We're trying to get rid of dependencies on environment variable (like CHROME_SRC), and since we need adb commands in order parts of WebKit as well, I'd prefer to have a smaller but similar runner on the WebKit side. This is out of scope, but is there any way we can work around needing this right now?

I can avoid relying on $CHROME_SRC and use a relative path assuming that this file won't be moved. Regarding the use of android_commands, AndroidCommands.PushIfNeeded() provides a way to sync data files with the device by using a custom implementation of md5sum (written in C++ and depending on Chromium's base/). Although we could have our own Python layer for that in WebKit, we can hardly do the same for md5sum (I don't think you want to duplicate it either). So it seems that we would have to use Chromium's md5sum (being submitted under tools/android/md5sum) anyway.
Since we own that file, which is Chromium-specific, I think that preventing us from depending on Chromium might be an unnecessary constraint. I agree that we could introduce at least a thin layer in WebKit, calling Chromium's python, rather than calling it directly though.
But I'm sure that I don't see the full picture since this is my first WebKit contribution. What do you think? Is this dependency to Chromium the only one?

>> LayoutTests/ChangeLog:5
>> +        http://crbug.com/143114
> 
> nit: it's uncommon to for Chromium to mention our own bug tracker, but it's harmless too. In general, you could use the URL field in the bug for this.

Done.

>> LayoutTests/http/tests/cache/resources/cache-simulator.cgi:-6
>> -print "Content-type: text/javascript\n"; 
> 
> I'd avoid touching this file if you're just removing a space.

Done.

>> LayoutTests/http/tests/cache/resources/subresource-test.js:31
>> +    var url = document.URL.replace(/subresource-expiration-..html/, "resources/current-time.cgi");
> 
> This seems rather fragile, can we rely on relative paths to work? Requesting "resources/current-time.cgi" should work fine.

Done.

>> LayoutTests/http/tests/cache/resources/subresource-test.js:40
>> +    return new Date((parseInt(req.responseText) * 1000) + elapsedTime);
> 
> nit: elapsedTime is the time the round trip took, while I guess we only care about half of that (server -> client)?

Good point.

>> LayoutTests/http/tests/cache/resources/subresource-test.js:85
>> +    now = getServerDate();
> 
> This introduces 24 additional synchronous requests for the subresource-expiration-{1,2}.html test cases. While this probably isn't a huge deal considering it's (for most platforms) local, could we maybe determine the difference between the server and client times once during initialization, and then correct the "new Date()" statement here with the offset?

Good point.

[Philippe Liard]

I've created this specific bug: https://bugs.webkit.org/show_bug.cgi?id=95346

Sorry for polluting this one!

[Peter Beverloo]

Comment on attachment 161190 [details]
Patch

Marking as obsolete since we're moving discussion to Bug 95346.

[Peter Beverloo]

Resolving as WontFix given that Chromium moved to Blink.