[Android] Move GTest options to test_options_parser.py.

[Android] Move GTest options to test_options_parser.py.

Also move some packages to utils.

BUG=167331
TEST=run_tests.py, device_stats_monitor.py

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175397

[klundberg, 2013-01-03 21:04:07]

Thanks, Craig!

It is great to see you already refactoring the test scripts!

I mainly have small nits so giving you lgtm, especially as I'm on/off today and
tomorrow.

https://codereview.chromium.org/11759012/diff/1/build/android/pylib/utils/tes...
File build/android/pylib/utils/test_options_parser.py (right):

https://codereview.chromium.org/11759012/diff/1/build/android/pylib/utils/tes...
build/android/pylib/utils/test_options_parser.py:7: from pylib import constants
Does it make sense to move constants too?

I don't like the idea of having utils depend on pylib and pylib depend on utils.

https://codereview.chromium.org/11759012/diff/1/build/android/pylib/utils/tes...
build/android/pylib/utils/test_options_parser.py:90: '(use -s help to list
them)')
nit: period at end of sentence.

https://codereview.chromium.org/11759012/diff/1/build/android/pylib/utils/tes...
build/android/pylib/utils/test_options_parser.py:95: help='Target device the
test suite to run ')
Seems like something is missing in this sentence. Maybe it should be:
Target device for the test suite to run on.

https://codereview.chromium.org/11759012/diff/1/build/android/pylib/utils/tes...
build/android/pylib/utils/test_options_parser.py:97: help='gtest filter')
nit: period at end of sentence.

https://codereview.chromium.org/11759012/diff/1/build/android/pylib/utils/tes...
build/android/pylib/utils/test_options_parser.py:99: help='Additional arguments
to pass to the test')
nit: period at end of sentence.

https://codereview.chromium.org/11759012/diff/1/build/android/pylib/utils/tes...
build/android/pylib/utils/test_options_parser.py:101: help='file name of log
dump, which will be put in '
file/File

https://codereview.chromium.org/11759012/diff/1/build/android/pylib/utils/tes...
build/android/pylib/utils/test_options_parser.py:106: help='Run tests in a new
instance of emulator')
nit: period at end of sentence.

https://codereview.chromium.org/11759012/diff/1/build/android/pylib/utils/tes...
build/android/pylib/utils/test_options_parser.py:113: help='Use Xvfb around
tests (ignored if not Linux)')
nit: period at end of sentence.

https://codereview.chromium.org/11759012/diff/1/build/android/pylib/utils/tes...
build/android/pylib/utils/test_options_parser.py:126: help='Repeat count on test
timeout')
nit: period at end of sentence.

[craigdh, 2013-01-03 21:13:12]

https://codereview.chromium.org/11759012/diff/1/build/android/pylib/utils/tes...
File build/android/pylib/utils/test_options_parser.py (right):

https://codereview.chromium.org/11759012/diff/1/build/android/pylib/utils/tes...
build/android/pylib/utils/test_options_parser.py:7: from pylib import constants
On 2013/01/03 21:04:07, klundberg wrote:
> Does it make sense to move constants too?
> 
> I don't like the idea of having utils depend on pylib and pylib depend on
utils.

I don't like it either, but constants.py is used in a lot of places, so I think
that should be its own CL if we decide to move it.

@Frank, any input? Do people outside build/android depend on constants.py?
utils/ seems like it should only have private utilities not used outside of
build/android/.

[frankf, 2013-01-03 21:17:23]

https://codereview.chromium.org/11759012/diff/1/build/android/device_stats_mo...
File build/android/device_stats_monitor.py (right):

https://codereview.chromium.org/11759012/diff/1/build/android/device_stats_mo...
build/android/device_stats_monitor.py:1: #!/usr/bin/env python
Can you check whether this script is used on the builldbot
(build/android/buildbot). If not, invoke it manually to verify this change
doesn't break it.

https://codereview.chromium.org/11759012/diff/1/build/android/pylib/utils/tes...
File build/android/pylib/utils/test_options_parser.py (right):

https://codereview.chromium.org/11759012/diff/1/build/android/pylib/utils/tes...
build/android/pylib/utils/test_options_parser.py:84: def
AddGTestOptions(option_parser):
So the reason the other options are here, is that they're used by more than one
test runner. This is only used by run_tests.py. But I see the value of moving it
here for consistancy, and also to be able to see what options can be factored
out into AddTestRunnerOptions.

https://codereview.chromium.org/11759012/diff/1/build/android/pylib/utils/tes...
build/android/pylib/utils/test_options_parser.py:107:
option_parser.add_option('-n', '--emulator_count',
For reference: at some point (not in this CL), it'd be good to make all options
params to use '-' not '_'.

[frankf, 2013-01-03 21:23:27]

On 2013/01/03 21:13:12, craigdh wrote:
>
https://codereview.chromium.org/11759012/diff/1/build/android/pylib/utils/tes...
> File build/android/pylib/utils/test_options_parser.py (right):
> 
>
https://codereview.chromium.org/11759012/diff/1/build/android/pylib/utils/tes...
> build/android/pylib/utils/test_options_parser.py:7: from pylib import
constants
> On 2013/01/03 21:04:07, klundberg wrote:
> > Does it make sense to move constants too?
> > 
> > I don't like the idea of having utils depend on pylib and pylib depend on
> utils.
> 
> I don't like it either, but constants.py is used in a lot of places, so I
think
> that should be its own CL if we decide to move it.
> 
> @Frank, any input? Do people outside build/android depend on constants.py?

Aside from the downtream build scripts (a lot of dependency here), the only
place I'm aware that depends on pylib is tools/telemetry. These are the imports
there:

  from pylib import android_commands # pylint: disable=F0401
  from pylib import cmd_helper # pylint: disable=F0401
  from pylib import forwarder # pylint: disable=F0401
  from pylib import ports # pylint: disable=F0401
  from pylib import valgrind_tools # pylint: disable=F0401

> utils/ seems like it should only have private utilities not used outside of
> build/android/.

[craigdh, 2013-01-03 21:44:05]

https://codereview.chromium.org/11759012/diff/1/build/android/device_stats_mo...
File build/android/device_stats_monitor.py (right):

https://codereview.chromium.org/11759012/diff/1/build/android/device_stats_mo...
build/android/device_stats_monitor.py:1: #!/usr/bin/env python
On 2013/01/03 21:17:23, frankf wrote:
> Can you check whether this script is used on the builldbot
> (build/android/buildbot). If not, invoke it manually to verify this change
> doesn't break it.

Already did. Adding it to the TEST= line.

https://codereview.chromium.org/11759012/diff/1/build/android/pylib/utils/tes...
File build/android/pylib/utils/test_options_parser.py (right):

https://codereview.chromium.org/11759012/diff/1/build/android/pylib/utils/tes...
build/android/pylib/utils/test_options_parser.py:84: def
AddGTestOptions(option_parser):
On 2013/01/03 21:17:23, frankf wrote:
> So the reason the other options are here, is that they're used by more than
one
> test runner. This is only used by run_tests.py. But I see the value of moving
it
> here for consistancy, and also to be able to see what options can be factored
> out into AddTestRunnerOptions.

Improving consistency between the various test runners in general should make
refactoring out common code much easier.

https://codereview.chromium.org/11759012/diff/1/build/android/pylib/utils/tes...
build/android/pylib/utils/test_options_parser.py:90: '(use -s help to list
them)')
On 2013/01/03 21:04:07, klundberg wrote:
> nit: period at end of sentence.

Done.

https://codereview.chromium.org/11759012/diff/1/build/android/pylib/utils/tes...
build/android/pylib/utils/test_options_parser.py:95: help='Target device the
test suite to run ')
On 2013/01/03 21:04:07, klundberg wrote:
> Seems like something is missing in this sentence. Maybe it should be:
> Target device for the test suite to run on. 
> 
> 
> 

Done.

https://codereview.chromium.org/11759012/diff/1/build/android/pylib/utils/tes...
build/android/pylib/utils/test_options_parser.py:97: help='gtest filter')
On 2013/01/03 21:04:07, klundberg wrote:
> nit: period at end of sentence.

Done.

https://codereview.chromium.org/11759012/diff/1/build/android/pylib/utils/tes...
build/android/pylib/utils/test_options_parser.py:97: help='gtest filter')
On 2013/01/03 21:04:07, klundberg wrote:
> nit: period at end of sentence.

Done.

https://codereview.chromium.org/11759012/diff/1/build/android/pylib/utils/tes...
build/android/pylib/utils/test_options_parser.py:99: help='Additional arguments
to pass to the test')
On 2013/01/03 21:04:07, klundberg wrote:
> nit: period at end of sentence.

Done.

https://codereview.chromium.org/11759012/diff/1/build/android/pylib/utils/tes...
build/android/pylib/utils/test_options_parser.py:101: help='file name of log
dump, which will be put in '
On 2013/01/03 21:04:07, klundberg wrote:
> file/File

Done.

https://codereview.chromium.org/11759012/diff/1/build/android/pylib/utils/tes...
build/android/pylib/utils/test_options_parser.py:106: help='Run tests in a new
instance of emulator')
On 2013/01/03 21:04:07, klundberg wrote:
> nit: period at end of sentence.

Done.

https://codereview.chromium.org/11759012/diff/1/build/android/pylib/utils/tes...
build/android/pylib/utils/test_options_parser.py:107:
option_parser.add_option('-n', '--emulator_count',
On 2013/01/03 21:17:23, frankf wrote:
> For reference: at some point (not in this CL), it'd be good to make all
options
> params to use '-' not '_'.

Added a TODO.

https://codereview.chromium.org/11759012/diff/1/build/android/pylib/utils/tes...
build/android/pylib/utils/test_options_parser.py:113: help='Use Xvfb around
tests (ignored if not Linux)')
On 2013/01/03 21:04:07, klundberg wrote:
> nit: period at end of sentence.

Done.

https://codereview.chromium.org/11759012/diff/1/build/android/pylib/utils/tes...
build/android/pylib/utils/test_options_parser.py:126: help='Repeat count on test
timeout')
On 2013/01/03 21:04:07, klundberg wrote:
> nit: period at end of sentence.

Done.

[frankf, 2013-01-07 18:33:11]

lgtm

[commit-bot: I haz the power, 2013-01-07 22:06:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/craigdh@chromium.org/11759012/10005

[commit-bot: I haz the power, 2013-01-07 22:18:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/craigdh@chromium.org/11759012/10005