Bug 103581 – run-perf-tests --chromium-android should not require adb in my path

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Eric Seidel (no email)]

run-perf-tests --chromium-android should not require adb in my path

If you don't have adb in your path it barfs:

Running 1 tests
Traceback (most recent call last):
  File "/src/WebKit/Tools/Scripts/run-perf-tests", line 40, in <module>
    sys.exit(PerfTestsRunner().run())
  File "/src/WebKit/Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py", line 187, in run
    unexpected = self._run_tests_set(sorted(list(tests), key=lambda test: test.test_name()), self._port)
  File "/src/WebKit/Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py", line 318, in _run_tests_set
    driver = port.create_driver(worker_number=0, no_timeout=True)
  File "/src/WebKit/Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py", line 257, in create_driver
    no_timeout=True)
  File "/src/WebKit/Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py", line 340, in __init__
    self._device_serial = port._get_device_serial(worker_number)
  File "/src/WebKit/Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py", line 322, in _get_device_serial
    devices = self._get_devices()
  File "/src/WebKit/Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py", line 315, in _get_devices
    result = self._executive.run_command(['adb', 'devices'], error_handler=self._executive.ignore_error)
  File "/src/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 405, in run_command
    close_fds=self._should_close_fds())
  File "/src/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 461, in popen
    return subprocess.Popen(*args, **kwargs)
  File "/usr/lib/python2.7/subprocess.py", line 679, in __init__
OSError: [Errno 2] No such file or directory
Exception AttributeError: "'ChromiumAndroidDriver' object has no attribute '_device_serial'" in <bound method ChromiumAndroidDriver.__del__ of <webkitpy.layout_tests.port.chromium_android.ChromiumAndroidDriver object at 0x1198150>> ignored


run-webkit-tests --chromium-android barfs as well, probably for the same reason (it just barfs before logging is set up):

%run-webkit-tests --chromium-android

OSError raised: [Errno 2] No such file or directory
No handlers could be found for logger "__main__"
Failed to execute Tools/Scripts/new-run-webkit-tests at /src/WebKit/Tools/Scripts/run-webkit-tests line 126.

[Eric Seidel (no email)]

Yup.  Adding adb to my path fixes both. :)

[Peter Beverloo]

The "adb" tool is available in Source/WebKit/chromium/third_party/android_tools/sdk/ for Chromium-Android checkouts. I agree that we should get rid of this.

We can add an _path_to_adb() method to ChromiumAndroidPort and use this both for ChromiumAndroidPort._get_devices() and for ChromiumAndroidDriver.__init__ (for self._adb_command).

run-api-tests and run-chromium-webkit-unit-tests have the same dependency right now. There, we may have to set the PATH variable before invoking Chromium's run_tests.py.

Not sure if you're going to work on this, otherwise I'll upload a patch tomorrow.

[Eric Seidel (no email)]

Please, consider it all yours. :)  I have my hands full trying to get --profile to be more user friendly. :)

[Peter Beverloo]

Created attachment 176722 [details]
Patch

[WebKit Review Bot]

Attachment 176722 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1
Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:267:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:273:  trailing whitespace  [pep8/W291] [5]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Peter Beverloo]

Created attachment 176725 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 176725 [details]
Patch

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

Lgtm

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:272
> +        assert provided_version, 'Please be sure to have a Chromium-Android check-out before running layout tests.'

I might say something like: path/to/adb is missing.  Are you sure you ran update-webkit --chromium-android?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:274
> +        if not path_version or provided_version > path_version:

You might log in this rare case.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:331
> +        try:

This try is redundant. Also I think you can pass ignore_error=true instead of having to dig up a handler function

[Peter Beverloo]

Created attachment 176746 [details]
Patch

[Peter Beverloo]

Comment on attachment 176725 [details]
Patch

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

Thanks Eric!

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:272
>> +        assert provided_version, 'Please be sure to have a Chromium-Android check-out before running layout tests.'
> 
> I might say something like: path/to/adb is missing.  Are you sure you ran update-webkit --chromium-android?

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:274
>> +        if not path_version or provided_version > path_version:
> 
> You might log in this rare case.

Done. I made an if/elif/else out of this.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:331
>> +        try:
> 
> This try is redundant. Also I think you can pass ignore_error=true instead of having to dig up a handler function

This is an exception, so it wouldn't be caught by the error_handler (which happens after running the command if the exit code != 0). I changed line 333 to only listen to OSError exceptions.

[WebKit Review Bot]

Attachment 176746 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1
Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:277:  [ChromiumAndroidPort.path_to_adb] Undefined variable 'log'  [pylint/E0602] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Peter Beverloo]

Comment on attachment 176746 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:277
>> +            log._warning('The "adb" version in your path is older than the one checked in, consider updating your local Android SDK. Using the checked in one.')
> 
> [ChromiumAndroidPort.path_to_adb] Undefined variable 'log'  [pylint/E0602] [5]

Hm. Bad. Sorry..

It seems to execute this code twice, the first time of which (as first line of output of new-run-webkit-tests) I'm getting this error:

$ Tools/Scripts/new-run-webkit-tests --chromium-android fast/text/shaping
No handlers could be found for logger "webkitpy.layout_tests.port.chromium_android"

This is because it's executed in run_webkit_tests._set_up_derived_options() as the default value for the number of child processes. This is *before* we attach the printer, which in turn creates a MeteredStream that registers to the logger as the handler.

checking for len(_log.handlers) doesn't work either. I'll try to find a fix for this.

[Eric Seidel (no email)]

dpranke may know.

[Peter Beverloo]

It turns out that the Mac port has exactly the same issue, and included a FIXME (see Tools/Scripts/webkitpy/layout_tests/port/mac.py).

# FIXME: The Printer isn't initialized when this is called, so using _log would just show an unitialized logger error.

As it's a more common problem we should definitely fix it, but depending on the scope it may be better as a bug of its own.

I am going to change the _adb_path as a static variable on the ChromiumAndroidPort. We construct a Port() object once for the whole test-run, and then another one for each worker in Worker.start(). Currently, this would determine the adb path and potentially output the message N+1 times (where N is the amount of workers).

[Peter Beverloo]

Created attachment 176753 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 176753 [details]
Patch

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

LGTM.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:35
> +import sys

It's sad to add this, but I understand why you are.  We've been slowly removing all most os and sys imports to allow for easy mocking/testing.

[Dirk Pranke]

There used to be a reason why we couldn't initialize the printer object (and logging) until after setup_derived_options, but that stopped being true a while ago; I've just never gotten around to flipping the order of the two calls. Fix that and your sys.stderr can go away (as well as a few other hacks scattered here and there), but I'd do that in a separate patch.

[Peter Beverloo]

Comment on attachment 176753 [details]
Patch

Ok, I'll follow up on that. Putting this on the queue. I'll address being able to run webkit_unit_tests and TestWebKitAPI without adb in your path on the Chromium side.

[WebKit Review Bot]

Comment on attachment 176753 [details]
Patch

Clearing flags on attachment: 176753

Committed r136158: <http://trac.webkit.org/changeset/136158>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dirk Pranke]

Note that this change broke the python unit tests for chromium_android_unittest ... did you somehow not run them?

I landed a build fix in http://trac.webkit.org/changeset/136192 that updated the mock_run_command to deal with the version calls and the checking of potentially two different paths-to-adb. I think this is probably an acceptable fix, but would appreciate someone sanity-checking the change.

Also, looking at that made me a bit nervous about this patch, because it looks like we now can't create drivers or call default_child_processes() on a machine where the android toolchain isn't installed. I *think* this is probably okay as long as we mock out all of the stuff we might need for testing (e.g., for run-webkit-tests --platform mock-chromium-android), but this is a bit more fragile than I would like it to be. I haven't come up with a good alternative, though.

Lastly, the fact that the driver constructor might actually fail as a result of these things makes me a bit nervous, just because I am generally nervous when constructors can fail.

[Eric Seidel (no email)]

abarth reminded me this evening that the cq does not run the python unit tests. :(

This is due to the fact that we have no good way to parse out which tests are failing from the output, and w/o that we can't use our "land while the tree is red" logic that the cq currently depends on.  So if the python unit tests ever broke, the cq would grind to a halt until they were fixed.

[Dirk Pranke]

(In reply to comment #21)
> abarth reminded me this evening that the cq does not run the python unit tests. :(
> 
> This is due to the fact that we have no good way to parse out which tests are failing from the output, and w/o that we can't use our "land while the tree is red" logic that the cq currently depends on.  So if the python unit tests ever broke, the cq would grind to a halt until they were fixed.

Why do we not have a good way to parse out which tests are failing? The output is pretty straightforward ... is it just a matter of implementing something?

[Eric Seidel (no email)]

Yup.  Just a matter of writing some code.

http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/tool/bot/expectedfailures.py
http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py#L65
is relevant code.

[Peter Beverloo]

(In reply to comment #20)
> Note that this change broke the python unit tests for chromium_android_unittest ... did you somehow not run them?
> 
> I landed a build fix in http://trac.webkit.org/changeset/136192 that updated the mock_run_command to deal with the version calls and the checking of potentially two different paths-to-adb. I think this is probably an acceptable fix, but would appreciate someone sanity-checking the change.

Sorry! Thank you for the fix, it looks good to me as well.
 
> Also, looking at that made me a bit nervous about this patch, because it looks like we now can't create drivers or call default_child_processes() on a machine where the android toolchain isn't installed. I *think* this is probably okay as long as we mock out all of the stuff we might need for testing (e.g., for run-webkit-tests --platform mock-chromium-android), but this is a bit more fragile than I would like it to be. I haven't come up with a good alternative, though.

I don't know a right solution to this issue either. One way could be to override the default using a second way than just an environment variable, for example by having another method that's invoked just before we actually start sharding, but that's not ideal either.
 
> Lastly, the fact that the driver constructor might actually fail as a result of these things makes me a bit nervous, just because I am generally nervous when constructors can fail.

Agreed. I uploaded a patch to https://bugs.webkit.org/show_bug.cgi?id=103758 to address this.

[Adam Barth]

> Yup.  Just a matter of writing some code.

Presumably we can add a flag to test-webkitpy to dump structured data that's easy for the bot to parse.

[Dirk Pranke]

(In reply to comment #25)
> > Yup.  Just a matter of writing some code.
> 
> Presumably we can add a flag to test-webkitpy to dump structured data that's easy for the bot to parse.

The existing output (at least in --verbose mode) should be structured enough to be sufficient, but obviously we can change that if we need to. I'll put this task onto my queue :).