Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15942016:
Creates a new test running script test_runner.py (Closed)
|
Created:
7 years, 6 months ago by gkanwar Modified:
7 years, 5 months ago CC:
chromium-reviews, craigdh+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org, klundberg+watch_chromium.org, frankf+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionCreates a new test running script test_runner.py
This new script serves as a unified entry point for all tests. The
existing scripts are now just wrappers around the new script. Old
commands should thus still work, but you can now make use of the
new script to run various types of tests.
There are a few TODOs left:
* Add options to run Monkey tests.
Miscellaneous notes:
* --python_test_root is now a required flag when Python host-driven
tests are being run.
BUG=248351
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=210035
Patch Set 1 #
Total comments: 33
Patch Set 2 : Fixes based on code review #
Total comments: 48
Patch Set 3 : Several more fixes #
Total comments: 42
Patch Set 4 : A few more updates from code review #
Total comments: 26
Patch Set 5 : Deprecates some options, updates some help text, and some miscellaneous things #
Total comments: 7
Patch Set 6 : Rebases the patch #
Total comments: 1
Patch Set 7 : Converts options to command-style format #
Total comments: 30
Patch Set 8 : Fixes help command and example text #
Total comments: 36
Patch Set 9 : Some name fixes and moving gtest_filter to gtest options #
Total comments: 4
Patch Set 10 : Changes host-driven logging, and other minor fixes #Patch Set 11 : Renames run_all_tests.py to test_runner.py #
Total comments: 4
Patch Set 12 : Reverts flakiness server change, uses os.path.join #Messages
Total messages: 39 (0 generated)
This is a good start Tej. https://codereview.chromium.org/15942016/diff/1/build/android/run_all_tests.py File build/android/run_all_tests.py (right): https://codereview.chromium.org/15942016/diff/1/build/android/run_all_tests.p... build/android/run_all_tests.py:7: Example: ./run_all_tests.py --gtest --release Don't include --release Specify the suite name https://codereview.chromium.org/15942016/diff/1/build/android/run_all_tests.p... build/android/run_all_tests.py:10: ./run_all_tests.py --browser --test-apk=ContentShellTest --release browser -> content_browsertests https://codereview.chromium.org/15942016/diff/1/build/android/run_all_tests.p... build/android/run_all_tests.py:11: 3. Instrumentation tests (--instrumentation) Seperate instrumentation into 1. Instrumentation 2. UIAutomator Although UIAutamator uses instrumentation test report convention, we don't refer to them as instrumentation. https://codereview.chromium.org/15942016/diff/1/build/android/run_all_tests.p... build/android/run_all_tests.py:193: def AddUnitTestOptions(option_parser): I would change this back to GTest. We care about the framework not whether it's unit or content_browsertests.
https://codereview.chromium.org/15942016/diff/1/build/android/run_browser_tes... File build/android/run_browser_tests.py (right): https://codereview.chromium.org/15942016/diff/1/build/android/run_browser_tes... build/android/run_browser_tests.py:13: args = ["./run_all_tests.py", "--browser"] + sys.argv[1:] Print a warning that this script is being deprecated and give the new command line to use.
More comments. https://codereview.chromium.org/15942016/diff/1/build/android/pylib/gtest/tes... File build/android/pylib/gtest/test_package_apk.py (right): https://codereview.chromium.org/15942016/diff/1/build/android/pylib/gtest/tes... build/android/pylib/gtest/test_package_apk.py:109: self._CreateTestRunnerScript('--test_filter=%s %s' % (test_filter, gtest_filter is passed to the actual framework here, so you can't just rename it. https://codereview.chromium.org/15942016/diff/1/build/android/pylib/gtest/tes... File build/android/pylib/gtest/test_package_executable.py (right): https://codereview.chromium.org/15942016/diff/1/build/android/pylib/gtest/tes... build/android/pylib/gtest/test_package_executable.py:105: '%s %s/%s --test_filter=%s %s\n' Same here. https://codereview.chromium.org/15942016/diff/1/build/android/pylib/host_driv... File build/android/pylib/host_driven/run_python_tests.py (right): https://codereview.chromium.org/15942016/diff/1/build/android/pylib/host_driv... build/android/pylib/host_driven/run_python_tests.py:68: attached_devices = [options.test_device] Why this name change? https://codereview.chromium.org/15942016/diff/1/build/android/pylib/host_driv... build/android/pylib/host_driven/run_python_tests.py:90: test_pkg = instrumentation_test_package.TestPackage(options.test_apk_path, Hmm. Actually let's not touch host_driven tests as they're due for major refactoring: crbug.com/176323. https://codereview.chromium.org/15942016/diff/1/build/android/pylib/uiautomat... File build/android/pylib/uiautomator/dispatch.py (right): https://codereview.chromium.org/15942016/diff/1/build/android/pylib/uiautomat... build/android/pylib/uiautomator/dispatch.py:58: def Dispatch(options): So currently the concept of dispatch is pretty ill-defined and there are a lot of duplication. Currently the following is done: 1. Use type-specific options to create a test runner 2. Get attached devices 3. Call shard.ShardAndRunTests() 4. Report results Steps 2-4 are common to all test types and can be factored out. Let's do this in a future CL to keep this small. https://codereview.chromium.org/15942016/diff/1/build/android/run_all_tests.py File build/android/run_all_tests.py (right): https://codereview.chromium.org/15942016/diff/1/build/android/run_all_tests.p... build/android/run_all_tests.py:51: def AddTestTypeOption(option_parser): You need to remove utils/test_option_... https://codereview.chromium.org/15942016/diff/1/build/android/run_all_tests.p... build/android/run_all_tests.py:392: def AddAllOptions(option_parser): Just inline this. https://codereview.chromium.org/15942016/diff/1/build/android/run_all_tests.p... build/android/run_all_tests.py:404: def RunBrowserTests(options, option_parser): Also inline these. https://codereview.chromium.org/15942016/diff/1/build/android/run_all_tests.p... build/android/run_all_tests.py:414: ValidateInstrumentationOptions(options, option_parser) Why is this duplicated? https://codereview.chromium.org/15942016/diff/1/build/android/run_all_tests.p... build/android/run_all_tests.py:447: failed_tests_count = RunTests(options, option_parser) As it stands RunTests doesn't return anything. https://codereview.chromium.org/15942016/diff/1/build/android/run_all_tests.p... build/android/run_all_tests.py:457: return failed_tests_count Returning the total number of failed tests here doesn't make sense (crbug.com/170477). We should fix this in a future CL. https://codereview.chromium.org/15942016/diff/1/build/android/run_tests.py File build/android/run_tests.py (right): https://codereview.chromium.org/15942016/diff/1/build/android/run_tests.py#ne... build/android/run_tests.py:9: 1. Copy over test binary to /data/local on device. This doc is pretty out-dated. Let's get rid of it.
I uploaded several fixes based on the code review / try server failures (now the try servers pass) https://codereview.chromium.org/15942016/diff/1/build/android/pylib/gtest/tes... File build/android/pylib/gtest/test_package_apk.py (right): https://codereview.chromium.org/15942016/diff/1/build/android/pylib/gtest/tes... build/android/pylib/gtest/test_package_apk.py:109: self._CreateTestRunnerScript('--test_filter=%s %s' % (test_filter, On 2013/06/11 02:50:15, frankf wrote: > gtest_filter is passed to the actual framework here, so you can't just rename > it. Done. https://codereview.chromium.org/15942016/diff/1/build/android/pylib/gtest/tes... File build/android/pylib/gtest/test_package_executable.py (right): https://codereview.chromium.org/15942016/diff/1/build/android/pylib/gtest/tes... build/android/pylib/gtest/test_package_executable.py:105: '%s %s/%s --test_filter=%s %s\n' On 2013/06/11 02:50:15, frankf wrote: > Same here. Done. https://codereview.chromium.org/15942016/diff/1/build/android/pylib/host_driv... File build/android/pylib/host_driven/run_python_tests.py (right): https://codereview.chromium.org/15942016/diff/1/build/android/pylib/host_driv... build/android/pylib/host_driven/run_python_tests.py:68: attached_devices = [options.test_device] There were previously two settings: --device for instrumentation tests, which set options.device, and --device for GTests which set options.test_device. I moved this setting to common options in the new script and changed the code here to options.test_device, so both type of tests now use options.test_device. On 2013/06/11 02:50:15, frankf wrote: > Why this name change? https://codereview.chromium.org/15942016/diff/1/build/android/pylib/host_driv... build/android/pylib/host_driven/run_python_tests.py:90: test_pkg = instrumentation_test_package.TestPackage(options.test_apk_path, On 2013/06/11 02:50:15, frankf wrote: > Hmm. Actually let's not touch host_driven tests as they're due for major > refactoring: crbug.com/176323. Done. https://codereview.chromium.org/15942016/diff/1/build/android/run_all_tests.py File build/android/run_all_tests.py (right): https://codereview.chromium.org/15942016/diff/1/build/android/run_all_tests.p... build/android/run_all_tests.py:7: Example: ./run_all_tests.py --gtest --release On 2013/06/11 01:51:47, frankf wrote: > Don't include --release > Specify the suite name Done. https://codereview.chromium.org/15942016/diff/1/build/android/run_all_tests.p... build/android/run_all_tests.py:10: ./run_all_tests.py --browser --test-apk=ContentShellTest --release On 2013/06/11 01:51:47, frankf wrote: > browser -> content_browsertests Done. https://codereview.chromium.org/15942016/diff/1/build/android/run_all_tests.p... build/android/run_all_tests.py:11: 3. Instrumentation tests (--instrumentation) On 2013/06/11 01:51:47, frankf wrote: > Seperate instrumentation into 1. Instrumentation 2. UIAutomator > > Although UIAutamator uses instrumentation test report convention, we don't refer > to them as instrumentation. Done. https://codereview.chromium.org/15942016/diff/1/build/android/run_all_tests.p... build/android/run_all_tests.py:51: def AddTestTypeOption(option_parser): On 2013/06/11 02:50:15, frankf wrote: > You need to remove utils/test_option_... Done. https://codereview.chromium.org/15942016/diff/1/build/android/run_all_tests.p... build/android/run_all_tests.py:193: def AddUnitTestOptions(option_parser): This set of options is common between both Gtests and Content Browser tests. I renamed it to AddGTestContentBrowserTestOptions. This is a bit wordy, but I think conveys the meaning of the function more clearly. On 2013/06/11 01:51:47, frankf wrote: > I would change this back to GTest. We care about the framework not whether it's > unit or content_browsertests. https://codereview.chromium.org/15942016/diff/1/build/android/run_all_tests.p... build/android/run_all_tests.py:392: def AddAllOptions(option_parser): On 2013/06/11 02:50:15, frankf wrote: > Just inline this. Done. https://codereview.chromium.org/15942016/diff/1/build/android/run_all_tests.p... build/android/run_all_tests.py:404: def RunBrowserTests(options, option_parser): On 2013/06/11 02:50:15, frankf wrote: > Also inline these. Done. https://codereview.chromium.org/15942016/diff/1/build/android/run_all_tests.p... build/android/run_all_tests.py:414: ValidateInstrumentationOptions(options, option_parser) On 2013/06/11 02:50:15, frankf wrote: > Why is this duplicated? Done. https://codereview.chromium.org/15942016/diff/1/build/android/run_all_tests.p... build/android/run_all_tests.py:447: failed_tests_count = RunTests(options, option_parser) On 2013/06/11 02:50:15, frankf wrote: > As it stands RunTests doesn't return anything. Fixed to return the total number of failed tests. https://codereview.chromium.org/15942016/diff/1/build/android/run_all_tests.p... build/android/run_all_tests.py:457: return failed_tests_count On 2013/06/11 02:50:15, frankf wrote: > Returning the total number of failed tests here doesn't make sense > (crbug.com/170477). We should fix this in a future CL. Sounds good. For now, I fixed the RunTests function to correctly return the number of failed tests. https://codereview.chromium.org/15942016/diff/1/build/android/run_browser_tes... File build/android/run_browser_tests.py (right): https://codereview.chromium.org/15942016/diff/1/build/android/run_browser_tes... build/android/run_browser_tests.py:13: args = ["./run_all_tests.py", "--browser"] + sys.argv[1:] On 2013/06/11 01:53:21, frankf wrote: > Print a warning that this script is being deprecated and give the new command > line to use. Done. https://codereview.chromium.org/15942016/diff/1/build/android/run_tests.py File build/android/run_tests.py (right): https://codereview.chromium.org/15942016/diff/1/build/android/run_tests.py#ne... build/android/run_tests.py:9: 1. Copy over test binary to /data/local on device. On 2013/06/11 02:50:15, frankf wrote: > This doc is pretty out-dated. Let's get rid of it. Done.
https://codereview.chromium.org/15942016/diff/27001/build/android/pylib/instr... File build/android/pylib/instrumentation/dispatch.py (right): https://codereview.chromium.org/15942016/diff/27001/build/android/pylib/instr... build/android/pylib/instrumentation/dispatch.py:19: def DispatchCore(options): Just inline this. https://codereview.chromium.org/15942016/diff/27001/build/android/pylib/uiaut... File build/android/pylib/uiautomator/dispatch.py (right): https://codereview.chromium.org/15942016/diff/27001/build/android/pylib/uiaut... build/android/pylib/uiautomator/dispatch.py:19: def DispatchCore(options): Same with this. https://codereview.chromium.org/15942016/diff/27001/build/android/pylib/utils... File build/android/pylib/utils/test_options_parser.py (left): https://codereview.chromium.org/15942016/diff/27001/build/android/pylib/utils... build/android/pylib/utils/test_options_parser.py:57: def AddTestRunnerOptions(option_parser, default_timeout=60): Some downstream scripts depend on this. Maybe leave this here with a TODO about the dependency to be removed in a future CL. Also make sure there are no other dependency on any of the code being change in this CL. https://codereview.chromium.org/15942016/diff/27001/build/android/run_all_tes... File build/android/run_all_tests.py (right): https://codereview.chromium.org/15942016/diff/27001/build/android/run_all_tes... build/android/run_all_tests.py:11: Example: ./run_all_tests.py --gtests -s android_webview_unittests Let's make test type a positional arg instead of an option. https://codereview.chromium.org/15942016/diff/27001/build/android/run_all_tes... build/android/run_all_tests.py:13: Example: ./adb_install_apk.py --apk=ContentShell.apk --release As said before, remove --release. https://codereview.chromium.org/15942016/diff/27001/build/android/run_all_tes... build/android/run_all_tests.py:14: ./run_all_tests.py --contentbrowsertests --test-apk=ContentShellTest content_browsertests https://codereview.chromium.org/15942016/diff/27001/build/android/run_all_tes... build/android/run_all_tests.py:19: 3a. Python host-driven Let's remove instructions for host-driven all together, it will change in the future and is not a common use case upstream. https://codereview.chromium.org/15942016/diff/27001/build/android/run_all_tes... build/android/run_all_tests.py:40: * Allow running many test types non-exclusively from the same command. Running multiple types of tests in not an important use case. https://codereview.chromium.org/15942016/diff/27001/build/android/run_all_tes... build/android/run_all_tests.py:211: def AddGTestContentBrowserTestOptions(option_parser): As said before, this should be AddGTestOptions, there's no content_browser-specific options. https://codereview.chromium.org/15942016/diff/27001/build/android/run_all_tes... build/android/run_all_tests.py:236: def AddInstrumentationUIAutomatorOptions(option_parser): Let's call these 'java tests' here and in the help description https://codereview.chromium.org/15942016/diff/27001/build/android/run_all_tes... build/android/run_all_tests.py:257: default=False, help='Run only the Python tests.') Python -> host-driven https://codereview.chromium.org/15942016/diff/27001/build/android/run_all_tes... build/android/run_all_tests.py:268: help='Root of the python-driven tests.') Same here. https://codereview.chromium.org/15942016/diff/27001/build/android/run_all_tes... build/android/run_all_tests.py:326: # determined based on the --test-apk or --test-jar flag. Why don't you validate based on the test type arg? https://codereview.chromium.org/15942016/diff/27001/build/android/run_all_tes... build/android/run_all_tests.py:447: ValidateTestTypeOption(options, option_parser) Move this to line 443 https://codereview.chromium.org/15942016/diff/27001/build/android/run_browser... File build/android/run_browser_tests.py (right): https://codereview.chromium.org/15942016/diff/27001/build/android/run_browser... build/android/run_browser_tests.py:16: os.path.expandvars("$CHROME_SRC/build/android/run_all_tests.py"), Use __file__ to get the path to the script. https://codereview.chromium.org/15942016/diff/27001/build/android/run_browser... build/android/run_browser_tests.py:18: logging.warning("This script is deprecated. " Make this more prominant by adding : '*' * 80 blah '*' * 80
https://codereview.chromium.org/15942016/diff/27001/build/android/run_all_tes... File build/android/run_all_tests.py (right): https://codereview.chromium.org/15942016/diff/27001/build/android/run_all_tes... build/android/run_all_tests.py:7: """Runs all types of tests from one unified interface. I don't like the name run_all_tests.py. This name is usually used for wrappers around running multiple tests. This is a launcher for all types, but under no condition will it run all tests. run_tests.py would be best but is already taken for the old gtest launcher. Maybe just tests.py. @Frank WDYT? https://codereview.chromium.org/15942016/diff/27001/build/android/run_all_tes... build/android/run_all_tests.py:62: """Decorates OptionParser with test type options in an OptionGroup.""" Decorate has a specific meaning in Python which doesn't match this. https://codereview.chromium.org/15942016/diff/27001/build/android/run_all_tes... build/android/run_all_tests.py:76: help='If set, run contentbrowser unit tests. ' contentbrowser -> content_browsertests https://codereview.chromium.org/15942016/diff/27001/build/android/run_all_tes... build/android/run_all_tests.py:166: 'giving up.') Be consistent (and follow PEP8) with indenting. Compare to line 187. https://codereview.chromium.org/15942016/diff/27001/build/android/run_browser... File build/android/run_browser_tests.py (right): https://codereview.chromium.org/15942016/diff/27001/build/android/run_browser... build/android/run_browser_tests.py:20: sys.exit(subprocess.call(args)) Use pylib/cmd_helper.py instead of subprocess directly as it has workarounds for bugs in Python 2.x's subprocess module. https://codereview.chromium.org/15942016/diff/27001/build/android/run_instrum... File build/android/run_instrumentation_tests.py (right): https://codereview.chromium.org/15942016/diff/27001/build/android/run_instrum... build/android/run_instrumentation_tests.py:20: sys.exit(subprocess.call(args)) ditto https://codereview.chromium.org/15942016/diff/27001/build/android/run_tests.py File build/android/run_tests.py (right): https://codereview.chromium.org/15942016/diff/27001/build/android/run_tests.p... build/android/run_tests.py:20: sys.exit(subprocess.call(args)) ditto https://codereview.chromium.org/15942016/diff/27001/build/android/run_uiautom... File build/android/run_uiautomator_tests.py (right): https://codereview.chromium.org/15942016/diff/27001/build/android/run_uiautom... build/android/run_uiautomator_tests.py:20: sys.exit(subprocess.call(args)) ditto
https://codereview.chromium.org/15942016/diff/27001/build/android/pylib/instr... File build/android/pylib/instrumentation/dispatch.py (right): https://codereview.chromium.org/15942016/diff/27001/build/android/pylib/instr... build/android/pylib/instrumentation/dispatch.py:19: def DispatchCore(options): On 2013/06/12 04:31:46, frankf wrote: > Just inline this. Done. https://codereview.chromium.org/15942016/diff/27001/build/android/pylib/uiaut... File build/android/pylib/uiautomator/dispatch.py (right): https://codereview.chromium.org/15942016/diff/27001/build/android/pylib/uiaut... build/android/pylib/uiautomator/dispatch.py:19: def DispatchCore(options): On 2013/06/12 04:31:46, frankf wrote: > Same with this. Done. https://codereview.chromium.org/15942016/diff/27001/build/android/pylib/utils... File build/android/pylib/utils/test_options_parser.py (left): https://codereview.chromium.org/15942016/diff/27001/build/android/pylib/utils... build/android/pylib/utils/test_options_parser.py:57: def AddTestRunnerOptions(option_parser, default_timeout=60): On 2013/06/12 04:31:46, frankf wrote: > Some downstream scripts depend on this. Maybe leave this here with a TODO about > the dependency to be removed in a future CL. Also make sure there are no other > dependency on any of the code being change in this CL. Added back AddTestRunnerOptions and AddBuildTypeOptions, with a comment. These are the only depended-on functions in clank and the main repo. https://codereview.chromium.org/15942016/diff/27001/build/android/run_all_tes... File build/android/run_all_tests.py (right): https://codereview.chromium.org/15942016/diff/27001/build/android/run_all_tes... build/android/run_all_tests.py:7: """Runs all types of tests from one unified interface. On 2013/06/12 18:37:03, craigdh wrote: > I don't like the name run_all_tests.py. This name is usually used for wrappers > around running multiple tests. This is a launcher for all types, but under no > condition will it run all tests. > > run_tests.py would be best but is already taken for the old gtest launcher. > Maybe just tests.py. @Frank WDYT? I agree that run_all_tests.py is not the best name. I would ideally have used run_tests.py, but it was taken unfortunately. What about run_tests_entry.py, or run_tests_generic.py, or something along those lines? I don't really like tests.py, since it's not clear that it's an actionable script rather than an include. I'm going to leave the filename as-is for now, and I can modify it once we've decided on a better name. https://codereview.chromium.org/15942016/diff/27001/build/android/run_all_tes... build/android/run_all_tests.py:11: Example: ./run_all_tests.py --gtests -s android_webview_unittests On 2013/06/12 04:31:46, frankf wrote: > Let's make test type a positional arg instead of an option. Done. https://codereview.chromium.org/15942016/diff/27001/build/android/run_all_tes... build/android/run_all_tests.py:13: Example: ./adb_install_apk.py --apk=ContentShell.apk --release On 2013/06/12 04:31:46, frankf wrote: > As said before, remove --release. Done. https://codereview.chromium.org/15942016/diff/27001/build/android/run_all_tes... build/android/run_all_tests.py:14: ./run_all_tests.py --contentbrowsertests --test-apk=ContentShellTest On 2013/06/12 04:31:46, frankf wrote: > content_browsertests Done. https://codereview.chromium.org/15942016/diff/27001/build/android/run_all_tes... build/android/run_all_tests.py:19: 3a. Python host-driven On 2013/06/12 04:31:46, frankf wrote: > Let's remove instructions for host-driven all together, it will change in the > future and is not a common use case upstream. Done. https://codereview.chromium.org/15942016/diff/27001/build/android/run_all_tes... build/android/run_all_tests.py:40: * Allow running many test types non-exclusively from the same command. On 2013/06/12 04:31:46, frankf wrote: > Running multiple types of tests in not an important use case. Done. https://codereview.chromium.org/15942016/diff/27001/build/android/run_all_tes... build/android/run_all_tests.py:62: """Decorates OptionParser with test type options in an OptionGroup.""" On 2013/06/12 18:37:03, craigdh wrote: > Decorate has a specific meaning in Python which doesn't match this. I was attempting to follow the pattern of comments on the test_options_parser file, but since we're making the file obsolete it makes sense to change these comments --> fixed in the latest patch set. https://codereview.chromium.org/15942016/diff/27001/build/android/run_all_tes... build/android/run_all_tests.py:76: help='If set, run contentbrowser unit tests. ' On 2013/06/12 18:37:03, craigdh wrote: > contentbrowser -> content_browsertests Done. https://codereview.chromium.org/15942016/diff/27001/build/android/run_all_tes... build/android/run_all_tests.py:166: 'giving up.') On 2013/06/12 18:37:03, craigdh wrote: > Be consistent (and follow PEP8) with indenting. Compare to line 187. Done. https://codereview.chromium.org/15942016/diff/27001/build/android/run_all_tes... build/android/run_all_tests.py:211: def AddGTestContentBrowserTestOptions(option_parser): On 2013/06/12 04:31:46, frankf wrote: > As said before, this should be AddGTestOptions, there's no > content_browser-specific options. Done. https://codereview.chromium.org/15942016/diff/27001/build/android/run_all_tes... build/android/run_all_tests.py:236: def AddInstrumentationUIAutomatorOptions(option_parser): On 2013/06/12 04:31:46, frankf wrote: > Let's call these 'java tests' here and in the help description Done. https://codereview.chromium.org/15942016/diff/27001/build/android/run_all_tes... build/android/run_all_tests.py:257: default=False, help='Run only the Python tests.') On 2013/06/12 04:31:46, frankf wrote: > Python -> host-driven Done. https://codereview.chromium.org/15942016/diff/27001/build/android/run_all_tes... build/android/run_all_tests.py:268: help='Root of the python-driven tests.') On 2013/06/12 04:31:46, frankf wrote: > Same here. Done. https://codereview.chromium.org/15942016/diff/27001/build/android/run_all_tes... build/android/run_all_tests.py:326: # determined based on the --test-apk or --test-jar flag. On 2013/06/12 04:31:46, frankf wrote: > Why don't you validate based on the test type arg? This was a remnant of having Instrumentation and UIAutomator tests combined -- fixed. https://codereview.chromium.org/15942016/diff/27001/build/android/run_all_tes... build/android/run_all_tests.py:447: ValidateTestTypeOption(options, option_parser) On 2013/06/12 04:31:46, frankf wrote: > Move this to line 443 Done. https://codereview.chromium.org/15942016/diff/27001/build/android/run_browser... File build/android/run_browser_tests.py (right): https://codereview.chromium.org/15942016/diff/27001/build/android/run_browser... build/android/run_browser_tests.py:16: os.path.expandvars("$CHROME_SRC/build/android/run_all_tests.py"), On 2013/06/12 04:31:46, frankf wrote: > Use __file__ to get the path to the script. Done. https://codereview.chromium.org/15942016/diff/27001/build/android/run_browser... build/android/run_browser_tests.py:18: logging.warning("This script is deprecated. " On 2013/06/12 04:31:46, frankf wrote: > Make this more prominant by adding : > > '*' * 80 > blah > '*' * 80 Done. https://codereview.chromium.org/15942016/diff/27001/build/android/run_browser... build/android/run_browser_tests.py:20: sys.exit(subprocess.call(args)) On 2013/06/12 18:37:03, craigdh wrote: > Use pylib/cmd_helper.py instead of subprocess directly as it has workarounds for > bugs in Python 2.x's subprocess module. Done. https://codereview.chromium.org/15942016/diff/27001/build/android/run_instrum... File build/android/run_instrumentation_tests.py (right): https://codereview.chromium.org/15942016/diff/27001/build/android/run_instrum... build/android/run_instrumentation_tests.py:20: sys.exit(subprocess.call(args)) On 2013/06/12 18:37:03, craigdh wrote: > ditto Done. https://codereview.chromium.org/15942016/diff/27001/build/android/run_tests.py File build/android/run_tests.py (right): https://codereview.chromium.org/15942016/diff/27001/build/android/run_tests.p... build/android/run_tests.py:20: sys.exit(subprocess.call(args)) On 2013/06/12 18:37:03, craigdh wrote: > ditto Done. https://codereview.chromium.org/15942016/diff/27001/build/android/run_uiautom... File build/android/run_uiautomator_tests.py (right): https://codereview.chromium.org/15942016/diff/27001/build/android/run_uiautom... build/android/run_uiautomator_tests.py:20: sys.exit(subprocess.call(args)) On 2013/06/12 18:37:03, craigdh wrote: > ditto Done.
https://codereview.chromium.org/15942016/diff/43001/build/android/pylib/host_... File build/android/pylib/host_driven/run_python_tests.py (right): https://codereview.chromium.org/15942016/diff/43001/build/android/pylib/host_... build/android/pylib/host_driven/run_python_tests.py:115: flakiness_server=options.flakiness_dashboard_server) Verify this works https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... File build/android/run_all_tests.py (right): https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:11: Example: ./run_all_tests.py gtests -s android_webview_unittests It'd be good to give such canonical examples on --help under different OptionGroups. Only include most commonly used options (i.e. not host_driven tests) https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:14: ./run_all_tests.py content_browsertests --test-apk=ContentShellTest Why does this take --test-apk. These tests don't take -s either. So please clarify this using an example in --help. https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:18: Example: ./adb_install_apk.py --apk=ChromiumTestShellTest.apk this is already done by -I option. Please update the TODO below as well. https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:50: VALID_TEST_TYPES = ["gtests", "content_browsertests", "instrumentationtests", Use single quotes consistantly. Please run gpylint on all changed files. https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:51: "uiautomatortests"] instrumentationtests -> instrumentation uiautomatortests -> uiautomator https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:57: option_parser.error("You must specify a test type.") List the test types here https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:57: option_parser.error("You must specify a test type.") Perhaps only pass in the error method as a parameter https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:85: option_container.add_option('-e', '--emulator', dest='use_emulator', You've moved options such as this to common, but they don't have the plumbing to work for all test types. In this case, it only works for gtest. For now, move such options back and add a TODO to make it common. https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:175: def AddGTestTestOptions(option_parser): second Test is redunant https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:176: """Adds gtest options in an OptionGroup to the OptionParser""" Rephrase this. e.g. Add gtest options to |option_parser| Same below https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:206: 'of which tests to run, and how to run ' Not sure if this doc is conveying any information. Please rephrase and make it specific about java tests. Same with above. https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:227: option_group.add_option('--shard_retries', type=int, default=1, I think this is not used anywhere and is specific to host-driven tests. Please add a TODO to delete in a future CL. https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:251: AddUIAutomatorOptions(option_group) To avoid confusion, let's have separate OptionGroups for Instrumentation-specific and UIAutomator-specific options. Under each OptionGroup provide a canonical example. https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:257: """Validates options/arguments and populates options with defaults.""" Let's just call these Process instead of Validate to be consistant everywhere. https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:290: """Adds java/python instrumentation test options to the OptionContainer.""" remove "java/python" https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:290: """Adds java/python instrumentation test options to the OptionContainer.""" What's an OptionContainer, you mean OptionGroup? https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:295: help='(Instrumentation only) Install APK.') "Install test APK" https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:367: total_failed += gtest_dispatch.Dispatch(options) No need to aggregate the count since we only run one type https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:373: total_failed += python_dispatch.Dispatch(options) This is run after instrumentation tests. Same below https://codereview.chromium.org/15942016/diff/43001/build/android/run_browser... File build/android/run_browser_tests.py (right): https://codereview.chromium.org/15942016/diff/43001/build/android/run_browser... build/android/run_browser_tests.py:16: args = ["python", Remove python
Also rebase often as there have been multiple patches in this area On 2013/06/13 23:17:48, frankf wrote: > https://codereview.chromium.org/15942016/diff/43001/build/android/pylib/host_... > File build/android/pylib/host_driven/run_python_tests.py (right): > > https://codereview.chromium.org/15942016/diff/43001/build/android/pylib/host_... > build/android/pylib/host_driven/run_python_tests.py:115: > flakiness_server=options.flakiness_dashboard_server) > Verify this works > > https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... > File build/android/run_all_tests.py (right): > > https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... > build/android/run_all_tests.py:11: Example: ./run_all_tests.py gtests -s > android_webview_unittests > It'd be good to give such canonical examples on --help under different > OptionGroups. Only include most commonly used options (i.e. not host_driven > tests) > > https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... > build/android/run_all_tests.py:14: ./run_all_tests.py content_browsertests > --test-apk=ContentShellTest > Why does this take --test-apk. These tests don't take -s either. So please > clarify this using an example in --help. > > https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... > build/android/run_all_tests.py:18: Example: ./adb_install_apk.py > --apk=ChromiumTestShellTest.apk > this is already done by -I option. Please update the TODO below as well. > > https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... > build/android/run_all_tests.py:50: VALID_TEST_TYPES = ["gtests", > "content_browsertests", "instrumentationtests", > Use single quotes consistantly. Please run gpylint on all changed files. > > https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... > build/android/run_all_tests.py:51: "uiautomatortests"] > instrumentationtests -> instrumentation > uiautomatortests -> uiautomator > > https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... > build/android/run_all_tests.py:57: option_parser.error("You must specify a test > type.") > List the test types here > > https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... > build/android/run_all_tests.py:57: option_parser.error("You must specify a test > type.") > Perhaps only pass in the error method as a parameter > > https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... > build/android/run_all_tests.py:85: option_container.add_option('-e', > '--emulator', dest='use_emulator', > You've moved options such as this to common, but they don't have the plumbing to > work for all test types. In this case, it only works for gtest. For now, move > such options back and add a TODO to make it common. > > https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... > build/android/run_all_tests.py:175: def AddGTestTestOptions(option_parser): > second Test is redunant > > https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... > build/android/run_all_tests.py:176: """Adds gtest options in an OptionGroup to > the OptionParser""" > Rephrase this. e.g. Add gtest options to |option_parser| > Same below > > https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... > build/android/run_all_tests.py:206: 'of which tests to run, and how to run ' > Not sure if this doc is conveying any information. Please rephrase and make it > specific about java tests. Same with above. > > https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... > build/android/run_all_tests.py:227: option_group.add_option('--shard_retries', > type=int, default=1, > I think this is not used anywhere and is specific to host-driven tests. Please > add a TODO to delete in a future CL. > > https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... > build/android/run_all_tests.py:251: AddUIAutomatorOptions(option_group) > To avoid confusion, let's have separate OptionGroups for > Instrumentation-specific and UIAutomator-specific options. Under each > OptionGroup provide a canonical example. > > https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... > build/android/run_all_tests.py:257: """Validates options/arguments and populates > options with defaults.""" > Let's just call these Process instead of Validate to be consistant everywhere. > > https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... > build/android/run_all_tests.py:290: """Adds java/python instrumentation test > options to the OptionContainer.""" > remove "java/python" > > https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... > build/android/run_all_tests.py:290: """Adds java/python instrumentation test > options to the OptionContainer.""" > What's an OptionContainer, you mean OptionGroup? > > https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... > build/android/run_all_tests.py:295: help='(Instrumentation only) Install APK.') > "Install test APK" > > https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... > build/android/run_all_tests.py:367: total_failed += > gtest_dispatch.Dispatch(options) > No need to aggregate the count since we only run one type > > https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... > build/android/run_all_tests.py:373: total_failed += > python_dispatch.Dispatch(options) > This is run after instrumentation tests. Same below > > https://codereview.chromium.org/15942016/diff/43001/build/android/run_browser... > File build/android/run_browser_tests.py (right): > > https://codereview.chromium.org/15942016/diff/43001/build/android/run_browser... > build/android/run_browser_tests.py:16: args = ["python", > Remove python
https://codereview.chromium.org/15942016/diff/43001/build/android/pylib/host_... File build/android/pylib/host_driven/run_python_tests.py (right): https://codereview.chromium.org/15942016/diff/43001/build/android/pylib/host_... build/android/pylib/host_driven/run_python_tests.py:115: flakiness_server=options.flakiness_dashboard_server) On 2013/06/13 23:17:48, frankf wrote: > Verify this works Done. https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... File build/android/run_all_tests.py (right): https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:11: Example: ./run_all_tests.py gtests -s android_webview_unittests On 2013/06/13 23:17:48, frankf wrote: > It'd be good to give such canonical examples on --help under different > OptionGroups. Only include most commonly used options (i.e. not host_driven > tests) Done. https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:14: ./run_all_tests.py content_browsertests --test-apk=ContentShellTest On 2013/06/13 23:17:48, frankf wrote: > Why does this take --test-apk. These tests don't take -s either. So please > clarify this using an example in --help. Done. https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:18: Example: ./adb_install_apk.py --apk=ChromiumTestShellTest.apk On 2013/06/13 23:17:48, frankf wrote: > this is already done by -I option. Please update the TODO below as well. Done. https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:50: VALID_TEST_TYPES = ["gtests", "content_browsertests", "instrumentationtests", On 2013/06/13 23:17:48, frankf wrote: > Use single quotes consistantly. Please run gpylint on all changed files. Done. https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:51: "uiautomatortests"] On 2013/06/13 23:17:48, frankf wrote: > instrumentationtests -> instrumentation > uiautomatortests -> uiautomator Done. https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:57: option_parser.error("You must specify a test type.") On 2013/06/13 23:17:48, frankf wrote: > List the test types here Test types -- Done. https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:57: option_parser.error("You must specify a test type.") On 2013/06/13 23:17:48, frankf wrote: > Perhaps only pass in the error method as a parameter Error method -- good idea, changed. https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:85: option_container.add_option('-e', '--emulator', dest='use_emulator', On 2013/06/13 23:17:48, frankf wrote: > You've moved options such as this to common, but they don't have the plumbing to > work for all test types. In this case, it only works for gtest. For now, move > such options back and add a TODO to make it common. Done. https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:175: def AddGTestTestOptions(option_parser): On 2013/06/13 23:17:48, frankf wrote: > second Test is redunant Done. https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:176: """Adds gtest options in an OptionGroup to the OptionParser""" On 2013/06/13 23:17:48, frankf wrote: > Rephrase this. e.g. Add gtest options to |option_parser| > Same below Done. https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:206: 'of which tests to run, and how to run ' On 2013/06/13 23:17:48, frankf wrote: > Not sure if this doc is conveying any information. Please rephrase and make it > specific about java tests. Same with above. Done. https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:227: option_group.add_option('--shard_retries', type=int, default=1, On 2013/06/13 23:17:48, frankf wrote: > I think this is not used anywhere and is specific to host-driven tests. Please > add a TODO to delete in a future CL. Done. https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:251: AddUIAutomatorOptions(option_group) On 2013/06/13 23:17:48, frankf wrote: > To avoid confusion, let's have separate OptionGroups for > Instrumentation-specific and UIAutomator-specific options. Under each > OptionGroup provide a canonical example. Done. https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:257: """Validates options/arguments and populates options with defaults.""" On 2013/06/13 23:17:48, frankf wrote: > Let's just call these Process instead of Validate to be consistant everywhere. Done. https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:290: """Adds java/python instrumentation test options to the OptionContainer.""" On 2013/06/13 23:17:48, frankf wrote: > remove "java/python" "java/python" -- Done. https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:290: """Adds java/python instrumentation test options to the OptionContainer.""" On 2013/06/13 23:17:48, frankf wrote: > What's an OptionContainer, you mean OptionGroup? I used OptionContainer here because these could be added to either another OptionGroup or to an OptionParser, which both are subclasses of OptionContainer. I changed this to create an OptionGroup now, however, so it just says option_parser now. https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:295: help='(Instrumentation only) Install APK.') On 2013/06/13 23:17:48, frankf wrote: > "Install test APK" Done. https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:367: total_failed += gtest_dispatch.Dispatch(options) On 2013/06/13 23:17:48, frankf wrote: > No need to aggregate the count since we only run one type Done. https://codereview.chromium.org/15942016/diff/43001/build/android/run_all_tes... build/android/run_all_tests.py:373: total_failed += python_dispatch.Dispatch(options) On 2013/06/13 23:17:48, frankf wrote: > This is run after instrumentation tests. Same below Done. https://codereview.chromium.org/15942016/diff/43001/build/android/run_browser... File build/android/run_browser_tests.py (right): https://codereview.chromium.org/15942016/diff/43001/build/android/run_browser... build/android/run_browser_tests.py:16: args = ["python", On 2013/06/13 23:17:48, frankf wrote: > Remove python I added this because on the try servers the tests were failing because the run_all_tests.py script was not marked executable. Adding the python command allows these scripts to call through to run_all_tests.py without having to add a command to mark it executable. Is there a better way to do this?
https://codereview.chromium.org/15942016/diff/52001/build/android/run_all_tes... File build/android/run_all_tests.py (right): https://codereview.chromium.org/15942016/diff/52001/build/android/run_all_tes... build/android/run_all_tests.py:53: errorf('You must specify a test type. Options are: ' + Let's stick a newline in here before "Options". This is a very long line. https://codereview.chromium.org/15942016/diff/52001/build/android/run_all_tes... build/android/run_all_tests.py:56: errorf('Invalid test type. The test type must be one of: ' + ditto https://codereview.chromium.org/15942016/diff/52001/build/android/run_all_tes... build/android/run_all_tests.py:102: option_group = optparse.OptionGroup(option_parser, 'Common Options', Also print the list of valid test_type first thing in the help. https://codereview.chromium.org/15942016/diff/52001/build/android/run_all_tes... build/android/run_all_tests.py:116: option_group.add_option('-t', dest='timeout', Does this actually work for all test types? https://codereview.chromium.org/15942016/diff/52001/build/android/run_all_tes... build/android/run_all_tests.py:123: option_group.add_option('--num_retries', dest='num_retries', type='int', Does this actually work for all test types? https://codereview.chromium.org/15942016/diff/52001/build/android/run_all_tes... build/android/run_all_tests.py:140: option_group.add_option('--tool', Let's come up with a list of options we'd like to remove completely in the future and mark them as deprecated in this help so people know not to use them in scripts. https://codereview.chromium.org/15942016/diff/52001/build/android/run_all_tes... build/android/run_all_tests.py:181: 'Browser specific options currently! Example usage: ./run_all_tests.py ' No ! https://codereview.chromium.org/15942016/diff/52001/build/android/run_all_tes... build/android/run_all_tests.py:199: option_group.add_option('-x', '--xvfb', dest='use_xvfb', Do these also work for content_browsertests? https://codereview.chromium.org/15942016/diff/52001/build/android/run_all_tes... build/android/run_all_tests.py:222: 'See the specific options for one or the ' Try to use as few words as possible. People tend to skip over blocks of text. https://codereview.chromium.org/15942016/diff/52001/build/android/run_all_tes... build/android/run_all_tests.py:245: option_group.add_option('--shard_retries', type=int, default=1, What's the difference between this and the common option retries? https://codereview.chromium.org/15942016/diff/52001/build/android/run_all_tes... build/android/run_all_tests.py:308: 'Options specific to Instrumentation Tests. Example usage: ' Clarify here that these are in addition to the java test options (for people who don't read all the other option descriptions). https://codereview.chromium.org/15942016/diff/52001/build/android/run_all_tes... build/android/run_all_tests.py:351: 'Options specific to UIAutomator tests. Example usage: ./run_all_tests.py' ditto https://codereview.chromium.org/15942016/diff/52001/build/android/run_all_tes... build/android/run_all_tests.py:356: help=('(UIAutomator only) The package name used by the apk ' No longer need all the "UIAutomator only" and "Instrumentation only".
https://codereview.chromium.org/15942016/diff/52001/build/android/run_all_tes... File build/android/run_all_tests.py (right): https://codereview.chromium.org/15942016/diff/52001/build/android/run_all_tes... build/android/run_all_tests.py:53: errorf('You must specify a test type. Options are: ' + On 2013/06/17 22:37:05, craigdh wrote: > Let's stick a newline in here before "Options". This is a very long line. Done. https://codereview.chromium.org/15942016/diff/52001/build/android/run_all_tes... build/android/run_all_tests.py:56: errorf('Invalid test type. The test type must be one of: ' + On 2013/06/17 22:37:05, craigdh wrote: > ditto Done. https://codereview.chromium.org/15942016/diff/52001/build/android/run_all_tes... build/android/run_all_tests.py:102: option_group = optparse.OptionGroup(option_parser, 'Common Options', On 2013/06/17 22:37:05, craigdh wrote: > Also print the list of valid test_type first thing in the help. Done. https://codereview.chromium.org/15942016/diff/52001/build/android/run_all_tes... build/android/run_all_tests.py:116: option_group.add_option('-t', dest='timeout', On 2013/06/17 22:37:05, craigdh wrote: > Does this actually work for all test types? Ah, you're right, this only works for gtests at the moment. Moved and added a TODO. https://codereview.chromium.org/15942016/diff/52001/build/android/run_all_tes... build/android/run_all_tests.py:123: option_group.add_option('--num_retries', dest='num_retries', type='int', On 2013/06/17 22:37:05, craigdh wrote: > Does this actually work for all test types? This was used by all test types except UIAutomator, and I couldn't see a good reason for it not to use it (it used the pylib.base sharder just like the other test types), so I added the num_retries argument to the sharder and left this option as-is. https://codereview.chromium.org/15942016/diff/52001/build/android/run_all_tes... build/android/run_all_tests.py:140: option_group.add_option('--tool', On 2013/06/17 22:37:05, craigdh wrote: > Let's come up with a list of options we'd like to remove completely in the > future and mark them as deprecated in this help so people know not to use them > in scripts. Done for most options. I'm still waiting on Peter Beverloo for the --webkit option, so I left it as a TODO. I can update this in a future CL, once I hear back. https://codereview.chromium.org/15942016/diff/52001/build/android/run_all_tes... build/android/run_all_tests.py:181: 'Browser specific options currently! Example usage: ./run_all_tests.py ' On 2013/06/17 22:37:05, craigdh wrote: > No ! Done. https://codereview.chromium.org/15942016/diff/52001/build/android/run_all_tes... build/android/run_all_tests.py:199: option_group.add_option('-x', '--xvfb', dest='use_xvfb', On 2013/06/17 22:37:05, craigdh wrote: > Do these also work for content_browsertests? Most of these do (--suite, --test_arguments, --webkit do, --xvfb and --exe don't), but my understanding was that this is "GTest" in the sense of framework, rather than unit vs. content browser tests, so I renamed this to AddGTestOptions based on Frank's comment on Patch Set 1: > I would change this back to GTest. We care about the framework not whether it's unit or content_browsertests. https://codereview.chromium.org/15942016/diff/52001/build/android/run_all_tes... build/android/run_all_tests.py:222: 'See the specific options for one or the ' On 2013/06/17 22:37:05, craigdh wrote: > Try to use as few words as possible. People tend to skip over blocks of text. Done. https://codereview.chromium.org/15942016/diff/52001/build/android/run_all_tes... build/android/run_all_tests.py:245: option_group.add_option('--shard_retries', type=int, default=1, On 2013/06/17 22:37:05, craigdh wrote: > What's the difference between this and the common option retries? This option is currently only used in host_driven tests and Monkey Tests. Since I'm waiting until the next CL to add Monkey Test options, I think it makes sense to wait until then to try to combine this with num_retries (thus the TODO). https://codereview.chromium.org/15942016/diff/52001/build/android/run_all_tes... build/android/run_all_tests.py:308: 'Options specific to Instrumentation Tests. Example usage: ' On 2013/06/17 22:37:05, craigdh wrote: > Clarify here that these are in addition to the java test options (for people who > don't read all the other option descriptions). Done. https://codereview.chromium.org/15942016/diff/52001/build/android/run_all_tes... build/android/run_all_tests.py:351: 'Options specific to UIAutomator tests. Example usage: ./run_all_tests.py' On 2013/06/17 22:37:05, craigdh wrote: > ditto Done. https://codereview.chromium.org/15942016/diff/52001/build/android/run_all_tes... build/android/run_all_tests.py:356: help=('(UIAutomator only) The package name used by the apk ' On 2013/06/17 22:37:05, craigdh wrote: > No longer need all the "UIAutomator only" and "Instrumentation only". Done.
Uploaded PS7 which converts the options to the command-style format.
https://codereview.chromium.org/15942016/diff/57001/build/android/pylib/brows... File build/android/pylib/browsertests/dispatch.py (right): https://codereview.chromium.org/15942016/diff/57001/build/android/pylib/brows... build/android/pylib/browsertests/dispatch.py:5: """Dispatches Content Browser tests.""" -> content_browsertests, also in other places. https://codereview.chromium.org/15942016/diff/57001/build/android/pylib/host_... File build/android/pylib/host_driven/run_python_tests.py (right): https://codereview.chromium.org/15942016/diff/57001/build/android/pylib/host_... build/android/pylib/host_driven/run_python_tests.py:117: test_type='HostDriven', Take a look at _LogToFlakinessDashboard https://codereview.chromium.org/15942016/diff/57001/build/android/run_all_tes... File build/android/run_all_tests.py (right): https://codereview.chromium.org/15942016/diff/57001/build/android/run_all_tes... build/android/run_all_tests.py:10: 1. GTest native unit tests (gtests) Let's remove these examples since --help should be sufficient. https://codereview.chromium.org/15942016/diff/60001/build/android/run_all_tes... File build/android/run_all_tests.py (right): https://codereview.chromium.org/15942016/diff/60001/build/android/run_all_tes... build/android/run_all_tests.py:184: 'Browser specific options currently. Example usage: ./run_all_tests.py ' Remove this all together. https://codereview.chromium.org/15942016/diff/67001/build/android/run_all_tes... File build/android/run_all_tests.py (right): https://codereview.chromium.org/15942016/diff/67001/build/android/run_all_tes... build/android/run_all_tests.py:138: # crbug.com/147176. We should remove this option once FlakyTests are removed We're not removing FlakyTests. The root issue is that tests should have no notion of buildbot. Buildbot scripts should decide how to deal with test failures: crbug.com/170477 https://codereview.chromium.org/15942016/diff/67001/build/android/run_all_tes... build/android/run_all_tests.py:168: option_parser.description = ('Example call: %prog gtests -s ' You have this as Example usage in other places. Have something like: Example <command> <optional description> https://codereview.chromium.org/15942016/diff/67001/build/android/run_all_tes... build/android/run_all_tests.py:168: option_parser.description = ('Example call: %prog gtests -s ' prog is shown as ./run_all_tests.py regardless of cwd. https://codereview.chromium.org/15942016/diff/67001/build/android/run_all_tes... build/android/run_all_tests.py:168: option_parser.description = ('Example call: %prog gtests -s ' %s/gtests/gtest/g https://codereview.chromium.org/15942016/diff/67001/build/android/run_all_tes... build/android/run_all_tests.py:328: option_parser.description = ('Example usage: ./run_all_tests.py uiautomator ' Indent this on the next lien https://codereview.chromium.org/15942016/diff/67001/build/android/run_all_tes... build/android/run_all_tests.py:367: def RunTestsCommand(command_string, options, args, option_parser): :%s/command_string/command/g https://codereview.chromium.org/15942016/diff/67001/build/android/run_all_tes... build/android/run_all_tests.py:420: # add all options relating to that test. Shorten this comment to (test type, list of functions to run) https://codereview.chromium.org/15942016/diff/67001/build/android/run_all_tes... build/android/run_all_tests.py:421: COMMANDS = { Defien all constants at the top. Follow the naming convention in the style guide relating to the scope https://codereview.chromium.org/15942016/diff/67001/build/android/run_all_tes... build/android/run_all_tests.py:422: 'gtests': (RunTestsCommand, AddGTestOptions), I would reverse the ordering of these two. https://codereview.chromium.org/15942016/diff/67001/build/android/run_all_tes... build/android/run_all_tests.py:427: 'help': (HelpCommand, lambda option_parser: None) "test --help" should be identical to: "help test" Also: "help content_browsertests" shows "./run_all_tests.py content_browsertests" for some reason. https://codereview.chromium.org/15942016/diff/67001/build/android/run_all_tes... build/android/run_all_tests.py:434: def __init__(self, *args, **kwargs): Why do you take arbitrary params? https://codereview.chromium.org/15942016/diff/67001/build/android/run_all_tes... build/android/run_all_tests.py:445: return 'Commands:\n ' + '\n '.join(sorted(self.command_list)) + '\n\n' Prefer using string formating '%s' % foo https://codereview.chromium.org/15942016/diff/67001/build/android/run_all_tes... build/android/run_all_tests.py:451: usage='Usage: %prog command [options]', <command> https://codereview.chromium.org/15942016/diff/67001/build/android/run_all_tes... build/android/run_all_tests.py:459: option_parser.error('Invalid command.') This shouldn't be thrown if --help is specified. https://codereview.chromium.org/15942016/diff/67001/build/android/run_all_tes... build/android/run_all_tests.py:460: COMMANDS[command][1](option_parser) # Call the function to add options for This is pretty unreadable. Think of a better way to do this.
https://codereview.chromium.org/15942016/diff/57001/build/android/pylib/brows... File build/android/pylib/browsertests/dispatch.py (right): https://codereview.chromium.org/15942016/diff/57001/build/android/pylib/brows... build/android/pylib/browsertests/dispatch.py:5: """Dispatches Content Browser tests.""" On 2013/06/27 23:13:35, frankf wrote: > -> content_browsertests, also in other places. Done. https://codereview.chromium.org/15942016/diff/57001/build/android/pylib/host_... File build/android/pylib/host_driven/run_python_tests.py (right): https://codereview.chromium.org/15942016/diff/57001/build/android/pylib/host_... build/android/pylib/host_driven/run_python_tests.py:117: test_type='HostDriven', On 2013/06/27 23:13:35, frankf wrote: > Take a look at _LogToFlakinessDashboard Ah, this is what I meant to ask you earlier -- I'm fixing this by changing the _LogToFlakinessDashboard function to accept 'HostDriven' as a valid test_type, but I wasn't sure if the Flakiness server would interpret this correctly. Let me know if I should change this back to 'Instrumentation'. https://codereview.chromium.org/15942016/diff/57001/build/android/run_all_tes... File build/android/run_all_tests.py (right): https://codereview.chromium.org/15942016/diff/57001/build/android/run_all_tes... build/android/run_all_tests.py:10: 1. GTest native unit tests (gtests) On 2013/06/27 23:13:35, frankf wrote: > Let's remove these examples since --help should be sufficient. Done. https://codereview.chromium.org/15942016/diff/67001/build/android/run_all_tes... File build/android/run_all_tests.py (right): https://codereview.chromium.org/15942016/diff/67001/build/android/run_all_tes... build/android/run_all_tests.py:138: # crbug.com/147176. We should remove this option once FlakyTests are removed On 2013/06/27 23:13:35, frankf wrote: > We're not removing FlakyTests. The root issue is that tests should have no > notion of buildbot. Buildbot scripts should decide how to deal with test > failures: crbug.com/170477 Ah, that clarifies things. Updated the comment. https://codereview.chromium.org/15942016/diff/67001/build/android/run_all_tes... build/android/run_all_tests.py:168: option_parser.description = ('Example call: %prog gtests -s ' On 2013/06/27 23:13:35, frankf wrote: > prog is shown as ./run_all_tests.py regardless of cwd. Looks like it shows just "run_all_tests.py" (no ./) for all commands -- I think this is pretty reasonable behavior, and is expected. https://codereview.chromium.org/15942016/diff/67001/build/android/run_all_tes... build/android/run_all_tests.py:168: option_parser.description = ('Example call: %prog gtests -s ' On 2013/06/27 23:13:35, frankf wrote: > You have this as Example usage in other places. Have something like: > > Example > <command> > > <optional description> Done. https://codereview.chromium.org/15942016/diff/67001/build/android/run_all_tes... build/android/run_all_tests.py:168: option_parser.description = ('Example call: %prog gtests -s ' On 2013/06/27 23:13:35, frankf wrote: > %s/gtests/gtest/g Done. https://codereview.chromium.org/15942016/diff/67001/build/android/run_all_tes... build/android/run_all_tests.py:328: option_parser.description = ('Example usage: ./run_all_tests.py uiautomator ' On 2013/06/27 23:13:35, frankf wrote: > Indent this on the next lien There are no newlines here, it is just a continuation of the previous line. I've updated the way examples are displayed. Without changing the formatter the OptionParser uses, we can't use newlines, but I think the new formatting is reasonable without needing newlines. https://codereview.chromium.org/15942016/diff/67001/build/android/run_all_tes... build/android/run_all_tests.py:367: def RunTestsCommand(command_string, options, args, option_parser): On 2013/06/27 23:13:35, frankf wrote: > :%s/command_string/command/g Done. https://codereview.chromium.org/15942016/diff/67001/build/android/run_all_tes... build/android/run_all_tests.py:420: # add all options relating to that test. On 2013/06/27 23:13:35, frankf wrote: > Shorten this comment to > > (test type, list of functions to run) I added a namedtuple for cleaning up the COMMANDS syntax, so I did something along these lines for a command on that. https://codereview.chromium.org/15942016/diff/67001/build/android/run_all_tes... build/android/run_all_tests.py:421: COMMANDS = { On 2013/06/27 23:13:35, frankf wrote: > Defien all constants at the top. Follow the naming convention in the style guide > relating to the scope The reason it's defined down here is because it needs to have the command functions defined before it can be defined. One way I could move this to the top is replacing the functions with strings, but I think that's more unclean than this. https://codereview.chromium.org/15942016/diff/67001/build/android/run_all_tes... build/android/run_all_tests.py:422: 'gtests': (RunTestsCommand, AddGTestOptions), On 2013/06/27 23:13:35, frankf wrote: > I would reverse the ordering of these two. Done. https://codereview.chromium.org/15942016/diff/67001/build/android/run_all_tes... build/android/run_all_tests.py:427: 'help': (HelpCommand, lambda option_parser: None) On 2013/06/27 23:13:35, frankf wrote: > "test --help" > should be identical to: > "help test" > > Also: > "help content_browsertests" > > shows "./run_all_tests.py content_browsertests" for some reason. > Done. https://codereview.chromium.org/15942016/diff/67001/build/android/run_all_tes... build/android/run_all_tests.py:434: def __init__(self, *args, **kwargs): On 2013/06/27 23:13:35, frankf wrote: > Why do you take arbitrary params? To pass through to the OptionParser __init__ command. https://codereview.chromium.org/15942016/diff/67001/build/android/run_all_tes... build/android/run_all_tests.py:445: return 'Commands:\n ' + '\n '.join(sorted(self.command_list)) + '\n\n' On 2013/06/27 23:13:35, frankf wrote: > Prefer using string formating '%s' % foo Done. https://codereview.chromium.org/15942016/diff/67001/build/android/run_all_tes... build/android/run_all_tests.py:451: usage='Usage: %prog command [options]', On 2013/06/27 23:13:35, frankf wrote: > <command> Done. https://codereview.chromium.org/15942016/diff/67001/build/android/run_all_tes... build/android/run_all_tests.py:459: option_parser.error('Invalid command.') On 2013/06/27 23:13:35, frankf wrote: > This shouldn't be thrown if --help is specified. Done. https://codereview.chromium.org/15942016/diff/67001/build/android/run_all_tes... build/android/run_all_tests.py:460: COMMANDS[command][1](option_parser) # Call the function to add options for On 2013/06/27 23:13:35, frankf wrote: > This is pretty unreadable. Think of a better way to do this. Replaced tuples with namedtuples to make this clearer.
https://codereview.chromium.org/15942016/diff/57001/build/android/pylib/host_... File build/android/pylib/host_driven/run_python_tests.py (right): https://codereview.chromium.org/15942016/diff/57001/build/android/pylib/host_... build/android/pylib/host_driven/run_python_tests.py:117: test_type='HostDriven', Yes, this requires server-side changes. Let's not change it in this CL On 2013/06/28 22:09:45, gkanwar wrote: > On 2013/06/27 23:13:35, frankf wrote: > > Take a look at _LogToFlakinessDashboard > > Ah, this is what I meant to ask you earlier -- I'm fixing this by changing the > _LogToFlakinessDashboard function to accept 'HostDriven' as a valid test_type, > but I wasn't sure if the Flakiness server would interpret this correctly. Let me > know if I should change this back to 'Instrumentation'. https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... File build/android/run_all_tests.py (right): https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... build/android/run_all_tests.py:86: option_parser.add_option('-f', '--test_filter', '--gtest_filter', Let's not make this a common option. gtest_filter is well known so we want to use it as as. https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... build/android/run_all_tests.py:151: def AddContentBrowserOptions(option_parser): Be consistent with naming. ContentBrowserTests/InstrumentationTests, etc. https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... build/android/run_all_tests.py:330: option_parser.example = ('%prog uiautomator ' Indent the line itself not the output. https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... build/android/run_all_tests.py:346: def ProcessUIAutomatorOptions(options, errorf): error_func https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... build/android/run_all_tests.py:401: """Display help for a certain command, or overall help.""" Add args and returns section to docstrings where appropriate https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... build/android/run_all_tests.py:408: if command not in COMMANDS: Blank lines around ifs https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... build/android/run_all_tests.py:425: Command = collections.namedtuple('Command', Command and COMMANDS are too generic. https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... build/android/run_all_tests.py:442: optparse.OptionParser.__init__(self, *args, **kwargs) Can you use super? https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... build/android/run_all_tests.py:444: def get_usage(self): add #override before overriden functions. https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... build/android/run_all_tests.py:469: if command not in COMMANDS: Combine the ifs https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... build/android/run_all_tests.py:474: exit_code = COMMANDS[command].run_command_func(command, options, args, Indent on next line
https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... File build/android/run_all_tests.py (right): https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... build/android/run_all_tests.py:119: help=('Address of the server that is hosting the ' no need for () around multi-line strings that are already in a larger set of ().
https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... File build/android/run_all_tests.py (right): https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... build/android/run_all_tests.py:86: option_parser.add_option('-f', '--test_filter', '--gtest_filter', On 2013/06/28 23:07:10, frankf wrote: > Let's not make this a common option. gtest_filter is well known so we want to > use it as as. Done. https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... build/android/run_all_tests.py:119: help=('Address of the server that is hosting the ' On 2013/07/01 18:28:52, craigdh wrote: > no need for () around multi-line strings that are already in a larger set of (). This was the style from the previous file (test_options_parser.py), so I just matched it. I could change this in the whole file, but to be honest I think it looks cleaner this way anyway. https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... build/android/run_all_tests.py:151: def AddContentBrowserOptions(option_parser): On 2013/06/28 23:07:10, frankf wrote: > Be consistent with naming. ContentBrowserTests/InstrumentationTests, etc. Done. https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... build/android/run_all_tests.py:330: option_parser.example = ('%prog uiautomator ' On 2013/06/28 23:07:10, frankf wrote: > Indent the line itself not the output. Got it. Done. https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... build/android/run_all_tests.py:346: def ProcessUIAutomatorOptions(options, errorf): On 2013/06/28 23:07:10, frankf wrote: > error_func Done. https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... build/android/run_all_tests.py:401: """Display help for a certain command, or overall help.""" On 2013/06/28 23:07:10, frankf wrote: > Add args and returns section to docstrings where appropriate Done. https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... build/android/run_all_tests.py:408: if command not in COMMANDS: On 2013/06/28 23:07:10, frankf wrote: > Blank lines around ifs Done. https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... build/android/run_all_tests.py:425: Command = collections.namedtuple('Command', On 2013/06/28 23:07:10, frankf wrote: > Command and COMMANDS are too generic. Done. https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... build/android/run_all_tests.py:442: optparse.OptionParser.__init__(self, *args, **kwargs) On 2013/06/28 23:07:10, frankf wrote: > Can you use super? Unfortunately not, since optparse.OptionParser is not a new-style class. https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... build/android/run_all_tests.py:444: def get_usage(self): On 2013/06/28 23:07:10, frankf wrote: > add #override before overriden functions. Done. https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... build/android/run_all_tests.py:469: if command not in COMMANDS: On 2013/06/28 23:07:10, frankf wrote: > Combine the ifs Done. https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... build/android/run_all_tests.py:474: exit_code = COMMANDS[command].run_command_func(command, options, args, On 2013/06/28 23:07:10, frankf wrote: > Indent on next line Done.
Looking good. https://codereview.chromium.org/15942016/diff/76001/build/android/pylib/host_... File build/android/pylib/host_driven/run_python_tests.py (right): https://codereview.chromium.org/15942016/diff/76001/build/android/pylib/host_... build/android/pylib/host_driven/run_python_tests.py:122: return len(results.GetNotPass()) We don't want to show the results for host-driven tests at the end. It will be confusing. Let's keep them combined for now. https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... File build/android/run_all_tests.py (right): https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... build/android/run_all_tests.py:1: #!/usr/bin/env python How about naming this test_runner.py? https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... build/android/run_all_tests.py:10: 1. GTest native unit tests (gtest) --help provides all this info, let's just remove this. https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... build/android/run_all_tests.py:280: """Adds Instrumentation test options to option_parser.""" |option_parser| https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... build/android/run_all_tests.py:298: AddJavaTestOptions(option_parser) Move these before the specific options. https://codereview.chromium.org/15942016/diff/86001/build/android/run_all_tes... File build/android/run_all_tests.py (right): https://codereview.chromium.org/15942016/diff/86001/build/android/run_all_tes... build/android/run_all_tests.py:165: option_parser.example = '%prog gtest -s android_webview_unittests' use base_unittests https://codereview.chromium.org/15942016/diff/86001/build/android/run_all_tes... build/android/run_all_tests.py:494: return '\nExample:\n %s\n\n' % self.example Why two blank lines here and above?
https://codereview.chromium.org/15942016/diff/76001/build/android/run_instrum... File build/android/run_instrumentation_tests.py (right): https://codereview.chromium.org/15942016/diff/76001/build/android/run_instrum... build/android/run_instrumentation_tests.py:22: logging.warning('The preferred command is: %s', ' '.join(args)) Also, let's use stronger language here. These will be removed soon.
https://codereview.chromium.org/15942016/diff/76001/build/android/pylib/host_... File build/android/pylib/host_driven/run_python_tests.py (right): https://codereview.chromium.org/15942016/diff/76001/build/android/pylib/host_... build/android/pylib/host_driven/run_python_tests.py:122: return len(results.GetNotPass()) On 2013/07/01 22:36:21, frankf wrote: > We don't want to show the results for host-driven tests at the end. It will be > confusing. Let's keep them combined for now. Done. https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... File build/android/run_all_tests.py (right): https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... build/android/run_all_tests.py:1: #!/usr/bin/env python On 2013/07/01 22:36:21, frankf wrote: > How about naming this test_runner.py? I feel like test_runner.py makes me feel like the script defines a TestRunner class, rather than being an actionable script. That said, I don't have a better suggestion yet, so I changed this in a separate patch set (so other changes are easy to see). https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... build/android/run_all_tests.py:10: 1. GTest native unit tests (gtest) On 2013/07/01 22:36:21, frankf wrote: > --help provides all this info, let's just remove this. Done. https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... build/android/run_all_tests.py:280: """Adds Instrumentation test options to option_parser.""" On 2013/07/01 22:36:21, frankf wrote: > |option_parser| Done. Also fixed in several other docstring comments. https://codereview.chromium.org/15942016/diff/76001/build/android/run_all_tes... build/android/run_all_tests.py:298: AddJavaTestOptions(option_parser) On 2013/07/01 22:36:21, frankf wrote: > Move these before the specific options. Done. Similar changes where I had more general options below specific ones in other functions as well. https://codereview.chromium.org/15942016/diff/76001/build/android/run_instrum... File build/android/run_instrumentation_tests.py (right): https://codereview.chromium.org/15942016/diff/76001/build/android/run_instrum... build/android/run_instrumentation_tests.py:22: logging.warning('The preferred command is: %s', ' '.join(args)) On 2013/07/01 22:39:28, frankf wrote: > Also, let's use stronger language here. These will be removed soon. Done. https://codereview.chromium.org/15942016/diff/86001/build/android/run_all_tes... File build/android/run_all_tests.py (right): https://codereview.chromium.org/15942016/diff/86001/build/android/run_all_tes... build/android/run_all_tests.py:165: option_parser.example = '%prog gtest -s android_webview_unittests' On 2013/07/01 22:36:21, frankf wrote: > use base_unittests Done. https://codereview.chromium.org/15942016/diff/86001/build/android/run_all_tes... build/android/run_all_tests.py:494: return '\nExample:\n %s\n\n' % self.example On 2013/07/01 22:36:21, frankf wrote: > Why two blank lines here and above? I wanted to separate Usage and Example from Options, since it felt like Usage and Example should be grouped. Looking back on this, however, it looks like 1 newline will probably look more consistent -- updated.
lgtm w/ nit. Please run the downstream host-driven tests before cqing. Also wait for Craig. https://codereview.chromium.org/15942016/diff/99001/build/android/pylib/utils... File build/android/pylib/utils/report_results.py (right): https://codereview.chromium.org/15942016/diff/99001/build/android/pylib/utils... build/android/pylib/utils/report_results.py:46: if test_type not in ['Instrumentation', 'HostDriven']: This is no longer needed right?
lgtm w/ nit. https://codereview.chromium.org/15942016/diff/99001/build/android/run_tests.py File build/android/run_tests.py (right): https://codereview.chromium.org/15942016/diff/99001/build/android/run_tests.p... build/android/run_tests.py:18: os.path.dirname(__file__) + '/test_runner.py', os.path.join() instead of hard coding the / Same for other files.
https://codereview.chromium.org/15942016/diff/99001/build/android/pylib/utils... File build/android/pylib/utils/report_results.py (right): https://codereview.chromium.org/15942016/diff/99001/build/android/pylib/utils... build/android/pylib/utils/report_results.py:46: if test_type not in ['Instrumentation', 'HostDriven']: On 2013/07/02 20:51:10, frankf wrote: > This is no longer needed right? Ah good catch. Fixed. https://codereview.chromium.org/15942016/diff/99001/build/android/run_tests.py File build/android/run_tests.py (right): https://codereview.chromium.org/15942016/diff/99001/build/android/run_tests.p... build/android/run_tests.py:18: os.path.dirname(__file__) + '/test_runner.py', On 2013/07/02 21:03:28, craigdh wrote: > os.path.join() instead of hard coding the / > > Same for other files. Done.
also update the issue title
On 2013/07/02 21:17:34, craigdh wrote: > also update the issue title Done. Working on running downstream tests, then I will CQ this.
On 2013/07/02 21:19:33, gkanwar wrote: > On 2013/07/02 21:17:34, craigdh wrote: > > also update the issue title > > Done. Working on running downstream tests, then I will CQ this. Downstream host-driven tests appear to work. CQ'ing!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gkanwar@google.com/15942016/109001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gkanwar@google.com/15942016/109001
Sorry for I got bad news for ya. Compile failed with a clobber build on win_x64_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
On 2013/07/03 02:17:15, I haz the power (commit-bot) wrote: > Sorry for I got bad news for ya. > Compile failed with a clobber build on win_x64_rel. > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re... > Your code is likely broken or HEAD is junk. Please ensure your > code is not broken then alert the build sheriffs. > Look at the try server FAQ for more details. This looks like something unrelated to my code. I'm going to try again to see what happens.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gkanwar@google.com/15942016/109001
Message was sent while issue was closed.
Change committed as 210035
Message was sent while issue was closed.
Why didn't you update our buildbot code in this CL? You've broken the output of all of our buildbot runs [bit.ly/19XKx7N]. I'm reverting this.
Message was sent while issue was closed.
On 2013/07/05 02:26:25, Isaac wrote: > Why didn't you update our buildbot code in this CL? You've broken the output of > all of our buildbot runs [bit.ly/19XKx7N]. > > I'm reverting this. I created a separate CL for the changes (https://codereview.chromium.org/18514008/) to the Buildbots, since this one was getting large. It sounds like it makes more sense to merge that CL into this one, so that the Buildbots don't show deprecation warnings for a short period in between. I can add that as a patch set to this CL.
Message was sent while issue was closed.
On 2013/07/08 16:38:15, gkanwar wrote: > On 2013/07/05 02:26:25, Isaac wrote: > > Why didn't you update our buildbot code in this CL? You've broken the output > of > > all of our buildbot runs [bit.ly/19XKx7N]. > > > > I'm reverting this. > > I created a separate CL for the changes > (https://codereview.chromium.org/18514008/) to the Buildbots, since this one was > getting large. It sounds like it makes more sense to merge that CL into this > one, so that the Buildbots don't show deprecation warnings for a short period in > between. I can add that as a patch set to this CL. Since this issue is closed at this point, updated the buildbot script CL to revert the revert and keep the buildbot changes, so the two can be committed together. Isaac, I'm going to add you as a reviewer on that CL.
Message was sent while issue was closed.
Thanks. In general it is best to make these changes atomically. And also to check the logs of the trybots before landing. :-) |
