[Android] Reland of b/a/test_runner.py optparse to argparse switch.

[Android] Reland of b/a/test_runner.py optparse to argparse switch.

BUG=428729
NOTRY=true

Committed: https://crrev.com/15cdcd58d86b7b24f102ed091bcbea3f1d5f302a
Cr-Commit-Position: refs/heads/master@{#306653}

[jbudorick, 2014-12-03 17:22:12]

jbudorick@chromium.org changed reviewers:
+ klundberg@chromium.org, perezju@chromium.org

[jbudorick, 2014-12-03 17:22:13]

After the original CL landed, both blink bots and downstream bots (and maybe
others?) were failing because the test suites they were trying to run weren't in
gtest_suites.

[perezju, 2014-12-03 17:36:21]

lgtm, but have a comment

https://codereview.chromium.org/754463007/diff/20001/build/android/test_runne...
File build/android/test_runner.py (right):

https://codereview.chromium.org/754463007/diff/20001/build/android/test_runne...
build/android/test_runner.py:148: '%s' % ', '.join('"%s"' % s for s in
gtest_suites)))
Not sure if we should list them here or, as in the original, state that '-s
help' will list them all (I'm not a fan of the "--option help" thing, but if
it's there I guess at least it should be documented).

[jbudorick, 2014-12-03 17:40:22]

https://codereview.chromium.org/754463007/diff/20001/build/android/test_runne...
File build/android/test_runner.py (right):

https://codereview.chromium.org/754463007/diff/20001/build/android/test_runne...
build/android/test_runner.py:148: '%s' % ', '.join('"%s"' % s for s in
gtest_suites)))
On 2014/12/03 17:36:21, perezju wrote:
> Not sure if we should list them here or, as in the original, state that '-s
> help' will list them all (I'm not a fan of the "--option help" thing, but if
> it's there I guess at least it should be documented).

I don't like "-s help" very much. A user who wants to list the suites has to run
(at least) twice: once with ./test_runner.py gtest --help, and then again with
./test_runner.py gtest -s help

[klundberg, 2014-12-03 17:50:49]

lgtm

https://codereview.chromium.org/754463007/diff/20001/build/android/test_runne...
File build/android/test_runner.py (right):

https://codereview.chromium.org/754463007/diff/20001/build/android/test_runne...
build/android/test_runner.py:148: '%s' % ', '.join('"%s"' % s for s in
gtest_suites)))
On 2014/12/03 17:40:22, jbudorick wrote:
> On 2014/12/03 17:36:21, perezju wrote:
> > Not sure if we should list them here or, as in the original, state that '-s
> > help' will list them all (I'm not a fan of the "--option help" thing, but if
> > it's there I guess at least it should be documented).
> 
> I don't like "-s help" very much. A user who wants to list the suites has to
run
> (at least) twice: once with ./test_runner.py gtest --help, and then again with
> ./test_runner.py gtest -s help

My preference would be to avoid having the user run ./test_runner.py gtest
twice.

[perezju, 2014-12-03 17:53:00]

On 2014/12/03 17:40:22, jbudorick wrote:
>
https://codereview.chromium.org/754463007/diff/20001/build/android/test_runne...
> File build/android/test_runner.py (right):
> 
>
https://codereview.chromium.org/754463007/diff/20001/build/android/test_runne...
> build/android/test_runner.py:148: '%s' % ', '.join('"%s"' % s for s in
> gtest_suites)))
> On 2014/12/03 17:36:21, perezju wrote:
> > Not sure if we should list them here or, as in the original, state that '-s
> > help' will list them all (I'm not a fan of the "--option help" thing, but if
> > it's there I guess at least it should be documented).
> 
> I don't like "-s help" very much. A user who wants to list the suites has to
run
> (at least) twice: once with ./test_runner.py gtest --help, and then again with
> ./test_runner.py gtest -s help

Agreed.

[jbudorick, 2014-12-03 17:53:49]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2014-12-03 17:54:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/754463007/20001

[commit-bot: I haz the power, 2014-12-03 18:39:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-03 18:39:58]

Failed to apply patch for build/android/test_runner.py:
While running git apply --index -3 -p1;
  error: patch failed: build/android/test_runner.py:6
  Falling back to three-way merge...
  Applied patch to 'build/android/test_runner.py' with conflicts.
  U build/android/test_runner.py

Patch:       build/android/test_runner.py
Index: build/android/test_runner.py
diff --git a/build/android/test_runner.py b/build/android/test_runner.py
index
f38b76e6af8a64d9a5da93c06b84cf2862f454a9..7fb267a614bf3ebdc2253181f23fc4018f093e83
100755
--- a/build/android/test_runner.py
+++ b/build/android/test_runner.py
@@ -6,9 +6,9 @@
 
 """Runs all types of tests from one unified interface."""
 
+import argparse
 import collections
 import logging
-import optparse
 import os
 import shutil
 import signal
@@ -44,189 +44,163 @@ from pylib.results import report_results
 from pylib.uiautomator import setup as uiautomator_setup
 from pylib.uiautomator import test_options as uiautomator_test_options
 from pylib.utils import apk_helper
-from pylib.utils import command_option_parser
 from pylib.utils import reraiser_thread
 from pylib.utils import run_tests_helper
 
 
-def AddCommonOptions(option_parser):
-  """Adds all common options to |option_parser|."""
+def AddCommonOptions(parser):
+  """Adds all common options to |parser|."""
+
+  group = parser.add_argument_group('Common Options')
 
-  group = optparse.OptionGroup(option_parser, 'Common Options')
   default_build_type = os.environ.get('BUILDTYPE', 'Debug')
-  group.add_option('--debug', action='store_const', const='Debug',
-                   dest='build_type', default=default_build_type,
-                   help=('If set, run test suites under out/Debug. '
-                         'Default is env var BUILDTYPE or Debug.'))
-  group.add_option('--release', action='store_const',
-                   const='Release', dest='build_type',
-                   help=('If set, run test suites under out/Release.'
-                         ' Default is env var BUILDTYPE or Debug.'))
-  group.add_option('--build-directory', dest='build_directory',
-                   help=('Path to the directory in which build files are'
-                         ' located (should not include build type)'))
-  group.add_option('--output-directory', dest='output_directory',
-                   help=('Path to the directory in which build files are'
-                         ' located (must include build type). This will take'
-                         ' precedence over --debug, --release and'
-                         ' --build-directory'))
-  group.add_option('--num_retries', dest='num_retries', type='int',
-                   default=2,
-                   help=('Number of retries for a test before '
-                         'giving up.'))
-  group.add_option('-v',
-                   '--verbose',
-                   dest='verbose_count',
-                   default=0,
-                   action='count',
-                   help='Verbose level (multiple times for more)')
-  group.add_option('--flakiness-dashboard-server',
-                   dest='flakiness_dashboard_server',
-                   help=('Address of the server that is hosting the '
-                         'Chrome for Android flakiness dashboard.'))
-  group.add_option('--enable-platform-mode', action='store_true',
-                   help=('Run the test scripts in platform mode, which '
-                         'conceptually separates the test runner from the '
-                         '"device" (local or remote, real or emulated) on '
-                         'which the tests are running. [experimental]'))
-  group.add_option('-e', '--environment', default='local',
-                   help=('Test environment to run in. Must be one of: %s' %
-                         ', '.join(constants.VALID_ENVIRONMENTS)))
-  group.add_option('--adb-path',
-                   help=('Specify the absolute path of the adb binary that '
-                         'should be used.'))
-  group.add_option('--json-results-file', dest='json_results_file',
-                   help='If set, will dump results in JSON format '
-                        'to specified file.')
-  option_parser.add_option_group(group)
-
-
-def ProcessCommonOptions(options, error_func):
+
+  debug_or_release_group = group.add_mutually_exclusive_group()
+  debug_or_release_group.add_argument(
+      '--debug', action='store_const', const='Debug', dest='build_type',
+      default=default_build_type,
+      help=('If set, run test suites under out/Debug. '
+            'Default is env var BUILDTYPE or Debug.'))
+  debug_or_release_group.add_argument(
+      '--release', action='store_const', const='Release', dest='build_type',
+      help=('If set, run test suites under out/Release. '
+            'Default is env var BUILDTYPE or Debug.'))
+
+  group.add_argument('--build-directory', dest='build_directory',
+                     help=('Path to the directory in which build files are'
+                           ' located (should not include build type)'))
+  group.add_argument('--output-directory', dest='output_directory',
+                     help=('Path to the directory in which build files are'
+                           ' located (must include build type). This will take'
+                           ' precedence over --debug, --release and'
+                           ' --build-directory'))
+  group.add_argument('--num_retries', dest='num_retries', type=int, default=2,
+                     help=('Number of retries for a test before '
+                           'giving up (default: %(default)s).'))
+  group.add_argument('-v',
+                     '--verbose',
+                     dest='verbose_count',
+                     default=0,
+                     action='count',
+                     help='Verbose level (multiple times for more)')
+  group.add_argument('--flakiness-dashboard-server',
+                     dest='flakiness_dashboard_server',
+                     help=('Address of the server that is hosting the '
+                           'Chrome for Android flakiness dashboard.'))
+  group.add_argument('--enable-platform-mode', action='store_true',
+                     help=('Run the test scripts in platform mode, which '
+                           'conceptually separates the test runner from the '
+                           '"device" (local or remote, real or emulated) on '
+                           'which the tests are running. [experimental]'))
+  group.add_argument('-e', '--environment', default='local',
+                     choices=constants.VALID_ENVIRONMENTS,
+                     help='Test environment to run in (default: %(default)s).')
+  group.add_argument('--adb-path',
+                     help=('Specify the absolute path of the adb binary that '
+                           'should be used.'))
+  group.add_argument('--json-results-file', dest='json_results_file',
+                     help='If set, will dump results in JSON form '
+                          'to specified file.')
+
+
+def ProcessCommonOptions(args):
   """Processes and handles all common options."""
-  run_tests_helper.SetLogLevel(options.verbose_count)
-  constants.SetBuildType(options.build_type)
-  if options.build_directory:
-    constants.SetBuildDirectory(options.build_directory)
-  if options.output_directory:
-    constants.SetOutputDirectort(options.output_directory)
-  if options.adb_path:
-    constants.SetAdbPath(options.adb_path)
+  run_tests_helper.SetLogLevel(args.verbose_count)
+  constants.SetBuildType(args.build_type)
+  if args.build_directory:
+    constants.SetBuildDirectory(args.build_directory)
+  if args.output_directory:
+    constants.SetOutputDirectort(args.output_directory)
+  if args.adb_path:
+    constants.SetAdbPath(args.adb_path)
   # Some things such as Forwarder require ADB to be in the environment path.
   adb_dir = os.path.dirname(constants.GetAdbPath())
   if adb_dir and adb_dir not in os.environ['PATH'].split(os.pathsep):
     os.environ['PATH'] = adb_dir + os.pathsep + os.environ['PATH']
-  if options.environment not in constants.VALID_ENVIRONMENTS:
-    error_func('--environment must be one of: %s' %
-               ', '.join(constants.VALID_ENVIRONMENTS))
-
-
-def AddDeviceOptions(option_parser):
-  group = optparse.OptionGroup(option_parser, 'Device Options')
-  group.add_option('-c', dest='cleanup_test_files',
-                   help='Cleanup test files on the device after run',
-                   action='store_true')
-  group.add_option('--tool',
-                   dest='tool',
-                   help=('Run the test under a tool '
-                         '(use --tool help to list them)'))
-  group.add_option('-d', '--device', dest='test_device',
-                   help=('Target device for the test suite '
-                         'to run on.'))
-  option_parser.add_option_group(group)
-
-
-def AddGTestOptions(option_parser):
-  """Adds gtest options to |option_parser|."""
-
-  option_parser.usage = '%prog gtest [options]'
-  option_parser.commands_dict = {}
-  option_parser.example = '%prog gtest -s base_unittests'
-
-  # TODO(gkanwar): Make this option required
-  option_parser.add_option('-s', '--suite', dest='suite_name',
-                           help=('Executable name of the test suite to run '
-                                 '(use -s help to list them).'))
-  option_parser.add_option('-f', '--gtest_filter', '--gtest-filter',
-                           dest='test_filter',
-                           help='googletest-style filter string.')
-  option_parser.add_option('--gtest_also_run_disabled_tests',
-                           '--gtest-also-run-disabled-tests',
-                           dest='run_disabled', action='store_true',
-                           help='Also run disabled tests if applicable.')
-  option_parser.add_option('-a', '--test-arguments', dest='test_arguments',
-                         …
(message too large)

[commit-bot: I haz the power, 2014-12-03 18:40:20]

The CQ bit was unchecked by commit-bot@chromium.org

[jbudorick, 2014-12-03 18:57:02]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2014-12-03 18:58:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/754463007/40001

[jbudorick, 2014-12-03 19:22:28]

The CQ bit was unchecked by jbudorick@chromium.org

[jbudorick, 2014-12-03 19:35:25]

I'll be submitting this as NOTRY to:
 - fix downstream bot
 - fix blink bot
 - keep CQ from accepting CLs with bugs.

[jbudorick, 2014-12-03 19:56:38]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2014-12-03 19:57:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/754463007/60001

[commit-bot: I haz the power, 2014-12-03 19:58:59]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-12-03 19:59:47]

Patchset 4 (id:??) landed as
https://crrev.com/15cdcd58d86b7b24f102ed091bcbea3f1d5f302a
Cr-Commit-Position: refs/heads/master@{#306653}