Allow mixing gtest and non-gtest args in gtest-parallel-wrapper

No-Try: True
Bug: chromium:776681
Change-Id: I412a63e4ea897512b6c7012b9eb6ec5c3cf06314
Reviewed-on: https://webrtc-review.googlesource.com/78287
Commit-Queue: Oleh Prypin <oprypin@webrtc.org>
Reviewed-by: Patrik Höglund <phoglund@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#23369}

tools_webrtc/gtest-parallel-wrapper.py

@@ -18,12 +18,9 @@ In particular, this translates the GTEST_SHARD_INDEX and GTEST_TOTAL_SHARDS
 environment variables to the --shard_index and --shard_count flags, and renames
 the --isolated-script-test-output flag to --dump_json_test_results.
 
-All flags before '--' will be passed as arguments to gtest-parallel, and
+Flags before '--' will be attempted to be understood as arguments to
-(almost) all flags after '--' will be passed as arguments to the test
+gtest-parallel. If gtest-parallel doesn't recognize the flag or the flag is
-executable.
+after '--', the flag will be passed on to the test executable.
-The exception is that --isolated-script-test-output and
---isolated-script-test-chartson-output are expected to be after '--', so they
-are processed and removed from there.
 
 If the --store-test-artifacts flag is set, an --output_dir must be also
 specified.
@@ -38,25 +35,27 @@ For example:
   gtest-parallel-wrapper.py some_test \
       --some_flag=some_value \
       --another_flag \
-      --output_dir=SOME_OUTPUT_DIR
+      --output_dir=SOME_OUTPUT_DIR \
-      -- \
       --store-test-artifacts
       --isolated-script-test-output=SOME_DIR \
       --isolated-script-test-perf-output=SOME_OTHER_DIR \
+      -- \
       --foo=bar \
       --baz
 
 Will be converted into:
 
-  python gtest-parallel some_test \
+  python gtest-parallel \
-      --shard_count 1 \
       --shard_index 0 \
-      --some_flag=some_value \
+      --shard_count 1 \
-      --another_flag \
       --output_dir=SOME_OUTPUT_DIR \
       --dump_json_test_results=SOME_DIR \
+      some_test \
       -- \
       --test_artifacts_dir=SOME_OUTPUT_DIR/test_artifacts \
+      --some_flag=some_value \
+      --another_flag \
+      --isolated-script-test-perf-output=SOME_OTHER_DIR \
       --foo=bar \
       --baz
 
@@ -83,62 +82,80 @@ def _CatFiles(file_list, output_file):
       os.remove(filename)
 
 
-def _GetOutputDir(gtest_parallel_args):
+class ReconstructibleArgumentGroup(object):
-  parser = argparse.ArgumentParser()
+  """An argument group that can be converted back into a command line.
-  parser.add_argument('--output_dir', type=str, default=None)
+
-  options, _ = parser.parse_known_args(gtest_parallel_args)
+  This acts like ArgumentParser.add_argument_group, but names of arguments added
-  return options.output_dir
+  to it are also kept in a list, so that parsed options from
+  ArgumentParser.parse_args can be reconstructed back into a command line (list
+  of args) based on the list of wanted keys."""
+  def __init__(self, parser, *args, **kwargs):
+    self._group = parser.add_argument_group(*args, **kwargs)
+    self._keys = []
+
+  def AddArgument(self, *args, **kwargs):
+    arg = self._group.add_argument(*args, **kwargs)
+    self._keys.append(arg.dest)
+
+  def RemakeCommandLine(self, options):
+    result = []
+    for key in self._keys:
+      value = getattr(options, key)
+      if value is True:
+        result.append('--%s' % key)
+      elif value is not None:
+        result.append('--%s=%s' % (key, value))
+    return result
 
 
-def _ParseArgs():
+def ParseArgs(argv=None):
-  if '--' in sys.argv:
+  parser = argparse.ArgumentParser(argv)
-    argv_index = sys.argv.index('--')
-  else:
-    argv_index = len(sys.argv)
 
-  gtest_parallel_args = sys.argv[1:argv_index]
+  gtest_group = ReconstructibleArgumentGroup(parser,
-  executable_args = sys.argv[argv_index + 1:]
+                                             'Arguments to gtest-parallel')
-
+  # These options will be passed unchanged to gtest-parallel.
-  parser = argparse.ArgumentParser()
+  gtest_group.AddArgument('-d', '--output_dir')
-  parser.add_argument('--isolated-script-test-output', type=str, default=None)
+  gtest_group.AddArgument('-r', '--repeat')
-
+  gtest_group.AddArgument('--retry_failed')
-  # Needed when the test wants to store test artifacts, because it doesn't know
+  gtest_group.AddArgument('-w', '--workers')
-  # what will be the swarming output dir.
+  gtest_group.AddArgument('--gtest_color')
-  parser.add_argument('--store-test-artifacts', action='store_true',
+  gtest_group.AddArgument('--gtest_filter')
-                      default=False)
+  gtest_group.AddArgument('--gtest_also_run_disabled_tests',
-
+                          action='store_true', default=None)
-  # No-sandbox is a Chromium-specific flag, ignore it.
+  gtest_group.AddArgument('--timeout')
-  # TODO(oprypin): Remove (bugs.webrtc.org/8115)
-  parser.add_argument('--no-sandbox', action='store_true', default=False)
-
-  # We have to do this, since --isolated-script-test-output is passed as an
-  # argument to the executable by the swarming scripts, and we want to pass it
-  # to gtest-parallel instead.
-  options, executable_args = parser.parse_known_args(executable_args)
 
   # --isolated-script-test-output is used to upload results to the flakiness
   # dashboard. This translation is made because gtest-parallel expects the flag
   # to be called --dump_json_test_results instead.
-  if options.isolated_script_test_output:
+  gtest_group.AddArgument('--isolated-script-test-output',
-    gtest_parallel_args += [
+                          dest='dump_json_test_results')
-        '--dump_json_test_results',
-        options.isolated_script_test_output,
-    ]
 
-  output_dir = _GetOutputDir(gtest_parallel_args)
+  # Needed when the test wants to store test artifacts, because it doesn't know
-  test_artifacts_dir = None
+  # what will be the swarming output dir.
+  parser.add_argument('--store-test-artifacts', action='store_true')
+
+  # No-sandbox is a Chromium-specific flag, ignore it.
+  # TODO(oprypin): Remove (bugs.webrtc.org/8115)
+  parser.add_argument('--no-sandbox', action='store_true',
+                      help=argparse.SUPPRESS)
+
+  parser.add_argument('executable')
+  parser.add_argument('executable_args', nargs='*')
+
+  options, unrecognized_args = parser.parse_known_args(argv)
+
+  executable_args = options.executable_args + unrecognized_args
 
   if options.store_test_artifacts:
-    assert output_dir, (
+    assert options.output_dir, (
         '--output_dir must be specified for storing test artifacts.')
-    test_artifacts_dir = os.path.join(output_dir, 'test_artifacts')
+    test_artifacts_dir = os.path.join(options.output_dir, 'test_artifacts')
-    if not os.path.isdir(test_artifacts_dir):
-      os.makedirs(test_artifacts_dir)
 
-    executable_args += [
+    executable_args.insert(0, '--test_artifacts_dir=%s' % test_artifacts_dir)
-        '--test_artifacts_dir',
+  else:
-        test_artifacts_dir,
+    test_artifacts_dir = None
-    ]
+
+  gtest_parallel_args = gtest_group.RemakeCommandLine(options)
 
   # GTEST_SHARD_INDEX and GTEST_TOTAL_SHARDS must be removed from the
   # environment. Otherwise it will be picked up by the binary, causing a bug
@@ -147,14 +164,15 @@ def _ParseArgs():
   gtest_shard_index = test_env.pop('GTEST_SHARD_INDEX', '0')
   gtest_total_shards = test_env.pop('GTEST_TOTAL_SHARDS', '1')
 
-  gtest_parallel_args += [
+  gtest_parallel_args.insert(0, '--shard_index=%s' % gtest_shard_index)
-      '--shard_count',
+  gtest_parallel_args.insert(1, '--shard_count=%s' % gtest_total_shards)
-      gtest_total_shards,
-      '--shard_index',
-      gtest_shard_index,
-  ] + ['--'] + executable_args
 
-  return Args(gtest_parallel_args, test_env, output_dir, test_artifacts_dir)
+  gtest_parallel_args.append(options.executable)
+  if executable_args:
+    gtest_parallel_args += ['--'] + executable_args
+
+  return Args(gtest_parallel_args, test_env, options.output_dir,
+              test_artifacts_dir)
 
 
 def main():
@@ -162,35 +180,36 @@ def main():
   gtest_parallel_path = os.path.join(
       webrtc_root, 'third_party', 'gtest-parallel', 'gtest-parallel')
 
-  args = _ParseArgs()
+  gtest_parallel_args, test_env, output_dir, test_artifacts_dir = ParseArgs()
 
   command = [
       sys.executable,
       gtest_parallel_path,
-  ] + args.gtest_parallel_args
+  ] + gtest_parallel_args
 
-  if args.output_dir and not os.path.isdir(args.output_dir):
+  if output_dir and not os.path.isdir(output_dir):
-    os.makedirs(args.output_dir)
+    os.makedirs(output_dir)
+  if test_artifacts_dir and not os.path.isdir(test_artifacts_dir):
+    os.makedirs(test_artifacts_dir)
 
   print 'gtest-parallel-wrapper: Executing command %s' % ' '.join(command)
   sys.stdout.flush()
 
-  exit_code = subprocess.call(command, env=args.test_env, cwd=os.getcwd())
+  exit_code = subprocess.call(command, env=test_env, cwd=os.getcwd())
 
-  if args.output_dir:
+  if output_dir:
     for test_status in 'passed', 'failed', 'interrupted':
-      logs_dir = os.path.join(args.output_dir, 'gtest-parallel-logs',
+      logs_dir = os.path.join(output_dir, 'gtest-parallel-logs', test_status)
-                              test_status)
       if not os.path.isdir(logs_dir):
         continue
       logs = [os.path.join(logs_dir, log) for log in os.listdir(logs_dir)]
-      log_file = os.path.join(args.output_dir, '%s-tests.log' % test_status)
+      log_file = os.path.join(output_dir, '%s-tests.log' % test_status)
       _CatFiles(logs, log_file)
       os.rmdir(logs_dir)
 
-  if args.test_artifacts_dir:
+  if test_artifacts_dir:
-    shutil.make_archive(args.test_artifacts_dir, 'zip', args.test_artifacts_dir)
+    shutil.make_archive(test_artifacts_dir, 'zip', test_artifacts_dir)
-    shutil.rmtree(args.test_artifacts_dir)
+    shutil.rmtree(test_artifacts_dir)
 
   return exit_code
 

tools_webrtc/gtest_parallel_wrapper_test.py

@@ -0,0 +1,112 @@
+#!/usr/bin/env python
+
+# Copyright (c) 2018 The WebRTC project authors. All Rights Reserved.
+#
+# Use of this source code is governed by a BSD-style license
+# that can be found in the LICENSE file in the root of the source
+# tree. An additional intellectual property rights grant can be found
+# in the file PATENTS.  All contributing project authors may
+# be found in the AUTHORS file in the root of the source tree.
+
+import unittest
+
+script = __import__('gtest-parallel-wrapper')  # pylint: disable=invalid-name
+
+
+class GtestParallelWrapperTest(unittest.TestCase):
+  @classmethod
+  def _Expected(cls, gtest_parallel_args):
+    return ['--shard_index=0', '--shard_count=1'] + gtest_parallel_args
+
+  def testOverwrite(self):
+    result = script.ParseArgs(['--timeout=123', 'exec', '--timeout', '124'])
+    expected = self._Expected(['--timeout=124', 'exec'])
+    self.assertEqual(result.gtest_parallel_args, expected)
+
+  def testMixing(self):
+    result = script.ParseArgs(
+        ['--timeout=123', '--param1', 'exec', '--param2', '--timeout', '124'])
+    expected = self._Expected(
+        ['--timeout=124', 'exec', '--', '--param1', '--param2'])
+    self.assertEqual(result.gtest_parallel_args, expected)
+
+  def testMixingPositional(self):
+    result = script.ParseArgs(['--timeout=123', 'exec', '--foo1', 'bar1',
+                               '--timeout', '124', '--foo2', 'bar2'])
+    expected = self._Expected(['--timeout=124', 'exec', '--', '--foo1', 'bar1',
+                               '--foo2', 'bar2'])
+    self.assertEqual(result.gtest_parallel_args, expected)
+
+  def testDoubleDash1(self):
+    result = script.ParseArgs(
+        ['--timeout', '123', 'exec', '--', '--timeout', '124'])
+    expected = self._Expected(
+        ['--timeout=123', 'exec', '--', '--timeout', '124'])
+    self.assertEqual(result.gtest_parallel_args, expected)
+
+  def testDoubleDash2(self):
+    result = script.ParseArgs(['--timeout=123', '--', 'exec', '--timeout=124'])
+    expected = self._Expected(['--timeout=123', 'exec', '--', '--timeout=124'])
+    self.assertEqual(result.gtest_parallel_args, expected)
+
+  def testArtifacts(self):
+    result = script.ParseArgs(['exec', '--store-test-artifacts',
+                               '--output_dir', '/tmp/foo'])
+    expected = self._Expected(['--output_dir=/tmp/foo', 'exec', '--',
+                               '--test_artifacts_dir=/tmp/foo/test_artifacts'])
+    self.assertEqual(result.gtest_parallel_args, expected)
+    self.assertEqual(result.output_dir, '/tmp/foo')
+    self.assertRegexpMatches(result.test_artifacts_dir,
+                             '/tmp/foo.test_artifacts')
+
+  def testNoDirsSpecified(self):
+    result = script.ParseArgs(['exec'])
+    self.assertEqual(result.output_dir, None)
+    self.assertEqual(result.test_artifacts_dir, None)
+
+  def testOutputDirSpecified(self):
+    result = script.ParseArgs(['exec', '--output_dir', '/tmp/foo'])
+    self.assertEqual(result.output_dir, '/tmp/foo')
+    self.assertEqual(result.test_artifacts_dir, None)
+
+  def testJsonTestResults(self):
+    result = script.ParseArgs(['--isolated-script-test-output', '/tmp/foo',
+                               'exec'])
+    expected = self._Expected(['--dump_json_test_results=/tmp/foo', 'exec'])
+    self.assertEqual(result.gtest_parallel_args, expected)
+
+  def testShortArg(self):
+    result = script.ParseArgs(['-d', '/tmp/foo', 'exec'])
+    expected = self._Expected(['--output_dir=/tmp/foo', 'exec'])
+    self.assertEqual(result.gtest_parallel_args, expected)
+    self.assertEqual(result.output_dir, '/tmp/foo')
+
+  def testBoolArg(self):
+    result = script.ParseArgs(['--gtest_also_run_disabled_tests', 'exec'])
+    expected = self._Expected(['--gtest_also_run_disabled_tests', 'exec'])
+    self.assertEqual(result.gtest_parallel_args, expected)
+
+  def testNoArgs(self):
+    result = script.ParseArgs(['exec'])
+    expected = self._Expected(['exec'])
+    self.assertEqual(result.gtest_parallel_args, expected)
+
+  def testDocExample(self):
+    result = script.ParseArgs([
+        'some_test', '--some_flag=some_value', '--another_flag',
+        '--output_dir=SOME_OUTPUT_DIR', '--store-test-artifacts',
+        '--isolated-script-test-output=SOME_DIR',
+        '--isolated-script-test-perf-output=SOME_OTHER_DIR',
+        '--foo=bar', '--baz'])
+    expected = self._Expected([
+        '--output_dir=SOME_OUTPUT_DIR', '--dump_json_test_results=SOME_DIR',
+        'some_test', '--',
+        '--test_artifacts_dir=SOME_OUTPUT_DIR/test_artifacts',
+        '--some_flag=some_value', '--another_flag',
+        '--isolated-script-test-perf-output=SOME_OTHER_DIR',
+        '--foo=bar', '--baz'])
+    self.assertEqual(result.gtest_parallel_args, expected)
+
+
+if __name__ == '__main__':
+  unittest.main()

tools_webrtc/mb/mb.py

@@ -899,8 +899,6 @@ class MetaBuildWrapper(object):
       executable = executable_prefix + target + executable_suffix
 
       cmdline.append(executable)
-      if test_type != 'raw':
-        cmdline.append('--')
 
       asan = 'is_asan=true' in vals['gn_args']
       lsan = 'is_lsan=true' in vals['gn_args']

tools_webrtc/mb/mb_unittest.py

@@ -390,7 +390,6 @@ class UnitTest(unittest.TestCase):
         '--retry_failed=3',
         '--workers=1',
         './base_unittests',
-        '--',
         '--asan=0',
         '--lsan=0',
         '--msan=0',
@@ -511,7 +510,6 @@ class UnitTest(unittest.TestCase):
         '--retry_failed=3',
         '--workers=1',
         './base_unittests',
-        '--',
         '--asan=0',
         '--lsan=0',
         '--msan=0',
@@ -561,7 +559,6 @@ class UnitTest(unittest.TestCase):
         '--timeout=900',
         '--retry_failed=3',
         './base_unittests',
-        '--',
         '--asan=0',
         '--lsan=0',
         '--msan=0',
@@ -612,7 +609,6 @@ class UnitTest(unittest.TestCase):
         '--timeout=900',
         '--retry_failed=3',
         r'.\unittests.exe',
-        '--',
         '--asan=0',
         '--lsan=0',
         '--msan=0',
@@ -659,7 +655,6 @@ class UnitTest(unittest.TestCase):
         '--timeout=900',
         '--retry_failed=3',
         './base_unittests',
-        '--',
         '--asan=0',
         '--lsan=0',
         '--msan=0',
@@ -711,7 +706,6 @@ class UnitTest(unittest.TestCase):
         '..',
         '--test',
         './base_unittests',
-        '--',
         '--asan=0',
         '--lsan=0',
         '--msan=0',
@@ -763,7 +757,6 @@ class UnitTest(unittest.TestCase):
         '--timeout=900',
         '--retry_failed=3',
         './base_unittests',
-        '--',
         '--asan=0',
         '--lsan=0',
         '--msan=0',