Bug 96687 – [Chromium] Support the --{in,out,err}-fifo arguments on TestWebKitAPI and webkit_unit_tests

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Peter Beverloo]

[Chromium] Support the --{in,out,err}-fifo arguments on TestWebKitAPI and webkit_unit_tests

[Peter Beverloo]

Created attachment 163946 [details]
Patch

[Adam Barth]

Maybe tony@ is a good person to review this patch?

[Tony Chang]

Conceptually, this is OK, but would it be easier to have a wrapper script that runs on the target device?  This script would start the test binary, collect stdout and stderr separately, and write it to a known location or send it back to the host.  This seems easier than modifying test binaries and would work for test binaries that aren't in WebKit.

[Peter Beverloo]

(In reply to comment #3)
> Conceptually, this is OK, but would it be easier to have a wrapper script that runs on the target device?  This script would start the test binary, collect stdout and stderr separately, and write it to a known location or send it back to the host.  This seems easier than modifying test binaries and would work for test binaries that aren't in WebKit.

The binaries are packed and then installed as APK files because we can't rely on having root access to the devices. The APKs then have to be started using the activity manager. Starting them is a shell command (which we send to the phone), but they will be executed in the background like any other application on the phone.

By default the output will end up in the logs, but these can be very tedious to parse. All we want is the STDOUT (and STDERR), giving us exactly the same result as when it'd have been run locally.

This is how DumpRenderTree on Android gets its output (formatted in the Python script):
http://build.webkit.org/builders/Chromium%20Android%20Release%20%28Tests%29/builds/226/steps/layout-test/logs/stdio

[Tony Chang]

(In reply to comment #4)
> (In reply to comment #3)
> > Conceptually, this is OK, but would it be easier to have a wrapper script that runs on the target device?  This script would start the test binary, collect stdout and stderr separately, and write it to a known location or send it back to the host.  This seems easier than modifying test binaries and would work for test binaries that aren't in WebKit.
> 
> The binaries are packed and then installed as APK files because we can't rely on having root access to the devices. The APKs then have to be started using the activity manager. Starting them is a shell command (which we send to the phone), but they will be executed in the background like any other application on the phone.
> 
> By default the output will end up in the logs, but these can be very tedious to parse. All we want is the STDOUT (and STDERR), giving us exactly the same result as when it'd have been run locally.
> 
> This is how DumpRenderTree on Android gets its output (formatted in the Python script):
> http://build.webkit.org/builders/Chromium%20Android%20Release%20%28Tests%29/builds/226/steps/layout-test/logs/stdio

I understand all this, but what I'm asking is rather than having to modify every test binary, can you run a wrapper script/program on the target device that collects stdout/stderr and passes it back to the host?

[Peter Beverloo]

(In reply to comment #5)
> I understand all this, but what I'm asking is rather than having to modify every test binary, can you run a wrapper script/program on the target device that collects stdout/stderr and passes it back to the host?

I'm not sure I understand what you mean. Due to the way the activity manager executes APKs in the background, which comes down to sending an intent, their stdout/stderr will never be available in the shell. They directly go to the logs.

It *is* possible to run the test suites as normal executables on the devices, which does allow us to get the output directly, but this requires root access to the device as well. Having a rooted device is something we can't rely on and don't want to enforce either. Philippe removed the need for that in r128181, and Chromium is about to switch over too.

[Tony Chang]

Can APKs launch other processes (i.e., like the way that the Chrome browser process launches the renderer process)?  Is it possible to make an APK that launches TestWebKitAPI (bundled in the same APK) and collects TestWebKitAPI's stdout/stderr?

[Peter Beverloo]

(In reply to comment #7)
> Can APKs launch other processes (i.e., like the way that the Chrome browser process launches the renderer process)?  Is it possible to make an APK that launches TestWebKitAPI (bundled in the same APK) and collects TestWebKitAPI's stdout/stderr?

Yes -- all of Chrome for Android is a single APK, and there is a separation between the browser, renderer and gpu process. For test-suites, this basically comes down to what we do now: we embed the test suite as a shared library (as such, we have TestWebKitAPI.so) and load and execute it using a very simple Java wrapper. This is shared among every test suite in Chrome, so diverging from this doesn't seem like a good idea.

An alternative would be to copy code from Chromium, i.e. _WatchTestOutput in src/build/android/pylib/test_package.py, but there's a lot of additional Python infrastructure required to make that work correctly. Chromium's implementation relies on an ADB interface licensed as Apache 2 code, which is why we can't import that.

[Peter Beverloo]

A third option could be to rely on Chromium's pylib (in src/build/android/pylib) directly in WebKit's test runner. This is available in Source/WebKit/chromium for Chromium checkouts, but no other areas of the test-runner scripts do this and this would be very fragile, as it doesn't have a stable API and lots of refactoring is still going on in there.

[Tony Chang]

Does src/build/android/pylib/test_package.py run on the target device or the host?

It sounds like you're saying for unit tests in Chromium, they use this python code.  I think it would be nice if for WebKit test binaries, we were able to use the same code/approach.

If you're worried about that code changing in API incompatible ways, we can freeze the revision we pull in Source/WebKit/chromium/DEPS.

[Peter Beverloo]

(In reply to comment #10)
> Does src/build/android/pylib/test_package.py run on the target device or the host?
> 
> It sounds like you're saying for unit tests in Chromium, they use this python code.  I think it would be nice if for WebKit test binaries, we were able to use the same code/approach.
> 
> If you're worried about that code changing in API incompatible ways, we can freeze the revision we pull in Source/WebKit/chromium/DEPS.

All Python code executes on the host, and talks to the device using an "adb" tool. The android_testrunner project it depends on[1] is the Apache 2 project. We already use all Chromium infrastructure for creating the APK itself.

Pulling in both build/android/pylib/ and third_party/android_testrunner/ to a second location (using a fixed revision) may be an idea. The code hasn't been designed for usage outside of the Chromium tree, isn't completely upstreamed yet and all of it is in transition between being used from within an Android tree and from a Chromium tree (called "SDK" build) right now. This definitely is a goal in the long term, however.

My plan is to factor some parts of the ChromiumAndroidDriver layout test runner out of the script and add an AndroidTestRunner object which can be used by all three of WebKit's suites, with a minimal interface to talk to the device using ADB..


[1] http://src.chromium.org/viewvc/chrome/trunk/src/third_party/android_testrunner/

[Tony Chang]

Comment on attachment 163946 [details]
Patch

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

It sounds like you're saying it'll be a long time before running gtests in Chromium will be stable enough for us to use, so we should use this in the meantime.  I think that's fine, but please make sure to have a tracking bug about switching over to a common framework.  For example, why can't Chromium use an approach like this?

> Source/WebKit/chromium/WebKit.gypi:246
> +                'webkit_unittest_files': [
> +                    'tests/ForwardIOStreamsAndroid.cpp',
> +                    'tests/ForwardIOStreamsAndroid.h',

Please put this into a separate static_library target and have the three targets depend on it rather than compiling it three times.

[Peter Beverloo]

Created attachment 164158 [details]
Patch

[Peter Beverloo]

(In reply to comment #12)
> (From update of attachment 163946 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163946&action=review
> 
> It sounds like you're saying it'll be a long time before running gtests in Chromium will be stable enough for us to use, so we should use this in the meantime.  I think that's fine, but please make sure to have a tracking bug about switching over to a common framework.  For example, why can't Chromium use an approach like this?
> 
> > Source/WebKit/chromium/WebKit.gypi:246
> > +                'webkit_unittest_files': [
> > +                    'tests/ForwardIOStreamsAndroid.cpp',
> > +                    'tests/ForwardIOStreamsAndroid.h',
> 
> Please put this into a separate static_library target and have the three targets depend on it rather than compiling it three times.

Their method is better, which is why I intend to switch to it when it stabilizes :-).

I've added the target to WebKitUnitTests.gyp, but am in doubt about whether it may make more sense to put it in WebKit.gyp. Happy to move it..

[Tony Chang]

Comment on attachment 164158 [details]
Patch

Putting the target in WebKitUnitTests.gyp is fine. I'm not sure if it's weirder to has test libraries in WebKit.gyp or to have DumpRenderTree.gyp depend on WebKitUnitTests.gyp.  *shrug*

[WebKit Review Bot]

Comment on attachment 164158 [details]
Patch

Clearing flags on attachment: 164158

Committed r128628: <http://trac.webkit.org/changeset/128628>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.