From 28b8a0b2bc87bd0d6e850ed924f9c8678728bc02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patrik=20H=C3=B6glund?= Date: Thu, 26 Mar 2020 20:30:50 +0100 Subject: [PATCH] Partial revert of flag simplification. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Unfortunately it turns out the Android test runner requires the isolated script flag to be in its current form, or it doesn't work. This means we have to keep translating the flag name. We can get rid of the isolated_script_test_output flag at least. Tbr: mbonadei@webrtc.org Bug: chromium:1051927 Change-Id: I4fdbff980e65332b757b1c95aa6587328411c0ed Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/171809 Reviewed-by: Patrik Höglund Commit-Queue: Patrik Höglund Cr-Commit-Position: refs/heads/master@{#30907} --- audio/test/low_bandwidth_audio_test.py | 2 +- tools_webrtc/flags_compatibility.py | 44 +++++++++++++++++++++ tools_webrtc/gtest-parallel-wrapper.py | 26 ++++++++++-- tools_webrtc/gtest_parallel_wrapper_test.py | 2 +- tools_webrtc/mb/mb.py | 4 ++ tools_webrtc/mb/mb_unittest.py | 2 + 6 files changed, 74 insertions(+), 6 deletions(-) create mode 100644 tools_webrtc/flags_compatibility.py diff --git a/audio/test/low_bandwidth_audio_test.py b/audio/test/low_bandwidth_audio_test.py index cf9047640a..c995cd6547 100755 --- a/audio/test/low_bandwidth_audio_test.py +++ b/audio/test/low_bandwidth_audio_test.py @@ -56,7 +56,7 @@ def _ParseArgs(): parser.add_argument('--adb-path', help='Path to adb binary.', default='adb') parser.add_argument('--num-retries', default='0', help='Number of times to retry the test on Android.') - parser.add_argument('--isolated_script_test_perf_output', default=None, + parser.add_argument('--isolated-script-test-perf-output', default=None, help='Path to store perf results in histogram proto format.') parser.add_argument('--extra-test-args', default=[], action='append', help='Extra args to path to the test binary.') diff --git a/tools_webrtc/flags_compatibility.py b/tools_webrtc/flags_compatibility.py new file mode 100644 index 0000000000..82655375dc --- /dev/null +++ b/tools_webrtc/flags_compatibility.py @@ -0,0 +1,44 @@ +#!/usr/bin/env python + +# Copyright (c) 2019 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 argparse +import logging +import subprocess +import sys + + +def main(): + parser = argparse.ArgumentParser() + parser.add_argument('--isolated-script-test-perf-output') + args, unrecognized_args = parser.parse_known_args() + + test_command = _ForcePythonInterpreter(unrecognized_args) + if args.isolated_script_test_perf_output: + test_command += ['--isolated_script_test_perf_output', + args.isolated_script_test_perf_output] + logging.info('Running %r', test_command) + + return subprocess.call(test_command) + + +def _ForcePythonInterpreter(cmd): + """Returns the fixed command line to call the right python executable.""" + out = cmd[:] + if out[0] == 'python': + out[0] = sys.executable + elif out[0].endswith('.py'): + out.insert(0, sys.executable) + return out + + +if __name__ == '__main__': + # pylint: disable=W0101 + logging.basicConfig(level=logging.INFO) + sys.exit(main()) diff --git a/tools_webrtc/gtest-parallel-wrapper.py b/tools_webrtc/gtest-parallel-wrapper.py index ec3b8b4160..fa37c5dd54 100755 --- a/tools_webrtc/gtest-parallel-wrapper.py +++ b/tools_webrtc/gtest-parallel-wrapper.py @@ -15,18 +15,25 @@ gtest-parallel, renaming options and translating environment variables into flags. Developers should execute gtest-parallel directly. In particular, this translates the GTEST_SHARD_INDEX and GTEST_TOTAL_SHARDS -environment variables to the --shard_index and --shard_count flags, and -interprets e.g. --workers=2x as 2 workers per core. +environment variables to the --shard_index and --shard_count flags +and interprets e.g. --workers=2x as 2 workers per core. Flags before '--' will be attempted to be understood as arguments to gtest-parallel. If gtest-parallel doesn't recognize the flag or the flag is after '--', the flag will be passed on to the test executable. +--isolated-script-test-perf-output is renamed to +--isolated_script_test_perf_output. The Android test runner needs the flag to +be in the former form, but our tests require the latter, so this is the only +place we can do it. + If the --store-test-artifacts flag is set, an --output_dir must be also specified. + The test artifacts will then be stored in a 'test_artifacts' subdirectory of the output dir, and will be compressed into a zip file once the test finishes executing. + This is useful when running the tests in swarming, since the output directory is not known beforehand. @@ -37,6 +44,7 @@ For example: --another_flag \ --output_dir=SOME_OUTPUT_DIR \ --store-test-artifacts + --isolated-script-test-perf-output=SOME_OTHER_DIR \ -- \ --foo=bar \ --baz @@ -47,11 +55,13 @@ Will be converted into: --shard_index 0 \ --shard_count 1 \ --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 @@ -146,8 +156,16 @@ def ParseArgs(argv=None): options, unrecognized_args = parser.parse_known_args(argv) - # Just pass on flags we don't recognize to the test binary. - executable_args = options.executable_args + unrecognized_args + args_to_pass = [] + for arg in unrecognized_args: + if arg.startswith('--isolated-script-test-perf-output'): + arg_split = arg.split('=') + assert len(arg_split) == 2, 'You must use the = syntax for this flag.' + args_to_pass.append('--isolated_script_test_perf_output=' + arg_split[1]) + else: + args_to_pass.append(arg) + + executable_args = options.executable_args + args_to_pass if options.store_test_artifacts: assert options.output_dir, ( diff --git a/tools_webrtc/gtest_parallel_wrapper_test.py b/tools_webrtc/gtest_parallel_wrapper_test.py index 60c496c635..26135e1abb 100755 --- a/tools_webrtc/gtest_parallel_wrapper_test.py +++ b/tools_webrtc/gtest_parallel_wrapper_test.py @@ -133,7 +133,7 @@ class GtestParallelWrapperTest(unittest.TestCase): result = gtest_parallel_wrapper.ParseArgs([ 'some_test', '--some_flag=some_value', '--another_flag', '--output_dir=' + output_dir, '--store-test-artifacts', - '--isolated_script_test_perf_output=SOME_OTHER_DIR', '--foo=bar', + '--isolated-script-test-perf-output=SOME_OTHER_DIR', '--foo=bar', '--baz' ]) expected_artifacts_dir = os.path.join(output_dir, 'test_artifacts') diff --git a/tools_webrtc/mb/mb.py b/tools_webrtc/mb/mb.py index f332a46aae..6287ca2366 100755 --- a/tools_webrtc/mb/mb.py +++ b/tools_webrtc/mb/mb.py @@ -855,6 +855,10 @@ class MetaBuildWrapper(object): '--logcat-output-file', '${ISOLATED_OUTDIR}/logcats', '--store-tombstones'] else: + if test_type == 'raw': + cmdline.append('../../tools_webrtc/flags_compatibility.py') + extra_files.append('../../tools_webrtc/flags_compatibility.py') + if isolate_map[target].get('use_webcam', False): cmdline.append('../../tools_webrtc/ensure_webcam_is_running.py') extra_files.append('../../tools_webrtc/ensure_webcam_is_running.py') diff --git a/tools_webrtc/mb/mb_unittest.py b/tools_webrtc/mb/mb_unittest.py index 7633dfed39..c1e477c104 100755 --- a/tools_webrtc/mb/mb_unittest.py +++ b/tools_webrtc/mb/mb_unittest.py @@ -453,9 +453,11 @@ class UnitTest(unittest.TestCase): self.assertEqual(files, [ '../../.vpython', '../../testing/test_env.py', + '../../tools_webrtc/flags_compatibility.py', 'base_unittests', ]) self.assertEqual(command, [ + '../../tools_webrtc/flags_compatibility.py', '../../testing/test_env.py', './base_unittests', '--asan=0',