Don't duplicate gtest-parallel flags in gtest-parallel-wrappers.

Also, move the --timeout=900 flag from the recipe to the mb.py script. That way it is passed as an arg to gtest-parallel and not to the test executable.

BUG=webrtc:7568
NOTRY=True

Review-Url: https://codereview.webrtc.org/2862803002
Cr-Commit-Position: refs/heads/master@{#18015}

tools-webrtc/gtest-parallel-wrapper.py

@@ -18,24 +18,35 @@ 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.
 
-Note that the flags unprocessed by this script will passed as arguments to the
+All flags before '--' will be passed as arguments to gtest-parallel, and
-test executable, i.e.
+(almost) all flags after '--' will be passed as arguments 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.
+For example:
 
   gtest-parallel-wrapper.py some_test \
-      --isolated-script-test-output=some_dir \
+      --some_flag=some_value \
-      --unprocessed_arg_1
+      --another_flag \
       -- \
-      --unprocessed_arg_2
+      --isolated-script-test-output=some_dir \
+      --isolated-script-test-chartjson-output=some_other_dir \
+      --foo=bar \
+      --baz
 
-will be converted into
+Will be converted into:
 
   python gtest-parallel some_test \
       --shard_count 1 \
       --shard_index 0 \
-      --dump_json_test_results some_dir \
+      --some_flag=some_value \
+      --another_flag \
+      --dump_json_test_results=some_dir \
       -- \
-      --unprocessed_arg_1
+      --foo=bar
-      --unprocessed_arg_2
+      --baz
+
 """
 
 import argparse
@@ -52,11 +63,14 @@ def CatFiles(file_list, output_file):
       os.remove(filename)
 
 
-def main():
+def get_args_and_env():
-  # Ignore '--'. Options unprocessed by this script will be passed to the test
+  if '--' not in sys.argv:
-  # as arguments.
+    return sys.argv, os.environ
-  if '--' in sys.argv:
+
-    del sys.argv[sys.argv.index('--')]
+  argv_index = sys.argv.index('--')
+
+  gtest_parallel_args = sys.argv[1:argv_index]
+  executable_args = sys.argv[argv_index + 1:]
 
   parser = argparse.ArgumentParser()
   parser.add_argument('--isolated-script-test-output', type=str, default=None)
@@ -67,11 +81,19 @@ def main():
   parser.add_argument('--isolated-script-test-chartjson-output', type=str,
                       default=None)
 
-  # TODO(ehmaldonado): Figure out a way to avoid duplicating the flags in
+  # We have to do this, since --isolated-script-test-output is passed as an
-  # gtest-parallel.
+  # argument to the executable by the swarming scripts, and we want to pass it
-  parser.add_argument('--gtest_color', type=str, default='auto')
+  # to gtest-parallel instead.
-  parser.add_argument('--output_dir', type=str, default=None)
+  options, executable_args = parser.parse_known_args(executable_args)
-  parser.add_argument('--timeout', type=int, default=None)
+
+  # --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_parallel_args += [
+        '--dump_json_test_results',
+        options.isolated_script_test_output,
+    ]
 
   # 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
@@ -80,62 +102,48 @@ def main():
   gtest_shard_index = test_env.pop('GTEST_SHARD_INDEX', '0')
   gtest_total_shards = test_env.pop('GTEST_TOTAL_SHARDS', '1')
 
-  webrtc_root = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
+  gtest_parallel_args += [
-  gtest_parallel_path = os.path.join(
-      webrtc_root, 'third_party', 'gtest-parallel', 'gtest-parallel')
-
-  options, unprocessed = parser.parse_known_args()
-  test_executable = unprocessed[0]
-  test_arguments = unprocessed[1:]
-
-  gtest_args = [
-      test_executable,
       '--shard_count',
       gtest_total_shards,
       '--shard_index',
       gtest_shard_index,
-      '--gtest_color',
+  ] + ['--'] + executable_args
-      options.gtest_color,
-  ]
 
-  # --isolated-script-test-output is used to upload results to the flakiness
+  return gtest_parallel_args, test_env
-  # 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_args += [
-        '--dump_json_test_results',
-        options.isolated_script_test_output,
-    ]
 
-  if options.output_dir:
-    gtest_args += [
-        '--output_dir',
-        options.output_dir,
-    ]
 
-  if options.timeout:
+def get_output_dir(gtest_parallel_args):
-    gtest_args += [
+  parser = argparse.ArgumentParser()
-        '--timeout',
+  parser.add_argument('--output_dir', type=str, default=None)
-        str(options.timeout),
+  options, _ = parser.parse_known_args(gtest_parallel_args)
-    ]
+  return options.output_dir
+
+
+def main():
+  webrtc_root = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
+  gtest_parallel_path = os.path.join(
+      webrtc_root, 'third_party', 'gtest-parallel', 'gtest-parallel')
+
+  gtest_parallel_args, test_env = get_args_and_env()
 
   command = [
       sys.executable,
       gtest_parallel_path,
-  ] + gtest_args + ['--'] + test_arguments
+  ] + gtest_parallel_args
 
   print 'gtest-parallel-wrapper: Executing command %s' % ' '.join(command)
   sys.stdout.flush()
 
   exit_code = subprocess.call(command, env=test_env, cwd=os.getcwd())
 
-  if options.output_dir:
+  output_dir = get_output_dir(gtest_parallel_args)
+  if output_dir:
     for test_status in 'passed', 'failed', 'interrupted':
-      logs_dir = os.path.join(options.output_dir, test_status)
+      logs_dir = os.path.join(output_dir, 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(options.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)
 

tools-webrtc/mb/mb.py

@@ -1096,8 +1096,12 @@ class MetaBuildWrapper(object):
         output_dir = '${ISOLATED_OUTDIR}' + sep + 'test_logs'
         gtest_parallel_wrapper = [
             '../../tools-webrtc/gtest-parallel-wrapper.py',
-            '--gtest_color=no',
             '--output_dir=%s' % output_dir,
+            '--gtest_color=no',
+            # We tell gtest-parallel to interrupt the test after 900 seconds,
+            # so it can exit cleanly and report results, instead of being
+            # interrupted by swarming and not reporting anything.
+            '--timeout=900',
         ]
 
       asan = 'is_asan=true' in vals['gn_args']

tools-webrtc/mb/mb_unittest.py

@@ -448,8 +448,9 @@ class UnitTest(unittest.TestCase):
     self.assertEqual(command, [
         '../../testing/test_env.py',
         '../../tools-webrtc/gtest-parallel-wrapper.py',
-        '--gtest_color=no',
         '--output_dir=${ISOLATED_OUTDIR}/test_logs',
+        '--gtest_color=no',
+        '--timeout=900',
         './base_unittests',
         '--workers=1',
         '--',
@@ -494,8 +495,9 @@ class UnitTest(unittest.TestCase):
     self.assertEqual(command, [
         '../../testing/xvfb.py',
         '../../tools-webrtc/gtest-parallel-wrapper.py',
-        '--gtest_color=no',
         '--output_dir=${ISOLATED_OUTDIR}/test_logs',
+        '--gtest_color=no',
+        '--timeout=900',
         './base_unittests',
         '--',
         '--asan=0',
@@ -540,8 +542,9 @@ class UnitTest(unittest.TestCase):
     self.assertEqual(command, [
         '../../testing/test_env.py',
         '../../tools-webrtc/gtest-parallel-wrapper.py',
-        '--gtest_color=no',
         '--output_dir=${ISOLATED_OUTDIR}\\test_logs',
+        '--gtest_color=no',
+        '--timeout=900',
         r'.\unittests.exe',
         '--',
         '--asan=0',
@@ -582,8 +585,9 @@ class UnitTest(unittest.TestCase):
     self.assertEqual(command, [
         '../../testing/test_env.py',
         '../../tools-webrtc/gtest-parallel-wrapper.py',
-        '--gtest_color=no',
         '--output_dir=${ISOLATED_OUTDIR}/test_logs',
+        '--gtest_color=no',
+        '--timeout=900',
         './base_unittests',
         '--',
         '--asan=0',