Add more logging in ScreenCapturerIntegrationTest

webrtc/modules/desktop_capture/screen_capturer_integration_test.cc

Index: webrtc/modules/desktop_capture/screen_capturer_integration_test.cc
diff --git a/webrtc/modules/desktop_capture/screen_capturer_integration_test.cc b/webrtc/modules/desktop_capture/screen_capturer_integration_test.cc
index 64fdd4a08468b15916934db0f57aeb88e915e267..53ba469946e9279c7d4fff99df35a59489b3e730 100644
--- a/webrtc/modules/desktop_capture/screen_capturer_integration_test.cc
+++ b/webrtc/modules/desktop_capture/screen_capturer_integration_test.cc
@@ -15,6 +15,8 @@
 #include <memory>
 #include <utility>
 
+// TODO(zijiehe): Remove once flaky has been resolved.
+#include "webrtc/base/base64.h"
 #include "webrtc/base/checks.h"
 #include "webrtc/base/constructormagic.h"
 #include "webrtc/base/logging.h"
@@ -116,7 +118,9 @@ class ScreenCapturerIntegrationTest : public testing::Test {
         RgbaColor color((c == 0 ? (i & 0xff) : 0x7f),
                         (c == 1 ? (i & 0xff) : 0x7f),
                         (c == 2 ? (i & 0xff) : 0x7f));
-        TestCaptureOneFrame(capturers, drawer.get(), rect, color);
+        // Fail fast.
+        ASSERT_NO_FATAL_FAILURE(
+            TestCaptureOneFrame(capturers, drawer.get(), rect, color));
       }
 
       // A variable-size rectangle.
@@ -126,7 +130,9 @@ class ScreenCapturerIntegrationTest : public testing::Test {
         RgbaColor color((c == 0 ? (i & 0xff) : 0x7f),
                         (c == 1 ? (i & 0xff) : 0x7f),
                         (c == 2 ? (i & 0xff) : 0x7f));
-        TestCaptureOneFrame(capturers, drawer.get(), rect, color);
+        // Fail fast.
+        ASSERT_NO_FATAL_FAILURE(
+            TestCaptureOneFrame(capturers, drawer.get(), rect, color));
       }
     }
   }
@@ -183,14 +189,14 @@ class ScreenCapturerIntegrationTest : public testing::Test {
       for (size_t j = 0; j < capturers.size(); j++) {
         if (capturers[j] == nullptr) {
           // DesktopCapturer should return an empty updated_region() if no
-          // update detected. So we won't test it again if it has captured
-          // the rectangle we drew.
+          // update detected. So we won't test it again if it has captured the
+          // rectangle we drew.
           continue;
         }
         std::unique_ptr<DesktopFrame> frame = CaptureFrame(capturers[j]);
         if (!frame) {
-          // CaptureFrame() has triggered an assertion failure already, we
-          // only need to return here.
+          // CaptureFrame() has triggered an assertion failure already, we only
+          // need to return here.
           return;
         }
 
@@ -199,6 +205,32 @@ class ScreenCapturerIntegrationTest : public testing::Test {
           capturers[j] = nullptr;
           succeeded_capturers++;
         }
+        // The following else if statement is for debugging purpose only, which
+        // should be removed after flaky of ScreenCapturerIntegrationTest has
+        // been resolved.
+        else if (i == wait_capture_round - 1) {
+          std::string result;
+          rtc::Base64::EncodeFromArray(frame->data(),
+                                       frame->size().height() * frame->stride(),
+                                       &result);
+          LOG(LS_INFO) << frame->size().width() << " x "
+                       << frame->size().height();
+          // Split the entire string (can be over 4M) into several lines to

[kjellander_webrtc, 2016/11/18 08:47:23]

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.

[Hzj_jie, 2016/11/18 22:07:25]

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?

[kjellander_webrtc, 2016/11/21 06:58:12]

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.
Since it's only printed on failures, it's OK. Let's see how it goes.

+          // avoid browser from stucking.
+          static const size_t kLineLength = 32768;
+          const char* result_end = result.c_str() + result.length();
+          for (const char* it = result.c_str();
+               it < result_end;
+               it += kLineLength) {
+            const size_t max_length = result_end - it;
+            LOG(LS_INFO) << std::string(it, std::min(kLineLength, max_length));
+          }
+          ASSERT_TRUE(false) << "ScreenCapturerIntegrationTest is flaky. "

[kjellander_webrtc, 2016/11/18 08:47:23]

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).

[Hzj_jie, 2016/11/18 22:07:25]

Updated.

+                                "Please disable *all* tests in "
+                                "screen_capturer_integration_test.cc, and "
+                                "kindly FYI the broken link to "
+                                "zijiehe@chromium.org";
+        }
       }
 
       if (succeeded_capturers == capturers.size()) {