Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2503503002:
MB: Run test with gtest-parallel on swarming. (Closed)
|
Created:
4 years, 1 month ago by ehmaldonado_webrtc Modified:
4 years, 1 month ago CC:
webrtc-reviews_webrtc.org Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionMB: Run test with gtest-parallel on swarming.
TBR=pbos@webrtc.org
BUG=chromium:497757, chromium:664425
NOTRY=True
Committed: https://crrev.com/b2fcf6d96f4efdc7a606c1cc0f999932c63226d4
Cr-Commit-Position: refs/heads/master@{#15093}
Patch Set 1 #Patch Set 2 : Modify gtest-parallel python magic line. #
Total comments: 1
Patch Set 3 : Seems to work now :) #
Total comments: 2
Patch Set 4 : Fixed some bugs. #
Total comments: 4
Patch Set 5 : Addressed comments. #
Total comments: 1
Patch Set 6 : Removed extra whitespace. #
Total comments: 3
Messages
Total messages: 36 (17 generated)
Description was changed from ========== Run test on gtest-parallel. BUG= ========== to ========== MB: Run test with gtest-parallel on swarming. BUG=chromium:497757 ==========
ehmaldonado@webrtc.org changed reviewers: + kjellander@webrtc.org
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
gtest-parallel doesn't seem to like the ./ syntax for the test binary. Maybe it needs to be updated somehow. I imagine the Chromium scripts have a way to handle that so maybe you can just look at how they do that. Otherwise we'll have to have a separate if-condition for Windows in this script (less preferred). Finally, we should get rid of both Chromium-specific flags, then we can finally remove the "ignore-unknown-flags" thing we have for our gtests. https://codereview.webrtc.org/2503503002/diff/60001/tools/mb/mb.py File tools/mb/mb.py (right): https://codereview.webrtc.org/2503503002/diff/60001/tools/mb/mb.py#newcode1119 tools/mb/mb.py:1119: '--test-launcher-bot-mode', We should get rid of --test-launcher-bot-mode too, since it's a Chromium-specific thing. Same below.
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
Description was changed from ========== MB: Run test with gtest-parallel on swarming. BUG=chromium:497757 ========== to ========== MB: Run test with gtest-parallel on swarming. BUG=chromium:497757, chromium:664425 ==========
lgtm with two comments Seems like this cuts down win_rel execution from 10-15 minutes to 5! https://codereview.webrtc.org/2503503002/diff/120001/third_party/gtest-parall... File third_party/gtest-parallel/gtest-parallel (right): https://codereview.webrtc.org/2503503002/diff/120001/third_party/gtest-parall... third_party/gtest-parallel/gtest-parallel:1: #!/usr/bin/env python I guess you won't need this now? https://codereview.webrtc.org/2503503002/diff/120001/tools/mb/mb.py File tools/mb/mb.py (right): https://codereview.webrtc.org/2503503002/diff/120001/tools/mb/mb.py#newcode1140 tools/mb/mb.py:1140: elif test_type == 'gpu_browser_test': Let's remove this type
On 2016/11/15 10:29:39, kjellander_webrtc wrote: > lgtm with two comments > > Seems like this cuts down win_rel execution from 10-15 minutes to 5! > > https://codereview.webrtc.org/2503503002/diff/120001/third_party/gtest-parall... > File third_party/gtest-parallel/gtest-parallel (right): > > https://codereview.webrtc.org/2503503002/diff/120001/third_party/gtest-parall... > third_party/gtest-parallel/gtest-parallel:1: #!/usr/bin/env python > I guess you won't need this now? > > https://codereview.webrtc.org/2503503002/diff/120001/tools/mb/mb.py > File tools/mb/mb.py (right): > > https://codereview.webrtc.org/2503503002/diff/120001/tools/mb/mb.py#newcode1140 > tools/mb/mb.py:1140: elif test_type == 'gpu_browser_test': > Let's remove this type Oh, I changed my mind. We need to make sure webrtc_nonparallel_tests does not execute with the parallel script, so you need to change the test type for that one. not lgtm
Patchset #4 (id:140001) has been deleted
On 2016/11/15 10:31:16, kjellander_webrtc wrote: > On 2016/11/15 10:29:39, kjellander_webrtc wrote: > > lgtm with two comments > > > > Seems like this cuts down win_rel execution from 10-15 minutes to 5! > > > > > https://codereview.webrtc.org/2503503002/diff/120001/third_party/gtest-parall... > > File third_party/gtest-parallel/gtest-parallel (right): > > > > > https://codereview.webrtc.org/2503503002/diff/120001/third_party/gtest-parall... > > third_party/gtest-parallel/gtest-parallel:1: #!/usr/bin/env python > > I guess you won't need this now? > > > > https://codereview.webrtc.org/2503503002/diff/120001/tools/mb/mb.py > > File tools/mb/mb.py (right): > > > > > https://codereview.webrtc.org/2503503002/diff/120001/tools/mb/mb.py#newcode1140 > > tools/mb/mb.py:1140: elif test_type == 'gpu_browser_test': > > Let's remove this type > > Oh, I changed my mind. We need to make sure webrtc_nonparallel_tests does not > execute with the parallel script, so you need to change the test type for that > one. > not lgtm ptal
It would be ideal if we could have the definition of the test_types in a local file, somewhat like the isolate_map.pyl file, so we could benefit of the changes they make to the mb script.
On 2016/11/15 14:16:01, ehmaldonado_webrtc wrote: > It would be ideal if we could have the definition of the test_types in a local > file, somewhat like the isolate_map.pyl file, so we could benefit of the changes > they make to the mb script. Yeah, but that would call for refactoring upstream first. I think Dirk would happily accept contributions to it, since it sounded like he was mostly planning to rip out the GYP support right now.
https://codereview.webrtc.org/2503503002/diff/160001/third_party/gtest-parall... File third_party/gtest-parallel/gtest-parallel (right): https://codereview.webrtc.org/2503503002/diff/160001/third_party/gtest-parall... third_party/gtest-parallel/gtest-parallel:324: shard_count = int(os.environ.get('GTEST_TOTAL_SHARDS', 1)) Since this is both ugly and hard to know, could we add two flags instead, where the default values are the environment variable's values, with fallback on 1 and 0? Like: parser.add_option('--shard-count', type='int', default=int(os.environ.get('GTEST_TOTAL_SHARDS', 1)), help='Total number of shards (for sharding test execution between multiple machines). Default: %default') parser.add_option('--shard-run', type='int', default=int(os.environ.get('GTEST_SHARD_INDEX', 0)), help='Zero-indexed number identifying this shard (for sharding test execution between multiple machines). Default: %default') Then use these flags here instead. https://codereview.webrtc.org/2503503002/diff/160001/tools/mb/mb.py File tools/mb/mb.py (right): https://codereview.webrtc.org/2503503002/diff/160001/tools/mb/mb.py#newcode1134 tools/mb/mb.py:1134: executable_preffix + str(executable) + executable_suffix, executable_preffix -> executable_prefix https://codereview.webrtc.org/2503503002/diff/160001/tools/mb/mb.py#newcode1146 tools/mb/mb.py:1146: './' + str(executable) + executable_suffix, This line should be: executable_preffix + str(executable) + executable_suffix, Followed by a '--', entry on the next line. Ideally we'd store these 5 lines in a variable and re-use that one for the test types that have the same code repeated. https://codereview.webrtc.org/2503503002/diff/160001/webrtc/build/gn_isolate_... File webrtc/build/gn_isolate_map.pyl (right): https://codereview.webrtc.org/2503503002/diff/160001/webrtc/build/gn_isolate_... webrtc/build/gn_isolate_map.pyl:96: "type": "non_parallel", non_parallel_console_test_launcher to make it more clear that it's almost identical to the console_test_launcher
Patchset #5 (id:180001) has been deleted
On 2016/11/15 15:13:37, kjellander_webrtc wrote: > https://codereview.webrtc.org/2503503002/diff/160001/third_party/gtest-parall... > File third_party/gtest-parallel/gtest-parallel (right): > > https://codereview.webrtc.org/2503503002/diff/160001/third_party/gtest-parall... > third_party/gtest-parallel/gtest-parallel:324: shard_count = > int(os.environ.get('GTEST_TOTAL_SHARDS', 1)) > Since this is both ugly and hard to know, could we add two flags instead, where > the default values are the environment variable's values, with fallback on 1 and > 0? > > Like: > parser.add_option('--shard-count', type='int', > default=int(os.environ.get('GTEST_TOTAL_SHARDS', 1)), > help='Total number of shards (for sharding test execution between multiple > machines). Default: %default') > parser.add_option('--shard-run', type='int', > default=int(os.environ.get('GTEST_SHARD_INDEX', 0)), > help='Zero-indexed number identifying this shard (for sharding test execution > between multiple machines). Default: %default') > > Then use these flags here instead. > > https://codereview.webrtc.org/2503503002/diff/160001/tools/mb/mb.py > File tools/mb/mb.py (right): > > https://codereview.webrtc.org/2503503002/diff/160001/tools/mb/mb.py#newcode1134 > tools/mb/mb.py:1134: executable_preffix + str(executable) + executable_suffix, > executable_preffix -> executable_prefix > > https://codereview.webrtc.org/2503503002/diff/160001/tools/mb/mb.py#newcode1146 > tools/mb/mb.py:1146: './' + str(executable) + executable_suffix, > This line should be: > executable_preffix + str(executable) + executable_suffix, > Followed by a '--', entry on the next line. > > Ideally we'd store these 5 lines in a variable and re-use that one for the test > types that have the same code repeated. > > https://codereview.webrtc.org/2503503002/diff/160001/webrtc/build/gn_isolate_... > File webrtc/build/gn_isolate_map.pyl (right): > > https://codereview.webrtc.org/2503503002/diff/160001/webrtc/build/gn_isolate_... > webrtc/build/gn_isolate_map.pyl:96: "type": "non_parallel", > non_parallel_console_test_launcher to make it more clear that it's almost > identical to the console_test_launcher PTAL
lgtm! https://codereview.webrtc.org/2503503002/diff/200001/third_party/gtest-parall... File third_party/gtest-parallel/gtest-parallel (right): https://codereview.webrtc.org/2503503002/diff/200001/third_party/gtest-parall... third_party/gtest-parallel/gtest-parallel:276: ' sharding test execution between multiple machines). ' Remove the extra space at the beginning of this line.
The CQ bit was checked by ehmaldonado@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/2503503002/#ps220001 (title: "Removed extra whitespace.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/18633) win_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_dbg/builds/8321)
Description was changed from ========== MB: Run test with gtest-parallel on swarming. BUG=chromium:497757, chromium:664425 ========== to ========== MB: Run test with gtest-parallel on swarming. TBR=pbos@webrtc.org BUG=chromium:497757, chromium:664425 NOTRY=True ==========
The CQ bit was checked by ehmaldonado@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== MB: Run test with gtest-parallel on swarming. TBR=pbos@webrtc.org BUG=chromium:497757, chromium:664425 NOTRY=True ========== to ========== MB: Run test with gtest-parallel on swarming. TBR=pbos@webrtc.org BUG=chromium:497757, chromium:664425 NOTRY=True ==========
Message was sent while issue was closed.
Committed patchset #6 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== MB: Run test with gtest-parallel on swarming. TBR=pbos@webrtc.org BUG=chromium:497757, chromium:664425 NOTRY=True ========== to ========== MB: Run test with gtest-parallel on swarming. TBR=pbos@webrtc.org BUG=chromium:497757, chromium:664425 NOTRY=True Committed: https://crrev.com/b2fcf6d96f4efdc7a606c1cc0f999932c63226d4 Cr-Commit-Position: refs/heads/master@{#15093} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/b2fcf6d96f4efdc7a606c1cc0f999932c63226d4 Cr-Commit-Position: refs/heads/master@{#15093}
Message was sent while issue was closed.
pbos@webrtc.org changed reviewers: + pbos@webrtc.org
Message was sent while issue was closed.
https://codereview.webrtc.org/2503503002/diff/220001/third_party/gtest-parall... File third_party/gtest-parallel/gtest-parallel (right): https://codereview.webrtc.org/2503503002/diff/220001/third_party/gtest-parall... third_party/gtest-parallel/gtest-parallel:269: parser.add_option('--shard-count', type='int', --shard_count to match other flags https://codereview.webrtc.org/2503503002/diff/220001/third_party/gtest-parall... third_party/gtest-parallel/gtest-parallel:273: parser.add_option('--shard-index', type='int', shard_index https://codereview.webrtc.org/2503503002/diff/220001/third_party/gtest-parall... third_party/gtest-parallel/gtest-parallel:275: help=('Zero-indexed number identifying this shard (for ' Add a validator that this is <= shard-count. I think you can replace type with a callable instead of int
Message was sent while issue was closed.
On 2016/11/16 17:33:10, pbos-webrtc wrote: > https://codereview.webrtc.org/2503503002/diff/220001/third_party/gtest-parall... > File third_party/gtest-parallel/gtest-parallel (right): > > https://codereview.webrtc.org/2503503002/diff/220001/third_party/gtest-parall... > third_party/gtest-parallel/gtest-parallel:269: > parser.add_option('--shard-count', type='int', > --shard_count to match other flags > > https://codereview.webrtc.org/2503503002/diff/220001/third_party/gtest-parall... > third_party/gtest-parallel/gtest-parallel:273: > parser.add_option('--shard-index', type='int', > shard_index > > https://codereview.webrtc.org/2503503002/diff/220001/third_party/gtest-parall... > third_party/gtest-parallel/gtest-parallel:275: help=('Zero-indexed number > identifying this shard (for ' > Add a validator that this is <= shard-count. I think you can replace type with a > callable instead of int Please always do changes to gtest-parallel upstream first, they should never be out of sync. I'll gladly take a pull request for this feature.
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:220001) has been created in https://codereview.webrtc.org/2507263003/ by ehmaldonado@webrtc.org. The reason for reverting is: There might be a bug where only failures in the first shard are reported..
Message was sent while issue was closed.
On 2016/11/16 23:46:46, ehmaldonado_webrtc wrote: > A revert of this CL (patchset #6 id:220001) has been created in > https://codereview.webrtc.org/2507263003/ by mailto:ehmaldonado@webrtc.org. > > The reason for reverting is: There might be a bug where only failures in the > first shard are reported.. Figured out the bug, will land the fix in: https://codereview.webrtc.org/2505093003/ And sorry for carrying it more out of sync. A pull request is on the way :) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
