Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2492723002:
Add more logging in ScreenCapturerIntegrationTest (Closed)
DescriptionAdd more logging in ScreenCapturerIntegrationTest
ScreenCapturerIntegrationTest is flaky on Windows systems due to some unknown
reason. But it's do easily impacted by the environment, so this change adds more
logging (entire screenshot) to help debugging.
Meanwhile, this change also includes a nice-to-have change in ScreenDrawerWin to
always bring the window to front in each WaitForPendingDraws() function call. I
cannot quite tell whether this change can help to resolve the issue, but it is
worth trying.
BUG=webrtc:6666
Committed: https://crrev.com/2184155782297969eaa2992aca270cb67efaa4e2
Cr-Commit-Position: refs/heads/master@{#15158}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Resolve review comments #
Total comments: 5
Patch Set 3 : Resolve review comments #
Messages
Total messages: 60 (51 generated)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10012)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10102)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10243)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: android_compile_mips_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_mips_db...) mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/16304)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/13218) win_x64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/19738)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #3 (id:100001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Patchset #2 (id:80001) has been deleted
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/18906)
Description was changed from ========== Enable tests Debug output BUG= ========== to ========== Add more logging in ScreenCapturerIntegrationTest ScreenCapturerIntegrationTest is flaky on Windows systems due to some unknown reason. But it's do easily impacted by the environment, so this change adds more logging (entire screenshot) to help debugging. Meanwhile, this change also includes a nice-to-have change in ScreenDrawerWin to always bring the window to front in each WaitForPendingDraws() function call. I cannot quite tell whether this change can help to resolve the issue, but it is worth trying. BUG=webrtc:6666 ==========
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Patchset #1 (id:120001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
zijiehe@chromium.org changed reviewers: + jamiewalch@chromium.org, kjellander@webrtc.org, sergeyu@chromium.org
LGTM, but please see my suggestions https://codereview.webrtc.org/2492723002/diff/140001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/screen_capturer_integration_test.cc (right): https://codereview.webrtc.org/2492723002/diff/140001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/screen_capturer_integration_test.cc:124: return; Not sure why we need to return result here? The caller could use ASSERT_NO_FATAL_FAILURE() to verify that there are no failures. https://codereview.webrtc.org/2492723002/diff/140001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/screen_capturer_integration_test.cc:223: // LOG and ASSERT won't output a long string, so we can only use What do you mean by "long string"? It looks like |result| is split into multiple lines. I think LOG(LS_INFO) should work here.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2492723002/diff/140001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/screen_capturer_integration_test.cc (right): https://codereview.webrtc.org/2492723002/diff/140001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/screen_capturer_integration_test.cc:124: return; On 2016/11/18 01:21:42, Sergey Ulanov wrote: > Not sure why we need to return result here? The caller could use > ASSERT_NO_FATAL_FAILURE() to verify that there are no failures. Ah, it's simply because I do not know this function. https://codereview.webrtc.org/2492723002/diff/140001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/screen_capturer_integration_test.cc:223: // LOG and ASSERT won't output a long string, so we can only use On 2016/11/18 01:21:42, Sergey Ulanov wrote: > What do you mean by "long string"? It looks like |result| is split into multiple > lines. > I think LOG(LS_INFO) should work here. Done.
lgtm with some comments https://codereview.webrtc.org/2492723002/diff/160001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/screen_capturer_integration_test.cc (right): https://codereview.webrtc.org/2492723002/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/screen_capturer_integration_test.cc:218: // Split the entire string (can be over 4M) into several lines to How large can this be? The swarming trybots have a limit of 16MB of logs, so going above that will cause tests to fail (and I'm not sure we'll even get these lines logged). There's a special directory we can pass to the script where output files that shall be archived can be written. Ideally we'll make use of that and write the images in their original format to that dir instead. For that we need to have a flag that makes the directory available for the test. I filed https://bugs.chromium.org/p/webrtc/issues/detail?id=6732 for implementing that, so you might want to wait for that (or look at implementing it yourself). That said, since this is only written upon failure, we could try this approach first either way. https://codereview.webrtc.org/2492723002/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/screen_capturer_integration_test.cc:228: ASSERT_TRUE(false) << "ScreenCapturerIntegrationTest is flaky. " Maybe it's a bit too strong of a claim to say that it's flaky when it fails. I mean it could also fail due to a code change, right? I'd suggest rephrasing to something that tells it might be flaky (since it has been in the past).
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2492723002/diff/160001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/screen_capturer_integration_test.cc (right): https://codereview.webrtc.org/2492723002/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/screen_capturer_integration_test.cc:218: // Split the entire string (can be over 4M) into several lines to On 2016/11/18 08:47:23, kjellander_webrtc wrote: > How large can this be? The swarming trybots have a limit of 16MB of logs, so > going above that will cause tests to fail (and I'm not sure we'll even get these > lines logged). > > There's a special directory we can pass to the script where output files that > shall be archived can be written. > Ideally we'll make use of that and write the images in their original format to > that dir instead. For that we need to have a flag that makes the directory > available for the test. > I filed https://bugs.chromium.org/p/webrtc/issues/detail?id=6732 for > implementing that, so you might want to wait for that (or look at implementing > it yourself). > > That said, since this is only written upon failure, we could try this approach > first either way. On our trybots, it should be 4M. Since the test will fail immediately after one logging, it should be OK. I do recall LOG(LS_INFO) will be ignored by trybot, so I used std::out in the first iteration. And also, printf / std::out are widely used in other tests, such as https://goo.gl/JNYVah and https://goo.gl/VFVaAj. So I would prefer to revert the change, and use std::out. I am happy with the change, and I believe we can even go further. For example, in ScreenCapturerIntegrationTest, a compression is really welcome. But we can do two things simultaneous, I would prefer to submit this change and a following change to enable ScreenCapturerIntegrationTests again, and see if we can get a valuable screenshot. Meanwhile, we can work on the tools for test cases to store large data. Sounds good? https://codereview.webrtc.org/2492723002/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/screen_capturer_integration_test.cc:228: ASSERT_TRUE(false) << "ScreenCapturerIntegrationTest is flaky. " On 2016/11/18 08:47:23, kjellander_webrtc wrote: > Maybe it's a bit too strong of a claim to say that it's flaky when it fails. I > mean it could also fail due to a code change, right? I'd suggest rephrasing to > something that tells it might be flaky (since it has been in the past). Updated.
The CQ bit was checked by zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org, kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/2492723002/#ps180001 (title: "Resolve review comments")
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 ========== Add more logging in ScreenCapturerIntegrationTest ScreenCapturerIntegrationTest is flaky on Windows systems due to some unknown reason. But it's do easily impacted by the environment, so this change adds more logging (entire screenshot) to help debugging. Meanwhile, this change also includes a nice-to-have change in ScreenDrawerWin to always bring the window to front in each WaitForPendingDraws() function call. I cannot quite tell whether this change can help to resolve the issue, but it is worth trying. BUG=webrtc:6666 ========== to ========== Add more logging in ScreenCapturerIntegrationTest ScreenCapturerIntegrationTest is flaky on Windows systems due to some unknown reason. But it's do easily impacted by the environment, so this change adds more logging (entire screenshot) to help debugging. Meanwhile, this change also includes a nice-to-have change in ScreenDrawerWin to always bring the window to front in each WaitForPendingDraws() function call. I cannot quite tell whether this change can help to resolve the issue, but it is worth trying. BUG=webrtc:6666 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Add more logging in ScreenCapturerIntegrationTest ScreenCapturerIntegrationTest is flaky on Windows systems due to some unknown reason. But it's do easily impacted by the environment, so this change adds more logging (entire screenshot) to help debugging. Meanwhile, this change also includes a nice-to-have change in ScreenDrawerWin to always bring the window to front in each WaitForPendingDraws() function call. I cannot quite tell whether this change can help to resolve the issue, but it is worth trying. BUG=webrtc:6666 ========== to ========== Add more logging in ScreenCapturerIntegrationTest ScreenCapturerIntegrationTest is flaky on Windows systems due to some unknown reason. But it's do easily impacted by the environment, so this change adds more logging (entire screenshot) to help debugging. Meanwhile, this change also includes a nice-to-have change in ScreenDrawerWin to always bring the window to front in each WaitForPendingDraws() function call. I cannot quite tell whether this change can help to resolve the issue, but it is worth trying. BUG=webrtc:6666 Committed: https://crrev.com/2184155782297969eaa2992aca270cb67efaa4e2 Cr-Commit-Position: refs/heads/master@{#15158} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/2184155782297969eaa2992aca270cb67efaa4e2 Cr-Commit-Position: refs/heads/master@{#15158}
Message was sent while issue was closed.
Just comments. https://codereview.webrtc.org/2492723002/diff/160001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/screen_capturer_integration_test.cc (right): https://codereview.webrtc.org/2492723002/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/screen_capturer_integration_test.cc:218: // Split the entire string (can be over 4M) into several lines to On 2016/11/18 22:07:25, Hzj_jie wrote: > On 2016/11/18 08:47:23, kjellander_webrtc wrote: > > How large can this be? The swarming trybots have a limit of 16MB of logs, so > > going above that will cause tests to fail (and I'm not sure we'll even get > these > > lines logged). > > > > There's a special directory we can pass to the script where output files that > > shall be archived can be written. > > Ideally we'll make use of that and write the images in their original format > to > > that dir instead. For that we need to have a flag that makes the directory > > available for the test. > > I filed https://bugs.chromium.org/p/webrtc/issues/detail?id=6732 for > > implementing that, so you might want to wait for that (or look at implementing > > it yourself). > > > > That said, since this is only written upon failure, we could try this approach > > first either way. > > On our trybots, it should be 4M. Since the test will fail immediately after one > logging, it should be OK. The running test case fails, yes, but the remaining test methods are still executed, which might use the same debug logging upon additional failures, since TestCaptureUpdatedRegion is used by multiple test cases. > I do recall LOG(LS_INFO) will be ignored by trybot, so I used std::out in the > first iteration. And also, printf / std::out are widely used in other tests, > such as https://goo.gl/JNYVah and https://goo.gl/VFVaAj. So I would prefer to > revert the change, and use std::out. > > I am happy with the change, and I believe we can even go further. For example, > in ScreenCapturerIntegrationTest, a compression is really welcome. > > But we can do two things simultaneous, I would prefer to submit this change and > a following change to enable ScreenCapturerIntegrationTests again, and see if we > can get a valuable screenshot. Meanwhile, we can work on the tools for test > cases to store large data. > > Sounds good? Since it's only printed on failures, it's OK. Let's see how it goes. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
