Modify the Android test running scripts so they can be used from WebKit

Modify the Android test running scripts so they can be used from WebKit

In order to avoid duplicating most of the run-tests-on-devices logic, make
sure that the Android scripts are able to be usable from a WebKit checkout.
Two flags have been added for this: --webkit, which is necessary as a file's
path on the host and device are not per se equal anymore, and --out-directory,
accepting a path as it's not always in the Chromium source directory.

BUG=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=167999

[Peter Beverloo, 2012-11-14 16:26:49]

+bulach, as we very briefly discussed this last week.

[bulach, 2012-11-15 01:06:39]

thanks peter! I have a few suggestions below, let me know your thoughts:

http://codereview.chromium.org/11312239/diff/1/build/android/pylib/base_test_...
File build/android/pylib/base_test_runner.py (right):

http://codereview.chromium.org/11312239/diff/1/build/android/pylib/base_test_...
build/android/pylib/base_test_runner.py:109: if not isinstance(p, list):
uh? 103 doesn't say this is allowed, who's passing list of lists? why is this
needed?

http://codereview.chromium.org/11312239/diff/1/build/android/pylib/cmd_helper.py
File build/android/pylib/cmd_helper.py (right):

http://codereview.chromium.org/11312239/diff/1/build/android/pylib/cmd_helper...
build/android/pylib/cmd_helper.py:71: return
os.environ.get('ANDROID_TEST_OUT_DIRECTORY', constants.CHROME_DIR)
sorry, when I mentioned to use an environment variable, I didn't mean internally
in the scripts, I was thinking that the external caller could set this... :)

if it's being set as a command line option, it's better to just have a global
state here...
maybe best to encapsulate in something like:

class OutDirectory(object):
  _out_directory = constants.CHROME_DIR
  @staticmethod
  def Set(out_directory):
    OutDirectory._out_directory = out_directory
  @staticmethod
  def get():
    return OutDirectory._out_directory


...
wdyt?

http://codereview.chromium.org/11312239/diff/1/build/android/pylib/constants.py
File build/android/pylib/constants.py (right):

http://codereview.chromium.org/11312239/diff/1/build/android/pylib/constants....
build/android/pylib/constants.py:8: 
revert this file?

http://codereview.chromium.org/11312239/diff/1/build/android/pylib/single_tes...
File build/android/pylib/single_test_runner.py (right):

http://codereview.chromium.org/11312239/diff/1/build/android/pylib/single_tes...
build/android/pylib/single_test_runner.py:95: """Returns the relative directory
to the WebKit checkout, relative to
needs to be one liner.
"""Returns the relative directory to WebKit checkout from $CHROME_SRC"""

http://codereview.chromium.org/11312239/diff/1/build/android/pylib/single_tes...
build/android/pylib/single_test_runner.py:242:
'third_party/Source/WebKit/chromium/tests/data']
ahn, I see... yeah, it seems entirely the wrong level to do this down there..
let's try to make it higher up at the test scripts?

http://codereview.chromium.org/11312239/diff/1/build/android/run_tests.py
File build/android/run_tests.py (right):

http://codereview.chromium.org/11312239/diff/1/build/android/run_tests.py#new...
build/android/run_tests.py:402: help='Path to the out/ directory, ambiguous of
the '
s/ambiguous/irrespective/

http://codereview.chromium.org/11312239/diff/1/build/android/run_tests.py#new...
build/android/run_tests.py:456: if options.out_directory:
see above: I was thinking that the caller coud set this up when running rather
than setting the env internally here...
if we're passing the command line anyways, no need to mess up with the
environment..

[Peter Beverloo, 2012-11-15 12:11:28]

Thanks for the review, Marcus!

https://codereview.chromium.org/11312239/diff/1/build/android/pylib/base_test...
File build/android/pylib/base_test_runner.py (right):

https://codereview.chromium.org/11312239/diff/1/build/android/pylib/base_test...
build/android/pylib/base_test_runner.py:109: if not isinstance(p, list):
On 2012/11/15 01:06:39, bulach wrote:
> uh? 103 doesn't say this is allowed, who's passing list of lists? why is this
> needed?

See single_test_runner.py:241; removed.

https://codereview.chromium.org/11312239/diff/1/build/android/pylib/cmd_helpe...
File build/android/pylib/cmd_helper.py (right):

https://codereview.chromium.org/11312239/diff/1/build/android/pylib/cmd_helpe...
build/android/pylib/cmd_helper.py:71: return
os.environ.get('ANDROID_TEST_OUT_DIRECTORY', constants.CHROME_DIR)
On 2012/11/15 01:06:39, bulach wrote:
> sorry, when I mentioned to use an environment variable, I didn't mean
internally
> in the scripts, I was thinking that the external caller could set this... :)
> 
> if it's being set as a command line option, it's better to just have a global
> state here...
> maybe best to encapsulate in something like:
> 
> class OutDirectory(object):
>   _out_directory = constants.CHROME_DIR
>   @staticmethod
>   def Set(out_directory):
>     OutDirectory._out_directory = out_directory
>   @staticmethod
>   def get():
>     return OutDirectory._out_directory
> 
> 
> ...
> wdyt?

Seems good to me, thank you! Done. In general we're trying to reduce the number
of environment variables used, so the argument seems like a better option. I've
s/Set/set/ in your code per the style guide.

https://codereview.chromium.org/11312239/diff/1/build/android/pylib/constants.py
File build/android/pylib/constants.py (right):

https://codereview.chromium.org/11312239/diff/1/build/android/pylib/constants...
build/android/pylib/constants.py:8: 
On 2012/11/15 01:06:39, bulach wrote:
> revert this file?

Oops, done.

https://codereview.chromium.org/11312239/diff/1/build/android/pylib/single_te...
File build/android/pylib/single_test_runner.py (right):

https://codereview.chromium.org/11312239/diff/1/build/android/pylib/single_te...
build/android/pylib/single_test_runner.py:95: """Returns the relative directory
to the WebKit checkout, relative to
On 2012/11/15 01:06:39, bulach wrote:
> needs to be one liner.
> """Returns the relative directory to WebKit checkout from $CHROME_SRC"""

Done.

https://codereview.chromium.org/11312239/diff/1/build/android/pylib/single_te...
build/android/pylib/single_test_runner.py:242:
'third_party/Source/WebKit/chromium/tests/data']
On 2012/11/15 01:06:39, bulach wrote:
> ahn, I see... yeah, it seems entirely the wrong level to do this down there..
> let's try to make it higher up at the test scripts?

Done, I've now put this in a method under StripAndCopyFiles. Another option
would be to subclass SingleTestRunner specifically for WebKitUnitTests..

https://codereview.chromium.org/11312239/diff/1/build/android/run_tests.py
File build/android/run_tests.py (right):

https://codereview.chromium.org/11312239/diff/1/build/android/run_tests.py#ne...
build/android/run_tests.py:402: help='Path to the out/ directory, ambiguous of
the '
On 2012/11/15 01:06:39, bulach wrote:
> s/ambiguous/irrespective/

Done.

https://codereview.chromium.org/11312239/diff/1/build/android/run_tests.py#ne...
build/android/run_tests.py:456: if options.out_directory:
On 2012/11/15 01:06:39, bulach wrote:
> see above: I was thinking that the caller coud set this up when running rather
> than setting the env internally here...
> if we're passing the command line anyways, no need to mess up with the
> environment..

As replied earlier on: amended.

https://codereview.chromium.org/11312239/diff/8001/build/android/pylib/single...
File build/android/pylib/single_test_runner.py (right):

https://codereview.chromium.org/11312239/diff/8001/build/android/pylib/single...
build/android/pylib/single_test_runner.py:279:
'%s/Source/WebKit/chromium/tests/data' % webkit_src,
q: would having a chain of os.path.join() arguments be preferred here?

[bulach, 2012-11-15 17:46:26]

lgtm, thanks peter! a: below:

https://codereview.chromium.org/11312239/diff/8001/build/android/pylib/single...
File build/android/pylib/single_test_runner.py (right):

https://codereview.chromium.org/11312239/diff/8001/build/android/pylib/single...
build/android/pylib/single_test_runner.py:279:
'%s/Source/WebKit/chromium/tests/data' % webkit_src,
On 2012/11/15 12:11:29, Peter Beverloo wrote:
> q: would having a chain of os.path.join() arguments be preferred here?

given 280, I think it's more readable.. just to be clear, not a chain of
os.path.join, just something like:
os.path.join(webkit_src, 'Source/WebKit/chromium/tests/data')

[Peter Beverloo, 2012-11-15 18:02:53]

Thank you for the reviews, Marcus!

https://codereview.chromium.org/11312239/diff/8001/build/android/pylib/single...
File build/android/pylib/single_test_runner.py (right):

https://codereview.chromium.org/11312239/diff/8001/build/android/pylib/single...
build/android/pylib/single_test_runner.py:279:
'%s/Source/WebKit/chromium/tests/data' % webkit_src,
On 2012/11/15 17:46:26, bulach wrote:
> On 2012/11/15 12:11:29, Peter Beverloo wrote:
> > q: would having a chain of os.path.join() arguments be preferred here?
> 
> given 280, I think it's more readable.. just to be clear, not a chain of
> os.path.join, just something like:
> os.path.join(webkit_src, 'Source/WebKit/chromium/tests/data')

Done.

[commit-bot: I haz the power, 2012-11-15 18:04:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/peter@chromium.org/11312239/11002

[commit-bot: I haz the power, 2012-11-15 19:57:17]

Change committed as 167999