Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2686563002:
Pass isolate output dir to gtest-parallel-wrapper.py (Closed)
|
Created:
3 years, 10 months ago by ehmaldonado_webrtc Modified:
3 years, 10 months ago Reviewers:
kjellander_webrtc CC:
webrtc-reviews_webrtc.org, qiang.lu, niklas.enbom, yujie_mao (webrtc), peah-webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionPass isolate output dir to gtest-parallel-wrapper.py
This makes it possible to archive all test execution logs on swarming jobs.
R=kjellander@webrtc.org
BUG=webrtc:7086
NOTRY=True
Review-Url: https://codereview.webrtc.org/2686563002
Cr-Commit-Position: refs/heads/master@{#16574}
Committed: https://chromium.googlesource.com/external/webrtc/+/5583384048bc33c0fae3c8c47346de668262d9f5
Patch Set 1 #Patch Set 2 : Fix windows. #
Total comments: 5
Patch Set 3 : Don't pass timeout. #
Messages
Total messages: 21 (9 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Roll gtest-parallel 8768563f..515269a4 R=kjellander@webrtc.org BUG=None ========== to ========== Roll gtest-parallel 8768563f..515269a4 and add timeout. Add a timeout flag so that the tests that take too long to run are interrupted and their names are displayed. R=kjellander@webrtc.org BUG=webrtc:7086 ==========
Hmm, at some point we're going to have to move things out of here into the build recipes. I guess we'll have to move them into our source repo first though. https://codereview.webrtc.org/2686563002/diff/40001/tools-webrtc/mb/mb.py File tools-webrtc/mb/mb.py (right): https://codereview.webrtc.org/2686563002/diff/40001/tools-webrtc/mb/mb.py#new... tools-webrtc/mb/mb.py:1108: sep = '\\' if self.platform == 'win32' else '/' Just use os.sep instead? https://docs.python.org/2/library/os.html#os.sep https://codereview.webrtc.org/2686563002/diff/40001/tools-webrtc/mb/mb.py#new... tools-webrtc/mb/mb.py:1113: '--timeout=900', # 15 min. I think we should make this more visible (but keeping it in mb.py will still hide it quite well, I'm not sure what we should do about that). Could you at least just extract it to a constant at the top of the file, like TEST_TIMEOUT?
> https://codereview.webrtc.org/2686563002/diff/40001/tools-webrtc/mb/mb.py#new... > tools-webrtc/mb/mb.py:1113: '--timeout=900', # 15 min. > I think we should make this more visible (but keeping it in mb.py will still > hide it quite well, I'm not sure what we should do about that). > > Could you at least just extract it to a constant at the top of the file, like > TEST_TIMEOUT? We could move it to the recipes, I think.
On 2017/02/07 22:03:51, ehmaldonado_webrtc wrote: > > > https://codereview.webrtc.org/2686563002/diff/40001/tools-webrtc/mb/mb.py#new... > > tools-webrtc/mb/mb.py:1113: '--timeout=900', # 15 min. > > I think we should make this more visible (but keeping it in mb.py will still > > hide it quite well, I'm not sure what we should do about that). > > > > Could you at least just extract it to a constant at the top of the file, like > > TEST_TIMEOUT? > > We could move it to the recipes, I think. Hmm, how would that look like?
On 2017/02/08 09:07:15, kjellander_webrtc wrote: > On 2017/02/07 22:03:51, ehmaldonado_webrtc wrote: > > > > > > https://codereview.webrtc.org/2686563002/diff/40001/tools-webrtc/mb/mb.py#new... > > > tools-webrtc/mb/mb.py:1113: '--timeout=900', # 15 min. > > > I think we should make this more visible (but keeping it in mb.py will still > > > hide it quite well, I'm not sure what we should do about that). > > > > > > Could you at least just extract it to a constant at the top of the file, > like > > > TEST_TIMEOUT? > > > > We could move it to the recipes, I think. > > Hmm, how would that look like? I think it would look like adding args=['--timeout=900'] here https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/webrtc/st... I also think the --output_dir flag makes more sense in mb.py
On 2017/02/08 13:18:04, ehmaldonado_webrtc wrote: > On 2017/02/08 09:07:15, kjellander_webrtc wrote: > > On 2017/02/07 22:03:51, ehmaldonado_webrtc wrote: > > > > > > > > > > https://codereview.webrtc.org/2686563002/diff/40001/tools-webrtc/mb/mb.py#new... > > > > tools-webrtc/mb/mb.py:1113: '--timeout=900', # 15 min. > > > > I think we should make this more visible (but keeping it in mb.py will > still > > > > hide it quite well, I'm not sure what we should do about that). > > > > > > > > Could you at least just extract it to a constant at the top of the file, > > like > > > > TEST_TIMEOUT? > > > > > > We could move it to the recipes, I think. > > > > Hmm, how would that look like? > > I think it would look like adding > args=['--timeout=900'] > here > https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/webrtc/st... Hmm, I don't recall how mb.py interacts with the recipe. Can you make a buildbot CL as well? I'm sure you're right and that would be preferred of course (rather than hardcoding the timeout in here). > I also think the --output_dir flag makes more sense in mb.py I agree --output_dir can stay here.
PTAL https://codereview.webrtc.org/2686563002/diff/40001/tools-webrtc/mb/mb.py File tools-webrtc/mb/mb.py (right): https://codereview.webrtc.org/2686563002/diff/40001/tools-webrtc/mb/mb.py#new... tools-webrtc/mb/mb.py:1108: sep = '\\' if self.platform == 'win32' else '/' On 2017/02/07 20:41:39, kjellander_webrtc wrote: > Just use os.sep instead? https://docs.python.org/2/library/os.html#os.sep I guess the platform for which the isolate files are generated need not match the one on which mb is executed. For example, in the unittest we test the output for both linux and windows, so we might be testing the case self.platform = 'win32' on linux. https://codereview.webrtc.org/2686563002/diff/40001/tools-webrtc/mb/mb.py#new... tools-webrtc/mb/mb.py:1113: '--timeout=900', # 15 min. On 2017/02/07 20:41:39, kjellander_webrtc wrote: > I think we should make this more visible (but keeping it in mb.py will still > hide it quite well, I'm not sure what we should do about that). > > Could you at least just extract it to a constant at the top of the file, like > TEST_TIMEOUT? Acknowledged.
Description was changed from ========== Roll gtest-parallel 8768563f..515269a4 and add timeout. Add a timeout flag so that the tests that take too long to run are interrupted and their names are displayed. R=kjellander@webrtc.org BUG=webrtc:7086 ========== to ========== Pass isolate output dir to gtest-parallel-wrapper.py This makes it possible to archive all test execution logs on swarming jobs. R=kjellander@webrtc.org BUG=webrtc:7086 ==========
lgtm I updated your CL description since this is now only covering the --output_dir flag. Will you make another CL that updates our DEPS entry to use the new mirror at https://chromium.googlesource.com/external/github.com/google/gtest-parallel ? I guess we'll still keep our own repo at https://chromium.googlesource.com/external/webrtc/deps/third_party/gtest-para... due to the gtest-parallel-wrapper.py? Maybe it makes sense to move that file into our main repo's tools-webrtc dir instead? That way we can retire that DEPS entry. https://codereview.webrtc.org/2686563002/diff/40001/tools-webrtc/mb/mb.py File tools-webrtc/mb/mb.py (right): https://codereview.webrtc.org/2686563002/diff/40001/tools-webrtc/mb/mb.py#new... tools-webrtc/mb/mb.py:1108: sep = '\\' if self.platform == 'win32' else '/' On 2017/02/10 17:50:27, ehmaldonado_webrtc wrote: > On 2017/02/07 20:41:39, kjellander_webrtc wrote: > > Just use os.sep instead? https://docs.python.org/2/library/os.html#os.sep > > I guess the platform for which the isolate files are generated need not match > the one on which mb is executed. > For example, in the unittest we test the output for both linux and windows, so > we might be testing the case self.platform = 'win32' on linux. Ah, you're right. I thought only to check this file, in which it's initialized to sys.platform at line 53. But you're right, it's overridden in the test. Maybe add a comment above to mention it's because of testability then.
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/...
Description was changed from ========== Pass isolate output dir to gtest-parallel-wrapper.py This makes it possible to archive all test execution logs on swarming jobs. R=kjellander@webrtc.org BUG=webrtc:7086 ========== to ========== Pass isolate output dir to gtest-parallel-wrapper.py This makes it possible to archive all test execution logs on swarming jobs. R=kjellander@webrtc.org BUG=webrtc:7086 NOTRY=True ==========
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_msan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/17639)
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/...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486986931122590,
"parent_rev": "1771a3ca0f3f751e92077a7c3347a1c3ac7eb1f9", "commit_rev":
"5583384048bc33c0fae3c8c47346de668262d9f5"}
Message was sent while issue was closed.
Description was changed from ========== Pass isolate output dir to gtest-parallel-wrapper.py This makes it possible to archive all test execution logs on swarming jobs. R=kjellander@webrtc.org BUG=webrtc:7086 NOTRY=True ========== to ========== Pass isolate output dir to gtest-parallel-wrapper.py This makes it possible to archive all test execution logs on swarming jobs. R=kjellander@webrtc.org BUG=webrtc:7086 NOTRY=True Review-Url: https://codereview.webrtc.org/2686563002 Cr-Commit-Position: refs/heads/master@{#16574} Committed: https://chromium.googlesource.com/external/webrtc/+/5583384048bc33c0fae3c8c47... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/5583384048bc33c0fae3c8c47... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
