Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2341933002:
Disable all screen-capturer tests (Closed)
|
Created:
4 years, 3 months ago by Hzj_jie Modified:
4 years, 3 months ago Reviewers:
kjellander_webrtc Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionDisable all screen-capturer tests
ScreenCapturerTest.CaptureUpdatedRegion* tests are flaky, disable them first to
unblock other changes.
BUG=647067
NOTRY=True
Patch Set 1 #
Total comments: 1
Patch Set 2 : Resolve review comments #Messages
Total messages: 18 (9 generated)
Description was changed from ========== Disable all screen-capturer tests ScreenCapturerTest.CaptureUpdatedRegion* tests are flaky, disable them first to unblock other changes. BUG= ========== to ========== Disable all screen-capturer tests ScreenCapturerTest.CaptureUpdatedRegion* tests are flaky, disable them first to unblock other changes. BUG=647067 ==========
nisse@chromium.org changed reviewers: + nisse@chromium.org, sergeyu@chromium.org
I take it all of those tests are flaky? For a proper review, I suggest sergeyu@ (OWNER). If it's blocking work, you could also TBR this simple change, but perhaps coordinate with sheriff first. Looks good to me, but I'm not familiar with these tests or the code they're testing.
nisse@chromium.org changed reviewers: + nisse@webrtc.org - nisse@chromium.org
nisse@webrtc.org changed reviewers: + kjellander@webrtc.org
+kjellander@ Probably good to land this asap?
This doesn't look right to me: https://build.chromium.org/p/client.webrtc/builders/Win%20DrMemory%20Full/bui... shows that you did a change in the dr memory suppressions, which probably caused the build to fail. You claim that the tests are flaky, what are you basing that on? When I look at https://build.chromium.org/p/client.webrtc/builders/Win%20DrMemory%20Full?num... it looks very reliable. The last time they failed there were CLs changing these tests involved as well. Finally, it's not the right thing to disable the tests for all platforms if they're just failing under one configuration/tool. That's why https://chromium.googlesource.com/external/webrtc/+/master/tools/valgrind-web... should be used. If that doesn't work, you can exclude the test for dr memory only by adding them to https://chromium.googlesource.com/external/webrtc/+/master/tools/valgrind-web... instead. That way we don't lose the other test coverage. https://codereview.webrtc.org/2341933002/diff/1/webrtc/modules/desktop_captur... File webrtc/modules/desktop_capture/screen_capturer_unittest.cc (right): https://codereview.webrtc.org/2341933002/diff/1/webrtc/modules/desktop_captur... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:274: TEST_F(ScreenCapturerTest, DISABLED_CaptureUpdatedRegion) { Please add a reference to the bug in a comment above this line
On 2016/09/15 09:59:37, kjellander_webrtc wrote: > This doesn't look right to me: > https://build.chromium.org/p/client.webrtc/builders/Win%20DrMemory%20Full/bui... > shows that you did a change in the dr memory suppressions, which probably caused > the build to fail. > > You claim that the tests are flaky, what are you basing that on? When I look at > https://build.chromium.org/p/client.webrtc/builders/Win%20DrMemory%20Full?num... > it looks very reliable. The last time they failed there were CLs changing these > tests involved as well. > > Finally, it's not the right thing to disable the tests for all platforms if > they're just failing under one configuration/tool. That's why > https://chromium.googlesource.com/external/webrtc/+/master/tools/valgrind-web... > should be used. If that doesn't work, you can exclude the test for dr memory > only by adding them to > https://chromium.googlesource.com/external/webrtc/+/master/tools/valgrind-web... > instead. That way we don't lose the other test coverage. > > https://codereview.webrtc.org/2341933002/diff/1/webrtc/modules/desktop_captur... > File webrtc/modules/desktop_capture/screen_capturer_unittest.cc (right): > > https://codereview.webrtc.org/2341933002/diff/1/webrtc/modules/desktop_captur... > webrtc/modules/desktop_capture/screen_capturer_unittest.cc:274: > TEST_F(ScreenCapturerTest, DISABLED_CaptureUpdatedRegion) { > Please add a reference to the bug in a comment above this line Finally, try to remember to use NOTRY=True for a change that just disables a test. That way we save a lot of trybot resources.
On 2016/09/15 10:00:15, kjellander_webrtc wrote: > On 2016/09/15 09:59:37, kjellander_webrtc wrote: > > This doesn't look right to me: > > > https://build.chromium.org/p/client.webrtc/builders/Win%20DrMemory%20Full/bui... > > shows that you did a change in the dr memory suppressions, which probably > caused > > the build to fail. > > > > You claim that the tests are flaky, what are you basing that on? When I look > at > > > https://build.chromium.org/p/client.webrtc/builders/Win%20DrMemory%20Full?num... > > it looks very reliable. The last time they failed there were CLs changing > these > > tests involved as well. OK I can se many flakes at our win trybots: https://build.chromium.org/p/tryserver.webrtc/builders/win_dbg https://build.chromium.org/p/client.webrtc/builders/Win32%20Debug?numbuilds=200 also shows a few. So I'm fine with disabling these as long as you add a comment. lgtm > > > > Finally, it's not the right thing to disable the tests for all platforms if > > they're just failing under one configuration/tool. That's why > > > https://chromium.googlesource.com/external/webrtc/+/master/tools/valgrind-web... > > should be used. If that doesn't work, you can exclude the test for dr memory > > only by adding them to > > > https://chromium.googlesource.com/external/webrtc/+/master/tools/valgrind-web... > > instead. That way we don't lose the other test coverage. > > > > > https://codereview.webrtc.org/2341933002/diff/1/webrtc/modules/desktop_captur... > > File webrtc/modules/desktop_capture/screen_capturer_unittest.cc (right): > > > > > https://codereview.webrtc.org/2341933002/diff/1/webrtc/modules/desktop_captur... > > webrtc/modules/desktop_capture/screen_capturer_unittest.cc:274: > > TEST_F(ScreenCapturerTest, DISABLED_CaptureUpdatedRegion) { > > Please add a reference to the bug in a comment above this line > > Finally, try to remember to use NOTRY=True for a change that just disables a > test. That way we save a lot of trybot resources.
Description was changed from ========== Disable all screen-capturer tests ScreenCapturerTest.CaptureUpdatedRegion* tests are flaky, disable them first to unblock other changes. BUG=647067 ========== to ========== Disable all screen-capturer tests ScreenCapturerTest.CaptureUpdatedRegion* tests are flaky, disable them first to unblock other changes. BUG=647067 NOTRY=True ==========
On 2016/09/15 11:27:30, kjellander_webrtc wrote: > On 2016/09/15 10:00:15, kjellander_webrtc wrote: > > On 2016/09/15 09:59:37, kjellander_webrtc wrote: > > > This doesn't look right to me: > > > > > > https://build.chromium.org/p/client.webrtc/builders/Win%20DrMemory%20Full/bui... > > > shows that you did a change in the dr memory suppressions, which probably > > caused > > > the build to fail. > > > > > > You claim that the tests are flaky, what are you basing that on? When I look > > at > > > > > > https://build.chromium.org/p/client.webrtc/builders/Win%20DrMemory%20Full?num... > > > it looks very reliable. The last time they failed there were CLs changing > > these > > > tests involved as well. > > OK I can se many flakes at our win trybots: > https://build.chromium.org/p/tryserver.webrtc/builders/win_dbg > https://build.chromium.org/p/client.webrtc/builders/Win32%20Debug?numbuilds=200 > also shows a few. > So I'm fine with disabling these as long as you add a comment. > > lgtm > > > > > > > Finally, it's not the right thing to disable the tests for all platforms if > > > they're just failing under one configuration/tool. That's why > > > > > > https://chromium.googlesource.com/external/webrtc/+/master/tools/valgrind-web... > > > should be used. If that doesn't work, you can exclude the test for dr memory > > > only by adding them to > > > > > > https://chromium.googlesource.com/external/webrtc/+/master/tools/valgrind-web... > > > instead. That way we don't lose the other test coverage. > > > > > > > > > https://codereview.webrtc.org/2341933002/diff/1/webrtc/modules/desktop_captur... > > > File webrtc/modules/desktop_capture/screen_capturer_unittest.cc (right): > > > > > > > > > https://codereview.webrtc.org/2341933002/diff/1/webrtc/modules/desktop_captur... > > > webrtc/modules/desktop_capture/screen_capturer_unittest.cc:274: > > > TEST_F(ScreenCapturerTest, DISABLED_CaptureUpdatedRegion) { > > > Please add a reference to the bug in a comment above this line > > > > Finally, try to remember to use NOTRY=True for a change that just disables a > > test. That way we save a lot of trybot resources. These tests are recently added by me :) So I just realize they are flaky. I am investigating the root cause, but this change is to disable them to unblock trybots and also save me from re-submit a bunch of changes.
The CQ bit was checked by zijiehe@chromium.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/2341933002/#ps20001 (title: "Resolve review comments")
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: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/8395)
On 2016/09/15 18:02:03, Hzj_jie wrote: > On 2016/09/15 11:27:30, kjellander_webrtc wrote: > > On 2016/09/15 10:00:15, kjellander_webrtc wrote: > > > On 2016/09/15 09:59:37, kjellander_webrtc wrote: > > > > This doesn't look right to me: > > > > > > > > > > https://build.chromium.org/p/client.webrtc/builders/Win%20DrMemory%20Full/bui... > > > > shows that you did a change in the dr memory suppressions, which probably > > > caused > > > > the build to fail. > > > > > > > > You claim that the tests are flaky, what are you basing that on? When I > look > > > at > > > > > > > > > > https://build.chromium.org/p/client.webrtc/builders/Win%20DrMemory%20Full?num... > > > > it looks very reliable. The last time they failed there were CLs changing > > > these > > > > tests involved as well. > > > > OK I can se many flakes at our win trybots: > > https://build.chromium.org/p/tryserver.webrtc/builders/win_dbg > > > https://build.chromium.org/p/client.webrtc/builders/Win32%20Debug?numbuilds=200 > > also shows a few. > > So I'm fine with disabling these as long as you add a comment. > > > > lgtm > > > > > > > > > > Finally, it's not the right thing to disable the tests for all platforms > if > > > > they're just failing under one configuration/tool. That's why > > > > > > > > > > https://chromium.googlesource.com/external/webrtc/+/master/tools/valgrind-web... > > > > should be used. If that doesn't work, you can exclude the test for dr > memory > > > > only by adding them to > > > > > > > > > > https://chromium.googlesource.com/external/webrtc/+/master/tools/valgrind-web... > > > > instead. That way we don't lose the other test coverage. > > > > > > > > > > > > > > https://codereview.webrtc.org/2341933002/diff/1/webrtc/modules/desktop_captur... > > > > File webrtc/modules/desktop_capture/screen_capturer_unittest.cc (right): > > > > > > > > > > > > > > https://codereview.webrtc.org/2341933002/diff/1/webrtc/modules/desktop_captur... > > > > webrtc/modules/desktop_capture/screen_capturer_unittest.cc:274: > > > > TEST_F(ScreenCapturerTest, DISABLED_CaptureUpdatedRegion) { > > > > Please add a reference to the bug in a comment above this line > > > > > > Finally, try to remember to use NOTRY=True for a change that just disables a > > > test. That way we save a lot of trybot resources. > > These tests are recently added by me :) So I just realize they are flaky. I am > investigating the root cause, but this change is to disable them to unblock > trybots and also save me from re-submit a bunch of changes. Stefan already landed a copy of this CL as https://codereview.webrtc.org/2342823002 to get our bots back into a stable state.
Message was sent while issue was closed.
zijiehe@chromium.org changed reviewers: - kjellander@webrtc.org, nisse@webrtc.org, sergeyu@chromium.org |
