Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
657433002:
Python wrapper to launch Chromoting browser-tests. (Closed)
DescriptionCreates a process for each line in specified input file.
BUG=
Committed: https://crrev.com/ad76c17dff74ba8a5e7446bb62840fd2e9882a59
Cr-Commit-Position: refs/heads/master@{#300156}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Include python wrapper in build output. #
Total comments: 6
Patch Set 3 : Add files as dependencies in .isolate file, instead of copying them over. #
Total comments: 2
Patch Set 4 : Raise exception if executing command fails. #
Total comments: 2
Patch Set 5 : Clarify help text for command-line parameters #Patch Set 6 : Pick up recently landed changes to master. #
Messages
Total messages: 28 (12 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
anandc@chromium.org changed reviewers: + leilei@chromium.org
Lei, PTAL. Thanks.
I prefer to create another isolate file and another target for v2 integration. In that case we could run the integration tests for both v1 and v2 in parallel. But it depends on you. https://codereview.chromium.org/657433002/diff/80001/testing/chromoting/brows... File testing/chromoting/browser_tests_launcher.py (right): https://codereview.chromium.org/657433002/diff/80001/testing/chromoting/brows... testing/chromoting/browser_tests_launcher.py:33: parser.add_argument('-c', '--cwd', Not sure how can you get cwd.
https://codereview.chromium.org/657433002/diff/80001/testing/chromoting/brows... File testing/chromoting/browser_tests_launcher.py (right): https://codereview.chromium.org/657433002/diff/80001/testing/chromoting/brows... testing/chromoting/browser_tests_launcher.py:33: parser.add_argument('-c', '--cwd', On 2014/10/14 01:58:37, Lei Lei wrote: > Not sure how can you get cwd. Hmmm. Good point. I was using cwd just to make sure the command was being run from the right place. I'll remove this. I'll also add a step to copy this file to the output-dir when building chromoting_integration_tests_run, and call it from there.
Patchset #2 (id:100001) has been deleted
Hi Lei, Please take a look at PS#2.
Patchset #2 (id:120001) has been deleted
https://codereview.chromium.org/657433002/diff/140001/testing/chromoting/brow... File testing/chromoting/browser_tests_launcher.py (right): https://codereview.chromium.org/657433002/diff/140001/testing/chromoting/brow... testing/chromoting/browser_tests_launcher.py:2: # Use of this source code is governed by a BSD-style license that can be One concern about this approach is scalability, swarming supports shard for gtest, see http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_.... It could split the big browser_tests to several tasks and run each task on different slave in parallel. I am not sure if you can use shard for your case, if not, that means the test cases will be running sequentially on one slave. With more and more test cases added, eventually we need to improve the performance. https://codereview.chromium.org/657433002/diff/140001/testing/chromoting/inte... File testing/chromoting/integration_tests.gyp (right): https://codereview.chromium.org/657433002/diff/140001/testing/chromoting/inte... testing/chromoting/integration_tests.gyp:27: './browser_tests_launcher.py', Why do you need to copy it to PRODUCT_DIR? You can add this file as dependent files in isolate file, just use relative path. https://codereview.chromium.org/657433002/diff/140001/testing/chromoting/inte... testing/chromoting/integration_tests.gyp:42: './browser_test_commands_linux.txt', Same comment with the above one.
Patchset #3 (id:160001) has been deleted
Thanks. PTAL. https://codereview.chromium.org/657433002/diff/140001/testing/chromoting/brow... File testing/chromoting/browser_tests_launcher.py (right): https://codereview.chromium.org/657433002/diff/140001/testing/chromoting/brow... testing/chromoting/browser_tests_launcher.py:2: # Use of this source code is governed by a BSD-style license that can be On 2014/10/14 22:54:21, Lei Lei wrote: > One concern about this approach is scalability, swarming supports shard for > gtest, see > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_.... > > It could split the big browser_tests to several tasks and run each task on > different slave in parallel. I am not sure if you can use shard for your case, > if not, that means the test cases will be running sequentially on one slave. > With more and more test cases added, eventually we need to improve the > performance. Acknowledged. Makes sense. I've opened https://code.google.com/p/chromium/issues/detail?id=423581 to track updating remoting browser-tests to take advantage of this. Until then, we can try this approach. https://codereview.chromium.org/657433002/diff/140001/testing/chromoting/inte... File testing/chromoting/integration_tests.gyp (right): https://codereview.chromium.org/657433002/diff/140001/testing/chromoting/inte... testing/chromoting/integration_tests.gyp:27: './browser_tests_launcher.py', On 2014/10/14 22:54:21, Lei Lei wrote: > Why do you need to copy it to PRODUCT_DIR? > > You can add this file as dependent files in isolate file, just use relative > path. Done. https://codereview.chromium.org/657433002/diff/140001/testing/chromoting/inte... testing/chromoting/integration_tests.gyp:42: './browser_test_commands_linux.txt', On 2014/10/14 22:54:21, Lei Lei wrote: > Same comment with the above one. Done.
lgtm
https://codereview.chromium.org/657433002/diff/180001/testing/chromoting/brow... File testing/chromoting/browser_tests_launcher.py (right): https://codereview.chromium.org/657433002/diff/180001/testing/chromoting/brow... testing/chromoting/browser_tests_launcher.py:21: print 'Failed to run command %s; %s' % (command, e) You should not swallow the exception, otherwise it will not make the test as failure if there is any test in browser tests failed.
anandc@chromium.org changed reviewers: + weitaosu@chromium.org
Weitao, could you PTAL? Thank you. This has been tested on a swarming slave, and works as expected. https://codereview.chromium.org/657433002/diff/180001/testing/chromoting/brow... File testing/chromoting/browser_tests_launcher.py (right): https://codereview.chromium.org/657433002/diff/180001/testing/chromoting/brow... testing/chromoting/browser_tests_launcher.py:21: print 'Failed to run command %s; %s' % (command, e) On 2014/10/15 01:09:44, Lei Lei wrote: > You should not swallow the exception, otherwise it will not make the test as > failure if there is any test in browser tests failed. Done.
On 2014/10/16 02:06:15, anandc wrote: > Weitao, could you PTAL? Thank you. > This has been tested on a swarming slave, and works as expected. > > https://codereview.chromium.org/657433002/diff/180001/testing/chromoting/brow... > File testing/chromoting/browser_tests_launcher.py (right): > > https://codereview.chromium.org/657433002/diff/180001/testing/chromoting/brow... > testing/chromoting/browser_tests_launcher.py:21: print 'Failed to run command > %s; %s' % (command, e) > On 2014/10/15 01:09:44, Lei Lei wrote: > > You should not swallow the exception, otherwise it will not make the test as > > failure if there is any test in browser tests failed. > > Done. LGTM with nits.
https://codereview.chromium.org/657433002/diff/200001/testing/chromoting/brow... File testing/chromoting/browser_tests_launcher.py (right): https://codereview.chromium.org/657433002/diff/200001/testing/chromoting/brow... testing/chromoting/browser_tests_launcher.py:31: help='path to file containing list of command to launch.') s/command/commands https://codereview.chromium.org/657433002/diff/200001/testing/chromoting/brow... testing/chromoting/browser_tests_launcher.py:33: help='build output folder, i.e., <(PRODUCT_DIR).') It doesn't have to be the build output folder. Suggested help text: the directory that contains the product and test binaries.
The CQ bit was checked by anandc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/657433002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/70111) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_chromium_gn_compile_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by anandc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/657433002/240001
Message was sent while issue was closed.
Committed patchset #6 (id:240001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/ad76c17dff74ba8a5e7446bb62840fd2e9882a59 Cr-Commit-Position: refs/heads/master@{#300156} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
